[libvirt] [PATCH 12/22] pci: optionally detach/reattach all devices in a VFIO group

It will sometimes be desirable for a "managed" hostdev that is in a VFIO group with several other devices to cause *all* of the devices in the group to be detached/reattached. This patch gives that ability to virPCIDeviceDetach and virPCIDeviceReattach. To cause this, you would need to have previously called the new virPCIDeviceSetAttachGroup() function, which isn't yet called by anyone. (The "attach_group" setting must be set in the virPCIDevice object rather than passed as an argument to virPCIDevice(De|Reat)tach() because there is at least one case where the necessary information is available when the virPCIDevice is created, but is *not* available at a later time when the attach function is called.) NB: the call to virPCIDeviceReattachInit() was moved from qemu_driver.c:qemuNodeDeviceReAttach() to virpci.c:virPCIDeviceReattach() because it must be done for each device that is being re-attached, and the qemu function only knows about a single device. It also appears that moving the call to virPCIDeviceReattachInit() coincidentally fixes a bug for the xen driver, since no reattach would ever take place without the setting of the flags done by virPCIDeviceReattachInit(), and the xen driver's implementation of xenNodeDeviceReAttach() was never calling it - since it is now called by virPCIDeviceReattach(), re-attach should work properly for the xen driver. --- src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hostdev.c | 4 +- src/util/virpci.c | 254 ++++++++++++++++++++++++++++++++++++++++++----- src/util/virpci.h | 6 +- src/xen/xen_driver.c | 2 +- 6 files changed, 241 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd3a138..9a877ff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1652,6 +1652,7 @@ virPCIDeviceCopy; virPCIDeviceDetach; virPCIDeviceFileIterate; virPCIDeviceFree; +virPCIDeviceGetAttachGroup; virPCIDeviceGetIOMMUGroupDev; virPCIDeviceGetIOMMUGroupList; virPCIDeviceGetManaged; @@ -1676,6 +1677,7 @@ virPCIDeviceNew; virPCIDeviceReattach; virPCIDeviceReattachInit; virPCIDeviceReset; +virPCIDeviceSetAttachGroup; virPCIDeviceSetManaged; virPCIDeviceSetRemoveSlot; virPCIDeviceSetReprobe; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 20b127c..8f498e6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10351,10 +10351,8 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) goto out; } - virPCIDeviceReattachInit(pci); - if (virPCIDeviceReattach(pci, driver->activePciHostdevs, - driver->inactivePciHostdevs) < 0) + driver->inactivePciHostdevs, true) < 0) goto out; ret = 0; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index dfe39c6..1abad59 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -661,7 +661,7 @@ reattachdevs: /* NB: This doesn't actually re-bind to original driver, just * unbinds from the stub driver */ - virPCIDeviceReattach(dev, driver->activePciHostdevs, NULL); + virPCIDeviceReattach(dev, driver->activePciHostdevs, NULL, false); } cleanup: @@ -1042,7 +1042,7 @@ void qemuReattachPciDevice(virPCIDevicePtr dev, virQEMUDriverPtr driver) } if (virPCIDeviceReattach(dev, driver->activePciHostdevs, - driver->inactivePciHostdevs) < 0) { + driver->inactivePciHostdevs, false) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to re-attach PCI device: %s"), err ? err->message : _("unknown error")); diff --git a/src/util/virpci.c b/src/util/virpci.c index cbade1b..80256f9 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -39,6 +39,7 @@ #include "dirname.h" #include "virlog.h" #include "viralloc.h" +#include "virbitmap.h" #include "vircommand.h" #include "virerror.h" #include "virfile.h" @@ -70,6 +71,7 @@ struct _virPCIDevice { bool has_pm_reset; bool managed; char *stubDriver; + bool attach_group; /* true if okay to attach/detach entire group */ /* used by reattach function */ bool unbind_from_stub; @@ -967,7 +969,7 @@ static const char *virPCIKnownStubs[] = { }; static int -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, const char *stubDriver) { int result = -1; char *drvdir = NULL; @@ -985,6 +987,13 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) if (!dev->unbind_from_stub) goto remove_slot; + /* if the caller only wants us to unbind if the device is + * currently bound to one specific stub driver, that driver name + * will be in @stubDriver. + */ + if (stubDriver && STRNEQ(driver, stubDriver)) + goto remove_slot; + /* If the device isn't bound to a known stub, skip the unbind. */ for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) { if (STREQ(driver, *stubTest)) { @@ -1067,9 +1076,19 @@ cleanup: } +/* these drivers are acceptable substitutes for vfio-pci + * in virPCIDeviceBindToStub(), unless "force" is true. + */ +static const char *virPCIVFIOSubstitutes[] = { + "pci-stub", + "pcieport", + NULL +}; + static int virPCIDeviceBindToStub(virPCIDevicePtr dev, - const char *stubDriverName) + const char *stubDriverName, + bool force, bool *bound) { int result = -1; int reprobe = false; @@ -1081,6 +1100,9 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, char *path = NULL; /* reused for different purposes */ const char *newDriverName = NULL; + /* this will be set if we change the binding */ + *bound = false; + if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 || virPCIFile(&driverLink, dev->name, "driver") < 0 || virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath, @@ -1088,6 +1110,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, goto cleanup; } + /* check whether the device is already bound to an acceptable driver */ if (virFileExists(driverLink)) { if (virFileLinkPointsTo(driverLink, stubDriverPath)) { /* The device is already bound to the correct driver */ @@ -1097,6 +1120,28 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, result = 0; goto cleanup; } + /* If force == false, this device is just a secondary driver + * in the same VFIO group as the primary driver we want bound + * to the stub (rather than the primary device that is the + * real target of the bind). In that case, it's okay for it to + * instead be bound to one of the acceptable "substitute" + * drivers (which are deemed by VFIO to be safe for isolation, + * and are on an internal "whitelist" in the vfio driver). + */ + if (!force && oldDriverName && STREQ(stubDriverName, "vfio-pci")) { + const char **stubTest; + for (stubTest = virPCIVFIOSubstitutes; + *stubTest != NULL; stubTest++) { + if (STREQ(oldDriverName, *stubTest)) { + VIR_DEBUG("Device %s is bound to acceptable " + "substitute driver %s", + dev->name, *stubTest); + newDriverName = *stubTest; + result = 0; + goto cleanup; + } + } + } reprobe = true; } @@ -1119,6 +1164,8 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, goto cleanup; } + *bound = true; + /* check whether the device is bound to pci-stub when we write dev->id to * ${stubDriver}/new_id. */ @@ -1221,7 +1268,7 @@ cleanup: result = VIR_STRDUP(dev->stubDriver, newDriverName); } if (result < 0) - virPCIDeviceUnbindFromStub(dev); + virPCIDeviceUnbindFromStub(dev, stubDriverName); return result; } @@ -1249,54 +1296,201 @@ virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceList *activeDevs, virPCIDeviceList *inactiveDevs) { + /* grouplist contains all devices in the group, even dev itself. */ + virPCIDeviceList *groupList = NULL; + virPCIDevicePtr current, copy = NULL; + virBitmapPtr newlyDetached = NULL; + bool bound; + int ii, ret = -1; + if (virPCIProbeStubDriver(dev->stubDriver) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to load PCI stub module %s"), dev->stubDriver); - return -1; + goto cleanup; } if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_OPERATION_INVALID, _("Not detaching active device %s"), dev->name); - return -1; + goto cleanup; } - if (virPCIDeviceBindToStub(dev, dev->stubDriver) < 0) - return -1; + if (dev->attach_group && STREQ(dev->stubDriver, "vfio-pci")) { + /* Get a list of all devices in the same vfio group as this one */ + if (!(groupList = virPCIDeviceGetIOMMUGroupList(dev))) + goto cleanup; + } else { + /* simple case - just detach this one device */ + if (!(groupList = virPCIDeviceListNew()) || + !(copy = virPCIDeviceCopy(dev)) || + virPCIDeviceListAdd(groupList, copy) < 0) { + goto cleanup; + } + copy = NULL; + } - /* Add *a copy of* the dev into list inactiveDevs, if - * it's not already there. + if (!(newlyDetached = virBitmapNew(virPCIDeviceListCount(groupList)))) + goto cleanup; + + /* * groupList contains a copy of all devices that should be + * detached (bound to the stub) + * + * * dev is the one that *must* be detached. + * + * * detachedDevs is a bitmap that has the bit set for each item + * in groupList that has been newly detached (and will need to + * be reattach in case of failure) */ - if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) { - virPCIDevicePtr copy = virPCIDeviceCopy(dev); - if ((!copy) || virPCIDeviceListAdd(inactiveDevs, copy) < 0) - return -1; + for (ii = 0; (current = virPCIDeviceListGet(groupList, ii)); ii++) { + /* If any device other than the "primary" (the one originally + * requested) is on the active list (already assigned to a + * domain), just skip detach - we can't tell right now if it's + * in use by the correct domain, but that will all come out + * when we get to the point of doing the assign. (NB: We've + * already determined that the primary *isn't* on the active + * list) + */ + if (virPCIDeviceListFind(activeDevs, current)) + continue; + + /* bind to stub (force if this is the original device) */ + if (virPCIDeviceBindToStub(current, dev->stubDriver, + STREQ(current->name, dev->name), + &bound) < 0) { + goto cleanup; + } + + /* remember if we detached it */ + if (bound) + ignore_value(virBitmapSetBit(newlyDetached, ii)); + + /* add to inactive */ + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, current) && + (!(copy = virPCIDeviceCopy(current)) || + virPCIDeviceListAdd(inactiveDevs, copy) < 0)) { + goto cleanup; + } + copy = NULL; /* it's on the list now */ } - return 0; + ret = 0; +cleanup: + + if (ret < 0) { + /* try to cleanup what was halfway done if we fail */ + if (groupList && newlyDetached) { + for (ii = 0; (current = virPCIDeviceListGet(groupList, ii)); ii++) { + ignore_value(virBitmapGetBit(newlyDetached, ii, &bound)); + if (bound) + virPCIDeviceUnbindFromStub(current, dev->stubDriver); + } + } + } + + virObjectUnref(groupList); + virPCIDeviceFree(copy); + virBitmapFree(newlyDetached); + return ret; } +/* virPCIDeviceReattach: + * + * Re-attach this device (and all devices in the same group, if + * dev->attach_group is true) to its host driver. + * + * if @force is true, return failure if the given device couldn't be + * re-attached due to another device in the group being buse. If + * @force is false, ignore this circumstance (i.e., don't re-attach + * the device to the host, but return success). + * + * Return 0 on success, -1 on failure. + */ int virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, - virPCIDeviceListPtr inactiveDevs) + virPCIDeviceListPtr inactiveDevs, + bool force) { + virPCIDeviceList *groupList = NULL; + virPCIDevicePtr current, copy = NULL; + char *drvPath = NULL, *drvName = NULL; + int ii, ret = -1; + if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_OPERATION_INVALID, _("Not reattaching active device %s"), dev->name); - return -1; + goto cleanup; } - if (virPCIDeviceUnbindFromStub(dev) < 0) - return -1; + if (virPCIDeviceGetDriverPathAndName(dev, &drvPath, &drvName) < 0) + goto cleanup; - /* Steal the dev from list inactiveDevs */ - if (inactiveDevs) - virPCIDeviceListDel(inactiveDevs, dev); + if (dev->attach_group && STREQ_NULLABLE(drvName, "vfio-pci")) { + /* Get a list of all devices in the same vfio group as this one */ + if (!(groupList = virPCIDeviceGetIOMMUGroupList(dev))) + goto cleanup; + } else { + /* simple case - just reattach this one device */ + if (!(groupList = virPCIDeviceListNew()) || + !(copy = virPCIDeviceCopy(dev)) || + virPCIDeviceListAdd(groupList, copy) < 0) { + goto cleanup; + } + copy = NULL; - return 0; + if (activeDevs) { + /* If any device in the list is still in use by a + * guest, don't reattach any of them to the host. + */ + for (ii = 0; (current = virPCIDeviceListGet(groupList, ii)); ii++) { + if (virPCIDeviceListFind(activeDevs, current)) { + if (force) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Not reattaching device %s " + "because it is in the same group " + "as %s which is still in use by a " + "domain."), + dev->name, current->name); + } else { + /* if force is false, this isn't an error */ + VIR_DEBUG("Not reattaching device %s " + "because it is in the same group " + "as %s which is still in use by a domain.", + dev->name, current->name); + ret = 0; + } + goto cleanup; + } + } + } + } + + /* if we get this far, all devices in this group are free to be + * reattached to the host driver + */ + for (ii = 0; (current = virPCIDeviceListGet(groupList, ii)); ii++) { + /* reattach to the host *IFF* this device is currently bound + * to exactly the same stub driver as the requested device is + * bound to. */ + + virPCIDeviceReattachInit(current); /* won't reattach w/o these set */ + if (virPCIDeviceUnbindFromStub(current, drvName) < 0) + goto cleanup; + /* Remove reattached device from inactiveDevs */ + if (inactiveDevs) + virPCIDeviceListDel(inactiveDevs, current); + } + + ret = 0; +cleanup: + + VIR_FREE(drvPath); + VIR_FREE(drvName); + virObjectUnref(groupList); + virPCIDeviceFree(copy); + return ret; } /* Certain hypervisors (like qemu/kvm) map the PCI bar(s) on @@ -1600,6 +1794,18 @@ virPCIDeviceGetStubDriver(virPCIDevicePtr dev) return dev->stubDriver; } +void +virPCIDeviceSetAttachGroup(virPCIDevicePtr dev, bool allow) +{ + dev->attach_group = allow; +} + +bool +virPCIDeviceGetAttachGroup(virPCIDevicePtr dev) +{ + return dev->attach_group; +} + unsigned int virPCIDeviceGetUnbindFromStub(virPCIDevicePtr dev) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 972f86b..c598456 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -54,7 +54,8 @@ int virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceListPtr inactiveDevs); int virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, - virPCIDeviceListPtr inactiveDevs); + virPCIDeviceListPtr inactiveDevs, + bool force); int virPCIDeviceReset(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, virPCIDeviceListPtr inactiveDevs); @@ -65,6 +66,9 @@ unsigned int virPCIDeviceGetManaged(virPCIDevice *dev); int virPCIDeviceSetStubDriver(virPCIDevicePtr dev, const char *driver); const char *virPCIDeviceGetStubDriver(virPCIDevicePtr dev); +void virPCIDeviceSetAttachGroup(virPCIDevicePtr dev, + bool allow); +bool virPCIDeviceGetAttachGroup(virPCIDevicePtr dev); void virPCIDeviceSetUsedBy(virPCIDevice *dev, const char *used_by); const char *virPCIDeviceGetUsedBy(virPCIDevice *dev); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 6724a36..c9e4db8 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2341,7 +2341,7 @@ xenUnifiedNodeDeviceReAttach(virNodeDevicePtr dev) goto out; } - if (virPCIDeviceReattach(pci, NULL, NULL) < 0) + if (virPCIDeviceReattach(pci, NULL, NULL, false) < 0) goto out; ret = 0; -- 1.7.11.7