On 12/9/20 4:18 AM, Daniel P. Berrangé wrote:
On Tue, Dec 08, 2020 at 08:37:43PM -0500, Laine Stump wrote: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@xxxxxxxxxx> --- 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; }I think there should be some API docs added here to describe the semantics this method is trying to achieve wrt unprivileged usage and pci vs pcie.
There is something about that in patch 6/7, but I think all of these functions could benefit from documenting their API (e.g. the fact that errno may be set on return from virPCIDeviceReadNN(), since the functions themselves directly return the value that is read, and so are unable indicate an error in the return value (I considered doing that, but wanted a less invasive change, and there are 20-30 callers of those functions, most of which wouldn't otherwise need to be changed).
I'll add in some API docublurbs and resend the entire series just for simplicity...
-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.0Regards, Daniel
