[libvirt] [PATCH v2] qemu: Introduce inactive PCI device list
Osier Yang
jyang at redhat.com
Tue Jan 17 19:40:21 UTC 2012
On 2012年01月18日 04:02, Osier Yang wrote:
> From: root<root at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: additional.patch
Type: text/x-patch
Size: 681 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120118/13e4a79f/attachment-0001.bin>
More information about the libvir-list
mailing list