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:
> > >
> > >> Currently, except for the create and remove, the rest of
> > >> mdev_parent_ops is designed for vfio-mdev driver only and may not
> > >> help for kernel mdev driver. With the help of class id, this patch
> > >> introduces device specific callbacks inside mdev_device structure.
> > >> This allows different set of callback to be used by vfio-mdev and
> > >> virtio-mdev.
> > >>
> > >> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > >> ---
> > >> .../driver-api/vfio-mediated-device.rst | 22 +++++---
> > >> MAINTAINERS | 1 +
> > >> drivers/gpu/drm/i915/gvt/kvmgt.c | 18 ++++---
> > >> drivers/s390/cio/vfio_ccw_ops.c | 18 ++++---
> > >> drivers/s390/crypto/vfio_ap_ops.c | 14 +++--
> > >> drivers/vfio/mdev/mdev_core.c | 9 +++-
> > >> drivers/vfio/mdev/mdev_private.h | 1 +
> > >> drivers/vfio/mdev/vfio_mdev.c | 37 ++++++-------
> > >> include/linux/mdev.h | 42 +++------------
> > >> include/linux/vfio_mdev.h | 52 +++++++++++++++++++
> > >> samples/vfio-mdev/mbochs.c | 20 ++++---
> > >> samples/vfio-mdev/mdpy.c | 21 +++++---
> > >> samples/vfio-mdev/mtty.c | 18 ++++---
> > >> 13 files changed, 177 insertions(+), 96 deletions(-)
> > >> create mode 100644 include/linux/vfio_mdev.h
> > >>
> > >> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> > >> b/Documentation/driver-api/vfio-mediated-device.rst
> > >> index 2035e48da7b2..553574ebba73 100644
> > >> --- a/Documentation/driver-api/vfio-mediated-device.rst
> > >> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > >> @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev
> type, or any other categorization.
> > >> Vendor drivers are expected to be fully asynchronous in this respect or
> > >> provide their own internal resource protection.)
> > >>
> > >> -The callbacks in the mdev_parent_ops structure are as follows:
> > >> +In order to support multiple types of device/driver, device needs
> > >> +to provide both class_id and device_ops through:
> > > "As multiple types of mediated devices may be supported, the device
> > > needs to set up the class id and the device specific callbacks via:"
> > >
> > > ?
> > >
> > >>
> > >> -* open: open callback of mediated device
> > >> -* close: close callback of mediated device
> > >> -* ioctl: ioctl callback of mediated device
> > >> + void mdev_set_class(struct mdev_device *mdev, u16 id, const
> > >> + void *ops);
> > >> +
> > >> +The class_id is used to be paired with ids in id_table in
> > >> +mdev_driver structure for probing the correct driver.
> > > "The class id (specified in id) is used to match a device with an
> > > mdev driver via its id table."
> > >
> > > ?
> > >
> > >> The device_ops is device
> > >> +specific callbacks which can be get through mdev_get_dev_ops()
> > >> +function by mdev bus driver.
> > > "The device specific callbacks (specified in *ops) are obtainable
> > > via
> > > mdev_get_dev_ops() (for use by the mdev bus driver.)"
> > >
> > > ?
> > >
> > >> For vfio-mdev device, its device specific
> > >> +ops are as follows:
> > > "A vfio-mdev device (class id MDEV_ID_VFIO) uses the following
> > > device-specific ops:"
> > >
> > > ?
> >
> >
> > All you propose is better than what I wrote, will change the docs.
> >
> >
> > >
> > >> +
> > >> +* open: open callback of vfio mediated device
> > >> +* close: close callback of vfio mediated device
> > >> +* ioctl: ioctl callback of vfio mediated device
> > >> * read : read emulation callback
> > >> * write: write emulation callback
> > >> * mmap: mmap emulation callback
> > >> @@ -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 */
};
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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
