Re: [PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()

Previously there was no way to differentiate between this function 1) encountering an error while reading the pci config, and 2) determining that the device in question is a conventional PCI device, and so has no Express Capabilities. The difference between these two conditions is important, because an unprivileged libvirtd will be unable to read all of the pci config (it can only read the first 64 bytes, and will get ENOENT when it tries to seek past that limit) even though the device is in fact a PCIe device. This patch just changes virPCIDeviceFindCapabilityOffset() to put the determined offset into an argument of the function (rather than sending it back as the return value), and to return the standard "0 on success, -1 on failure". Failure is determined by checking the value of errno after each attemptd read of the config file (which can only work reliably if errno is reset to 0 before each read, and after virPCIDeviceFindCapabilityOffset() has finished examining it). (NB: if the config file is read successfully, but no Express Capabilities are found, then the function returns success, but the returned offset will be 0 (which is an impossible offset for Express Capabilities, and so easily recognizeable). An upcoming patch will take advantage of the change made here. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/util/virpci.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 9109fb4f3a..2f99bb93bd 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev, unsigned int buflen) { memset(buf, 0, buflen); + errno = 0; if (lseek(cfgfd, pos, SEEK_SET) != pos || saferead(cfgfd, buf, buflen) != buflen) { @@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, return ret; }

-static uint8_t +static int virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev, int cfgfd, - unsigned int capability) + unsigned int capability, + unsigned int *offset) { uint16_t status; uint8_t pos; + *offset = 0; /* assume failure (*nothing* can be at offset 0) */ + status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS); - if (!(status & PCI_STATUS_CAP_LIST)) - return 0; + if (errno != 0 || !(status & PCI_STATUS_CAP_LIST)) + goto error; pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST); + if (errno != 0) + goto error; /* Zero indicates last capability, capabilities can't * be in the config space header and 0xff is returned @@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev, */ while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) { uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos); + if (errno != 0) + goto error; + if (capid == capability) { VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x", dev->id, dev->name, capability, pos); - return pos; + *offset = pos; + return 0; } pos = virPCIDeviceRead8(dev, cfgfd, pos + 1); + if (errno != 0) + goto error; } - VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name, capability); + error: + VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)", + dev->id, dev->name, capability, g_strerror(errno)); - return 0; + /* reset errno in case the failure was due to insufficient + * privileges to read the entire PCI config file + */ + errno = 0; + + return -1; } static unsigned int @@ -553,7 +572,7 @@ static bool virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd) { uint32_t caps; - uint8_t pos; + unsigned int pos; g_autofree char *path = NULL; int found; @@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd) * the same thing, except for conventional PCI * devices. This is not common yet. */ - pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF); + virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos); + if (pos) { caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP); if (caps & PCI_AF_CAP_FLR) { @@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd) static int virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd) { - dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP); - dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM); + virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP, &dev->pcie_cap_pos); + virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM, &dev->pci_pm_cap_pos); dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd); dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd); -- 2.28.0