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

[Jonathon Jongsma]

On Tue, 16 Feb 2021 15:20:28 -0700
Alex Williamson <alex.williamson at redhat.com> wrote:

> 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,
> 

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...

Jonathon