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:
