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

[Michal Privoznik]

On 24.12.2013 19:07, 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'.
> 
> 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>
> ---
> 
> The new test cases succeed with this patch. The second test fails
> without the changes to src/util/virpci.c.
> 
> ---
>  src/util/virpci.c                        |   41 +++++++++++++++++++++++++++--
>  tests/virpcimock.c                       |   33 ++++++++++++++++++++++++
>  tests/virpcitest.c                       |   34 ++++++++++++++++++++++++
>  tests/virpcitestdata/0001:00:00.0.config |  Bin 0 -> 4096 bytes
>  tests/virpcitestdata/0001:01:00.0.config |  Bin 0 -> 4096 bytes
>  tests/virpcitestdata/0001:01:00.1.config |  Bin 0 -> 4096 bytes
>  tests/virpcitestdata/0005:80:00.0.config |  Bin 0 -> 4096 bytes
>  tests/virpcitestdata/0005:90:01.0.config |  Bin 0 -> 256 bytes
>  tests/virpcitestdata/0005:90:01.1.config |  Bin 0 -> 256 bytes
>  tests/virpcitestdata/0005:90:01.2.config |  Bin 0 -> 256 bytes
>  10 files changed, 105 insertions(+), 3 deletions(-)
>  create mode 100644 tests/virpcitestdata/0001:00:00.0.config
>  create mode 100644 tests/virpcitestdata/0001:01:00.0.config
>  create mode 100644 tests/virpcitestdata/0001:01:00.1.config
>  create mode 100644 tests/virpcitestdata/0005:80:00.0.config
>  create mode 100644 tests/virpcitestdata/0005:90:01.0.config
>  create mode 100644 tests/virpcitestdata/0005:90:01.1.config
>  create mode 100644 tests/virpcitestdata/0005:90:01.2.config
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 8ec642f..8c10a93 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -344,6 +344,37 @@ 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;
> +    else
> +        VIR_WARN("Unusual value in " PCI_SYSFS "devices/%s/class: %s",
> +                 dev->name, id_str);
> +
> +    VIR_FREE(id_str);
> +    return ret;
> +}
> +

I've changed this function slightly to match the pattern used in the
rest of our code.

> +static int
>  virPCIDeviceWrite(virPCIDevicePtr dev,
>                    int cfgfd,
>                    unsigned int pos,
> @@ -645,8 +676,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)

s/== -1/< 0/

>          goto cleanup;
>  
>      /* Is it a plane? */
> @@ -2110,6 +2141,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 +2151,11 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
>          goto cleanup;
>      }
>  
> +    if (virPCIDeviceReadClass(dev, &device_class))
> +        goto cleanup;
> +

'if (virPCIDevicReadClass(dev,&device_class) < 0) ' looks nicer.

>      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);

> diff --git a/tests/virpcitestdata/0001:00:00.0.config b/tests/virpcitestdata/0001:00:00.0.config
> new file mode 100644
> index 0000000000000000000000000000000000000000..808d48993cfc0f41223fcb5f49deffc594f136b7
> GIT binary patch
> literal 4096
> zcmeH at I}XAy5Jca`5~RZg60MJb!~ys;a0<jx5^hCDNy!mt=t)n(8Yfir2x&(4YG(bB
> z{ig90wibyn0^=hytn<`78f&|D=zEvd5U8+Sxa1hw*nI*%I6fEDtkd58IUH=(- at Ei&
> z`ONZ@#r(MD|GagrnWu5_32tAW*RPg6sv;l)A|L`HAOa#F0wN#+A|L`H at J9q*PZkdc
> 
> literal 0
> HcmV?d00001
> 

Some binary garbage ...

Fixed, ACKed and pushed. Congratulations on your first libvirt patch!

Michal