Re: [PATCH 1/7] hostdev: Add reattachPCIDevices()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]