[libvirt] [PATCH v2] qemu: Introduce inactive PCI device list

[Eric Blake]

On 01/17/2012 01:02 PM, Osier Yang wrote:
> pciTrySecondaryBusReset checks if there is active device on the
> same bus, however, qemu driver doesn't maintain an effective
> list for the inactive devices, and it passes meaningless argment

s/argment/argument/

> for parameter "inactiveDevs". e.g. (qemuPrepareHostdevPCIDevices)
> 
> if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs)))
>     return -1;
> 
> ...skipped...
> 
> if (pciResetDevice(dev, driver->activePciHostdevs, pcidevs) < 0)
>     goto reattachdevs;
> 
> NB, the "pcidevs" used above are extracted from domain def, and
> thus one won't be able to attach a device of which bus has other
> device even detached from host (nodedev-detach). To see more
> details of the problem:
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=773667
> 
> This patch is to resolve the problem by introduce an inactive

s/introduce/introducing/

> PCI device list (just like qemu_driver->activePciHostdevs), and
> the whole logic is:
> 
>   * Add the device to inactive list when do nodedev-dettach

s/when do/during/

>   * Remove the device from inactive list when do nodedev-reattach
>   * Remove the device from inactive list when do attach-device
>     (for not managed device)
>   * Add the device to inactive list after detach-device, only
>     if the device is not managed
> 
> With above, we have sufficient inactive PCI device list, and thus
> we can use it for pciResetDevice. e.g.(qemuPrepareHostdevPCIDevices)
> 
> if (pciResetDevice(dev, driver->activePciHostdevs,
>                    driver->inactivePciHostdevs) < 0)
>     goto reattachdevs;
> 
> v1 ~ v2
> 
> Fixed a potential crash.
> ---
> Got a testing box from Miroslav and tested the patch, it works well.

I'm glad you were able to test it; I tried to reproduce things locally,
but didn't quite have the right hardware, so I'm reviewing this on
inspection alone.  But it is a serious enough issue that I'm okay
pushing once the nits are fixed.

> ---
>  src/qemu/qemu_conf.h    |    5 +++++
>  src/qemu/qemu_driver.c  |   19 +++++++++++++++----
>  src/qemu/qemu_hostdev.c |   32 +++++++++++++++++++++++---------
>  src/util/pci.c          |   28 ++++++++++++++++++++++++----
>  src/util/pci.h          |    8 ++++++--
>  src/xen/xen_driver.c    |    4 ++--
>  7 files changed, 77 insertions(+), 22 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index d8d7915..3f1b668 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -128,6 +128,11 @@ struct qemud_driver {
>      pciDeviceList *activePciHostdevs;
>      usbDeviceList *activeUsbHostdevs;
>  
> +    /* The device which is not used by both host
> +     * and any guest.

The devices which are not in use by the host or any guest.

> @@ -778,6 +781,7 @@ qemudShutdown(void) {
>  
>      qemuDriverLock(qemu_driver);
>      pciDeviceListFree(qemu_driver->activePciHostdevs);
> +    pciDeviceListFree(qemu_driver->inactivePciHostdevs);
>      usbDeviceListFree(qemu_driver->activeUsbHostdevs);

We'll probably have to repeat this exercise for USB passthrough devices,
but that can be a separate patch.

> @@ -445,8 +452,13 @@ void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver)
>  {
>      int retries = 100;
>  
> -    if (!pciDeviceGetManaged(dev))
> +    /* If the device is not managed and was attached to guest
> +     * successfully, it must had be inactive.

s/had be/have been/

Overall, the design looks sound.  I squashed in your followup, plus
this, then pushed:

diff --git i/src/qemu/qemu_conf.h w/src/qemu/qemu_conf.h
index 3f1b668..7d79823 100644
--- i/src/qemu/qemu_conf.h
+++ w/src/qemu/qemu_conf.h
@@ -1,7 +1,7 @@
 /*
  * qemu_conf.h: QEMU configuration management
  *
- * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2007, 2009-2012 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -128,9 +128,7 @@ struct qemud_driver {
     pciDeviceList *activePciHostdevs;
     usbDeviceList *activeUsbHostdevs;

-    /* The device which is not used by both host
-     * and any guest.
-     */
+    /* The devices which is are not in use by the host or any guest. */
     pciDeviceList *inactivePciHostdevs;

     virBitmapPtr reservedVNCPorts;
diff --git i/src/qemu/qemu_hostdev.c w/src/qemu/qemu_hostdev.c
index 6141cfe..b3cad8e 100644
--- i/src/qemu/qemu_hostdev.c
+++ w/src/qemu/qemu_hostdev.c
@@ -1,7 +1,7 @@
 /*
  * qemu_hostdev.c: QEMU hostdev management
  *
- * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2007, 2009-2012 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -453,7 +453,7 @@ void qemuReattachPciDevice(pciDevice *dev, struct
qemud_driver *driver)
     int retries = 100;

     /* If the device is not managed and was attached to guest
-     * successfully, it must had be inactive.
+     * successfully, it must have been inactive.
      */
     if (!pciDeviceGetManaged(dev)) {
         pciDeviceListAdd(driver->inactivePciHostdevs, dev);


-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120117/c2a08e4d/attachment-0001.sig>