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.
> -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
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
