On 7/7/21 11:29 PM, Jonathon Jongsma wrote:
Inactive mdevs were simply formatting their parent name as the value
received from mdevctl rather than looking up the libvirt nodedev name of
the parent device. This resulted in a parent value of e.g.
'0000:5b:00.0' instead of 'pci_0000_5b_00_0'. This prevented defining a
new mdev device from the output of nodedev-dumpxml.

Unfortunately, it's not simple to fix this comprehensively due to the
fact that mdevctl supports defining (inactive) mdevs for parent devices
that do not actually exist on the host (yet). So for those persistent
mdev definitions that do not have a valid parent in the device list, the
parent device will be set to the root "computer" device.

Unfortunately, because the value of the 'parent' field now depends on
the configuration of the host, the mdevctl parsing test will output
'computer' for all test devices. Fixing this would require a more
extensive mock test environment.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1979761

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>

fixup
---
 src/node_device/node_device_driver.c          | 20 ++++++++++++++++++-
 .../mdevctl-list-multiple.out.xml             |  8 ++++----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index b4dd57e5f4..26b0c2f032 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1051,6 +1051,7 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
     virJSONValue *props;
     virJSONValue *attrs;
     g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
+    g_autofree char *parent_sysfs_path = NULL;

     /* the child object should have a single key equal to its uuid.
      * The value is an object describing the properties of the mdev */
@@ -1060,7 +1061,24 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
     uuid = virJSONValueObjectGetKey(json, 0);
     props = virJSONValueObjectGetValue(json, 0);

-    child->parent = g_strdup(parent);
+    /* Look up id of parent device. mdevctl supports defining mdevs for parent
+     * devices that are not present on the system (to support starting mdevs on
+     * hotplug, etc) so the parent may not actually exist. */
+    parent_sysfs_path = g_strdup_printf("/sys/class/mdev_bus/%s", parent);
+    if (virFileExists(parent_sysfs_path)) {
+        g_autofree char *canon_syspath = realpath(parent_sysfs_path, NULL);
+        virNodeDeviceObj *parentobj = NULL;
+        if ((parentobj = virNodeDeviceObjListFindBySysfsPath(driver->devs,
+                                                             canon_syspath))) {
+            virNodeDeviceDef *parentdef = virNodeDeviceObjGetDef(parentobj);
+            child->parent = g_strdup(parentdef->name);
+            virNodeDeviceObjEndAPI(&parentobj);
+
+            child->parent_sysfs_path = g_steal_pointer(&canon_syspath);
+        }
+    }
+    if (!child->parent)
+        child->parent = g_strdup("computer");
     child->caps = g_new0(virNodeDevCapsDef, 1);
     child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;

diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
index cf7e966256..eead6f2a40 100644
--- a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
+++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
@@ -1,6 +1,6 @@
 <device>
   <name>mdev_200f228a_c80a_4d50_bfb7_f5a0e4e34045</name>
-  <parent>0000:00:02.0</parent>
+  <parent>computer</parent>
   <capability type='mdev'>
     <type id='i915-GVTg_V5_4'/>
     <uuid>200f228a-c80a-4d50-bfb7-f5a0e4e34045</uuid>
@@ -9,7 +9,7 @@
 </device>
 <device>
   <name>mdev_de807ffc_1923_4d5f_b6c9_b20ecebc6d4b</name>
-  <parent>0000:00:02.0</parent>
+  <parent>computer</parent>
   <capability type='mdev'>
     <type id='i915-GVTg_V5_4'/>
     <uuid>de807ffc-1923-4d5f-b6c9-b20ecebc6d4b</uuid>
@@ -18,7 +18,7 @@
 </device>
 <device>
   <name>mdev_435722ea_5f43_468a_874f_da34f1217f13</name>
-  <parent>0000:00:02.0</parent>
+  <parent>computer</parent>
   <capability type='mdev'>
     <type id='i915-GVTg_V5_8'/>
     <uuid>435722ea-5f43-468a-874f-da34f1217f13</uuid>
@@ -28,7 +28,7 @@
 </device>
 <device>
   <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name>
-  <parent>matrix</parent>
+  <parent>computer</parent>
   <capability type='mdev'>
     <type id='vfio_ap-passthrough'/>
     <uuid>783e6dbb-ea0e-411f-94e2-717eaad438bf</uuid>

Hello Jonathon,

While testing the patches, I noticed that a mdev is listed with virsh nodedev-list,
even after the mdev device is undefined and is not available in mdevctl. I have provided
below the test results for your reference.

$ lsmod | grep vfio_ap
vfio_ap                28672  0
kvm                   454656  1 vfio_ap
mdev                   24576  3 vfio_ccw,vfio_mdev,vfio_ap
vfio                   36864  5 vfio_ccw,vfio_mdev,vfio_iommu_type1,vfio_pci,vfio_ap

$ ./tools/virsh nodedev-list --all | grep ap_matrix
ap_matrix

$ ./tools/virsh nodedev-list --cap mdev --inactive

$ ./tools/virsh nodedev-define ../../mdev-inactive.xml
Node device 'mdev_9eb4280d_2752_429a_b91b_813b4a5f4598' defined from '../../mdev-inactive.xml'

$ ./tools/virsh nodedev-dumpxml mdev_9eb4280d_2752_429a_b91b_813b4a5f4598
<device>
  <name>mdev_9eb4280d_2752_429a_b91b_813b4a5f4598</name>
  <parent>css_0_0_0024</parent>
  <capability type='mdev'>
    <type id='vfio_ccw-io'/>
    <uuid>9eb4280d-2752-429a-b91b-813b4a5f4598</uuid>
    <iommuGroup number='0'/>
  </capability>
</device>

$ ./tools/virsh nodedev-list --cap mdev --inactive
mdev_9eb4280d_2752_429a_b91b_813b4a5f4598

$ ./tools/virsh nodedev-undefine mdev_9eb4280d_2752_429a_b91b_813b4a5f4598
Undefined node device 'mdev_9eb4280d_2752_429a_b91b_813b4a5f4598'

$ ./tools/virsh nodedev-list --cap mdev --inactive
mdev_9eb4280d_2752_429a_b91b_813b4a5f4598

$ mdevctl list --defined


-- 
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294