[libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.

[Laine Stump]

On 12/10/2013 11:57 AM, Thadeu Lima de Souza Cascardo wrote:
> When determining if a device is behind a PCI bridge, the PCI device
> class is checked by reading the config space. However, there are some
> devices which have the wrong class on the config space, but the class is
> initialized by Linux correctly as a PCI BRIDGE. This class can be read
> by the sysfs file '/sys/bus/pci/devices/xxxx:xx:xx.x/class'.

Since I'm not an expert on the PCI spec, I'll just have to take your
word for this :-)

>
> One example of such bridge is IBM PCI Bridge 1014:03b9, which is
> identified as a Host Bridge when reading the config space.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at linux.vnet.ibm.com>
> ---
>  src/util/virpci.c |   38 +++++++++++++++++++++++++++++++++++---
>  1 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 8ec642f..8353411 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -344,6 +344,34 @@ virPCIDeviceRead32(virPCIDevicePtr dev, int cfgfd, unsigned int pos)
>  }
>  
>  static int
> +virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class)
> +{
> +    char *path = NULL;
> +    char *id_str;
> +    int ret = 0;
> +    unsigned int value;
> +
> +    if (virPCIFile(&path, dev->name, "class") < 0)
> +        return -1;
> +
> +    /* class string is '0xNNNNNN\n' ... i.e. 9 bytes */
> +    if (virFileReadAll(path, 9, &id_str) < 0) {
> +        VIR_FREE(path);
> +        return -1;
> +    }
> +
> +    VIR_FREE(path);
> +
> +    id_str[8] = '\0';
> +    ret = virStrToLong_ui(id_str, NULL, 16, &value);
> +    if (ret == 0)
> +        *device_class = (value >> 8) & 0xFFFF;

If the class file for some reason doesn't contain a hexadecimal number,
this will return an error (-1) without having logged an error. This
results in one of those "An error occurred, but the cause is unknown"
messages, which is always very frustrating to debug. Even though it's
*highly* unlikely that such an error would occur, we should still have
the code to log it just in case.


> +
> +    VIR_FREE(id_str);
> +    return ret;
> +}
> +
> +static int
>  virPCIDeviceWrite(virPCIDevicePtr dev,
>                    int cfgfd,
>                    unsigned int pos,
> @@ -645,8 +673,8 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data)
>          return 0;
>  
>      /* Is it a bridge? */
> -    device_class = virPCIDeviceRead16(check, fd, PCI_CLASS_DEVICE);
> -    if (device_class != PCI_CLASS_BRIDGE_PCI)
> +    ret = virPCIDeviceReadClass(check, &device_class);
> +    if (ret == -1 || device_class != PCI_CLASS_BRIDGE_PCI)
>          goto cleanup;
>  
>      /* Is it a plane? */
> @@ -2110,6 +2138,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
>      unsigned int pos;
>      int fd;
>      int ret = 0;
> +    uint16_t device_class;
>  
>      if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
>          return -1;
> @@ -2119,8 +2148,11 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
>          goto cleanup;
>      }
>  
> +    if (virPCIDeviceReadClass(dev, &device_class))
> +        goto cleanup;
> +
>      pos = dev->pcie_cap_pos;
> -    if (!pos || virPCIDeviceRead16(dev, fd, PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
> +    if (!pos || device_class != PCI_CLASS_BRIDGE_PCI)
>          goto cleanup;
>  
>      flags = virPCIDeviceRead16(dev, fd, pos + PCI_EXP_FLAGS);

Aside from the above lack of error logging, as Michal suggested a test
case would be very useful.