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

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: