On Thu, Dec 24, 2020 at 08:14:31AM -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 <jjong...@redhat.com>
> ---
> src/node_device/node_device_driver.c | 76 ++++++++++++++++++++++++++++
> src/node_device/node_device_driver.h | 3 ++
> src/node_device/node_device_udev.c | 48 ++++++++++++++++++
> 3 files changed, 127 insertions(+)
>
> diff --git a/src/node_device/node_device_driver.c
> b/src/node_device/node_device_driver.c
> index 5309b8abd5..0267005af1 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -1144,3 +1144,79 @@ 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);
> +}
> +
> +
> +int
> +nodeDeviceUpdateMediatedDevices(void)
^This was called mdevctlEnumerateDevices in v2, so I'm wondering why did you
change the name? I think it was okay the way it was (I double checked that I
didn't suggest this change in v2 by accident).
> +{
> + g_autofree virNodeDeviceDefPtr *devs = NULL;
> + int ndevs;
> + size_t i;
> +
> + if ((ndevs = virMdevctlListDefined(&devs)) < 0) {
> + virReportSystemError(errno, "%s",
> + _("failed to query mdevs from mdevctl"));
> + return -1;
> + }
> +
> + for (i = 0; i < ndevs; i++) {
> + virNodeDeviceObjPtr obj;
> + virObjectEventPtr event;
> + virNodeDeviceDefPtr dev = devs[i];
> + bool new_device = true;
> +
> + dev->driver = g_strdup("vfio_mdev");
> +
> + /* If a device defined by mdevctl is already in the list, that means
> + * that it was found via the normal device discovery process and thus
> + * is already activated. Active devices contain some additional
> + * information (e.g. sysfs path) that is not provided by mdevctl, so
> + * preserve that info */
> + if ((obj = virNodeDeviceObjListFindByName(driver->devs, dev->name)))
> {
> + virNodeDeviceDefPtr olddef = virNodeDeviceObjGetDef(obj);
> +
> + /* Copy any data from the existing device */
> + dev->sysfs_path = g_strdup(olddef->sysfs_path);
> + dev->parent_sysfs_path = g_strdup(olddef->parent_sysfs_path);
> + dev->driver = g_strdup(olddef->driver);
> + dev->devnode = g_strdup(olddef->devnode);
> + dev->caps->data.mdev.iommuGroupNumber =
> olddef->caps->data.mdev.iommuGroupNumber;
So, what data other than attributes are coming from mdevctl that we have to
copy the udev provided data from the old def ^this way?
When I look at virNodeDeviceDef, If I'm not missing anything, the added value
from mdevctl are the caps, more specifically, attributes right? If that is true
I think we should revert the logic and use the function that you introduce
at the end of the patch that copies extra data and use it here as well.
To describe my thoughts into more details, at the end of the patch you add
calls to udevAddOneDevice and nodeStateInitializeEnumerate.
In the former case, we already enumerated the devices, including mdevctl, so
now when we start an mdev, we get an event from udev and what do we do? We take
that info and copy the attributes from the existing definition to it, so
the data flows more or less like udev <- mdevctl.
In the latter case though, we first enumerated udev devices, then asked mdevctl
which created a new def ptr for us and we copied the data from udev and then
redefined the device, so mdevctl <- udev.
If I'm not missing some crucial information I think the logic can be reverted
in one of those cases so that we always copy from the same source to the same
destination to apply a new definition.
> +
> + virNodeDeviceObjEndAPI(&obj);
> + new_device = false;
> + }
> +
> + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, dev))) {
> + virNodeDeviceDefFree(dev);
> + return -1;
> + }
> +
> + if (new_device)
> + event = virNodeDeviceEventLifecycleNew(dev->name,
> +
> VIR_NODE_DEVICE_EVENT_CREATED,
> + 0);
> + else
> + event = virNodeDeviceEventUpdateNew(dev->name);
> + virNodeDeviceObjEndAPI(&obj);
> + virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> + }
> +
> + return 0;
> +}
> diff --git a/src/node_device/node_device_driver.h
> b/src/node_device/node_device_driver.h
> index 80ac7c5320..4315f6d6ed 100644
> --- a/src/node_device/node_device_driver.h
> +++ b/src/node_device/node_device_driver.h
> @@ -126,6 +126,9 @@ int
> nodeDeviceParseMdevctlJSON(const char *jsonstring,
> virNodeDeviceDefPtr **devs);
>
> +int
> +nodeDeviceUpdateMediatedDevices(void);
> +
> void
> nodeDeviceGenerateName(virNodeDeviceDefPtr def,
> const char *subsystem,
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index 632413d046..223ee5a2ff 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1494,6 +1494,50 @@ udevSetParent(struct udev_device *device,
> return 0;
> }
>
> +static virMediatedDeviceAttrPtr *
> +virMediatedDeviceAttrsCopy(virMediatedDeviceAttrPtr *attrs,
> + size_t nattrs)
> +{
> + size_t i;
> + size_t j = 0;
> + g_autofree virMediatedDeviceAttrPtr *ret = NULL;
> +
> + if (nattrs == 0)
> + return NULL;
> +
> + ret = g_new0(virMediatedDeviceAttrPtr, nattrs);
> +
> + for (i = 0; i < nattrs; i++) {
> + virMediatedDeviceAttrPtr attr = virMediatedDeviceAttrNew();
> + attr->name = g_strdup(attrs[i]->name);
> + attr->value = g_strdup(attrs[i]->value);
> + VIR_APPEND_ELEMENT_INPLACE(ret, j, attr);
> + }
> +
> + return g_steal_pointer(ret);
> +}
> +
> +/* An existing device definition may have additional info from mdevctl that
> is
> + * not available from udev. Transfer this data to the new definition */
> +static void
> +nodeDeviceDefCopyExtraData(virNodeDeviceDefPtr dst,
> + virNodeDeviceDefPtr src)
^This name is too vague, we're likely going to only use it with mdevs (and if
the time comes we need to make it generic, we can easily create a wrapper).
I'd suggest nodeDeviceDefCopyFromMdevctl or something similar to make it clear
at first glance what we're trying to copy, because extra data is basically
"void *".
> +{
> + virNodeDevCapMdevPtr srcmdev;
> + virNodeDevCapMdevPtr dstmdev;
> +
> + if (dst->caps->data.type != VIR_NODE_DEV_CAP_MDEV)
> + return;
I think ^this check would be better on the caller side since we're tailoring it
to mdevs.
> +
> + srcmdev = &src->caps->data.mdev;
> + dstmdev = &dst->caps->data.mdev;
> +
> + dstmdev->persistent = srcmdev->persistent;
> + dstmdev->nattributes = srcmdev->nattributes;
> + dstmdev->attributes = virMediatedDeviceAttrsCopy(srcmdev->attributes,
> + srcmdev->nattributes);
> +}
> +
>
> static int
> udevAddOneDevice(struct udev_device *device)
> @@ -1527,6 +1571,8 @@ udevAddOneDevice(struct udev_device *device)
> goto cleanup;
>
> if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) {
> + nodeDeviceDefCopyExtraData(def, virNodeDeviceObjGetDef(obj));
...the check mentioned above should come here, it'll help the code readability
instantly to signal that we're not trying to change/copy anything unless the
device for which udev generated an event is an mdev.
Erik
> +
> virNodeDeviceObjEndAPI(&obj);
> new_device = false;
> }
> @@ -1953,6 +1999,8 @@ nodeStateInitializeEnumerate(void *opaque)
> /* Populate with known devices */
> if (udevEnumerateDevices(udev) != 0)
> goto error;
> + if (nodeDeviceUpdateMediatedDevices() != 0)
> + goto error;
>
> nodeDeviceLock();
> driver->initialized = true;
> --
> 2.26.2
>
