Re: [PATCH v2 1/2] Documentation/driver-api/vfio-mediated-device.rst: update for aggregation support

[Zhenyu Wang <zhenyuw@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4941 bytes --]

On 2020.03.27 06:16:11 +0000, Tian, Kevin wrote:
> > From: Zhenyu Wang
> > Sent: Thursday, March 26, 2020 4:22 PM
> > 
> > On 2020.03.26 08:17:20 +0000, Tian, Kevin wrote:
> > > > From: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > > Sent: Thursday, March 26, 2020 1:42 PM
> > > >
> > > > Update doc for mdev aggregation support. Describe mdev generic
> > > > parameter directory under mdev device directory.
> > > >
> > > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > > Cc: "Jiang, Dave" <dave.jiang@intel.com>
> > > > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > > ---
> > > >  .../driver-api/vfio-mediated-device.rst       | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> > > > b/Documentation/driver-api/vfio-mediated-device.rst
> > > > index 25eb7d5b834b..29c29432a847 100644
> > > > --- a/Documentation/driver-api/vfio-mediated-device.rst
> > > > +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > > > @@ -269,6 +269,9 @@ Directories and Files Under the sysfs for Each
> > mdev
> > > > Device
> > > >    |--- [$MDEV_UUID]
> > > >           |--- remove
> > > >           |--- mdev_type {link to its type}
> > > > +         |--- mdev [optional]
> > > > +	     |--- aggregated_instances [optional]
> > > > +	     |--- max_aggregation [optional]
> > > >           |--- vendor-specific-attributes [optional]
> > > >
> > > >  * remove (write only)
> > > > @@ -281,6 +284,22 @@ Example::
> > > >
> > > >  	# echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
> > > >
> > > > +* mdev directory (optional)
> > >
> > > It sounds confusing to me when seeing a 'mdev' directory under a
> > > mdev instance. How could one tell which attribute should put inside
> > > or outside of 'mdev'?
> > >
> > 
> > After mdev create you get uuid directory under normal device path, so
> > from that point a 'mdev' directory can just tell this is a mdev
> > device. And it's proposed by Alex before.
> 
> I didn't quite get. Isn't $MDEV_UUID plus mdev_type already tell this is
> a mdev device? If it is insufficient, then we're broken already since there
> is no such 'mdev' sub-directory before.
>

yep, I ignored mdev_type link there. The original purpose is to have a
general agreed directory to put generic parameters on mdev. 

> Alex?
> 
> > 
> > Currently only mdev core could create attribute e.g 'remove' under
> > device dir, vendor specific attrs need to be in attrs group. So 'mdev'
> > directory here tries to be optional generic interface.
> 
> I'm a bit confused. Then why cannot the new nodes exposed through
> vendor specific attributes? I may overlook previous discussion why using
> attrs group doesn't work here. ????

Vendor driver e.g vfio-ccw or future SIOV driver is free to have
custom attributes for mdev resource definition. If you choose that
way, fine it's just defined by vendor. But if utilize common mdev
parameter attributes, you get common defined resource config method
instead.

> 
> > 
> > > > +
> > > > +Vendor driver could create mdev directory to specify extra generic
> > > > parameters
> > > > +on mdev device by its type. Currently aggregation parameters are
> > defined.
> > > > +Vendor driver should provide both items to support.
> > > > +
> > > > +1) aggregated_instances (read/write)
> > > > +
> > > > +Set target aggregated instances for device. Reading will show current
> > > > +count of aggregated instances. Writing value larger than
> > max_aggregation
> > > > +would fail and return error.
> > >
> > > Can one write a value multiple-times and at any time?
> > >
> > 
> > yeah, of coz multiple times, but normally won't succeed after open.
> > 
> > > > +
> > > > +2) max_aggregation (read only)
> > > > +
> > > > +Show maxium instances for aggregation.
> > > > +
> > >
> > > "show maximum-allowed instances which can be aggregated for this
> > device". is
> > > this value static or dynamic? if dynamic then the user is expected to read
> > this
> > > field before every write. worthy of some clarification here.
> > 
> > yeah, user needs to read this before setting actual number, either static or
> > dynamic
> > depends on vendor resource type.
> 
> Then adding above information might make the description clearer.
>

Sure.

Thanks

> > > >  Mediated device Hot plug
> > > >  ------------------------
> > > >
> > > > --
> > > > 2.25.1
> > >
> > 
> > --
> > Open Source Technology Center, Intel ltd.
> > 
> > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]