[libvirt] [PATCH 0/7] VFIO support part 2

More patches to go on top of the 3 patches yesterday. These patches add a new API that I just learned was necessary - virNodeDeviceDetachFlags() (explanation in patches), implement it for xen and qemu, and use it in virsh. Laine Stump (7): pci: keep a stubDriver in each virPCIDevice qemu: bind/unbind stub driver according to config <driver name='x'/> hypervisor api: new virNodeDeviceDetachFlags hypervisor api: implement RPC calls for virNodeDeviceDetachFlags qemu: implement virNodeDeviceDetachFlags backend xen: implement virNodeDeviceDetachFlags backend virsh: use new virNodeDeviceDetachFlags include/libvirt/libvirt.h.in | 5 +++- src/driver.h | 6 ++++ src/libvirt.c | 69 ++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 2 ++ src/libvirt_public.syms | 5 ++++ src/qemu/qemu_driver.c | 25 ++++++++++++++-- src/qemu/qemu_hostdev.c | 22 ++++++++++++-- src/remote/remote_driver.c | 34 +++++++++++++++++++++- src/remote/remote_protocol.x | 15 ++++++++-- src/remote_protocol-structs | 6 ++++ src/util/virpci.c | 19 ++++++++++++ src/util/virpci.h | 5 +++- src/xen/xen_driver.c | 25 ++++++++++++++-- tools/virsh-nodedev.c | 28 +++++++++++++----- 14 files changed, 246 insertions(+), 20 deletions(-) -- 1.7.11.7

This can be set when the virPCIDevice is created and placed on a list, then used later when traversing the list to determine which stub driver to bind/unbind for managed devices. The existing Detach and Attach functions' signatures haven't been changed (they still accept a stub driver name in the arg list), but if the arg list has NULL for stub driver and one is available in the device's object, that will be used. (we may later deprecate and remove the arg from those functions). --- src/libvirt_private.syms | 2 ++ src/util/virpci.c | 19 +++++++++++++++++++ src/util/virpci.h | 5 ++++- 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b3f8521..bbf15f6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1604,6 +1604,7 @@ virPCIDeviceGetManaged; virPCIDeviceGetName; virPCIDeviceGetRemoveSlot; virPCIDeviceGetReprobe; +virPCIDeviceGetStubDriver; virPCIDeviceGetUnbindFromStub; virPCIDeviceGetUsedBy; virPCIDeviceIsAssignable; @@ -1623,6 +1624,7 @@ virPCIDeviceReset; virPCIDeviceSetManaged; virPCIDeviceSetRemoveSlot; virPCIDeviceSetReprobe; +virPCIDeviceSetStubDriver; virPCIDeviceSetUnbindFromStub; virPCIDeviceSetUsedBy; virPCIDeviceWaitForCleanup; diff --git a/src/util/virpci.c b/src/util/virpci.c index e58010b..73f36d0 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -66,6 +66,7 @@ struct _virPCIDevice { bool has_flr; bool has_pm_reset; bool managed; + const char *stubDriver; /* used by reattach function */ bool unbind_from_stub; @@ -1151,6 +1152,9 @@ virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceList *inactiveDevs, const char *driver) { + if (!driver && dev->stubDriver) + driver = dev->stubDriver; + if (virPCIProbeStubDriver(driver) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to load PCI stub module %s"), driver); @@ -1181,6 +1185,9 @@ virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr inactiveDevs, const char *driver) { + if (!driver && dev->stubDriver) + driver = dev->stubDriver; + if (virPCIProbeStubDriver(driver) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to load PCI stub module %s"), driver); @@ -1466,6 +1473,18 @@ virPCIDeviceGetManaged(virPCIDevicePtr dev) return dev->managed; } +void +virPCIDeviceSetStubDriver(virPCIDevicePtr dev, const char *driver) +{ + dev->stubDriver = driver; +} + +const char * +virPCIDeviceGetStubDriver(virPCIDevicePtr dev) +{ + return dev->stubDriver; +} + unsigned int virPCIDeviceGetUnbindFromStub(virPCIDevicePtr dev) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 67bee3d..db0be35 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -1,7 +1,7 @@ /* * virpci.h: helper APIs for managing host PCI devices * - * Copyright (C) 2009, 2011-2012 Red Hat, Inc. + * Copyright (C) 2009, 2011-2013 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 @@ -63,6 +63,9 @@ int virPCIDeviceReset(virPCIDevicePtr dev, void virPCIDeviceSetManaged(virPCIDevice *dev, bool managed); unsigned int virPCIDeviceGetManaged(virPCIDevice *dev); +void virPCIDeviceSetStubDriver(virPCIDevicePtr dev, + const char *driver); +const char *virPCIDeviceGetStubDriver(virPCIDevicePtr dev); void virPCIDeviceSetUsedBy(virPCIDevice *dev, const char *used_by); const char *virPCIDeviceGetUsedBy(virPCIDevice *dev); -- 1.7.11.7

On 04/24/2013 01:26 PM, Laine Stump wrote:
This can be set when the virPCIDevice is created and placed on a list, then used later when traversing the list to determine which stub driver to bind/unbind for managed devices.
The existing Detach and Attach functions' signatures haven't been changed (they still accept a stub driver name in the arg list), but if the arg list has NULL for stub driver and one is available in the device's object, that will be used. (we may later deprecate and remove the arg from those functions). --- src/libvirt_private.syms | 2 ++ src/util/virpci.c | 19 +++++++++++++++++++ src/util/virpci.h | 5 ++++- 3 files changed, 25 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

If the config for a device has specified <driver name='vfio'/>, "backend" in the pci part of the hostdev object will be set to ..._VFIO. In this case, when creating a virPCIDevice set the stubDriver to "vfio-pci", otherwise set it to "pci-stub". We will rely on the lower levels to report an error if the vfio driver isn't loaded. The detach/attach functions in virpci.c will pay attention to the stubDriver setting in the device, and bind/unbind the appropriate driver when preparing hostdevs for the domain. Note that we don't yet attempt to do anything to mark active any other devices in the same vfio "group" as a single device that is being marked active. We do need to do that, but in order to get basic VFIO functionality testing sooner rather than later, initially we'll just live with more cryptic errors when someone tries to do that. --- src/qemu/qemu_hostdev.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 308fdcd..036e922 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -67,6 +67,12 @@ qemuGetPciHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) } virPCIDeviceSetManaged(dev, hostdev->managed); + if (hostdev->source.subsys.u.pci.backend + == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_VFIO) { + virPCIDeviceSetStubDriver(dev, "vfio-pci"); + } else { + virPCIDeviceSetStubDriver(dev, "pci-stub"); + } } return list; @@ -150,6 +156,12 @@ int qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver, goto cleanup; virPCIDeviceSetManaged(dev, hostdev->managed); + if (hostdev->source.subsys.u.pci.backend + == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_VFIO) { + virPCIDeviceSetStubDriver(dev, "vfio-pci"); + } else { + virPCIDeviceSetStubDriver(dev, "pci-stub"); + } virPCIDeviceSetUsedBy(dev, def->name); /* Setup the original states for the PCI device */ @@ -472,11 +484,11 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, } } - /* Loop 2: detach managed devices */ + /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); if (virPCIDeviceGetManaged(dev) && - virPCIDeviceDetach(dev, driver->activePciHostdevs, NULL, "pci-stub") < 0) + virPCIDeviceDetach(dev, driver->activePciHostdevs, NULL, NULL) < 0) goto reattachdevs; } @@ -593,7 +605,11 @@ resetvfnetconfig: reattachdevs: for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - virPCIDeviceReattach(dev, driver->activePciHostdevs, NULL, "pci-stub"); + + /* NB: This doesn't actually re-bind to original driver, just + * unbinds from the stub driver + */ + virPCIDeviceReattach(dev, driver->activePciHostdevs, NULL, NULL); } cleanup: -- 1.7.11.7

On 04/24/2013 01:26 PM, Laine Stump wrote:
If the config for a device has specified <driver name='vfio'/>, "backend" in the pci part of the hostdev object will be set to ..._VFIO. In this case, when creating a virPCIDevice set the stubDriver to "vfio-pci", otherwise set it to "pci-stub". We will rely on the lower levels to report an error if the vfio driver isn't loaded.
The detach/attach functions in virpci.c will pay attention to the stubDriver setting in the device, and bind/unbind the appropriate driver when preparing hostdevs for the domain.
Note that we don't yet attempt to do anything to mark active any other devices in the same vfio "group" as a single device that is being marked active. We do need to do that, but in order to get basic VFIO functionality testing sooner rather than later, initially we'll just live with more cryptic errors when someone tries to do that. --- src/qemu/qemu_hostdev.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The existing virNodeDeviceDettach() assumes that there is only a single PCI device assignment backend driver appropriate for any hypervisor. This is no longer true, as the qemu driver is getting support for PCI device assigment via VFIO. The new API virNodeDeviceDetachFlags adds a driverName arg that should be set to the exact same string set in a domain <hostdev>'s <driver name='x'/> element (i.e. "vfio", "kvm", or NULL for default). It also adds a flags arg for good measure (and because it's possible we may need it when we start dealing with VFIO's "device groups"). --- include/libvirt/libvirt.h.in | 5 +++- src/driver.h | 6 ++++ src/libvirt.c | 69 ++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 84 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 518f0fe..79c74fd 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4,7 +4,7 @@ * Description: Provides the interfaces of the libvirt library to handle * virtualized domains * - * Copyright (C) 2005-2006, 2010-2012 Red Hat, Inc. + * Copyright (C) 2005-2006, 2010-2013 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 @@ -3289,6 +3289,9 @@ int virNodeDeviceRef (virNodeDevicePtr dev); int virNodeDeviceFree (virNodeDevicePtr dev); int virNodeDeviceDettach (virNodeDevicePtr dev); +int virNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags); int virNodeDeviceReAttach (virNodeDevicePtr dev); int virNodeDeviceReset (virNodeDevicePtr dev); diff --git a/src/driver.h b/src/driver.h index f6feb15..405e0a7 100644 --- a/src/driver.h +++ b/src/driver.h @@ -604,6 +604,11 @@ typedef int (*virDrvNodeDeviceDettach)(virNodeDevicePtr dev); typedef int +(*virDrvNodeDeviceDetachFlags)(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags); + +typedef int (*virDrvNodeDeviceReAttach)(virNodeDevicePtr dev); typedef int @@ -1152,6 +1157,7 @@ struct _virDriver { virDrvDomainMigratePrepare2 domainMigratePrepare2; virDrvDomainMigrateFinish2 domainMigrateFinish2; virDrvNodeDeviceDettach nodeDeviceDettach; + virDrvNodeDeviceDetachFlags nodeDeviceDetachFlags; virDrvNodeDeviceReAttach nodeDeviceReAttach; virDrvNodeDeviceReset nodeDeviceReset; virDrvDomainMigratePrepareTunnel domainMigratePrepareTunnel; diff --git a/src/libvirt.c b/src/libvirt.c index c236152..2daa0b4 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -14687,6 +14687,11 @@ virNodeDeviceRef(virNodeDevicePtr dev) * Once the device is not assigned to any guest, it may be re-attached * to the node using the virNodeDeviceReattach() method. * + * If the caller needs control over which backend driver will be used + * during PCI device assignment (to use something other than the + * default, for example VFIO), the newer virNodeDeviceDetachFlags() + * API should be used instead. + * * Returns 0 in case of success, -1 in case of failure. */ int @@ -14723,6 +14728,70 @@ error: } /** + * virNodeDeviceDetachFlags: + * @dev: pointer to the node device + * @driverName: name of backend driver that will be used + * for later device assignment to a domain. NULL + * means "use the hypervisor default driver" + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Detach the node device from the node itself so that it may be + * assigned to a guest domain. + * + * Depending on the hypervisor, this may involve operations such as + * unbinding any device drivers from the device, binding the device to + * a dummy device driver and resetting the device. Different backend + * drivers expect the device to be bound to different dummy + * devices. For example, QEMU's "kvm" backend driver (the default) + * expects the device to be bound to "pci-stub", but its "vfio" + * backend driver expects the device to be bound to "vfio-pci". + * + * If the device is currently in use by the node, this method may + * fail. + * + * Once the device is not assigned to any guest, it may be re-attached + * to the node using the virNodeDeviceReAttach() method. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags) +{ + VIR_DEBUG("dev=%p, conn=%p driverName=%s flags=%x", + dev, dev ? dev->conn : NULL, + driverName ? driverName : "(default)", flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_NODE_DEVICE(dev)) { + virLibNodeDeviceError(VIR_ERR_INVALID_NODE_DEVICE, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (dev->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (dev->conn->driver->nodeDeviceDetachFlags) { + int ret; + ret = dev->conn->driver->nodeDeviceDetachFlags(dev, driverName, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dev->conn); + return -1; +} + +/** * virNodeDeviceReAttach: * @dev: pointer to the node device * diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index ab993ed..dfbf44e 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -611,4 +611,9 @@ LIBVIRT_1.0.3 { virNodeDeviceLookupSCSIHostByWWN; } LIBVIRT_1.0.2; +LIBVIRT_1.0.5 { + global: + virNodeDeviceDetachFlags; +} LIBVIRT_1.0.3; + # .... define new API here using predicted next version number .... -- 1.7.11.7

On 04/24/2013 01:26 PM, Laine Stump wrote:
The existing virNodeDeviceDettach() assumes that there is only a single PCI device assignment backend driver appropriate for any hypervisor. This is no longer true, as the qemu driver is getting support for PCI device assigment via VFIO. The new API
s/assigment/assignment/
virNodeDeviceDetachFlags adds a driverName arg that should be set to the exact same string set in a domain <hostdev>'s <driver name='x'/> element (i.e. "vfio", "kvm", or NULL for default). It also adds a flags arg for good measure (and because it's possible we may need it when we start dealing with VFIO's "device groups"). --- include/libvirt/libvirt.h.in | 5 +++- src/driver.h | 6 ++++ src/libvirt.c | 69 ++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 84 insertions(+), 1 deletion(-)
Looks good; and we should definitely get this in before freeze if we plan on having the API as part of 1.0.5. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This requires a custom function for remoteNodeDeviceDetachFlags, because it is named *NodeDevice, but it goes through the hypervisor driver rather than nodedevice driver, and so it uses privateData instead of nodeDevicePrivateData. (It has to go through the hypervisor driver, because that is the driver that nows about the backend drivers that will perform the pci device assignment). --- src/remote/remote_driver.c | 34 +++++++++++++++++++++++++++++++++- src/remote/remote_protocol.x | 15 +++++++++++++-- src/remote_protocol-structs | 6 ++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ee7a2a4..f66304c 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2,7 +2,7 @@ * remote_driver.c: driver to provide access to libvirtd running * on a remote machine * - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 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 @@ -3405,6 +3405,37 @@ done: } static int +remoteNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags) +{ + int rv = -1; + remote_node_device_detach_flags_args args; + /* This method is unusual in that it uses the HV driver, not the + * devMon driver hence its use of privateData, instead of + * nodeDevicePrivateData + */ + struct private_data *priv = dev->conn->privateData; + + remoteDriverLock(priv); + + args.name = dev->name; + args.driverName = driverName ? (char**)&driverName : NULL; + args.flags = flags; + + if (call(dev->conn, priv, 0, REMOTE_PROC_NODE_DEVICE_DETACH_FLAGS, + (xdrproc_t) xdr_remote_node_device_detach_flags_args, + (char *) &args, (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteNodeDeviceReAttach(virNodeDevicePtr dev) { int rv = -1; @@ -6225,6 +6256,7 @@ static virDriver remote_driver = { .domainMigratePrepare2 = remoteDomainMigratePrepare2, /* 0.5.0 */ .domainMigrateFinish2 = remoteDomainMigrateFinish2, /* 0.5.0 */ .nodeDeviceDettach = remoteNodeDeviceDettach, /* 0.6.1 */ + .nodeDeviceDetachFlags = remoteNodeDeviceDetachFlags, /* 1.0.5 */ .nodeDeviceReAttach = remoteNodeDeviceReAttach, /* 0.6.1 */ .nodeDeviceReset = remoteNodeDeviceReset, /* 0.6.1 */ .domainMigratePrepareTunnel = remoteDomainMigratePrepareTunnel, /* 0.7.2 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 486d640..512ba2e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3,7 +3,7 @@ * remote_internal driver and libvirtd. This protocol is * internal and may change at any time. * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2013 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 @@ -1900,6 +1900,12 @@ struct remote_node_device_dettach_args { remote_nonnull_string name; }; +struct remote_node_device_detach_flags_args { + remote_nonnull_string name; + remote_string driverName; + unsigned int flags; +}; + struct remote_node_device_re_attach_args { remote_nonnull_string name; }; @@ -4423,6 +4429,11 @@ enum remote_procedure { /** * @generate: both */ - REMOTE_PROC_DOMAIN_MIGRATE_SET_COMPRESSION_CACHE = 300 + REMOTE_PROC_DOMAIN_MIGRATE_SET_COMPRESSION_CACHE = 300, + + /** + * @generate: server + */ + REMOTE_PROC_NODE_DEVICE_DETACH_FLAGS = 301 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 11a53a2..ea38ea2 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1440,6 +1440,11 @@ struct remote_node_device_list_caps_ret { struct remote_node_device_dettach_args { remote_nonnull_string name; }; +struct remote_node_device_detach_flags_args { + remote_nonnull_string name; + remote_string driverName; + u_int flags; +}; struct remote_node_device_re_attach_args { remote_nonnull_string name; }; @@ -2488,4 +2493,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_JOB_STATS = 298, REMOTE_PROC_DOMAIN_MIGRATE_GET_COMPRESSION_CACHE = 299, REMOTE_PROC_DOMAIN_MIGRATE_SET_COMPRESSION_CACHE = 300, + REMOTE_PROC_NODE_DEVICE_DETACH_FLAGS = 301, }; -- 1.7.11.7

On 04/24/2013 01:26 PM, Laine Stump wrote:
This requires a custom function for remoteNodeDeviceDetachFlags, because it is named *NodeDevice, but it goes through the hypervisor driver rather than nodedevice driver, and so it uses privateData instead of nodeDevicePrivateData. (It has to go through the hypervisor driver, because that is the driver that nows about the backend drivers
s/nows/knows/
that will perform the pci device assignment). --- src/remote/remote_driver.c | 34 +++++++++++++++++++++++++++++++++- src/remote/remote_protocol.x | 15 +++++++++++++-- src/remote_protocol-structs | 6 ++++++ 3 files changed, 52 insertions(+), 3 deletions(-)
static int +remoteNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags) +{ + int rv = -1; + remote_node_device_detach_flags_args args; + /* This method is unusual in that it uses the HV driver, not the + * devMon driver hence its use of privateData, instead of + * nodeDevicePrivateData + */
This comment is slightly out-of-date after commit 07a6b9aa (Dan's rename to NodeDeviceDriver); but you are copying-and-pasting from an existing problem. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The differences from virNodeDeviceDettach are very minor: 1) Check that the flags are 0. 2) Set the virPCIDevice's stubDriver according to the driverName that is passed in. 3) Call virPCIDeviceDetach with a NULL stubDriver, indicating it should get the name of the stub driver from the virPCIDevice object. --- src/qemu/qemu_driver.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba5600d..237d4de 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9741,7 +9741,9 @@ out: } static int -qemuNodeDeviceDettach(virNodeDevicePtr dev) +qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags) { virQEMUDriverPtr driver = dev->conn->privateData; virPCIDevicePtr pci; @@ -9749,6 +9751,8 @@ qemuNodeDeviceDettach(virNodeDevicePtr dev) int ret = -1; bool in_inactive_list = false; + virCheckFlags(0, -1); + if (qemuNodeDeviceGetPciInfo(dev, &domain, &bus, &slot, &function) < 0) return -1; @@ -9756,12 +9760,22 @@ qemuNodeDeviceDettach(virNodeDevicePtr dev) if (!pci) return -1; + if (!driverName || STREQ(driverName, "kvm")) { + virPCIDeviceSetStubDriver(pci, "pci-stub"); + } else if (STREQ(driverName, "vfio")) { + virPCIDeviceSetStubDriver(pci, "vfio-pci"); + } else { + virReportError(VIR_ERR_INVALID_ARG, + _("unknown driver name '%s'"), driverName); + goto out; + } + virObjectLock(driver->activePciHostdevs); virObjectLock(driver->inactivePciHostdevs); in_inactive_list = virPCIDeviceListFind(driver->inactivePciHostdevs, pci); if (virPCIDeviceDetach(pci, driver->activePciHostdevs, - driver->inactivePciHostdevs, "pci-stub") < 0) + driver->inactivePciHostdevs, NULL) < 0) goto out; ret = 0; @@ -9774,6 +9788,12 @@ out: } static int +qemuNodeDeviceDettach(virNodeDevicePtr dev) +{ + return qemuNodeDeviceDetachFlags(dev, NULL, 0); +} + +static int qemuNodeDeviceReAttach(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; @@ -14725,6 +14745,7 @@ static virDriver qemuDriver = { .domainMigratePrepare2 = qemuDomainMigratePrepare2, /* 0.5.0 */ .domainMigrateFinish2 = qemuDomainMigrateFinish2, /* 0.5.0 */ .nodeDeviceDettach = qemuNodeDeviceDettach, /* 0.6.1 */ + .nodeDeviceDetachFlags = qemuNodeDeviceDetachFlags, /* 1.0.5 */ .nodeDeviceReAttach = qemuNodeDeviceReAttach, /* 0.6.1 */ .nodeDeviceReset = qemuNodeDeviceReset, /* 0.6.1 */ .domainMigratePrepareTunnel = qemuDomainMigratePrepareTunnel, /* 0.7.2 */ -- 1.7.11.7

On 04/24/2013 01:26 PM, Laine Stump wrote:
The differences from virNodeDeviceDettach are very minor:
1) Check that the flags are 0.
2) Set the virPCIDevice's stubDriver according to the driverName that is passed in.
3) Call virPCIDeviceDetach with a NULL stubDriver, indicating it should get the name of the stub driver from the virPCIDevice object. --- src/qemu/qemu_driver.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This was the only hypervisor driver other than qemu that implemented virNodeDeviceDettach. It doesn't currently support multiple pci device assignment driver backends, but it is simple to plug in this new API, which will make it easier for Xen people to fill it in later when they decide to support VFIO (or whatever other) device assignment. Also it means that management applications will have the same API available to them for both hypervisors on any given version of libvirt. The only acceptable value for driverName in this case is NULL, since there is no alternate, and I'm not willing to pick a name for the default driver used by Xen. --- src/xen/xen_driver.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index a6990cf..8971d4c 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1,7 +1,7 @@ /* * xen_driver.c: Unified Xen driver. * - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 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 @@ -2133,12 +2133,16 @@ out: } static int -xenUnifiedNodeDeviceDettach(virNodeDevicePtr dev) +xenUnifiedNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags) { virPCIDevicePtr pci; unsigned domain, bus, slot, function; int ret = -1; + virCheckFlags(0, -1); + if (xenUnifiedNodeDeviceGetPciInfo(dev, &domain, &bus, &slot, &function) < 0) return -1; @@ -2146,7 +2150,15 @@ xenUnifiedNodeDeviceDettach(virNodeDevicePtr dev) if (!pci) return -1; - if (virPCIDeviceDetach(pci, NULL, NULL, "pciback") < 0) + if (!driverName) { + virPCIDeviceSetStubDriver(pci, "pciback"); + } else { + virReportError(VIR_ERR_INVALID_ARG, + _("unknown driver name '%s'"), driverName); + goto out; + } + + if (virPCIDeviceDetach(pci, NULL, NULL, NULL) < 0) goto out; ret = 0; @@ -2156,6 +2168,12 @@ out: } static int +xenUnifiedNodeDeviceDettach(virNodeDevicePtr dev) +{ + return xenUnifiedNodeDeviceDetachFlags(dev, NULL, 0); +} + +static int xenUnifiedNodeDeviceAssignedDomainId(virNodeDevicePtr dev) { int numdomains; @@ -2405,6 +2423,7 @@ static virDriver xenUnifiedDriver = { .connectDomainEventRegister = xenUnifiedConnectDomainEventRegister, /* 0.5.0 */ .connectDomainEventDeregister = xenUnifiedConnectDomainEventDeregister, /* 0.5.0 */ .nodeDeviceDettach = xenUnifiedNodeDeviceDettach, /* 0.6.1 */ + .nodeDeviceDetachFlags = xenUnifiedNodeDeviceDetachFlags, /* 1.0.5 */ .nodeDeviceReAttach = xenUnifiedNodeDeviceReAttach, /* 0.6.1 */ .nodeDeviceReset = xenUnifiedNodeDeviceReset, /* 0.6.1 */ .connectIsEncrypted = xenUnifiedConnectIsEncrypted, /* 0.7.3 */ -- 1.7.11.7

Laine Stump wrote:
This was the only hypervisor driver other than qemu that implemented virNodeDeviceDettach. It doesn't currently support multiple pci device assignment driver backends, but it is simple to plug in this new API, which will make it easier for Xen people to fill it in later when they decide to support VFIO (or whatever other) device assignment. Also it means that management applications will have the same API available to them for both hypervisors on any given version of libvirt.
The only acceptable value for driverName in this case is NULL, since there is no alternate, and I'm not willing to pick a name for the default driver used by Xen.
:-) ACK to the xen driver changes.
--- src/xen/xen_driver.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index a6990cf..8971d4c 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1,7 +1,7 @@ /* * xen_driver.c: Unified Xen driver. * - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 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 @@ -2133,12 +2133,16 @@ out: }
static int -xenUnifiedNodeDeviceDettach(virNodeDevicePtr dev) +xenUnifiedNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags) { virPCIDevicePtr pci; unsigned domain, bus, slot, function; int ret = -1;
+ virCheckFlags(0, -1); + if (xenUnifiedNodeDeviceGetPciInfo(dev, &domain, &bus, &slot, &function) < 0) return -1;
@@ -2146,7 +2150,15 @@ xenUnifiedNodeDeviceDettach(virNodeDevicePtr dev) if (!pci) return -1;
- if (virPCIDeviceDetach(pci, NULL, NULL, "pciback") < 0) + if (!driverName) { + virPCIDeviceSetStubDriver(pci, "pciback"); + } else { + virReportError(VIR_ERR_INVALID_ARG, + _("unknown driver name '%s'"), driverName); + goto out; + } + + if (virPCIDeviceDetach(pci, NULL, NULL, NULL) < 0) goto out;
ret = 0; @@ -2156,6 +2168,12 @@ out: }
static int +xenUnifiedNodeDeviceDettach(virNodeDevicePtr dev) +{ + return xenUnifiedNodeDeviceDetachFlags(dev, NULL, 0); +} + +static int xenUnifiedNodeDeviceAssignedDomainId(virNodeDevicePtr dev) { int numdomains; @@ -2405,6 +2423,7 @@ static virDriver xenUnifiedDriver = { .connectDomainEventRegister = xenUnifiedConnectDomainEventRegister, /* 0.5.0 */ .connectDomainEventDeregister = xenUnifiedConnectDomainEventDeregister, /* 0.5.0 */ .nodeDeviceDettach = xenUnifiedNodeDeviceDettach, /* 0.6.1 */ + .nodeDeviceDetachFlags = xenUnifiedNodeDeviceDetachFlags, /* 1.0.5 */ .nodeDeviceReAttach = xenUnifiedNodeDeviceReAttach, /* 0.6.1 */ .nodeDeviceReset = xenUnifiedNodeDeviceReset, /* 0.6.1 */ .connectIsEncrypted = xenUnifiedConnectIsEncrypted, /* 0.7.3 */

The virsh nodedev-detach command has a new --driver option. If it's given virsh will attempt to use the new virNodeDeviceDetachFlags API instead of virNodeDeviceDettach. Validation of the driver name string is left to the hypervisor (qemu accepts "kvm" or "vfio". The only other hypervisor that implements these functions is xen, and it only accepts NULL). --- tools/virsh-nodedev.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 9c0bd90..c196e7a 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1,7 +1,7 @@ /* * virsh-nodedev.c: Commands in node device group * - * Copyright (C) 2005, 2007-2012 Red Hat, Inc. + * Copyright (C) 2005, 2007-2013 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 @@ -600,6 +600,10 @@ static const vshCmdOptDef opts_node_device_detach[] = { .flags = VSH_OFLAG_REQ, .help = N_("device key") }, + {.name = "driver", + .type = VSH_OT_STRING, + .help = N_("pci device assignment backend driver (e.g. 'vfio' or 'kvm'") + }, {.name = NULL} }; @@ -607,26 +611,36 @@ static bool cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd) { const char *name = NULL; + const char *driverName = NULL; virNodeDevicePtr device; bool ret = true; if (vshCommandOptStringReq(ctl, cmd, "device", &name) < 0) return false; + ignore_value(vshCommandOptString(cmd, "driver", &driverName)); + if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { vshError(ctl, _("Could not find matching device '%s'"), name); return false; } - /* Yes, our public API is misspelled. At least virsh can accept - * either spelling. */ - if (virNodeDeviceDettach(device) == 0) { - vshPrint(ctl, _("Device %s detached\n"), name); + if (driverName) { + /* we must use the newer API that accepts a driverName */ + if (virNodeDeviceDetachFlags(device, driverName, 0) < 0) + ret = false; } else { - vshError(ctl, _("Failed to detach device %s"), name); - ret = false; + /* Yes, our (old) public API is misspelled. At least virsh + * can accept either spelling. */ + if (virNodeDeviceDettach(device) < 0) + ret = false; } + if (ret) + vshPrint(ctl, _("Device %s detached\n"), name); + else + vshError(ctl, _("Failed to detach device %s"), name); + virNodeDeviceFree(device); return ret; } -- 1.7.11.7

On 04/24/2013 01:26 PM, Laine Stump wrote:
The virsh nodedev-detach command has a new --driver option. If it's given virsh will attempt to use the new virNodeDeviceDetachFlags API instead of virNodeDeviceDettach. Validation of the driver name string is left to the hypervisor (qemu accepts "kvm" or "vfio". The only other hypervisor that implements these functions is xen, and it only accepts NULL). --- tools/virsh-nodedev.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)
- /* Yes, our public API is misspelled. At least virsh can accept - * either spelling. */ - if (virNodeDeviceDettach(device) == 0) { - vshPrint(ctl, _("Device %s detached\n"), name); + if (driverName) { + /* we must use the newer API that accepts a driverName */ + if (virNodeDeviceDetachFlags(device, driverName, 0) < 0) + ret = false; } else { - vshError(ctl, _("Failed to detach device %s"), name); - ret = false; + /* Yes, our (old) public API is misspelled. At least virsh + * can accept either spelling. */ + if (virNodeDeviceDettach(device) < 0) + ret = false;
I'm glad you fixed the spelling in the new api; I'm also glad you still added a Flags suffix instead of using the subtle change in spelling as the only distinguishing factor. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 04/24/2013 03:26 PM, Laine Stump wrote:
More patches to go on top of the 3 patches yesterday. These patches add a new API that I just learned was necessary - virNodeDeviceDetachFlags() (explanation in patches), implement it for xen and qemu, and use it in virsh.
Laine Stump (7): pci: keep a stubDriver in each virPCIDevice qemu: bind/unbind stub driver according to config <driver name='x'/> hypervisor api: new virNodeDeviceDetachFlags hypervisor api: implement RPC calls for virNodeDeviceDetachFlags qemu: implement virNodeDeviceDetachFlags backend xen: implement virNodeDeviceDetachFlags backend virsh: use new virNodeDeviceDetachFlags
I addressed Eric's concerns and pushed these 7 patches. Thanks!
participants (3)
-
Eric Blake
-
Jim Fehlig
-
Laine Stump