[libvirt PATCH 1/2] nodedev: fix parent device of inactive mdevs
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> -- 2.31.1
Allow the tree view with --all so that we can see all inactive mdevs in a tree structure nested under their parent devices. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- tools/virsh-nodedev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 8c2086b71f..5b1afe4601 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -428,8 +428,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) return false; } - if (tree && (cap_str || inactive || all)) { - vshError(ctl, "%s", _("Option --tree is incompatible with other options")); + if (tree && (cap_str || inactive)) { + vshError(ctl, "%s", _("Option --tree is incompatible with --cap and --inactive")); return false; } -- 2.31.1
On 7/7/21 11:29 PM, Jonathon Jongsma wrote:
Allow the tree view with --all so that we can see all inactive mdevs in a tree structure nested under their parent devices.
Signed-off-by: Jonathon Jongsma<jjongsma@redhat.com> Reviewed-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
-- 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
On 7/7/21 11:29 PM, Jonathon Jongsma wrote:
Allow the tree view with --all so that we can see all inactive mdevs in a tree structure nested under their parent devices.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- tools/virsh-nodedev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
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
Some observations without these patches # mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual (active) # virsh nodedev-list --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 # virsh nodedev-list --inactive --cap mdev # 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-destroy mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Destroyed node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' # virsh nodedev-list --inactive --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 QUESTION: My mdev is defined and active. I know this from looking at mdevctl. As the option inactive seems not to match the mdevctl option defined how can I find out that I can e.g. use nodedev-undefine without stopping/destroying it first? Do we need another option like defined on nodedev-list? Anyway using nodedev-dumpxml I get the parent correctly. # 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> And now the wrap up to start over again... # virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' # virsh nodedev-list --inactive --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 # virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 # virsh nodedev-list --cap mdev # mdevctl list -d # mdevctl list # 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 error: Failed to start device mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 error: internal error: Unable to create mediated device: Config for e60cef97-3f6b-485e-ac46-0520f9f66ac2 does not exist, define it first? # virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' # virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 That is definitely a bug. But in all dumped XMLs the parent seems to be provided correctly. # cat mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml <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-define mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml Node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' defined from 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml' # virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 # mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual # 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> Rerunning it with the dumpxml from the defined only mdev. # virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' # mdevctl list -d # 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> # cat mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml <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-define mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml Node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' defined from 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml' # mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual # 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> So either I misunderstood the problem you are trying to resolve or it exists on PCI only. 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>
-- 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 Fri, Jul 9, 2021 at 6:11 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
Some observations without these patches
# mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual (active)
# virsh nodedev-list --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# virsh nodedev-list --inactive --cap mdev
# 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-destroy mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Destroyed node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# virsh nodedev-list --inactive --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
QUESTION: My mdev is defined and active. I know this from looking at mdevctl. As the option inactive seems not to match the mdevctl option defined how can I find out that I can e.g. use nodedev-undefine without stopping/destroying it first? Do we need another option like defined on nodedev-list?
Anyway using nodedev-dumpxml I get the parent correctly. # 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>
And now the wrap up to start over again...
# virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# virsh nodedev-list --inactive --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# virsh nodedev-list --cap mdev
# mdevctl list -d # mdevctl list
# 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 error: Failed to start device mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 error: internal error: Unable to create mediated device: Config for e60cef97-3f6b-485e-ac46-0520f9f66ac2 does not exist, define it first?
# virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
That is definitely a bug. But in all dumped XMLs the parent seems to be provided correctly.
This looks like the bug parsing an empty list that you just submitted a patch for.
# cat mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml <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-define mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml Node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' defined from 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml'
# virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual # 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>
Rerunning it with the dumpxml from the defined only mdev.
# virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# mdevctl list -d # 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>
# cat mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml <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-define mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml Node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' defined from 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml'
# mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual
# 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>
So either I misunderstood the problem you are trying to resolve or it exists on PCI only.
Ah, this bug only manifests for inactive devices that were loaded from mdevctl without first being processed by libvirt some other way. So, in your example, the bug is not present because libvirt parsed the appropriate parent id from your XML input. Here are a couple simple ways to reproduce the bug: - define a device outside of libvirt (using mdevctl directly) and then dump the xml in libvirt. - define the device with nodedev-define, then restart the libvirt daemon so that the cached value from the XML input is forgotten. Then dump xml for the defined device. Jonathon
On 7/9/21 4:48 PM, Jonathon Jongsma wrote:
On Fri, Jul 9, 2021 at 6:11 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
Some observations without these patches
# mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual (active)
# virsh nodedev-list --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# virsh nodedev-list --inactive --cap mdev
# 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-destroy mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Destroyed node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# virsh nodedev-list --inactive --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
QUESTION: My mdev is defined and active. I know this from looking at mdevctl. As the option inactive seems not to match the mdevctl option defined how can I find out that I can e.g. use nodedev-undefine without stopping/destroying it first? Do we need another option like defined on nodedev-list?
Anyway using nodedev-dumpxml I get the parent correctly. # 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>
And now the wrap up to start over again...
# virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# virsh nodedev-list --inactive --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# virsh nodedev-list --cap mdev
# mdevctl list -d # mdevctl list
# 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 error: Failed to start device mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 error: internal error: Unable to create mediated device: Config for e60cef97-3f6b-485e-ac46-0520f9f66ac2 does not exist, define it first?
# virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
That is definitely a bug. But in all dumped XMLs the parent seems to be provided correctly.
This looks like the bug parsing an empty list that you just submitted a patch for.
Yes, but I might have broken another path with that fix... nodedev-create followed by nodedev-dumpxml
# cat mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml <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-define mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml Node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' defined from 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2.xml'
# virsh nodedev-list --all --cap mdev mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual # 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>
Rerunning it with the dumpxml from the defined only mdev.
# virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# mdevctl list -d # 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>
# cat mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml <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-define mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml Node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' defined from 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2_defined.xml'
# mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io manual
# 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>
So either I misunderstood the problem you are trying to resolve or it exists on PCI only.
Ah, this bug only manifests for inactive devices that were loaded from mdevctl without first being processed by libvirt some other way. So, in your example, the bug is not present because libvirt parsed the appropriate parent id from your XML input. Here are a couple simple ways to reproduce the bug: - define a device outside of libvirt (using mdevctl directly) and then dump the xml in libvirt. - define the device with nodedev-define, then restart the libvirt daemon so that the cached value from the XML input is forgotten. Then dump xml for the defined device.
Shalini or I will give that a spin
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
Ah, this bug only manifests for inactive devices that were loaded from mdevctl without first being processed by libvirt some other way. So, in your example, the bug is not present because libvirt parsed the appropriate parent id from your XML input. Here are a couple simple ways to reproduce the bug: - define a device outside of libvirt (using mdevctl directly) and then dump the xml in libvirt. - define the device with nodedev-define, then restart the libvirt daemon so that the cached value from the XML input is forgotten. Then dump xml for the defined device.
Shalini or I will give that a spin
Jonathon
Hello Jonathon, I tested the patches with a mdev device of type vfio_ccw, these patches fix these issues. Thank you. -- 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
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>
Hello Jonathon, I also tested the patch with vfio_ap devices. It works fine. Thank you. Reviewed-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> -- 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
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);
I wonder why syntax-check did not catch this. We prefer virFileCanonicalizePath().
+ virNodeDeviceObj *parentobj = NULL; + if ((parentobj = virNodeDeviceObjListFindBySysfsPath(driver->devs,
And we also like an empty line between these two ^^ to break the block into two.
+ 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;
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Boris Fiuczynski -
Jonathon Jongsma -
Michal Prívozník -
Shalini Chellathurai Saroja