On 2012年01月18日 04:02, Osier Yang wrote:

From: root<root@hp-dl360g7-01.lab.eng.brq.redhat.com>

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 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 PCI device list (just like qemu_driver->activePciHostdevs), and the whole logic is:

* Add the device to inactive list when do nodedev-dettach * 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. --- 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. + */ + pciDeviceList *inactivePciHostdevs; + virBitmapPtr reservedVNCPorts;

virSysinfoDefPtr hostsysinfo; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 712f1fc..47e2380 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -588,6 +588,9 @@ qemudStartup(int privileged) { if ((qemu_driver->activeUsbHostdevs = usbDeviceListNew()) == NULL) goto error;

+ if ((qemu_driver->inactivePciHostdevs = pciDeviceListNew()) == NULL) + goto error; + if (privileged) { if (chown(qemu_driver->libDir, qemu_driver->user, qemu_driver->group)< 0) { virReportSystemError(errno, @@ -778,6 +781,7 @@ qemudShutdown(void) {

qemuDriverLock(qemu_driver); pciDeviceListFree(qemu_driver->activePciHostdevs); + pciDeviceListFree(qemu_driver->inactivePciHostdevs); usbDeviceListFree(qemu_driver->activeUsbHostdevs); virCapabilitiesFree(qemu_driver->caps);

@@ -9211,6 +9215,7 @@ qemudNodeDeviceDettach (virNodeDevicePtr dev) pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; + bool in_inactive_list = false;

if (qemudNodeDeviceGetPciInfo(dev,&domain,&bus,&slot,&function)< 0) return -1; @@ -9220,13 +9225,17 @@ qemudNodeDeviceDettach (virNodeDevicePtr dev) return -1;

qemuDriverLock(driver); - if (pciDettachDevice(pci, driver->activePciHostdevs)< 0) + in_inactive_list = pciDeviceListFind(driver->inactivePciHostdevs, pci); + + if (pciDettachDevice(pci, driver->activePciHostdevs, + driver->inactivePciHostdevs)< 0) goto out;

ret = 0; out: qemuDriverUnlock(driver); - pciFreeDevice(pci); + if (in_inactive_list) + pciFreeDevice(pci); return ret; }

@@ -9248,7 +9257,8 @@ qemudNodeDeviceReAttach (virNodeDevicePtr dev) pciDeviceReAttachInit(pci);

qemuDriverLock(driver); - if (pciReAttachDevice(pci, driver->activePciHostdevs)< 0) + if (pciReAttachDevice(pci, driver->activePciHostdevs, + driver->inactivePciHostdevs)< 0) goto out;

ret = 0; @@ -9275,7 +9285,8 @@ qemudNodeDeviceReset (virNodeDevicePtr dev)

qemuDriverLock(driver);

- if (pciResetDevice(pci, driver->activePciHostdevs, NULL)< 0) + if (pciResetDevice(pci, driver->activePciHostdevs, + driver->inactivePciHostdevs)< 0) goto out;

ret = 0; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index c7adb1d..6141cfe 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -212,7 +212,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, for (i = 0; i< pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (pciDeviceGetManaged(dev)&& - pciDettachDevice(dev, driver->activePciHostdevs)< 0) + pciDettachDevice(dev, driver->activePciHostdevs, NULL)< 0) goto reattachdevs; }

@@ -220,7 +220,8 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, * can safely reset them */ for (i = 0; i< pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - if (pciResetDevice(dev, driver->activePciHostdevs, pcidevs)< 0) + if (pciResetDevice(dev, driver->activePciHostdevs, + driver->inactivePciHostdevs)< 0) goto reattachdevs; }

@@ -233,7 +234,13 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, } }

- /* Loop 5: Now set the used_by_domain of the device in + /* Loop 5: Now remove the devices from inactive list. */ + for (i = 0; i< pciDeviceListCount(pcidevs); i++) { + pciDevice *dev = pciDeviceListGet(pcidevs, i); + pciDeviceListDel(driver->inactivePciHostdevs, dev); + } + + /* Loop 6: Now set the used_by_domain of the device in * driver->activePciHostdevs as domain name. */ for (i = 0; i< pciDeviceListCount(pcidevs); i++) { @@ -245,7 +252,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, pciDeviceSetUsedBy(activeDev, name); }

- /* Loop 6: Now set the original states for hostdev def */ + /* Loop 7: Now set the original states for hostdev def */ for (i = 0; i< nhostdevs; i++) { pciDevice *dev; pciDevice *pcidev; @@ -277,7 +284,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, pciFreeDevice(dev); }

- /* Loop 7: Now steal all the devices from pcidevs */ + /* Loop 8: Now steal all the devices from pcidevs */ while (pciDeviceListCount(pcidevs)> 0) { pciDevice *dev = pciDeviceListGet(pcidevs, 0); pciDeviceListSteal(pcidevs, dev); @@ -298,7 +305,7 @@ inactivedevs: reattachdevs: for (i = 0; i< pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciReAttachDevice(dev, driver->activePciHostdevs); + pciReAttachDevice(dev, driver->activePciHostdevs, NULL); }

cleanup: @@ -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. + */ + if (!pciDeviceGetManaged(dev)) { + pciDeviceListAdd(driver->inactivePciHostdevs, dev); return; + }

while (pciWaitForDeviceCleanup(dev, "kvm_assigned_device") && retries) { @@ -454,7 +466,8 @@ void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver) retries--; }

- if (pciReAttachDevice(dev, driver->activePciHostdevs)< 0) { + if (pciReAttachDevice(dev, driver->activePciHostdevs, + driver->inactivePciHostdevs)< 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to re-attach PCI device: %s"), err ? err->message : _("unknown error")); @@ -503,7 +516,8 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver,

for (i = 0; i< pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - if (pciResetDevice(dev, driver->activePciHostdevs, pcidevs)< 0) { + if (pciResetDevice(dev, driver->activePciHostdevs, + driver->inactivePciHostdevs)< 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s"), err ? err->message : _("unknown error")); iff --git a/src/util/pci.c b/src/util/pci.c index 17cde81..633dcd8 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1117,7 +1117,9 @@ cleanup: }

int -pciDettachDevice(pciDevice *dev, pciDeviceList *activeDevs) +pciDettachDevice(pciDevice *dev, + pciDeviceList *activeDevs, + pciDeviceList *inactiveDevs) { const char *driver = pciFindStubDriver(); if (!driver) { @@ -1132,11 +1134,22 @@ pciDettachDevice(pciDevice *dev, pciDeviceList *activeDevs) return -1; }

- return pciBindDeviceToStub(dev, driver); + if (pciBindDeviceToStub(dev, driver)< 0) + return -1; + + /* Add the dev into list inactiveDevs */ + if (inactiveDevs&& !pciDeviceListFind(inactiveDevs, dev)) { + if (pciDeviceListAdd(inactiveDevs, dev)< 0) + return -1; + } + + return 0; }

int -pciReAttachDevice(pciDevice *dev, pciDeviceList *activeDevs) +pciReAttachDevice(pciDevice *dev, + pciDeviceList *activeDevs, + pciDeviceList *inactiveDevs) { const char *driver = pciFindStubDriver(); if (!driver) { @@ -1151,7 +1164,14 @@ pciReAttachDevice(pciDevice *dev, pciDeviceList *activeDevs) return -1; }

- return pciUnbindDeviceFromStub(dev, driver); + if (pciUnbindDeviceFromStub(dev, driver)< 0) + return -1; + + /* Steal the dev from list inactiveDevs */ + if (inactiveDevs) + pciDeviceListSteal(inactiveDevs, dev); + + return 0; }

/* Certain hypervisors (like qemu/kvm) map the PCI bar(s) on diff --git a/src/util/pci.h b/src/util/pci.h index 5493e0a..25b5b66 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -40,8 +40,12 @@ pciDevice *pciGetDevice (unsigned domain, unsigned function); void pciFreeDevice (pciDevice *dev); const char *pciDeviceGetName (pciDevice *dev); -int pciDettachDevice (pciDevice *dev, pciDeviceList *activeDevs); -int pciReAttachDevice (pciDevice *dev, pciDeviceList *activeDevs); +int pciDettachDevice (pciDevice *dev, + pciDeviceList *activeDevs, + pciDeviceList *inactiveDevs); +int pciReAttachDevice (pciDevice *dev, + pciDeviceList *activeDevs, + pciDeviceList *inactiveDevs); int pciResetDevice (pciDevice *dev, pciDeviceList *activeDevs, pciDeviceList *inactiveDevs); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 20671c0..520ec03 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1985,7 +1985,7 @@ xenUnifiedNodeDeviceDettach (virNodeDevicePtr dev) if (!pci) return -1;

- if (pciDettachDevice(pci, NULL)< 0) + if (pciDettachDevice(pci, NULL, NULL)< 0) goto out;

ret = 0; @@ -2075,7 +2075,7 @@ xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) goto out; }

- if (pciReAttachDevice(pci, NULL)< 0) + if (pciReAttachDevice(pci, NULL, NULL)< 0) goto out;

ret = 0;

Along with attached patch, it has to honor inactive PCI device list when detaching too. And the attached patch fixed another problem incidentally (pciResetDevice returns -1 and 0, there is logic error of the return value checking). Regards, Osier