Re: [libvirt] [PATCH v2] qemu: Do not reattach PCI device used by other domain when shutdown

On 10/12/2011 10:05 PM, Osier Yang wrote: This time around, I actually spent time reproducing the bug scenario on hardware rather than just analyzing by inspection (it took me a while to figure out why the same machine that used to let me do pci passthrough was no longer working, until I realized that my hardware is old enough to be insecure, and I've upgraded software since my last passthrough test, so the CVE fix from https://bugzilla.redhat.com/show_bug.cgi?id=715555 has to be intentionally bypassed for me to get passthrough again). With that testing, I proved that this patch prevents that problem. But, as written, your patch caused the dreaded "An error occurred, but the cause is unknown". I added further loop labels. Here's where the bad message comes in - you jump to cleanup without ever emitting an error message. Not to mention now that we have the new used_by field, the message could actually be informative :) With my changes below, I get the much nicer: Error starting domain: Requested operation is not valid: PCI device 0000:0a:0a.0 is in use by domain dom In that case, use STRNEQ_NULLABLE. But you are right - across libvirtd restarts, we lose track of which domain owns the device - so I did indeed reproduce a case of NULL. Perhaps food for a later patch (when reloading domains, cycle through all hostdevs in use by active domains to repopulate the used_by fields). But not a show-stopper to getting this bug fix in now. Well, I did find one other problem with a libvirtd restart - as soon as used_by is NULL, then stopping the domain that was actually using the device no longer frees that device out of the driver->active list (since we now only free if used_by matches) - so maybe it's important to fix libvirtd reload repopulating used_by sooner rather than later. Pointless, since VIR_ALLOC 0-initializes. Pointless, since dev is freed in the next statement. ACK with this squashed in, so I went ahead and pushed. diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms index 4673332..1e03e33 100644 --- i/src/libvirt_private.syms +++ w/src/libvirt_private.syms @@ -881,6 +881,7 @@ virNWFilterHashTableRemoveEntry; pciDettachDevice; pciDeviceFileIterate; pciDeviceGetManaged; +pciDeviceGetName; pciDeviceGetUsedBy; pciDeviceIsAssignable; pciDeviceIsVirtualFunction; diff --git i/src/qemu/qemu_hostdev.c w/src/qemu/qemu_hostdev.c index f354ea8..c82c512 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-2010 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -119,21 +119,40 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, * must be reset before being marked as active. */ - /* XXX validate that non-managed device isn't in use, eg + /* Loop 1: validate that non-managed device isn't in use, eg * by checking that device is either un-bound, or bound * to pci-stub.ko */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); + pciDevice *other; + + if (!pciDeviceIsAssignable(dev, !driver->relaxedACS)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("PCI device %s is not assignable"), + pciDeviceGetName(dev)); + goto cleanup; + } /* The device is in use by other active domain if * the dev is in list driver->activePciHostdevs. */ - if (!pciDeviceIsAssignable(dev, !driver->relaxedACS) || - pciDeviceListFind(driver->activePciHostdevs, dev)) + if ((other = pciDeviceListFind(driver->activePciHostdevs, dev))) { + const char *other_name = pciDeviceGetUsedBy(other); + + if (other_name) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("PCI device %s is in use by domain %s"), + pciDeviceGetName(dev), other_name); + else + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("PCI device %s is already in use"), + pciDeviceGetName(dev)); goto cleanup; + } } + /* Loop 2: detach managed devices */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (pciDeviceGetManaged(dev) && @@ -141,15 +160,15 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, goto reattachdevs; } - /* Now that all the PCI hostdevs have be dettached, we can safely - * reset them */ + /* Loop 3: Now that all the PCI hostdevs have been detached, we + * can safely reset them */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (pciResetDevice(dev, driver->activePciHostdevs, pcidevs) < 0) goto reattachdevs; } - /* Now mark all the devices as active */ + /* Loop 4: Now mark all the devices as active */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (pciDeviceListAdd(driver->activePciHostdevs, dev) < 0) { @@ -158,8 +177,8 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, } } - /* Now set the used_by_domain of the device in driver->activePciHostdevs - * as domain name. + /* Loop 5: Now set the used_by_domain of the device in + * driver->activePciHostdevs as domain name. */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev, *activeDev; @@ -170,7 +189,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, pciDeviceSetUsedBy(activeDev, name); } - /* Now steal all the devices from pcidevs */ + /* Loop 6: Now steal all the devices from pcidevs */ while (pciDeviceListCount(pcidevs) > 0) { pciDevice *dev = pciDeviceListGet(pcidevs, 0); pciDeviceListSteal(pcidevs, dev); @@ -299,15 +318,13 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); pciDevice *activeDev = NULL; - const char *used_by = NULL; /* Never delete the dev from list driver->activePciHostdevs - * if it's used by other domain. + * if it's used by other domain. */ activeDev = pciDeviceListFind(driver->activePciHostdevs, dev); if (activeDev && - (used_by = pciDeviceGetUsedBy(activeDev)) && - STRNEQ(name, used_by)) + STRNEQ_NULLABLE(name, pciDeviceGetUsedBy(activeDev))) continue; pciDeviceListDel(driver->activePciHostdevs, dev); diff --git i/src/util/pci.c w/src/util/pci.c index 888784a..2bbb90c 100644 --- i/src/util/pci.c +++ w/src/util/pci.c @@ -1313,7 +1313,6 @@ pciGetDevice(unsigned domain, dev->bus = bus; dev->slot = slot; dev->function = function; - dev->used_by = NULL; if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", dev->domain, dev->bus, dev->slot, @@ -1376,10 +1375,15 @@ pciFreeDevice(pciDevice *dev) VIR_DEBUG("%s %s: freeing", dev->id, dev->name); pciCloseConfig(dev); VIR_FREE(dev->path); - dev->used_by = NULL; VIR_FREE(dev); } +const char * +pciDeviceGetName(pciDevice *dev) +{ + return dev->name; +} + void pciDeviceSetManaged(pciDevice *dev, unsigned managed) { dev->managed = !!managed; diff --git i/src/util/pci.h w/src/util/pci.h index 640c6af..ab29c0b 100644 --- i/src/util/pci.h +++ w/src/util/pci.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009, 2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -39,6 +39,7 @@ pciDevice *pciGetDevice (unsigned domain, unsigned slot, unsigned function); void pciFreeDevice (pciDevice *dev); +const char *pciDeviceGetName (pciDevice *dev); int pciDettachDevice (pciDevice *dev, pciDeviceList *activeDevs); int pciReAttachDevice (pciDevice *dev, pciDeviceList *activeDevs); int pciResetDevice (pciDevice *dev, -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org