Bug 2143158

This can be reproduced with libvirt-8.0.0-10.module+el8.8.0+16781+9f4724c2.s390x

@jjongsma , @bfiuczyn : Could you please comment on this BZ? Is this a real bug, or is this expected behavior?

nodedev-create creates with mdevctl a transient mdev.
This means: mdevctl does not store a definition for the mdev but it just creates the mdev from json which is generated from the libvirt XML.
When you call mdevctl to list and dump the json of the mdev it is not possible for mdevctl to retrieve the attributes provided during creation time from the running mdev.

Above is the generic answer which needs to be extended if your system has also mdevctl callout scripts installed.
A callout script can enable mdevctl to provide the attributes of the running mdev as the callout script hold the mdev type specific knowledge how to retrieve the attribute data from the drivers sysfs.

As your example uses the mdev type vfio_ap-passthrough I suspect that you also have the ap-check callout script provided by s390-tools available and installed on your system.

I just recently sent a fix for mdevctl (https://github.com/mdevctl/mdevctl/pull/71) to cover transient mdevs such that callout scripts are called on mdevctl list and therefor also provide the attributes back to the libvirt nodedev driver.

It is no regression as it has never worked before.

I think it is reasonable to require nodedev-create provide the indicated attributes. This is consistent with how other 'dumpxml' commands work, e.g. virsh dumpxml provides the live domain xml on a transient domain. It's also useful because, IMO, because as it enables a user to learn which crypto devices are passed through through libvirt's API instead of switching to sysfs or logging into the guest.

With mdevctl-1.1.0-4.el9.s390x and the callout scripts I can retrieve the attributes already (in decimal instead of hexadecimal representation):

# mdevctl list -vu d36d7d0f-cf3d-4fef-bb9c-ed393954996b
d36d7d0f-cf3d-4fef-bb9c-ed393954996b matrix vfio_ap-passthrough manual
  Attrs:
    @{0}: {"assign_adapter":"2"}
    @{1}: {"assign_domain":"43"}

I just checked this with a mdevctl that contains the fix from https://github.com/mdevctl/mdevctl/pull/71 , and "mdevctl list -vu ..." also shows the attributes here - but "virsh nodedev-dumpxml ..." still ignores them.

Boris, could you please have a closer look at this?

The callout script ap-check are provided by s390-tools/s390utils.

I tested with both upstream and downstream mdevctl and ap-check versions and the result is the same for a transient device:

1. The attributes are set (I updated the title and description to reflect this)
2. mdevctl -vu <uuid> --dumpjson lists the attributes
3. nodedev-dumpxml mdev_XXX does NOT list the attributes
4. For a persisted device, the attributes are listed.

I also made sure to restart virtnodedevd before defining the device.

Unless I've done something wrong, it looks like there's a problem in how libvirt retrieves the device information for transient devices only.

Boris, could you have a look?

I can reproduce this.
It seems libvirt is calling mdevctl with the options "list --dumpjson --defined".
Therefore transient devices which only do show up with the options "list --dumpjson" are not getting any attributes in the nodedev driver.

@

(In reply to Boris Fiuczynski (IBM) from comment #7)
> I can reproduce this.
> It seems libvirt is calling mdevctl with the options "list --dumpjson
> --defined".
> Therefore transient devices which only do show up with the options "list
> --dumpjson" are not getting any attributes in the nodedev driver.
> 
> @

Pressing enter after typing @ is a bad idea. Sorry about this. Anyway...

@jjongsma: Do you have any good ideas how libvirt could fetch the active devices?
Should nodeDeviceUpdateMediatedDevice trigger a second mdevctl execution with options "list --dumpjson" and update the returned the devices in the nodedev driver?

Something like this.. (which worked for me)

Author: Boris Fiuczynski <fiuczy.com>
Date:   Tue Apr 25 17:19:57 2023 +0200

    nodedev: update transient mdevs

    Instead of updating defined mdevs only add another update for active
    devices as well to cover transient mdev devices as well.

    Signed-off-by: Boris Fiuczynski <fiuczy.com>

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 3cac25a10c..afa3ea1c95 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1636,6 +1636,24 @@ virMdevctlListDefined(virNodeDeviceDef ***devs, char **errmsg)
 }


+static int
+virMdevctlListActive(virNodeDeviceDef ***devs, char **errmsg)
+{
+    int status;
+    g_autofree char *output = NULL;
+    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(false, &output, errmsg);
+
+    if (virCommandRun(cmd, &status) < 0 || status != 0) {
+        return -1;
+    }
+
+    if (!output)
+        return -1;
+
+    return nodeDeviceParseMdevctlJSON(output, devs);
+}
+
+
 typedef struct _virMdevctlForEachData virMdevctlForEachData;
 struct _virMdevctlForEachData {
     int ndefs;
@@ -1699,6 +1717,8 @@ int
 nodeDeviceUpdateMediatedDevices(void)
 {
     g_autofree virNodeDeviceDef **defs = NULL;
+    g_autofree virNodeDeviceDef **act_defs = NULL;
+    int act_ndefs = 0;
     g_autofree char *errmsg = NULL;
     g_autofree char *mdevctl = NULL;
     virMdevctlForEachData data = { 0, };
@@ -1725,6 +1745,17 @@ nodeDeviceUpdateMediatedDevices(void)
         if (nodeDeviceUpdateMediatedDevice(defs[i]) < 0)
             return -1;

+    /* Update transient mdev devices */
+    if ((act_ndefs = virMdevctlListActive(&act_defs, &errmsg)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("failed to query mdevs from mdevctl: %1$s"), errmsg);
+        return -1;
+    }
+
+    for (i = 0; i < act_ndefs; i++)
+        if (nodeDeviceUpdateMediatedDevice(act_defs[i]) < 0)
+            return -1;
+
     return 0;
 }

Thanks Boris,

I guess that's probably a reasonable approach. Would you be willing to post that patch to the libvirt mailing list for review?

(In reply to Jonathon Jongsma from comment #10)
> Thanks Boris,
> 
> I guess that's probably a reasonable approach. Would you be willing to post
> that patch to the libvirt mailing list for review?

I will post the code above and that will fix the problem.

Aside from that:
Actually at this point we might want to revisited the discussion about the defined and active states of an mdev device.
The defined and active configuration can be different. Therefore defined and active is not just a boolean state variable but it is/can be two different configurations.
With the fix above a defined mdev configuration stored in the nodedev object is overridden with the active configuration of the mdev (unless we add code to only update the transient mdevs).
In nodedev-list there are three possibilities given by options:
 1) without any option: the active mdevs with their configuration are listed
 2) with option "inactive": the defined mdevs with their configuration are listed
 3) with option "all": the defined and active mdevs with their configurations are listed

If an mdev is active and inactive which configuration should than be listed?
The defined configuration, the active configuration or both?

Sent patch to libvirt mailing list:
https://listman.redhat.com/archives/libvir-list/2023-May/239820.html

pushed upstream as 44a0f2f0c8ff5e78c238013ed297b8fce223ac5a

Sebastian - I believe you're QE for s390x - can you also set the qa_ack+ and ITM

Tested on build:
# rpm -q mdevctl libvirt
mdevctl-1.1.0-4.el9.s390x
libvirt-9.4.0-1.el9.s390x

Test steps:
1. prepare a node device xml:
# cat vfio_ap_nodedev.xml 
<device>
	<!-- same parent device for all -->
	<parent>ap_matrix</parent>
	<capability type="mdev">
		<type id="vfio_ap-passthrough"/>
		<uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
		<attr name="assign_adapter" value="0x02"/>
		<attr name="assign_domain" value="0x002b"/>
	</capability>
</device>

2. virsh nodedev-create vfio_ap_nodedev.xml
# virsh nodedev-create vfio_ap_nodedev.xml
Node device mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix created from vfio_ap_nodedev.xml

3. virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix, and could not see attribute "assign_adapter" and "assign_domain"
# virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix
<device>
  <name>mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix</name>
  <path>/sys/devices/vfio_ap/matrix/d36d7d0f-cf3d-4fef-bb9c-ed393954996b</path>
  <parent>ap_matrix</parent>
  <driver>
    <name>vfio_ap_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ap-passthrough'/>
    <uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
    <parent_addr>matrix</parent_addr>
    <iommuGroup number='4'/>
  </capability>
</device>

4. Check the guest matrix, we could see the value is set
# cat /sys/devices/vfio_ap/matrix/d36d7d0f-cf3d-4fef-bb9c-ed393954996b/guest_matrix
02.002b

Hi Boris, this issue is still reproduced on libvirt-9.4.0-1.el9.s390x, could you help to clarify this, thx?

As you did not list it on comment 15:
Did you install the callout script for vfio_ap?

(In reply to Boris Fiuczynski (IBM) from comment #16)
> As you did not list it on comment 15:
> Did you install the callout script for vfio_ap?

Hi Boris, could you provide more info about the install step and check steps for callout script of vfio_ap?

Please read comments #3, #4 and #6.

(In reply to Boris Fiuczynski (IBM) from comment #18)
> Please read comments #3, #4 and #6.

Hi boris, I check as these things:

1. make sure s390utils related packages, mdevctl and libvirt are installed:
# rpm -qa s390utils*
s390utils-core-2.25.0-4.el9.s390x
s390utils-base-2.25.0-4.el9.s390x
s390utils-iucvterm-2.25.0-4.el9.s390x
s390utils-mon_statd-2.25.0-4.el9.s390x
s390utils-cpuplugd-2.25.0-4.el9.s390x
s390utils-osasnmpd-2.25.0-4.el9.s390x
s390utils-ziomon-2.25.0-4.el9.s390x
s390utils-cmsfs-fuse-2.25.0-4.el9.s390x
s390utils-hmcdrvfs-2.25.0-4.el9.s390x
s390utils-zdsfs-2.25.0-4.el9.s390x
s390utils-2.25.0-4.el9.s390x
s390utils-devel-2.25.0-4.el9.s390x
s390utils-cpacfstatsd-2.25.0-4.el9.s390x
s390utils-chreipl-fcp-mpath-2.25.0-4.el9.s390x

# rpm -q mdevctl
mdevctl-1.1.0-4.el9.s390x

# rpm -q libvirt qemu-kvm
libvirt-9.4.0-1.el9.s390x
qemu-kvm-8.0.0-5.el9.s390x

2. I could find mdevctl callout script at places:
# ls /etc/mdevctl.d/scripts.d/callouts/ap-check.sh 
/etc/mdevctl.d/scripts.d/callouts/ap-check.sh

# cat /etc/mdevctl.d/scripts.d/callouts/ap-check.sh
[ -e /usr/lib/mdevctl/scripts.d/callouts/ap-check ] && /usr/lib/mdevctl/scripts.d/callouts/ap-check "$@"

# ls /usr/lib/mdevctl/scripts.d/callouts/ap-check
/usr/lib/mdevctl/scripts.d/callouts/ap-check

3. Restart virtnodedevd service
# systemctl restart virtnodedevd.service

4. virsh nodedev-create vfio_ap_nodedev.xml
# cat vfio_ap_nodedev.xml 
<device>
	<!-- same parent device for all -->
	<parent>ap_matrix</parent>
	<capability type="mdev">
		<type id="vfio_ap-passthrough"/>
		<uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
		<attr name="assign_adapter" value="0x02"/>
		<attr name="assign_domain" value="0x002b"/>
	</capability>
</device>

# virsh nodedev-create vfio_ap_nodedev.xml
Node device mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix created from vfio_ap_nodedev.xml

5. virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix, and could not see attribute "assign_adapter" and "assign_domain"
# virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix
<device>
  <name>mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix</name>
  <path>/sys/devices/vfio_ap/matrix/d36d7d0f-cf3d-4fef-bb9c-ed393954996b</path>
  <parent>ap_matrix</parent>
  <driver>
    <name>vfio_ap_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ap-passthrough'/>
    <uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
    <parent_addr>matrix</parent_addr>
    <iommuGroup number='4'/>
  </capability>
</device>

6. Check the mdevctl output:
# mdevctl list -vu d36d7d0f-cf3d-4fef-bb9c-ed393954996b --dumpjson
{
  "mdev_type": "vfio_ap-passthrough",
  "start": "manual",
  "attrs": [
    {
      "assign_adapter": "2"
    },
    {
      "assign_domain": "43"
    }
  ]
}

# mdevctl list -vu d36d7d0f-cf3d-4fef-bb9c-ed393954996b
d36d7d0f-cf3d-4fef-bb9c-ed393954996b matrix vfio_ap-passthrough manual
  Attrs:
    @{0}: {"assign_adapter":"2"}
    @{1}: {"assign_domain":"43"}

7. virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix again. And still could not see attribute "assign_adapter" and "assign_domain"
# virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix
<device>
  <name>mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix</name>
  <path>/sys/devices/vfio_ap/matrix/d36d7d0f-cf3d-4fef-bb9c-ed393954996b</path>
  <parent>ap_matrix</parent>
  <driver>
    <name>vfio_ap_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ap-passthrough'/>
    <uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
    <parent_addr>matrix</parent_addr>
    <iommuGroup number='4'/>
  </capability>
</device>

8. Then I restart virtnodedevd service again
# systemctl restart virtnodedevd.service

9. Check virsh nodedev-dumpxml output, then found attribute "assign_adapter" and "assign_domain".
# virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix
<device>
  <name>mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix</name>
  <path>/sys/devices/vfio_ap/matrix/d36d7d0f-cf3d-4fef-bb9c-ed393954996b</path>
  <parent>ap_matrix</parent>
  <driver>
    <name>vfio_ap_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ap-passthrough'/>
    <uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
    <parent_addr>matrix</parent_addr>
    <iommuGroup number='4'/>
    <attr name='assign_adapter' value='2'/>
    <attr name='assign_domain' value='43'/>
  </capability>
</device>

So for above steps, after virsh nodedev-create we have to restart the virtnodedevd service then virsh nodedev-dumpxml could get the attributes we want, I am not sure this is expected, could you help to clarify it?

And I also found with steps:
1. virsh nodedev-define vfio_ap_nodedev.xml
2. virsh nodedev-start mdev_XXX
3. virsh nodedev-dumpxml mdev_XXX

the assign_adapter and assign_domain is in Hex：
# virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix
<device>
  <name>mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix</name>
  <path>/sys/devices/vfio_ap/matrix/d36d7d0f-cf3d-4fef-bb9c-ed393954996b</path>
  <parent>ap_matrix</parent>
  <driver>
    <name>vfio_ap_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ap-passthrough'/>
    <uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
    <parent_addr>matrix</parent_addr>
    <iommuGroup number='4'/>
    <attr name='assign_adapter' value='0x02'/>
    <attr name='assign_domain' value='0x002b'/>
  </capability>
</device>


with steps:
1. virsh nodedev-create vfio_ap_nodedev.xml
2. systemctl restart virtnodedevd.service

the assign_adapter and assign_domain is in Dec：
# virsh nodedev-dumpxml mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix
<device>
  <name>mdev_d36d7d0f_cf3d_4fef_bb9c_ed393954996b_matrix</name>
  <path>/sys/devices/vfio_ap/matrix/d36d7d0f-cf3d-4fef-bb9c-ed393954996b</path>
  <parent>ap_matrix</parent>
  <driver>
    <name>vfio_ap_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ap-passthrough'/>
    <uuid>d36d7d0f-cf3d-4fef-bb9c-ed393954996b</uuid>
    <parent_addr>matrix</parent_addr>
    <iommuGroup number='4'/>
    <attr name='assign_adapter' value='2'/>
    <attr name='assign_domain' value='43'/>
  </capability>
</device>

Regarding comment #19:
A restart of virtnodedevd should not be required. I have to look into the problem.

Regarding comment #20:
That is expected behavior as ap-check returns the values in decimal from the system.

To resolve the missing attributes issue after the nodedev-create (without virtnodedevd restarted) I posted this patch
https://listman.redhat.com/archives/libvir-list/2023-June/240418.html

Will the patch from comment 22 be needed for the RHEL8 related bug 2143160 ?  

If so then we should have Jonathon revisit that bug too

(In reply to Boris Fiuczynski (IBM) from comment #22)
> To resolve the missing attributes issue after the nodedev-create (without
> virtnodedevd restarted) I posted this patch
> https://listman.redhat.com/archives/libvir-list/2023-June/240418.html

I sent v2 of the patch which triggers a mdevctl update when a udev add event for a mdev device occurs.
https://listman.redhat.com/archives/libvir-list/2023-June/240563.html

(In reply to John Ferlan from comment #23)
> Will the patch from comment 22 be needed for the RHEL8 related bug 2143160 ?
> 
> 
> If so then we should have Jonathon revisit that bug too

Hi John,
yes, the additional patch (comment #24) is needed to fix the problem found during verification mentioned in comment #19.

(In reply to Boris Fiuczynski (IBM) from comment #24)
> (In reply to Boris Fiuczynski (IBM) from comment #22)
> > To resolve the missing attributes issue after the nodedev-create (without
> > virtnodedevd restarted) I posted this patch
> > https://listman.redhat.com/archives/libvir-list/2023-June/240418.html
> 
> I sent v2 of the patch which triggers a mdevctl update when a udev add event
> for a mdev device occurs.
> https://listman.redhat.com/archives/libvir-list/2023-June/240563.html

I tested this patch on Jonathon's branch https://gitlab.com/jjongsma/libvirt/-/tree/mdev-transient-attributes?ref_type=heads and it worked well, no need to restart the virtnodedevd

I confirm the attributes are listed with w/ libvirt-9.5.0-2.el9.s390x

However, I see a regression, nodedev-info with that version tells me the transient device is persistent:

# virsh nodedev-info mdev_a5272b12_2577_11ee_b7a2_0a12c58839ea_matrix
Name:           mdev_a5272b12_2577_11ee_b7a2_0a12c58839ea_matrix
Parent:         ap_matrix
Active:         yes
Persistent:     yes
Autostart:      no


Whereas with the latest version in nightly, the information is correct:
libvirt-9.3.0-2.el9.s390x

# virsh nodedev-info mdev_a5272b12_2577_11ee_b7a2_0a12c58839ea_matrix 
Name:           mdev_a5272b12_2577_11ee_b7a2_0a12c58839ea_matrix
Parent:         ap_matrix
Active:         yes
Persistent:     no
Autostart:      no

I see the regression also in the build that I created from Jonathon's branch, I'm sorry I didn't notice it back then libvirt-9.6.0-1.el9.s390x comment#27

Unfortunately, I can reproduce the regression also on the latest upstream libvirt now at commit aece25f66517a327c2a6bde4d06b432d415ed7da

Unfortunately, the regression was caused by commit 44a0f2f0. I've posted a patch upstream.

Automated test https://github.com/autotest/tp-libvirt/pull/5040

Pre-verified with libvirt-9.5.0-3.el9.s390x

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Moderate: libvirt security, bug fix, and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2023:6409