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