[libvirt] [PATCH v2] qemu: Do not reattach PCI device used by other domain when shutdown
Osier Yang
jyang at redhat.com
Thu Oct 13 03:33:16 UTC 2011
于 2011年10月13日 12:05, Osier Yang 写道:
> 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 to be 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, (const 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 "const 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)
>
> v1 - v2:
> * char * used_by => const char *used_by, and no strdup()
> * Other nits pointed out by Eric.
> ---
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_hostdev.c | 43 ++++++++++++++++++++++++++++++++++++++-----
> src/qemu/qemu_hostdev.h | 2 ++
> src/qemu/qemu_hotplug.c | 4 ++--
> src/util/pci.c | 15 +++++++++++++++
> src/util/pci.h | 3 +++
> 6 files changed, 62 insertions(+), 7 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 4f96518..4d50d86 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -880,6 +880,7 @@ virNWFilterHashTableRemoveEntry;
> pciDettachDevice;
> pciDeviceFileIterate;
> pciDeviceGetManaged;
> +pciDeviceGetUsedBy;
> pciDeviceIsAssignable;
> pciDeviceIsVirtualFunction;
> pciDeviceListAdd;
> @@ -892,6 +893,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..f354ea8 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)
> {
> @@ -111,7 +112,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
> if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs)))
> return -1;
>
> - /* We have to use 4 loops here. *All* devices must
> + /* We have to use 6 loops here. *All* devices must
> * be detached before we reset any of them, because
> * in some cases you have to reset the whole PCI,
> * which impacts all devices on it. Also, all devices
> @@ -125,9 +126,16 @@ 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;
> + /* 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))
> + goto cleanup;
> + }
>
> + for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
> + pciDevice *dev = pciDeviceListGet(pcidevs, i);
> if (pciDeviceGetManaged(dev) &&
> pciDettachDevice(dev, driver->activePciHostdevs) < 0)
> goto reattachdevs;
> @@ -150,6 +158,18 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
> }
> }
>
> + /* 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;
> +
> + dev = pciDeviceListGet(pcidevs, i);
> + activeDev = pciDeviceListFind(driver->activePciHostdevs, dev);
> +
> + pciDeviceSetUsedBy(activeDev, name);
> + }
> +
> /* Now steal all the devices from pcidevs */
> while (pciDeviceListCount(pcidevs) > 0) {
> pciDevice *dev = pciDeviceListGet(pcidevs, 0);
> @@ -183,7 +203,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,6 +278,7 @@ void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver)
>
>
> void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver,
> + const char *name,
> virDomainHostdevDefPtr *hostdevs,
> int nhostdevs)
> {
> @@ -277,6 +298,18 @@ 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.
> + */
> + activeDev = pciDeviceListFind(driver->activePciHostdevs, dev);
> + if (activeDev &&
> + (used_by = pciDeviceGetUsedBy(activeDev)) &&
> + STRNEQ(name, used_by))
used_by is kept as it might be NULL.
> + continue;
> +
> pciDeviceListDel(driver->activePciHostdevs, dev);
> }
>
> @@ -305,5 +338,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 4c1705d..bfa524b 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -893,7 +893,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)) {
> @@ -959,7 +959,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..888784a 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;
> + const 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);
> + dev->used_by = NULL;
> VIR_FREE(dev);
> }
>
> @@ -1387,6 +1390,18 @@ unsigned pciDeviceGetManaged(pciDevice *dev)
> return dev->managed;
> }
>
> +void
> +pciDeviceSetUsedBy(pciDevice *dev, const char *name)
> +{
> + dev->used_by = name;
> +}
> +
> +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..640c6af 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);
> +void pciDeviceSetUsedBy(pciDevice *dev,
> + const char *used_by);
> +const char *pciDeviceGetUsedBy(pciDevice *dev);
> void pciDeviceReAttachInit(pciDevice *dev);
>
> pciDeviceList *pciDeviceListNew (void);
More information about the libvir-list
mailing list