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

[Jonathon Jongsma]

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.

Thanks,
Jonathon