[libvirt] [PATCH 0/3] Add callback mech for qemu and nodedev
Add a callback mechanism for the nodedev driver (mimics the nwfilter) to allow the (c)ability to directly enumerate node devices and add/remove devices based on udev events. This is a set up of sorts for the ability to add a vHBA definition to a domain, create the vHBA, and allow the LUN's associated with the vHBA to be automagially added/removed to/from the domain. The vHBA code exists partially in some local branches I have, but I figure I need to get this part of the logic accepted. See bz: https://bugzilla.redhat.com/show_bug.cgi?id=1404964 for some "thoughts" on where this is headed. An alternative solution would require the qemu driver to handle or recognize nodedev connection events, but would require a few more calls in order to get the data needed for vHBA handling. The logic ends up being "hidden" behind a new qemu configuration variable "enumerate_nodedev" (I'm fine using a different name). The default is for the code to be disabled. The end result may be usable by the (on list) vGPU mdev code as well as a way to find/recogize an mdev create and the various "children" of the mdev that would be created (and of course deletion). But that's just me considering other uses. John Ferlan (3): nodedev: Add driver callback mechanism to add/remove devices qemu: Use nodedev callback mechanism to to get a nodedev data qemu: Add configuration variable to control nodedev enumeration src/check-aclrules.pl | 3 +- src/check-driverimpls.pl | 1 + src/conf/node_device_conf.c | 125 ++++++++++++++++++++++++++++++++++++- src/conf/node_device_conf.h | 18 ++++++ src/libvirt_private.syms | 3 + src/node_device/node_device_udev.c | 29 +++++++++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 19 ++++++ src/qemu/qemu_conf.c | 53 ++++++++++++++++ src/qemu/qemu_conf.h | 17 +++++ src/qemu/qemu_driver.c | 34 ++++++++++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 12 files changed, 302 insertions(+), 2 deletions(-) -- 2.7.4
Add a callback mechanism as a side door of sorts to the event mgmt functions that are triggered when a node_device object is added or removed. This includes a mechanism to enumerate already added devices for those stateInitialize functions called after the initial nodedevRegister is called. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/check-aclrules.pl | 3 +- src/check-driverimpls.pl | 1 + src/conf/node_device_conf.c | 125 ++++++++++++++++++++++++++++++++++++- src/conf/node_device_conf.h | 18 ++++++ src/libvirt_private.syms | 3 + src/node_device/node_device_udev.c | 29 +++++++++ 6 files changed, 177 insertions(+), 2 deletions(-) diff --git a/src/check-aclrules.pl b/src/check-aclrules.pl index 8739cda..161d954 100755 --- a/src/check-aclrules.pl +++ b/src/check-aclrules.pl @@ -243,7 +243,8 @@ while (<>) { } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) { if ($1 ne "virNWFilterCallbackDriver" && $1 ne "virNWFilterTechDriver" && - $1 ne "virDomainConfNWFilterDriver") { + $1 ne "virDomainConfNWFilterDriver" && + $1 ne "virNodeDeviceCallbackDriver") { $intable = 1; $table = $1; } diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index e320558..b707fc8 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -70,6 +70,7 @@ while (<>) { } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) { next if $1 eq "virNWFilterCallbackDriver" || $1 eq "virNWFilterTechDriver" || + $1 eq "virNodeDeviceCallbackDriver" || $1 eq "virConnectDriver"; $intable = 1; $table = $1; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4d3268f..7a4df44 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -33,6 +33,7 @@ #include "virstring.h" #include "node_device_conf.h" #include "device_conf.h" +#include "virlog.h" #include "virxml.h" #include "virbuffer.h" #include "viruuid.h" @@ -40,6 +41,8 @@ #define VIR_FROM_THIS VIR_FROM_NODEDEV +VIR_LOG_INIT("conf.node_device_conf"); + VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST, "system", "pci", @@ -263,6 +266,104 @@ void virNodeDeviceDefFree(virNodeDeviceDefPtr def) VIR_FREE(def); } + +/* Provide callback infrastructure that is expected to be called when + * devices are added/removed from a node_device subsystem that supports + * the callback mechanism. This provides a way for drivers to register + * to be notified when a node_device is added/removed. */ +virNodedevEnumerateAddDevices virNodedevEnumerateAddDevicesCb = NULL; +int nodeDeviceCallbackDriver; +#define MAX_CALLBACK_DRIVER 10 +static virNodeDeviceCallbackDriverPtr nodeDeviceDrvArray[MAX_CALLBACK_DRIVER]; + + +/** + * @cb: Driver function to be called. + * + * Set a callback at driver initialization to provide a callback to a + * driver specific function that can handle the enumeration of the existing + * devices and the addition of those devices for the registering driver. + * + * The initial node_device enumeration is done prior to other drivers, thus + * this provides a mechanism to load the existing data since the functions + * virNodeDeviceAssignDef and virNodeDeviceObjRemove would typically + * only be called when a new device is added/removed after the initial + * enumeration. The registering driver will need to handle duplication + * of data. + */ +void +virNodeDeviceConfEnumerateInit(virNodedevEnumerateAddDevices cb) +{ + virNodedevEnumerateAddDevicesCb = cb; +} + + +/** + * @cbd: Driver callback pointers to add/remove devices + * + * Register a callback function in the registering driver to be called + * when devices are added or removed. Additionally, ensure the initial + * enumeration of the devices is completed taking care to do it after + * setting the callbacks + * + * Returns 0 on success, -1 on failure + */ +int +virNodeDeviceRegisterCallbackDriver(virNodeDeviceCallbackDriverPtr cbd) +{ + if (nodeDeviceCallbackDriver >= MAX_CALLBACK_DRIVER) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no space available for another callback driver")); + return -1; + } + + if (!cbd->nodeDeviceAdd || !cbd->nodeDeviceRemove) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("callback requires both add and remove functions")); + return -1; + } + + nodeDeviceDrvArray[nodeDeviceCallbackDriver++] = cbd; + + /* Setting the add/remove callback first ensures that there is no + * window of opportunity for a device to be added after enumeration + * is complete, but before the callback is in place. So, set the + * callback first, then do the enumeration. */ + if (virNodedevEnumerateAddDevicesCb) { + if (virNodedevEnumerateAddDevicesCb(cbd->nodeDeviceAdd) < 0) { + nodeDeviceDrvArray[--nodeDeviceCallbackDriver] = NULL; + return -1; + } + } + + return 0; +} + + +/** + * @cb: Driver function to be called. + * + * Clear the callback function. It'll be up to the calling driver to + * clear it's own data properly + */ +void +virNodeDeviceUnregisterCallbackDriver(virNodeDeviceCallbackDriverPtr cbd) +{ + size_t i = 0; + + while (i < nodeDeviceCallbackDriver && nodeDeviceDrvArray[i] != cbd) + i++; + + if (i < nodeDeviceCallbackDriver) { + memmove(&nodeDeviceDrvArray[i], &nodeDeviceDrvArray[i+1], + (nodeDeviceCallbackDriver - i - 1) * + sizeof(nodeDeviceDrvArray[i])); + nodeDeviceDrvArray[i] = NULL; + nodeDeviceCallbackDriver--; + } +} + + void virNodeDeviceObjFree(virNodeDeviceObjPtr dev) { if (!dev) @@ -289,6 +390,7 @@ void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { + size_t i; virNodeDeviceObjPtr device; if ((device = virNodeDeviceFindByName(devs, def->name))) { @@ -315,6 +417,18 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, } device->def = def; + /* Call any registered drivers that want to be notified of a new device */ + for (i = 0; i < nodeDeviceCallbackDriver; i++) { + if (nodeDeviceDrvArray[i]->nodeDeviceAdd(def, false) < 0) { + VIR_DELETE_ELEMENT(devs->objs, devs->count - 1, devs->count); + virNodeDeviceObjUnlock(device); + virNodeDeviceObjFree(device); + /* NB: If we fail to remove from one driver - it's not a problem + * since adding a new device wouldn't fail if already found */ + return NULL; + } + } + return device; } @@ -322,13 +436,22 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr *dev) { - size_t i; + size_t i, j; virNodeDeviceObjUnlock(*dev); for (i = 0; i < devs->count; i++) { virNodeDeviceObjLock(*dev); if (devs->objs[i] == *dev) { + /* Call any registered drivers that want to be notified of a + * removed device */ + for (j = 0; j < nodeDeviceCallbackDriver; j++) { + if (nodeDeviceDrvArray[j]->nodeDeviceRemove((*dev)->def) < 0) { + VIR_DEBUG("failed to remove name='%s' parent='%s' from " + "callback driver, continuing", + (*dev)->def->name, (*dev)->def->parent); + } + } virNodeDeviceObjUnlock(*dev); virNodeDeviceObjFree(devs->objs[i]); *dev = NULL; diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 1634483..925cb6e 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -207,6 +207,24 @@ struct _virNodeDeviceDef { virNodeDevCapsDefPtr caps; /* optional device capabilities */ }; +/* Callback mechanism to add/remove node device's by name/parent + * to a target driver that cares to know */ +typedef int (*virNodeDeviceAdd)(virNodeDeviceDefPtr def, bool enumerate); +typedef int (*virNodeDeviceRemove)(virNodeDeviceDefPtr def); + +typedef struct _virNodeDeviceCallbackDriver virNodeDeviceCallbackDriver; +typedef virNodeDeviceCallbackDriver *virNodeDeviceCallbackDriverPtr; +struct _virNodeDeviceCallbackDriver { + const char *name; + virNodeDeviceAdd nodeDeviceAdd; + virNodeDeviceRemove nodeDeviceRemove; +}; + +typedef int (*virNodedevEnumerateAddDevices)(virNodeDeviceAdd deviceAddCb); +void virNodeDeviceConfEnumerateInit(virNodedevEnumerateAddDevices cb); + +int virNodeDeviceRegisterCallbackDriver(virNodeDeviceCallbackDriverPtr); +void virNodeDeviceUnregisterCallbackDriver(virNodeDeviceCallbackDriverPtr); typedef struct _virNodeDeviceObj virNodeDeviceObj; typedef virNodeDeviceObj *virNodeDeviceObjPtr; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d556c7d..6d11463 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -695,6 +695,7 @@ virNodeDevCapsDefFree; virNodeDevCapTypeFromString; virNodeDevCapTypeToString; virNodeDeviceAssignDef; +virNodeDeviceConfEnumerateInit; virNodeDeviceDefFormat; virNodeDeviceDefFree; virNodeDeviceDefParseFile; @@ -713,6 +714,8 @@ virNodeDeviceObjListFree; virNodeDeviceObjLock; virNodeDeviceObjRemove; virNodeDeviceObjUnlock; +virNodeDeviceRegisterCallbackDriver; +virNodeDeviceUnregisterCallbackDriver; # conf/node_device_event.h diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4b81312..ea6970b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1457,6 +1457,33 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) return 0; } + +/* + * @deviceAddCb: Callback routine for adding a device + * + * Enumerate all known devices calling the add device callback function + * + * Returns 0 on success, -1 on failure + */ +static int +udevEnumerateAddDevices(virNodeDeviceAdd deviceAddCb) +{ + size_t i; + int ret = 0; + + nodeDeviceLock(); + for (i = 0; i < driver->devs.count && ret >= 0; i++) { + virNodeDeviceObjPtr obj = driver->devs.objs[i]; + virNodeDeviceObjLock(obj); + ret = deviceAddCb(obj->def, true); + virNodeDeviceObjUnlock(obj); + } + nodeDeviceUnlock(); + + return ret; +} + + static int nodeStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) @@ -1535,6 +1562,8 @@ static int nodeStateInitialize(bool privileged, if (udevEnumerateDevices(udev) != 0) goto cleanup; + virNodeDeviceConfEnumerateInit(udevEnumerateAddDevices); + ret = 0; cleanup: -- 2.7.4
John Ferlan <jferlan@redhat.com> [2017-02-09, 04:36AM +0100]:
Add a callback mechanism as a side door of sorts to the event mgmt functions that are triggered when a node_device object is added or removed. This includes a mechanism to enumerate already added devices for those stateInitialize functions called after the initial nodedevRegister is called.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/check-aclrules.pl | 3 +- src/check-driverimpls.pl | 1 + src/conf/node_device_conf.c | 125 ++++++++++++++++++++++++++++++++++++- src/conf/node_device_conf.h | 18 ++++++ src/libvirt_private.syms | 3 + src/node_device/node_device_udev.c | 29 +++++++++ 6 files changed, 177 insertions(+), 2 deletions(-)
diff --git a/src/check-aclrules.pl b/src/check-aclrules.pl index 8739cda..161d954 100755 --- a/src/check-aclrules.pl +++ b/src/check-aclrules.pl @@ -243,7 +243,8 @@ while (<>) { } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) { if ($1 ne "virNWFilterCallbackDriver" && $1 ne "virNWFilterTechDriver" && - $1 ne "virDomainConfNWFilterDriver") { + $1 ne "virDomainConfNWFilterDriver" && + $1 ne "virNodeDeviceCallbackDriver") { $intable = 1; $table = $1; } diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index e320558..b707fc8 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -70,6 +70,7 @@ while (<>) { } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) { next if $1 eq "virNWFilterCallbackDriver" || $1 eq "virNWFilterTechDriver" || + $1 eq "virNodeDeviceCallbackDriver" || $1 eq "virConnectDriver"; $intable = 1; $table = $1; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4d3268f..7a4df44 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -33,6 +33,7 @@ #include "virstring.h" #include "node_device_conf.h" #include "device_conf.h" +#include "virlog.h" #include "virxml.h" #include "virbuffer.h" #include "viruuid.h" @@ -40,6 +41,8 @@
#define VIR_FROM_THIS VIR_FROM_NODEDEV
+VIR_LOG_INIT("conf.node_device_conf"); + VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST, "system", "pci", @@ -263,6 +266,104 @@ void virNodeDeviceDefFree(virNodeDeviceDefPtr def) VIR_FREE(def); }
+ +/* Provide callback infrastructure that is expected to be called when + * devices are added/removed from a node_device subsystem that supports + * the callback mechanism. This provides a way for drivers to register + * to be notified when a node_device is added/removed. */ +virNodedevEnumerateAddDevices virNodedevEnumerateAddDevicesCb = NULL; +int nodeDeviceCallbackDriver; +#define MAX_CALLBACK_DRIVER 10 +static virNodeDeviceCallbackDriverPtr nodeDeviceDrvArray[MAX_CALLBACK_DRIVER]; + + +/** + * @cb: Driver function to be called. + * + * Set a callback at driver initialization to provide a callback to a + * driver specific function that can handle the enumeration of the existing + * devices and the addition of those devices for the registering driver. + * + * The initial node_device enumeration is done prior to other drivers, thus + * this provides a mechanism to load the existing data since the functions + * virNodeDeviceAssignDef and virNodeDeviceObjRemove would typically + * only be called when a new device is added/removed after the initial + * enumeration. The registering driver will need to handle duplication + * of data. + */ +void +virNodeDeviceConfEnumerateInit(virNodedevEnumerateAddDevices cb) +{ + virNodedevEnumerateAddDevicesCb = cb; +} + + +/** + * @cbd: Driver callback pointers to add/remove devices + * + * Register a callback function in the registering driver to be called + * when devices are added or removed. Additionally, ensure the initial + * enumeration of the devices is completed taking care to do it after + * setting the callbacks + * + * Returns 0 on success, -1 on failure + */ +int +virNodeDeviceRegisterCallbackDriver(virNodeDeviceCallbackDriverPtr cbd) +{ + if (nodeDeviceCallbackDriver >= MAX_CALLBACK_DRIVER) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no space available for another callback driver")); + return -1; + } + + if (!cbd->nodeDeviceAdd || !cbd->nodeDeviceRemove) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("callback requires both add and remove functions")); + return -1; + } + + nodeDeviceDrvArray[nodeDeviceCallbackDriver++] = cbd; + + /* Setting the add/remove callback first ensures that there is no + * window of opportunity for a device to be added after enumeration + * is complete, but before the callback is in place. So, set the + * callback first, then do the enumeration. */ + if (virNodedevEnumerateAddDevicesCb) { + if (virNodedevEnumerateAddDevicesCb(cbd->nodeDeviceAdd) < 0) { + nodeDeviceDrvArray[--nodeDeviceCallbackDriver] = NULL; + return -1; + } + } + + return 0; +} + + +/** + * @cb: Driver function to be called. + * + * Clear the callback function. It'll be up to the calling driver to + * clear it's own data properly + */ +void +virNodeDeviceUnregisterCallbackDriver(virNodeDeviceCallbackDriverPtr cbd) +{ + size_t i = 0; + + while (i < nodeDeviceCallbackDriver && nodeDeviceDrvArray[i] != cbd) + i++; + + if (i < nodeDeviceCallbackDriver) { + memmove(&nodeDeviceDrvArray[i], &nodeDeviceDrvArray[i+1], + (nodeDeviceCallbackDriver - i - 1) * + sizeof(nodeDeviceDrvArray[i])); + nodeDeviceDrvArray[i] = NULL; + nodeDeviceCallbackDriver--; + } +} + + void virNodeDeviceObjFree(virNodeDeviceObjPtr dev) { if (!dev) @@ -289,6 +390,7 @@ void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { + size_t i; virNodeDeviceObjPtr device;
if ((device = virNodeDeviceFindByName(devs, def->name))) { @@ -315,6 +417,18 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, } device->def = def;
+ /* Call any registered drivers that want to be notified of a new device */ + for (i = 0; i < nodeDeviceCallbackDriver; i++) { + if (nodeDeviceDrvArray[i]->nodeDeviceAdd(def, false) < 0) { + VIR_DELETE_ELEMENT(devs->objs, devs->count - 1, devs->count);
I don't understand the reasoning why a failure to process the device on one listening driver leads to the complete rejection of the device in the node device driver.
+ virNodeDeviceObjUnlock(device); + virNodeDeviceObjFree(device); + /* NB: If we fail to remove from one driver - it's not a problem + * since adding a new device wouldn't fail if already found */ + return NULL; + } + } + return device;
} @@ -322,13 +436,22 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr *dev) { - size_t i; + size_t i, j;
virNodeDeviceObjUnlock(*dev);
for (i = 0; i < devs->count; i++) { virNodeDeviceObjLock(*dev); if (devs->objs[i] == *dev) { + /* Call any registered drivers that want to be notified of a + * removed device */ + for (j = 0; j < nodeDeviceCallbackDriver; j++) { + if (nodeDeviceDrvArray[j]->nodeDeviceRemove((*dev)->def) < 0) { + VIR_DEBUG("failed to remove name='%s' parent='%s' from " + "callback driver, continuing", + (*dev)->def->name, (*dev)->def->parent); + } + } virNodeDeviceObjUnlock(*dev); virNodeDeviceObjFree(devs->objs[i]); *dev = NULL; diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 1634483..925cb6e 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -207,6 +207,24 @@ struct _virNodeDeviceDef { virNodeDevCapsDefPtr caps; /* optional device capabilities */ };
+/* Callback mechanism to add/remove node device's by name/parent + * to a target driver that cares to know */ +typedef int (*virNodeDeviceAdd)(virNodeDeviceDefPtr def, bool enumerate); +typedef int (*virNodeDeviceRemove)(virNodeDeviceDefPtr def); + +typedef struct _virNodeDeviceCallbackDriver virNodeDeviceCallbackDriver; +typedef virNodeDeviceCallbackDriver *virNodeDeviceCallbackDriverPtr; +struct _virNodeDeviceCallbackDriver { + const char *name; + virNodeDeviceAdd nodeDeviceAdd; + virNodeDeviceRemove nodeDeviceRemove; +}; + +typedef int (*virNodedevEnumerateAddDevices)(virNodeDeviceAdd deviceAddCb); +void virNodeDeviceConfEnumerateInit(virNodedevEnumerateAddDevices cb); + +int virNodeDeviceRegisterCallbackDriver(virNodeDeviceCallbackDriverPtr); +void virNodeDeviceUnregisterCallbackDriver(virNodeDeviceCallbackDriverPtr);
typedef struct _virNodeDeviceObj virNodeDeviceObj; typedef virNodeDeviceObj *virNodeDeviceObjPtr; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d556c7d..6d11463 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -695,6 +695,7 @@ virNodeDevCapsDefFree; virNodeDevCapTypeFromString; virNodeDevCapTypeToString; virNodeDeviceAssignDef; +virNodeDeviceConfEnumerateInit; virNodeDeviceDefFormat; virNodeDeviceDefFree; virNodeDeviceDefParseFile; @@ -713,6 +714,8 @@ virNodeDeviceObjListFree; virNodeDeviceObjLock; virNodeDeviceObjRemove; virNodeDeviceObjUnlock; +virNodeDeviceRegisterCallbackDriver; +virNodeDeviceUnregisterCallbackDriver;
# conf/node_device_event.h diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4b81312..ea6970b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1457,6 +1457,33 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) return 0; }
+ +/* + * @deviceAddCb: Callback routine for adding a device + * + * Enumerate all known devices calling the add device callback function + * + * Returns 0 on success, -1 on failure + */ +static int +udevEnumerateAddDevices(virNodeDeviceAdd deviceAddCb) +{ + size_t i; + int ret = 0; + + nodeDeviceLock(); + for (i = 0; i < driver->devs.count && ret >= 0; i++) { + virNodeDeviceObjPtr obj = driver->devs.objs[i]; + virNodeDeviceObjLock(obj); + ret = deviceAddCb(obj->def, true); + virNodeDeviceObjUnlock(obj); + } + nodeDeviceUnlock(); + + return ret; +} + + static int nodeStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) @@ -1535,6 +1562,8 @@ static int nodeStateInitialize(bool privileged, if (udevEnumerateDevices(udev) != 0) goto cleanup;
+ virNodeDeviceConfEnumerateInit(udevEnumerateAddDevices);
I don't quite see the need for this callback indirection. What other drivers want to implement a different enumeration method for devices?
+ ret = 0;
cleanup: -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- IBM Systems Linux on z Systems & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Schönaicher Str. 220 71032 Böblingen Phone: +49 7031 16 1819 E-Mail: bwalk@de.ibm.com ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 02/09/2017 07:52 AM, Bjoern Walk wrote: [...]
virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { + size_t i; virNodeDeviceObjPtr device;
if ((device = virNodeDeviceFindByName(devs, def->name))) { @@ -315,6 +417,18 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, } device->def = def;
+ /* Call any registered drivers that want to be notified of a new device */ + for (i = 0; i < nodeDeviceCallbackDriver; i++) { + if (nodeDeviceDrvArray[i]->nodeDeviceAdd(def, false) < 0) { + VIR_DELETE_ELEMENT(devs->objs, devs->count - 1, devs->count);
I don't understand the reasoning why a failure to process the device on one listening driver leads to the complete rejection of the device in the node device driver.
That's why there's a code review ;-) If I write this without a failure, someone could query why I chose that! One can consider this like an event - failing to fail means the event isn't delivered. If there are nodedev types that rely on one another that could be a problem. For example, a scsi_target relies on a scsi_host being present. If we skip/ignore the scsi_host event, then the subsequent scsi_target event will fail to find the scsi_host and fail as well. So how does one "fix" that? That is how to "re"-enumerate. In any case, at least a way to message a failure via VIR_WARN I think would be necessary. I'm fine with not failing out.
+ virNodeDeviceObjUnlock(device); + virNodeDeviceObjFree(device); + /* NB: If we fail to remove from one driver - it's not a problem + * since adding a new device wouldn't fail if already found */ + return NULL; + } + } + return device;
[...]
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4b81312..ea6970b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1457,6 +1457,33 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) return 0; }
+ +/* + * @deviceAddCb: Callback routine for adding a device + * + * Enumerate all known devices calling the add device callback function + * + * Returns 0 on success, -1 on failure + */ +static int +udevEnumerateAddDevices(virNodeDeviceAdd deviceAddCb) +{ + size_t i; + int ret = 0; + + nodeDeviceLock(); + for (i = 0; i < driver->devs.count && ret >= 0; i++) { + virNodeDeviceObjPtr obj = driver->devs.objs[i]; + virNodeDeviceObjLock(obj); + ret = deviceAddCb(obj->def, true); + virNodeDeviceObjUnlock(obj); + } + nodeDeviceUnlock(); + + return ret; +} + + static int nodeStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) @@ -1535,6 +1562,8 @@ static int nodeStateInitialize(bool privileged, if (udevEnumerateDevices(udev) != 0) goto cleanup;
+ virNodeDeviceConfEnumerateInit(udevEnumerateAddDevices);
I don't quite see the need for this callback indirection. What other drivers want to implement a different enumeration method for devices?
I struggled with a couple of different mechanisms in order to enumerate the "existing" node devices as the node devices are discovered/added in virNodeDeviceAssignDef before qemu is started. The one thing I really needed was a way to traverse the node device objects in order to grab/pass the node device def since vHBA creation would need to know the wwnn/wwpn to search/match a domain definition with the same wwnn/wwpn. So one option was to call the virConnectListAllNodeDevices in order to get a list of all devices by name (and well parent if the other patch is ACK'd and pushed). Having the name/parent wasn't quite enough as I needed to get the virNodeDeviceDefPtr (because I'd need the wwnn/wwpn). There is a virNodeDeviceGetWWNs API, but it requires a node device ptr. Another option would be to add an API that would take a name and perform the wwnn/wwpn fetch based on that. Similar to the virConnect API - that would require having a connection pointer. So I chose this methodology which allowed the udev driver to provide the enumeration API that virNodeDeviceRegisterCallbackDriver would be able to call. But of course I see your point (now) - how would a different driver registration be able to delineate which cb API to call. Of course it solved *my* problem, but isn't quite generic enough I guess <sigh>. Going to need to think of a different mechanism I guess... Drat. Any suggestions? ;-) John
+ ret = 0;
cleanup: -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Using the nodedev driver callback mechanism - allow qemu to be notified whenever a nodedev is added/removed. Keep track of the node devices in a hash table rather than requiring a connection in order to get specific information about a node device that qemu would eventually like to keep track of. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 14 ++++++++++++++ src/qemu/qemu_driver.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6613d59..f88c7f3 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1598,3 +1598,51 @@ qemuGetDomainHupageMemPath(const virDomainDef *def, return 0; } + + +/** + * @driver: qemu driver pointer + * @def: Node device definition + * @enumeration: boolean for emumeration indication + * + * Taking the @def from the node device driver, add the device to + * the qemu node device hash table. The @enumeration is true when + * this callback is called from the node device enumeration and false + * when called when the @def was added to the node device. + * + * Returns -1 on failure, 0 on adding device name, 1 on already added name + */ +int +qemuNodeDeviceEntryAdd(virQEMUDriverPtr driver, + virNodeDeviceDefPtr def, + bool enumerate) +{ + VIR_DEBUG("Attempt to add name='%s', parent='%s' enumerate=%d", + def->name, def->parent, enumerate); + + if (!virHashLookup(driver->nodeDevices, def->name)) { + if (virHashAddEntry(driver->nodeDevices, def->name, NULL) < 0) + return -1; + return 0; + } + return 1; +} + + +/** + * @driver: qemu driver pointer + * @def: node device definition + * + * Remove the definition from qemu's node device hash table. + * + * Returns 0 on success, -1 on failure + */ +int +qemuNodeDeviceEntryRemove(virQEMUDriverPtr driver, + virNodeDeviceDefPtr def) +{ + VIR_DEBUG("Attempt to remove name='%s' parent='%s'", + def->name, def->parent); + + return virHashRemoveEntry(driver->nodeDevices, def->name); +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 92a7a50..7d514a9 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -32,6 +32,7 @@ # include "network_conf.h" # include "domain_conf.h" # include "snapshot_conf.h" +# include "node_device_conf.h" # include "domain_event.h" # include "virthread.h" # include "security/security_manager.h" @@ -253,6 +254,9 @@ struct _virQEMUDriver { virHashTablePtr sharedDevices; /* Immutable pointer, self-locking APIs */ + virHashTablePtr nodeDevices; + + /* Immutable pointer, self-locking APIs */ virPortAllocatorPtr remotePorts; /* Immutable pointer, self-locking APIs */ @@ -348,4 +352,14 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def, virQEMUDriverConfigPtr cfg, unsigned long long pagesize, char **memPath); + +void qemuNodeDeviceEntryFree(void *payload, const void *name); + +int qemuNodeDeviceEntryAdd(virQEMUDriverPtr driver, + virNodeDeviceDefPtr def, + bool enumerate); + +int qemuNodeDeviceEntryRemove(virQEMUDriverPtr driver, + virNodeDeviceDefPtr def); + #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 89bc833..758a7f5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -197,6 +197,28 @@ static virNWFilterCallbackDriver qemuCallbackDriver = { }; +static int +qemuNodeDeviceAdd(virNodeDeviceDefPtr def, + bool enumerate) +{ + return qemuNodeDeviceEntryAdd(qemu_driver, def, enumerate); +} + + +static int +qemuNodeDeviceRemove(virNodeDeviceDefPtr def) +{ + return qemuNodeDeviceEntryRemove(qemu_driver, def); +} + + +static virNodeDeviceCallbackDriver qemuNodedevCallbackDriver = { + .name = QEMU_DRIVER_NAME, + .nodeDeviceAdd = qemuNodeDeviceAdd, + .nodeDeviceRemove = qemuNodeDeviceRemove, +}; + + struct qemuAutostartData { virQEMUDriverPtr driver; virConnectPtr conn; @@ -776,6 +798,15 @@ qemuStateInitialize(bool privileged, if (!(qemu_driver->sharedDevices = virHashCreate(30, qemuSharedDeviceEntryFree))) goto error; + /* Create a hash table to keep track of node device's by name */ + if (!(qemu_driver->nodeDevices = virHashCreate(100, NULL))) + goto error; + + /* Set up a callback mechanism with the node device conf code to get + * called whenever a node device is added or removed. */ + if (virNodeDeviceRegisterCallbackDriver(&qemuNodedevCallbackDriver) < 0) + goto error; + if (qemuMigrationErrorInit(qemu_driver) < 0) goto error; -- 2.7.4
Add an 'enumerate_nodedev' qemu configuration variable to control whether the node device enumeration and add/remove event notification mechanism is enabled. This ensures only those environments that desire to utilize a domain vHBA need to have the (slight) overhead of adding the device hash table to qemu and managing add/remove events for specific scsi_host and scsi_target events. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 19 +++++++++++++++++++ src/qemu/qemu_conf.c | 5 +++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_driver.c | 17 ++++++++++------- src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index de723b2..d011f2c 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -92,6 +92,7 @@ module Libvirtd_qemu = let device_entry = bool_entry "mac_filter" | bool_entry "relaxed_acs_check" | bool_entry "allow_disk_format_probing" + | bool_entry "nodedev_enumeration" | str_entry "lock_manager" let rpc_entry = int_entry "max_queued" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index a8cd369..c726bc9 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -531,6 +531,25 @@ #allow_disk_format_probing = 1 +# In order for a properly configured qemu domain to be able to passthrough +# NPIV vHBA LUN's to the guest qemu will need to enumerate all the node +# device's and then handle the add and remove events. In order to enable +# qemu to handle this, set this parameter to 1 and restart libvirtd. This +# enables communication between qemu and udev node device event management +# to provide the functionality. When a domain is started or reconnected +# with a vHBA controller configured in the domain XML or when a vHBA +# controller is hot-plugged, the qemu driver will handle the "scsi_hostN" +# device creation and removal events by hot-(un)plugging "scsi_targetB_T_L" +# (Bus, Target, Lun) Direct-Access LUNs to/from the guest. +# +# This is not required for qemu domain environments that utilize the Storage +# Pool NPIV vHBA's to define LUNs in the domain XML. +# +# Defaults to 0. +# +#nodedev_enumeration = 1 + + # In order to prevent accidentally starting two domains that # share one writable disk, libvirt offers two approaches for # locking files. The first one is sanlock, the other one, diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f88c7f3..cfe0f84 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -347,6 +347,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) goto error; #endif + cfg->nodeDeviceEnumeration = false; + return cfg; error: @@ -707,6 +709,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "allow_disk_format_probing", &cfg->allowDiskFormatProbing) < 0) goto cleanup; + if (virConfGetValueBool(conf, "nodedev_enumeration", + &cfg->nodeDeviceEnumeration) < 0) + goto cleanup; if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 0) goto cleanup; if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 7d514a9..9e5d9ad 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -196,6 +196,9 @@ struct _virQEMUDriverConfig { virFirmwarePtr *firmwares; size_t nfirmwares; unsigned int glusterDebugLevel; + + /* Node device enumeration enabled */ + bool nodeDeviceEnumeration; }; /* Main driver state */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 758a7f5..023bb30 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -798,14 +798,17 @@ qemuStateInitialize(bool privileged, if (!(qemu_driver->sharedDevices = virHashCreate(30, qemuSharedDeviceEntryFree))) goto error; - /* Create a hash table to keep track of node device's by name */ - if (!(qemu_driver->nodeDevices = virHashCreate(100, NULL))) - goto error; + /* If node device enumeration is enabled, create a hash table to + * keep track of node device's by name and set up a callback mechanism + * with the node device conf code to get called whenever a node device + * is added or removed. */ + if (cfg->nodeDeviceEnumeration) { + if (!(qemu_driver->nodeDevices = virHashCreate(100, NULL))) + goto error; - /* Set up a callback mechanism with the node device conf code to get - * called whenever a node device is added or removed. */ - if (virNodeDeviceRegisterCallbackDriver(&qemuNodedevCallbackDriver) < 0) - goto error; + if (virNodeDeviceRegisterCallbackDriver(&qemuNodedevCallbackDriver) < 0) + goto error; + } if (qemuMigrationErrorInit(qemu_driver) < 0) goto error; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index a749f09..4c46e36 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -74,6 +74,7 @@ module Test_libvirtd_qemu = { "mac_filter" = "1" } { "relaxed_acs_check" = "1" } { "allow_disk_format_probing" = "1" } +{ "nodedev_enumeration" = "1" } { "lock_manager" = "lockd" } { "max_queued" = "0" } { "keepalive_interval" = "5" } -- 2.7.4
participants (2)
-
Bjoern Walk -
John Ferlan