On 2019/9/25 上午7:06, Alex Williamson wrote:

On Tue, 24 Sep 2019 21:53:29 +0800
Jason Wang [off-list ref] wrote:

quoted

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@redhat.com>
---
 .../driver-api/vfio-mediated-device.rst       |  4 +-
 MAINTAINERS                                   |  1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c              | 17 +++---
 drivers/s390/cio/vfio_ccw_ops.c               | 17 ++++--
 drivers/s390/crypto/vfio_ap_ops.c             | 13 +++--
 drivers/vfio/mdev/mdev_core.c                 | 12 +++++
 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                    | 19 ++++---
 samples/vfio-mdev/mdpy.c                      | 19 ++++---
 samples/vfio-mdev/mtty.c                      | 17 ++++--
 13 files changed, 168 insertions(+), 83 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 a5bdc60d62a1..d50425b368bb 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst

@@ -152,7 +152,9 @@ 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:
+The device specific callbacks are referred through device_ops pointer
+in mdev_parent_ops. For vfio-mdev device, its callbacks in device_ops
+are as follows:

This is not accurate.  device_ops is now on the mdev_device and is an
mdev bus driver specific structure of callbacks that must be registered
for each mdev device by the parent driver during the create callback.
There's a one to one mapping of class_id to mdev_device_ops callbacks.

Yes.

That also suggests to me that we could be more clever in registering
both of these with mdev-core.  Can we embed the class_id in the ops
structure in a common way so that the core can extract it and the bus
drivers can access their specific callbacks?

That seems much cleaner, let me try to do that in V3.

quoted

 * open: open callback of mediated device
 * close: close callback of mediated device

diff --git a/MAINTAINERS b/MAINTAINERS
index b2326dece28e..89832b316500 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -17075,6 +17075,7 @@ S:	Maintained
 F:	Documentation/driver-api/vfio-mediated-device.rst
 F:	drivers/vfio/mdev/
 F:	include/linux/mdev.h
+F:	include/linux/vfio_mdev.h
 F:	samples/vfio-mdev/
 
 VFIO PLATFORM DRIVER

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index f793252a3d2a..b274f5ee481f 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c

@@ -42,6 +42,7 @@
 #include <linux/kvm_host.h>
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <linux/vfio_mdev.h>
 #include <linux/debugfs.h>
 
 #include <linux/nospec.h>

@@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu)
 	vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
 }
 
+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops;
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;

@@ -679,6 +682,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 	ret = 0;
 
 	mdev_set_class_id(mdev, MDEV_ID_VFIO);
+	mdev_set_dev_ops(mdev, &intel_vfio_vgpu_dev_ops);

This seems rather unrefined.  We're registering interdependent data in
separate calls.  All drivers need to make both of these calls.  I'm not
sure if this is a good idea, but what if we had:

static const struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = {
	.id			= MDEV_ID_VFIO,
 	.open			= intel_vgpu_open,
 	.release		= intel_vgpu_release,
        ...

And the set function passed &intel_vfio_vgpu_dev_ops.id and the mdev
bus drivers used container_of to get to their callbacks?

Yes, with the check of mdev_device_create() if nothing is set by the device.

quoted

 out:
 	return ret;
 }

@@ -1601,20 +1605,21 @@ static const struct attribute_group *intel_vgpu_groups[] = {
 	NULL,
 };
 
-static struct mdev_parent_ops intel_vgpu_ops = {
-	.mdev_attr_groups       = intel_vgpu_groups,
-	.create			= intel_vgpu_create,
-	.remove			= intel_vgpu_remove,
-
+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = {
 	.open			= intel_vgpu_open,
 	.release		= intel_vgpu_release,
-
 	.read			= intel_vgpu_read,
 	.write			= intel_vgpu_write,
 	.mmap			= intel_vgpu_mmap,
 	.ioctl			= intel_vgpu_ioctl,
 };
 
+static struct mdev_parent_ops intel_vgpu_ops = {

These could maybe be made const at the same time.  Thanks,

Alex

Ok, let me fix.

Thanks