On Tue, Jun 22, 2021 at 2:08 AM Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote:
>
> On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
> > On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote:
> >>
> >> On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
> >>> Implement these new API functions in the nodedev driver.
> >>>
> >>> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >>> ---
> >>> src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++
> >>> src/node_device/node_device_driver.h | 6 ++++
> >>> src/node_device/node_device_udev.c | 21 +++++++-----
> >>> 3 files changed, 69 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> >>> index 9ebe609aa4..75391f18b8 100644
> >>> --- a/src/node_device/node_device_driver.c
> >>> +++ b/src/node_device/node_device_driver.c
> >>> @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device,
> >>> virNodeDeviceObjEndAPI(&obj);
> >>> return ret;
> >>> }
> >>> +
> >>> +
> >>> +int
> >>> +nodeDeviceIsPersistent(virNodeDevice *device)
> >>> +{
> >>> + virNodeDeviceObj *obj = NULL;
> >>> + virNodeDeviceDef *def = NULL;
> >>> + int ret = -1;
> >>> +
> >>> + if (nodeDeviceInitWait() < 0)
> >>> + return -1;
> >>> +
> >>> + if (!(obj = nodeDeviceObjFindByName(device->name)))
> >>> + return -1;
> >>> + def = virNodeDeviceObjGetDef(obj);
> >>> +
> >>> + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0)
> >>> + goto cleanup;
> >>> +
> >>> + ret = virNodeDeviceObjIsPersistent(obj);
> >>> +
> >>> + cleanup:
> >>> + virNodeDeviceObjEndAPI(&obj);
> >>> + return ret;
> >>> +}
> >>> +
> >>> +
> >>> +int
> >>> +nodeDeviceIsActive(virNodeDevice *device)
> >>> +{
> >>> + virNodeDeviceObj *obj = NULL;
> >>> + virNodeDeviceDef *def = NULL;
> >>> + int ret = -1;
> >>> +
> >>> + if (nodeDeviceInitWait() < 0)
> >>> + return -1;
> >>> +
> >>> + if (!(obj = nodeDeviceObjFindByName(device->name)))
> >>> + return -1;
> >>> + def = virNodeDeviceObjGetDef(obj);
> >>> +
> >>> + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0)
> >>> + goto cleanup;
> >>> +
> >>> + ret = virNodeDeviceObjIsActive(obj);
> >>> +
> >>> + cleanup:
> >>> + virNodeDeviceObjEndAPI(&obj);
> >>> + return ret;
> >>> +}
> >>> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
> >>> index d178a18180..744dd42632 100644
> >>> --- a/src/node_device/node_device_driver.h
> >>> +++ b/src/node_device/node_device_driver.h
> >>> @@ -180,6 +180,12 @@ int
> >>> nodeDeviceGetAutostart(virNodeDevice *dev,
> >>> int *autostart);
> >>>
> >>> +int
> >>> +nodeDeviceIsPersistent(virNodeDevice *dev);
> >>> +
> >>> +int
> >>> +nodeDeviceIsActive(virNodeDevice *dev);
> >>> +
> >>> virCommand*
> >>> nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def,
> >>> bool autostart,
> >>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> >>> index 21273083a6..eb15ccce7f 100644
> >>> --- a/src/node_device/node_device_udev.c
> >>> +++ b/src/node_device/node_device_udev.c
> >>> @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device)
> >>> virObjectEvent *event = NULL;
> >>> bool new_device = true;
> >>> int ret = -1;
> >>> - bool was_persistent = false;
> >>> + bool persistent = true;
> >>> bool autostart = true;
> >>> bool is_mdev;
> >>>
> >>> @@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
> >>>
> >>> if (is_mdev)
> >>> nodeDeviceDefCopyFromMdevctl(def, objdef);
> >>> - was_persistent = virNodeDeviceObjIsPersistent(obj);
> >>> +
> >>> + persistent = virNodeDeviceObjIsPersistent(obj);
> >>> autostart = virNodeDeviceObjIsAutostart(obj);
> >>>
> >>> /* If the device was defined by mdevctl and was never instantiated, it
> >>> @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
> >>>
> >>> virNodeDeviceObjEndAPI(&obj);
> >>> } else {
> >>> - /* All non-mdev devices report themselves as autostart since they
> >>> - * should still be present and active after a reboot unless the device
> >>> - * is removed from the host. Mediated devices can only be persistent if
> >>> - * they are in already in the device list from parsing the mdevctl
> >>> - * output. */
> >>> + /* All non-mdev devices report themselves as persistent and autostart
> >>> + * since they should still be present and active after a reboot unless
> >>> + * the device is removed from the host. Mediated devices can only be
> >>> + * persistent if they are in already in the device list from parsing
> >>> + * the mdevctl output. */
> >>
> >> The assumption for all non-mdev devices ends up very misleading.
> >> For example: The parent device of an mdev needs to be bound to a vfio
> >> device driver. Without it the device ends up without the capability to
> >> create mdevs.
> >> If this driver binding is not persisted (e.g. with setting up driverctl)
> >> but instead the device is just manually being rebound to a vfio device
> >> driver than after reboot neither the mdev capability on the parent
> >> device nor the mdev device as a child device will be existing.
> >> A user calling nodedev-info before the reboot gets
> >> on the parent device
> >> Persistent: yes
> >> Autostart: yes
> >> and on the mdev device
> >> Persistent: yes
> >> Autostart: yes
> >> After a reboot he ends up with with nodedev-info
> >> on the parent device
> >> Persistent: yes
> >> Autostart: yes
> >> and the mdev device does not exist.
> >
> > Before I get to the rest of your suggestion, I'd like to know more
> > about this. If the mdev device is persistent (i.e. "defined" in
> > mdevctl terminology), then it should still exist after a reboot, even
> > if it's not started. If it doesn't, then it's a bug. An mdev can be
> > defined even if its parent device is not present.
> >
> > Does this device show up if you run 'mdevctl list --defined'?
> > Does it show up if you run `virsh nodedev-list --cap mdev --all`?
> >
> >> I suggest to use three states like yes, no, unknown
> >> or not showing Persistent and Autostart on devices that libvirt does not
> >> manage/know about their persistence and autostart configuration.
> >
> > Well, I suppose that we have the error value (-1) that could be used
> > for this case, but it doesn't exactly seem like an error. Adding a
> > separate tri-state return value would make this API inconsistent with
> > all of the other IsActive()/IsPersistent() APIs in libvirt, so I don't
> > think that's a very good idea.
> >
> > Jonathon
> >
>
> Just noticed something while playing around with this.
> The default "yes" seems to be not really a good choice even for mdevs.
> See below:
>
> # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Parent: css_0_0_0033
> Active: yes
> Persistent: yes
> Autostart: yes
>
> # virsh nodedev-destroy mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Destroyed node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
>
> # mdevctl list --defined
> e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto
>
> # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Parent: css_0_0_0033
> Active: no
> Persistent: yes
> Autostart: yes
>
> # virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> <device>
> <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name>
> <parent>css_0_0_0033</parent>
> <capability type='mdev'>
> <type id='vfio_ccw-io'/>
> <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid>
> <iommuGroup number='1'/>
> </capability>
> </device>
>
> # virsh nodedev-start mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Device mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 started
>
> # virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
>
> # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Parent: css_0_0_0033
> Active: yes
> Persistent: no
> Autostart: yes
So it appears that there is a bug where an mdev is still marked as
autostart even after it's undefined. Was there anything else you were
trying to demonstrate?
Jonathon
