Re: [Qemu-devel] [RFC v2 4/4] docs: Add Documentation for Mediated devices

[Neo Jia <cjia@nvidia.com>]

On Fri, Sep 02, 2016 at 05:09:46PM -0500, Eric Blake wrote:
> * PGP Signed by an unknown key
> 
> On 09/02/2016 03:16 AM, Jike Song wrote:
> > From: Kirti Wankhede <kwankhede@nvidia.com>
> > 
> > Add file Documentation/vfio-mediated-device.txt that include details of
> > mediated device framework.
> > 
> > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> > Signed-off-by: Neo Jia <cjia@nvidia.com>
> > Signed-off-by: Jike Song <jike.song@intel.com>
> > ---
> >  Documentation/vfio-mediated-device.txt | 203 +++++++++++++++++++++++++++++++++
> >  1 file changed, 203 insertions(+)
> >  create mode 100644 Documentation/vfio-mediated-device.txt
> > 
> > diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt
> > new file mode 100644
> > index 0000000..39bdcd9
> > --- /dev/null
> > +++ b/Documentation/vfio-mediated-device.txt
> > @@ -0,0 +1,203 @@
> > +VFIO Mediated devices [1]
> > +-------------------------------------------------------------------------------
> 
> Many files under Documentation trim the ---- decorator to the length of
> the line above.
> 
> Also, since you have no explicit copyright/license notice, your
> documentation is under GPLv2+ per the top level.  Other files do this,
> and if you are okay with it, I won't complain; but if you intended
> something else, or even if you wanted to make it explicit rather than
> implicit, then you may want to copy the example of files that call out a
> quick blurb on copyright and licensing.
> 

Hi Eric,

Thanks for the review and really sorry about the extra email threads of this review, 
we already have one actively going on for a while starting from RFC to currently v7.

http://www.spinics.net/lists/kvm/msg137208.html

And the related latest v7 document is at:

http://www.spinics.net/lists/kvm/msg137210.html

We will address all your review comments there.

Thanks,
Neo


> > +
> > +There are more and more use cases/demands to virtualize the DMA devices which
> > +doesn't have SR_IOV capability built-in. To do this, drivers of different
> 
> s/doesn't/don't/
> 
> > +devices had to develop their own management interface and set of APIs and then
> > +integrate it to user space software. We've identified common requirements and
> > +unified management interface for such devices to make user space software
> > +integration easier.
> > +
> > +The VFIO driver framework provides unified APIs for direct device access. It is
> > +an IOMMU/device agnostic framework for exposing direct device access to
> > +user space, in a secure, IOMMU protected environment. This framework is
> > +used for multiple devices like GPUs, network adapters and compute accelerators.
> > +With direct device access, virtual machines or user space applications have
> > +direct access of physical device. This framework is reused for mediated devices.
> > +
> > +Mediated core driver provides a common interface for mediated device management
> > +that can be used by drivers of different devices. This module provides a generic
> > +interface to create/destroy mediated device, add/remove it to mediated bus
> 
> s/mediated/a mediated/ twice
> 
> > +driver, add/remove device to IOMMU group. It also provides an interface to
> 
> s/add/and add/
> s/device to/a device to an/
> 
> > +register different types of bus drivers, for example, Mediated VFIO PCI driver
> > +is designed for mediated PCI devices and supports VFIO APIs. Similarly, driver
> 
> s/driver/the driver/
> 
> > +can be designed to support any type of mediated device and added to this
> > +framework. Mediated bus driver add/delete mediated device to VFIO Group.
> 
> Missing a verb and several articles, but I'm not sure what you meant.
> Maybe:
> 
> A mediated bus driver can add/delete mediated devices to a VFIO Group.
> 
> > +
> > +Below is the high level block diagram, with NVIDIA, Intel and IBM devices
> > +as examples, since these are the devices which are going to actively use
> > +this module as of now.
> > +
> 
> > +
> > +
> > +Registration Interfaces
> > +-------------------------------------------------------------------------------
> > +
> 
> Again, rather long separator,
> 
> > +Mediated core driver provides two types of registration interfaces:
> > +
> > +1. Registration interface for mediated bus driver:
> > +-------------------------------------------------
> 
> while this seems one byte short.  I'll quit pointing it out.
> 
> > +
> > +	/**
> > +	 * struct mdev_driver [2] - Mediated device driver
> > +	 * @name: driver name
> > +	 * @probe: called when new device created
> > +	 * @remove: called when device removed
> > +	 * @driver: device driver structure
> 
> No mention of online, offline.
> 
> > +	 **/
> > +	struct mdev_driver {
> > +		const char *name;
> > +		int (*probe)(struct device *dev);
> > +		void (*remove)(struct device *dev);
> > +		int (*online)(struct device *dev);
> > +		int (*offline)(struct device *dev);
> > +		struct device_driver driver;
> > +	};
> > +
> > +
> ...
> > +
> > +For echo physical device, there is a mdev host device created, it shows in sysfs:
> > +/sys/bus/pci/devices/0000:05:00.0/mdev-host/
> 
> Did you mean 'For echoing a' (as in duplicating the device), or maybe
> 'For echoing to a' (as in duplicating data sent to the device)?  Or is
> this a typo s/echo/each/?
> 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 
> * Unknown Key
> * 0x2527436A