On Wed, Feb 03, 2021 at 11:38:53AM -0600, Jonathon Jongsma wrote:
At startup, query devices that are defined by 'mdevctl' and
add them to
the node device list.
This adds a complication: we now have two potential sources of
information for a node device:
- udev for all devices and for activated mediated devices
- mdevctl for persistent mediated devices
Unfortunately, neither backend returns full information for a mediated
device. For example, if a persistent mediated device in the list (with
information provided from mdevctl) is 'started', that same device will
now be detected by udev. If we simply overwrite the existing device
definition with the new one provided by the udev backend, we will lose
extra information that was provided by mdevctl (e.g. attributes, etc).
To avoid this, make sure to copy the extra information into the new
device definition.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 164 +++++++++++++++++++++++++++
src/node_device/node_device_driver.h | 6 +
src/node_device/node_device_udev.c | 31 ++++-
3 files changed, 196 insertions(+), 5 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 2778e41f97..fd57dcacc1 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1156,3 +1156,167 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr def,
*(def->name + i) = '_';
}
}
+
+
+static int
+virMdevctlListDefined(virNodeDeviceDefPtr **devs)
+{
+ int status;
+ g_autofree char *output = NULL;
+ g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output);
+
+ if (virCommandRun(cmd, &status) < 0)
+ return -1;
+
+ if (!output)
+ return -1;
+
+ return nodeDeviceParseMdevctlJSON(output, devs);
+}
+
+
+typedef struct _virMdevctlForEachData virMdevctlForEachData;
+struct _virMdevctlForEachData {
+ int ndefs;
+ virNodeDeviceDefPtr *defs;
+};
+
^This struct doesn't seem to be used anywhere in the patch, so please define
it in the patch that makes use of it.
+
+int
+nodeDeviceUpdateMediatedDevices(void)
+{
+ g_autofree virNodeDeviceDefPtr *defs = NULL;
+ int ndefs;
+ GList * tofree = NULL;
+ size_t i;
+
+ if ((ndefs = virMdevctlListDefined(&defs)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("failed to query mdevs from mdevctl"));
+ return -1;
+ }
+
+ for (i = 0; i < ndefs; i++) {
+ virNodeDeviceObjPtr obj;
+ virObjectEventPtr event;
+ virNodeDeviceDefPtr def = defs[i];
+ bool was_defined = false;
+
+ def->driver = g_strdup("vfio_mdev");
+
+ if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) {
^This condition should be flipped so that the 'else' block becomes the
larger one.
+ bool changed;
+ virNodeDeviceDefPtr olddef = virNodeDeviceObjGetDef(obj);
+
+ was_defined = virNodeDeviceObjIsPersistent(obj);
"defined" as a name will do just fine
+ /* Active devices contain some additional information
(e.g. sysfs
+ * path) that is not provided by mdevctl, so re-use the existing
+ * definition and copy over new mdev data */
+ changed = nodeDeviceDefCopyFromMdevctl(olddef, def);
+
+ /* Re-use the existing definition (after merging new data from
+ * mdevctl), so def needs to be freed at the end of the function */
+ tofree = g_list_prepend(tofree, def);
I'm not a fan of ^this. Plain g_autoptr on virNodeDeviceDef would do just
fine.
+
+ if (was_defined && !changed) {
+ /* if this device was already defined and the definition
+ * hasn't changed, there's nothing to do for this device */
+ virNodeDeviceObjEndAPI(&obj);
+ continue;
+ }
+ } else {
+ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) {
+ virNodeDeviceDefFree(def);
+ goto cleanup;
+ }
+ }
+
+ /* all devices returned by virMdevctlListDefined() are persistent */
+ virNodeDeviceObjSetPersistent(obj, true);
+
+ if (!was_defined)
+ event = virNodeDeviceEventLifecycleNew(def->name,
+ VIR_NODE_DEVICE_EVENT_DEFINED,
+ 0);
+ else
+ event = virNodeDeviceEventUpdateNew(def->name);
+
+ virNodeDeviceObjEndAPI(&obj);
+ virObjectEventStateQueue(driver->nodeDeviceEventState, event);
+ }
+
+ cleanup:
+ g_list_free_full(tofree, (GDestroyNotify)virNodeDeviceDefFree);
+
+ return 0;
+}
+
+
+/* returns true if any attributes were copied, else returns false */
+static bool
+virMediatedDeviceAttrsCopy(virNodeDevCapMdevPtr dst,
+ virNodeDevCapMdevPtr src)
+{
+ bool ret = false;
+ size_t i;
+
+ if (src->nattributes != dst->nattributes) {
+ ret = true;
+ for (i = 0; i < dst->nattributes; i++)
+ virMediatedDeviceAttrFree(dst->attributes[i]);
+ g_free(dst->attributes);
+
+ dst->nattributes = src->nattributes;
+ dst->attributes = g_new0(virMediatedDeviceAttrPtr,
+ src->nattributes);
+ for (i = 0; i < dst->nattributes; i++)
+ dst->attributes[i] = virMediatedDeviceAttrNew();
+ }
+
+ for (i = 0; i < src->nattributes; i++) {
+ if (STRNEQ_NULLABLE(src->attributes[i]->name,
+ dst->attributes[i]->name)) {
+ ret = true;
+ dst->attributes[i]->name =
+ g_strdup(src->attributes[i]->name);
^This could leak memory if src->attributes == dst->attributes and the
loop above therefore did not execute.
+ }
+ if (STRNEQ_NULLABLE(src->attributes[i]->value,
+ dst->attributes[i]->value)) {
+ ret = true;
+ dst->attributes[i]->value =
+ g_strdup(src->attributes[i]->value);
+ }
+ }
I think we can just straight overwrite all the data in question, yes, some CPU
cycles will be wasted, but time complexity remains at O(n) and readability
improves significantly. These are mdev devices, so I doubt this is an RT
critical feature. The function can then remain as void.
+
+ return ret;
+}
+
+
+/* A mediated device definitions from mdevctl contains additional info that is
+ * not available from udev. Transfer this data to the new definition.
+ * Returns true if anything was copied, else returns false */
+bool
+nodeDeviceDefCopyFromMdevctl(virNodeDeviceDefPtr dst,
+ virNodeDeviceDefPtr src)
+{
+ bool ret = false;
+ virNodeDevCapMdevPtr srcmdev = &src->caps->data.mdev;
+ virNodeDevCapMdevPtr dstmdev = &dst->caps->data.mdev;
+
+ if (STRNEQ_NULLABLE(dstmdev->type, srcmdev->type)) {
+ ret = true;
+ g_free(dstmdev->type);
+ dstmdev->type = g_strdup(srcmdev->type);
+ }
+
+ if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) {
+ ret = true;
+ g_free(dstmdev->uuid);
+ dstmdev->uuid = g_strdup(srcmdev->uuid);
+ }
+
+ if (virMediatedDeviceAttrsCopy(dstmdev, srcmdev))
+ ret = true;
Same as above, just overwrite all the data in question directly with those
coming from mdevctl.
+
+ return ret;
+}
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index 80ac7c5320..d8837e4ef8 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -126,8 +126,14 @@ int
nodeDeviceParseMdevctlJSON(const char *jsonstring,
virNodeDeviceDefPtr **devs);
+int
+nodeDeviceUpdateMediatedDevices(void);
+
void
nodeDeviceGenerateName(virNodeDeviceDefPtr def,
const char *subsystem,
const char *sysname,
const char *s);
+
+bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDefPtr dst,
+ virNodeDeviceDefPtr src);
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 5d1a192411..038941ec51 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1416,9 +1416,17 @@ udevRemoveOneDeviceSysPath(const char *path)
VIR_NODE_DEVICE_EVENT_DELETED,
0);
- VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
- def->name, path);
- virNodeDeviceObjListRemove(driver->devs, obj);
+ /* If the device is a mediated device that has been 'stopped', it may still
+ * be defined by mdevctl and can therefore be started again. Don't drop it
+ * from the list of node devices */
+ if (virNodeDeviceObjIsPersistent(obj)) {
+ VIR_FREE(def->sysfs_path);
+ virNodeDeviceObjSetActive(obj, false);
+ } else {
+ VIR_DEBUG("Removing device '%s' with sysfs path
'%s'",
+ def->name, path);
+ virNodeDeviceObjListRemove(driver->devs, obj);
+ }
virNodeDeviceObjEndAPI(&obj);
virObjectEventStateQueue(driver->nodeDeviceEventState, event);
@@ -1476,7 +1484,6 @@ udevSetParent(struct udev_device *device,
return 0;
}
-
static int
udevAddOneDevice(struct udev_device *device)
{
@@ -1486,6 +1493,7 @@ udevAddOneDevice(struct udev_device *device)
virObjectEventPtr event = NULL;
bool new_device = true;
int ret = -1;
+ bool was_persistent = false;
def = g_new0(virNodeDeviceDef, 1);
@@ -1509,14 +1517,23 @@ udevAddOneDevice(struct udev_device *device)
goto cleanup;
if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) {
+ objdef = virNodeDeviceObjGetDef(obj);
+
+ if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV)
+ nodeDeviceDefCopyFromMdevctl(def, objdef);
+ was_persistent = virNodeDeviceObjIsPersistent(obj);
+ /* If the device was defined by mdevctl and was never instantiated, it
+ * won't have a sysfs path. We need to emit a CREATED event... */
Why CREATED instead of DEFINED?
Erik
+ new_device = (objdef->sysfs_path == NULL);
+
virNodeDeviceObjEndAPI(&obj);
- new_device = false;
}
/* If this is a device change, the old definition will be freed
* and the current definition will take its place. */
if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
goto cleanup;
+ virNodeDeviceObjSetPersistent(obj, was_persistent);
objdef = virNodeDeviceObjGetDef(obj);
if (new_device)
@@ -1935,6 +1952,10 @@ nodeStateInitializeEnumerate(void *opaque)
/* Populate with known devices */
if (udevEnumerateDevices(udev) != 0)
goto error;
+ /* Load persistent mdevs (which might not be activated yet) and additional
+ * information about active mediated devices from mdevctl */
+ if (nodeDeviceUpdateMediatedDevices() != 0)
+ goto error;
nodeDeviceLock();
driver->initialized = true;
--
2.26.2