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

[Cornelia Huck]

On Thu, 17 Oct 2019 11:53:10 -0600
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:

> On Thu, 17 Oct 2019 17:07:55 +0200
> Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> 
> > On Thu, 17 Oct 2019 18:48:33 +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       | 25 +++++----
> > >  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                 | 18 +++++--
> > >  drivers/vfio/mdev/mdev_private.h              |  1 +
> > >  drivers/vfio/mdev/vfio_mdev.c                 | 37 ++++++-------
> > >  include/linux/mdev.h                          | 45 ++++------------
> > >  include/linux/vfio_mdev.h                     | 52 +++++++++++++++++++
> > >  samples/vfio-mdev/mbochs.c                    | 20 ++++---
> > >  samples/vfio-mdev/mdpy.c                      | 20 ++++---
> > >  samples/vfio-mdev/mtty.c                      | 18 ++++---
> > >  13 files changed, 184 insertions(+), 103 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 f9a78d75a67a..0cca84d19603 100644
> > > --- a/Documentation/driver-api/vfio-mediated-device.rst
> > > +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > > @@ -152,11 +152,22 @@ 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:
> > > -
> > > -* open: open callback of mediated device
> > > -* close: close callback of mediated device
> > > -* ioctl: ioctl callback of mediated device
> > > +As multiple types of mediated devices may be supported, the device
> > > +must set up the class id and the device specific callbacks in create()    
> > 
> > s/in create()/in the create()/
> >   
> > > +callback. E.g for vfio-mdev device it needs to be done through:    
> > 
> > "Each class provides a helper function to do so; e.g. for vfio-mdev
> > devices, the function to be called is:"
> > 
> > ?
> >   
> > > +
> > > +    int mdev_set_vfio_ops(struct mdev_device *mdev,
> > > +                          const struct vfio_mdev_ops *vfio_ops);
> > > +
> > > +The class id (set to MDEV_CLASS_ID_VFIO) is used to match a device    
> > 
> > "(set by this helper function to MDEV_CLASS_ID_VFIO)" ?
> >   
> > > +with an mdev driver via its id table. The device specific callbacks
> > > +(specified in *ops) are obtainable via mdev_get_dev_ops() (for use by    
> > 
> > "(specified in *vfio_ops by the caller)" ?
> >   
> > > +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
> > > @@ -167,10 +178,6 @@ 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 in create() callback through::
> > > -
> > > -	int mdev_set_class(struct mdev_device *mdev, u16 id);
> > > -    
> > 
> > I'm wondering if this patch set should start out with introducing
> > helper functions already (i.e. don't introduce mdev_set_class(), but
> > start out with mdev_set_class_vfio() which will gain the *vfio_ops
> > argument in this patch.)  
> 
> Yes, it would be cleaner, but is it really worth the churn?  Correct me
> if I'm wrong, but I think we get to the same point after this patch and
> aside from the function name itself, the difference is really just that
> the class_id is briefly exposed to the parent driver, right?  Thanks,

Yes, it does not really matter much. If there's no other reason to
rework things, I'd just keep this as it is now.