On Tue, 5 Nov 2019 17:32:37 +0800 Jason Wang <jasowang at redhat.com> 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 at mellanox.com> > Signed-off-by: Jason Wang <jasowang at redhat.com> > --- > .../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 doneThe 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 :/> +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.
On Tue, 5 Nov 2019 17:50:25 +0100 Cornelia Huck <cohuck at redhat.com> wrote:> On Tue, 5 Nov 2019 17:32:37 +0800 > Jason Wang <jasowang at redhat.com> 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 at mellanox.com> > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > --- > > .../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.
On Tue, 5 Nov 2019 10:44:18 -0700 Alex Williamson <alex.williamson at redhat.com> wrote:> On Tue, 5 Nov 2019 17:50:25 +0100 > Cornelia Huck <cohuck at redhat.com> wrote: > > > On Tue, 5 Nov 2019 17:32:37 +0800 > > Jason Wang <jasowang at redhat.com> 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 at mellanox.com> > > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > > --- > > > .../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, > > AlexAh, ok. So maybe use the phrasing "devices that interact with the mdev device directly" vs "devices that use device-specific ops" instead? Not a strong critique, though.> > > > +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. >