Re: [libvirt PATCH v4 05/25] nodedev: add ability to parse mdevs from mdevctl

On Mon, 15 Feb 2021 18:22:08 +0100 Erik Skultety <eskultet(a)redhat.com&gt; wrote: > On Wed, Feb 03, 2021 at 11:38:49AM -0600, Jonathon Jongsma wrote: > > diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.json > > b/tests/nodedevmdevctldata/mdevctl-list-multiple.json new file mode > > 100644 index 0000000000..eefcd90c62 > > --- /dev/null > > +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.json > > @@ -0,0 +1,59 @@ > > +[ > > + { > > + "0000:00:02.0": [ > > + { > > + "200f228a-c80a-4d50-bfb7-f5a0e4e34045": { > > + "mdev_type": "i915-GVTg_V5_4", > > + "start": "manual" > > + } > > + }, > > + { > > + "de807ffc-1923-4d5f-b6c9-b20ecebc6d4b": { > > + "mdev_type": "i915-GVTg_V5_4", > > + "start": "auto" > > + } > > + }, > > + { > > + "435722ea-5f43-468a-874f-da34f1217f13": { > > + "mdev_type": "i915-GVTg_V5_8", > > + "start": "manual", > > + "attrs": [ > > + { > > + "testattr": "42" > > + } > > + ] > > + } > > + } > > + ] > > + }, > > + { > > + "matrix": [ > > + { "783e6dbb-ea0e-411f-94e2-717eaad438bf": { > > + "mdev_type": "vfio_ap-passthrough", > > + "start": "manual", > > + "attrs": [ > > + { > > + "assign_adapter": "5" > > + }, > > + { > > + "assign_adapter": "6" > > + }, > > I'd still like to know why there are 2 assign_adapter and 2 > assign_domain attributes here, I'm simply confused what the outcome > should be. As far as I can recall, i was just trying to use some real-world-ish mdevctl output to test the parsing and handling of mdev attributes. In this case, I believe that I simply copied the example output from the mdevctl documentation. See: https://github.com/mdevctl/mdevctl#advanced-usage-attributes-and-json > > > + { > > + "assign_domain": "0xab" > > + }, > > + { > > + "assign_control_domain": "0xab" > > + }, > > + { > > + "assign_domain": "4" > > + }, > > + { > > + "assign_control_domain": "4" > > + } > > + ] > > + } > > + } > > + ] > > + } > > +] > > + > > > ... > > > > + <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name> > > + <parent>matrix</parent> > > + <capability type='mdev'> > > + <type id='vfio_ap-passthrough'/> > > + <iommuGroup number='0'/> > > + <attr name='assign_adapter' value='5'/> > > + <attr name='assign_adapter' value='6'/> > > + <attr name='assign_domain' value='0xab'/> > > + <attr name='assign_control_domain' value='0xab'/> > > + <attr name='assign_domain' value='4'/> > > + <attr name='assign_control_domain' value='4'/> > > Here too I'd like to hear an opinion (since v3) on naming the > attributes in such way that they correspond to the respective > elements: ap-adapter, ap-domain, etc. This naming is very > unintuitive if not documented properly; unless there's an internal > reason why they have to be named "assign_control", etc. These are the names of the attributes that are used to configure these devices in sysfs: https://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.lj... The idea here is that <attr> is a direct mapping to and from the mdev sysfs attributes, since that is how these devices are configured. An attribute name means nothing to libvirt, it is just an opaque name that we pass to mdevctl. If we were to deviate from this strict mapping and add "friendlier" names in libvirt, we would need to maintain a custom per-device mapping from mdev sysfs attribute name to libvirt friendly-name. That seems unmaintainable.