https://bugzilla.redhat.com/show_bug.cgi?id=1046919 When a PCI device is not bound to any driver, reattach should just trigger driver probe rather than failing with Invalid device 0000:00:19.0 driver file /sys/bus/pci/devices/0000:00:19.0/driver is not a symlink While virPCIDeviceGetDriverPathAndName was documented to return success and NULL driver and path when a device is not attached to any driver but didn't do so. Thus callers could not distinguish unbound devices from failures. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/util/virpci.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 51dbee6..8577fd4 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -236,6 +236,11 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) if (virPCIFile(&drvlink, dev->name, "driver") < 0) goto cleanup; + if (!virFileExists(drvlink)) { + ret = 0; + goto cleanup; + } + if (virFileIsLink(drvlink) != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid device %s driver file %s is not a symlink"), @@ -1023,6 +1028,11 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) if (virPCIDeviceGetDriverPathAndName(dev, &drvdir, &driver) < 0) goto cleanup; + if (!driver) { + /* The device is not bound to any driver and we are almost done. */ + goto reprobe; + } + if (!dev->unbind_from_stub) goto remove_slot; @@ -1079,11 +1089,10 @@ reprobe: * available, then re-probing would just cause the device to be * re-bound to the stub. */ - if (virPCIDriverFile(&path, driver, "remove_id") < 0) { + if (driver && virPCIDriverFile(&path, driver, "remove_id") < 0) goto cleanup; - } - if (!virFileExists(drvdir) || virFileExists(path)) { + if (!driver || !virFileExists(drvdir) || virFileExists(path)) { if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) { virReportSystemError(errno, _("Failed to trigger a re-probe for PCI device '%s'"), -- 1.8.5.3

On Fri, Jan 17, 2014 at 15:29:27 +0100, Michal Privoznik wrote: Yeah, this whole code is quite strange... if there is remove_id file in the driver's directory, then the device's ID has already been written there as part of attaching a device to a stub driver at the time the device was detached from host system (see virPCIDeviceBindToStub). So this code is just checking if detach could have done that and if so, triggers driver probe. Note that when we get this, the device is already unbound from stub. No idea why it is done this way but I'd rather keep it as is at least in this series. Jirka

https://bugzilla.redhat.com/show_bug.cgi?id=1046919 Since commit v0.9.0-47-g4e8969e (released in 0.9.1) some failures during device detach were reported to callers of virPCIDeviceBindToStub as success. For example, even though a device seemed to be detached virsh # nodedev-detach pci_0000_07_05_0 --driver vfio Device pci_0000_07_05_0 detached one could find similar message in libvirt logs: Failed to bind PCI device '0000:07:05.0' to vfio-pci: No such device This patch fixes these paths and also avoids overwriting real errors with errors encountered during a cleanup phase. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/util/virpci.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 8577fd4..f6ab794 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1123,14 +1123,15 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, { int result = -1; int reprobe = false; - char *stubDriverPath = NULL; char *driverLink = NULL; char *path = NULL; /* reused for different purposes */ - const char *newDriverName = NULL; + char *newDriverName = NULL; + virErrorPtr err = NULL; if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 || - virPCIFile(&driverLink, dev->name, "driver") < 0) + virPCIFile(&driverLink, dev->name, "driver") < 0 || + VIR_STRDUP(newDriverName, stubDriverName) < 0) goto cleanup; if (virFileExists(driverLink)) { @@ -1138,7 +1139,6 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, /* The device is already bound to the correct driver */ VIR_DEBUG("Device %s is already bound to %s", dev->name, stubDriverName); - newDriverName = stubDriverName; result = 0; goto cleanup; } @@ -1170,6 +1170,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, if (virFileLinkPointsTo(driverLink, stubDriverPath)) { dev->unbind_from_stub = true; dev->remove_slot = true; + result = 0; goto remove_id; } @@ -1178,16 +1179,15 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, * PCI device happens to be IDE controller for the disk hosting * your root filesystem. */ - if (virPCIFile(&path, dev->name, "driver/unbind") < 0) { - goto cleanup; - } + if (virPCIFile(&path, dev->name, "driver/unbind") < 0) + goto remove_id; if (virFileExists(path)) { if (virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, _("Failed to unbind PCI device '%s'"), dev->name); - goto cleanup; + goto remove_id; } dev->reprobe = reprobe; } @@ -1222,7 +1222,11 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, dev->unbind_from_stub = true; } + result = 0; + remove_id: + err = virSaveLastError(); + /* If 'remove_id' exists, remove the device id from pci-stub's dynamic * ID table so that 'drivers_probe' works below. */ @@ -1233,6 +1237,7 @@ remove_id: "cannot be probed again.", dev->id, stubDriverName); } dev->reprobe = false; + result = -1; goto cleanup; } @@ -1247,24 +1252,26 @@ remove_id: "cannot be probed again.", dev->id, stubDriverName); } dev->reprobe = false; + result = -1; goto cleanup; } - newDriverName = stubDriverName; - result = 0; - cleanup: VIR_FREE(stubDriverPath); VIR_FREE(driverLink); VIR_FREE(path); - if (newDriverName && - STRNEQ_NULLABLE(dev->stubDriver, newDriverName)) { + if (result < 0) { + VIR_FREE(newDriverName); + virPCIDeviceUnbindFromStub(dev); + } else { VIR_FREE(dev->stubDriver); - result = VIR_STRDUP(dev->stubDriver, newDriverName); + dev->stubDriver = newDriverName; } - if (result < 0) - virPCIDeviceUnbindFromStub(dev); + + if (err) + virSetError(err); + virFreeError(err); return result; } -- 1.8.5.3

From: Jincheng Miao <jmiao(a)redhat.com&gt; https://bugzilla.redhat.com/show_bug.cgi?id=1046919 If none (KVM, VFIO) of the supported PCI passthrough methods is known to work on a host, it's better to fail right away with a nice error message rather than letting attachment fail with a more cryptic message such as Failed to bind PCI device '0000:07:05.0' to vfio-pci: No such device Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b964b3c..55ce6ec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10789,6 +10789,8 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, int ret = -1; virNodeDeviceDefPtr def = NULL; char *xml = NULL; + bool legacy = qemuHostdevHostSupportsPassthroughLegacy(); + bool vfio = qemuHostdevHostSupportsPassthroughVFIO(); virCheckFlags(0, -1); @@ -10811,22 +10813,34 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, goto cleanup; if (!driverName) { - /* prefer vfio */ - if (qemuHostdevHostSupportsPassthroughVFIO()) + if (vfio) { driverName = "vfio"; - else if (qemuHostdevHostSupportsPassthroughLegacy()) + } else if (legacy) { driverName = "kvm"; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("neither VFIO nor KVM device assignment is " + "currently supported on this system")); + goto cleanup; + } } - if (!driverName) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("neither VFIO nor kvm device assignment is " - "currently supported on this system")); - goto cleanup; - } else if (STREQ(driverName, "vfio")) { + if (STREQ(driverName, "vfio")) { + if (!vfio) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("VFIO device assignment is currently not " + "supported on this system")); + goto cleanup; + } if (virPCIDeviceSetStubDriver(pci, "vfio-pci") < 0) goto cleanup; } else if (STREQ(driverName, "kvm")) { + if (!legacy) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("KVM device assignment is currently not " + "supported on this system")); + goto cleanup; + } if (virPCIDeviceSetStubDriver(pci, "pci-stub") < 0) goto cleanup; } else { -- 1.8.5.3

For example: ... 5) testVirPCIDeviceIsAssignable(0005:90:01.0) ... OK 6) testVirPCIDeviceIsAssignable(0001:01:00.0) ... OK Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tests/virpcitest.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 82a173a..e96d7c0 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -237,8 +237,15 @@ mymain(void) # define DO_TEST_PCI(fnc, domain, bus, slot, function) \ do { \ struct testPCIDevData data = { domain, bus, slot, function }; \ - if (virtTestRun(#fnc, fnc, &data) < 0) \ + char *label = NULL; \ + if (virAsprintf(&label, "%s(%04x:%02x:%02x.%x)", \ + #fnc, domain, bus, slot, function) < 0) { \ ret = -1; \ + break; \ + } \ + if (virtTestRun(label, fnc, &data) < 0) \ + ret = -1; \ + VIR_FREE(label); \ } while (0) DO_TEST(testVirPCIDeviceNew); -- 1.8.5.3

Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/util/virpci.c | 71 ++++++++++++++++++++++++++++-------------------- src/util/virpci.h | 5 ++++ 3 files changed, 49 insertions(+), 29 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 91d1304..68140e6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1581,6 +1581,7 @@ virPCIDeviceCopy; virPCIDeviceDetach; virPCIDeviceFileIterate; virPCIDeviceFree; +virPCIDeviceGetDriverPathAndName; virPCIDeviceGetIOMMUGroupDev; virPCIDeviceGetIOMMUGroupList; virPCIDeviceGetManaged; @@ -1612,6 +1613,7 @@ virPCIDeviceSetReprobe; virPCIDeviceSetStubDriver; virPCIDeviceSetUnbindFromStub; virPCIDeviceSetUsedBy; +virPCIDeviceUnbind; virPCIDeviceWaitForCleanup; virPCIGetNetName; virPCIGetPhysicalFunction; diff --git a/src/util/virpci.c b/src/util/virpci.c index f6ab794..e2d222e 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -225,7 +225,7 @@ virPCIFile(char **buffer, const char *device, const char *file) * * Return 0 for success, -1 for error. */ -static int +int virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) { int ret = -1; @@ -1005,6 +1005,44 @@ recheck: return -1; } +int +virPCIDeviceUnbind(virPCIDevicePtr dev, bool reprobe) +{ + char *path = NULL; + char *drvpath = NULL; + char *driver = NULL; + int ret = -1; + + if (virPCIDeviceGetDriverPathAndName(dev, &drvpath, &driver) < 0) + goto cleanup; + + if (!driver) { + /* The device is not bound to any driver */ + ret = 0; + goto cleanup; + } + + if (virPCIFile(&path, dev->name, "driver/unbind") < 0) + goto cleanup; + + if (virFileExists(path)) { + if (virFileWriteStr(path, dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to unbind PCI device '%s' from %s"), + dev->name, driver); + goto cleanup; + } + dev->reprobe = reprobe; + } + + ret = 0; +cleanup: + VIR_FREE(path); + VIR_FREE(drvpath); + VIR_FREE(driver); + return ret; +} + static const char *virPCIKnownStubs[] = { "pciback", /* used by xen */ "pci-stub", /* used by kvm legacy passthrough */ @@ -1047,18 +1085,8 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) if (!isStub) goto remove_slot; - if (virFileExists(drvdir)) { - if (virPCIDriverFile(&path, driver, "unbind") < 0) { - goto cleanup; - } - - if (virFileWriteStr(path, dev->name, 0) < 0) { - virReportSystemError(errno, - _("Failed to unbind PCI device '%s' from %s"), - dev->name, driver); - goto cleanup; - } - } + if (virPCIDeviceUnbind(dev, dev->reprobe) < 0) + goto cleanup; dev->unbind_from_stub = false; remove_slot: @@ -1174,24 +1202,9 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, goto remove_id; } - /* If the device is already bound to a driver, unbind it. - * Note, this will have rather unpleasant side effects if this - * PCI device happens to be IDE controller for the disk hosting - * your root filesystem. - */ - if (virPCIFile(&path, dev->name, "driver/unbind") < 0) + if (virPCIDeviceUnbind(dev, reprobe) < 0) goto remove_id; - if (virFileExists(path)) { - if (virFileWriteStr(path, dev->name, 0) < 0) { - virReportSystemError(errno, - _("Failed to unbind PCI device '%s'"), - dev->name); - goto remove_id; - } - dev->reprobe = reprobe; - } - /* If the device isn't already bound to pci-stub, try binding it now. */ if (!virFileLinkPointsTo(driverLink, stubDriverPath)) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 08bf4c3..42c3c95 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -167,4 +167,9 @@ int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, char **pfname, int *vf_index); +int virPCIDeviceUnbind(virPCIDevicePtr dev, bool reprobe); +int virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, + char **path, + char **name); + #endif /* __VIR_PCI_H__ */ -- 1.8.5.3

This file is used by PCI detach and reattach APIs to probe for a driver that handles a specific device. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tests/virpcimock.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 49759b0..bf56143 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -91,6 +91,10 @@ char *fakesysfsdir; * Unbind driver from the device. * Data in format "DDDD:BB:DD.F" (Domain:Bus:Device.Function). * + * /sys/bus/pci/drivers_probe + * Probe for a driver that handles the specified device. + * Data in format "DDDD:BB:DD.F" (Domain:Bus:Device.Function). + * * As a little hack, we are not mocking write to these files, but close() * instead. The advantage is we don't need any self growing array to hold the * partial writes and construct them back. We can let all the writes finish, @@ -568,27 +572,39 @@ cleanup: } static int +pci_driver_handle_drivers_probe(const char *path) +{ + struct pciDevice *dev; + + if (!(dev = pci_device_find_by_content(path))) { + errno = ENODEV; + return -1; + } + + if (dev->driver) + return 0; + + return pci_device_autobind(dev); +} + +static int pci_driver_handle_change(int fd ATTRIBUTE_UNUSED, const char *path) { int ret; const char *file = last_component(path); - if (STREQ(file, "bind")) { - /* handle write to bind */ + if (STREQ(file, "bind")) ret = pci_driver_handle_bind(path); - } else if (STREQ(file, "unbind")) { - /* handle write to unbind */ + else if (STREQ(file, "unbind")) ret = pci_driver_handle_unbind(path); - } else if (STREQ(file, "new_id")) { - /* handle write to new_id */ + else if (STREQ(file, "new_id")) ret = pci_driver_handle_new_id(path); - } else if (STREQ(file, "remove_id")) { - /* handle write to remove_id */ + else if (STREQ(file, "remove_id")) ret = pci_driver_handle_remove_id(path); - } else { - /* yet not handled write */ + else if (STREQ(file, "drivers_probe")) + ret = pci_driver_handle_drivers_probe(path); + else ABORT("Not handled write to: %s", path); - } return ret; } @@ -766,6 +782,8 @@ init_env(void) if (!(fakesysfsdir = getenv("LIBVIRT_FAKE_SYSFS_DIR"))) ABORT("Missing LIBVIRT_FAKE_SYSFS_DIR env variable\n"); + make_file(fakesysfsdir, "drivers_probe", NULL, -1); + # define MAKE_PCI_DRIVER(name, ...) \ pci_driver_new(name, __VA_ARGS__, -1, -1) -- 1.8.5.3

Especially for devices that are not bound to any driver. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tests/virpcimock.c | 3 + tests/virpcitest.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 159 insertions(+), 1 deletion(-) diff --git a/tests/virpcimock.c b/tests/virpcimock.c index bf56143..f8ea9c7 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -809,6 +809,9 @@ init_env(void) 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); + MAKE_PCI_DEVICE("0000:0a:01.0", 0x8086, 0x0047); + MAKE_PCI_DEVICE("0000:0a:02.0", 0x8286, 0x0048); + MAKE_PCI_DEVICE("0000:0a:03.0", 0x8386, 0x0048); } diff --git a/tests/virpcitest.c b/tests/virpcitest.c index e96d7c0..848014d 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -34,6 +34,30 @@ # define VIR_FROM_THIS VIR_FROM_NONE static int +testVirPCIDeviceCheckDriver(virPCIDevicePtr dev, const char *expected) +{ + char *path = NULL; + char *driver = NULL; + int ret = -1; + + if (virPCIDeviceGetDriverPathAndName(dev, &path, &driver) < 0) + goto cleanup; + + if (STRNEQ_NULLABLE(driver, expected)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "PCI device %s driver mismatch: %s, expecting %s", + virPCIDeviceGetName(dev), driver, expected); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(path); + VIR_FREE(driver); + return ret; +} + +static int testVirPCIDeviceNew(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; @@ -89,6 +113,9 @@ testVirPCIDeviceDetach(const void *oaque ATTRIBUTE_UNUSED) if (virPCIDeviceDetach(dev[i], activeDevs, inactiveDevs) < 0) goto cleanup; + if (testVirPCIDeviceCheckDriver(dev[i], "pci-stub") < 0) + goto cleanup; + CHECK_LIST_COUNT(activeDevs, 0); CHECK_LIST_COUNT(inactiveDevs, i + 1); } @@ -188,6 +215,7 @@ struct testPCIDevData { unsigned int bus; unsigned int slot; unsigned int function; + const char *driver; }; static int @@ -208,6 +236,88 @@ cleanup: return ret; } +static int +testVirPCIDeviceDetachSingle(const void *opaque) +{ + const struct testPCIDevData *data = opaque; + int ret = -1; + virPCIDevicePtr dev; + + dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + if (!dev) + goto cleanup; + + if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0 || + virPCIDeviceDetach(dev, NULL, NULL) < 0) + goto cleanup; + + ret = 0; +cleanup: + virPCIDeviceFree(dev); + return ret; +} + +static int +testVirPCIDeviceReattachSingle(const void *opaque) +{ + const struct testPCIDevData *data = opaque; + int ret = -1; + virPCIDevicePtr dev; + + dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + if (!dev) + goto cleanup; + + virPCIDeviceReattachInit(dev); + if (virPCIDeviceReattach(dev, NULL, NULL) < 0) + goto cleanup; + + ret = 0; +cleanup: + virPCIDeviceFree(dev); + return ret; +} + +static int +testVirPCIDeviceCheckDriverTest(const void *opaque) +{ + const struct testPCIDevData *data = opaque; + int ret = -1; + virPCIDevicePtr dev; + + dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + if (!dev) + goto cleanup; + + if (testVirPCIDeviceCheckDriver(dev, data->driver) < 0) + goto cleanup; + + ret = 0; +cleanup: + virPCIDeviceFree(dev); + return ret; +} + +static int +testVirPCIDeviceUnbind(const void *opaque) +{ + const struct testPCIDevData *data = opaque; + int ret = -1; + virPCIDevicePtr dev; + + dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + if (!dev) + goto cleanup; + + if (virPCIDeviceUnbind(dev, false) < 0) + goto cleanup; + + ret = 0; +cleanup: + virPCIDeviceFree(dev); + return ret; +} + # define FAKESYSFSDIRTEMPLATE abs_builddir "/fakesysfsdir-XXXXXX" static int @@ -236,7 +346,9 @@ mymain(void) # define DO_TEST_PCI(fnc, domain, bus, slot, function) \ do { \ - struct testPCIDevData data = { domain, bus, slot, function }; \ + struct testPCIDevData data = { \ + domain, bus, slot, function, NULL \ + }; \ char *label = NULL; \ if (virAsprintf(&label, "%s(%04x:%02x:%02x.%x)", \ #fnc, domain, bus, slot, function) < 0) { \ @@ -248,6 +360,28 @@ mymain(void) VIR_FREE(label); \ } while (0) +# define DO_TEST_PCI_DRIVER(domain, bus, slot, function, driver) \ + do { \ + struct testPCIDevData data = { \ + domain, bus, slot, function, driver \ + }; \ + char *label = NULL; \ + if (virAsprintf(&label, "PCI driver %04x:%02x:%02x.%x is %s", \ + domain, bus, slot, function, \ + NULLSTR(driver)) < 0) { \ + ret = -1; \ + break; \ + } \ + if (virtTestRun(label, testVirPCIDeviceCheckDriverTest, \ + &data) < 0) \ + ret = -1; \ + VIR_FREE(label); \ + } while (0) + + /* Changes made to individual devices are persistent and the + * tests often rely on the state set by previous tests. + */ + DO_TEST(testVirPCIDeviceNew); DO_TEST(testVirPCIDeviceDetach); DO_TEST(testVirPCIDeviceReset); @@ -255,6 +389,27 @@ mymain(void) DO_TEST_PCI(testVirPCIDeviceIsAssignable, 5, 0x90, 1, 0); DO_TEST_PCI(testVirPCIDeviceIsAssignable, 1, 1, 0, 0); + /* Reattach a device already bound to non-stub a driver */ + DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, "i915"); + DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 1, 0); + DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, "i915"); + + /* Reattach an unbound device */ + DO_TEST_PCI(testVirPCIDeviceUnbind, 0, 0x0a, 1, 0); + DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, NULL); + DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 1, 0); + DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, "i915"); + + /* Detach an unbound device */ + DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, NULL); + DO_TEST_PCI(testVirPCIDeviceDetachSingle, 0, 0x0a, 2, 0); + DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, "pci-stub"); + + /* Reattach an unknown unbound device */ + DO_TEST_PCI_DRIVER(0, 0x0a, 3, 0, NULL); + DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 3, 0); + DO_TEST_PCI_DRIVER(0, 0x0a, 3, 0, NULL); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakesysfsdir); -- 1.8.5.3

Such driver can be used to make sure PCI APIs fail properly. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tests/virpcimock.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/tests/virpcimock.c b/tests/virpcimock.c index f8ea9c7..b514619 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -107,11 +107,19 @@ char *fakesysfsdir; * */ +enum driverActions { + PCI_ACTION_BIND = 1 << 0, + PCI_ACTION_UNBIND = 1 << 1, + PCI_ACTION_NEW_ID = 1 << 2, + PCI_ACTION_REMOVE_ID = 1 << 3, +}; + struct pciDriver { char *name; int *vendor; /* List of vendor:device IDs the driver can handle */ int *device; size_t len; /* @len is used for both @vendor and @device */ + int fail; /* Actions on this driver which should fail? */ }; struct pciDevice { @@ -143,7 +151,7 @@ static void pci_device_new_from_stub(const struct pciDevice *data); static struct pciDevice *pci_device_find_by_id(const char *id); static struct pciDevice *pci_device_find_by_content(const char *path); -static void pci_driver_new(const char *name, ...); +static void pci_driver_new(const char *name, int fail, ...); static struct pciDriver *pci_driver_find_by_dev(struct pciDevice *dev); static struct pciDriver *pci_driver_find_by_path(const char *path); static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev); @@ -415,7 +423,7 @@ pci_device_autobind(struct pciDevice *dev) * PCI Driver functions */ static void -pci_driver_new(const char *name, ...) +pci_driver_new(const char *name, int fail, ...) { struct pciDriver *driver; va_list args; @@ -427,10 +435,12 @@ pci_driver_new(const char *name, ...) virAsprintfQuiet(&driverpath, "%s/drivers/%s", fakesysfsdir, name) < 0) ABORT_OOM(); + driver->fail = fail; + if (virFileMakePath(driverpath) < 0) ABORT("Unable to create: %s", driverpath); - va_start(args, name); + va_start(args, fail); while ((vendor = va_arg(args, int)) != -1) { if ((device = va_arg(args, int)) == -1) @@ -497,8 +507,8 @@ pci_driver_bind(struct pciDriver *driver, int ret = -1; char *devpath = NULL, *driverpath = NULL; - if (dev->driver) { - /* Device already bound */ + if (dev->driver || PCI_ACTION_BIND & driver->fail) { + /* Device already bound or failing driver requested */ errno = ENODEV; return ret; } @@ -544,8 +554,8 @@ pci_driver_unbind(struct pciDriver *driver, int ret = -1; char *devpath = NULL, *driverpath = NULL; - if (dev->driver != driver) { - /* Device not bound to the @driver */ + if (dev->driver != driver || PCI_ACTION_UNBIND & driver->fail) { + /* Device not bound to the @driver or failing driver used */ errno = ENODEV; return ret; } @@ -651,8 +661,7 @@ pci_driver_handle_new_id(const char *path) int vendor, device; char buf[32]; - if (!driver) { - /* This should never happen (TM) */ + if (!driver || PCI_ACTION_NEW_ID & driver->fail) { errno = ENODEV; goto cleanup; } @@ -707,8 +716,7 @@ pci_driver_handle_remove_id(const char *path) int vendor, device; char buf[32]; - if (!driver) { - /* This should never happen (TM) */ + if (!driver || PCI_ACTION_REMOVE_ID & driver->fail) { errno = ENODEV; goto cleanup; } @@ -785,11 +793,12 @@ init_env(void) make_file(fakesysfsdir, "drivers_probe", NULL, -1); # define MAKE_PCI_DRIVER(name, ...) \ - pci_driver_new(name, __VA_ARGS__, -1, -1) + pci_driver_new(name, 0, __VA_ARGS__, -1, -1) MAKE_PCI_DRIVER("iwlwifi", 0x8086, 0x0044); MAKE_PCI_DRIVER("i915", 0x8086, 0x0046, 0x8086, 0x0047); MAKE_PCI_DRIVER("pci-stub", -1, -1); + pci_driver_new("vfio-pci", PCI_ACTION_BIND, -1, -1); # define MAKE_PCI_DEVICE(Id, Vendor, Device, ...) \ do { \ -- 1.8.5.3

Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tests/virpcitest.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 848014d..6a2291d 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -258,6 +258,37 @@ cleanup: } static int +testVirPCIDeviceDetachFail(const void *opaque) +{ + const struct testPCIDevData *data = opaque; + int ret = -1; + virPCIDevicePtr dev; + + dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + if (!dev) + goto cleanup; + + if (virPCIDeviceSetStubDriver(dev, "vfio-pci") < 0) + goto cleanup; + + if (virPCIDeviceDetach(dev, NULL, NULL) < 0) { + if (virTestGetVerbose() || virTestGetDebug()) + virDispatchError(NULL); + virResetLastError(); + ret = 0; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Attaching device %s to %s should have failed", + virPCIDeviceGetName(dev), + virPCIDeviceGetStubDriver(dev)); + } + +cleanup: + virPCIDeviceFree(dev); + return ret; +} + +static int testVirPCIDeviceReattachSingle(const void *opaque) { const struct testPCIDevData *data = opaque; @@ -389,6 +420,8 @@ mymain(void) DO_TEST_PCI(testVirPCIDeviceIsAssignable, 5, 0x90, 1, 0); DO_TEST_PCI(testVirPCIDeviceIsAssignable, 1, 1, 0, 0); + DO_TEST_PCI(testVirPCIDeviceDetachFail, 0, 0x0a, 1, 0); + /* Reattach a device already bound to non-stub a driver */ DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, "i915"); DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 1, 0); -- 1.8.5.3