[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

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:


Thanks.

Osier


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