9:33 a.m.
On Tue, Jun 22, 2021 at 2:08 AM Boris Fiuczynski <fiuczy(a)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(a)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(a)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