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