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

On Tue, 16 Feb 2021 15:20:28 -0700 Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > On Tue, 16 Feb 2021 16:00:40 -0600 > Jonathon Jongsma <jjongsma(a)redhat.com&gt; wrote: > > > 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. > > > > Yep, and the names don't mean anything to mdevctl either, as you say > they're just sysfs attributes, mdevctl looks for the named attribute > and writes the value to it. Note that ordering of the attributes > might be important, which is why mdevctl stores them in an array and > provides some utility to re-index attributes. I can't tell if the > above example necessarily imposes that ordering from the xml. Thanks, > Yes, the order of xml attributes is significant, they should be passed to mdevctl in the same order as they are defined in the xml. Though I don't think I've documented that anywhere...