[RFC PATCH 0/2] Mdev: support mutiple kinds of devices

[RFC PATCH 0/2] Mdev: support mutiple kinds of devices

[Jason Wang]

Hi all:

During the development of virtio-mdev[1]. I find that mdev needs to be
extended to support devices other than vfio mdev device. So this
series tries to extend the mdev to be able to differ from different
devices by:

- device id and matching for mdev bus
- device speicfic callbacks and move vfio callbacks there

Sent for early reivew, compile test only!

Thanks

[1] https://lkml.org/lkml/2019/9/10/135

Jason Wang (2):
  mdev: device id support
  mdev: introduce device specific ops

 drivers/gpu/drm/i915/gvt/kvmgt.c  | 16 ++++---
 drivers/s390/cio/vfio_ccw_ops.c   | 16 ++++---
 drivers/s390/crypto/vfio_ap_ops.c | 13 ++++--
 drivers/vfio/mdev/mdev_core.c     | 14 +++++-
 drivers/vfio/mdev/mdev_driver.c   | 14 ++++++
 drivers/vfio/mdev/mdev_private.h  |  1 +
 drivers/vfio/mdev/vfio_mdev.c     | 36 ++++++++++-----
 include/linux/mdev.h              | 76 +++++++++++++++++++------------
 include/linux/mod_devicetable.h   |  6 +++
 samples/vfio-mdev/mbochs.c        | 18 +++++---
 samples/vfio-mdev/mdpy.c          | 18 +++++---
 samples/vfio-mdev/mtty.c          | 16 ++++---
 12 files changed, 163 insertions(+), 81 deletions(-)

-- 
2.19.1

[RFC PATCH 1/2] mdev: device id support

[Jason Wang]

Mdev bus only support vfio driver right now, so it doesn't implement
match method. But in the future, we may add drivers other than vfio,
one example is virtio-mdev[1] driver. This means we need to add device
id support in bus match method to pair the mdev device and mdev driver
correctly.

So this patch add id_table to mdev_driver and id for mdev parent, and
implement the match method for mdev bus.

[1] https://lkml.org/lkml/2019/9/10/135

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c  |  2 +-
 drivers/s390/cio/vfio_ccw_ops.c   |  2 +-
 drivers/s390/crypto/vfio_ap_ops.c |  3 ++-
 drivers/vfio/mdev/mdev_core.c     | 14 ++++++++++++--
 drivers/vfio/mdev/mdev_driver.c   | 14 ++++++++++++++
 drivers/vfio/mdev/mdev_private.h  |  1 +
 drivers/vfio/mdev/vfio_mdev.c     |  6 ++++++
 include/linux/mdev.h              |  6 +++++-
 include/linux/mod_devicetable.h   |  6 ++++++
 samples/vfio-mdev/mbochs.c        |  2 +-
 samples/vfio-mdev/mdpy.c          |  2 +-
 samples/vfio-mdev/mtty.c          |  2 +-
 12 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 23aa3e50cbf8..19d51a35f019 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1625,7 +1625,7 @@ static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
 		return -EFAULT;
 	intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;
 
-	return mdev_register_device(dev, &intel_vgpu_ops);
+	return mdev_register_vfio_device(dev, &intel_vgpu_ops);
 }
 
 static void kvmgt_host_exit(struct device *dev)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 5eb61116ca6f..f87d9409e290 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -578,7 +578,7 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
 
 int vfio_ccw_mdev_reg(struct subchannel *sch)
 {
-	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
+	return mdev_register_vfio_device(&sch->dev, &vfio_ccw_mdev_ops);
 }
 
 void vfio_ccw_mdev_unreg(struct subchannel *sch)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 0604b49a4d32..eacbde3c7a97 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1295,7 +1295,8 @@ int vfio_ap_mdev_register(void)
 {
 	atomic_set(&matrix_dev->available_instances, MAX_ZDEV_ENTRIES_EXT);
 
-	return mdev_register_device(&matrix_dev->device, &vfio_ap_matrix_ops);
+	return mdev_register_vfio_device(&matrix_dev->device,
+					 &vfio_ap_matrix_ops);
 }
 
 void vfio_ap_mdev_unregister(void)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..fc07ff3ebe96 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -135,11 +135,14 @@ static int mdev_device_remove_cb(struct device *dev, void *data)
  * mdev_register_device : Register a device
  * @dev: device structure representing parent device.
  * @ops: Parent device operation structure to be registered.
+ * @id: device id.
  *
  * Add device to list of registered parent devices.
  * Returns a negative value on error, otherwise 0.
  */
-int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
+int mdev_register_device(struct device *dev,
+			 const struct mdev_parent_ops *ops,
+			 u8 id)
 {
 	int ret;
 	struct mdev_parent *parent;
@@ -175,6 +178,7 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
 
 	parent->dev = dev;
 	parent->ops = ops;
+	parent->device_id = id;
 
 	if (!mdev_bus_compat_class) {
 		mdev_bus_compat_class = class_compat_register("mdev_bus");
@@ -208,7 +212,13 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
 		put_device(dev);
 	return ret;
 }
-EXPORT_SYMBOL(mdev_register_device);
+
+int mdev_register_vfio_device(struct device *dev,
+			      const struct mdev_parent_ops *ops)
+{
+	return mdev_register_device(dev, ops, MDEV_ID_VFIO);
+}
+EXPORT_SYMBOL(mdev_register_vfio_device);
 
 /*
  * mdev_unregister_device : Unregister a parent device
diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
index 0d3223aee20b..fd5e9541d18e 100644
--- a/drivers/vfio/mdev/mdev_driver.c
+++ b/drivers/vfio/mdev/mdev_driver.c
@@ -69,8 +69,22 @@ static int mdev_remove(struct device *dev)
 	return 0;
 }
 
+static int mdev_match(struct device *dev, struct device_driver *drv)
+{
+	unsigned int i;
+	struct mdev_device *mdev = to_mdev_device(dev);
+	struct mdev_driver *mdrv = to_mdev_driver(drv);
+	const struct mdev_device_id *ids = mdrv->id_table;
+
+	for (i = 0; ids[i].id; i++)
+		if (ids[i].id == mdev->parent->device_id)
+			return 1;
+	return 0;
+}
+
 struct bus_type mdev_bus_type = {
 	.name		= "mdev",
+	.match		= mdev_match,
 	.probe		= mdev_probe,
 	.remove		= mdev_remove,
 };
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 7d922950caaf..7fc8153671e0 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -22,6 +22,7 @@ struct mdev_parent {
 	struct list_head type_list;
 	/* Synchronize device creation/removal with parent unregistration */
 	struct rw_semaphore unreg_sem;
+	u8 device_id;
 };
 
 struct mdev_device {
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 30964a4e0a28..887c57f10880 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -120,10 +120,16 @@ static void vfio_mdev_remove(struct device *dev)
 	vfio_del_group_dev(dev);
 }
 
+static struct mdev_device_id id_table[] = {
+	{ MDEV_ID_VFIO },
+	{ 0 },
+};
+
 static struct mdev_driver vfio_mdev_driver = {
 	.name	= "vfio_mdev",
 	.probe	= vfio_mdev_probe,
 	.remove	= vfio_mdev_remove,
+	.id_table = id_table,
 };
 
 static int __init vfio_mdev_init(void)
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 0ce30ca78db0..f85045392120 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -118,6 +118,7 @@ struct mdev_type_attribute mdev_type_attr_##_name =		\
  * @probe: called when new device created
  * @remove: called when device removed
  * @driver: device driver structure
+ * @id_table: the ids serviced by this driver.
  *
  **/
 struct mdev_driver {
@@ -125,6 +126,7 @@ struct mdev_driver {
 	int  (*probe)(struct device *dev);
 	void (*remove)(struct device *dev);
 	struct device_driver driver;
+	const struct mdev_device_id *id_table;
 };
 
 #define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
@@ -135,7 +137,7 @@ const guid_t *mdev_uuid(struct mdev_device *mdev);
 
 extern struct bus_type mdev_bus_type;
 
-int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops);
+int mdev_register_vfio_device(struct device *dev, const struct mdev_parent_ops *ops);
 void mdev_unregister_device(struct device *dev);
 
 int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
@@ -145,4 +147,6 @@ struct device *mdev_parent_dev(struct mdev_device *mdev);
 struct device *mdev_dev(struct mdev_device *mdev);
 struct mdev_device *mdev_from_dev(struct device *dev);
 
+#define MDEV_ID_VFIO 1 /* VFIO device */
+
 #endif /* MDEV_H */
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5714fd35a83c..f1fc143df042 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -821,4 +821,10 @@ struct wmi_device_id {
 	const void *context;
 };
 
+/* MDEV */
+
+struct mdev_device_id {
+	__u8 id;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index ac5c8c17b1ff..71a4469be85d 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -1468,7 +1468,7 @@ static int __init mbochs_dev_init(void)
 	if (ret)
 		goto failed2;
 
-	ret = mdev_register_device(&mbochs_dev, &mdev_fops);
+	ret = mdev_register_vfio_device(&mbochs_dev, &mdev_fops);
 	if (ret)
 		goto failed3;
 
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index cc86bf6566e4..d3029dd27d91 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -775,7 +775,7 @@ static int __init mdpy_dev_init(void)
 	if (ret)
 		goto failed2;
 
-	ret = mdev_register_device(&mdpy_dev, &mdev_fops);
+	ret = mdev_register_vfio_device(&mdpy_dev, &mdev_fops);
 	if (ret)
 		goto failed3;
 
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..744c88a6b22c 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1468,7 +1468,7 @@ static int __init mtty_dev_init(void)
 	if (ret)
 		goto failed2;
 
-	ret = mdev_register_device(&mtty_dev.dev, &mdev_fops);
+	ret = mdev_register_vfio_device(&mtty_dev.dev, &mdev_fops);
 	if (ret)
 		goto failed3;
 
-- 
2.19.1

[RFC PATCH 2/2] mdev: introduce device specific ops

[Jason Wang]

Currently, except for the crate and remove. The rest fields of
mdev_parent_ops is just designed for vfio-mdev driver and may not help
for kernel mdev driver. So follow the device id support by previous
patch, this patch introduces device specific ops which points to
device specific ops (e.g vfio ops). This allows the future drivers
like virtio-mdev to implement its own device specific ops.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c  | 14 +++---
 drivers/s390/cio/vfio_ccw_ops.c   | 14 +++---
 drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
 drivers/vfio/mdev/vfio_mdev.c     | 30 +++++++------
 include/linux/mdev.h              | 72 ++++++++++++++++++-------------
 samples/vfio-mdev/mbochs.c        | 16 ++++---
 samples/vfio-mdev/mdpy.c          | 16 ++++---
 samples/vfio-mdev/mtty.c          | 14 +++---
 8 files changed, 113 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 19d51a35f019..64823998fd58 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1600,20 +1600,22 @@ 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_parent_ops intel_vfio_vgpu_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 = {
+	.mdev_attr_groups       = intel_vgpu_groups,
+	.create			= intel_vgpu_create,
+	.remove			= intel_vgpu_remove,
+	.device_ops	        = &intel_vfio_vgpu_ops,
+};
+
 static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
 {
 	struct attribute **kvm_type_attrs;
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f87d9409e290..96e9f18100ae 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -564,11 +564,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
 	}
 }
 
-static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
-	.owner			= THIS_MODULE,
-	.supported_type_groups  = mdev_type_groups,
-	.create			= vfio_ccw_mdev_create,
-	.remove			= vfio_ccw_mdev_remove,
+static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
 	.open			= vfio_ccw_mdev_open,
 	.release		= vfio_ccw_mdev_release,
 	.read			= vfio_ccw_mdev_read,
@@ -576,6 +572,14 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
 	.ioctl			= vfio_ccw_mdev_ioctl,
 };
 
+static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
+	.owner			= THIS_MODULE,
+	.supported_type_groups  = mdev_type_groups,
+	.create			= vfio_ccw_mdev_create,
+	.remove			= vfio_ccw_mdev_remove,
+	.device_ops		= &vfio_mdev_ops,
+};
+
 int vfio_ccw_mdev_reg(struct subchannel *sch)
 {
 	return mdev_register_vfio_device(&sch->dev, &vfio_ccw_mdev_ops);
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index eacbde3c7a97..a48282bff066 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1280,15 +1280,19 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
 	return ret;
 }
 
+static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
+	.open			= vfio_ap_mdev_open,
+	.release		= vfio_ap_mdev_release,
+	.ioctl			= vfio_ap_mdev_ioctl,
+};
+
 static const struct mdev_parent_ops vfio_ap_matrix_ops = {
 	.owner			= THIS_MODULE,
 	.supported_type_groups	= vfio_ap_mdev_type_groups,
 	.mdev_attr_groups	= vfio_ap_mdev_attr_groups,
 	.create			= vfio_ap_mdev_create,
 	.remove			= vfio_ap_mdev_remove,
-	.open			= vfio_ap_mdev_open,
-	.release		= vfio_ap_mdev_release,
-	.ioctl			= vfio_ap_mdev_ioctl,
+	.device_ops		= &vfio_mdev_ops,
 };
 
 int vfio_ap_mdev_register(void)
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 887c57f10880..1196fbb6c3d2 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -25,15 +25,16 @@ static int vfio_mdev_open(void *device_data)
 {
 	struct mdev_device *mdev = device_data;
 	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
 	int ret;
 
-	if (unlikely(!parent->ops->open))
+	if (unlikely(!ops->open))
 		return -EINVAL;
 
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
 
-	ret = parent->ops->open(mdev);
+	ret = ops->open(mdev);
 	if (ret)
 		module_put(THIS_MODULE);
 
@@ -44,9 +45,10 @@ static void vfio_mdev_release(void *device_data)
 {
 	struct mdev_device *mdev = device_data;
 	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
 
-	if (likely(parent->ops->release))
-		parent->ops->release(mdev);
+	if (likely(ops->release))
+		ops->release(mdev);
 
 	module_put(THIS_MODULE);
 }
@@ -56,11 +58,12 @@ static long vfio_mdev_unlocked_ioctl(void *device_data,
 {
 	struct mdev_device *mdev = device_data;
 	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
 
-	if (unlikely(!parent->ops->ioctl))
+	if (unlikely(!ops->ioctl))
 		return -EINVAL;
 
-	return parent->ops->ioctl(mdev, cmd, arg);
+	return ops->ioctl(mdev, cmd, arg);
 }
 
 static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
@@ -68,11 +71,12 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
 {
 	struct mdev_device *mdev = device_data;
 	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
 
-	if (unlikely(!parent->ops->read))
+	if (unlikely(!ops->read))
 		return -EINVAL;
 
-	return parent->ops->read(mdev, buf, count, ppos);
+	return ops->read(mdev, buf, count, ppos);
 }
 
 static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
@@ -80,22 +84,24 @@ static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
 {
 	struct mdev_device *mdev = device_data;
 	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
 
-	if (unlikely(!parent->ops->write))
+	if (unlikely(!ops->write))
 		return -EINVAL;
 
-	return parent->ops->write(mdev, buf, count, ppos);
+	return ops->write(mdev, buf, count, ppos);
 }
 
 static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
 {
 	struct mdev_device *mdev = device_data;
 	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
 
-	if (unlikely(!parent->ops->mmap))
+	if (unlikely(!ops->mmap))
 		return -EINVAL;
 
-	return parent->ops->mmap(mdev, vma);
+	return ops->mmap(mdev, vma);
 }
 
 static const struct vfio_device_ops vfio_mdev_dev_ops = {
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index f85045392120..3b8a76bc69cf 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev, struct device *iommu_device);
 struct device *mdev_get_iommu_device(struct device *dev);
 
 /**
- * struct mdev_parent_ops - Structure to be registered for each parent device to
- * register the device to mdev module.
+ * struct vfio_mdev_parent_ops - Structure to be registered for each
+ * parent device to register the device to vfio-mdev module.
  *
- * @owner:		The module owner.
- * @dev_attr_groups:	Attributes of the parent device.
- * @mdev_attr_groups:	Attributes of the mediated device.
- * @supported_type_groups: Attributes to define supported types. It is mandatory
- *			to provide supported types.
- * @create:		Called to allocate basic resources in parent device's
- *			driver for a particular mediated device. It is
- *			mandatory to provide create ops.
- *			@kobj: kobject of type for which 'create' is called.
- *			@mdev: mdev_device structure on of mediated device
- *			      that is being created
- *			Returns integer: success (0) or error (< 0)
- * @remove:		Called to free resources in parent device's driver for a
- *			a mediated device. It is mandatory to provide 'remove'
- *			ops.
- *			@mdev: mdev_device device structure which is being
- *			       destroyed
- *			Returns integer: success (0) or error (< 0)
  * @open:		Open mediated device.
  *			@mdev: mediated device.
  *			Returns integer: success (0) or error (< 0)
@@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct device *dev);
  * @mmap:		mmap callback
  *			@mdev: mediated device structure
  *			@vma: vma structure
+ */
+struct vfio_mdev_parent_ops {
+	int     (*open)(struct mdev_device *mdev);
+	void    (*release)(struct mdev_device *mdev);
+	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
+			size_t count, loff_t *ppos);
+	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
+			 size_t count, loff_t *ppos);
+	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
+			 unsigned long arg);
+	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
+};
+
+/**
+ * struct mdev_parent_ops - Structure to be registered for each parent device to
+ * register the device to mdev module.
+ *
+ * @owner:		The module owner.
+ * @dev_attr_groups:	Attributes of the parent device.
+ * @mdev_attr_groups:	Attributes of the mediated device.
+ * @supported_type_groups: Attributes to define supported types. It is mandatory
+ *			to provide supported types.
+ * @create:		Called to allocate basic resources in parent device's
+ *			driver for a particular mediated device. It is
+ *			mandatory to provide create ops.
+ *			@kobj: kobject of type for which 'create' is called.
+ *			@mdev: mdev_device structure on of mediated device
+ *			      that is being created
+ *			Returns integer: success (0) or error (< 0)
+ * @remove:		Called to free resources in parent device's driver for a
+ *			a mediated device. It is mandatory to provide 'remove'
+ *			ops.
+ *			@mdev: mdev_device device structure which is being
+ *			       destroyed
+ *			Returns integer: success (0) or error (< 0)
+ * @device_ops:         Device specific emulation callback.
+ *
  * Parent device that support mediated device should be registered with mdev
  * module with mdev_parent_ops structure.
  **/
@@ -83,15 +102,7 @@ struct mdev_parent_ops {
 
 	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
 	int     (*remove)(struct mdev_device *mdev);
-	int     (*open)(struct mdev_device *mdev);
-	void    (*release)(struct mdev_device *mdev);
-	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
-			size_t count, loff_t *ppos);
-	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
-			 size_t count, loff_t *ppos);
-	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
-			 unsigned long arg);
-	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
+	const void *device_ops;
 };
 
 /* interface for exporting mdev supported type attributes */
@@ -137,7 +148,8 @@ const guid_t *mdev_uuid(struct mdev_device *mdev);
 
 extern struct bus_type mdev_bus_type;
 
-int mdev_register_vfio_device(struct device *dev, const struct mdev_parent_ops *ops);
+int mdev_register_vfio_device(struct device *dev,
+                              const struct mdev_parent_ops *ops);
 void mdev_unregister_device(struct device *dev);
 
 int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index 71a4469be85d..53ceb357f152 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -1418,12 +1418,7 @@ static struct attribute_group *mdev_type_groups[] = {
 	NULL,
 };
 
-static const struct mdev_parent_ops mdev_fops = {
-	.owner			= THIS_MODULE,
-	.mdev_attr_groups	= mdev_dev_groups,
-	.supported_type_groups	= mdev_type_groups,
-	.create			= mbochs_create,
-	.remove			= mbochs_remove,
+static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
 	.open			= mbochs_open,
 	.release		= mbochs_close,
 	.read			= mbochs_read,
@@ -1432,6 +1427,15 @@ static const struct mdev_parent_ops mdev_fops = {
 	.mmap			= mbochs_mmap,
 };
 
+static const struct mdev_parent_ops mdev_fops = {
+	.owner			= THIS_MODULE,
+	.mdev_attr_groups	= mdev_dev_groups,
+	.supported_type_groups	= mdev_type_groups,
+	.create			= mbochs_create,
+	.remove			= mbochs_remove,
+	.device_ops		= &vfio_mdev_ops,
+};
+
 static const struct file_operations vd_fops = {
 	.owner		= THIS_MODULE,
 };
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index d3029dd27d91..4ba285a5768f 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -725,12 +725,7 @@ static struct attribute_group *mdev_type_groups[] = {
 	NULL,
 };
 
-static const struct mdev_parent_ops mdev_fops = {
-	.owner			= THIS_MODULE,
-	.mdev_attr_groups	= mdev_dev_groups,
-	.supported_type_groups	= mdev_type_groups,
-	.create			= mdpy_create,
-	.remove			= mdpy_remove,
+static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
 	.open			= mdpy_open,
 	.release		= mdpy_close,
 	.read			= mdpy_read,
@@ -739,6 +734,15 @@ static const struct mdev_parent_ops mdev_fops = {
 	.mmap			= mdpy_mmap,
 };
 
+static const struct mdev_parent_ops mdev_fops = {
+	.owner			= THIS_MODULE,
+	.mdev_attr_groups	= mdev_dev_groups,
+	.supported_type_groups	= mdev_type_groups,
+	.create			= mdpy_create,
+	.remove			= mdpy_remove,
+	.device_ops		= &vfio_mdev_ops,
+};
+
 static const struct file_operations vd_fops = {
 	.owner		= THIS_MODULE,
 };
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 744c88a6b22c..a468904ec626 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1410,6 +1410,14 @@ static struct attribute_group *mdev_type_groups[] = {
 	NULL,
 };
 
+static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
+	.open                   = mtty_open,
+	.release                = mtty_close,
+	.read                   = mtty_read,
+	.write                  = mtty_write,
+	.ioctl		        = mtty_ioctl,
+};
+
 static const struct mdev_parent_ops mdev_fops = {
 	.owner                  = THIS_MODULE,
 	.dev_attr_groups        = mtty_dev_groups,
@@ -1417,11 +1425,7 @@ static const struct mdev_parent_ops mdev_fops = {
 	.supported_type_groups  = mdev_type_groups,
 	.create                 = mtty_create,
 	.remove			= mtty_remove,
-	.open                   = mtty_open,
-	.release                = mtty_close,
-	.read                   = mtty_read,
-	.write                  = mtty_write,
-	.ioctl		        = mtty_ioctl,
+	.device_ops             = &vfio_mdev_ops,
 };
 
 static void mtty_device_release(struct device *dev)
-- 
2.19.1

Re: [RFC PATCH 2/2] mdev: introduce device specific ops

[Michael S. Tsirkin]

On Thu, Sep 12, 2019 at 05:40:12PM +0800, Jason Wang wrote:
> Currently, except for the crate and remove. The rest fields of


better:

Currently, except for create and remove, the rest of the field in ...


> mdev_parent_ops is just designed for vfio-mdev driver and may not help
> for kernel mdev driver. So follow the device id support by previous
> patch, this patch introduces device specific ops which points to
> device specific ops (e.g vfio ops). This allows the future drivers
> like virtio-mdev to implement its own device specific ops.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c  | 14 +++---
>  drivers/s390/cio/vfio_ccw_ops.c   | 14 +++---
>  drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
>  drivers/vfio/mdev/vfio_mdev.c     | 30 +++++++------
>  include/linux/mdev.h              | 72 ++++++++++++++++++-------------
>  samples/vfio-mdev/mbochs.c        | 16 ++++---
>  samples/vfio-mdev/mdpy.c          | 16 ++++---
>  samples/vfio-mdev/mtty.c          | 14 +++---
>  8 files changed, 113 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 19d51a35f019..64823998fd58 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1600,20 +1600,22 @@ 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_parent_ops intel_vfio_vgpu_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 = {
> +	.mdev_attr_groups       = intel_vgpu_groups,
> +	.create			= intel_vgpu_create,
> +	.remove			= intel_vgpu_remove,
> +	.device_ops	        = &intel_vfio_vgpu_ops,
> +};
> +
>  static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
>  {
>  	struct attribute **kvm_type_attrs;
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index f87d9409e290..96e9f18100ae 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -564,11 +564,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
>  	}
>  }
>  
> -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> -	.owner			= THIS_MODULE,
> -	.supported_type_groups  = mdev_type_groups,
> -	.create			= vfio_ccw_mdev_create,
> -	.remove			= vfio_ccw_mdev_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>  	.open			= vfio_ccw_mdev_open,
>  	.release		= vfio_ccw_mdev_release,
>  	.read			= vfio_ccw_mdev_read,
> @@ -576,6 +572,14 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>  	.ioctl			= vfio_ccw_mdev_ioctl,
>  };
>  
> +static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> +	.owner			= THIS_MODULE,
> +	.supported_type_groups  = mdev_type_groups,
> +	.create			= vfio_ccw_mdev_create,
> +	.remove			= vfio_ccw_mdev_remove,
> +	.device_ops		= &vfio_mdev_ops,
> +};
> +
>  int vfio_ccw_mdev_reg(struct subchannel *sch)
>  {
>  	return mdev_register_vfio_device(&sch->dev, &vfio_ccw_mdev_ops);
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index eacbde3c7a97..a48282bff066 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1280,15 +1280,19 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
>  	return ret;
>  }
>  
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> +	.open			= vfio_ap_mdev_open,
> +	.release		= vfio_ap_mdev_release,
> +	.ioctl			= vfio_ap_mdev_ioctl,
> +};
> +
>  static const struct mdev_parent_ops vfio_ap_matrix_ops = {
>  	.owner			= THIS_MODULE,
>  	.supported_type_groups	= vfio_ap_mdev_type_groups,
>  	.mdev_attr_groups	= vfio_ap_mdev_attr_groups,
>  	.create			= vfio_ap_mdev_create,
>  	.remove			= vfio_ap_mdev_remove,
> -	.open			= vfio_ap_mdev_open,
> -	.release		= vfio_ap_mdev_release,
> -	.ioctl			= vfio_ap_mdev_ioctl,
> +	.device_ops		= &vfio_mdev_ops,
>  };
>  
>  int vfio_ap_mdev_register(void)
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> index 887c57f10880..1196fbb6c3d2 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -25,15 +25,16 @@ static int vfio_mdev_open(void *device_data)
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>  	int ret;
>  
> -	if (unlikely(!parent->ops->open))
> +	if (unlikely(!ops->open))
>  		return -EINVAL;
>  
>  	if (!try_module_get(THIS_MODULE))
>  		return -ENODEV;
>  
> -	ret = parent->ops->open(mdev);
> +	ret = ops->open(mdev);
>  	if (ret)
>  		module_put(THIS_MODULE);
>  
> @@ -44,9 +45,10 @@ static void vfio_mdev_release(void *device_data)
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>  
> -	if (likely(parent->ops->release))
> -		parent->ops->release(mdev);
> +	if (likely(ops->release))
> +		ops->release(mdev);
>  
>  	module_put(THIS_MODULE);
>  }
> @@ -56,11 +58,12 @@ static long vfio_mdev_unlocked_ioctl(void *device_data,
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>  
> -	if (unlikely(!parent->ops->ioctl))
> +	if (unlikely(!ops->ioctl))
>  		return -EINVAL;
>  
> -	return parent->ops->ioctl(mdev, cmd, arg);
> +	return ops->ioctl(mdev, cmd, arg);
>  }
>  
>  static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> @@ -68,11 +71,12 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>  
> -	if (unlikely(!parent->ops->read))
> +	if (unlikely(!ops->read))
>  		return -EINVAL;
>  
> -	return parent->ops->read(mdev, buf, count, ppos);
> +	return ops->read(mdev, buf, count, ppos);
>  }
>  
>  static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
> @@ -80,22 +84,24 @@ static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>  
> -	if (unlikely(!parent->ops->write))
> +	if (unlikely(!ops->write))
>  		return -EINVAL;
>  
> -	return parent->ops->write(mdev, buf, count, ppos);
> +	return ops->write(mdev, buf, count, ppos);
>  }
>  
>  static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>  
> -	if (unlikely(!parent->ops->mmap))
> +	if (unlikely(!ops->mmap))
>  		return -EINVAL;
>  
> -	return parent->ops->mmap(mdev, vma);
> +	return ops->mmap(mdev, vma);
>  }
>  
>  static const struct vfio_device_ops vfio_mdev_dev_ops = {
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index f85045392120..3b8a76bc69cf 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev, struct device *iommu_device);
>  struct device *mdev_get_iommu_device(struct device *dev);
>  
>  /**
> - * struct mdev_parent_ops - Structure to be registered for each parent device to
> - * register the device to mdev module.
> + * struct vfio_mdev_parent_ops - Structure to be registered for each
> + * parent device to register the device to vfio-mdev module.
>   *
> - * @owner:		The module owner.
> - * @dev_attr_groups:	Attributes of the parent device.
> - * @mdev_attr_groups:	Attributes of the mediated device.
> - * @supported_type_groups: Attributes to define supported types. It is mandatory
> - *			to provide supported types.
> - * @create:		Called to allocate basic resources in parent device's
> - *			driver for a particular mediated device. It is
> - *			mandatory to provide create ops.
> - *			@kobj: kobject of type for which 'create' is called.
> - *			@mdev: mdev_device structure on of mediated device
> - *			      that is being created
> - *			Returns integer: success (0) or error (< 0)
> - * @remove:		Called to free resources in parent device's driver for a
> - *			a mediated device. It is mandatory to provide 'remove'
> - *			ops.
> - *			@mdev: mdev_device device structure which is being
> - *			       destroyed
> - *			Returns integer: success (0) or error (< 0)
>   * @open:		Open mediated device.
>   *			@mdev: mediated device.
>   *			Returns integer: success (0) or error (< 0)
> @@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct device *dev);
>   * @mmap:		mmap callback
>   *			@mdev: mediated device structure
>   *			@vma: vma structure
> + */
> +struct vfio_mdev_parent_ops {
> +	int     (*open)(struct mdev_device *mdev);
> +	void    (*release)(struct mdev_device *mdev);
> +	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> +			size_t count, loff_t *ppos);
> +	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> +			 size_t count, loff_t *ppos);
> +	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> +			 unsigned long arg);
> +	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> +};
> +
> +/**
> + * struct mdev_parent_ops - Structure to be registered for each parent device to
> + * register the device to mdev module.
> + *
> + * @owner:		The module owner.
> + * @dev_attr_groups:	Attributes of the parent device.
> + * @mdev_attr_groups:	Attributes of the mediated device.
> + * @supported_type_groups: Attributes to define supported types. It is mandatory
> + *			to provide supported types.
> + * @create:		Called to allocate basic resources in parent device's
> + *			driver for a particular mediated device. It is
> + *			mandatory to provide create ops.
> + *			@kobj: kobject of type for which 'create' is called.
> + *			@mdev: mdev_device structure on of mediated device
> + *			      that is being created
> + *			Returns integer: success (0) or error (< 0)
> + * @remove:		Called to free resources in parent device's driver for a
> + *			a mediated device. It is mandatory to provide 'remove'
> + *			ops.
> + *			@mdev: mdev_device device structure which is being
> + *			       destroyed
> + *			Returns integer: success (0) or error (< 0)
> + * @device_ops:         Device specific emulation callback.
> + *
>   * Parent device that support mediated device should be registered with mdev
>   * module with mdev_parent_ops structure.
>   **/
> @@ -83,15 +102,7 @@ struct mdev_parent_ops {
>  
>  	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
>  	int     (*remove)(struct mdev_device *mdev);
> -	int     (*open)(struct mdev_device *mdev);
> -	void    (*release)(struct mdev_device *mdev);
> -	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> -			size_t count, loff_t *ppos);
> -	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> -			 size_t count, loff_t *ppos);
> -	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> -			 unsigned long arg);
> -	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> +	const void *device_ops;
>  };
>  
>  /* interface for exporting mdev supported type attributes */
> @@ -137,7 +148,8 @@ const guid_t *mdev_uuid(struct mdev_device *mdev);
>  
>  extern struct bus_type mdev_bus_type;
>  
> -int mdev_register_vfio_device(struct device *dev, const struct mdev_parent_ops *ops);
> +int mdev_register_vfio_device(struct device *dev,
> +                              const struct mdev_parent_ops *ops);
>  void mdev_unregister_device(struct device *dev);
>  
>  int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index 71a4469be85d..53ceb357f152 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -1418,12 +1418,7 @@ static struct attribute_group *mdev_type_groups[] = {
>  	NULL,
>  };
>  
> -static const struct mdev_parent_ops mdev_fops = {
> -	.owner			= THIS_MODULE,
> -	.mdev_attr_groups	= mdev_dev_groups,
> -	.supported_type_groups	= mdev_type_groups,
> -	.create			= mbochs_create,
> -	.remove			= mbochs_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>  	.open			= mbochs_open,
>  	.release		= mbochs_close,
>  	.read			= mbochs_read,
> @@ -1432,6 +1427,15 @@ static const struct mdev_parent_ops mdev_fops = {
>  	.mmap			= mbochs_mmap,
>  };
>  
> +static const struct mdev_parent_ops mdev_fops = {
> +	.owner			= THIS_MODULE,
> +	.mdev_attr_groups	= mdev_dev_groups,
> +	.supported_type_groups	= mdev_type_groups,
> +	.create			= mbochs_create,
> +	.remove			= mbochs_remove,
> +	.device_ops		= &vfio_mdev_ops,
> +};
> +
>  static const struct file_operations vd_fops = {
>  	.owner		= THIS_MODULE,
>  };
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index d3029dd27d91..4ba285a5768f 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -725,12 +725,7 @@ static struct attribute_group *mdev_type_groups[] = {
>  	NULL,
>  };
>  
> -static const struct mdev_parent_ops mdev_fops = {
> -	.owner			= THIS_MODULE,
> -	.mdev_attr_groups	= mdev_dev_groups,
> -	.supported_type_groups	= mdev_type_groups,
> -	.create			= mdpy_create,
> -	.remove			= mdpy_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>  	.open			= mdpy_open,
>  	.release		= mdpy_close,
>  	.read			= mdpy_read,
> @@ -739,6 +734,15 @@ static const struct mdev_parent_ops mdev_fops = {
>  	.mmap			= mdpy_mmap,
>  };
>  
> +static const struct mdev_parent_ops mdev_fops = {
> +	.owner			= THIS_MODULE,
> +	.mdev_attr_groups	= mdev_dev_groups,
> +	.supported_type_groups	= mdev_type_groups,
> +	.create			= mdpy_create,
> +	.remove			= mdpy_remove,
> +	.device_ops		= &vfio_mdev_ops,
> +};
> +
>  static const struct file_operations vd_fops = {
>  	.owner		= THIS_MODULE,
>  };
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index 744c88a6b22c..a468904ec626 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -1410,6 +1410,14 @@ static struct attribute_group *mdev_type_groups[] = {
>  	NULL,
>  };
>  
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> +	.open                   = mtty_open,
> +	.release                = mtty_close,
> +	.read                   = mtty_read,
> +	.write                  = mtty_write,
> +	.ioctl		        = mtty_ioctl,
> +};
> +
>  static const struct mdev_parent_ops mdev_fops = {
>  	.owner                  = THIS_MODULE,
>  	.dev_attr_groups        = mtty_dev_groups,
> @@ -1417,11 +1425,7 @@ static const struct mdev_parent_ops mdev_fops = {
>  	.supported_type_groups  = mdev_type_groups,
>  	.create                 = mtty_create,
>  	.remove			= mtty_remove,
> -	.open                   = mtty_open,
> -	.release                = mtty_close,
> -	.read                   = mtty_read,
> -	.write                  = mtty_write,
> -	.ioctl		        = mtty_ioctl,
> +	.device_ops             = &vfio_mdev_ops,
>  };
>  
>  static void mtty_device_release(struct device *dev)
> -- 
> 2.19.1

RE: [RFC PATCH 1/2] mdev: device id support

[Tian, Kevin]

> From: Jason Wang
> Sent: Thursday, September 12, 2019 5:40 PM
> 
> Mdev bus only support vfio driver right now, so it doesn't implement
> match method. But in the future, we may add drivers other than vfio,
> one example is virtio-mdev[1] driver. This means we need to add device
> id support in bus match method to pair the mdev device and mdev driver
> correctly.

"device id" sound a bit confusing to me - it usually means something
unique to each device, while here it is used to indicate expected driver
types (vfio, virtio, etc.). but using "bus id" is also not good - we have
only one mdev bus here. Then what about "class id"?

> 
> So this patch add id_table to mdev_driver and id for mdev parent, and
> implement the match method for mdev bus.
> 
> [1] https://lkml.org/lkml/2019/9/10/135
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c  |  2 +-
>  drivers/s390/cio/vfio_ccw_ops.c   |  2 +-
>  drivers/s390/crypto/vfio_ap_ops.c |  3 ++-
>  drivers/vfio/mdev/mdev_core.c     | 14 ++++++++++++--
>  drivers/vfio/mdev/mdev_driver.c   | 14 ++++++++++++++
>  drivers/vfio/mdev/mdev_private.h  |  1 +
>  drivers/vfio/mdev/vfio_mdev.c     |  6 ++++++
>  include/linux/mdev.h              |  6 +++++-
>  include/linux/mod_devicetable.h   |  6 ++++++
>  samples/vfio-mdev/mbochs.c        |  2 +-
>  samples/vfio-mdev/mdpy.c          |  2 +-
>  samples/vfio-mdev/mtty.c          |  2 +-
>  12 files changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 23aa3e50cbf8..19d51a35f019 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1625,7 +1625,7 @@ static int kvmgt_host_init(struct device *dev, void
> *gvt, const void *ops)
>  		return -EFAULT;
>  	intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;
> 
> -	return mdev_register_device(dev, &intel_vgpu_ops);
> +	return mdev_register_vfio_device(dev, &intel_vgpu_ops);
>  }
> 
>  static void kvmgt_host_exit(struct device *dev)
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> b/drivers/s390/cio/vfio_ccw_ops.c
> index 5eb61116ca6f..f87d9409e290 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -578,7 +578,7 @@ static const struct mdev_parent_ops
> vfio_ccw_mdev_ops = {
> 
>  int vfio_ccw_mdev_reg(struct subchannel *sch)
>  {
> -	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
> +	return mdev_register_vfio_device(&sch->dev,
> &vfio_ccw_mdev_ops);
>  }
> 
>  void vfio_ccw_mdev_unreg(struct subchannel *sch)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index 0604b49a4d32..eacbde3c7a97 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1295,7 +1295,8 @@ int vfio_ap_mdev_register(void)
>  {
>  	atomic_set(&matrix_dev->available_instances,
> MAX_ZDEV_ENTRIES_EXT);
> 
> -	return mdev_register_device(&matrix_dev->device,
> &vfio_ap_matrix_ops);
> +	return mdev_register_vfio_device(&matrix_dev->device,
> +					 &vfio_ap_matrix_ops);
>  }
> 
>  void vfio_ap_mdev_unregister(void)
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index b558d4cfd082..fc07ff3ebe96 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -135,11 +135,14 @@ static int mdev_device_remove_cb(struct device
> *dev, void *data)
>   * mdev_register_device : Register a device
>   * @dev: device structure representing parent device.
>   * @ops: Parent device operation structure to be registered.
> + * @id: device id.
>   *
>   * Add device to list of registered parent devices.
>   * Returns a negative value on error, otherwise 0.
>   */
> -int mdev_register_device(struct device *dev, const struct
> mdev_parent_ops *ops)
> +int mdev_register_device(struct device *dev,
> +			 const struct mdev_parent_ops *ops,
> +			 u8 id)
>  {
>  	int ret;
>  	struct mdev_parent *parent;
> @@ -175,6 +178,7 @@ int mdev_register_device(struct device *dev, const
> struct mdev_parent_ops *ops)
> 
>  	parent->dev = dev;
>  	parent->ops = ops;
> +	parent->device_id = id;
> 
>  	if (!mdev_bus_compat_class) {
>  		mdev_bus_compat_class =
> class_compat_register("mdev_bus");
> @@ -208,7 +212,13 @@ int mdev_register_device(struct device *dev, const
> struct mdev_parent_ops *ops)
>  		put_device(dev);
>  	return ret;
>  }
> -EXPORT_SYMBOL(mdev_register_device);
> +
> +int mdev_register_vfio_device(struct device *dev,
> +			      const struct mdev_parent_ops *ops)
> +{
> +	return mdev_register_device(dev, ops, MDEV_ID_VFIO);
> +}
> +EXPORT_SYMBOL(mdev_register_vfio_device);
> 
>  /*
>   * mdev_unregister_device : Unregister a parent device
> diff --git a/drivers/vfio/mdev/mdev_driver.c
> b/drivers/vfio/mdev/mdev_driver.c
> index 0d3223aee20b..fd5e9541d18e 100644
> --- a/drivers/vfio/mdev/mdev_driver.c
> +++ b/drivers/vfio/mdev/mdev_driver.c
> @@ -69,8 +69,22 @@ static int mdev_remove(struct device *dev)
>  	return 0;
>  }
> 
> +static int mdev_match(struct device *dev, struct device_driver *drv)
> +{
> +	unsigned int i;
> +	struct mdev_device *mdev = to_mdev_device(dev);
> +	struct mdev_driver *mdrv = to_mdev_driver(drv);
> +	const struct mdev_device_id *ids = mdrv->id_table;
> +
> +	for (i = 0; ids[i].id; i++)
> +		if (ids[i].id == mdev->parent->device_id)
> +			return 1;
> +	return 0;
> +}
> +
>  struct bus_type mdev_bus_type = {
>  	.name		= "mdev",
> +	.match		= mdev_match,
>  	.probe		= mdev_probe,
>  	.remove		= mdev_remove,
>  };
> diff --git a/drivers/vfio/mdev/mdev_private.h
> b/drivers/vfio/mdev/mdev_private.h
> index 7d922950caaf..7fc8153671e0 100644
> --- a/drivers/vfio/mdev/mdev_private.h
> +++ b/drivers/vfio/mdev/mdev_private.h
> @@ -22,6 +22,7 @@ struct mdev_parent {
>  	struct list_head type_list;
>  	/* Synchronize device creation/removal with parent unregistration
> */
>  	struct rw_semaphore unreg_sem;
> +	u8 device_id;
>  };
> 
>  struct mdev_device {
> diff --git a/drivers/vfio/mdev/vfio_mdev.c
> b/drivers/vfio/mdev/vfio_mdev.c
> index 30964a4e0a28..887c57f10880 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -120,10 +120,16 @@ static void vfio_mdev_remove(struct device *dev)
>  	vfio_del_group_dev(dev);
>  }
> 
> +static struct mdev_device_id id_table[] = {
> +	{ MDEV_ID_VFIO },
> +	{ 0 },
> +};
> +
>  static struct mdev_driver vfio_mdev_driver = {
>  	.name	= "vfio_mdev",
>  	.probe	= vfio_mdev_probe,
>  	.remove	= vfio_mdev_remove,
> +	.id_table = id_table,
>  };
> 
>  static int __init vfio_mdev_init(void)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 0ce30ca78db0..f85045392120 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -118,6 +118,7 @@ struct mdev_type_attribute
> mdev_type_attr_##_name =		\
>   * @probe: called when new device created
>   * @remove: called when device removed
>   * @driver: device driver structure
> + * @id_table: the ids serviced by this driver.
>   *
>   **/
>  struct mdev_driver {
> @@ -125,6 +126,7 @@ struct mdev_driver {
>  	int  (*probe)(struct device *dev);
>  	void (*remove)(struct device *dev);
>  	struct device_driver driver;
> +	const struct mdev_device_id *id_table;
>  };
> 
>  #define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
> @@ -135,7 +137,7 @@ const guid_t *mdev_uuid(struct mdev_device
> *mdev);
> 
>  extern struct bus_type mdev_bus_type;
> 
> -int mdev_register_device(struct device *dev, const struct
> mdev_parent_ops *ops);
> +int mdev_register_vfio_device(struct device *dev, const struct
> mdev_parent_ops *ops);
>  void mdev_unregister_device(struct device *dev);
> 
>  int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
> @@ -145,4 +147,6 @@ struct device *mdev_parent_dev(struct
> mdev_device *mdev);
>  struct device *mdev_dev(struct mdev_device *mdev);
>  struct mdev_device *mdev_from_dev(struct device *dev);
> 
> +#define MDEV_ID_VFIO 1 /* VFIO device */
> +
>  #endif /* MDEV_H */
> diff --git a/include/linux/mod_devicetable.h
> b/include/linux/mod_devicetable.h
> index 5714fd35a83c..f1fc143df042 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -821,4 +821,10 @@ struct wmi_device_id {
>  	const void *context;
>  };
> 
> +/* MDEV */
> +
> +struct mdev_device_id {
> +	__u8 id;
> +};
> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index ac5c8c17b1ff..71a4469be85d 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -1468,7 +1468,7 @@ static int __init mbochs_dev_init(void)
>  	if (ret)
>  		goto failed2;
> 
> -	ret = mdev_register_device(&mbochs_dev, &mdev_fops);
> +	ret = mdev_register_vfio_device(&mbochs_dev, &mdev_fops);
>  	if (ret)
>  		goto failed3;
> 
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index cc86bf6566e4..d3029dd27d91 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -775,7 +775,7 @@ static int __init mdpy_dev_init(void)
>  	if (ret)
>  		goto failed2;
> 
> -	ret = mdev_register_device(&mdpy_dev, &mdev_fops);
> +	ret = mdev_register_vfio_device(&mdpy_dev, &mdev_fops);
>  	if (ret)
>  		goto failed3;
> 
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index 92e770a06ea2..744c88a6b22c 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -1468,7 +1468,7 @@ static int __init mtty_dev_init(void)
>  	if (ret)
>  		goto failed2;
> 
> -	ret = mdev_register_device(&mtty_dev.dev, &mdev_fops);
> +	ret = mdev_register_vfio_device(&mtty_dev.dev, &mdev_fops);
>  	if (ret)
>  		goto failed3;
> 
> --
> 2.19.1
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

RE: [RFC PATCH 2/2] mdev: introduce device specific ops

[Tian, Kevin]

> From: Jason Wang
> Sent: Thursday, September 12, 2019 5:40 PM
> 
> Currently, except for the crate and remove. The rest fields of
> mdev_parent_ops is just designed for vfio-mdev driver and may not help
> for kernel mdev driver. So follow the device id support by previous
> patch, this patch introduces device specific ops which points to
> device specific ops (e.g vfio ops). This allows the future drivers
> like virtio-mdev to implement its own device specific ops.

Can you give an example about what ops might be required to support
kernel mdev driver? I know you posted a link earlier, but putting a small
example here can save time and avoid inconsistent understanding. Then
it will help whether the proposed split makes sense or there is a 
possibility of redefining the callbacks to meet the both requirements.
imo those callbacks fulfill some basic requirements when mediating
a device...

> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c  | 14 +++---
>  drivers/s390/cio/vfio_ccw_ops.c   | 14 +++---
>  drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
>  drivers/vfio/mdev/vfio_mdev.c     | 30 +++++++------
>  include/linux/mdev.h              | 72 ++++++++++++++++++-------------
>  samples/vfio-mdev/mbochs.c        | 16 ++++---
>  samples/vfio-mdev/mdpy.c          | 16 ++++---
>  samples/vfio-mdev/mtty.c          | 14 +++---
>  8 files changed, 113 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 19d51a35f019..64823998fd58 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1600,20 +1600,22 @@ 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_parent_ops intel_vfio_vgpu_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 = {
> +	.mdev_attr_groups       = intel_vgpu_groups,
> +	.create			= intel_vgpu_create,
> +	.remove			= intel_vgpu_remove,
> +	.device_ops	        = &intel_vfio_vgpu_ops,
> +};
> +
>  static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
>  {
>  	struct attribute **kvm_type_attrs;
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> b/drivers/s390/cio/vfio_ccw_ops.c
> index f87d9409e290..96e9f18100ae 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -564,11 +564,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct
> mdev_device *mdev,
>  	}
>  }
> 
> -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> -	.owner			= THIS_MODULE,
> -	.supported_type_groups  = mdev_type_groups,
> -	.create			= vfio_ccw_mdev_create,
> -	.remove			= vfio_ccw_mdev_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>  	.open			= vfio_ccw_mdev_open,
>  	.release		= vfio_ccw_mdev_release,
>  	.read			= vfio_ccw_mdev_read,
> @@ -576,6 +572,14 @@ static const struct mdev_parent_ops
> vfio_ccw_mdev_ops = {
>  	.ioctl			= vfio_ccw_mdev_ioctl,
>  };
> 
> +static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> +	.owner			= THIS_MODULE,
> +	.supported_type_groups  = mdev_type_groups,
> +	.create			= vfio_ccw_mdev_create,
> +	.remove			= vfio_ccw_mdev_remove,
> +	.device_ops		= &vfio_mdev_ops,
> +};
> +
>  int vfio_ccw_mdev_reg(struct subchannel *sch)
>  {
>  	return mdev_register_vfio_device(&sch->dev,
> &vfio_ccw_mdev_ops);
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index eacbde3c7a97..a48282bff066 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1280,15 +1280,19 @@ static ssize_t vfio_ap_mdev_ioctl(struct
> mdev_device *mdev,
>  	return ret;
>  }
> 
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> +	.open			= vfio_ap_mdev_open,
> +	.release		= vfio_ap_mdev_release,
> +	.ioctl			= vfio_ap_mdev_ioctl,
> +};
> +
>  static const struct mdev_parent_ops vfio_ap_matrix_ops = {
>  	.owner			= THIS_MODULE,
>  	.supported_type_groups	= vfio_ap_mdev_type_groups,
>  	.mdev_attr_groups	= vfio_ap_mdev_attr_groups,
>  	.create			= vfio_ap_mdev_create,
>  	.remove			= vfio_ap_mdev_remove,
> -	.open			= vfio_ap_mdev_open,
> -	.release		= vfio_ap_mdev_release,
> -	.ioctl			= vfio_ap_mdev_ioctl,
> +	.device_ops		= &vfio_mdev_ops,
>  };
> 
>  int vfio_ap_mdev_register(void)
> diff --git a/drivers/vfio/mdev/vfio_mdev.c
> b/drivers/vfio/mdev/vfio_mdev.c
> index 887c57f10880..1196fbb6c3d2 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -25,15 +25,16 @@ static int vfio_mdev_open(void *device_data)
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >device_ops;
>  	int ret;
> 
> -	if (unlikely(!parent->ops->open))
> +	if (unlikely(!ops->open))
>  		return -EINVAL;
> 
>  	if (!try_module_get(THIS_MODULE))
>  		return -ENODEV;
> 
> -	ret = parent->ops->open(mdev);
> +	ret = ops->open(mdev);
>  	if (ret)
>  		module_put(THIS_MODULE);
> 
> @@ -44,9 +45,10 @@ static void vfio_mdev_release(void *device_data)
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >device_ops;
> 
> -	if (likely(parent->ops->release))
> -		parent->ops->release(mdev);
> +	if (likely(ops->release))
> +		ops->release(mdev);
> 
>  	module_put(THIS_MODULE);
>  }
> @@ -56,11 +58,12 @@ static long vfio_mdev_unlocked_ioctl(void
> *device_data,
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >device_ops;
> 
> -	if (unlikely(!parent->ops->ioctl))
> +	if (unlikely(!ops->ioctl))
>  		return -EINVAL;
> 
> -	return parent->ops->ioctl(mdev, cmd, arg);
> +	return ops->ioctl(mdev, cmd, arg);
>  }
> 
>  static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> @@ -68,11 +71,12 @@ static ssize_t vfio_mdev_read(void *device_data,
> char __user *buf,
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >device_ops;
> 
> -	if (unlikely(!parent->ops->read))
> +	if (unlikely(!ops->read))
>  		return -EINVAL;
> 
> -	return parent->ops->read(mdev, buf, count, ppos);
> +	return ops->read(mdev, buf, count, ppos);
>  }
> 
>  static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
> @@ -80,22 +84,24 @@ static ssize_t vfio_mdev_write(void *device_data,
> const char __user *buf,
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >device_ops;
> 
> -	if (unlikely(!parent->ops->write))
> +	if (unlikely(!ops->write))
>  		return -EINVAL;
> 
> -	return parent->ops->write(mdev, buf, count, ppos);
> +	return ops->write(mdev, buf, count, ppos);
>  }
> 
>  static int vfio_mdev_mmap(void *device_data, struct vm_area_struct
> *vma)
>  {
>  	struct mdev_device *mdev = device_data;
>  	struct mdev_parent *parent = mdev->parent;
> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >device_ops;
> 
> -	if (unlikely(!parent->ops->mmap))
> +	if (unlikely(!ops->mmap))
>  		return -EINVAL;
> 
> -	return parent->ops->mmap(mdev, vma);
> +	return ops->mmap(mdev, vma);
>  }
> 
>  static const struct vfio_device_ops vfio_mdev_dev_ops = {
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index f85045392120..3b8a76bc69cf 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev,
> struct device *iommu_device);
>  struct device *mdev_get_iommu_device(struct device *dev);
> 
>  /**
> - * struct mdev_parent_ops - Structure to be registered for each parent
> device to
> - * register the device to mdev module.
> + * struct vfio_mdev_parent_ops - Structure to be registered for each
> + * parent device to register the device to vfio-mdev module.
>   *
> - * @owner:		The module owner.
> - * @dev_attr_groups:	Attributes of the parent device.
> - * @mdev_attr_groups:	Attributes of the mediated device.
> - * @supported_type_groups: Attributes to define supported types. It is
> mandatory
> - *			to provide supported types.
> - * @create:		Called to allocate basic resources in parent device's
> - *			driver for a particular mediated device. It is
> - *			mandatory to provide create ops.
> - *			@kobj: kobject of type for which 'create' is called.
> - *			@mdev: mdev_device structure on of mediated
> device
> - *			      that is being created
> - *			Returns integer: success (0) or error (< 0)
> - * @remove:		Called to free resources in parent device's driver for
> a
> - *			a mediated device. It is mandatory to provide
> 'remove'
> - *			ops.
> - *			@mdev: mdev_device device structure which is
> being
> - *			       destroyed
> - *			Returns integer: success (0) or error (< 0)
>   * @open:		Open mediated device.
>   *			@mdev: mediated device.
>   *			Returns integer: success (0) or error (< 0)
> @@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct
> device *dev);
>   * @mmap:		mmap callback
>   *			@mdev: mediated device structure
>   *			@vma: vma structure
> + */
> +struct vfio_mdev_parent_ops {
> +	int     (*open)(struct mdev_device *mdev);
> +	void    (*release)(struct mdev_device *mdev);
> +	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> +			size_t count, loff_t *ppos);
> +	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> +			 size_t count, loff_t *ppos);
> +	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> +			 unsigned long arg);
> +	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
> *vma);
> +};
> +
> +/**
> + * struct mdev_parent_ops - Structure to be registered for each parent
> device to
> + * register the device to mdev module.
> + *
> + * @owner:		The module owner.
> + * @dev_attr_groups:	Attributes of the parent device.
> + * @mdev_attr_groups:	Attributes of the mediated device.
> + * @supported_type_groups: Attributes to define supported types. It is
> mandatory
> + *			to provide supported types.
> + * @create:		Called to allocate basic resources in parent device's
> + *			driver for a particular mediated device. It is
> + *			mandatory to provide create ops.
> + *			@kobj: kobject of type for which 'create' is called.
> + *			@mdev: mdev_device structure on of mediated
> device
> + *			      that is being created
> + *			Returns integer: success (0) or error (< 0)
> + * @remove:		Called to free resources in parent device's driver for
> a
> + *			a mediated device. It is mandatory to provide
> 'remove'
> + *			ops.
> + *			@mdev: mdev_device device structure which is
> being
> + *			       destroyed
> + *			Returns integer: success (0) or error (< 0)
> + * @device_ops:         Device specific emulation callback.
> + *
>   * Parent device that support mediated device should be registered with
> mdev
>   * module with mdev_parent_ops structure.
>   **/
> @@ -83,15 +102,7 @@ struct mdev_parent_ops {
> 
>  	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
>  	int     (*remove)(struct mdev_device *mdev);
> -	int     (*open)(struct mdev_device *mdev);
> -	void    (*release)(struct mdev_device *mdev);
> -	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> -			size_t count, loff_t *ppos);
> -	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> -			 size_t count, loff_t *ppos);
> -	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> -			 unsigned long arg);
> -	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
> *vma);
> +	const void *device_ops;
>  };
> 
>  /* interface for exporting mdev supported type attributes */
> @@ -137,7 +148,8 @@ const guid_t *mdev_uuid(struct mdev_device
> *mdev);
> 
>  extern struct bus_type mdev_bus_type;
> 
> -int mdev_register_vfio_device(struct device *dev, const struct
> mdev_parent_ops *ops);
> +int mdev_register_vfio_device(struct device *dev,
> +                              const struct mdev_parent_ops *ops);
>  void mdev_unregister_device(struct device *dev);
> 
>  int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index 71a4469be85d..53ceb357f152 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -1418,12 +1418,7 @@ static struct attribute_group
> *mdev_type_groups[] = {
>  	NULL,
>  };
> 
> -static const struct mdev_parent_ops mdev_fops = {
> -	.owner			= THIS_MODULE,
> -	.mdev_attr_groups	= mdev_dev_groups,
> -	.supported_type_groups	= mdev_type_groups,
> -	.create			= mbochs_create,
> -	.remove			= mbochs_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>  	.open			= mbochs_open,
>  	.release		= mbochs_close,
>  	.read			= mbochs_read,
> @@ -1432,6 +1427,15 @@ static const struct mdev_parent_ops mdev_fops
> = {
>  	.mmap			= mbochs_mmap,
>  };
> 
> +static const struct mdev_parent_ops mdev_fops = {
> +	.owner			= THIS_MODULE,
> +	.mdev_attr_groups	= mdev_dev_groups,
> +	.supported_type_groups	= mdev_type_groups,
> +	.create			= mbochs_create,
> +	.remove			= mbochs_remove,
> +	.device_ops		= &vfio_mdev_ops,
> +};
> +
>  static const struct file_operations vd_fops = {
>  	.owner		= THIS_MODULE,
>  };
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index d3029dd27d91..4ba285a5768f 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -725,12 +725,7 @@ static struct attribute_group *mdev_type_groups[]
> = {
>  	NULL,
>  };
> 
> -static const struct mdev_parent_ops mdev_fops = {
> -	.owner			= THIS_MODULE,
> -	.mdev_attr_groups	= mdev_dev_groups,
> -	.supported_type_groups	= mdev_type_groups,
> -	.create			= mdpy_create,
> -	.remove			= mdpy_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>  	.open			= mdpy_open,
>  	.release		= mdpy_close,
>  	.read			= mdpy_read,
> @@ -739,6 +734,15 @@ static const struct mdev_parent_ops mdev_fops =
> {
>  	.mmap			= mdpy_mmap,
>  };
> 
> +static const struct mdev_parent_ops mdev_fops = {
> +	.owner			= THIS_MODULE,
> +	.mdev_attr_groups	= mdev_dev_groups,
> +	.supported_type_groups	= mdev_type_groups,
> +	.create			= mdpy_create,
> +	.remove			= mdpy_remove,
> +	.device_ops		= &vfio_mdev_ops,
> +};
> +
>  static const struct file_operations vd_fops = {
>  	.owner		= THIS_MODULE,
>  };
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index 744c88a6b22c..a468904ec626 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -1410,6 +1410,14 @@ static struct attribute_group
> *mdev_type_groups[] = {
>  	NULL,
>  };
> 
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> +	.open                   = mtty_open,
> +	.release                = mtty_close,
> +	.read                   = mtty_read,
> +	.write                  = mtty_write,
> +	.ioctl		        = mtty_ioctl,
> +};
> +
>  static const struct mdev_parent_ops mdev_fops = {
>  	.owner                  = THIS_MODULE,
>  	.dev_attr_groups        = mtty_dev_groups,
> @@ -1417,11 +1425,7 @@ static const struct mdev_parent_ops mdev_fops
> = {
>  	.supported_type_groups  = mdev_type_groups,
>  	.create                 = mtty_create,
>  	.remove			= mtty_remove,
> -	.open                   = mtty_open,
> -	.release                = mtty_close,
> -	.read                   = mtty_read,
> -	.write                  = mtty_write,
> -	.ioctl		        = mtty_ioctl,
> +	.device_ops             = &vfio_mdev_ops,
>  };
> 
>  static void mtty_device_release(struct device *dev)
> --
> 2.19.1
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH 1/2] mdev: device id support

[Jason Wang]

On 2019/9/17 下午3:55, Tian, Kevin wrote:
>> From: Jason Wang
>> Sent: Thursday, September 12, 2019 5:40 PM
>>
>> Mdev bus only support vfio driver right now, so it doesn't implement
>> match method. But in the future, we may add drivers other than vfio,
>> one example is virtio-mdev[1] driver. This means we need to add device
>> id support in bus match method to pair the mdev device and mdev driver
>> correctly.
> "device id" sound a bit confusing to me - it usually means something
> unique to each device, while here it is used to indicate expected driver
> types (vfio, virtio, etc.). but using "bus id" is also not good - we have
> only one mdev bus here. Then what about "class id"?


I'm fine with this.

Thanks


>
>> So this patch add id_table to mdev_driver and id for mdev parent, and
>> implement the match method for mdev bus.
>>
>> [1] https://lkml.org/lkml/2019/9/10/135
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/kvmgt.c  |  2 +-
>>   drivers/s390/cio/vfio_ccw_ops.c   |  2 +-
>>   drivers/s390/crypto/vfio_ap_ops.c |  3 ++-
>>   drivers/vfio/mdev/mdev_core.c     | 14 ++++++++++++--
>>   drivers/vfio/mdev/mdev_driver.c   | 14 ++++++++++++++
>>   drivers/vfio/mdev/mdev_private.h  |  1 +
>>   drivers/vfio/mdev/vfio_mdev.c     |  6 ++++++
>>   include/linux/mdev.h              |  6 +++++-
>>   include/linux/mod_devicetable.h   |  6 ++++++
>>   samples/vfio-mdev/mbochs.c        |  2 +-
>>   samples/vfio-mdev/mdpy.c          |  2 +-
>>   samples/vfio-mdev/mtty.c          |  2 +-
>>   12 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 23aa3e50cbf8..19d51a35f019 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -1625,7 +1625,7 @@ static int kvmgt_host_init(struct device *dev, void
>> *gvt, const void *ops)
>>   		return -EFAULT;
>>   	intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;
>>
>> -	return mdev_register_device(dev, &intel_vgpu_ops);
>> +	return mdev_register_vfio_device(dev, &intel_vgpu_ops);
>>   }
>>
>>   static void kvmgt_host_exit(struct device *dev)
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
>> b/drivers/s390/cio/vfio_ccw_ops.c
>> index 5eb61116ca6f..f87d9409e290 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -578,7 +578,7 @@ static const struct mdev_parent_ops
>> vfio_ccw_mdev_ops = {
>>
>>   int vfio_ccw_mdev_reg(struct subchannel *sch)
>>   {
>> -	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
>> +	return mdev_register_vfio_device(&sch->dev,
>> &vfio_ccw_mdev_ops);
>>   }
>>
>>   void vfio_ccw_mdev_unreg(struct subchannel *sch)
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 0604b49a4d32..eacbde3c7a97 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -1295,7 +1295,8 @@ int vfio_ap_mdev_register(void)
>>   {
>>   	atomic_set(&matrix_dev->available_instances,
>> MAX_ZDEV_ENTRIES_EXT);
>>
>> -	return mdev_register_device(&matrix_dev->device,
>> &vfio_ap_matrix_ops);
>> +	return mdev_register_vfio_device(&matrix_dev->device,
>> +					 &vfio_ap_matrix_ops);
>>   }
>>
>>   void vfio_ap_mdev_unregister(void)
>> diff --git a/drivers/vfio/mdev/mdev_core.c
>> b/drivers/vfio/mdev/mdev_core.c
>> index b558d4cfd082..fc07ff3ebe96 100644
>> --- a/drivers/vfio/mdev/mdev_core.c
>> +++ b/drivers/vfio/mdev/mdev_core.c
>> @@ -135,11 +135,14 @@ static int mdev_device_remove_cb(struct device
>> *dev, void *data)
>>    * mdev_register_device : Register a device
>>    * @dev: device structure representing parent device.
>>    * @ops: Parent device operation structure to be registered.
>> + * @id: device id.
>>    *
>>    * Add device to list of registered parent devices.
>>    * Returns a negative value on error, otherwise 0.
>>    */
>> -int mdev_register_device(struct device *dev, const struct
>> mdev_parent_ops *ops)
>> +int mdev_register_device(struct device *dev,
>> +			 const struct mdev_parent_ops *ops,
>> +			 u8 id)
>>   {
>>   	int ret;
>>   	struct mdev_parent *parent;
>> @@ -175,6 +178,7 @@ int mdev_register_device(struct device *dev, const
>> struct mdev_parent_ops *ops)
>>
>>   	parent->dev = dev;
>>   	parent->ops = ops;
>> +	parent->device_id = id;
>>
>>   	if (!mdev_bus_compat_class) {
>>   		mdev_bus_compat_class =
>> class_compat_register("mdev_bus");
>> @@ -208,7 +212,13 @@ int mdev_register_device(struct device *dev, const
>> struct mdev_parent_ops *ops)
>>   		put_device(dev);
>>   	return ret;
>>   }
>> -EXPORT_SYMBOL(mdev_register_device);
>> +
>> +int mdev_register_vfio_device(struct device *dev,
>> +			      const struct mdev_parent_ops *ops)
>> +{
>> +	return mdev_register_device(dev, ops, MDEV_ID_VFIO);
>> +}
>> +EXPORT_SYMBOL(mdev_register_vfio_device);
>>
>>   /*
>>    * mdev_unregister_device : Unregister a parent device
>> diff --git a/drivers/vfio/mdev/mdev_driver.c
>> b/drivers/vfio/mdev/mdev_driver.c
>> index 0d3223aee20b..fd5e9541d18e 100644
>> --- a/drivers/vfio/mdev/mdev_driver.c
>> +++ b/drivers/vfio/mdev/mdev_driver.c
>> @@ -69,8 +69,22 @@ static int mdev_remove(struct device *dev)
>>   	return 0;
>>   }
>>
>> +static int mdev_match(struct device *dev, struct device_driver *drv)
>> +{
>> +	unsigned int i;
>> +	struct mdev_device *mdev = to_mdev_device(dev);
>> +	struct mdev_driver *mdrv = to_mdev_driver(drv);
>> +	const struct mdev_device_id *ids = mdrv->id_table;
>> +
>> +	for (i = 0; ids[i].id; i++)
>> +		if (ids[i].id == mdev->parent->device_id)
>> +			return 1;
>> +	return 0;
>> +}
>> +
>>   struct bus_type mdev_bus_type = {
>>   	.name		= "mdev",
>> +	.match		= mdev_match,
>>   	.probe		= mdev_probe,
>>   	.remove		= mdev_remove,
>>   };
>> diff --git a/drivers/vfio/mdev/mdev_private.h
>> b/drivers/vfio/mdev/mdev_private.h
>> index 7d922950caaf..7fc8153671e0 100644
>> --- a/drivers/vfio/mdev/mdev_private.h
>> +++ b/drivers/vfio/mdev/mdev_private.h
>> @@ -22,6 +22,7 @@ struct mdev_parent {
>>   	struct list_head type_list;
>>   	/* Synchronize device creation/removal with parent unregistration
>> */
>>   	struct rw_semaphore unreg_sem;
>> +	u8 device_id;
>>   };
>>
>>   struct mdev_device {
>> diff --git a/drivers/vfio/mdev/vfio_mdev.c
>> b/drivers/vfio/mdev/vfio_mdev.c
>> index 30964a4e0a28..887c57f10880 100644
>> --- a/drivers/vfio/mdev/vfio_mdev.c
>> +++ b/drivers/vfio/mdev/vfio_mdev.c
>> @@ -120,10 +120,16 @@ static void vfio_mdev_remove(struct device *dev)
>>   	vfio_del_group_dev(dev);
>>   }
>>
>> +static struct mdev_device_id id_table[] = {
>> +	{ MDEV_ID_VFIO },
>> +	{ 0 },
>> +};
>> +
>>   static struct mdev_driver vfio_mdev_driver = {
>>   	.name	= "vfio_mdev",
>>   	.probe	= vfio_mdev_probe,
>>   	.remove	= vfio_mdev_remove,
>> +	.id_table = id_table,
>>   };
>>
>>   static int __init vfio_mdev_init(void)
>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
>> index 0ce30ca78db0..f85045392120 100644
>> --- a/include/linux/mdev.h
>> +++ b/include/linux/mdev.h
>> @@ -118,6 +118,7 @@ struct mdev_type_attribute
>> mdev_type_attr_##_name =		\
>>    * @probe: called when new device created
>>    * @remove: called when device removed
>>    * @driver: device driver structure
>> + * @id_table: the ids serviced by this driver.
>>    *
>>    **/
>>   struct mdev_driver {
>> @@ -125,6 +126,7 @@ struct mdev_driver {
>>   	int  (*probe)(struct device *dev);
>>   	void (*remove)(struct device *dev);
>>   	struct device_driver driver;
>> +	const struct mdev_device_id *id_table;
>>   };
>>
>>   #define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
>> @@ -135,7 +137,7 @@ const guid_t *mdev_uuid(struct mdev_device
>> *mdev);
>>
>>   extern struct bus_type mdev_bus_type;
>>
>> -int mdev_register_device(struct device *dev, const struct
>> mdev_parent_ops *ops);
>> +int mdev_register_vfio_device(struct device *dev, const struct
>> mdev_parent_ops *ops);
>>   void mdev_unregister_device(struct device *dev);
>>
>>   int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
>> @@ -145,4 +147,6 @@ struct device *mdev_parent_dev(struct
>> mdev_device *mdev);
>>   struct device *mdev_dev(struct mdev_device *mdev);
>>   struct mdev_device *mdev_from_dev(struct device *dev);
>>
>> +#define MDEV_ID_VFIO 1 /* VFIO device */
>> +
>>   #endif /* MDEV_H */
>> diff --git a/include/linux/mod_devicetable.h
>> b/include/linux/mod_devicetable.h
>> index 5714fd35a83c..f1fc143df042 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -821,4 +821,10 @@ struct wmi_device_id {
>>   	const void *context;
>>   };
>>
>> +/* MDEV */
>> +
>> +struct mdev_device_id {
>> +	__u8 id;
>> +};
>> +
>>   #endif /* LINUX_MOD_DEVICETABLE_H */
>> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
>> index ac5c8c17b1ff..71a4469be85d 100644
>> --- a/samples/vfio-mdev/mbochs.c
>> +++ b/samples/vfio-mdev/mbochs.c
>> @@ -1468,7 +1468,7 @@ static int __init mbochs_dev_init(void)
>>   	if (ret)
>>   		goto failed2;
>>
>> -	ret = mdev_register_device(&mbochs_dev, &mdev_fops);
>> +	ret = mdev_register_vfio_device(&mbochs_dev, &mdev_fops);
>>   	if (ret)
>>   		goto failed3;
>>
>> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
>> index cc86bf6566e4..d3029dd27d91 100644
>> --- a/samples/vfio-mdev/mdpy.c
>> +++ b/samples/vfio-mdev/mdpy.c
>> @@ -775,7 +775,7 @@ static int __init mdpy_dev_init(void)
>>   	if (ret)
>>   		goto failed2;
>>
>> -	ret = mdev_register_device(&mdpy_dev, &mdev_fops);
>> +	ret = mdev_register_vfio_device(&mdpy_dev, &mdev_fops);
>>   	if (ret)
>>   		goto failed3;
>>
>> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
>> index 92e770a06ea2..744c88a6b22c 100644
>> --- a/samples/vfio-mdev/mtty.c
>> +++ b/samples/vfio-mdev/mtty.c
>> @@ -1468,7 +1468,7 @@ static int __init mtty_dev_init(void)
>>   	if (ret)
>>   		goto failed2;
>>
>> -	ret = mdev_register_device(&mtty_dev.dev, &mdev_fops);
>> +	ret = mdev_register_vfio_device(&mtty_dev.dev, &mdev_fops);
>>   	if (ret)
>>   		goto failed3;
>>
>> --
>> 2.19.1
>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [RFC PATCH 2/2] mdev: introduce device specific ops

[Jason Wang]

On 2019/9/17 下午4:09, Tian, Kevin wrote:
>> From: Jason Wang
>> Sent: Thursday, September 12, 2019 5:40 PM
>>
>> Currently, except for the crate and remove. The rest fields of
>> mdev_parent_ops is just designed for vfio-mdev driver and may not help
>> for kernel mdev driver. So follow the device id support by previous
>> patch, this patch introduces device specific ops which points to
>> device specific ops (e.g vfio ops). This allows the future drivers
>> like virtio-mdev to implement its own device specific ops.
> Can you give an example about what ops might be required to support
> kernel mdev driver? I know you posted a link earlier, but putting a small
> example here can save time and avoid inconsistent understanding. Then
> it will help whether the proposed split makes sense or there is a
> possibility of redefining the callbacks to meet the both requirements.
> imo those callbacks fulfill some basic requirements when mediating
> a device...


I put it in the cover letter.

The link is https://lkml.org/lkml/2019/9/10/135 which abuses the current 
VFIO based mdev parent ops.

Thanks


>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/kvmgt.c  | 14 +++---
>>   drivers/s390/cio/vfio_ccw_ops.c   | 14 +++---
>>   drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
>>   drivers/vfio/mdev/vfio_mdev.c     | 30 +++++++------
>>   include/linux/mdev.h              | 72 ++++++++++++++++++-------------
>>   samples/vfio-mdev/mbochs.c        | 16 ++++---
>>   samples/vfio-mdev/mdpy.c          | 16 ++++---
>>   samples/vfio-mdev/mtty.c          | 14 +++---
>>   8 files changed, 113 insertions(+), 73 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 19d51a35f019..64823998fd58 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -1600,20 +1600,22 @@ 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_parent_ops intel_vfio_vgpu_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 = {
>> +	.mdev_attr_groups       = intel_vgpu_groups,
>> +	.create			= intel_vgpu_create,
>> +	.remove			= intel_vgpu_remove,
>> +	.device_ops	        = &intel_vfio_vgpu_ops,
>> +};
>> +
>>   static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
>>   {
>>   	struct attribute **kvm_type_attrs;
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
>> b/drivers/s390/cio/vfio_ccw_ops.c
>> index f87d9409e290..96e9f18100ae 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -564,11 +564,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct
>> mdev_device *mdev,
>>   	}
>>   }
>>
>> -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>> -	.owner			= THIS_MODULE,
>> -	.supported_type_groups  = mdev_type_groups,
>> -	.create			= vfio_ccw_mdev_create,
>> -	.remove			= vfio_ccw_mdev_remove,
>> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>>   	.open			= vfio_ccw_mdev_open,
>>   	.release		= vfio_ccw_mdev_release,
>>   	.read			= vfio_ccw_mdev_read,
>> @@ -576,6 +572,14 @@ static const struct mdev_parent_ops
>> vfio_ccw_mdev_ops = {
>>   	.ioctl			= vfio_ccw_mdev_ioctl,
>>   };
>>
>> +static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>> +	.owner			= THIS_MODULE,
>> +	.supported_type_groups  = mdev_type_groups,
>> +	.create			= vfio_ccw_mdev_create,
>> +	.remove			= vfio_ccw_mdev_remove,
>> +	.device_ops		= &vfio_mdev_ops,
>> +};
>> +
>>   int vfio_ccw_mdev_reg(struct subchannel *sch)
>>   {
>>   	return mdev_register_vfio_device(&sch->dev,
>> &vfio_ccw_mdev_ops);
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index eacbde3c7a97..a48282bff066 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -1280,15 +1280,19 @@ static ssize_t vfio_ap_mdev_ioctl(struct
>> mdev_device *mdev,
>>   	return ret;
>>   }
>>
>> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>> +	.open			= vfio_ap_mdev_open,
>> +	.release		= vfio_ap_mdev_release,
>> +	.ioctl			= vfio_ap_mdev_ioctl,
>> +};
>> +
>>   static const struct mdev_parent_ops vfio_ap_matrix_ops = {
>>   	.owner			= THIS_MODULE,
>>   	.supported_type_groups	= vfio_ap_mdev_type_groups,
>>   	.mdev_attr_groups	= vfio_ap_mdev_attr_groups,
>>   	.create			= vfio_ap_mdev_create,
>>   	.remove			= vfio_ap_mdev_remove,
>> -	.open			= vfio_ap_mdev_open,
>> -	.release		= vfio_ap_mdev_release,
>> -	.ioctl			= vfio_ap_mdev_ioctl,
>> +	.device_ops		= &vfio_mdev_ops,
>>   };
>>
>>   int vfio_ap_mdev_register(void)
>> diff --git a/drivers/vfio/mdev/vfio_mdev.c
>> b/drivers/vfio/mdev/vfio_mdev.c
>> index 887c57f10880..1196fbb6c3d2 100644
>> --- a/drivers/vfio/mdev/vfio_mdev.c
>> +++ b/drivers/vfio/mdev/vfio_mdev.c
>> @@ -25,15 +25,16 @@ static int vfio_mdev_open(void *device_data)
>>   {
>>   	struct mdev_device *mdev = device_data;
>>   	struct mdev_parent *parent = mdev->parent;
>> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
>>> device_ops;
>>   	int ret;
>>
>> -	if (unlikely(!parent->ops->open))
>> +	if (unlikely(!ops->open))
>>   		return -EINVAL;
>>
>>   	if (!try_module_get(THIS_MODULE))
>>   		return -ENODEV;
>>
>> -	ret = parent->ops->open(mdev);
>> +	ret = ops->open(mdev);
>>   	if (ret)
>>   		module_put(THIS_MODULE);
>>
>> @@ -44,9 +45,10 @@ static void vfio_mdev_release(void *device_data)
>>   {
>>   	struct mdev_device *mdev = device_data;
>>   	struct mdev_parent *parent = mdev->parent;
>> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
>>> device_ops;
>> -	if (likely(parent->ops->release))
>> -		parent->ops->release(mdev);
>> +	if (likely(ops->release))
>> +		ops->release(mdev);
>>
>>   	module_put(THIS_MODULE);
>>   }
>> @@ -56,11 +58,12 @@ static long vfio_mdev_unlocked_ioctl(void
>> *device_data,
>>   {
>>   	struct mdev_device *mdev = device_data;
>>   	struct mdev_parent *parent = mdev->parent;
>> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
>>> device_ops;
>> -	if (unlikely(!parent->ops->ioctl))
>> +	if (unlikely(!ops->ioctl))
>>   		return -EINVAL;
>>
>> -	return parent->ops->ioctl(mdev, cmd, arg);
>> +	return ops->ioctl(mdev, cmd, arg);
>>   }
>>
>>   static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
>> @@ -68,11 +71,12 @@ static ssize_t vfio_mdev_read(void *device_data,
>> char __user *buf,
>>   {
>>   	struct mdev_device *mdev = device_data;
>>   	struct mdev_parent *parent = mdev->parent;
>> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
>>> device_ops;
>> -	if (unlikely(!parent->ops->read))
>> +	if (unlikely(!ops->read))
>>   		return -EINVAL;
>>
>> -	return parent->ops->read(mdev, buf, count, ppos);
>> +	return ops->read(mdev, buf, count, ppos);
>>   }
>>
>>   static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
>> @@ -80,22 +84,24 @@ static ssize_t vfio_mdev_write(void *device_data,
>> const char __user *buf,
>>   {
>>   	struct mdev_device *mdev = device_data;
>>   	struct mdev_parent *parent = mdev->parent;
>> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
>>> device_ops;
>> -	if (unlikely(!parent->ops->write))
>> +	if (unlikely(!ops->write))
>>   		return -EINVAL;
>>
>> -	return parent->ops->write(mdev, buf, count, ppos);
>> +	return ops->write(mdev, buf, count, ppos);
>>   }
>>
>>   static int vfio_mdev_mmap(void *device_data, struct vm_area_struct
>> *vma)
>>   {
>>   	struct mdev_device *mdev = device_data;
>>   	struct mdev_parent *parent = mdev->parent;
>> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
>>> device_ops;
>> -	if (unlikely(!parent->ops->mmap))
>> +	if (unlikely(!ops->mmap))
>>   		return -EINVAL;
>>
>> -	return parent->ops->mmap(mdev, vma);
>> +	return ops->mmap(mdev, vma);
>>   }
>>
>>   static const struct vfio_device_ops vfio_mdev_dev_ops = {
>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
>> index f85045392120..3b8a76bc69cf 100644
>> --- a/include/linux/mdev.h
>> +++ b/include/linux/mdev.h
>> @@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev,
>> struct device *iommu_device);
>>   struct device *mdev_get_iommu_device(struct device *dev);
>>
>>   /**
>> - * struct mdev_parent_ops - Structure to be registered for each parent
>> device to
>> - * register the device to mdev module.
>> + * struct vfio_mdev_parent_ops - Structure to be registered for each
>> + * parent device to register the device to vfio-mdev module.
>>    *
>> - * @owner:		The module owner.
>> - * @dev_attr_groups:	Attributes of the parent device.
>> - * @mdev_attr_groups:	Attributes of the mediated device.
>> - * @supported_type_groups: Attributes to define supported types. It is
>> mandatory
>> - *			to provide supported types.
>> - * @create:		Called to allocate basic resources in parent device's
>> - *			driver for a particular mediated device. It is
>> - *			mandatory to provide create ops.
>> - *			@kobj: kobject of type for which 'create' is called.
>> - *			@mdev: mdev_device structure on of mediated
>> device
>> - *			      that is being created
>> - *			Returns integer: success (0) or error (< 0)
>> - * @remove:		Called to free resources in parent device's driver for
>> a
>> - *			a mediated device. It is mandatory to provide
>> 'remove'
>> - *			ops.
>> - *			@mdev: mdev_device device structure which is
>> being
>> - *			       destroyed
>> - *			Returns integer: success (0) or error (< 0)
>>    * @open:		Open mediated device.
>>    *			@mdev: mediated device.
>>    *			Returns integer: success (0) or error (< 0)
>> @@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct
>> device *dev);
>>    * @mmap:		mmap callback
>>    *			@mdev: mediated device structure
>>    *			@vma: vma structure
>> + */
>> +struct vfio_mdev_parent_ops {
>> +	int     (*open)(struct mdev_device *mdev);
>> +	void    (*release)(struct mdev_device *mdev);
>> +	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
>> +			size_t count, loff_t *ppos);
>> +	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
>> +			 size_t count, loff_t *ppos);
>> +	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
>> +			 unsigned long arg);
>> +	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
>> *vma);
>> +};
>> +
>> +/**
>> + * struct mdev_parent_ops - Structure to be registered for each parent
>> device to
>> + * register the device to mdev module.
>> + *
>> + * @owner:		The module owner.
>> + * @dev_attr_groups:	Attributes of the parent device.
>> + * @mdev_attr_groups:	Attributes of the mediated device.
>> + * @supported_type_groups: Attributes to define supported types. It is
>> mandatory
>> + *			to provide supported types.
>> + * @create:		Called to allocate basic resources in parent device's
>> + *			driver for a particular mediated device. It is
>> + *			mandatory to provide create ops.
>> + *			@kobj: kobject of type for which 'create' is called.
>> + *			@mdev: mdev_device structure on of mediated
>> device
>> + *			      that is being created
>> + *			Returns integer: success (0) or error (< 0)
>> + * @remove:		Called to free resources in parent device's driver for
>> a
>> + *			a mediated device. It is mandatory to provide
>> 'remove'
>> + *			ops.
>> + *			@mdev: mdev_device device structure which is
>> being
>> + *			       destroyed
>> + *			Returns integer: success (0) or error (< 0)
>> + * @device_ops:         Device specific emulation callback.
>> + *
>>    * Parent device that support mediated device should be registered with
>> mdev
>>    * module with mdev_parent_ops structure.
>>    **/
>> @@ -83,15 +102,7 @@ struct mdev_parent_ops {
>>
>>   	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
>>   	int     (*remove)(struct mdev_device *mdev);
>> -	int     (*open)(struct mdev_device *mdev);
>> -	void    (*release)(struct mdev_device *mdev);
>> -	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
>> -			size_t count, loff_t *ppos);
>> -	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
>> -			 size_t count, loff_t *ppos);
>> -	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
>> -			 unsigned long arg);
>> -	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
>> *vma);
>> +	const void *device_ops;
>>   };
>>
>>   /* interface for exporting mdev supported type attributes */
>> @@ -137,7 +148,8 @@ const guid_t *mdev_uuid(struct mdev_device
>> *mdev);
>>
>>   extern struct bus_type mdev_bus_type;
>>
>> -int mdev_register_vfio_device(struct device *dev, const struct
>> mdev_parent_ops *ops);
>> +int mdev_register_vfio_device(struct device *dev,
>> +                              const struct mdev_parent_ops *ops);
>>   void mdev_unregister_device(struct device *dev);
>>
>>   int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
>> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
>> index 71a4469be85d..53ceb357f152 100644
>> --- a/samples/vfio-mdev/mbochs.c
>> +++ b/samples/vfio-mdev/mbochs.c
>> @@ -1418,12 +1418,7 @@ static struct attribute_group
>> *mdev_type_groups[] = {
>>   	NULL,
>>   };
>>
>> -static const struct mdev_parent_ops mdev_fops = {
>> -	.owner			= THIS_MODULE,
>> -	.mdev_attr_groups	= mdev_dev_groups,
>> -	.supported_type_groups	= mdev_type_groups,
>> -	.create			= mbochs_create,
>> -	.remove			= mbochs_remove,
>> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>>   	.open			= mbochs_open,
>>   	.release		= mbochs_close,
>>   	.read			= mbochs_read,
>> @@ -1432,6 +1427,15 @@ static const struct mdev_parent_ops mdev_fops
>> = {
>>   	.mmap			= mbochs_mmap,
>>   };
>>
>> +static const struct mdev_parent_ops mdev_fops = {
>> +	.owner			= THIS_MODULE,
>> +	.mdev_attr_groups	= mdev_dev_groups,
>> +	.supported_type_groups	= mdev_type_groups,
>> +	.create			= mbochs_create,
>> +	.remove			= mbochs_remove,
>> +	.device_ops		= &vfio_mdev_ops,
>> +};
>> +
>>   static const struct file_operations vd_fops = {
>>   	.owner		= THIS_MODULE,
>>   };
>> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
>> index d3029dd27d91..4ba285a5768f 100644
>> --- a/samples/vfio-mdev/mdpy.c
>> +++ b/samples/vfio-mdev/mdpy.c
>> @@ -725,12 +725,7 @@ static struct attribute_group *mdev_type_groups[]
>> = {
>>   	NULL,
>>   };
>>
>> -static const struct mdev_parent_ops mdev_fops = {
>> -	.owner			= THIS_MODULE,
>> -	.mdev_attr_groups	= mdev_dev_groups,
>> -	.supported_type_groups	= mdev_type_groups,
>> -	.create			= mdpy_create,
>> -	.remove			= mdpy_remove,
>> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>>   	.open			= mdpy_open,
>>   	.release		= mdpy_close,
>>   	.read			= mdpy_read,
>> @@ -739,6 +734,15 @@ static const struct mdev_parent_ops mdev_fops =
>> {
>>   	.mmap			= mdpy_mmap,
>>   };
>>
>> +static const struct mdev_parent_ops mdev_fops = {
>> +	.owner			= THIS_MODULE,
>> +	.mdev_attr_groups	= mdev_dev_groups,
>> +	.supported_type_groups	= mdev_type_groups,
>> +	.create			= mdpy_create,
>> +	.remove			= mdpy_remove,
>> +	.device_ops		= &vfio_mdev_ops,
>> +};
>> +
>>   static const struct file_operations vd_fops = {
>>   	.owner		= THIS_MODULE,
>>   };
>> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
>> index 744c88a6b22c..a468904ec626 100644
>> --- a/samples/vfio-mdev/mtty.c
>> +++ b/samples/vfio-mdev/mtty.c
>> @@ -1410,6 +1410,14 @@ static struct attribute_group
>> *mdev_type_groups[] = {
>>   	NULL,
>>   };
>>
>> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
>> +	.open                   = mtty_open,
>> +	.release                = mtty_close,
>> +	.read                   = mtty_read,
>> +	.write                  = mtty_write,
>> +	.ioctl		        = mtty_ioctl,
>> +};
>> +
>>   static const struct mdev_parent_ops mdev_fops = {
>>   	.owner                  = THIS_MODULE,
>>   	.dev_attr_groups        = mtty_dev_groups,
>> @@ -1417,11 +1425,7 @@ static const struct mdev_parent_ops mdev_fops
>> = {
>>   	.supported_type_groups  = mdev_type_groups,
>>   	.create                 = mtty_create,
>>   	.remove			= mtty_remove,
>> -	.open                   = mtty_open,
>> -	.release                = mtty_close,
>> -	.read                   = mtty_read,
>> -	.write                  = mtty_write,
>> -	.ioctl		        = mtty_ioctl,
>> +	.device_ops             = &vfio_mdev_ops,
>>   };
>>
>>   static void mtty_device_release(struct device *dev)
>> --
>> 2.19.1
>>
>> _______________________________________________
>> Virtualization mailing list
>> Virtualization@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH 1/2] mdev: device id support

[Cornelia Huck]

On Thu, 12 Sep 2019 17:40:11 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Mdev bus only support vfio driver right now, so it doesn't implement
> match method. But in the future, we may add drivers other than vfio,
> one example is virtio-mdev[1] driver. This means we need to add device
> id support in bus match method to pair the mdev device and mdev driver
> correctly.

Sounds reasonable.

> 
> So this patch add id_table to mdev_driver and id for mdev parent, and
> implement the match method for mdev bus.
> 
> [1] https://lkml.org/lkml/2019/9/10/135
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c  |  2 +-
>  drivers/s390/cio/vfio_ccw_ops.c   |  2 +-
>  drivers/s390/crypto/vfio_ap_ops.c |  3 ++-
>  drivers/vfio/mdev/mdev_core.c     | 14 ++++++++++++--
>  drivers/vfio/mdev/mdev_driver.c   | 14 ++++++++++++++
>  drivers/vfio/mdev/mdev_private.h  |  1 +
>  drivers/vfio/mdev/vfio_mdev.c     |  6 ++++++
>  include/linux/mdev.h              |  6 +++++-
>  include/linux/mod_devicetable.h   |  6 ++++++
>  samples/vfio-mdev/mbochs.c        |  2 +-
>  samples/vfio-mdev/mdpy.c          |  2 +-
>  samples/vfio-mdev/mtty.c          |  2 +-
>  12 files changed, 51 insertions(+), 9 deletions(-)

(...)

The transformations of the vendor drivers and the new interface look
sane.

(...)

> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5714fd35a83c..f1fc143df042 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -821,4 +821,10 @@ struct wmi_device_id {
>  	const void *context;
>  };
>  
> +/* MDEV */
> +

Maybe add some kerneldoc and give vfio as an example of what we're
matching here?

> +struct mdev_device_id {
> +	__u8 id;

I agree with the suggestion to rename this to 'class_id'.

> +};
> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */

Re: [RFC PATCH 2/2] mdev: introduce device specific ops

[Cornelia Huck]

On Thu, 12 Sep 2019 17:40:12 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Currently, except for the crate and remove. The rest fields of
> mdev_parent_ops is just designed for vfio-mdev driver and may not help
> for kernel mdev driver. So follow the device id support by previous
> patch, this patch introduces device specific ops which points to
> device specific ops (e.g vfio ops). This allows the future drivers
> like virtio-mdev to implement its own device specific ops.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c  | 14 +++---
>  drivers/s390/cio/vfio_ccw_ops.c   | 14 +++---
>  drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
>  drivers/vfio/mdev/vfio_mdev.c     | 30 +++++++------
>  include/linux/mdev.h              | 72 ++++++++++++++++++-------------
>  samples/vfio-mdev/mbochs.c        | 16 ++++---
>  samples/vfio-mdev/mdpy.c          | 16 ++++---
>  samples/vfio-mdev/mtty.c          | 14 +++---
>  8 files changed, 113 insertions(+), 73 deletions(-)

> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index f85045392120..3b8a76bc69cf 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev, struct device *iommu_device);
>  struct device *mdev_get_iommu_device(struct device *dev);
>  
>  /**
> - * struct mdev_parent_ops - Structure to be registered for each parent device to
> - * register the device to mdev module.
> + * struct vfio_mdev_parent_ops - Structure to be registered for each
> + * parent device to register the device to vfio-mdev module.
>   *
> - * @owner:		The module owner.
> - * @dev_attr_groups:	Attributes of the parent device.
> - * @mdev_attr_groups:	Attributes of the mediated device.
> - * @supported_type_groups: Attributes to define supported types. It is mandatory
> - *			to provide supported types.
> - * @create:		Called to allocate basic resources in parent device's
> - *			driver for a particular mediated device. It is
> - *			mandatory to provide create ops.
> - *			@kobj: kobject of type for which 'create' is called.
> - *			@mdev: mdev_device structure on of mediated device
> - *			      that is being created
> - *			Returns integer: success (0) or error (< 0)
> - * @remove:		Called to free resources in parent device's driver for a
> - *			a mediated device. It is mandatory to provide 'remove'
> - *			ops.
> - *			@mdev: mdev_device device structure which is being
> - *			       destroyed
> - *			Returns integer: success (0) or error (< 0)
>   * @open:		Open mediated device.
>   *			@mdev: mediated device.
>   *			Returns integer: success (0) or error (< 0)
> @@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct device *dev);
>   * @mmap:		mmap callback
>   *			@mdev: mediated device structure
>   *			@vma: vma structure
> + */
> +struct vfio_mdev_parent_ops {
> +	int     (*open)(struct mdev_device *mdev);
> +	void    (*release)(struct mdev_device *mdev);
> +	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> +			size_t count, loff_t *ppos);
> +	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> +			 size_t count, loff_t *ppos);
> +	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> +			 unsigned long arg);
> +	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> +};
> +
> +/**
> + * struct mdev_parent_ops - Structure to be registered for each parent device to
> + * register the device to mdev module.
> + *
> + * @owner:		The module owner.
> + * @dev_attr_groups:	Attributes of the parent device.
> + * @mdev_attr_groups:	Attributes of the mediated device.
> + * @supported_type_groups: Attributes to define supported types. It is mandatory
> + *			to provide supported types.
> + * @create:		Called to allocate basic resources in parent device's
> + *			driver for a particular mediated device. It is
> + *			mandatory to provide create ops.
> + *			@kobj: kobject of type for which 'create' is called.
> + *			@mdev: mdev_device structure on of mediated device
> + *			      that is being created
> + *			Returns integer: success (0) or error (< 0)
> + * @remove:		Called to free resources in parent device's driver for a
> + *			a mediated device. It is mandatory to provide 'remove'
> + *			ops.
> + *			@mdev: mdev_device device structure which is being
> + *			       destroyed
> + *			Returns integer: success (0) or error (< 0)
> + * @device_ops:         Device specific emulation callback.
> + *
>   * Parent device that support mediated device should be registered with mdev
>   * module with mdev_parent_ops structure.
>   **/
> @@ -83,15 +102,7 @@ struct mdev_parent_ops {
>  
>  	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
>  	int     (*remove)(struct mdev_device *mdev);
> -	int     (*open)(struct mdev_device *mdev);
> -	void    (*release)(struct mdev_device *mdev);
> -	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> -			size_t count, loff_t *ppos);
> -	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> -			 size_t count, loff_t *ppos);
> -	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> -			 unsigned long arg);
> -	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> +	const void *device_ops;
>  };
>  
>  /* interface for exporting mdev supported type attributes */

This basically looks like a split between stuff that is always
triggered from userspace (create and the like) and stuff that is
triggered from userspace for vfio mdevs, but not necessarily for other
mdevs. Seems reasonable at a glance.

If we decide to go forward with this, we should also update the
documentation (split out stuff from driver-api/vfio-mediated-device.rst
etc.)

Re: [RFC PATCH 0/2] Mdev: support mutiple kinds of devices

[Alex Williamson]

[cc +Parav]

On Thu, 12 Sep 2019 17:40:10 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Hi all:
> 
> During the development of virtio-mdev[1]. I find that mdev needs to be
> extended to support devices other than vfio mdev device. So this
> series tries to extend the mdev to be able to differ from different
> devices by:
> 
> - device id and matching for mdev bus
> - device speicfic callbacks and move vfio callbacks there
> 
> Sent for early reivew, compile test only!
> 
> Thanks
> 
> [1] https://lkml.org/lkml/2019/9/10/135

I expect Parav must have something similar in the works for their
in-kernel networking mdev support.  Link to discussion so far:

https://lore.kernel.org/kvm/20190912094012.29653-1-jasowang@redhat.com/T/#t

Thanks,
Alex


> Jason Wang (2):
>   mdev: device id support
>   mdev: introduce device specific ops
> 
>  drivers/gpu/drm/i915/gvt/kvmgt.c  | 16 ++++---
>  drivers/s390/cio/vfio_ccw_ops.c   | 16 ++++---
>  drivers/s390/crypto/vfio_ap_ops.c | 13 ++++--
>  drivers/vfio/mdev/mdev_core.c     | 14 +++++-
>  drivers/vfio/mdev/mdev_driver.c   | 14 ++++++
>  drivers/vfio/mdev/mdev_private.h  |  1 +
>  drivers/vfio/mdev/vfio_mdev.c     | 36 ++++++++++-----
>  include/linux/mdev.h              | 76 +++++++++++++++++++------------
>  include/linux/mod_devicetable.h   |  6 +++
>  samples/vfio-mdev/mbochs.c        | 18 +++++---
>  samples/vfio-mdev/mdpy.c          | 18 +++++---
>  samples/vfio-mdev/mtty.c          | 16 ++++---
>  12 files changed, 163 insertions(+), 81 deletions(-)
> 

RE: [RFC PATCH 2/2] mdev: introduce device specific ops

[Tian, Kevin]

> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Tuesday, September 17, 2019 6:17 PM
> 
> On 2019/9/17 下午4:09, Tian, Kevin wrote:
> >> From: Jason Wang
> >> Sent: Thursday, September 12, 2019 5:40 PM
> >>
> >> Currently, except for the crate and remove. The rest fields of
> >> mdev_parent_ops is just designed for vfio-mdev driver and may not
> help
> >> for kernel mdev driver. So follow the device id support by previous
> >> patch, this patch introduces device specific ops which points to
> >> device specific ops (e.g vfio ops). This allows the future drivers
> >> like virtio-mdev to implement its own device specific ops.
> > Can you give an example about what ops might be required to support
> > kernel mdev driver? I know you posted a link earlier, but putting a small
> > example here can save time and avoid inconsistent understanding. Then
> > it will help whether the proposed split makes sense or there is a
> > possibility of redefining the callbacks to meet the both requirements.
> > imo those callbacks fulfill some basic requirements when mediating
> > a device...
> 
> 
> I put it in the cover letter.
> 
> The link is https://lkml.org/lkml/2019/9/10/135 which abuses the current
> VFIO based mdev parent ops.
> 
> Thanks

So the main problem is the handling of userspace pointers vs.
kernel space pointers. You still implement read/write/ioctl
callbacks which is a subset of current parent_ops definition.
In that regard is it better to introduce some helper to handle
the pointer difference in mdev core, while still keeping the 
same set of parent ops (in whatever form suitable for both)? 

> 
> 
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>   drivers/gpu/drm/i915/gvt/kvmgt.c  | 14 +++---
> >>   drivers/s390/cio/vfio_ccw_ops.c   | 14 +++---
> >>   drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
> >>   drivers/vfio/mdev/vfio_mdev.c     | 30 +++++++------
> >>   include/linux/mdev.h              | 72 ++++++++++++++++++-------------
> >>   samples/vfio-mdev/mbochs.c        | 16 ++++---
> >>   samples/vfio-mdev/mdpy.c          | 16 ++++---
> >>   samples/vfio-mdev/mtty.c          | 14 +++---
> >>   8 files changed, 113 insertions(+), 73 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> index 19d51a35f019..64823998fd58 100644
> >> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> @@ -1600,20 +1600,22 @@ 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_parent_ops intel_vfio_vgpu_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 = {
> >> +	.mdev_attr_groups       = intel_vgpu_groups,
> >> +	.create			= intel_vgpu_create,
> >> +	.remove			= intel_vgpu_remove,
> >> +	.device_ops	        = &intel_vfio_vgpu_ops,
> >> +};
> >> +
> >>   static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
> >>   {
> >>   	struct attribute **kvm_type_attrs;
> >> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> >> b/drivers/s390/cio/vfio_ccw_ops.c
> >> index f87d9409e290..96e9f18100ae 100644
> >> --- a/drivers/s390/cio/vfio_ccw_ops.c
> >> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> >> @@ -564,11 +564,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct
> >> mdev_device *mdev,
> >>   	}
> >>   }
> >>
> >> -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> >> -	.owner			= THIS_MODULE,
> >> -	.supported_type_groups  = mdev_type_groups,
> >> -	.create			= vfio_ccw_mdev_create,
> >> -	.remove			= vfio_ccw_mdev_remove,
> >> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> >>   	.open			= vfio_ccw_mdev_open,
> >>   	.release		= vfio_ccw_mdev_release,
> >>   	.read			= vfio_ccw_mdev_read,
> >> @@ -576,6 +572,14 @@ static const struct mdev_parent_ops
> >> vfio_ccw_mdev_ops = {
> >>   	.ioctl			= vfio_ccw_mdev_ioctl,
> >>   };
> >>
> >> +static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> >> +	.owner			= THIS_MODULE,
> >> +	.supported_type_groups  = mdev_type_groups,
> >> +	.create			= vfio_ccw_mdev_create,
> >> +	.remove			= vfio_ccw_mdev_remove,
> >> +	.device_ops		= &vfio_mdev_ops,
> >> +};
> >> +
> >>   int vfio_ccw_mdev_reg(struct subchannel *sch)
> >>   {
> >>   	return mdev_register_vfio_device(&sch->dev,
> >> &vfio_ccw_mdev_ops);
> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> >> b/drivers/s390/crypto/vfio_ap_ops.c
> >> index eacbde3c7a97..a48282bff066 100644
> >> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >> @@ -1280,15 +1280,19 @@ static ssize_t vfio_ap_mdev_ioctl(struct
> >> mdev_device *mdev,
> >>   	return ret;
> >>   }
> >>
> >> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> >> +	.open			= vfio_ap_mdev_open,
> >> +	.release		= vfio_ap_mdev_release,
> >> +	.ioctl			= vfio_ap_mdev_ioctl,
> >> +};
> >> +
> >>   static const struct mdev_parent_ops vfio_ap_matrix_ops = {
> >>   	.owner			= THIS_MODULE,
> >>   	.supported_type_groups	= vfio_ap_mdev_type_groups,
> >>   	.mdev_attr_groups	= vfio_ap_mdev_attr_groups,
> >>   	.create			= vfio_ap_mdev_create,
> >>   	.remove			= vfio_ap_mdev_remove,
> >> -	.open			= vfio_ap_mdev_open,
> >> -	.release		= vfio_ap_mdev_release,
> >> -	.ioctl			= vfio_ap_mdev_ioctl,
> >> +	.device_ops		= &vfio_mdev_ops,
> >>   };
> >>
> >>   int vfio_ap_mdev_register(void)
> >> diff --git a/drivers/vfio/mdev/vfio_mdev.c
> >> b/drivers/vfio/mdev/vfio_mdev.c
> >> index 887c57f10880..1196fbb6c3d2 100644
> >> --- a/drivers/vfio/mdev/vfio_mdev.c
> >> +++ b/drivers/vfio/mdev/vfio_mdev.c
> >> @@ -25,15 +25,16 @@ static int vfio_mdev_open(void *device_data)
> >>   {
> >>   	struct mdev_device *mdev = device_data;
> >>   	struct mdev_parent *parent = mdev->parent;
> >> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >>> device_ops;
> >>   	int ret;
> >>
> >> -	if (unlikely(!parent->ops->open))
> >> +	if (unlikely(!ops->open))
> >>   		return -EINVAL;
> >>
> >>   	if (!try_module_get(THIS_MODULE))
> >>   		return -ENODEV;
> >>
> >> -	ret = parent->ops->open(mdev);
> >> +	ret = ops->open(mdev);
> >>   	if (ret)
> >>   		module_put(THIS_MODULE);
> >>
> >> @@ -44,9 +45,10 @@ static void vfio_mdev_release(void *device_data)
> >>   {
> >>   	struct mdev_device *mdev = device_data;
> >>   	struct mdev_parent *parent = mdev->parent;
> >> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >>> device_ops;
> >> -	if (likely(parent->ops->release))
> >> -		parent->ops->release(mdev);
> >> +	if (likely(ops->release))
> >> +		ops->release(mdev);
> >>
> >>   	module_put(THIS_MODULE);
> >>   }
> >> @@ -56,11 +58,12 @@ static long vfio_mdev_unlocked_ioctl(void
> >> *device_data,
> >>   {
> >>   	struct mdev_device *mdev = device_data;
> >>   	struct mdev_parent *parent = mdev->parent;
> >> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >>> device_ops;
> >> -	if (unlikely(!parent->ops->ioctl))
> >> +	if (unlikely(!ops->ioctl))
> >>   		return -EINVAL;
> >>
> >> -	return parent->ops->ioctl(mdev, cmd, arg);
> >> +	return ops->ioctl(mdev, cmd, arg);
> >>   }
> >>
> >>   static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> >> @@ -68,11 +71,12 @@ static ssize_t vfio_mdev_read(void *device_data,
> >> char __user *buf,
> >>   {
> >>   	struct mdev_device *mdev = device_data;
> >>   	struct mdev_parent *parent = mdev->parent;
> >> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >>> device_ops;
> >> -	if (unlikely(!parent->ops->read))
> >> +	if (unlikely(!ops->read))
> >>   		return -EINVAL;
> >>
> >> -	return parent->ops->read(mdev, buf, count, ppos);
> >> +	return ops->read(mdev, buf, count, ppos);
> >>   }
> >>
> >>   static ssize_t vfio_mdev_write(void *device_data, const char __user
> *buf,
> >> @@ -80,22 +84,24 @@ static ssize_t vfio_mdev_write(void *device_data,
> >> const char __user *buf,
> >>   {
> >>   	struct mdev_device *mdev = device_data;
> >>   	struct mdev_parent *parent = mdev->parent;
> >> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >>> device_ops;
> >> -	if (unlikely(!parent->ops->write))
> >> +	if (unlikely(!ops->write))
> >>   		return -EINVAL;
> >>
> >> -	return parent->ops->write(mdev, buf, count, ppos);
> >> +	return ops->write(mdev, buf, count, ppos);
> >>   }
> >>
> >>   static int vfio_mdev_mmap(void *device_data, struct vm_area_struct
> >> *vma)
> >>   {
> >>   	struct mdev_device *mdev = device_data;
> >>   	struct mdev_parent *parent = mdev->parent;
> >> +	const struct vfio_mdev_parent_ops *ops = parent->ops-
> >>> device_ops;
> >> -	if (unlikely(!parent->ops->mmap))
> >> +	if (unlikely(!ops->mmap))
> >>   		return -EINVAL;
> >>
> >> -	return parent->ops->mmap(mdev, vma);
> >> +	return ops->mmap(mdev, vma);
> >>   }
> >>
> >>   static const struct vfio_device_ops vfio_mdev_dev_ops = {
> >> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> >> index f85045392120..3b8a76bc69cf 100644
> >> --- a/include/linux/mdev.h
> >> +++ b/include/linux/mdev.h
> >> @@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev,
> >> struct device *iommu_device);
> >>   struct device *mdev_get_iommu_device(struct device *dev);
> >>
> >>   /**
> >> - * struct mdev_parent_ops - Structure to be registered for each parent
> >> device to
> >> - * register the device to mdev module.
> >> + * struct vfio_mdev_parent_ops - Structure to be registered for each
> >> + * parent device to register the device to vfio-mdev module.
> >>    *
> >> - * @owner:		The module owner.
> >> - * @dev_attr_groups:	Attributes of the parent device.
> >> - * @mdev_attr_groups:	Attributes of the mediated device.
> >> - * @supported_type_groups: Attributes to define supported types. It is
> >> mandatory
> >> - *			to provide supported types.
> >> - * @create:		Called to allocate basic resources in parent device's
> >> - *			driver for a particular mediated device. It is
> >> - *			mandatory to provide create ops.
> >> - *			@kobj: kobject of type for which 'create' is called.
> >> - *			@mdev: mdev_device structure on of mediated
> >> device
> >> - *			      that is being created
> >> - *			Returns integer: success (0) or error (< 0)
> >> - * @remove:		Called to free resources in parent device's
> driver for
> >> a
> >> - *			a mediated device. It is mandatory to provide
> >> 'remove'
> >> - *			ops.
> >> - *			@mdev: mdev_device device structure which is
> >> being
> >> - *			       destroyed
> >> - *			Returns integer: success (0) or error (< 0)
> >>    * @open:		Open mediated device.
> >>    *			@mdev: mediated device.
> >>    *			Returns integer: success (0) or error (< 0)
> >> @@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct
> >> device *dev);
> >>    * @mmap:		mmap callback
> >>    *			@mdev: mediated device structure
> >>    *			@vma: vma structure
> >> + */
> >> +struct vfio_mdev_parent_ops {
> >> +	int     (*open)(struct mdev_device *mdev);
> >> +	void    (*release)(struct mdev_device *mdev);
> >> +	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> >> +			size_t count, loff_t *ppos);
> >> +	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> >> +			 size_t count, loff_t *ppos);
> >> +	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> >> +			 unsigned long arg);
> >> +	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
> >> *vma);
> >> +};
> >> +
> >> +/**
> >> + * struct mdev_parent_ops - Structure to be registered for each parent
> >> device to
> >> + * register the device to mdev module.
> >> + *
> >> + * @owner:		The module owner.
> >> + * @dev_attr_groups:	Attributes of the parent device.
> >> + * @mdev_attr_groups:	Attributes of the mediated device.
> >> + * @supported_type_groups: Attributes to define supported types. It is
> >> mandatory
> >> + *			to provide supported types.
> >> + * @create:		Called to allocate basic resources in parent
> device's
> >> + *			driver for a particular mediated device. It is
> >> + *			mandatory to provide create ops.
> >> + *			@kobj: kobject of type for which 'create' is called.
> >> + *			@mdev: mdev_device structure on of mediated
> >> device
> >> + *			      that is being created
> >> + *			Returns integer: success (0) or error (< 0)
> >> + * @remove:		Called to free resources in parent device's
> driver for
> >> a
> >> + *			a mediated device. It is mandatory to provide
> >> 'remove'
> >> + *			ops.
> >> + *			@mdev: mdev_device device structure which is
> >> being
> >> + *			       destroyed
> >> + *			Returns integer: success (0) or error (< 0)
> >> + * @device_ops:         Device specific emulation callback.
> >> + *
> >>    * Parent device that support mediated device should be registered
> with
> >> mdev
> >>    * module with mdev_parent_ops structure.
> >>    **/
> >> @@ -83,15 +102,7 @@ struct mdev_parent_ops {
> >>
> >>   	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
> >>   	int     (*remove)(struct mdev_device *mdev);
> >> -	int     (*open)(struct mdev_device *mdev);
> >> -	void    (*release)(struct mdev_device *mdev);
> >> -	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> >> -			size_t count, loff_t *ppos);
> >> -	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> >> -			 size_t count, loff_t *ppos);
> >> -	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> >> -			 unsigned long arg);
> >> -	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
> >> *vma);
> >> +	const void *device_ops;
> >>   };
> >>
> >>   /* interface for exporting mdev supported type attributes */
> >> @@ -137,7 +148,8 @@ const guid_t *mdev_uuid(struct mdev_device
> >> *mdev);
> >>
> >>   extern struct bus_type mdev_bus_type;
> >>
> >> -int mdev_register_vfio_device(struct device *dev, const struct
> >> mdev_parent_ops *ops);
> >> +int mdev_register_vfio_device(struct device *dev,
> >> +                              const struct mdev_parent_ops *ops);
> >>   void mdev_unregister_device(struct device *dev);
> >>
> >>   int mdev_register_driver(struct mdev_driver *drv, struct module
> *owner);
> >> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-
> mdev/mbochs.c
> >> index 71a4469be85d..53ceb357f152 100644
> >> --- a/samples/vfio-mdev/mbochs.c
> >> +++ b/samples/vfio-mdev/mbochs.c
> >> @@ -1418,12 +1418,7 @@ static struct attribute_group
> >> *mdev_type_groups[] = {
> >>   	NULL,
> >>   };
> >>
> >> -static const struct mdev_parent_ops mdev_fops = {
> >> -	.owner			= THIS_MODULE,
> >> -	.mdev_attr_groups	= mdev_dev_groups,
> >> -	.supported_type_groups	= mdev_type_groups,
> >> -	.create			= mbochs_create,
> >> -	.remove			= mbochs_remove,
> >> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> >>   	.open			= mbochs_open,
> >>   	.release		= mbochs_close,
> >>   	.read			= mbochs_read,
> >> @@ -1432,6 +1427,15 @@ static const struct mdev_parent_ops
> mdev_fops
> >> = {
> >>   	.mmap			= mbochs_mmap,
> >>   };
> >>
> >> +static const struct mdev_parent_ops mdev_fops = {
> >> +	.owner			= THIS_MODULE,
> >> +	.mdev_attr_groups	= mdev_dev_groups,
> >> +	.supported_type_groups	= mdev_type_groups,
> >> +	.create			= mbochs_create,
> >> +	.remove			= mbochs_remove,
> >> +	.device_ops		= &vfio_mdev_ops,
> >> +};
> >> +
> >>   static const struct file_operations vd_fops = {
> >>   	.owner		= THIS_MODULE,
> >>   };
> >> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> >> index d3029dd27d91..4ba285a5768f 100644
> >> --- a/samples/vfio-mdev/mdpy.c
> >> +++ b/samples/vfio-mdev/mdpy.c
> >> @@ -725,12 +725,7 @@ static struct attribute_group
> *mdev_type_groups[]
> >> = {
> >>   	NULL,
> >>   };
> >>
> >> -static const struct mdev_parent_ops mdev_fops = {
> >> -	.owner			= THIS_MODULE,
> >> -	.mdev_attr_groups	= mdev_dev_groups,
> >> -	.supported_type_groups	= mdev_type_groups,
> >> -	.create			= mdpy_create,
> >> -	.remove			= mdpy_remove,
> >> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> >>   	.open			= mdpy_open,
> >>   	.release		= mdpy_close,
> >>   	.read			= mdpy_read,
> >> @@ -739,6 +734,15 @@ static const struct mdev_parent_ops
> mdev_fops =
> >> {
> >>   	.mmap			= mdpy_mmap,
> >>   };
> >>
> >> +static const struct mdev_parent_ops mdev_fops = {
> >> +	.owner			= THIS_MODULE,
> >> +	.mdev_attr_groups	= mdev_dev_groups,
> >> +	.supported_type_groups	= mdev_type_groups,
> >> +	.create			= mdpy_create,
> >> +	.remove			= mdpy_remove,
> >> +	.device_ops		= &vfio_mdev_ops,
> >> +};
> >> +
> >>   static const struct file_operations vd_fops = {
> >>   	.owner		= THIS_MODULE,
> >>   };
> >> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> >> index 744c88a6b22c..a468904ec626 100644
> >> --- a/samples/vfio-mdev/mtty.c
> >> +++ b/samples/vfio-mdev/mtty.c
> >> @@ -1410,6 +1410,14 @@ static struct attribute_group
> >> *mdev_type_groups[] = {
> >>   	NULL,
> >>   };
> >>
> >> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> >> +	.open                   = mtty_open,
> >> +	.release                = mtty_close,
> >> +	.read                   = mtty_read,
> >> +	.write                  = mtty_write,
> >> +	.ioctl		        = mtty_ioctl,
> >> +};
> >> +
> >>   static const struct mdev_parent_ops mdev_fops = {
> >>   	.owner                  = THIS_MODULE,
> >>   	.dev_attr_groups        = mtty_dev_groups,
> >> @@ -1417,11 +1425,7 @@ static const struct mdev_parent_ops
> mdev_fops
> >> = {
> >>   	.supported_type_groups  = mdev_type_groups,
> >>   	.create                 = mtty_create,
> >>   	.remove			= mtty_remove,
> >> -	.open                   = mtty_open,
> >> -	.release                = mtty_close,
> >> -	.read                   = mtty_read,
> >> -	.write                  = mtty_write,
> >> -	.ioctl		        = mtty_ioctl,
> >> +	.device_ops             = &vfio_mdev_ops,
> >>   };
> >>
> >>   static void mtty_device_release(struct device *dev)
> >> --
> >> 2.19.1
> >>
> >> _______________________________________________
> >> Virtualization mailing list
> >> Virtualization@lists.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH 1/2] mdev: device id support

[Jason Wang]

On 2019/9/17 下午8:07, Cornelia Huck wrote:
> On Thu, 12 Sep 2019 17:40:11 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Mdev bus only support vfio driver right now, so it doesn't implement
>> match method. But in the future, we may add drivers other than vfio,
>> one example is virtio-mdev[1] driver. This means we need to add device
>> id support in bus match method to pair the mdev device and mdev driver
>> correctly.
> Sounds reasonable.
>
>> So this patch add id_table to mdev_driver and id for mdev parent, and
>> implement the match method for mdev bus.
>>
>> [1] https://lkml.org/lkml/2019/9/10/135
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/kvmgt.c  |  2 +-
>>   drivers/s390/cio/vfio_ccw_ops.c   |  2 +-
>>   drivers/s390/crypto/vfio_ap_ops.c |  3 ++-
>>   drivers/vfio/mdev/mdev_core.c     | 14 ++++++++++++--
>>   drivers/vfio/mdev/mdev_driver.c   | 14 ++++++++++++++
>>   drivers/vfio/mdev/mdev_private.h  |  1 +
>>   drivers/vfio/mdev/vfio_mdev.c     |  6 ++++++
>>   include/linux/mdev.h              |  6 +++++-
>>   include/linux/mod_devicetable.h   |  6 ++++++
>>   samples/vfio-mdev/mbochs.c        |  2 +-
>>   samples/vfio-mdev/mdpy.c          |  2 +-
>>   samples/vfio-mdev/mtty.c          |  2 +-
>>   12 files changed, 51 insertions(+), 9 deletions(-)
> (...)
>
> The transformations of the vendor drivers and the new interface look
> sane.
>
> (...)
>
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index 5714fd35a83c..f1fc143df042 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -821,4 +821,10 @@ struct wmi_device_id {
>>   	const void *context;
>>   };
>>   
>> +/* MDEV */
>> +
> Maybe add some kerneldoc and give vfio as an example of what we're
> matching here?


Will add when posting a non RFC patch.


>
>> +struct mdev_device_id {
>> +	__u8 id;
> I agree with the suggestion to rename this to 'class_id'.
>

Let me change it.

Thanks


>> +};
>> +
>>   #endif /* LINUX_MOD_DEVICETABLE_H */

Re: [RFC PATCH 2/2] mdev: introduce device specific ops

[Jason Wang]

On 2019/9/17 下午8:42, Cornelia Huck wrote:
> On Thu, 12 Sep 2019 17:40:12 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Currently, except for the crate and remove. The rest fields of
>> mdev_parent_ops is just designed for vfio-mdev driver and may not help
>> for kernel mdev driver. So follow the device id support by previous
>> patch, this patch introduces device specific ops which points to
>> device specific ops (e.g vfio ops). This allows the future drivers
>> like virtio-mdev to implement its own device specific ops.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/kvmgt.c  | 14 +++---
>>   drivers/s390/cio/vfio_ccw_ops.c   | 14 +++---
>>   drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
>>   drivers/vfio/mdev/vfio_mdev.c     | 30 +++++++------
>>   include/linux/mdev.h              | 72 ++++++++++++++++++-------------
>>   samples/vfio-mdev/mbochs.c        | 16 ++++---
>>   samples/vfio-mdev/mdpy.c          | 16 ++++---
>>   samples/vfio-mdev/mtty.c          | 14 +++---
>>   8 files changed, 113 insertions(+), 73 deletions(-)
>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
>> index f85045392120..3b8a76bc69cf 100644
>> --- a/include/linux/mdev.h
>> +++ b/include/linux/mdev.h
>> @@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev, struct device *iommu_device);
>>   struct device *mdev_get_iommu_device(struct device *dev);
>>   
>>   /**
>> - * struct mdev_parent_ops - Structure to be registered for each parent device to
>> - * register the device to mdev module.
>> + * struct vfio_mdev_parent_ops - Structure to be registered for each
>> + * parent device to register the device to vfio-mdev module.
>>    *
>> - * @owner:		The module owner.
>> - * @dev_attr_groups:	Attributes of the parent device.
>> - * @mdev_attr_groups:	Attributes of the mediated device.
>> - * @supported_type_groups: Attributes to define supported types. It is mandatory
>> - *			to provide supported types.
>> - * @create:		Called to allocate basic resources in parent device's
>> - *			driver for a particular mediated device. It is
>> - *			mandatory to provide create ops.
>> - *			@kobj: kobject of type for which 'create' is called.
>> - *			@mdev: mdev_device structure on of mediated device
>> - *			      that is being created
>> - *			Returns integer: success (0) or error (< 0)
>> - * @remove:		Called to free resources in parent device's driver for a
>> - *			a mediated device. It is mandatory to provide 'remove'
>> - *			ops.
>> - *			@mdev: mdev_device device structure which is being
>> - *			       destroyed
>> - *			Returns integer: success (0) or error (< 0)
>>    * @open:		Open mediated device.
>>    *			@mdev: mediated device.
>>    *			Returns integer: success (0) or error (< 0)
>> @@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct device *dev);
>>    * @mmap:		mmap callback
>>    *			@mdev: mediated device structure
>>    *			@vma: vma structure
>> + */
>> +struct vfio_mdev_parent_ops {
>> +	int     (*open)(struct mdev_device *mdev);
>> +	void    (*release)(struct mdev_device *mdev);
>> +	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
>> +			size_t count, loff_t *ppos);
>> +	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
>> +			 size_t count, loff_t *ppos);
>> +	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
>> +			 unsigned long arg);
>> +	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
>> +};
>> +
>> +/**
>> + * struct mdev_parent_ops - Structure to be registered for each parent device to
>> + * register the device to mdev module.
>> + *
>> + * @owner:		The module owner.
>> + * @dev_attr_groups:	Attributes of the parent device.
>> + * @mdev_attr_groups:	Attributes of the mediated device.
>> + * @supported_type_groups: Attributes to define supported types. It is mandatory
>> + *			to provide supported types.
>> + * @create:		Called to allocate basic resources in parent device's
>> + *			driver for a particular mediated device. It is
>> + *			mandatory to provide create ops.
>> + *			@kobj: kobject of type for which 'create' is called.
>> + *			@mdev: mdev_device structure on of mediated device
>> + *			      that is being created
>> + *			Returns integer: success (0) or error (< 0)
>> + * @remove:		Called to free resources in parent device's driver for a
>> + *			a mediated device. It is mandatory to provide 'remove'
>> + *			ops.
>> + *			@mdev: mdev_device device structure which is being
>> + *			       destroyed
>> + *			Returns integer: success (0) or error (< 0)
>> + * @device_ops:         Device specific emulation callback.
>> + *
>>    * Parent device that support mediated device should be registered with mdev
>>    * module with mdev_parent_ops structure.
>>    **/
>> @@ -83,15 +102,7 @@ struct mdev_parent_ops {
>>   
>>   	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
>>   	int     (*remove)(struct mdev_device *mdev);
>> -	int     (*open)(struct mdev_device *mdev);
>> -	void    (*release)(struct mdev_device *mdev);
>> -	ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
>> -			size_t count, loff_t *ppos);
>> -	ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
>> -			 size_t count, loff_t *ppos);
>> -	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
>> -			 unsigned long arg);
>> -	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
>> +	const void *device_ops;
>>   };
>>   
>>   /* interface for exporting mdev supported type attributes */
> This basically looks like a split between stuff that is always
> triggered from userspace (create and the like) and stuff that is
> triggered from userspace for vfio mdevs, but not necessarily for other
> mdevs.


Yes.


>   Seems reasonable at a glance.
>
> If we decide to go forward with this, we should also update the
> documentation (split out stuff from driver-api/vfio-mediated-device.rst
> etc.)


Yes, I plan to do that.

Thanks

Re: [RFC PATCH 2/2] mdev: introduce device specific ops

[Jason Wang]

On 2019/9/18 上午10:57, Tian, Kevin wrote:
>> From: Jason Wang [mailto:jasowang@redhat.com]
>> Sent: Tuesday, September 17, 2019 6:17 PM
>>
>> On 2019/9/17 下午4:09, Tian, Kevin wrote:
>>>> From: Jason Wang
>>>> Sent: Thursday, September 12, 2019 5:40 PM
>>>>
>>>> Currently, except for the crate and remove. The rest fields of
>>>> mdev_parent_ops is just designed for vfio-mdev driver and may not
>> help
>>>> for kernel mdev driver. So follow the device id support by previous
>>>> patch, this patch introduces device specific ops which points to
>>>> device specific ops (e.g vfio ops). This allows the future drivers
>>>> like virtio-mdev to implement its own device specific ops.
>>> Can you give an example about what ops might be required to support
>>> kernel mdev driver? I know you posted a link earlier, but putting a small
>>> example here can save time and avoid inconsistent understanding. Then
>>> it will help whether the proposed split makes sense or there is a
>>> possibility of redefining the callbacks to meet the both requirements.
>>> imo those callbacks fulfill some basic requirements when mediating
>>> a device...
>> I put it in the cover letter.
>>
>> The link ishttps://lkml.org/lkml/2019/9/10/135  which abuses the current
>> VFIO based mdev parent ops.
>>
>> Thanks
> So the main problem is the handling of userspace pointers vs.
> kernel space pointers. You still implement read/write/ioctl
> callbacks which is a subset of current parent_ops definition.
> In that regard is it better to introduce some helper to handle
> the pointer difference in mdev core, while still keeping the
> same set of parent ops (in whatever form suitable for both)?


Pointers is one of the issues. And read/write/ioctl is designed for 
userspace API not kernel. Technically, we can use them for kernel but it 
would not be as simple and straightforward a set of device specific 
callbacks functions. The link above is just an example, e.g we can 
simply pass the vring address through a dedicated API instead of 
mandatory an offset of a file.

Thanks

>