Re: [PATCH V3 4/7] mdev: introduce device specific ops

Cornelia Huck
2019-10-16T08:52:32Z

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

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