Re: [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Tian, Kevin]

> From: Daniel P. Berrange [mailto:address@hidden
> Sent: Thursday, September 29, 2016 10:39 PM
> 
> On Thu, Sep 29, 2016 at 02:35:48PM +0000, Tian, Kevin wrote:
> > > From: Daniel P. Berrange [mailto:address@hidden
> > > Sent: Thursday, September 29, 2016 4:06 PM
> > >
> > > On Wed, Sep 28, 2016 at 12:48:33PM -0700, Neo Jia wrote:
> > > > On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
> > > > > On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
> > > > > >
> > > > > > Hi libvirt experts,
> > > > > >
> > > > > > Thanks for valuable input on v1 version of RFC.
> > > > > >
> > > > > > Quick brief, VFIO based mediated device framework provides a way to
> > > > > > virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel 
> > > > > > KVMGT
> > > > > > and IBM's channel IO. This framework reuses VFIO APIs for all the
> > > > > > functionalities for mediated devices which are currently being used 
> > > > > > for
> > > > > > pass through devices. This framework introduces a set of new sysfs 
> > > > > > files
> > > > > > for device creation and its life cycle management.
> > > > > >
> > > > > > Here is the summary of discussion on v1:
> > > > > > 1. Discover mediated device:
> > > > > > As part of physical device initialization process, vendor driver 
> > > > > > will
> > > > > > register their physical devices, which will be used to create 
> > > > > > virtual
> > > > > > device (mediated device, aka mdev) to the mediated framework.
> > > > > >
> > > > > > Vendor driver should specify mdev_supported_types in directory 
> > > > > > format.
> > > > > > This format is class based, for example, display class directory 
> > > > > > format
> > > > > > should be as below. We need to define such set for each class of 
> > > > > > devices
> > > > > > which would be supported by mediated device framework.
> > > > > >
> > > > > >  --- mdev_destroy
> > > > > >  --- mdev_supported_types
> > > > > >      |-- 11
> > > > > >      |   |-- create
> > > > > >      |   |-- name
> > > > > >      |   |-- fb_length
> > > > > >      |   |-- resolution
> > > > > >      |   |-- heads
> > > > > >      |   |-- max_instances
> > > > > >      |   |-- params
> > > > > >      |   |-- requires_group
> > > > > >      |-- 12
> > > > > >      |   |-- create
> > > > > >      |   |-- name
> > > > > >      |   |-- fb_length
> > > > > >      |   |-- resolution
> > > > > >      |   |-- heads
> > > > > >      |   |-- max_instances
> > > > > >      |   |-- params
> > > > > >      |   |-- requires_group
> > > > > >      |-- 13
> > > > > >          |-- create
> > > > > >          |-- name
> > > > > >          |-- fb_length
> > > > > >          |-- resolution
> > > > > >          |-- heads
> > > > > >          |-- max_instances
> > > > > >          |-- params
> > > > > >          |-- requires_group
> > > > > >
> > > > > >
> > > > > > In the above example directory '11' represents a type id of mdev 
> > > > > > device.
> > > > > > 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
> > > > > > 'requires_group' would be Read-Only files that vendor would provide 
> > > > > > to
> > > > > > describe about that type.
> > > > > >
> > > > > > 'create':
> > > > > >     Write-only file. Mandatory.
> > > > > >     Accepts string to create mediated device.
> > > > > >
> > > > > > 'name':
> > > > > >     Read-Only file. Mandatory.
> > > > > >     Returns string, the name of that type id.
> > > > >
> > > > > Presumably this is a human-targetted title/description of
> > > > > the device.
> > > > >
> > > > > >
> > > > > > 'fb_length':
> > > > > >     Read-only file. Mandatory.
> > > > > >     Returns <number>{K,M,G}, size of framebuffer.
> > > > > >
> > > > > > 'resolution':
> > > > > >     Read-Only file. Mandatory.
> > > > > >     Returns 'hres x vres' format. Maximum supported resolution.
> > > > > >
> > > > > > 'heads':
> > > > > >     Read-Only file. Mandatory.
> > > > > >     Returns integer. Number of maximum heads supported.
> > > > >
> > > > > None of these should be mandatory as that makes the mdev
> > > > > useless for non-GPU devices.
> > > > >
> > > > > I'd expect to see a 'class' or 'type' attribute in the
> > > > > directory whcih tells you what kind of mdev it is. A
> > > > > valid 'class' value would be 'gpu'. The fb_length,
> > > > > resolution, and heads parameters would only be mandatory
> > > > > when class==gpu.
> > > > >
> > > >
> > > > Hi Daniel,
> > > >
> > > > Here you are proposing to add a class named "gpu", which will make all 
> > > > those gpu
> > > > related attributes mandatory, which libvirt can allow user to better
> > > > parse/present a particular mdev configuration?
> > > >
> > > > I am just wondering if there is another option that we just make all 
> > > > those
> > > > attributes that a mdev device can have as optional but still meaningful 
> > > > to
> > > > libvirt, so libvirt can still parse / recognize them as an class "mdev".
> > >
> > > 'mdev' isn't a class - mdev is the name of the kernel module. The class
> > > refers to the broad capability of the device. class would be things
> > > like "gpu", "nic", "fpga" or other such things. The point of the class
> > > is to identify which other attributes will be considered mandatory.
> > >
> > >
> >
> > Thanks Daniel. This class definition makes sense to me.
> >
> > However I'm not sure whether we should define such common mandatory 
> > attributes
> > of a 'gpu' class now. Intel will go with a 2's power sharing of type 
> > definition... actual
> > type name to be finalized, but an example looks like below:
> >
> > [GVTG-SKL-x2]: available instances (2)
> > [GVTG-SKL-x4]: available instances (4)
> > [GVTG-SKL-x8]: available instances (8)
> > ...
> >
> > User can create different types of vGPUs simultaneously. A GVTG-SKL-x2 type
> > vGPU will get half of the physical GPU resource, while a GVTG-SKL-x4 type 
> > will
> > get a quarter. However it's unclear to me how we want to enumerate those
> > resources into resolution or heads. I feel it'd be more reasonable for us 
> > to push
> > initial libvirt mdev support w/o vgpu specific class definition, until we 
> > see
> > a clear value of doing so (at that time we then follow Daniel's guideline 
> > to define
> > mandatory attributes common to all GPU vendors).
> 
> Libvirt won't report arbitrary vendor define attributes. So if we are not
> going to define a gpu class & associated attributes, then there will be
> no reporting of the 'heads', 'resolution', 'fb_length' data described
> above.
> 

yes, that's my point. I think nvidia may put them into the 'description' 
attribute
just for descriptive purpose for now.
Thanks
Kevin