On Wed, 16 Oct 2019 05:50:08 +0000
Parav Pandit <parav@xxxxxxxxxxxx> wrote:
> Hi Alex,
>
> > -----Original Message-----
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Tuesday, October 15, 2019 12:27 PM
> > To: Jason Wang <jasowang@xxxxxxxxxx>
> > Cc: Cornelia Huck <cohuck@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; linux-
> > s390@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dri-
> > devel@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-
> > dev@xxxxxxxxxxxxxxxxxxxxx; kwankhede@xxxxxxxxxx; mst@xxxxxxxxxx;
> > tiwei.bie@xxxxxxxxx; virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx;
> > netdev@xxxxxxxxxxxxxxx; maxime.coquelin@xxxxxxxxxx;
> > cunming.liang@xxxxxxxxx; zhihong.wang@xxxxxxxxx;
> > rob.miller@xxxxxxxxxxxx; xiao.w.wang@xxxxxxxxx;
> > haotian.wang@xxxxxxxxxx; zhenyuw@xxxxxxxxxxxxxxx; zhi.a.wang@xxxxxxxxx;
> > jani.nikula@xxxxxxxxxxxxxxx; joonas.lahtinen@xxxxxxxxxxxxxxx;
> > rodrigo.vivi@xxxxxxxxx; airlied@xxxxxxxx; daniel@xxxxxxxx;
> > farman@xxxxxxxxxxxxx; pasic@xxxxxxxxxxxxx; sebott@xxxxxxxxxxxxx;
> > oberpar@xxxxxxxxxxxxx; heiko.carstens@xxxxxxxxxx; gor@xxxxxxxxxxxxx;
> > borntraeger@xxxxxxxxxx; akrowiak@xxxxxxxxxxxxx; freude@xxxxxxxxxxxxx;
> > lingshan.zhu@xxxxxxxxx; Ido Shamay <idos@xxxxxxxxxxxx>;
> > eperezma@xxxxxxxxxx; lulu@xxxxxxxxxx; Parav Pandit
> > <parav@xxxxxxxxxxxx>; christophe.de.dinechin@xxxxxxxxx;
> > kevin.tian@xxxxxxxxx
> > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> >
> > On Tue, 15 Oct 2019 20:17:01 +0800
> > Jason Wang <jasowang@xxxxxxxxxx> wrote:
> >
> > > On 2019/10/15 下午6:41, Cornelia Huck wrote:
> > > > On Fri, 11 Oct 2019 16:15:54 +0800
> > > > Jason Wang <jasowang@xxxxxxxxxx> 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;
}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
