[libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.
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@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; +} + +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) 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; + 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/virpcimock.c b/tests/virpcimock.c index a5cef46..49759b0 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -29,6 +29,7 @@ # include <fcntl.h> # include <sys/stat.h> # include <stdarg.h> +# include <dirent.h> # include "viralloc.h" # include "virstring.h" # include "virfile.h" @@ -42,6 +43,7 @@ static int (*real__xstat)(int ver, const char *path, struct stat *sb); static char *(*realcanonicalize_file_name)(const char *path); static int (*realopen)(const char *path, int flags, ...); static int (*realclose)(int fd); +static DIR * (*realopendir)(const char *name); /* Don't make static, since it causes problems with clang * when passed as an arg to virAsprintf() @@ -112,6 +114,7 @@ struct pciDevice { char *id; int vendor; int device; + int class; struct pciDriver *driver; /* Driver attached. NULL if attached to no driver */ }; @@ -351,6 +354,10 @@ pci_device_new_from_stub(const struct pciDevice *data) ABORT("@tmp overflow"); make_file(devpath, "device", tmp, -1); + if (snprintf(tmp, sizeof(tmp), "0x%.4x", dev->class) < 0) + ABORT("@tmp overflow"); + make_file(devpath, "class", tmp, -1); + if (pci_device_autobind(dev) < 0) ABORT("Unable to bind: %s", data->id); @@ -747,6 +754,7 @@ init_syms(void) LOAD_SYM(canonicalize_file_name); LOAD_SYM(open); LOAD_SYM(close); + LOAD_SYM(opendir); } static void @@ -776,6 +784,13 @@ init_env(void) MAKE_PCI_DEVICE("0000:00:01.0", 0x8086, 0x0044); MAKE_PCI_DEVICE("0000:00:02.0", 0x8086, 0x0046); MAKE_PCI_DEVICE("0000:00:03.0", 0x8086, 0x0048); + MAKE_PCI_DEVICE("0001:00:00.0", 0x1014, 0x03b9, .class = 0x060400); + MAKE_PCI_DEVICE("0001:01:00.0", 0x8086, 0x105e); + MAKE_PCI_DEVICE("0001:01:00.1", 0x8086, 0x105e); + MAKE_PCI_DEVICE("0005:80:00.0", 0x10b5, 0x8112, .class = 0x060400); + MAKE_PCI_DEVICE("0005:90:01.0", 0x1033, 0x0035); + MAKE_PCI_DEVICE("0005:90:01.1", 0x1033, 0x0035); + MAKE_PCI_DEVICE("0005:90:01.2", 0x1033, 0x00e0); } @@ -934,6 +949,24 @@ open(const char *path, int flags, ...) return ret; } +DIR * +opendir(const char *path) +{ + DIR *ret; + char *newpath = NULL; + + init_syms(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX) && + getrealpath(&newpath, path) < 0) + return NULL; + + ret = realopendir(newpath ? newpath : path); + + VIR_FREE(newpath); + return ret; +} + int close(int fd) { diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 5fe6d49..82a173a 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -183,6 +183,31 @@ cleanup: return ret; } +struct testPCIDevData { + unsigned int domain; + unsigned int bus; + unsigned int slot; + unsigned int function; +}; + +static int +testVirPCIDeviceIsAssignable(const void *opaque) +{ + const struct testPCIDevData *data = opaque; + int ret = -1; + virPCIDevicePtr dev; + + if (!(dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function))) + goto cleanup; + + if (virPCIDeviceIsAssignable(dev, true)) + ret = 0; + + virPCIDeviceFree(dev); +cleanup: + return ret; +} + # define FAKESYSFSDIRTEMPLATE abs_builddir "/fakesysfsdir-XXXXXX" static int @@ -209,10 +234,19 @@ mymain(void) ret = -1; \ } while (0) +# define DO_TEST_PCI(fnc, domain, bus, slot, function) \ + do { \ + struct testPCIDevData data = { domain, bus, slot, function }; \ + if (virtTestRun(#fnc, fnc, &data) < 0) \ + ret = -1; \ + } while (0) + DO_TEST(testVirPCIDeviceNew); DO_TEST(testVirPCIDeviceDetach); DO_TEST(testVirPCIDeviceReset); DO_TEST(testVirPCIDeviceReattach); + DO_TEST_PCI(testVirPCIDeviceIsAssignable, 5, 0x90, 1, 0); + DO_TEST_PCI(testVirPCIDeviceIsAssignable, 1, 1, 0, 0); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakesysfsdir); 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@I}XAy5Jca`5~RZg60MJb!~ys;a0<jx5^hCDNy!mt=t)n(8Yfir2x&(4YG(bB z{ig90wibyn0^=hytn<`78f&|D=zEvd5U8+Sxa1hw*nI*%I6fEDtkd58IUH=(-@Ei& z`ONZ@#r(MD|GagrnWu5_32tAW*RPg6sv;l)A|L`HAOa#F0wN#+A|L`H@J9q*PZkdc literal 0 HcmV?d00001 diff --git a/tests/virpcitestdata/0001:01:00.0.config b/tests/virpcitestdata/0001:01:00.0.config new file mode 100644 index 0000000000000000000000000000000000000000..f63aacbea08fc83b345e1ee404756145e7d349a5 GIT binary patch literal 4096 zcmZo`h!bFD5MW?qU|>>UXkY*WAi>nY$iN6<qW}>B2WF7K379CR5||9#X~qjmCm0kM z6j>iMK<#Fl0AdIL{c(^7NNY$jIHrJ{?<fFO0H%Qc6alFM0YL>exUYeN42%s7Ec~1x z877=QWd`bav)TXue_{D2AeEzFGz3ONU^E0qLtr!nMnhmU1V%$(Gz3ONU^E0qLtr!n I24e^S0CUn1m;e9( literal 0 HcmV?d00001 diff --git a/tests/virpcitestdata/0001:01:00.1.config b/tests/virpcitestdata/0001:01:00.1.config new file mode 100644 index 0000000000000000000000000000000000000000..db9e3876a528266bb5a6e9651d7cd18b45b7d71f GIT binary patch literal 4096 zcmZo`h!bFD5MW?qU|>>UXkcJqU;z?r4T=nmKsG865pZAziJX85Gr<@GPcvRnI>Dg8 zpvd~50qPj02_PQ`0R3^02S{s3F*v4xobM<ARRE?K85l)Csz5+cfer3!pdbTd0|N^` zCrE|~GcZ8HK)?*t@n*CC|Np}BPaq<r<Y)+thQMeDjE2By2#kinXb6mkz-S1JhQMeD NjE2By2n@au000k05W)Ze literal 0 HcmV?d00001 diff --git a/tests/virpcitestdata/0005:80:00.0.config b/tests/virpcitestdata/0005:80:00.0.config new file mode 100644 index 0000000000000000000000000000000000000000..cc4f94a849ed3d0a47f5b47331aaecdea88765c3 GIT binary patch literal 4096 zcmdlgAk@gtAi%JSfrX8Mfsp|Q8YawV_`sl`#L)Di>BE1RD1>%^ae>Mi1DK*fTmcO} zuqbOn1DJ*p0t|%^rUDm(pbry}Ey&Qo0}^ro5pYlpXVI6zg5+oV+Kg)3K=~il6$8f` z_5Z+y{a~<pH2w!eEDW3*M&p0r!hSH=JR1LlAr=PC4WsctaA7|fY#xpO!4L}r=Z4Yv IADFNQ0PH*z9smFU literal 0 HcmV?d00001 diff --git a/tests/virpcitestdata/0005:90:01.0.config b/tests/virpcitestdata/0005:90:01.0.config new file mode 100644 index 0000000000000000000000000000000000000000..a60599bd342d3ebcdc7b8367ca36ad337f602fde GIT binary patch literal 256 zcmXpOFlAt45MXi^VCG@qXkY+>CJ=!Q7z5RUfCHEW5{!&mj0{Y5Fz#TaS&cX3;ByxM DCDjDB literal 0 HcmV?d00001 diff --git a/tests/virpcitestdata/0005:90:01.1.config b/tests/virpcitestdata/0005:90:01.1.config new file mode 100644 index 0000000000000000000000000000000000000000..beee76534041a7020c08ae9ac03d9a349c6ea12e GIT binary patch literal 256 ycmXpOFlAt45MXi^VCG@qU|?VnU}yr8Sb;H6EeJS(Ng%<*sKv;@R0rb@MH&E<&;s)S literal 0 HcmV?d00001 diff --git a/tests/virpcitestdata/0005:90:01.2.config b/tests/virpcitestdata/0005:90:01.2.config new file mode 100644 index 0000000000000000000000000000000000000000..cfd72e4d7165bff2751ecbdc570ce7f1e0646226 GIT binary patch literal 256 zcmXpOc)-BMAi%_;z|6zo!oa|wz|aIFu>xbDS`csmlR$!5K#7rosSd`)Mk^@TV-u#E T7_0Gy9FS#<5E~CbC<F-rZiWW} literal 0 HcmV?d00001 -- 1.7.1
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@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@I}XAy5Jca`5~RZg60MJb!~ys;a0<jx5^hCDNy!mt=t)n(8Yfir2x&(4YG(bB z{ig90wibyn0^=hytn<`78f&|D=zEvd5U8+Sxa1hw*nI*%I6fEDtkd58IUH=(-@Ei& z`ONZ@#r(MD|GagrnWu5_32tAW*RPg6sv;l)A|L`HAOa#F0wN#+A|L`H@J9q*PZkdc
literal 0 HcmV?d00001
Some binary garbage ... Fixed, ACKed and pushed. Congratulations on your first libvirt patch! Michal
On 01/07/2014 09:35 AM, Michal Privoznik wrote:
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@I}XAy5Jca`5~RZg60MJb!~ys;a0<jx5^hCDNy!mt=t)n(8Yfir2x&(4YG(bB z{ig90wibyn0^=hytn<`78f&|D=zEvd5U8+Sxa1hw*nI*%I6fEDtkd58IUH=(-@Ei& z`ONZ@#r(MD|GagrnWu5_32tAW*RPg6sv;l)A|L`HAOa#F0wN#+A|L`H@J9q*PZkdc
literal 0 HcmV?d00001
Some binary garbage ...
I know this was copied from a working sample file, as opposed to something you generated (if it were generated, I'd request that we instead version-control the human-readable source that got converted into the binary blob). As long as it doesn't trip up 'make syntax-check' or git server side hooks, I think we can live with this. Does the resulting file contain any newlines or carriage returns that might get botched on Windows platforms, if we don't add a .gitattributes listing that mentions it is explicitly a binary file? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jan 07, 2014 at 09:53:51AM -0700, Eric Blake wrote:
On 01/07/2014 09:35 AM, Michal Privoznik wrote:
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@I}XAy5Jca`5~RZg60MJb!~ys;a0<jx5^hCDNy!mt=t)n(8Yfir2x&(4YG(bB z{ig90wibyn0^=hytn<`78f&|D=zEvd5U8+Sxa1hw*nI*%I6fEDtkd58IUH=(-@Ei& z`ONZ@#r(MD|GagrnWu5_32tAW*RPg6sv;l)A|L`HAOa#F0wN#+A|L`H@J9q*PZkdc
literal 0 HcmV?d00001
Some binary garbage ...
I know this was copied from a working sample file, as opposed to something you generated (if it were generated, I'd request that we instead version-control the human-readable source that got converted into the binary blob). As long as it doesn't trip up 'make syntax-check' or git server side hooks, I think we can live with this. Does the resulting file contain any newlines or carriage returns that might get botched on Windows platforms, if we don't add a .gitattributes listing that mentions it is explicitly a binary file?
This was copied from a system where it was failing before. I also copied the config files from a working device + bridge. make syntax-check seems to work, but some of the files have a newline character. I haven't worked with git on Windows. Is that going to be a problem? Regards. Cascardo.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/07/2014 10:46 AM, Thadeu Lima de Souza Cascardo wrote:
Does the resulting file contain any newlines or carriage returns that might get botched on Windows platforms, if we don't add a .gitattributes listing that mentions it is explicitly a binary file?
make syntax-check seems to work, but some of the files have a newline character. I haven't worked with git on Windows. Is that going to be a problem?
Let's not worry about it until someone reports an actual failure where trying to use the repository in line-ending-munge mode actually breaks things for them (personally, when I use git on Windows, I intentionally use it in binary-only mode because I find line-ending-munge mode causes more problems than it cures). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Michal Privoznik -
Thadeu Lima de Souza Cascardo