On Wed, 16 Oct 2019 05:50:08 +0000
Parav Pandit <pa...@mellanox.com> wrote:
> Hi Alex,
>
> > -----Original Message-----
> > From: Alex Williamson <alex.william...@redhat.com>
> > Sent: Tuesday, October 15, 2019 12:27 PM
> > To: Jason Wang <jasow...@redhat.com>
> > Cc: Cornelia Huck <coh...@redhat.com>; k...@vger.kernel.org; linux-
> > s...@vger.kernel.org; linux-ker...@vger.kernel.org; dri-
> > de...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-gvt-
> > d...@lists.freedesktop.org; kwankh...@nvidia.com; m...@redhat.com;
> > tiwei....@intel.com; virtualizat...@lists.linux-foundation.org;
> > net...@vger.kernel.org; maxime.coque...@redhat.com;
> > cunming.li...@intel.com; zhihong.w...@intel.com;
> > rob.mil...@broadcom.com; xiao.w.w...@intel.com;
> > haotian.w...@sifive.com; zhen...@linux.intel.com; zhi.a.w...@intel.com;
> > jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com;
> > rodrigo.v...@intel.com; airl...@linux.ie; dan...@ffwll.ch;
> > far...@linux.ibm.com; pa...@linux.ibm.com; seb...@linux.ibm.com;
> > ober...@linux.ibm.com; heiko.carst...@de.ibm.com; g...@linux.ibm.com;
> > borntrae...@de.ibm.com; akrow...@linux.ibm.com; fre...@linux.ibm.com;
> > lingshan....@intel.com; Ido Shamay <i...@mellanox.com>;
> > epere...@redhat.com; l...@redhat.com; Parav Pandit
> > <pa...@mellanox.com>; christophe.de.dinec...@gmail.com;
> > kevin.t...@intel.com
> > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> >
> > On Tue, 15 Oct 2019 20:17:01 +0800
> > Jason Wang <jasow...@redhat.com> wrote:
> >
> > > On 2019/10/15 下午6:41, Cornelia Huck wrote:
> > > > On Fri, 11 Oct 2019 16:15:54 +0800
> > > > Jason Wang <jasow...@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;
}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
