On Wed, Jan 06, 2021 at 11:27:09AM +0100, Erik Skultety wrote:
> 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 <jjongsma@xxxxxxxxxx>
> > ---
> > 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;
Also, I think a one line commentary explaining why we're explicitly
updating/enumerating mdevs from mdevctl would be nice too, something like
"we don't all the data about mdevs from udev, we query mdevctl for the rest"
Erik
> > + if (nodeDeviceUpdateMediatedDevices() != 0)
> > + goto error;
> >
> > nodeDeviceLock();
> > driver->initialized = true;
> > --
> > 2.26.2
> >
>
