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

When failing on starting a domain, it tries to reattach all the PCI devices defined in the domain conf, regardless of whether the devices are still used by other domain. This will cause the devices are deleted from the list qemu_driver->activePciHostdevs, thus the devices will be thought as usable even if it's not true. And following commands nodedev-{reattach,reset} will be successful. How to reproduce: 1) Define two domains with same PCI device defined in the confs. 2) # virsh start domain1 3) # virsh start domain2 4) # virsh nodedev-reattach $pci_device You will see the device will be reattached to host successfully. As pciDeviceReattach just check if the device is still used by other domain via checking if the device is in list driver->activePciHostdevs, however, the device is deleted from the list by step 2). This patch is to prohibit the bug by: 1) Prohibit a domain starting or device attachment right at preparation period (qemuPrepareHostdevPCIDevices) if the device is in list driver->activePciHostdevs, which means it's used by other domain. 2) Introduces a new field for struct _pciDevice, (char *used_by), it will be set as the domain name at preparation period, (qemuPrepareHostdevPCIDevices). Thus we can prohibit deleting the device from driver->activePciHostdevs if it's still used by other domain when stopping the domain process. * src/pci.h (define two internal functions, pciDeviceSetUsedBy and pciDevceGetUsedBy) * src/pci.c (new field "char *used_by" for struct _pciDevice, implementations for the two new functions) * src/libvirt_private.syms (Add the two new internal functions) * src/qemu_hostdev.h (Modify the definition of functions qemuPrepareHostdevPCIDevices, and qemuDomainReAttachHostdevDevices) * src/qemu_hostdev.c (Prohibit preparation and don't delete the device from activePciHostdevs list if it's still used by other domain) * src/qemu_hotplug.c (Update function usage, as the definitions are changed) --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_hostdev.c | 31 ++++++++++++++++++++++++++++--- src/qemu/qemu_hostdev.h | 2 ++ src/qemu/qemu_hotplug.c | 4 ++-- src/util/pci.c | 22 ++++++++++++++++++++++ src/util/pci.h | 3 +++ 6 files changed, 59 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8235ea1..a5c5e6c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -872,6 +872,7 @@ virNWFilterHashTableRemoveEntry; pciDettachDevice; pciDeviceFileIterate; pciDeviceGetManaged; +pciDeviceGetUsedBy; pciDeviceIsAssignable; pciDeviceIsVirtualFunction; pciDeviceListAdd; @@ -884,6 +885,7 @@ pciDeviceListSteal; pciDeviceNetName; pciDeviceReAttachInit; pciDeviceSetManaged; +pciDeviceSetUsedBy; pciFreeDevice; pciGetDevice; pciGetPhysicalFunction; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 6f77717..ef9e3b7 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -101,6 +101,7 @@ cleanup: int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, + const char *name, virDomainHostdevDefPtr *hostdevs, int nhostdevs) { @@ -126,7 +127,10 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (!pciDeviceIsAssignable(dev, !driver->relaxedACS)) - goto reattachdevs; + goto cleanup; + + if (pciDeviceListFind(driver->activePciHostdevs, dev)) + goto cleanup; if (pciDeviceGetManaged(dev) && pciDettachDevice(dev, driver->activePciHostdevs) < 0) @@ -156,6 +160,14 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, pciDeviceListSteal(pcidevs, dev); } + /* Now set the used_by_domain of the device in driver->activePciHostdevs + * as domain name. + */ + for (i = 0; i < pciDeviceListCount(driver->activePciHostdevs); i++) { + pciDevice * dev = pciDeviceListGet(driver->activePciHostdevs, i); + pciDeviceSetUsedBy(dev, name); + } + ret = 0; goto cleanup; @@ -183,7 +195,7 @@ static int qemuPrepareHostPCIDevices(struct qemud_driver *driver, virDomainDefPtr def) { - return qemuPrepareHostdevPCIDevices(driver, def->hostdevs, def->nhostdevs); + return qemuPrepareHostdevPCIDevices(driver, def->name, def->hostdevs, def->nhostdevs); } @@ -258,11 +270,13 @@ void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver) void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, + const char *name, virDomainHostdevDefPtr *hostdevs, int nhostdevs) { pciDeviceList *pcidevs; int i; + const char *used_by = NULL; if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) { virErrorPtr err = virGetLastError(); @@ -277,6 +291,17 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); + pciDevice *activeDev = NULL; + + /* Never delete the dev from list driver->activePciHostdevs + * if it's used by other domain. + */ + activeDev = pciDeviceListFind(driver->activePciHostdevs, dev); + if (activeDev && + (used_by = pciDeviceGetUsedBy(activeDev)) && + STRNEQ(used_by, name)) + continue; + pciDeviceListDel(driver->activePciHostdevs, dev); } @@ -305,5 +330,5 @@ void qemuDomainReAttachHostDevices(struct qemud_driver *driver, if (!def->nhostdevs) return; - qemuDomainReAttachHostdevDevices(driver, def->hostdevs, def->nhostdevs); + qemuDomainReAttachHostdevDevices(driver, def->name, def->hostdevs, def->nhostdevs); } diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 1f3d1bc..07d7de2 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -30,12 +30,14 @@ int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, virDomainDefPtr def); int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, + const char *name, virDomainHostdevDefPtr *hostdevs, int nhostdevs); int qemuPrepareHostDevices(struct qemud_driver *driver, virDomainDefPtr def); void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver); void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, + const char *name, virDomainHostdevDefPtr *hostdevs, int nhostdevs); void qemuDomainReAttachHostDevices(struct qemud_driver *driver, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6cfe392..dc920e7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -859,7 +859,7 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, return -1; } - if (qemuPrepareHostdevPCIDevices(driver, &hostdev, 1) < 0) + if (qemuPrepareHostdevPCIDevices(driver, vm->def->name, &hostdev, 1) < 0) return -1; if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { @@ -925,7 +925,7 @@ error: hostdev->info.addr.pci.slot) < 0) VIR_WARN("Unable to release PCI address on host device"); - qemuDomainReAttachHostdevDevices(driver, &hostdev, 1); + qemuDomainReAttachHostdevDevices(driver, vm->def->name, &hostdev, 1); VIR_FREE(devstr); VIR_FREE(configfd_name); diff --git a/src/util/pci.c b/src/util/pci.c index 8d8e157..38548c7 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -62,6 +62,7 @@ struct _pciDevice { char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ char id[PCI_ID_LEN]; /* product vendor */ char *path; + char *used_by; /* The domain which uses the device */ int fd; unsigned initted; @@ -1312,6 +1313,7 @@ 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, @@ -1374,6 +1376,7 @@ pciFreeDevice(pciDevice *dev) VIR_DEBUG("%s %s: freeing", dev->id, dev->name); pciCloseConfig(dev); VIR_FREE(dev->path); + VIR_FREE(dev->used_by); VIR_FREE(dev); } @@ -1387,6 +1390,25 @@ unsigned pciDeviceGetManaged(pciDevice *dev) return dev->managed; } +int +pciDeviceSetUsedBy(pciDevice *dev, const char *name) +{ + dev->used_by = strdup(name); + + if (!dev->used_by) { + virReportOOMError(); + return -1; + } + + return 0; +} + +const char * +pciDeviceGetUsedBy(pciDevice *dev) +{ + return dev->used_by; +} + void pciDeviceReAttachInit(pciDevice *pci) { pci->unbind_from_stub = 1; diff --git a/src/util/pci.h b/src/util/pci.h index a1600fe..c9d8227 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -47,6 +47,9 @@ int pciResetDevice (pciDevice *dev, void pciDeviceSetManaged(pciDevice *dev, unsigned managed); unsigned pciDeviceGetManaged(pciDevice *dev); +int pciDeviceSetUsedBy(pciDevice *dev, + const char *used_by); +const char *pciDeviceGetUsedBy(pciDevice *dev); void pciDeviceReAttachInit(pciDevice *dev); pciDeviceList *pciDeviceListNew (void);