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@xxxxxxxxxxxxxxxxxx>
> ---
> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list