Re: [PATCH V8 3/6] mdev: introduce device specific ops

[Alex Williamson]

On Tue, 5 Nov 2019 17:50:25 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

> On Tue,  5 Nov 2019 17:32:37 +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.
> > 
> > Reviewed-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > ---
> >  .../driver-api/vfio-mediated-device.rst       | 35 +++++++++----
> >  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                 | 24 ++++++++-
> >  drivers/vfio/mdev/mdev_private.h              |  5 ++
> >  drivers/vfio/mdev/vfio_mdev.c                 | 37 ++++++-------
> >  include/linux/mdev.h                          | 43 ++++-----------
> >  include/linux/mdev_vfio_ops.h                 | 52 +++++++++++++++++++
> >  samples/vfio-mdev/mbochs.c                    | 20 ++++---
> >  samples/vfio-mdev/mdpy.c                      | 20 ++++---
> >  samples/vfio-mdev/mtty.c                      | 18 ++++---
> >  13 files changed, 206 insertions(+), 99 deletions(-)
> >  create mode 100644 include/linux/mdev_vfio_ops.h
> >   
> 
> (...)
> 
> > @@ -172,10 +163,34 @@ that a driver should use to unregister itself with the mdev core driver::
> >  
> >  	extern void mdev_unregister_device(struct device *dev);
> >  
> > -It is also required to specify the class_id in create() callback through::
> > +As multiple types of mediated devices may be supported, class id needs
> > +to be specified in the create callback(). This could be done  
> 
> The brackets should probably go behind 'create'?
> 
> > +explicitly for the device that does not use on mdev bus for its  
> 
> "for devices that do not use the mdev bus" ?
> 
> But why wouldn't they? I feel like I've missed some discussion here :/

The device ops provide a route through mdev-core for known callbacks,
which is primarily useful when we have 1:N relation between mdev bus
driver and vendor drivers.  The obvious example here is vfio-mdev,
where we have GVT-g, vfio-ap, vfio-ccw, NVIDIA GRID, and various sample
drivers all advertising vfio-mdev support via their class id.  However,
if we have a tightly coupled vendor driver and mdev bus driver, as the
mlx5 support that Parav is developing, the claim is that they prefer
not to expose any device ops and intend to interact directly with the
mdev device.  At least that's my understanding.  Thanks,

Alex

> > +operation through:
> >  
> >  	int mdev_set_class(struct mdev_device *mdev, u16 id);
> >  
> > +For the device that uses on the mdev bus for its operation, the class  
> 
> "For devices that use the mdev bus..."
> 
> But same comment as above.
> 
> > +should provide helper function to set class id and device specific
> > +ops. E.g for vfio-mdev devices, the function to be called is::
> > +
> > +	int mdev_set_vfio_ops(struct mdev_device *mdev,
> > +                              const struct mdev_vfio_device_ops *vfio_ops);
> > +
> > +The class id (set by this function to MDEV_CLASS_ID_VFIO) is used to
> > +match a device with an mdev driver via its id table. The device
> > +specific callbacks (specified in *vfio_ops) are obtainable via
> > +mdev_get_vfio_ops() (for use by the mdev bus driver). A vfio-mdev
> > +device (class id MDEV_CLASS_ID_VFIO) uses the following
> > +device-specific ops:
> > +
> > +* 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
> > +
> >  Mediated Device Management Interface Through sysfs
> >  ==================================================  
> 
> Otherwise, looks good.