This function replaces virHostdevReattachPCIDevice() and handles several PCI devices instead of requiring to be called once for every device. The handling of active and inactive devices is updated and made more explicit, which means virHostdevReAttachPCIDevices() has to be updated as well. --- src/util/virhostdev.c | 170 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 103 insertions(+), 67 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f31ad41..0f258a5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -526,6 +526,73 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, return ret; } +/** + * reattachPCIDevices: + * @mgr: hostdev manager + * @pcidevs: PCI devices to be reattached + * @skipUnmanaged: whether to skip unmanaged devices + * + * Reattach PCI devices to the host. + * + * All devices in @pcidevs must have already been marked as inactive, and + * the PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) + * must have been locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +reattachPCIDevices(virHostdevManagerPtr mgr, + virPCIDeviceListPtr pcidevs, + bool skipUnmanaged) +{ + size_t i; + int ret = -1; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr tmp = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr dev; + + /* Retrieve the actual device from the inactive list */ + if (!(dev = virPCIDeviceListFind(mgr->inactivePCIHostdevs, tmp))) { + VIR_DEBUG("PCI device %s is not marked as inactive", + virPCIDeviceGetName(tmp)); + goto out; + } + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(dev) && skipUnmanaged) { + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); + continue; + } + + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + } + + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); + if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to reattach PCI device: %s"), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + } + + ret = 0; + + out: + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -753,45 +820,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, return ret; } -/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) -{ - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ - if (!virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) - virPCIDeviceFree(dev); - return; - } - - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - err ? err->message : _("unknown error")); - virResetError(err); - } - virPCIDeviceFree(dev); -} - /* @oldStateDir: * For upgrade purpose: see virHostdevNetConfigRestore */ @@ -803,7 +831,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, int nhostdevs, const char *oldStateDir) { - virPCIDeviceListPtr pcidevs; + virPCIDeviceListPtr pcidevs = NULL; size_t i; if (!nhostdevs) @@ -822,16 +850,11 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - /* Loop through the assigned devices 4 times: 1) delete them all from - * activePCIHostdevs, 2) restore network config of SRIOV netdevs, 3) Do a - * PCI reset on each device, 4) reattach the devices to their host drivers - * (managed) or add them to inactivePCIHostdevs (!managed). - */ + /* Reattaching devices to the host involves several steps; each of them + * is described at length below */ - /* - * Loop 1: verify that each device in the hostdevs list really was in use - * by this domain, and remove them all from the activePCIHostdevs list. - */ + /* Step 1: verify that each device in the hostdevs list really was in use + * by this domain */ i = 0; while (i < virPCIDeviceListCount(pcidevs)) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -848,21 +871,32 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, continue; } } + i++; + } + + /* Step 2: move all devices from the active list to the inactive list */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr tmp = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr dev; VIR_DEBUG("Removing PCI device %s from active list", + virPCIDeviceGetName(tmp)); + if (!(dev = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, + tmp))) + goto cleanup; + + VIR_DEBUG("Adding PCI device %s to inactive list", virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev); - i++; + if (virPCIDeviceListAdd(hostdev_mgr->inactivePCIHostdevs, + dev) < 0) + goto cleanup; } - /* At this point, any device that had been used by the guest is in - * pcidevs, but has been removed from activePCIHostdevs. - */ + /* At this point, devices are no longer marked as active and have been + * marked as inactive instead */ - /* - * Loop 2: restore original network config of hostdevs that used - * <interface type='hostdev'> - */ + /* Step 3: restore original network config of hostdevs that used + * <interface type='hostdev'> */ for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; @@ -883,7 +917,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 3: perform a PCI Reset on all devices */ + /* Step 4: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -897,16 +931,18 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 4: reattach devices to their host drivers (if managed) or place - * them on the inactive list (if not managed) - */ - while (virPCIDeviceListCount(pcidevs) > 0) { - virPCIDevicePtr dev = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(dev, hostdev_mgr); - } + /* Step 5: reattach managed devices to their host drivers; unmanaged + * devices need no further processing. reattachPCIDevices() will + * remove devices from the inactive list as they are reattached + * to the host */ + ignore_value(reattachPCIDevices(hostdev_mgr, pcidevs, true)); + + /* At this point, all devices are either marked as inactive (if they + * were unmanaged), or have been reattached to the host driver (if they + * were managed) and are no longer tracked by any of our device lists */ - virObjectUnref(pcidevs); cleanup: + virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); } -- 2.5.0

On Fri, 2016-01-22 at 15:00 +0000, Daniel P. Berrange wrote: When I started splitting off this code (which, as explained in the cover letter, is something I'm doing in preparation of an upcoming series) I planned to use the device list for more than just iterating through its members. Turns out that I won't need to do that after all, so having the loop in the caller makes more sense. I'll change it. I concede that I could have done a better job at isolating independent changes, I'll try to improve with v2 :) In that regard, would you rather have the comments dealt with in a separate commit, even if that would mean have them not reflect the code mid-series? Cheers. --  Andrea Bolognani Software Engineer - Virtualization Team

--- src/util/virhostdev.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 0f258a5..267aa55 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -795,23 +795,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir, NULL); reattachdevs: - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - - if (virPCIDeviceGetManaged(dev)) { - /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ - VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(dev)); - ignore_value(virPCIDeviceReattach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs)); - } else { - VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); - } - } + ignore_value(reattachPCIDevices(hostdev_mgr, pcidevs, true)); cleanup: virObjectUnlock(hostdev_mgr->activePCIHostdevs); -- 2.5.0

On Tue, Jan 19, 2016 at 04:36:05PM +0100, Andrea Bolognani wrote: This is rather crazy - if we need to reattach single PCI devices we should have an API for doing that, and have the list API call the individual API multiple times. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Tue, 2016-01-19 at 16:36 +0100, Andrea Bolognani wrote: s/managed // Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2016-01-22 at 15:02 +0000, Daniel P. Berrange wrote: I wanted to avoid having   virHostdevReattachPCIDevice()   virHostdevPCINodeDeviceReAttach()   virHostdevReAttachPCIDevices() and I couldn't come up with a better name myself. Any ideas? :) Cheers. --  Andrea Bolognani Software Engineer - Virtualization Team

This ensures the behavior for detach is consistent, no matter how it was triggered (eg. 'virsh nodedev-detach', 'virsh attach-device' or startup of a domain that is configured to use hostdevs). --- src/util/virhostdev.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 2d219dd..bc7dd77 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1630,6 +1630,7 @@ int virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { + virPCIDeviceListPtr pcidevs = NULL; struct virHostdevIsPCINodeDeviceUsedData data = { hostdev_mgr, NULL, false }; int ret = -1; @@ -1638,17 +1639,37 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->inactivePCIHostdevs); if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) - goto out; + goto cleanup; - if (virPCIDeviceDetach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) { - goto out; + /* We want this function to be idempotent, so if the device has already + * been added to the inactive list (and is not active, as per the check + * above) just return right away */ + if (virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, pci)) { + VIR_DEBUG("PCI device %s is already detached from the host", + virPCIDeviceGetName(pci)); + ret = 0; + goto cleanup; } + /* Create a temporary device list */ + if (!(pcidevs = virPCIDeviceListNew())) + goto cleanup; + if (virPCIDeviceListAddCopy(pcidevs, pci) < 0) + goto cleanup; + + /* Detach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be detached from the host driver */ + if (detachPCIDevices(hostdev_mgr, pcidevs, false) < 0) + goto cleanup; + ret = 0; - out: + + cleanup: + virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); + return ret; } -- 2.5.0

Unbinding a PCI device from the stub driver can require several steps, and it can be useful for debugging to be able to trace which of these steps are performed and which are skipped for each device. --- src/util/virpci.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index f56dbe6..51a3f88 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1106,26 +1106,37 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) if (!driver) { /* The device is not bound to any driver and we are almost done. */ + VIR_DEBUG("PCI device %s is not bound to any driver", dev->name); goto reprobe; } - if (!dev->unbind_from_stub) + if (!dev->unbind_from_stub) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name); goto remove_slot; + } /* If the device isn't bound to a known stub, skip the unbind. */ if (virPCIStubDriverTypeFromString(driver) < 0 || - virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) + virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s because of " + "unknown stub driver", dev->name); goto remove_slot; + } - VIR_DEBUG("Found stub driver %s", driver); + VIR_DEBUG("Found stub driver %s for PCI device %s", driver, dev->name); + VIR_DEBUG("Unbinding PCI device %s", dev->name); if (virPCIDeviceUnbind(dev) < 0) goto cleanup; dev->unbind_from_stub = false; remove_slot: - if (!dev->remove_slot) + if (!dev->remove_slot) { + VIR_DEBUG("Slot removal skipped for PCI device %s", dev->name); goto reprobe; + } + + VIR_DEBUG("Removing slot for PCI device %s", dev->name); /* Xen's pciback.ko wants you to use remove_slot on the specific device */ if (!(path = virPCIDriverFile(driver, "remove_slot"))) @@ -1141,10 +1152,13 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) reprobe: if (!dev->reprobe) { + VIR_DEBUG("Reprobe skipped for PCI device %s", dev->name); result = 0; goto cleanup; } + VIR_DEBUG("Reprobing for PCI device %s", dev->name); + /* Trigger a re-probe of the device is not in the stub's dynamic * ID table. If the stub is available, but 'remove_id' isn't * available, then re-probing would just cause the device to be -- 2.5.0