Implement these new API functions in the nodedev driver.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 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. */
+        persistent = !is_mdev;
         autostart = !is_mdev;
     }
 
@@ -1539,7 +1541,7 @@ udevAddOneDevice(struct udev_device *device)
      * and the current definition will take its place. */
     if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
         goto cleanup;
-    virNodeDeviceObjSetPersistent(obj, was_persistent);
+    virNodeDeviceObjSetPersistent(obj, persistent);
     virNodeDeviceObjSetAutostart(obj, autostart);
     objdef = virNodeDeviceObjGetDef(obj);
 
@@ -1945,6 +1947,7 @@ udevSetupSystemDev(void)
 
     virNodeDeviceObjSetActive(obj, true);
     virNodeDeviceObjSetAutostart(obj, true);
+    virNodeDeviceObjSetPersistent(obj, true);
 
     virNodeDeviceObjEndAPI(&obj);
 
@@ -2348,6 +2351,8 @@ static virNodeDeviceDriver udevNodeDeviceDriver = {
     .nodeDeviceCreate = nodeDeviceCreate, /* 7.3.0 */
     .nodeDeviceSetAutostart = nodeDeviceSetAutostart, /* 7.5.0 */
     .nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.5.0 */
+    .nodeDeviceIsPersistent = nodeDeviceIsPersistent, /* 7.5.0 */
+    .nodeDeviceIsActive = nodeDeviceIsActive, /* 7.5.0 */
 };
 
 
-- 
2.31.1

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@redhat.com>
> ---
>   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.

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.


> +        persistent = !is_mdev;
>           autostart = !is_mdev;
>       }
>   
> @@ -1539,7 +1541,7 @@ udevAddOneDevice(struct udev_device *device)
>        * and the current definition will take its place. */
>       if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
>           goto cleanup;
> -    virNodeDeviceObjSetPersistent(obj, was_persistent);
> +    virNodeDeviceObjSetPersistent(obj, persistent);
>       virNodeDeviceObjSetAutostart(obj, autostart);
>       objdef = virNodeDeviceObjGetDef(obj);
>   
> @@ -1945,6 +1947,7 @@ udevSetupSystemDev(void)
>   
>       virNodeDeviceObjSetActive(obj, true);
>       virNodeDeviceObjSetAutostart(obj, true);
> +    virNodeDeviceObjSetPersistent(obj, true);
>   
>       virNodeDeviceObjEndAPI(&obj);
>   
> @@ -2348,6 +2351,8 @@ static virNodeDeviceDriver udevNodeDeviceDriver = {
>       .nodeDeviceCreate = nodeDeviceCreate, /* 7.3.0 */
>       .nodeDeviceSetAutostart = nodeDeviceSetAutostart, /* 7.5.0 */
>       .nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.5.0 */
> +    .nodeDeviceIsPersistent = nodeDeviceIsPersistent, /* 7.5.0 */
> +    .nodeDeviceIsActive = nodeDeviceIsActive, /* 7.5.0 */
>   };
>   
>   
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> 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@redhat.com>
> > ---
> >   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

On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
> On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> 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@redhat.com>
>>> ---
>>>    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

# mdevctl list --defined
# mdevctl list
e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On Tue, Jun 22, 2021 at 2:08 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
>
> On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
> > On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> 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@redhat.com>
> >>> ---
> >>>    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

On 6/22/21 4:33 PM, Jonathon Jongsma wrote:
> 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

Don't you need to resync with mdevctl on nodedev-info?
If you would resync and fail to find the definition for the mdev your 
bugfix would be to set the autostart to "no". The shortcut would be to 
set autostart to "no" when undefine is called but that would leave out 
direct usage of mdectl.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On Tue, Jun 22, 2021 at 10:03 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
>
> On 6/22/21 4:33 PM, Jonathon Jongsma wrote:
> > 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
>
> Don't you need to resync with mdevctl on nodedev-info?
> If you would resync and fail to find the definition for the mdev your
> bugfix would be to set the autostart to "no". The shortcut would be to
> set autostart to "no" when undefine is called but that would leave out
> direct usage of mdectl.

Yes, it appears that there's a bug in my patch that needs to be fixed.

But your email said

> "The default "yes" seems to be not really a good choice even for mdevs.

That makes it sound like you think there's something more
fundamentally wrong than just a simple bug where a value didn't get
updated properly.

Jonathon

On 6/22/21 5:08 PM, Jonathon Jongsma wrote:
> On Tue, Jun 22, 2021 at 10:03 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
>>
>> On 6/22/21 4:33 PM, Jonathon Jongsma wrote:
>>> 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
>>
>> Don't you need to resync with mdevctl on nodedev-info?
>> If you would resync and fail to find the definition for the mdev your
>> bugfix would be to set the autostart to "no". The shortcut would be to
>> set autostart to "no" when undefine is called but that would leave out
>> direct usage of mdectl.
> 
> Yes, it appears that there's a bug in my patch that needs to be fixed.
> 
> But your email said
> 
>> "The default "yes" seems to be not really a good choice even for mdevs.
> 
> That makes it sound like you think there's something more
> fundamentally wrong than just a simple bug where a value didn't get
> updated properly.
> 
> Jonathon
> 

Is the current code resyncing with mdevctl on nodedev-info calls 
persisted and autostart? Is using mdevctl directly something libvirt 
needs to worry about with regard to data consistency?

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
> On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> 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@redhat.com>
>>> ---
>>>    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'?

Yes, the mdev definition exists.
Here is the information before the reboot with a vfio-ccw device setup 
by manually binding it to the vfio-ccw device driver. Manually means 
that I did not use driverctl to persist the device driver binding.

# 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

# mdevctl list -d
e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto (active)

# virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
<device>
   <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name>
 
<path>/sys/devices/css0/0.0.0033/e60cef97-3f6b-485e-ac46-0520f9f66ac2</path>
   <parent>css_0_0_0033</parent>
   <driver>
     <name>vfio_mdev</name>
   </driver>
   <capability type='mdev'>
     <type id='vfio_ccw-io'/>
     <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid>
     <iommuGroup number='1'/>
   </capability>
</device>

# virsh nodedev-info css_0_0_0033
Name:           css_0_0_0033
Parent:         computer
Active:         yes
Persistent:     yes
Autostart:      yes

This is the state before rebooting.
After the reboot:
# virsh nodedev-list --tree
  ...
   +- css_0_0_0033
   |   |
   |   +- ccw_0_0_c670
   |       |
   |       +- block_dasdb_IBM_750000000KMV11_c600_70
   |
   +- css_0_0_0034
   |   |
   |   +- ccw_0_0_c671
   |       |
   |       +- block_dasda_IBM_750000000KMV11_c600_71
  ...

# virsh nodedev-info css_0_0_0033
Name:           css_0_0_0033
Parent:         computer
Active:         yes
Persistent:     yes
Autostart:      yes

# virsh nodedev-info ccw_0_0_c670
Name:           ccw_0_0_c670
Parent:         css_0_0_0033
Active:         yes
Persistent:     yes
Autostart:      yes

# virsh nodedev-dumpxml css_0_0_0033
<device>
   <name>css_0_0_0033</name>
   <path>/sys/devices/css0/0.0.0033</path>
   <parent>computer</parent>
   <driver>
     <name>io_subchannel</name>
   </driver>
   <capability type='css'>
     <cssid>0x0</cssid>
     <ssid>0x0</ssid>
     <devno>0x0033</devno>
   </capability>
</device>

# virsh nodedev-list --cap mdev --all
mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2

# virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
Name:           mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
Parent:         0.0.0033
Active:         no
Persistent:     yes
Autostart:      yes

As you can see the default device driver is bound to the parent device 
now and therefore the mdev does not get created although being defined 
in mdevctl.
Also note that Persistent ond Autostart are misleading as they show on 
the mdev definition as well as the ccw device being set both "yes".

On s390 there is also cio_ignore which allows devices to be ignored. 
This can also cause that devices exist before the reboot afterwards due 
to being ignored no longer exist on the system.

For these reasons I think that these two new attributes show something 
that libvirt should not make assumptions on unless knowing about it.


> 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
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On Wed, Jun 16, 2021 at 8:30 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
>
> On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
> > On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> 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@redhat.com>
> >>> ---
> >>>    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'?
>
> Yes, the mdev definition exists.
> Here is the information before the reboot with a vfio-ccw device setup
> by manually binding it to the vfio-ccw device driver. Manually means
> that I did not use driverctl to persist the device driver binding.
>
> # 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
>
> # mdevctl list -d
> e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto (active)
>
> # virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> <device>
>    <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name>
>
> <path>/sys/devices/css0/0.0.0033/e60cef97-3f6b-485e-ac46-0520f9f66ac2</path>
>    <parent>css_0_0_0033</parent>
>    <driver>
>      <name>vfio_mdev</name>
>    </driver>
>    <capability type='mdev'>
>      <type id='vfio_ccw-io'/>
>      <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid>
>      <iommuGroup number='1'/>
>    </capability>
> </device>
>
> # virsh nodedev-info css_0_0_0033
> Name:           css_0_0_0033
> Parent:         computer
> Active:         yes
> Persistent:     yes
> Autostart:      yes
>
> This is the state before rebooting.
> After the reboot:
> # virsh nodedev-list --tree
>   ...
>    +- css_0_0_0033
>    |   |
>    |   +- ccw_0_0_c670
>    |       |
>    |       +- block_dasdb_IBM_750000000KMV11_c600_70
>    |
>    +- css_0_0_0034
>    |   |
>    |   +- ccw_0_0_c671
>    |       |
>    |       +- block_dasda_IBM_750000000KMV11_c600_71
>   ...
>
> # virsh nodedev-info css_0_0_0033
> Name:           css_0_0_0033
> Parent:         computer
> Active:         yes
> Persistent:     yes
> Autostart:      yes
>
> # virsh nodedev-info ccw_0_0_c670
> Name:           ccw_0_0_c670
> Parent:         css_0_0_0033
> Active:         yes
> Persistent:     yes
> Autostart:      yes
>
> # virsh nodedev-dumpxml css_0_0_0033
> <device>
>    <name>css_0_0_0033</name>
>    <path>/sys/devices/css0/0.0.0033</path>
>    <parent>computer</parent>
>    <driver>
>      <name>io_subchannel</name>
>    </driver>
>    <capability type='css'>
>      <cssid>0x0</cssid>
>      <ssid>0x0</ssid>
>      <devno>0x0033</devno>
>    </capability>
> </device>
>
> # virsh nodedev-list --cap mdev --all
> mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
>
> # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Name:           mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Parent:         0.0.0033
> Active:         no
> Persistent:     yes
> Autostart:      yes
>
> As you can see the default device driver is bound to the parent device
> now and therefore the mdev does not get created although being defined
> in mdevctl.
> Also note that Persistent ond Autostart are misleading as they show on
> the mdev definition as well as the ccw device being set both "yes".
>
> On s390 there is also cio_ignore which allows devices to be ignored.
> This can also cause that devices exist before the reboot afterwards due
> to being ignored no longer exist on the system.
>
> For these reasons I think that these two new attributes show something
> that libvirt should not make assumptions on unless knowing about it.
>


This is an interesting case. The vast majority of these devices will
be "persistent" for all practical purposes, because they will still be
there on the next reboot, and they will still be using the same
drivers. On the other hand, it is true that there is no record of the
device that remains after the physical device is removed, so strictly
speaking they're not persistently defined. After reflecting, I've kind
of flip-flopped and decided that maybe the best choice would be to
mark them as non-persistent (and also non-autostart?). Would there be
any downside to such a designation? They don't exactly match the
behavior of other transient objects (domains, etc) because they
(mostly) will still be present after a reboot...I still don't really
like the idea of introducing a tristate return type. Opinions from
longtime libvirt developers particularly appreciated.

Jonathon