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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list