[Intel-gfx] [PATCH V3 4/7] mdev: introduce device specific ops

[Cornelia Huck]

On Wed, 16 Oct 2019 05:50:08 +0000
Parav Pandit <parav at mellanox.com> wrote:

> Hi Alex,
> 
> > -----Original Message-----
> > From: Alex Williamson <alex.williamson at redhat.com>
> > Sent: Tuesday, October 15, 2019 12:27 PM
> > To: Jason Wang <jasowang at redhat.com>
> > Cc: Cornelia Huck <cohuck at redhat.com>; kvm at vger.kernel.org; linux-
> > s390 at vger.kernel.org; linux-kernel at vger.kernel.org; dri-
> > devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; intel-gvt-
> > dev at lists.freedesktop.org; kwankhede at nvidia.com; mst at redhat.com;
> > tiwei.bie at intel.com; virtualization at lists.linux-foundation.org;
> > netdev at vger.kernel.org; maxime.coquelin at redhat.com;
> > cunming.liang at intel.com; zhihong.wang at intel.com;
> > rob.miller at broadcom.com; xiao.w.wang at intel.com;
> > haotian.wang at sifive.com; zhenyuw at linux.intel.com; zhi.a.wang at intel.com;
> > jani.nikula at linux.intel.com; joonas.lahtinen at linux.intel.com;
> > rodrigo.vivi at intel.com; airlied at linux.ie; daniel at ffwll.ch;
> > farman at linux.ibm.com; pasic at linux.ibm.com; sebott at linux.ibm.com;
> > oberpar at linux.ibm.com; heiko.carstens at de.ibm.com; gor at linux.ibm.com;
> > borntraeger at de.ibm.com; akrowiak at linux.ibm.com; freude at linux.ibm.com;
> > lingshan.zhu at intel.com; Ido Shamay <idos at mellanox.com>;
> > eperezma at redhat.com; lulu at redhat.com; Parav Pandit
> > <parav at mellanox.com>; christophe.de.dinechin at gmail.com;
> > kevin.tian at intel.com
> > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> > 
> > On Tue, 15 Oct 2019 20:17:01 +0800
> > Jason Wang <jasowang at redhat.com> wrote:
> >   
> > > On 2019/10/15 下午6:41, Cornelia Huck wrote:  
> > > > On Fri, 11 Oct 2019 16:15:54 +0800
> > > > Jason Wang <jasowang at redhat.com> wrote:

> > > >> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
> > > >>   	extern int  mdev_register_device(struct device *dev,
> > > >>   	                                 const struct mdev_parent_ops
> > > >> *ops);
> > > >>
> > > >> -It is also required to specify the class_id through::
> > > >> +It is also required to specify the class_id and device specific ops  
> > through::  
> > > >>
> > > >> -	extern int mdev_set_class(struct device *dev, u16 id);
> > > >> +	extern int mdev_set_class(struct device *dev, u16 id,
> > > >> +	                          const void *ops);  
> > > > Apologies if that has already been discussed, but do we want a 1:1
> > > > relationship between id and ops, or can different devices with the
> > > > same id register different ops?  
> > >
> > >
> > > I think we have a N:1 mapping between id and ops, e.g we want both
> > > virtio-mdev and vhost-mdev use a single set of device ops.  
> > 
> > The contents of the ops structure is essentially defined by the id, which is
> > why I was leaning towards them being defined together.  They are effectively
> > interlocked, the id defines which mdev "endpoint"
> > driver is loaded and that driver requires mdev_get_dev_ops() to return the
> > structure required by the driver.  I wish there was a way we could
> > incorporate type checking here.  We toyed with the idea of having the class
> > in the same structure as the ops, but I think this approach was chosen for
> > simplicity.  We could still do something like:
> > 
> > int mdev_set_class_struct(struct device *dev, const struct mdev_class_struct
> > *class);
> > 
> > struct mdev_class_struct {
> > 	u16	id;
> > 	union {
> > 		struct vfio_mdev_ops vfio_ops;
> > 		struct virtio_mdev_ops virtio_ops;
> > 	};
> > };
> > 
> > Maybe even:
> > 
> > struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev) {
> > 	BUG_ON(mdev->class.id != MDEV_ID_VFIO);
> > 	return &mdev->class.vfio_ops;
> > }
> > 
> > The match callback would of course just use the mdev->class.id value.
> > Functionally equivalent, but maybe better type characteristics.  Thanks,
> > 
> > Alex  
> 
> We have 3 use cases of mdev.
> 1. current mdev binding to vfio_mdev
> 2. mdev binding to virtio
> 3. mdev binding to mlx5_core without dev_ops
> 
> Also 
> (a) a given parent may serve multiple types of classes in future.
> (b) number of classes may not likely explode, they will be handful of them. (vfio_mdev, virtio)
> 
> So, instead of making copies of this dev_ops pointer in each mdev, it is better to keep const multiple ops in their parent device.
> Something like below,
> 
> struct mdev_parent {
> 	[..]
> 	struct mdev_parent_ops *parent_ops; /* create, remove */
> 	struct vfio_mdev_ops *vfio_ops; /* read,write, ioctl etc */
> 	struct virtio_mdev_ops *virtio_ops; /* virtio ops */
> };

That feels a bit odd. Why should the parent carry pointers to every
possible version of ops?

> 
> const struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_parent *parent);
> const struct virtio_mdev_ops *mdev_get_virtio_ops(struct mdev_parent *parent);
> 
> This way, 
> (a) we have strong type check support
> (b) ops pointer is not duplicated across several hundred mdev devices, and don't have to set on every mdev creation
> (c) all 3 classes of mdev are supported
> (d) one extra symbol table entry used per ops type, but there are not expected to grow a lot.
> (e) multiple classes per single parent is still supported
> (f) still extendible for multiple classes (well defined classes = vfio, virtio, and vendor class)

Yet another suggestion: have the class id derive from the function you
use to set up the ops.

void mdev_set_vfio_ops(struct mdev_device *mdev, const struct vfio_mdev_ops *vfio_ops)
{
	mdev->device_ops = vfio_ops;
	mdev->class_id = MDEV_ID_VFIO;
}

void mdev_set_virtio_ops(struct mdev_device *mdev, const struct virtio_mdev_ops *virtio_ops)
{
	mdev->device_ops = virtio_ops;
	mdev->class_id = MDEV_ID_VIRTIO;
}

void mdev_set_vhost_ops(struct mdev_device *mdev, const struct virtio_mdev_ops *virtio_ops)
{
	mdev->device_ops = virtio_ops;
	mdev->class_id = MDEV_ID_VHOST;
}

void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */
{
	mdev->class_id = MDEV_ID_VENDOR;
}