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

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

On 2012年01月18日 08:14, Eric Blake wrote:
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


for parameter "inactiveDevs". e.g. (qemuPrepareHostdevPCIDevices)

if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs)))
     return -1;


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


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) {

+    pciDeviceListFree(qemu_driver->inactivePciHostdevs);

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:



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