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

2016-09-02 Thread Neo Jia
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 
> > 
> > Add file Documentation/vfio-mediated-device.txt that include details of
> > mediated device framework.
> > 
> > Signed-off-by: Kirti Wankhede 
> > Signed-off-by: Neo Jia 
> > Signed-off-by: Jike Song 
> > ---
> >  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 000..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 

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

2016-09-02 Thread Eric Blake
On 09/02/2016 03:16 AM, Jike Song wrote:
> From: Kirti Wankhede 
> 
> Add file Documentation/vfio-mediated-device.txt that include details of
> mediated device framework.
> 
> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> Signed-off-by: Jike Song 
> ---
>  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 000..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.

> +
> +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/: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



signature.asc
Description: OpenPGP digital signature


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

2016-09-02 Thread Jike Song
From: Kirti Wankhede 

Add file Documentation/vfio-mediated-device.txt that include details of
mediated device framework.

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
Signed-off-by: Jike Song 
---
 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 000..39bdcd9
--- /dev/null
+++ b/Documentation/vfio-mediated-device.txt
@@ -0,0 +1,203 @@
+VFIO Mediated devices [1]
+---
+
+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
+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
+driver, add/remove device to IOMMU group. It also provides an interface to
+register different types of bus drivers, for example, Mediated VFIO PCI driver
+is designed for mediated PCI devices and supports VFIO APIs. Similarly, 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.
+
+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.
+
+
+ +---+
+ |   |
+ | +---+ |
+ | |   | |
+ | |   | |
+ | |  mdev | |mdev_register_driver() +---+
+ | |  bus  | |<--+   |
+ | |  driver   | |   |   |
+ | |   | +-->| vfio_mdev.ko  |<-> VFIO 
user
+ | |   | | probe()/remove()  |   |APIs
+ | |   | |   +---+
+ | |   | |
+ | +---+ |
+ |   |
+ |  MDEV CORE|
+ |   MODULE  |
+ |   mdev.ko |
+ |   |
+ | +---+ |  mdev_register_host_device()  +---+
+ | |   | +<--+   |
+ | |   | |   |  nvidia.ko|<-> 
physical
+ | |   | +-->+   |
device
+ | |   | |callbacks  +---+
+ | |   | |
+ | | Physical  | |  mdev_register_host_device()  +---+
+ | | Device| |<--+   |
+ | | Interface | |   |  i915.ko  |<-> 
physical
+ | |   | +-->+   |
device
+ | |   | |callbacks  +---+
+ | |   | |
+ | |   | |  mdev_register_host_device()  +---+
+ | |   | +<--+   |
+ | |   | |   | ccw_device.ko |<-> 
physical
+ | |   | +-->+   |
device
+ | |   | |callbacks  +---+
+ | +---+ |
+ +---+
+
+
+Registration Interfaces
+---
+
+Mediated core driver provides two types of registration interfaces:
+
+1. Registration interface for mediated bus driver:
+-
+
+   /**
+* 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
+**/
+   struct mdev_driver {
+   const char *name;
+