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

[Alex Williamson]

On Tue, 16 Feb 2021 16:00:40 -0600
Jonathon Jongsma <jjongsma at redhat.com> wrote:

> On Mon, 15 Feb 2021 18:22:08 +0100
> Erik Skultety <eskultet at redhat.com> 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.ljdd/ljdd_c_vfio_bind_ap.html
> 
> 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,

Alex