9:37 a.m.
On 12/17/2015 10:23 AM, Andrea Bolognani wrote:
We want to eventually factor out the code dealing with device
detaching
and reattaching, so that we can share it and make sure it's called eg.
when 'virsh nodedev-detach' is used.
For that to happen, it's important that the lists of active and inactive
PCI devices are updated every time a device changes its state.
We need to know which devices from an iommu group were unbound from the
host driver by us, so that we'll only re-bind those that we had unbound
in the first place. So I can see the reasoning for wanting the inactive
list to always hold the complete list of devices that libvirt has
detached from the host driver, but that aren't at the moment in use by
any domain.
In this case that you're fixing, though, it's only a temporary
inconsistency, since "Loop 6" of that same function will add the
detached devices to the inactive list.
However, when I trace down into the one use of the inactive list between
Loop 2 (where you're adding the devices in this patch) and Loop 6 (where
they were previously added), I've found that your patch may actually be
fixing a latent bug, but only in the case where someone is using the
legacy KVM device assignment - if virPCIDeviceReset() (which returns
with no action when vfio-pci is used) is unsuccessful at resetting the
device individually, it will try resetting the entire bus the device is
on using virPCIDeviceTrySecondaryBusReset(), but it can't do this unless
all the other devices on the bus are unused. This is determined by
calling virPCIDeviceBusContainsActiveDevices(), which iterates through
every PCI device on the host calling virPCIDeviceSharesBusWithActive().
That function will return true if it finds a device on the same bus that
*isn't* on the inactive list. So if a device is on a bus with multiple
devices, those devices have all been assigned to the guest using legacy
KVM assignment, and the normal device reset functions don't work for
them, the current code would fail, while yours would succeed. *Highly*
unlikely (and we don't really care, since legacy KVM device assignment
is, well, legacy; deprecated; soon to go away; "pining for the fjords"
as it were :-)
(sorry for the digression. I spent so much time investigating that it
didn't feel right to just conclude the search by saying "nothing here.
Move on!)
Anyway I see no problem caused by this patch, so ACK.
I guess you also intend to begin storing the inactive device list
somewhere so that it can be reread if libvirtd is restarted?
Instead of passing NULL as the last argument of virPCIDeviceDetach() and
virPCIDeviceReattach(), pass the proper list so that it can be updated.
---
src/util/virhostdev.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index f9072a4..afacd4e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -595,11 +595,17 @@ 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) &&
- virPCIDeviceDetach(dev, hostdev_mgr->activePCIHostdevs, NULL) < 0)
- goto reattachdevs;
+ virPCIDeviceDetach(dev,
+ hostdev_mgr->activePCIHostdevs,
+ hostdev_mgr->inactivePCIHostdevs) < 0)
+ goto reattachdevs;
}
+ /* At this point, all 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 */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
@@ -708,8 +714,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
/* NB: This doesn't actually re-bind to original driver, just
* unbinds from the stub driver
*/
- ignore_value(virPCIDeviceReattach(dev, hostdev_mgr->activePCIHostdevs,
- NULL));
+ ignore_value(virPCIDeviceReattach(dev,
+ hostdev_mgr->activePCIHostdevs,
+ hostdev_mgr->inactivePCIHostdevs));
}
cleanup: