[libvirt] [PATCH 0/7] PCI hostdev refactoring
This series is part of my ongoing quest to fix Bug 1272300[1]. I had previously posted a mostly complete solution[2], which did however fail to handle devices detached / reattached using virsh's nodedev-* commands. Part of that series has already been merged. This series refactors the PCI hostdev code so that all detach / reattach operations go through a single function, which means that once the previous code is reimplemented on top of it, all cases will be handled correctly. One problem with the previous code is that bookkeeping was performed all over the place, making it very easy to introduce subtle bugs. After this refactoring it's hopefully easier to follow along. Despite this being a refactoring, it introduces some changes in behaviour. These changes do, in fact, improve consistency, and should not cause any issue AFAICT, but they're still worth mentioning and thinking over. Basically, every time a device is detached from the host, its original status will be restored once it's reattached, which means that if it was not bound to any driver when 'virsh nodedev-detach' is called, it will not be bound to any driver after 'virsh nodedev-reattach' is called. This is the same thing that used to happen, and still happens, when such a managed PCI hostdev is attached and then detached from a guest. One more thing worth mentioning is that the code now relies heavily on libvirt's internal state, ie. the list of active and inactive PCI hostdevs. These lists do not currently survive daemon restarts, so the series is unfit for merge until that detail is sorted out. It's unclear to me how I could go about that, any suggestion? Cheers. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1272300 [2] https://www.redhat.com/archives/libvir-list/2015-December/msg00070.html Andrea Bolognani (7): hostdev: Add reattachPCIDevices() hostdev: Use common reattach code to rollback prepare errors hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach() hostdev: Add detachPCIDevices() hostdev: Use common detach code in virHostdevPCINodeDeviceDetach() pci: Phase out virPCIDeviceReattachInit() pci: Add debug messages when unbinding from stub driver src/libvirt_private.syms | 1 - src/util/virhostdev.c | 389 +++++++++++++++++++++++++++++------------------ src/util/virpci.c | 30 ++-- src/util/virpci.h | 1 - tests/virpcitest.c | 5 +- 5 files changed, 267 insertions(+), 159 deletions(-) -- 2.5.0
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 Tue, Jan 19, 2016 at 04:36:03PM +0100, Andrea Bolognani wrote:
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; +}
IMHO you should leave virHostdevReattachPCIDevice alone, and just make this new method call that one. In later patches you are calling this reattachPCIDevices() method with a single device, forcing you to put it into a temporary virPCIDeviceListPtr before calling it. If you keep virHostdevReattachPCIDevice then you can call it directly and avoid creating temporary lists.
+ 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);
I'm inclined to say that all the changes above this point should have been a separate commit from the commit that introduces the reattachPCIDevices method, as this is really mixing 2 sets of unrelated changes in one commit.
@@ -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); }
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 Fri, 2016-01-22 at 15:00 +0000, Daniel P. Berrange wrote:
IMHO you should leave virHostdevReattachPCIDevice alone, and just make this new method call that one. In later patches you are calling this reattachPCIDevices() method with a single device, forcing you to put it into a temporary virPCIDeviceListPtr before calling it. If you keep virHostdevReattachPCIDevice then you can call it directly and avoid creating temporary lists.
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.
@@ -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); I'm inclined to say that all the changes above this point should have been a separate commit from the commit that introduces the reattachPCIDevices method, as this is really mixing 2 sets of unrelated changes in one commit.
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
On Fri, Jan 22, 2016 at 05:39:33PM +0100, Andrea Bolognani wrote:
On Fri, 2016-01-22 at 15:00 +0000, Daniel P. Berrange wrote:
IMHO you should leave virHostdevReattachPCIDevice alone, and just make this new method call that one. In later patches you are calling this reattachPCIDevices() method with a single device, forcing you to put it into a temporary virPCIDeviceListPtr before calling it. If you keep virHostdevReattachPCIDevice then you can call it directly and avoid creating temporary lists.
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.
@@ -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); I'm inclined to say that all the changes above this point should have been a separate commit from the commit that introduces the reattachPCIDevices method, as this is really mixing 2 sets of unrelated changes in one commit.
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?
I don't mind really. 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 :|
--- 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
This ensures the behavior for reattach is consistent, no matter how it was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or shutdown of a domain that is configured to use hostdevs). --- src/util/virhostdev.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 267aa55..74c43f2 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1625,10 +1625,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, return ret; } +/** + * virHostdevPCINodeDeviceReAttach: + * @hostdev_mgr: hostdev manager + * @pci: PCI device + * + * Reattach a specific PCI device to the host. + * + * This function makes sure the device is not in use before reattaching it + * to the host; if the device has already been reattached to the host, the + * operation is considered successful. + * + * Returns: 0 on success, <0 on failure + */ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { + virPCIDeviceListPtr pcidevs = NULL; struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, false}; int ret = -1; @@ -1637,18 +1651,37 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->inactivePCIHostdevs); if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) - goto out; + goto cleanup; - virPCIDeviceReattachInit(pci); + /* We want this function to be idempotent, so if the device has already + * been removed from the inactive list (and is not active either, as + * per the check above) just return right away */ + if (!virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, pci)) { + VIR_DEBUG("PCI device %s is already attached to the host", + virPCIDeviceGetName(pci)); + ret = 0; + goto cleanup; + } - if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) - goto out; + /* Create a temporary device list */ + if (!(pcidevs = virPCIDeviceListNew())) + goto cleanup; + if (virPCIDeviceListAddCopy(pcidevs, pci) < 0) + goto cleanup; + + /* Reattach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be reattached to the host driver */ + if (reattachPCIDevices(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
On Tue, Jan 19, 2016 at 04:36:05PM +0100, Andrea Bolognani wrote:
This ensures the behavior for reattach is consistent, no matter how it was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or shutdown of a domain that is configured to use hostdevs). --- src/util/virhostdev.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 267aa55..74c43f2 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1625,10 +1625,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, return ret; }
+/** + * virHostdevPCINodeDeviceReAttach: + * @hostdev_mgr: hostdev manager + * @pci: PCI device + * + * Reattach a specific PCI device to the host. + * + * This function makes sure the device is not in use before reattaching it + * to the host; if the device has already been reattached to the host, the + * operation is considered successful. + * + * Returns: 0 on success, <0 on failure + */ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { + virPCIDeviceListPtr pcidevs = NULL; struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, false}; int ret = -1; @@ -1637,18 +1651,37 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->inactivePCIHostdevs);
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) - goto out; + goto cleanup;
- virPCIDeviceReattachInit(pci); + /* We want this function to be idempotent, so if the device has already + * been removed from the inactive list (and is not active either, as + * per the check above) just return right away */ + if (!virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, pci)) { + VIR_DEBUG("PCI device %s is already attached to the host", + virPCIDeviceGetName(pci)); + ret = 0; + goto cleanup; + }
- if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) - goto out; + /* Create a temporary device list */ + if (!(pcidevs = virPCIDeviceListNew())) + goto cleanup; + if (virPCIDeviceListAddCopy(pcidevs, pci) < 0) + goto cleanup;
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.
+ + /* Reattach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be reattached to the host driver */ + if (reattachPCIDevices(hostdev_mgr, pcidevs, false) < 0) + goto cleanup;
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 :|
This function mirrors reattachPCIDevices(). The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 125 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 49 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 74c43f2..2d219dd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -593,6 +593,56 @@ reattachPCIDevices(virHostdevManagerPtr mgr, return ret; } +/** + * detachPCIDevices: + * @mgr: hostdev manager + * @pcidevs: PCI devices to be detached + * @skipUnmanaged: whether to skip unmanaged devices + * + * Detach PCI devices from the host. + * + * The PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) + * must have been locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +detachPCIDevices(virHostdevManagerPtr mgr, + virPCIDeviceListPtr pcidevs, + bool skipUnmanaged) +{ + size_t i; + int ret = -1; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(dev) && skipUnmanaged) { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); + continue; + } + + VIR_DEBUG("Detaching managed PCI device %s", + virPCIDeviceGetName(dev)); + if (virPCIDeviceDetach(dev, + mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to detach 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, @@ -624,11 +674,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, * must be reset before being marked as active. */ - /* Loop 1: validate that non-managed device isn't in use, eg - * by checking that device is either un-bound, or bound - * to pci-stub.ko - */ + /* Detaching devices from the host involves several steps; each of them + * is described at length below */ + /* Step 1: perform safety checks, eg. ensure the devices are assignable */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); @@ -659,28 +708,15 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - - if (virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Detaching managed PCI device %s", - virPCIDeviceGetName(dev)); - if (virPCIDeviceDetach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) - goto reattachdevs; - } else { - VIR_DEBUG("Not detaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); - } - } + /* Step 2: detach managed devices (i.e. bind to appropriate stub driver). + * detachPCIDevices() will also mark devices as inactive */ + if (detachPCIDevices(hostdev_mgr, pcidevs, true) < 0) + goto reattachdevs; - /* At this point, all devices are attached to the stub driver and have + /* At this point, devices are attached to the stub driver and have * been marked as inactive */ - /* Loop 3: Now that all the PCI hostdevs have been detached, we - * can safely reset them */ + /* Step 3: perform a PCI reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -690,8 +726,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto reattachdevs; } - /* Loop 4: For SRIOV network devices, Now that we have detached the - * the network device, set the netdev config */ + /* Step 4: set the netdev config for SRIOV network devices */ for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; if (!virHostdevIsPCINetDevice(hostdev)) @@ -703,9 +738,16 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, last_processed_hostdev_vf = i; } - /* Loop 5: Now mark all the devices as active */ + /* Step 5: move all devices from the inactive list to the active list */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr tmp = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr dev; + + VIR_DEBUG("Removing PCI device %s from inactive list", + virPCIDeviceGetName(tmp)); + if (!(dev = virPCIDeviceListSteal(hostdev_mgr->inactivePCIHostdevs, + tmp))) + goto inactivedevs; VIR_DEBUG("Adding PCI device %s to active list", virPCIDeviceGetName(dev)); @@ -713,18 +755,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto inactivedevs; } - /* Loop 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - - VIR_DEBUG("Removing PCI device %s from inactive list", - virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); - } - - /* Loop 7: Now set the used_by_domain of the device in - * activePCIHostdevs as domain name. - */ + /* Step 6: set driver and domain information */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev, activeDev; @@ -737,7 +768,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name); } - /* Loop 8: Now set the original states for hostdev def */ + /* Step 7: set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr dev; virPCIDevicePtr pcidev; @@ -752,10 +783,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, pcisrc->addr.function); - /* original states "unbind_from_stub", "remove_slot", - * "reprobe" were already set by pciDettachDevice in - * loop 2. - */ + /* original states for "unbind_from_stub", "remove_slot" and + * "reprobe" (used when reattaching) were already set by + * detachPCIDevices() in a previous step */ VIR_DEBUG("Saving network configuration of PCI device %s", virPCIDeviceGetName(dev)); if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) { @@ -770,10 +800,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceFree(dev); } - /* Loop 9: Now steal all the devices from pcidevs */ - while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); - ret = 0; goto cleanup; @@ -798,9 +824,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, ignore_value(reattachPCIDevices(hostdev_mgr, pcidevs, true)); cleanup: + virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); - virObjectUnref(pcidevs); + return ret; } -- 2.5.0
On Tue, 2016-01-19 at 16:36 +0100, Andrea Bolognani wrote:
This function mirrors reattachPCIDevices().
The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 125 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 49 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 74c43f2..2d219dd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -593,6 +593,56 @@ reattachPCIDevices(virHostdevManagerPtr mgr, return ret; } +/** + * detachPCIDevices: + * @mgr: hostdev manager + * @pcidevs: PCI devices to be detached + * @skipUnmanaged: whether to skip unmanaged devices + * + * Detach PCI devices from the host. + * + * The PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) + * must have been locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +detachPCIDevices(virHostdevManagerPtr mgr, + virPCIDeviceListPtr pcidevs, + bool skipUnmanaged) +{ + size_t i; + int ret = -1; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(dev) && skipUnmanaged) { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); + continue; + } + + VIR_DEBUG("Detaching managed PCI device %s", + virPCIDeviceGetName(dev));
s/managed // Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Tue, Jan 19, 2016 at 04:36:06PM +0100, Andrea Bolognani wrote:
This function mirrors reattachPCIDevices().
The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 125 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 49 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 74c43f2..2d219dd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -593,6 +593,56 @@ reattachPCIDevices(virHostdevManagerPtr mgr, return ret; }
+/** + * detachPCIDevices:
Please keep a virHostdev prefix on this method, and also on the one added in the earlier patch. All methods should be prefixed to match the filename, even if they're static
+ * @mgr: hostdev manager + * @pcidevs: PCI devices to be detached + * @skipUnmanaged: whether to skip unmanaged devices + * + * Detach PCI devices from the host. + * + * The PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) + * must have been locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +detachPCIDevices(virHostdevManagerPtr mgr, + virPCIDeviceListPtr pcidevs, + bool skipUnmanaged) +{ + size_t i; + int ret = -1; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(dev) && skipUnmanaged) { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); + continue; + } + + VIR_DEBUG("Detaching managed PCI device %s", + virPCIDeviceGetName(dev)); + if (virPCIDeviceDetach(dev, + mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to detach PCI device: %s"), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + } + + ret = 0; + + out: + return ret; +}
Again the body of the for() loop should really be a separate method that you can call passing in a single PCI device, to avoid later code where you have to stuff a single device into a list. 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 Fri, 2016-01-22 at 15:02 +0000, Daniel P. Berrange wrote:
On Tue, Jan 19, 2016 at 04:36:06PM +0100, Andrea Bolognani wrote:
This function mirrors reattachPCIDevices(). The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 125 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 49 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 74c43f2..2d219dd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -593,6 +593,56 @@ reattachPCIDevices(virHostdevManagerPtr mgr, return ret; } +/** + * detachPCIDevices: Please keep a virHostdev prefix on this method, and also on the one added in the earlier patch. All methods should be prefixed to match the filename, even if they're static
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
On Fri, Jan 22, 2016 at 06:30:42PM +0100, Andrea Bolognani wrote:
On Fri, 2016-01-22 at 15:02 +0000, Daniel P. Berrange wrote:
On Tue, Jan 19, 2016 at 04:36:06PM +0100, Andrea Bolognani wrote:
This function mirrors reattachPCIDevices(). The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 125 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 49 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 74c43f2..2d219dd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -593,6 +593,56 @@ reattachPCIDevices(virHostdevManagerPtr mgr, return ret; } +/** + * detachPCIDevices: Please keep a virHostdev prefix on this method, and also on the one added in the earlier patch. All methods should be prefixed to match the filename, even if they're static
I wanted to avoid having
virHostdevReattachPCIDevice()
How about virHostdevReAttachPCIDeviceKMod() since this method is specifically only doing the kmod rebind.
virHostdevPCINodeDeviceReAttach() virHostdevReAttachPCIDevices()
and I couldn't come up with a better name myself. Any ideas? :)
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 :|
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
The name is confusing, and the only use left is in a test case. --- src/libvirt_private.syms | 1 - src/util/virpci.c | 8 -------- src/util/virpci.h | 1 - tests/virpcitest.c | 5 ++++- 4 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 83f6e2c..ab7b71d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1987,7 +1987,6 @@ virPCIDeviceListSteal; virPCIDeviceListStealIndex; virPCIDeviceNew; virPCIDeviceReattach; -virPCIDeviceReattachInit; virPCIDeviceReset; virPCIDeviceSetManaged; virPCIDeviceSetRemoveSlot; diff --git a/src/util/virpci.c b/src/util/virpci.c index 505c1f3..f56dbe6 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1771,14 +1771,6 @@ virPCIDeviceGetUsedBy(virPCIDevicePtr dev, *dom_name = dev->used_by_domname; } -void virPCIDeviceReattachInit(virPCIDevicePtr pci) -{ - pci->unbind_from_stub = true; - pci->remove_slot = true; - pci->reprobe = true; -} - - virPCIDeviceListPtr virPCIDeviceListNew(void) { diff --git a/src/util/virpci.h b/src/util/virpci.h index d1ac942..0debd7b 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -119,7 +119,6 @@ void virPCIDeviceSetRemoveSlot(virPCIDevice *dev, unsigned int virPCIDeviceGetReprobe(virPCIDevicePtr dev); void virPCIDeviceSetReprobe(virPCIDevice *dev, bool reprobe); -void virPCIDeviceReattachInit(virPCIDevice *dev); virPCIDeviceListPtr virPCIDeviceListNew(void); diff --git a/tests/virpcitest.c b/tests/virpcitest.c index fb0c970..6dceef2 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -301,7 +301,10 @@ testVirPCIDeviceReattachSingle(const void *opaque) if (!dev) goto cleanup; - virPCIDeviceReattachInit(dev); + virPCIDeviceSetUnbindFromStub(dev, true); + virPCIDeviceSetRemoveSlot(dev, true); + virPCIDeviceSetReprobe(dev, true); + if (virPCIDeviceReattach(dev, NULL, NULL) < 0) goto cleanup; -- 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
participants (2)
-
Andrea Bolognani -
Daniel P. Berrange