[libvirt] [PATCH 4/6] hostdev: Mark PCI devices as inactive as they're detached

[Laine Stump]

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: