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