[Intel-gfx] [PATCH V3 4/7] mdev: introduce device specific ops
Parav Pandit
parav at mellanox.com
Wed Oct 16 15:31:25 UTC 2019
> -----Original Message-----
> From: Cornelia Huck <cohuck at redhat.com>
> Sent: Wednesday, October 16, 2019 3:53 AM
> To: Parav Pandit <parav at mellanox.com>
> Cc: Alex Williamson <alex.williamson at redhat.com>; Jason Wang
> <jasowang 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; christophe.de.dinechin at gmail.com;
> kevin.tian at intel.com
> Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
>
> 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?
>
How many are we expecting? I envisioned handful of them.
It carries because parent is few, mdevs are several hundreds.
It makes sense to keep few copies, instead of several hundred copies and it doesn't need to setup on every mdev creation.
> >
> > 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;
> }
More information about the Intel-gfx
mailing list