[PATCH V3 1/7] mdev: class id support

[PATCH V3 1/7] mdev: class id support

[Jason Wang]

Mdev bus only supports vfio driver right now, so it doesn't implement
match method. But in the future, we may add drivers other than vfio,
the first driver could be virtio-mdev. This means we need to add
device class id support in bus match method to pair the mdev device
and mdev driver correctly.

So this patch adds id_table to mdev_driver and class_id for mdev
device with the match method for mdev bus.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 Documentation/driver-api/vfio-mediated-device.rst |  7 ++++++-
 drivers/gpu/drm/i915/gvt/kvmgt.c                  |  1 +
 drivers/s390/cio/vfio_ccw_ops.c                   |  1 +
 drivers/s390/crypto/vfio_ap_ops.c                 |  1 +
 drivers/vfio/mdev/mdev_core.c                     | 11 +++++++++++
 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                              |  8 ++++++++
 include/linux/mod_devicetable.h                   |  8 ++++++++
 samples/vfio-mdev/mbochs.c                        |  1 +
 samples/vfio-mdev/mdpy.c                          |  1 +
 samples/vfio-mdev/mtty.c                          |  1 +
 13 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index 25eb7d5b834b..2035e48da7b2 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -102,12 +102,14 @@ structure to represent a mediated device's driver::
       * @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 {
 	     const char *name;
 	     int  (*probe)  (struct device *dev);
 	     void (*remove) (struct device *dev);
 	     struct device_driver    driver;
+	     const struct mdev_class_id *id_table;
      };
 
 A mediated bus driver for mdev should use this structure in the function calls
@@ -165,12 +167,15 @@ register itself with the mdev core driver::
 	extern int  mdev_register_device(struct device *dev,
 	                                 const struct mdev_parent_ops *ops);
 
+It is also required to specify the class_id through::
+
+	extern int mdev_set_class(struct device *dev, u16 id);
+
 However, the mdev_parent_ops structure is not required in the function call
 that a driver should use to unregister itself with the mdev core driver::
 
 	extern void mdev_unregister_device(struct device *dev);
 
-
 Mediated Device Management Interface Through sysfs
 ==================================================
 
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 343d79c1cb7e..17e9d4634c84 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -678,6 +678,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 		     dev_name(mdev_dev(mdev)));
 	ret = 0;
 
+	mdev_set_class(mdev, MDEV_ID_VFIO);
 out:
 	return ret;
 }
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f0d71ab77c50..b5d223882c6c 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -129,6 +129,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
+	mdev_set_class(mdev, MDEV_ID_VFIO);
 	return 0;
 }
 
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 5c0f53c6dde7..47df1c593c35 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -343,6 +343,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
 	mutex_unlock(&matrix_dev->lock);
 
+	mdev_set_class(mdev, MDEV_ID_VFIO);
 	return 0;
 }
 
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..724e9b9841d8 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -45,6 +45,12 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data)
 }
 EXPORT_SYMBOL(mdev_set_drvdata);
 
+void mdev_set_class(struct mdev_device *mdev, u16 id)
+{
+	mdev->class_id = id;
+}
+EXPORT_SYMBOL(mdev_set_class);
+
 struct device *mdev_dev(struct mdev_device *mdev)
 {
 	return &mdev->dev;
@@ -135,6 +141,7 @@ 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: class id.
  *
  * Add device to list of registered parent devices.
  * Returns a negative value on error, otherwise 0.
@@ -324,6 +331,9 @@ int mdev_device_create(struct kobject *kobj,
 	if (ret)
 		goto ops_create_fail;
 
+	if (!mdev->class_id)
+		goto class_id_fail;
+
 	ret = device_add(&mdev->dev);
 	if (ret)
 		goto add_fail;
@@ -340,6 +350,7 @@ int mdev_device_create(struct kobject *kobj,
 
 sysfs_fail:
 	device_del(&mdev->dev);
+class_id_fail:
 add_fail:
 	parent->ops->remove(mdev);
 ops_create_fail:
diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
index 0d3223aee20b..b7c40ce86ee3 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_class_id *ids = mdrv->id_table;
+
+	for (i = 0; ids[i].id; i++)
+		if (ids[i].id == mdev->class_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..c65f436c1869 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -33,6 +33,7 @@ struct mdev_device {
 	struct kobject *type_kobj;
 	struct device *iommu_device;
 	bool active;
+	u16 class_id;
 };
 
 #define to_mdev_device(dev)	container_of(dev, struct mdev_device, dev)
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 30964a4e0a28..fd2a4d9a3f26 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_class_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..a7570cf13ba4 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,12 +126,14 @@ struct mdev_driver {
 	int  (*probe)(struct device *dev);
 	void (*remove)(struct device *dev);
 	struct device_driver driver;
+	const struct mdev_class_id *id_table;
 };
 
 #define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
 
 void *mdev_get_drvdata(struct mdev_device *mdev);
 void mdev_set_drvdata(struct mdev_device *mdev, void *data);
+void mdev_set_class(struct mdev_device *mdev, u16 id);
 const guid_t *mdev_uuid(struct mdev_device *mdev);
 
 extern struct bus_type mdev_bus_type;
@@ -145,4 +148,9 @@ 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);
 
+enum {
+	MDEV_ID_VFIO = 1,
+	/* New entries must be added here */
+};
+
 #endif /* MDEV_H */
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5714fd35a83c..f32c6e44fb1a 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -821,4 +821,12 @@ struct wmi_device_id {
 	const void *context;
 };
 
+/**
+ * struct mdev_class_id - MDEV device class identifier
+ * @id: Used to identify a specific class of device, e.g vfio-mdev device.
+ */
+struct mdev_class_id {
+	__u16 id;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index ac5c8c17b1ff..fd8491e58fe2 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -561,6 +561,7 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev)
 	mbochs_reset(mdev);
 
 	mbochs_used_mbytes += type->mbytes;
+	mdev_set_class(mdev, MDEV_ID_VFIO);
 	return 0;
 
 err_mem:
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index cc86bf6566e4..889472b06708 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -269,6 +269,7 @@ static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev)
 	mdpy_reset(mdev);
 
 	mdpy_count++;
+	mdev_set_class(mdev, MDEV_ID_VFIO);
 	return 0;
 }
 
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index ce84a300a4da..618810ca4331 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -755,6 +755,7 @@ static int mtty_create(struct kobject *kobj, struct mdev_device *mdev)
 	list_add(&mdev_state->next, &mdev_devices_list);
 	mutex_unlock(&mdev_list_lock);
 
+	mdev_set_class(mdev, MDEV_ID_VFIO);
 	return 0;
 }
 
-- 
2.19.1

[PATCH V3 2/7] mdev: bus uevent support

[Jason Wang]

This patch adds bus uevent support for mdev bus in order to allow
cooperation with userspace.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vfio/mdev/mdev_driver.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
index b7c40ce86ee3..319d886ffaf7 100644
--- a/drivers/vfio/mdev/mdev_driver.c
+++ b/drivers/vfio/mdev/mdev_driver.c
@@ -82,9 +82,17 @@ static int mdev_match(struct device *dev, struct device_driver *drv)
 	return 0;
 }
 
+static int mdev_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct mdev_device *mdev = to_mdev_device(dev);
+
+	return add_uevent_var(env, "MODALIAS=mdev:c%02X", mdev->class_id);
+}
+
 struct bus_type mdev_bus_type = {
 	.name		= "mdev",
 	.match		= mdev_match,
+	.uevent		= mdev_uevent,
 	.probe		= mdev_probe,
 	.remove		= mdev_remove,
 };
-- 
2.19.1

[PATCH V3 3/7] modpost: add support for mdev class id

[Jason Wang]

Add support to parse mdev class id table.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vfio/mdev/vfio_mdev.c     |  2 ++
 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 10 ++++++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index fd2a4d9a3f26..891cf83a2d9a 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -125,6 +125,8 @@ static struct mdev_class_id id_table[] = {
 	{ 0 },
 };
 
+MODULE_DEVICE_TABLE(mdev, id_table);
+
 static struct mdev_driver vfio_mdev_driver = {
 	.name	= "vfio_mdev",
 	.probe	= vfio_mdev_probe,
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 054405b90ba4..6cbb1062488a 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -231,5 +231,8 @@ int main(void)
 	DEVID(wmi_device_id);
 	DEVID_FIELD(wmi_device_id, guid_string);
 
+	DEVID(mdev_class_id);
+	DEVID_FIELD(mdev_class_id, id);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index c91eba751804..d365dfe7c718 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1335,6 +1335,15 @@ static int do_wmi_entry(const char *filename, void *symval, char *alias)
 	return 1;
 }
 
+/* looks like: "mdev:cN" */
+static int do_mdev_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD(symval, mdev_class_id, id);
+
+	sprintf(alias, "mdev:c%02X", id);
+	return 1;
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1407,6 +1416,7 @@ static const struct devtable devtable[] = {
 	{"typec", SIZE_typec_device_id, do_typec_entry},
 	{"tee", SIZE_tee_client_device_id, do_tee_entry},
 	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
+	{"mdev", SIZE_mdev_class_id, do_mdev_entry},
 };
 
 /* Create MODULE_ALIAS() statements.
-- 
2.19.1

[PATCH V3 4/7] mdev: introduce device specific ops

[Jason Wang]

Currently, except for the create and remove, the rest of
mdev_parent_ops is designed for vfio-mdev driver only and may not help
for kernel mdev driver. With the help of class id, this patch
introduces device specific callbacks inside mdev_device
structure. This allows different set of callback to be used by
vfio-mdev and virtio-mdev.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 .../driver-api/vfio-mediated-device.rst       | 22 +++++---
 MAINTAINERS                                   |  1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c              | 18 ++++---
 drivers/s390/cio/vfio_ccw_ops.c               | 18 ++++---
 drivers/s390/crypto/vfio_ap_ops.c             | 14 +++--
 drivers/vfio/mdev/mdev_core.c                 |  9 +++-
 drivers/vfio/mdev/mdev_private.h              |  1 +
 drivers/vfio/mdev/vfio_mdev.c                 | 37 ++++++-------
 include/linux/mdev.h                          | 42 +++------------
 include/linux/vfio_mdev.h                     | 52 +++++++++++++++++++
 samples/vfio-mdev/mbochs.c                    | 20 ++++---
 samples/vfio-mdev/mdpy.c                      | 21 +++++---
 samples/vfio-mdev/mtty.c                      | 18 ++++---
 13 files changed, 177 insertions(+), 96 deletions(-)
 create mode 100644 include/linux/vfio_mdev.h

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index 2035e48da7b2..553574ebba73 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any other categorization.
 Vendor drivers are expected to be fully asynchronous in this respect or
 provide their own internal resource protection.)
 
-The callbacks in the mdev_parent_ops structure are as follows:
+In order to support multiple types of device/driver, device needs to
+provide both class_id and device_ops through:
 
-* open: open callback of mediated device
-* close: close callback of mediated device
-* ioctl: ioctl callback of mediated device
+    void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops);
+
+The class_id is used to be paired with ids in id_table in mdev_driver
+structure for probing the correct driver. The device_ops is device
+specific callbacks which can be get through mdev_get_dev_ops()
+function by mdev bus driver. For vfio-mdev device, its device specific
+ops are as follows:
+
+* open: open callback of vfio mediated device
+* close: close callback of vfio mediated device
+* ioctl: ioctl callback of vfio mediated device
 * read : read emulation callback
 * write: write emulation callback
 * mmap: mmap emulation callback
@@ -167,9 +176,10 @@ register itself with the mdev core driver::
 	extern int  mdev_register_device(struct device *dev,
 	                                 const struct mdev_parent_ops *ops);
 
-It is also required to specify the class_id through::
+It is also required to specify the class_id and device specific ops through::
 
-	extern int mdev_set_class(struct device *dev, u16 id);
+	extern int mdev_set_class(struct device *dev, u16 id,
+	                          const void *ops);
 
 However, the mdev_parent_ops structure is not required in the function call
 that a driver should use to unregister itself with the mdev core driver::
diff --git a/MAINTAINERS b/MAINTAINERS
index 8824f61cd2c0..3d196a023b5e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17127,6 +17127,7 @@ S:	Maintained
 F:	Documentation/driver-api/vfio-mediated-device.rst
 F:	drivers/vfio/mdev/
 F:	include/linux/mdev.h
+F:	include/linux/vfio_mdev.h
 F:	samples/vfio-mdev/
 
 VFIO PLATFORM DRIVER
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 17e9d4634c84..7e2b720074fd 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -42,6 +42,7 @@
 #include <linux/kvm_host.h>
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <linux/vfio_mdev.h>
 #include <linux/debugfs.h>
 
 #include <linux/nospec.h>
@@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu)
 	vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
 }
 
+static const struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops;
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -678,7 +681,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 		     dev_name(mdev_dev(mdev)));
 	ret = 0;
 
-	mdev_set_class(mdev, MDEV_ID_VFIO);
+	mdev_set_class(mdev, MDEV_ID_VFIO, &intel_vfio_vgpu_dev_ops);
 out:
 	return ret;
 }
@@ -1599,20 +1602,21 @@ static const struct attribute_group *intel_vgpu_groups[] = {
 	NULL,
 };
 
-static struct mdev_parent_ops intel_vgpu_ops = {
-	.mdev_attr_groups       = intel_vgpu_groups,
-	.create			= intel_vgpu_create,
-	.remove			= intel_vgpu_remove,
-
+static const struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = {
 	.open			= intel_vgpu_open,
 	.release		= intel_vgpu_release,
-
 	.read			= intel_vgpu_read,
 	.write			= intel_vgpu_write,
 	.mmap			= intel_vgpu_mmap,
 	.ioctl			= intel_vgpu_ioctl,
 };
 
+static struct mdev_parent_ops intel_vgpu_ops = {
+	.mdev_attr_groups       = intel_vgpu_groups,
+	.create			= intel_vgpu_create,
+	.remove			= intel_vgpu_remove,
+};
+
 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 b5d223882c6c..322991f0feb0 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -12,6 +12,7 @@
 
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <linux/vfio_mdev.h>
 #include <linux/nospec.h>
 #include <linux/slab.h>
 
@@ -110,6 +111,8 @@ static struct attribute_group *mdev_type_groups[] = {
 	NULL,
 };
 
+static const struct vfio_mdev_device_ops vfio_mdev_ops;
+
 static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private =
@@ -129,7 +132,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
-	mdev_set_class(mdev, MDEV_ID_VFIO);
+	mdev_set_class(mdev, MDEV_ID_VFIO, &vfio_mdev_ops);
 	return 0;
 }
 
@@ -575,11 +578,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_device_ops vfio_mdev_ops = {
 	.open			= vfio_ccw_mdev_open,
 	.release		= vfio_ccw_mdev_release,
 	.read			= vfio_ccw_mdev_read,
@@ -587,6 +586,13 @@ 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,
+};
+
 int vfio_ccw_mdev_reg(struct subchannel *sch)
 {
 	return mdev_register_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 47df1c593c35..e9bd5f3b1cc2 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -16,6 +16,7 @@
 #include <linux/bitops.h>
 #include <linux/kvm_host.h>
 #include <linux/module.h>
+#include <linux/vfio_mdev.h>
 #include <asm/kvm.h>
 #include <asm/zcrypt.h>
 
@@ -321,6 +322,8 @@ static void vfio_ap_matrix_init(struct ap_config_info *info,
 	matrix->adm_max = info->apxa ? info->Nd : 15;
 }
 
+static const struct vfio_mdev_device_ops vfio_mdev_ops;
+
 static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct ap_matrix_mdev *matrix_mdev;
@@ -343,7 +346,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
 	mutex_unlock(&matrix_dev->lock);
 
-	mdev_set_class(mdev, MDEV_ID_VFIO);
+	mdev_set_class(mdev, MDEV_ID_VFIO, &vfio_mdev_ops);
 	return 0;
 }
 
@@ -1281,15 +1284,18 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
 	return ret;
 }
 
+static const struct vfio_mdev_device_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,
 };
 
 int vfio_ap_mdev_register(void)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 724e9b9841d8..c79d0e16522b 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -45,12 +45,19 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data)
 }
 EXPORT_SYMBOL(mdev_set_drvdata);
 
-void mdev_set_class(struct mdev_device *mdev, u16 id)
+void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops)
 {
 	mdev->class_id = id;
+	mdev->device_ops = ops;
 }
 EXPORT_SYMBOL(mdev_set_class);
 
+const void *mdev_get_dev_ops(struct mdev_device *mdev)
+{
+	return mdev->device_ops;
+}
+EXPORT_SYMBOL(mdev_get_dev_ops);
+
 struct device *mdev_dev(struct mdev_device *mdev)
 {
 	return &mdev->dev;
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index c65f436c1869..b666805f0b1f 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -34,6 +34,7 @@ struct mdev_device {
 	struct device *iommu_device;
 	bool active;
 	u16 class_id;
+	const void *device_ops;
 };
 
 #define to_mdev_device(dev)	container_of(dev, struct mdev_device, dev)
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 891cf83a2d9a..95efa054442f 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <linux/vfio_mdev.h>
 
 #include "mdev_private.h"
 
@@ -24,16 +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_device_ops *ops = mdev_get_dev_ops(mdev);
 	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);
 
@@ -43,10 +44,10 @@ static int vfio_mdev_open(void *device_data)
 static void vfio_mdev_release(void *device_data)
 {
 	struct mdev_device *mdev = device_data;
-	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
 
-	if (likely(parent->ops->release))
-		parent->ops->release(mdev);
+	if (likely(ops->release))
+		ops->release(mdev);
 
 	module_put(THIS_MODULE);
 }
@@ -55,47 +56,47 @@ static long vfio_mdev_unlocked_ioctl(void *device_data,
 				     unsigned int cmd, unsigned long arg)
 {
 	struct mdev_device *mdev = device_data;
-	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
 
-	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,
 			      size_t count, loff_t *ppos)
 {
 	struct mdev_device *mdev = device_data;
-	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
 
-	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,
 			       size_t count, loff_t *ppos)
 {
 	struct mdev_device *mdev = device_data;
-	struct mdev_parent *parent = mdev->parent;
+	const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
 
-	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_device_ops *ops = mdev_get_dev_ops(mdev);
 
-	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 a7570cf13ba4..f491308674e5 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -10,6 +10,11 @@
 #ifndef MDEV_H
 #define MDEV_H
 
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/mdev.h>
+#include <uapi/linux/uuid.h>
+
 struct mdev_device;
 
 /*
@@ -48,30 +53,7 @@ struct device *mdev_get_iommu_device(struct device *dev);
  *			@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)
- * @release:		release mediated device
- *			@mdev: mediated device.
- * @read:		Read emulation callback
- *			@mdev: mediated device structure
- *			@buf: read buffer
- *			@count: number of bytes to read
- *			@ppos: address.
- *			Retuns number on bytes read on success or error.
- * @write:		Write emulation callback
- *			@mdev: mediated device structure
- *			@buf: write buffer
- *			@count: number of bytes to be written
- *			@ppos: address.
- *			Retuns number on bytes written on success or error.
- * @ioctl:		IOCTL callback
- *			@mdev: mediated device structure
- *			@cmd: ioctl command
- *			@arg: arguments to ioctl
- * @mmap:		mmap callback
- *			@mdev: mediated device structure
- *			@vma: vma structure
+ *
  * Parent device that support mediated device should be registered with mdev
  * module with mdev_parent_ops structure.
  **/
@@ -83,15 +65,6 @@ 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);
 };
 
 /* interface for exporting mdev supported type attributes */
@@ -133,7 +106,8 @@ struct mdev_driver {
 
 void *mdev_get_drvdata(struct mdev_device *mdev);
 void mdev_set_drvdata(struct mdev_device *mdev, void *data);
-void mdev_set_class(struct mdev_device *mdev, u16 id);
+void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops);
+const void *mdev_get_dev_ops(struct mdev_device *mdev);
 const guid_t *mdev_uuid(struct mdev_device *mdev);
 
 extern struct bus_type mdev_bus_type;
diff --git a/include/linux/vfio_mdev.h b/include/linux/vfio_mdev.h
new file mode 100644
index 000000000000..3907c5371c2b
--- /dev/null
+++ b/include/linux/vfio_mdev.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * VFIO Mediated device definition
+ */
+
+#ifndef VFIO_MDEV_H
+#define VFIO_MDEV_H
+
+#include <linux/mdev.h>
+
+/**
+ * struct vfio_mdev_device_ops - Structure to be registered for each
+ * mdev device to register the device to vfio-mdev module.
+ *
+ * @open:		Open mediated device.
+ *			@mdev: mediated device.
+ *			Returns integer: success (0) or error (< 0)
+ * @release:		release mediated device
+ *			@mdev: mediated device.
+ * @read:		Read emulation callback
+ *			@mdev: mediated device structure
+ *			@buf: read buffer
+ *			@count: number of bytes to read
+ *			@ppos: address.
+ *			Retuns number on bytes read on success or error.
+ * @write:		Write emulation callback
+ *			@mdev: mediated device structure
+ *			@buf: write buffer
+ *			@count: number of bytes to be written
+ *			@ppos: address.
+ *			Retuns number on bytes written on success or error.
+ * @ioctl:		IOCTL callback
+ *			@mdev: mediated device structure
+ *			@cmd: ioctl command
+ *			@arg: arguments to ioctl
+ * @mmap:		mmap callback
+ *			@mdev: mediated device structure
+ *			@vma: vma structure
+ */
+struct vfio_mdev_device_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);
+};
+
+#endif
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index fd8491e58fe2..9db771ed5e4b 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -30,6 +30,7 @@
 #include <linux/iommu.h>
 #include <linux/sysfs.h>
 #include <linux/mdev.h>
+#include <linux/vfio_mdev.h>
 #include <linux/pci.h>
 #include <linux/dma-buf.h>
 #include <linux/highmem.h>
@@ -516,6 +517,8 @@ static int mbochs_reset(struct mdev_device *mdev)
 	return 0;
 }
 
+static const struct vfio_mdev_device_ops vfio_mdev_ops;
+
 static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	const struct mbochs_type *type = mbochs_find_type(kobj);
@@ -561,7 +564,7 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev)
 	mbochs_reset(mdev);
 
 	mbochs_used_mbytes += type->mbytes;
-	mdev_set_class(mdev, MDEV_ID_VFIO);
+	mdev_set_class(mdev, MDEV_ID_VFIO, &vfio_mdev_ops);
 	return 0;
 
 err_mem:
@@ -1419,12 +1422,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_device_ops vfio_mdev_ops = {
 	.open			= mbochs_open,
 	.release		= mbochs_close,
 	.read			= mbochs_read,
@@ -1433,6 +1431,14 @@ 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,
+};
+
 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 889472b06708..f0fdbc6ec706 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -26,6 +26,7 @@
 #include <linux/iommu.h>
 #include <linux/sysfs.h>
 #include <linux/mdev.h>
+#include <linux/vfio_mdev.h>
 #include <linux/pci.h>
 #include <drm/drm_fourcc.h>
 #include "mdpy-defs.h"
@@ -226,6 +227,8 @@ static int mdpy_reset(struct mdev_device *mdev)
 	return 0;
 }
 
+static const struct vfio_mdev_device_ops vfio_mdev_ops;
+
 static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	const struct mdpy_type *type = mdpy_find_type(kobj);
@@ -269,7 +272,8 @@ static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev)
 	mdpy_reset(mdev);
 
 	mdpy_count++;
-	mdev_set_class(mdev, MDEV_ID_VFIO);
+
+	mdev_set_class(mdev, MDEV_ID_VFIO, &vfio_mdev_ops);
 	return 0;
 }
 
@@ -726,12 +730,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_device_ops vfio_mdev_ops = {
 	.open			= mdpy_open,
 	.release		= mdpy_close,
 	.read			= mdpy_read,
@@ -740,6 +739,14 @@ 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,
+};
+
 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 618810ca4331..18d0f83a228a 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -27,6 +27,7 @@
 #include <linux/ctype.h>
 #include <linux/file.h>
 #include <linux/mdev.h>
+#include <linux/vfio_mdev.h>
 #include <linux/pci.h>
 #include <linux/serial.h>
 #include <uapi/linux/serial_reg.h>
@@ -708,6 +709,8 @@ static ssize_t mdev_access(struct mdev_device *mdev, u8 *buf, size_t count,
 	return ret;
 }
 
+static const struct vfio_mdev_device_ops vfio_dev_ops;
+
 static int mtty_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct mdev_state *mdev_state;
@@ -755,7 +758,7 @@ static int mtty_create(struct kobject *kobj, struct mdev_device *mdev)
 	list_add(&mdev_state->next, &mdev_devices_list);
 	mutex_unlock(&mdev_list_lock);
 
-	mdev_set_class(mdev, MDEV_ID_VFIO);
+	mdev_set_class(mdev, MDEV_ID_VFIO, &vfio_dev_ops);
 	return 0;
 }
 
@@ -1388,6 +1391,14 @@ static struct attribute_group *mdev_type_groups[] = {
 	NULL,
 };
 
+static const struct vfio_mdev_device_ops vfio_dev_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,
@@ -1395,11 +1406,6 @@ 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,
 };
 
 static void mtty_device_release(struct device *dev)
-- 
2.19.1

[PATCH V3 5/7] mdev: introduce virtio device and its device ops

[Jason Wang]

This patch implements basic support for mdev driver that supports
virtio transport for kernel virtio driver.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/linux/mdev.h        |   1 +
 include/linux/virtio_mdev.h | 148 ++++++++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+)
 create mode 100644 include/linux/virtio_mdev.h

diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index f491308674e5..298581e7ad45 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -124,6 +124,7 @@ struct mdev_device *mdev_from_dev(struct device *dev);
 
 enum {
 	MDEV_ID_VFIO = 1,
+	MDEV_ID_VIRTIO = 2,
 	/* New entries must be added here */
 };
 
diff --git a/include/linux/virtio_mdev.h b/include/linux/virtio_mdev.h
new file mode 100644
index 000000000000..b39c62534acc
--- /dev/null
+++ b/include/linux/virtio_mdev.h
@@ -0,0 +1,148 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Virtio mediated device driver
+ *
+ * Copyright 2019, Red Hat Corp.
+ *     Author: Jason Wang <jasowang@redhat.com>
+ */
+#ifndef _LINUX_VIRTIO_MDEV_H
+#define _LINUX_VIRTIO_MDEV_H
+
+#include <linux/interrupt.h>
+#include <linux/mdev.h>
+#include <uapi/linux/vhost.h>
+
+#define VIRTIO_MDEV_DEVICE_API_STRING		"virtio-mdev"
+#define VIRTIO_MDEV_F_VERSION_1 0x1
+
+struct virtio_mdev_callback {
+	irqreturn_t (*callback)(void *data);
+	void *private;
+};
+
+/**
+ * struct vfio_mdev_device_ops - Structure to be registered for each
+ * mdev device to register the device to virtio-mdev module.
+ *
+ * @set_vq_address:		Set the address of virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ *				@desc_area: address of desc area
+ *				@driver_area: address of driver area
+ *				@device_area: address of device area
+ *				Returns integer: success (0) or error (< 0)
+ * @set_vq_num:		Set the size of virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ *				@num: the size of virtqueue
+ * @kick_vq:			Kick the virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ * @set_vq_cb:			Set the interrut calback function for
+ *				a virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ *				@cb: virtio-mdev interrupt callback structure
+ * @set_vq_ready:		Set ready status for a virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ *				@ready: ready (true) not ready(false)
+ * @get_vq_ready:		Get ready status for a virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ *				Returns boolean: ready (true) or not (false)
+ * @set_vq_state:		Set the state for a virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ *				@state: virtqueue state (last_avail_idx)
+ *				Returns integer: success (0) or error (< 0)
+ * @get_vq_state:		Get the state for a virtqueue
+ *				@mdev: mediated device
+ *				@idx: virtqueue index
+ *				Returns virtqueue state (last_avail_idx)
+ * @get_vq_align:		Get the virtqueue align requirement
+ *				for the device
+ *				@mdev: mediated device
+ *				Returns virtqueue algin requirement
+ * @get_features:		Get virtio features supported by the device
+ *				@mdev: mediated device
+ *				Returns the virtio features support by the
+ *				device
+ * @get_features:		Set virtio features supported by the driver
+ *				@mdev: mediated device
+ *				@features: feature support by the driver
+ *				Returns integer: success (0) or error (< 0)
+ * @set_config_cb:		Set the config interrupt callback
+ *				@mdev: mediated device
+ *				@cb: virtio-mdev interrupt callback structure
+ * @get_vq_num_max:		Get the max size of virtqueue
+ *				@mdev: mediated device
+ *				Returns u16: max size of virtqueue
+ * @get_device_id:		Get virtio device id
+ *				@mdev: mediated device
+ *				Returns u32: virtio device id
+ * @get_vendor_id:		Get virtio vendor id
+ *				@mdev: mediated device
+ *				Returns u32: virtio vendor id
+ * @get_status: 		Get the device status
+ *				@mdev: mediated device
+ *				Returns u8: virtio device status
+ * @set_status: 		Set the device status
+ *				@mdev: mediated device
+ *				@status: virtio device status
+ * @get_config: 		Read from device specific configuration space
+ *				@mdev: mediated device
+ *				@offset: offset from the beginning of
+ *				configuration space
+ *				@buf: buffer used to read to
+ *				@len: the length to read from
+ *				configration space
+ * @set_config: 		Write to device specific configuration space
+ *				@mdev: mediated device
+ *				@offset: offset from the beginning of
+ *				configuration space
+ *				@buf: buffer used to write from
+ *				@len: the length to write to
+ *				configration space
+ * @get_mdev_features:		Get the feature of virtio mdev device
+ *				@mdev: mediated device
+ *				Returns the mdev features (API) support by
+ *				the device.
+ * @get_generation:		Get device generaton
+ *				@mdev: mediated device
+ *				Returns u32: device generation
+ */
+struct virtio_mdev_device_ops {
+	/* Virtqueue ops */
+	int (*set_vq_address)(struct mdev_device *mdev,
+			      u16 idx, u64 desc_area, u64 driver_area,
+			      u64 device_area);
+	void (*set_vq_num)(struct mdev_device *mdev, u16 idx, u32 num);
+	void (*kick_vq)(struct mdev_device *mdev, u16 idx);
+	void (*set_vq_cb)(struct mdev_device *mdev, u16 idx,
+			  struct virtio_mdev_callback *cb);
+	void (*set_vq_ready)(struct mdev_device *mdev, u16 idx, bool ready);
+	bool (*get_vq_ready)(struct mdev_device *mdev, u16 idx);
+	int (*set_vq_state)(struct mdev_device *mdev, u16 idx, u64 state);
+	u64 (*get_vq_state)(struct mdev_device *mdev, u16 idx);
+
+	/* Device ops */
+	u16 (*get_vq_align)(struct mdev_device *mdev);
+	u64 (*get_features)(struct mdev_device *mdev);
+	int (*set_features)(struct mdev_device *mdev, u64 features);
+	void (*set_config_cb)(struct mdev_device *mdev,
+			      struct virtio_mdev_callback *cb);
+	u16 (*get_vq_num_max)(struct mdev_device *mdev);
+	u32 (*get_device_id)(struct mdev_device *mdev);
+	u32 (*get_vendor_id)(struct mdev_device *mdev);
+	u8 (*get_status)(struct mdev_device *mdev);
+	void (*set_status)(struct mdev_device *mdev, u8 status);
+	void (*get_config)(struct mdev_device *mdev, unsigned int offset,
+			   void *buf, unsigned int len);
+	void (*set_config)(struct mdev_device *mdev, unsigned int offset,
+			   const void *buf, unsigned int len);
+	u64 (*get_mdev_features)(struct mdev_device *mdev);
+	u32 (*get_generation)(struct mdev_device *mdev);
+};
+
+#endif
-- 
2.19.1

[PATCH V3 6/7] virtio: introduce a mdev based transport

[Jason Wang]

This patch introduces a new mdev transport for virtio. This is used to
use kernel virtio driver to drive the mediated device that is capable
of populating virtqueue directly.

A new virtio-mdev driver will be registered to the mdev bus, when a
new virtio-mdev device is probed, it will register the device with
mdev based config ops. This means it is a software transport between
mdev driver and mdev device. The transport was implemented through
device specific opswhich is a part of mdev_parent_ops now.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/Kconfig       |   7 +
 drivers/virtio/Makefile      |   1 +
 drivers/virtio/virtio_mdev.c | 416 +++++++++++++++++++++++++++++++++++
 3 files changed, 424 insertions(+)
 create mode 100644 drivers/virtio/virtio_mdev.c

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 078615cf2afc..8d18722ab572 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -43,6 +43,13 @@ config VIRTIO_PCI_LEGACY
 
 	  If unsure, say Y.
 
+config VIRTIO_MDEV_DEVICE
+	tristate "VIRTIO driver for Mediated devices"
+	depends on VFIO_MDEV && VIRTIO
+	default n
+	help
+	  VIRTIO based driver for Mediated devices.
+
 config VIRTIO_PMEM
 	tristate "Support for virtio pmem driver"
 	depends on VIRTIO
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 3a2b5c5dcf46..ebc7fa15ae82 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -6,3 +6,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
 virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
 obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
 obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
+obj-$(CONFIG_VIRTIO_MDEV_DEVICE) += virtio_mdev.o
diff --git a/drivers/virtio/virtio_mdev.c b/drivers/virtio/virtio_mdev.c
new file mode 100644
index 000000000000..8516f3f0f658
--- /dev/null
+++ b/drivers/virtio/virtio_mdev.c
@@ -0,0 +1,416 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * VIRTIO based driver for Mediated device
+ *
+ * Copyright (c) 2019, Red Hat. All rights reserved.
+ *     Author: Jason Wang <jasowang@redhat.com>
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/uuid.h>
+#include <linux/mdev.h>
+#include <linux/virtio_mdev.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ring.h>
+
+#define DRIVER_VERSION  "0.1"
+#define DRIVER_AUTHOR   "Red Hat Corporation"
+#define DRIVER_DESC     "VIRTIO based driver for Mediated device"
+
+#define to_virtio_mdev_device(dev) \
+	container_of(dev, struct virtio_mdev_device, vdev)
+
+struct virtio_mdev_device {
+	struct virtio_device vdev;
+	struct mdev_device *mdev;
+	unsigned long version;
+
+	struct virtqueue **vqs;
+	/* The lock to protect virtqueue list */
+	spinlock_t lock;
+	struct list_head virtqueues;
+};
+
+struct virtio_mdev_vq_info {
+	/* the actual virtqueue */
+	struct virtqueue *vq;
+
+	/* the list node for the virtqueues list */
+	struct list_head node;
+};
+
+static struct mdev_device *vm_get_mdev(struct virtio_device *vdev)
+{
+	struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vdev);
+	struct mdev_device *mdev = vm_dev->mdev;
+
+	return mdev;
+}
+
+static void virtio_mdev_get(struct virtio_device *vdev, unsigned offset,
+			    void *buf, unsigned len)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	ops->get_config(mdev, offset, buf, len);
+}
+
+static void virtio_mdev_set(struct virtio_device *vdev, unsigned offset,
+			    const void *buf, unsigned len)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	ops->set_config(mdev, offset, buf, len);
+}
+
+static u32 virtio_mdev_generation(struct virtio_device *vdev)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	return ops->get_generation(mdev);
+}
+
+static u8 virtio_mdev_get_status(struct virtio_device *vdev)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	return ops->get_status(mdev);
+}
+
+static void virtio_mdev_set_status(struct virtio_device *vdev, u8 status)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	return ops->set_status(mdev, status);
+}
+
+static void virtio_mdev_reset(struct virtio_device *vdev)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	return ops->set_status(mdev, 0);
+}
+
+static bool virtio_mdev_notify(struct virtqueue *vq)
+{
+	struct mdev_device *mdev = vm_get_mdev(vq->vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	ops->kick_vq(mdev, vq->index);
+
+	return true;
+}
+
+static irqreturn_t virtio_mdev_config_cb(void *private)
+{
+	struct virtio_mdev_device *vm_dev = private;
+
+	virtio_config_changed(&vm_dev->vdev);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t virtio_mdev_virtqueue_cb(void *private)
+{
+	struct virtio_mdev_vq_info *info = private;
+
+	return vring_interrupt(0, info->vq);
+}
+
+static struct virtqueue *
+virtio_mdev_setup_vq(struct virtio_device *vdev, unsigned index,
+		     void (*callback)(struct virtqueue *vq),
+		     const char *name, bool ctx)
+{
+	struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vdev);
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+	struct virtio_mdev_vq_info *info;
+	struct virtio_mdev_callback cb;
+	struct virtqueue *vq;
+	u64 desc_addr, driver_addr, device_addr;
+	unsigned long flags;
+	u32 align, num;
+	int err;
+
+	if (!name)
+		return NULL;
+
+	/* Queue shouldn't already be set up. */
+	if (ops->get_vq_ready(mdev, index))
+		return ERR_PTR(-ENOENT);
+
+	/* Allocate and fill out our active queue description */
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	num = ops->get_vq_num_max(mdev);
+	if (num == 0) {
+		err = -ENOENT;
+		goto error_new_virtqueue;
+	}
+
+	/* Create the vring */
+	align = ops->get_vq_align(mdev);
+	vq = vring_create_virtqueue(index, num, align, vdev,
+				    true, true, ctx,
+				    virtio_mdev_notify, callback, name);
+	if (!vq) {
+		err = -ENOMEM;
+		goto error_new_virtqueue;
+	}
+
+	/* Setup virtqueue callback */
+	cb.callback = virtio_mdev_virtqueue_cb;
+	cb.private = info;
+	ops->set_vq_cb(mdev, index, &cb);
+	ops->set_vq_num(mdev, index, virtqueue_get_vring_size(vq));
+
+	desc_addr = virtqueue_get_desc_addr(vq);
+	driver_addr = virtqueue_get_avail_addr(vq);
+	device_addr = virtqueue_get_used_addr(vq);
+
+	if (ops->set_vq_address(mdev, index,
+				desc_addr, driver_addr,
+				device_addr)) {
+		err = -EINVAL;
+		goto err_vq;
+	}
+
+	ops->set_vq_ready(mdev, index, 1);
+
+	vq->priv = info;
+	info->vq = vq;
+
+	spin_lock_irqsave(&vm_dev->lock, flags);
+	list_add(&info->node, &vm_dev->virtqueues);
+	spin_unlock_irqrestore(&vm_dev->lock, flags);
+
+	return vq;
+
+err_vq:
+	vring_del_virtqueue(vq);
+error_new_virtqueue:
+	ops->set_vq_ready(mdev, index, 0);
+	WARN_ON(ops->get_vq_ready(mdev, index));
+	kfree(info);
+	return ERR_PTR(err);
+
+}
+
+static void virtio_mdev_del_vq(struct virtqueue *vq)
+{
+	struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vq->vdev);
+	struct mdev_device *mdev = vm_dev->mdev;
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+	struct virtio_mdev_vq_info *info = vq->priv;
+	unsigned int index = vq->index;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vm_dev->lock, flags);
+	list_del(&info->node);
+	spin_unlock_irqrestore(&vm_dev->lock, flags);
+
+	/* Select and deactivate the queue */
+	ops->set_vq_ready(mdev, index, 0);
+	WARN_ON(ops->get_vq_ready(mdev, index));
+
+	vring_del_virtqueue(vq);
+
+	kfree(info);
+}
+
+static void virtio_mdev_del_vqs(struct virtio_device *vdev)
+{
+	struct virtqueue *vq, *n;
+
+	list_for_each_entry_safe(vq, n, &vdev->vqs, list)
+		virtio_mdev_del_vq(vq);
+}
+
+static int virtio_mdev_find_vqs(struct virtio_device *vdev, unsigned nvqs,
+				struct virtqueue *vqs[],
+				vq_callback_t *callbacks[],
+				const char * const names[],
+				const bool *ctx,
+				struct irq_affinity *desc)
+{
+	struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vdev);
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+	struct virtio_mdev_callback cb;
+	int i, err, queue_idx = 0;
+
+	vm_dev->vqs = kmalloc_array(queue_idx, sizeof(*vm_dev->vqs),
+				    GFP_KERNEL);
+	if (!vm_dev->vqs)
+		return -ENOMEM;
+
+	for (i = 0; i < nvqs; ++i) {
+		if (!names[i]) {
+			vqs[i] = NULL;
+			continue;
+		}
+
+		vqs[i] = virtio_mdev_setup_vq(vdev, queue_idx++,
+					      callbacks[i], names[i], ctx ?
+					      ctx[i] : false);
+		if (IS_ERR(vqs[i])) {
+			err = PTR_ERR(vqs[i]);
+			goto err_setup_vq;
+		}
+	}
+
+	cb.callback = virtio_mdev_config_cb;
+	cb.private = vm_dev;
+	ops->set_config_cb(mdev, &cb);
+
+	return 0;
+
+err_setup_vq:
+	kfree(vm_dev->vqs);
+	virtio_mdev_del_vqs(vdev);
+	return err;
+}
+
+static u64 virtio_mdev_get_features(struct virtio_device *vdev)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	return ops->get_features(mdev);
+}
+
+static int virtio_mdev_finalize_features(struct virtio_device *vdev)
+{
+	struct mdev_device *mdev = vm_get_mdev(vdev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+
+	/* Give virtio_ring a chance to accept features. */
+	vring_transport_features(vdev);
+
+	return ops->set_features(mdev, vdev->features);
+}
+
+static const char *virtio_mdev_bus_name(struct virtio_device *vdev)
+{
+	struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vdev);
+	struct mdev_device *mdev = vm_dev->mdev;
+
+	return dev_name(mdev_dev(mdev));
+}
+
+static const struct virtio_config_ops virtio_mdev_config_ops = {
+	.get		= virtio_mdev_get,
+	.set		= virtio_mdev_set,
+	.generation	= virtio_mdev_generation,
+	.get_status	= virtio_mdev_get_status,
+	.set_status	= virtio_mdev_set_status,
+	.reset		= virtio_mdev_reset,
+	.find_vqs	= virtio_mdev_find_vqs,
+	.del_vqs	= virtio_mdev_del_vqs,
+	.get_features	= virtio_mdev_get_features,
+	.finalize_features = virtio_mdev_finalize_features,
+	.bus_name	= virtio_mdev_bus_name,
+};
+
+static void virtio_mdev_release_dev(struct device *_d)
+{
+	struct virtio_device *vdev =
+	       container_of(_d, struct virtio_device, dev);
+	struct virtio_mdev_device *vm_dev =
+	       container_of(vdev, struct virtio_mdev_device, vdev);
+
+	devm_kfree(_d, vm_dev);
+}
+
+static int virtio_mdev_probe(struct device *dev)
+{
+	struct mdev_device *mdev = mdev_from_dev(dev);
+	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
+	struct virtio_mdev_device *vm_dev;
+	int rc;
+
+	vm_dev = devm_kzalloc(dev, sizeof(*vm_dev), GFP_KERNEL);
+	if (!vm_dev)
+		return -ENOMEM;
+
+	vm_dev->vdev.dev.parent = dev;
+	vm_dev->vdev.dev.release = virtio_mdev_release_dev;
+	vm_dev->vdev.config = &virtio_mdev_config_ops;
+	vm_dev->mdev = mdev;
+	vm_dev->vqs = NULL;
+	INIT_LIST_HEAD(&vm_dev->virtqueues);
+	spin_lock_init(&vm_dev->lock);
+
+	vm_dev->version = ops->get_mdev_features(mdev);
+	if (vm_dev->version != VIRTIO_MDEV_F_VERSION_1) {
+		dev_err(dev, "VIRTIO_MDEV_F_VERSION_1 is mandatory\n");
+		return -ENXIO;
+	}
+
+	vm_dev->vdev.id.device = ops->get_device_id(mdev);
+	if (vm_dev->vdev.id.device == 0)
+		return -ENODEV;
+
+	vm_dev->vdev.id.vendor = ops->get_vendor_id(mdev);
+	rc = register_virtio_device(&vm_dev->vdev);
+	if (rc)
+		put_device(dev);
+	else
+		dev_set_drvdata(dev, vm_dev);
+
+	return rc;
+}
+
+static void virtio_mdev_remove(struct device *dev)
+{
+	struct virtio_mdev_device *vm_dev = dev_get_drvdata(dev);
+
+	unregister_virtio_device(&vm_dev->vdev);
+}
+
+static struct mdev_class_id id_table[] = {
+	{ MDEV_ID_VIRTIO },
+	{ 0 },
+};
+
+MODULE_DEVICE_TABLE(mdev, id_table);
+
+static struct mdev_driver virtio_mdev_driver = {
+	.name	= "virtio_mdev",
+	.probe	= virtio_mdev_probe,
+	.remove = virtio_mdev_remove,
+	.id_table = id_table,
+};
+
+static int __init virtio_mdev_init(void)
+{
+	return mdev_register_driver(&virtio_mdev_driver, THIS_MODULE);
+}
+
+static void __exit virtio_mdev_exit(void)
+{
+	mdev_unregister_driver(&virtio_mdev_driver);
+}
+
+module_init(virtio_mdev_init)
+module_exit(virtio_mdev_exit)
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
-- 
2.19.1

[PATCH V3 7/7] docs: sample driver to demonstrate how to implement virtio-mdev framework

[Jason Wang]

This sample driver creates mdev device that simulate virtio net device
over virtio mdev transport. The device is implemented through vringh
and workqueue. A device specific dma ops is to make sure HVA is used
directly as the IOVA. This should be sufficient for kernel virtio
driver to work.

Only 'virtio' type is supported right now. I plan to add 'vhost' type
on top which requires some virtual IOMMU implemented in this sample
driver.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 MAINTAINERS                |   1 +
 samples/Kconfig            |   7 +
 samples/vfio-mdev/Makefile |   1 +
 samples/vfio-mdev/mvnet.c  | 691 +++++++++++++++++++++++++++++++++++++
 4 files changed, 700 insertions(+)
 create mode 100644 samples/vfio-mdev/mvnet.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d196a023b5e..cb51351cd5c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17254,6 +17254,7 @@ F:	include/linux/virtio*.h
 F:	include/uapi/linux/virtio_*.h
 F:	drivers/crypto/virtio/
 F:	mm/balloon_compaction.c
+F:	samples/vfio-mdev/mvnet.c
 
 VIRTIO BLOCK AND SCSI DRIVERS
 M:	"Michael S. Tsirkin" <mst@redhat.com>
diff --git a/samples/Kconfig b/samples/Kconfig
index c8dacb4dda80..a1a1ca2c00b7 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -131,6 +131,13 @@ config SAMPLE_VFIO_MDEV_MDPY
 	  mediated device.  It is a simple framebuffer and supports
 	  the region display interface (VFIO_GFX_PLANE_TYPE_REGION).
 
+config SAMPLE_VIRTIO_MDEV_NET
+        tristate "Build virtio mdev net example mediated device sample code -- loadable modules only"
+	depends on VIRTIO_MDEV_DEVICE && VHOST_RING && m
+	help
+	  Build a networking sample device for use as a virtio
+	  mediated device.
+
 config SAMPLE_VFIO_MDEV_MDPY_FB
 	tristate "Build VFIO mdpy example guest fbdev driver -- loadable module only"
 	depends on FB && m
diff --git a/samples/vfio-mdev/Makefile b/samples/vfio-mdev/Makefile
index 10d179c4fdeb..f34af90ed0a0 100644
--- a/samples/vfio-mdev/Makefile
+++ b/samples/vfio-mdev/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_SAMPLE_VFIO_MDEV_MTTY) += mtty.o
 obj-$(CONFIG_SAMPLE_VFIO_MDEV_MDPY) += mdpy.o
 obj-$(CONFIG_SAMPLE_VFIO_MDEV_MDPY_FB) += mdpy-fb.o
 obj-$(CONFIG_SAMPLE_VFIO_MDEV_MBOCHS) += mbochs.o
+obj-$(CONFIG_SAMPLE_VIRTIO_MDEV_NET) += mvnet.o
diff --git a/samples/vfio-mdev/mvnet.c b/samples/vfio-mdev/mvnet.c
new file mode 100644
index 000000000000..b218e7075611
--- /dev/null
+++ b/samples/vfio-mdev/mvnet.c
@@ -0,0 +1,691 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Mediated virtual virtio-net device driver.
+ *
+ * Copyright (c) 2019, Red Hat Inc. All rights reserved.
+ *     Author: Jason Wang <jasowang@redhat.com>
+ *
+ * Sample driver that creates mdev device that simulates ethernet loopback
+ * device.
+ *
+ * Usage:
+ *
+ * # modprobe virtio_mdev
+ * # modprobe mvnet
+ * # cd /sys/devices/virtual/mvnet/mvnet/mdev_supported_types/mvnet-virtio
+ * # echo "83b8f4f2-509f-382f-3c1e-e6bfe0fa1001" > ./create
+ * # cd devices/83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
+ * # ls -d virtio0
+ * virtio0
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
+#include <linux/uuid.h>
+#include <linux/iommu.h>
+#include <linux/sysfs.h>
+#include <linux/file.h>
+#include <linux/etherdevice.h>
+#include <linux/mdev.h>
+#include <linux/vringh.h>
+#include <linux/virtio_mdev.h>
+#include <uapi/linux/virtio_config.h>
+#include <uapi/linux/virtio_net.h>
+
+#define VERSION_STRING  "0.1"
+#define DRIVER_AUTHOR   "Red Hat Corporation"
+
+#define MVNET_CLASS_NAME "mvnet"
+#define MVNET_NAME       "mvnet"
+
+/*
+ * Global Structures
+ */
+
+static struct mvnet_dev {
+	struct class	*vd_class;
+	struct idr	vd_idr;
+	struct device	dev;
+} mvnet_dev;
+
+struct mvnet_virtqueue {
+	struct vringh vring;
+	struct vringh_kiov iov;
+	unsigned short head;
+	bool ready;
+	u64 desc_addr;
+	u64 device_addr;
+	u64 driver_addr;
+	u32 num;
+	void *private;
+	irqreturn_t (*cb)(void *data);
+};
+
+#define MVNET_QUEUE_ALIGN PAGE_SIZE
+#define MVNET_QUEUE_MAX 256
+#define MVNET_DEVICE_ID 0x1
+#define MVNET_VENDOR_ID 0
+
+u64 mvnet_features = (1ULL << VIRTIO_F_ANY_LAYOUT) |
+		     (1ULL << VIRTIO_F_VERSION_1) |
+		     (1ULL << VIRTIO_F_IOMMU_PLATFORM);
+
+/* State of each mdev device */
+struct mvnet_state {
+	struct mvnet_virtqueue vqs[2];
+	struct work_struct work;
+	spinlock_t lock;
+	struct mdev_device *mdev;
+	struct virtio_net_config config;
+	void *buffer;
+	u32 status;
+	u32 generation;
+	u64 features;
+	struct list_head next;
+};
+
+static struct mutex mdev_list_lock;
+static struct list_head mdev_devices_list;
+
+static void mvnet_queue_ready(struct mvnet_state *mvnet, unsigned int idx)
+{
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+	int ret;
+
+	ret = vringh_init_kern(&vq->vring, mvnet_features, MVNET_QUEUE_MAX,
+			       false, (struct vring_desc *)vq->desc_addr,
+			       (struct vring_avail *)vq->driver_addr,
+			       (struct vring_used *)vq->device_addr);
+}
+
+static void mvnet_vq_reset(struct mvnet_virtqueue *vq)
+{
+	vq->ready = 0;
+	vq->desc_addr = 0;
+	vq->driver_addr = 0;
+	vq->device_addr = 0;
+	vq->cb = NULL;
+	vq->private = NULL;
+	vringh_init_kern(&vq->vring, mvnet_features, MVNET_QUEUE_MAX,
+			false, 0, 0, 0);
+}
+
+static void mvnet_reset(struct mvnet_state *mvnet)
+{
+	int i;
+
+	for (i = 0; i < 2; i++)
+		mvnet_vq_reset(&mvnet->vqs[i]);
+
+	mvnet->features = 0;
+	mvnet->status = 0;
+	++mvnet->generation;
+}
+
+static void mvnet_work(struct work_struct *work)
+{
+	struct mvnet_state *mvnet = container_of(work, struct
+						 mvnet_state, work);
+	struct mvnet_virtqueue *txq = &mvnet->vqs[1];
+	struct mvnet_virtqueue *rxq = &mvnet->vqs[0];
+	size_t read, write, total_write;
+	int err;
+	int pkts = 0;
+
+	spin_lock(&mvnet->lock);
+
+	if (!txq->ready || !rxq->ready)
+		goto out;
+
+	while (true) {
+		total_write = 0;
+		err = vringh_getdesc_kern(&txq->vring, &txq->iov, NULL,
+					  &txq->head, GFP_ATOMIC);
+		if (err <= 0)
+			break;
+
+		err = vringh_getdesc_kern(&rxq->vring, NULL, &rxq->iov,
+					  &rxq->head, GFP_ATOMIC);
+		if (err <= 0) {
+			vringh_complete_kern(&txq->vring, txq->head, 0);
+			break;
+		}
+
+		while (true) {
+			read = vringh_iov_pull_kern(&txq->iov, mvnet->buffer,
+						    PAGE_SIZE);
+			if (read <= 0)
+				break;
+
+			write = vringh_iov_push_kern(&rxq->iov, mvnet->buffer,
+						     read);
+			if (write <= 0)
+				break;
+
+			total_write += write;
+		}
+
+		/* Make sure data is wrote before advancing index */
+		smp_wmb();
+
+		vringh_complete_kern(&txq->vring, txq->head, 0);
+		vringh_complete_kern(&rxq->vring, rxq->head, total_write);
+
+		/* Make sure used is visible before rasing the interrupt. */
+		smp_wmb();
+
+		local_bh_disable();
+		if (txq->cb)
+			txq->cb(txq->private);
+		if (rxq->cb)
+			rxq->cb(rxq->private);
+		local_bh_enable();
+
+		if (++pkts > 4) {
+			schedule_work(&mvnet->work);
+			goto out;
+		}
+	}
+
+out:
+	spin_unlock(&mvnet->lock);
+}
+
+static dma_addr_t mvnet_map_page(struct device *dev, struct page *page,
+				 unsigned long offset, size_t size,
+				 enum dma_data_direction dir,
+				 unsigned long attrs)
+{
+	/* Vringh can only use HVA */
+	return (dma_addr_t)(page_address(page) + offset);
+}
+
+static void mvnet_unmap_page(struct device *dev, dma_addr_t dma_addr,
+			     size_t size, enum dma_data_direction dir,
+			     unsigned long attrs)
+{
+}
+
+static void *mvnet_alloc_coherent(struct device *dev, size_t size,
+				  dma_addr_t *dma_addr, gfp_t flag,
+				  unsigned long attrs)
+{
+	void *addr = kmalloc(size, flag);
+
+	if (addr == NULL)
+		*dma_addr = DMA_MAPPING_ERROR;
+	else
+		*dma_addr = (dma_addr_t) addr;
+
+	return addr;
+}
+
+static void mvnet_free_coherent(struct device *dev, size_t size,
+				void *vaddr, dma_addr_t dma_addr,
+				unsigned long attrs)
+{
+	kfree((void *)dma_addr);
+}
+
+static const struct dma_map_ops mvnet_dma_ops = {
+	.map_page = mvnet_map_page,
+	.unmap_page = mvnet_unmap_page,
+	.alloc = mvnet_alloc_coherent,
+	.free = mvnet_free_coherent,
+};
+
+static const struct virtio_mdev_device_ops virtio_mdev_ops;
+
+static int mvnet_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+	struct mvnet_state *mvnet;
+	struct virtio_net_config *config;
+	struct device *dev = mdev_dev(mdev);
+
+	if (!mdev)
+		return -EINVAL;
+
+	mvnet = kzalloc(sizeof(struct mvnet_state), GFP_KERNEL);
+	if (mvnet == NULL)
+		return -ENOMEM;
+
+	mvnet->buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!mvnet->buffer) {
+		kfree(mvnet);
+		return -ENOMEM;
+	}
+
+	config = &mvnet->config;
+	config->mtu = 1500;
+	config->status = VIRTIO_NET_S_LINK_UP;
+	eth_random_addr(config->mac);
+
+	INIT_WORK(&mvnet->work, mvnet_work);
+
+	spin_lock_init(&mvnet->lock);
+	mvnet->mdev = mdev;
+	mdev_set_drvdata(mdev, mvnet);
+
+	mutex_lock(&mdev_list_lock);
+	list_add(&mvnet->next, &mdev_devices_list);
+	mutex_unlock(&mdev_list_lock);
+
+	dev->coherent_dma_mask = DMA_BIT_MASK(64);
+	set_dma_ops(dev, &mvnet_dma_ops);
+
+	mdev_set_class(mdev, MDEV_ID_VIRTIO, &virtio_mdev_ops);
+
+	return 0;
+}
+
+static int mvnet_remove(struct mdev_device *mdev)
+{
+	struct mvnet_state *mds, *tmp_mds;
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	int ret = -EINVAL;
+
+	mutex_lock(&mdev_list_lock);
+	list_for_each_entry_safe(mds, tmp_mds, &mdev_devices_list, next) {
+		if (mvnet == mds) {
+			list_del(&mvnet->next);
+			mdev_set_drvdata(mdev, NULL);
+			kfree(mvnet->buffer);
+			kfree(mvnet);
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&mdev_list_lock);
+
+	return ret;
+}
+
+static ssize_t
+sample_mvnet_dev_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	if (mdev_from_dev(dev))
+		return sprintf(buf, "This is MDEV %s\n", dev_name(dev));
+
+	return sprintf(buf, "\n");
+}
+
+static DEVICE_ATTR_RO(sample_mvnet_dev);
+
+static struct attribute *mvnet_dev_attrs[] = {
+	&dev_attr_sample_mvnet_dev.attr,
+	NULL,
+};
+
+static const struct attribute_group mvnet_dev_group = {
+	.name  = "mvnet_dev",
+	.attrs = mvnet_dev_attrs,
+};
+
+static const struct attribute_group *mvnet_dev_groups[] = {
+	&mvnet_dev_group,
+	NULL,
+};
+
+static ssize_t
+sample_mdev_dev_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	if (mdev_from_dev(dev))
+		return sprintf(buf, "This is MDEV %s\n", dev_name(dev));
+
+	return sprintf(buf, "\n");
+}
+
+static DEVICE_ATTR_RO(sample_mdev_dev);
+
+static struct attribute *mdev_dev_attrs[] = {
+	&dev_attr_sample_mdev_dev.attr,
+	NULL,
+};
+
+static const struct attribute_group mdev_dev_group = {
+	.name  = "vendor",
+	.attrs = mdev_dev_attrs,
+};
+
+static const struct attribute_group *mdev_dev_groups[] = {
+	&mdev_dev_group,
+	NULL,
+};
+
+#define MVNET_STRING_LEN 16
+
+static ssize_t
+name_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	char name[MVNET_STRING_LEN];
+	const char *name_str = "virtio-net";
+
+	snprintf(name, MVNET_STRING_LEN, "%s", dev_driver_string(dev));
+	if (!strcmp(kobj->name, name))
+		return sprintf(buf, "%s\n", name_str);
+
+	return -EINVAL;
+}
+
+static MDEV_TYPE_ATTR_RO(name);
+
+static ssize_t
+available_instances_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	return sprintf(buf, "%d\n", INT_MAX);
+}
+
+static MDEV_TYPE_ATTR_RO(available_instances);
+
+static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
+			       char *buf)
+{
+	return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
+}
+
+static MDEV_TYPE_ATTR_RO(device_api);
+
+static struct attribute *mdev_types_attrs[] = {
+	&mdev_type_attr_name.attr,
+	&mdev_type_attr_device_api.attr,
+	&mdev_type_attr_available_instances.attr,
+	NULL,
+};
+
+static struct attribute_group mdev_type_group = {
+	.name  = "virtio",
+	.attrs = mdev_types_attrs,
+};
+
+/* TBD: "vhost" type */
+
+static struct attribute_group *mdev_type_groups[] = {
+	&mdev_type_group,
+	NULL,
+};
+
+static int mvnet_set_vq_address(struct mdev_device *mdev, u16 idx,
+				u64 desc_area, u64 driver_area, u64 device_area)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+
+	vq->desc_addr = desc_area;
+	vq->driver_addr = driver_area;
+	vq->device_addr = device_area;
+
+	return 0;
+}
+
+static void mvnet_set_vq_num(struct mdev_device *mdev, u16 idx, u32 num)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+
+	vq->num = num;
+}
+
+static void mvnet_kick_vq(struct mdev_device *mdev, u16 idx)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+
+	if (vq->ready)
+		schedule_work(&mvnet->work);
+}
+
+static void mvnet_set_vq_cb(struct mdev_device *mdev, u16 idx,
+			    struct virtio_mdev_callback *cb)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+
+	vq->cb = cb->callback;
+	vq->private = cb->private;
+}
+
+static void mvnet_set_vq_ready(struct mdev_device *mdev, u16 idx, bool ready)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+
+	spin_lock(&mvnet->lock);
+	vq->ready = ready;
+	if (vq->ready)
+		mvnet_queue_ready(mvnet, idx);
+	spin_unlock(&mvnet->lock);
+}
+
+static bool mvnet_get_vq_ready(struct mdev_device *mdev, u16 idx)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+
+	return vq->ready;
+}
+
+static int mvnet_set_vq_state(struct mdev_device *mdev, u16 idx, u64 state)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+	struct vringh *vrh = &vq->vring;
+
+	spin_lock(&mvnet->lock);
+	vrh->last_avail_idx = state;
+	spin_unlock(&mvnet->lock);
+
+	return 0;
+}
+
+static u64 mvnet_get_vq_state(struct mdev_device *mdev, u16 idx)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+	struct mvnet_virtqueue *vq = &mvnet->vqs[idx];
+	struct vringh *vrh = &vq->vring;
+
+	return vrh->last_avail_idx;
+}
+
+static u16 mvnet_get_vq_align(struct mdev_device *mdev)
+{
+	return MVNET_QUEUE_ALIGN;
+}
+
+static u64 mvnet_get_features(struct mdev_device *mdev)
+{
+	return mvnet_features;
+}
+
+static int mvnet_set_features(struct mdev_device *mdev, u64 features)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+
+	/* DMA mapping must be done by driver */
+	if (!(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)))
+		return -EINVAL;
+
+	mvnet->features = features & mvnet_features;
+
+	return 0;
+}
+
+static void mvnet_set_config_cb(struct mdev_device *mdev,
+				struct virtio_mdev_callback *cb)
+{
+	/* We don't support config interrupt */
+}
+
+static u16 mvnet_get_vq_num_max(struct mdev_device *mdev)
+{
+	return MVNET_QUEUE_MAX;
+}
+
+static u32 mvnet_get_device_id(struct mdev_device *mdev)
+{
+	return MVNET_DEVICE_ID;
+}
+
+static u32 mvnet_get_vendor_id(struct mdev_device *mdev)
+{
+	return MVNET_VENDOR_ID;
+}
+
+static u8 mvnet_get_status(struct mdev_device *mdev)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+
+	return mvnet->status;
+}
+
+static void mvnet_set_status(struct mdev_device *mdev, u8 status)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+
+	mvnet->status = status;
+
+	if (status == 0) {
+		spin_lock(&mvnet->lock);
+		mvnet_reset(mvnet);
+		spin_unlock(&mvnet->lock);
+	}
+}
+
+static void mvnet_get_config(struct mdev_device *mdev, unsigned int offset,
+			     void *buf, unsigned int len)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+
+	if (offset + len < sizeof(struct virtio_net_config))
+		memcpy(buf, &mvnet->config + offset, len);
+}
+
+static void mvnet_set_config(struct mdev_device *mdev, unsigned int offset,
+			     const void *buf, unsigned int len)
+{
+	/* No writable config supportted by mvnet */
+}
+
+static u64 mvnet_get_mdev_features(struct mdev_device *mdev)
+{
+	return VIRTIO_MDEV_F_VERSION_1;
+}
+
+static u32 mvnet_get_generation(struct mdev_device *mdev)
+{
+	struct mvnet_state *mvnet = mdev_get_drvdata(mdev);
+
+	return mvnet->generation;
+}
+
+static const struct virtio_mdev_device_ops virtio_mdev_ops = {
+	.set_vq_address         = mvnet_set_vq_address,
+	.set_vq_num             = mvnet_set_vq_num,
+	.kick_vq                = mvnet_kick_vq,
+	.set_vq_cb              = mvnet_set_vq_cb,
+	.set_vq_ready           = mvnet_set_vq_ready,
+	.get_vq_ready           = mvnet_get_vq_ready,
+	.set_vq_state           = mvnet_set_vq_state,
+	.get_vq_state           = mvnet_get_vq_state,
+	.get_vq_align           = mvnet_get_vq_align,
+	.get_features           = mvnet_get_features,
+	.set_features           = mvnet_set_features,
+	.set_config_cb          = mvnet_set_config_cb,
+	.get_vq_num_max         = mvnet_get_vq_num_max,
+	.get_device_id          = mvnet_get_device_id,
+	.get_vendor_id          = mvnet_get_vendor_id,
+	.get_status             = mvnet_get_status,
+	.set_status             = mvnet_set_status,
+	.get_config             = mvnet_get_config,
+	.set_config             = mvnet_set_config,
+	.get_mdev_features      = mvnet_get_mdev_features,
+	.get_generation         = mvnet_get_generation,
+};
+
+static const struct mdev_parent_ops mdev_fops = {
+	.owner                  = THIS_MODULE,
+	.dev_attr_groups        = mvnet_dev_groups,
+	.mdev_attr_groups       = mdev_dev_groups,
+	.supported_type_groups  = mdev_type_groups,
+	.create                 = mvnet_create,
+	.remove			= mvnet_remove,
+};
+
+static void mvnet_device_release(struct device *dev)
+{
+	dev_dbg(dev, "mvnet: released\n");
+}
+
+static int __init mvnet_dev_init(void)
+{
+	int ret = 0;
+
+	pr_info("mvnet_dev: %s\n", __func__);
+
+	memset(&mvnet_dev, 0, sizeof(mvnet_dev));
+
+	idr_init(&mvnet_dev.vd_idr);
+
+	mvnet_dev.vd_class = class_create(THIS_MODULE, MVNET_CLASS_NAME);
+
+	if (IS_ERR(mvnet_dev.vd_class)) {
+		pr_err("Error: failed to register mvnet_dev class\n");
+		ret = PTR_ERR(mvnet_dev.vd_class);
+		goto failed1;
+	}
+
+	mvnet_dev.dev.class = mvnet_dev.vd_class;
+	mvnet_dev.dev.release = mvnet_device_release;
+	dev_set_name(&mvnet_dev.dev, "%s", MVNET_NAME);
+
+	ret = device_register(&mvnet_dev.dev);
+	if (ret)
+		goto failed2;
+
+	ret = mdev_register_device(&mvnet_dev.dev, &mdev_fops);
+	if (ret)
+		goto failed3;
+
+	mutex_init(&mdev_list_lock);
+	INIT_LIST_HEAD(&mdev_devices_list);
+
+	goto all_done;
+
+failed3:
+
+	device_unregister(&mvnet_dev.dev);
+failed2:
+	class_destroy(mvnet_dev.vd_class);
+
+failed1:
+all_done:
+	return ret;
+}
+
+static void __exit mvnet_dev_exit(void)
+{
+	mvnet_dev.dev.bus = NULL;
+	mdev_unregister_device(&mvnet_dev.dev);
+
+	device_unregister(&mvnet_dev.dev);
+	idr_destroy(&mvnet_dev.vd_idr);
+	class_destroy(mvnet_dev.vd_class);
+	mvnet_dev.vd_class = NULL;
+	pr_info("mvnet_dev: Unloaded!\n");
+}
+
+module_init(mvnet_dev_init)
+module_exit(mvnet_dev_exit)
+
+MODULE_LICENSE("GPL v2");
+MODULE_INFO(supported, "Simulate loopback ethernet device over mdev");
+MODULE_VERSION(VERSION_STRING);
+MODULE_AUTHOR(DRIVER_AUTHOR);
-- 
2.19.1

Re: [PATCH V3 5/7] mdev: introduce virtio device and its device ops

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 162 bytes --]

On Fri, Oct 11, 2019 at 04:15:55PM +0800, Jason Wang wrote:
> + * @set_vq_cb:			Set the interrut calback function for

s/interrut/interrupt/

s/calback/callback/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 5/7] mdev: introduce virtio device and its device ops

[Jason Wang]

On 2019/10/15 上午1:23, Stefan Hajnoczi wrote:
> On Fri, Oct 11, 2019 at 04:15:55PM +0800, Jason Wang wrote:
>> + * @set_vq_cb:			Set the interrut calback function for
> s/interrut/interrupt/
>
> s/calback/callback/


Fixed.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 6/7] virtio: introduce a mdev based transport

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1151 bytes --]

On Fri, Oct 11, 2019 at 04:15:56PM +0800, Jason Wang wrote:
> +struct virtio_mdev_device {
> +	struct virtio_device vdev;
> +	struct mdev_device *mdev;
> +	unsigned long version;
> +
> +	struct virtqueue **vqs;
> +	/* The lock to protect virtqueue list */
> +	spinlock_t lock;
> +	struct list_head virtqueues;

Is this a list of struct virtio_mdev_vq_info?  Please document the
actual type in a comment.

> +static int virtio_mdev_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> +				struct virtqueue *vqs[],
> +				vq_callback_t *callbacks[],
> +				const char * const names[],
> +				const bool *ctx,
> +				struct irq_affinity *desc)
> +{
> +	struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vdev);
> +	struct mdev_device *mdev = vm_get_mdev(vdev);
> +	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
> +	struct virtio_mdev_callback cb;
> +	int i, err, queue_idx = 0;
> +
> +	vm_dev->vqs = kmalloc_array(queue_idx, sizeof(*vm_dev->vqs),
> +				    GFP_KERNEL);

kmalloc_array(0, ...)?  I would have expected nvqs instead of queue_idx
(0).

What is this the purpose of vm_dev->vqs and does anything ever access it?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 6/7] virtio: introduce a mdev based transport

[Jason Wang]

On 2019/10/15 上午1:39, Stefan Hajnoczi wrote:
> On Fri, Oct 11, 2019 at 04:15:56PM +0800, Jason Wang wrote:
>> +struct virtio_mdev_device {
>> +	struct virtio_device vdev;
>> +	struct mdev_device *mdev;
>> +	unsigned long version;
>> +
>> +	struct virtqueue **vqs;
>> +	/* The lock to protect virtqueue list */
>> +	spinlock_t lock;
>> +	struct list_head virtqueues;
> Is this a list of struct virtio_mdev_vq_info?  Please document the
> actual type in a comment.


Ok.


>> +static int virtio_mdev_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>> +				struct virtqueue *vqs[],
>> +				vq_callback_t *callbacks[],
>> +				const char * const names[],
>> +				const bool *ctx,
>> +				struct irq_affinity *desc)
>> +{
>> +	struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vdev);
>> +	struct mdev_device *mdev = vm_get_mdev(vdev);
>> +	const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
>> +	struct virtio_mdev_callback cb;
>> +	int i, err, queue_idx = 0;
>> +
>> +	vm_dev->vqs = kmalloc_array(queue_idx, sizeof(*vm_dev->vqs),
>> +				    GFP_KERNEL);
> kmalloc_array(0, ...)?  I would have expected nvqs instead of queue_idx
> (0).
>
> What is this the purpose of vm_dev->vqs and does anything ever access it?


It's useless, will remove it.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 0/7] mdev based hardware virtio offloading support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1585 bytes --]

On Fri, Oct 11, 2019 at 04:15:50PM +0800, Jason Wang wrote:
> There are hardware that can do virtio datapath offloading while having
> its own control path. This path tries to implement a mdev based
> unified API to support using kernel virtio driver to drive those
> devices. This is done by introducing a new mdev transport for virtio
> (virtio_mdev) and register itself as a new kind of mdev driver. Then
> it provides a unified way for kernel virtio driver to talk with mdev
> device implementation.
> 
> Though the series only contains kernel driver support, the goal is to
> make the transport generic enough to support userspace drivers. This
> means vhost-mdev[1] could be built on top as well by resuing the
> transport.
> 
> A sample driver is also implemented which simulate a virito-net
> loopback ethernet device on top of vringh + workqueue. This could be
> used as a reference implementation for real hardware driver.
> 
> Consider mdev framework only support VFIO device and driver right now,
> this series also extend it to support other types. This is done
> through introducing class id to the device and pairing it with
> id_talbe claimed by the driver. On top, this seris also decouple
> device specific parents ops out of the common ones.

I was curious so I took a quick look and posted comments.

I guess this driver runs inside the guest since it registers virtio
devices?

If this is used with physical PCI devices that support datapath
offloading then how are physical devices presented to the guest without
SR-IOV?

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 0/7] mdev based hardware virtio offloading support

[Jason Wang]

On 2019/10/15 上午1:49, Stefan Hajnoczi wrote:
> On Fri, Oct 11, 2019 at 04:15:50PM +0800, Jason Wang wrote:
>> There are hardware that can do virtio datapath offloading while having
>> its own control path. This path tries to implement a mdev based
>> unified API to support using kernel virtio driver to drive those
>> devices. This is done by introducing a new mdev transport for virtio
>> (virtio_mdev) and register itself as a new kind of mdev driver. Then
>> it provides a unified way for kernel virtio driver to talk with mdev
>> device implementation.
>>
>> Though the series only contains kernel driver support, the goal is to
>> make the transport generic enough to support userspace drivers. This
>> means vhost-mdev[1] could be built on top as well by resuing the
>> transport.
>>
>> A sample driver is also implemented which simulate a virito-net
>> loopback ethernet device on top of vringh + workqueue. This could be
>> used as a reference implementation for real hardware driver.
>>
>> Consider mdev framework only support VFIO device and driver right now,
>> this series also extend it to support other types. This is done
>> through introducing class id to the device and pairing it with
>> id_talbe claimed by the driver. On top, this seris also decouple
>> device specific parents ops out of the common ones.
> I was curious so I took a quick look and posted comments.
>
> I guess this driver runs inside the guest since it registers virtio
> devices?


It could run in either guest or host. But the main focus is to run in 
the host then we can use virtio drivers in containers.


>
> If this is used with physical PCI devices that support datapath
> offloading then how are physical devices presented to the guest without
> SR-IOV?


We will do control path meditation through vhost-mdev[1] and 
vhost-vfio[2]. Then we will present a full virtio compatible ethernet 
device for guest.

SR-IOV is not a must, any mdev device that implements the API defined in 
patch 5 can be used by this framework.

Thanks

[1] https://lkml.org/lkml/2019/9/26/15

[2] https://patchwork.ozlabs.org/cover/984763/


>
> Stefan
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 0/7] mdev based hardware virtio offloading support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 2548 bytes --]

On Tue, Oct 15, 2019 at 11:37:17AM +0800, Jason Wang wrote:
> 
> On 2019/10/15 上午1:49, Stefan Hajnoczi wrote:
> > On Fri, Oct 11, 2019 at 04:15:50PM +0800, Jason Wang wrote:
> > > There are hardware that can do virtio datapath offloading while having
> > > its own control path. This path tries to implement a mdev based
> > > unified API to support using kernel virtio driver to drive those
> > > devices. This is done by introducing a new mdev transport for virtio
> > > (virtio_mdev) and register itself as a new kind of mdev driver. Then
> > > it provides a unified way for kernel virtio driver to talk with mdev
> > > device implementation.
> > > 
> > > Though the series only contains kernel driver support, the goal is to
> > > make the transport generic enough to support userspace drivers. This
> > > means vhost-mdev[1] could be built on top as well by resuing the
> > > transport.
> > > 
> > > A sample driver is also implemented which simulate a virito-net
> > > loopback ethernet device on top of vringh + workqueue. This could be
> > > used as a reference implementation for real hardware driver.
> > > 
> > > Consider mdev framework only support VFIO device and driver right now,
> > > this series also extend it to support other types. This is done
> > > through introducing class id to the device and pairing it with
> > > id_talbe claimed by the driver. On top, this seris also decouple
> > > device specific parents ops out of the common ones.
> > I was curious so I took a quick look and posted comments.
> > 
> > I guess this driver runs inside the guest since it registers virtio
> > devices?
> 
> 
> It could run in either guest or host. But the main focus is to run in the
> host then we can use virtio drivers in containers.
> 
> 
> > 
> > If this is used with physical PCI devices that support datapath
> > offloading then how are physical devices presented to the guest without
> > SR-IOV?
> 
> 
> We will do control path meditation through vhost-mdev[1] and vhost-vfio[2].
> Then we will present a full virtio compatible ethernet device for guest.
> 
> SR-IOV is not a must, any mdev device that implements the API defined in
> patch 5 can be used by this framework.

What I'm trying to understand is: if you want to present a virtio-pci
device to the guest (e.g. using vhost-mdev or vhost-vfio), then how is
that related to this patch series?

Does this mean this patch series is useful mostly for presenting virtio
devices to containers or the host?

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 0/7] mdev based hardware virtio offloading support

[Jason Wang]

On 2019/10/15 下午10:37, Stefan Hajnoczi wrote:
> On Tue, Oct 15, 2019 at 11:37:17AM +0800, Jason Wang wrote:
>> On 2019/10/15 上午1:49, Stefan Hajnoczi wrote:
>>> On Fri, Oct 11, 2019 at 04:15:50PM +0800, Jason Wang wrote:
>>>> There are hardware that can do virtio datapath offloading while having
>>>> its own control path. This path tries to implement a mdev based
>>>> unified API to support using kernel virtio driver to drive those
>>>> devices. This is done by introducing a new mdev transport for virtio
>>>> (virtio_mdev) and register itself as a new kind of mdev driver. Then
>>>> it provides a unified way for kernel virtio driver to talk with mdev
>>>> device implementation.
>>>>
>>>> Though the series only contains kernel driver support, the goal is to
>>>> make the transport generic enough to support userspace drivers. This
>>>> means vhost-mdev[1] could be built on top as well by resuing the
>>>> transport.
>>>>
>>>> A sample driver is also implemented which simulate a virito-net
>>>> loopback ethernet device on top of vringh + workqueue. This could be
>>>> used as a reference implementation for real hardware driver.
>>>>
>>>> Consider mdev framework only support VFIO device and driver right now,
>>>> this series also extend it to support other types. This is done
>>>> through introducing class id to the device and pairing it with
>>>> id_talbe claimed by the driver. On top, this seris also decouple
>>>> device specific parents ops out of the common ones.
>>> I was curious so I took a quick look and posted comments.
>>>
>>> I guess this driver runs inside the guest since it registers virtio
>>> devices?
>>
>> It could run in either guest or host. But the main focus is to run in the
>> host then we can use virtio drivers in containers.
>>
>>
>>> If this is used with physical PCI devices that support datapath
>>> offloading then how are physical devices presented to the guest without
>>> SR-IOV?
>>
>> We will do control path meditation through vhost-mdev[1] and vhost-vfio[2].
>> Then we will present a full virtio compatible ethernet device for guest.
>>
>> SR-IOV is not a must, any mdev device that implements the API defined in
>> patch 5 can be used by this framework.
> What I'm trying to understand is: if you want to present a virtio-pci
> device to the guest (e.g. using vhost-mdev or vhost-vfio), then how is
> that related to this patch series?


This series introduce some infrastructure that would be used by vhost-mdev:

1) allow new type of mdev devices/drivers other than vfio (through 
class_id and device ops)

2) a set of virtio specific callbacks that will be used by both 
vhost-mdev and virtio-mdev defined in patch 5

Then vhost-mdev can be implemented on top: a new mdev class id but reuse 
the callback defined in 2. Through this way the parent can provides a 
single set of callbacks (device ops) for both kernel virtio driver 
(through virtio-mdev) or userspace virtio driver (through vhost-mdev).


>
> Does this mean this patch series is useful mostly for presenting virtio
> devices to containers or the host?


Patch 6 is mainly for bare metal or container use case, through it could 
be used in guest as well. Patch 7 is a sample virtio mdev device 
implementation. Patch 1 - 5 was the infrastructure for implementing 
types other than vfio, the first user is virito-mdev, then Tiwei's 
vhost-mdev and Parav's mlx5 mdev.

Thanks


>
> Stefan
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 0/7] mdev based hardware virtio offloading support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 3357 bytes --]

On Thu, Oct 17, 2019 at 09:42:53AM +0800, Jason Wang wrote:
> 
> On 2019/10/15 下午10:37, Stefan Hajnoczi wrote:
> > On Tue, Oct 15, 2019 at 11:37:17AM +0800, Jason Wang wrote:
> > > On 2019/10/15 上午1:49, Stefan Hajnoczi wrote:
> > > > On Fri, Oct 11, 2019 at 04:15:50PM +0800, Jason Wang wrote:
> > > > > There are hardware that can do virtio datapath offloading while having
> > > > > its own control path. This path tries to implement a mdev based
> > > > > unified API to support using kernel virtio driver to drive those
> > > > > devices. This is done by introducing a new mdev transport for virtio
> > > > > (virtio_mdev) and register itself as a new kind of mdev driver. Then
> > > > > it provides a unified way for kernel virtio driver to talk with mdev
> > > > > device implementation.
> > > > > 
> > > > > Though the series only contains kernel driver support, the goal is to
> > > > > make the transport generic enough to support userspace drivers. This
> > > > > means vhost-mdev[1] could be built on top as well by resuing the
> > > > > transport.
> > > > > 
> > > > > A sample driver is also implemented which simulate a virito-net
> > > > > loopback ethernet device on top of vringh + workqueue. This could be
> > > > > used as a reference implementation for real hardware driver.
> > > > > 
> > > > > Consider mdev framework only support VFIO device and driver right now,
> > > > > this series also extend it to support other types. This is done
> > > > > through introducing class id to the device and pairing it with
> > > > > id_talbe claimed by the driver. On top, this seris also decouple
> > > > > device specific parents ops out of the common ones.
> > > > I was curious so I took a quick look and posted comments.
> > > > 
> > > > I guess this driver runs inside the guest since it registers virtio
> > > > devices?
> > > 
> > > It could run in either guest or host. But the main focus is to run in the
> > > host then we can use virtio drivers in containers.
> > > 
> > > 
> > > > If this is used with physical PCI devices that support datapath
> > > > offloading then how are physical devices presented to the guest without
> > > > SR-IOV?
> > > 
> > > We will do control path meditation through vhost-mdev[1] and vhost-vfio[2].
> > > Then we will present a full virtio compatible ethernet device for guest.
> > > 
> > > SR-IOV is not a must, any mdev device that implements the API defined in
> > > patch 5 can be used by this framework.
> > What I'm trying to understand is: if you want to present a virtio-pci
> > device to the guest (e.g. using vhost-mdev or vhost-vfio), then how is
> > that related to this patch series?
> 
> 
> This series introduce some infrastructure that would be used by vhost-mdev:
> 
> 1) allow new type of mdev devices/drivers other than vfio (through class_id
> and device ops)
> 
> 2) a set of virtio specific callbacks that will be used by both vhost-mdev
> and virtio-mdev defined in patch 5
> 
> Then vhost-mdev can be implemented on top: a new mdev class id but reuse the
> callback defined in 2. Through this way the parent can provides a single set
> of callbacks (device ops) for both kernel virtio driver (through
> virtio-mdev) or userspace virtio driver (through vhost-mdev).

Okay, thanks for explaining!

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 2/7] mdev: bus uevent support

[Cornelia Huck]

On Fri, 11 Oct 2019 16:15:52 +0800
Jason Wang <jasowang@redhat.com> wrote:

> This patch adds bus uevent support for mdev bus in order to allow
> cooperation with userspace.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vfio/mdev/mdev_driver.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
> index b7c40ce86ee3..319d886ffaf7 100644
> --- a/drivers/vfio/mdev/mdev_driver.c
> +++ b/drivers/vfio/mdev/mdev_driver.c
> @@ -82,9 +82,17 @@ static int mdev_match(struct device *dev, struct device_driver *drv)
>  	return 0;
>  }
>  
> +static int mdev_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct mdev_device *mdev = to_mdev_device(dev);
> +
> +	return add_uevent_var(env, "MODALIAS=mdev:c%02X", mdev->class_id);
> +}
> +
>  struct bus_type mdev_bus_type = {
>  	.name		= "mdev",
>  	.match		= mdev_match,
> +	.uevent		= mdev_uevent,
>  	.probe		= mdev_probe,
>  	.remove		= mdev_remove,
>  };

I'd merge that into the previous patch.

Re: [PATCH V3 2/7] mdev: bus uevent support

[Jason Wang]

On 2019/10/15 下午6:27, Cornelia Huck wrote:
> On Fri, 11 Oct 2019 16:15:52 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> This patch adds bus uevent support for mdev bus in order to allow
>> cooperation with userspace.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/vfio/mdev/mdev_driver.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
>> index b7c40ce86ee3..319d886ffaf7 100644
>> --- a/drivers/vfio/mdev/mdev_driver.c
>> +++ b/drivers/vfio/mdev/mdev_driver.c
>> @@ -82,9 +82,17 @@ static int mdev_match(struct device *dev, struct device_driver *drv)
>>   	return 0;
>>   }
>>   
>> +static int mdev_uevent(struct device *dev, struct kobj_uevent_env *env)
>> +{
>> +	struct mdev_device *mdev = to_mdev_device(dev);
>> +
>> +	return add_uevent_var(env, "MODALIAS=mdev:c%02X", mdev->class_id);
>> +}
>> +
>>   struct bus_type mdev_bus_type = {
>>   	.name		= "mdev",
>>   	.match		= mdev_match,
>> +	.uevent		= mdev_uevent,
>>   	.probe		= mdev_probe,
>>   	.remove		= mdev_remove,
>>   };
> I'd merge that into the previous patch.


Will do.

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Cornelia Huck]

On Fri, 11 Oct 2019 16:15:54 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Currently, except for the create and remove, the rest of
> mdev_parent_ops is designed for vfio-mdev driver only and may not help
> for kernel mdev driver. With the help of class id, this patch
> introduces device specific callbacks inside mdev_device
> structure. This allows different set of callback to be used by
> vfio-mdev and virtio-mdev.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  .../driver-api/vfio-mediated-device.rst       | 22 +++++---
>  MAINTAINERS                                   |  1 +
>  drivers/gpu/drm/i915/gvt/kvmgt.c              | 18 ++++---
>  drivers/s390/cio/vfio_ccw_ops.c               | 18 ++++---
>  drivers/s390/crypto/vfio_ap_ops.c             | 14 +++--
>  drivers/vfio/mdev/mdev_core.c                 |  9 +++-
>  drivers/vfio/mdev/mdev_private.h              |  1 +
>  drivers/vfio/mdev/vfio_mdev.c                 | 37 ++++++-------
>  include/linux/mdev.h                          | 42 +++------------
>  include/linux/vfio_mdev.h                     | 52 +++++++++++++++++++
>  samples/vfio-mdev/mbochs.c                    | 20 ++++---
>  samples/vfio-mdev/mdpy.c                      | 21 +++++---
>  samples/vfio-mdev/mtty.c                      | 18 ++++---
>  13 files changed, 177 insertions(+), 96 deletions(-)
>  create mode 100644 include/linux/vfio_mdev.h
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index 2035e48da7b2..553574ebba73 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any other categorization.
>  Vendor drivers are expected to be fully asynchronous in this respect or
>  provide their own internal resource protection.)
>  
> -The callbacks in the mdev_parent_ops structure are as follows:
> +In order to support multiple types of device/driver, device needs to
> +provide both class_id and device_ops through:

"As multiple types of mediated devices may be supported, the device
needs to set up the class id and the device specific callbacks via:"

?

>  
> -* open: open callback of mediated device
> -* close: close callback of mediated device
> -* ioctl: ioctl callback of mediated device
> +    void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops);
> +
> +The class_id is used to be paired with ids in id_table in mdev_driver
> +structure for probing the correct driver.

"The class id  (specified in id) is used to match a device with an mdev
driver via its id table."

?

> The device_ops is device
> +specific callbacks which can be get through mdev_get_dev_ops()
> +function by mdev bus driver. 

"The device specific callbacks (specified in *ops) are obtainable via
mdev_get_dev_ops() (for use by the mdev bus driver.)"

?

> For vfio-mdev device, its device specific
> +ops are as follows:

"A vfio-mdev device (class id MDEV_ID_VFIO) uses the following
device-specific ops:"

?

> +
> +* open: open callback of vfio mediated device
> +* close: close callback of vfio mediated device
> +* ioctl: ioctl callback of vfio mediated device
>  * read : read emulation callback
>  * write: write emulation callback
>  * mmap: mmap emulation callback
> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
>  	extern int  mdev_register_device(struct device *dev,
>  	                                 const struct mdev_parent_ops *ops);
>  
> -It is also required to specify the class_id through::
> +It is also required to specify the class_id and device specific ops through::
>  
> -	extern int mdev_set_class(struct device *dev, u16 id);
> +	extern int mdev_set_class(struct device *dev, u16 id,
> +	                          const void *ops);

Apologies if that has already been discussed, but do we want a 1:1
relationship between id and ops, or can different devices with the same
id register different ops? If the former, would it make sense to first
register the ops for an id (once), and then have the ->create callback
only set the class id (with the core doing the lookup of the ops)?

>  
>  However, the mdev_parent_ops structure is not required in the function call
>  that a driver should use to unregister itself with the mdev core driver::

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Jason Wang]

On 2019/10/15 下午6:41, Cornelia Huck wrote:
> On Fri, 11 Oct 2019 16:15:54 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Currently, except for the create and remove, the rest of
>> mdev_parent_ops is designed for vfio-mdev driver only and may not help
>> for kernel mdev driver. With the help of class id, this patch
>> introduces device specific callbacks inside mdev_device
>> structure. This allows different set of callback to be used by
>> vfio-mdev and virtio-mdev.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   .../driver-api/vfio-mediated-device.rst       | 22 +++++---
>>   MAINTAINERS                                   |  1 +
>>   drivers/gpu/drm/i915/gvt/kvmgt.c              | 18 ++++---
>>   drivers/s390/cio/vfio_ccw_ops.c               | 18 ++++---
>>   drivers/s390/crypto/vfio_ap_ops.c             | 14 +++--
>>   drivers/vfio/mdev/mdev_core.c                 |  9 +++-
>>   drivers/vfio/mdev/mdev_private.h              |  1 +
>>   drivers/vfio/mdev/vfio_mdev.c                 | 37 ++++++-------
>>   include/linux/mdev.h                          | 42 +++------------
>>   include/linux/vfio_mdev.h                     | 52 +++++++++++++++++++
>>   samples/vfio-mdev/mbochs.c                    | 20 ++++---
>>   samples/vfio-mdev/mdpy.c                      | 21 +++++---
>>   samples/vfio-mdev/mtty.c                      | 18 ++++---
>>   13 files changed, 177 insertions(+), 96 deletions(-)
>>   create mode 100644 include/linux/vfio_mdev.h
>>
>> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
>> index 2035e48da7b2..553574ebba73 100644
>> --- a/Documentation/driver-api/vfio-mediated-device.rst
>> +++ b/Documentation/driver-api/vfio-mediated-device.rst
>> @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any other categorization.
>>   Vendor drivers are expected to be fully asynchronous in this respect or
>>   provide their own internal resource protection.)
>>   
>> -The callbacks in the mdev_parent_ops structure are as follows:
>> +In order to support multiple types of device/driver, device needs to
>> +provide both class_id and device_ops through:
> "As multiple types of mediated devices may be supported, the device
> needs to set up the class id and the device specific callbacks via:"
>
> ?
>
>>   
>> -* open: open callback of mediated device
>> -* close: close callback of mediated device
>> -* ioctl: ioctl callback of mediated device
>> +    void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops);
>> +
>> +The class_id is used to be paired with ids in id_table in mdev_driver
>> +structure for probing the correct driver.
> "The class id  (specified in id) is used to match a device with an mdev
> driver via its id table."
>
> ?
>
>> The device_ops is device
>> +specific callbacks which can be get through mdev_get_dev_ops()
>> +function by mdev bus driver.
> "The device specific callbacks (specified in *ops) are obtainable via
> mdev_get_dev_ops() (for use by the mdev bus driver.)"
>
> ?
>
>> For vfio-mdev device, its device specific
>> +ops are as follows:
> "A vfio-mdev device (class id MDEV_ID_VFIO) uses the following
> device-specific ops:"
>
> ?


All you propose is better than what I wrote, will change the docs.


>
>> +
>> +* open: open callback of vfio mediated device
>> +* close: close callback of vfio mediated device
>> +* ioctl: ioctl callback of vfio mediated device
>>   * read : read emulation callback
>>   * write: write emulation callback
>>   * mmap: mmap emulation callback
>> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
>>   	extern int  mdev_register_device(struct device *dev,
>>   	                                 const struct mdev_parent_ops *ops);
>>   
>> -It is also required to specify the class_id through::
>> +It is also required to specify the class_id and device specific ops through::
>>   
>> -	extern int mdev_set_class(struct device *dev, u16 id);
>> +	extern int mdev_set_class(struct device *dev, u16 id,
>> +	                          const void *ops);
> Apologies if that has already been discussed, but do we want a 1:1
> relationship between id and ops, or can different devices with the same
> id register different ops?


I think we have a N:1 mapping between id and ops, e.g we want both 
virtio-mdev and vhost-mdev use a single set of device ops.

Thanks


> If the former, would it make sense to first
> register the ops for an id (once), and then have the ->create callback
> only set the class id (with the core doing the lookup of the ops)?
>
>>   
>>   However, the mdev_parent_ops structure is not required in the function call
>>   that a driver should use to unregister itself with the mdev core driver::
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Alex Williamson]

On Tue, 15 Oct 2019 20:17:01 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2019/10/15 下午6:41, Cornelia Huck wrote:
> > On Fri, 11 Oct 2019 16:15:54 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >  
> >> Currently, except for the create and remove, the rest of
> >> mdev_parent_ops is designed for vfio-mdev driver only and may not help
> >> for kernel mdev driver. With the help of class id, this patch
> >> introduces device specific callbacks inside mdev_device
> >> structure. This allows different set of callback to be used by
> >> vfio-mdev and virtio-mdev.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>   .../driver-api/vfio-mediated-device.rst       | 22 +++++---
> >>   MAINTAINERS                                   |  1 +
> >>   drivers/gpu/drm/i915/gvt/kvmgt.c              | 18 ++++---
> >>   drivers/s390/cio/vfio_ccw_ops.c               | 18 ++++---
> >>   drivers/s390/crypto/vfio_ap_ops.c             | 14 +++--
> >>   drivers/vfio/mdev/mdev_core.c                 |  9 +++-
> >>   drivers/vfio/mdev/mdev_private.h              |  1 +
> >>   drivers/vfio/mdev/vfio_mdev.c                 | 37 ++++++-------
> >>   include/linux/mdev.h                          | 42 +++------------
> >>   include/linux/vfio_mdev.h                     | 52 +++++++++++++++++++
> >>   samples/vfio-mdev/mbochs.c                    | 20 ++++---
> >>   samples/vfio-mdev/mdpy.c                      | 21 +++++---
> >>   samples/vfio-mdev/mtty.c                      | 18 ++++---
> >>   13 files changed, 177 insertions(+), 96 deletions(-)
> >>   create mode 100644 include/linux/vfio_mdev.h
> >>
> >> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> >> index 2035e48da7b2..553574ebba73 100644
> >> --- a/Documentation/driver-api/vfio-mediated-device.rst
> >> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> >> @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any other categorization.
> >>   Vendor drivers are expected to be fully asynchronous in this respect or
> >>   provide their own internal resource protection.)
> >>   
> >> -The callbacks in the mdev_parent_ops structure are as follows:
> >> +In order to support multiple types of device/driver, device needs to
> >> +provide both class_id and device_ops through:  
> > "As multiple types of mediated devices may be supported, the device
> > needs to set up the class id and the device specific callbacks via:"
> >
> > ?
> >  
> >>   
> >> -* open: open callback of mediated device
> >> -* close: close callback of mediated device
> >> -* ioctl: ioctl callback of mediated device
> >> +    void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops);
> >> +
> >> +The class_id is used to be paired with ids in id_table in mdev_driver
> >> +structure for probing the correct driver.  
> > "The class id  (specified in id) is used to match a device with an mdev
> > driver via its id table."
> >
> > ?
> >  
> >> The device_ops is device
> >> +specific callbacks which can be get through mdev_get_dev_ops()
> >> +function by mdev bus driver.  
> > "The device specific callbacks (specified in *ops) are obtainable via
> > mdev_get_dev_ops() (for use by the mdev bus driver.)"
> >
> > ?
> >  
> >> For vfio-mdev device, its device specific
> >> +ops are as follows:  
> > "A vfio-mdev device (class id MDEV_ID_VFIO) uses the following
> > device-specific ops:"
> >
> > ?  
> 
> 
> All you propose is better than what I wrote, will change the docs.
> 
> 
> >  
> >> +
> >> +* open: open callback of vfio mediated device
> >> +* close: close callback of vfio mediated device
> >> +* ioctl: ioctl callback of vfio mediated device
> >>   * read : read emulation callback
> >>   * write: write emulation callback
> >>   * mmap: mmap emulation callback
> >> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
> >>   	extern int  mdev_register_device(struct device *dev,
> >>   	                                 const struct mdev_parent_ops *ops);
> >>   
> >> -It is also required to specify the class_id through::
> >> +It is also required to specify the class_id and device specific ops through::
> >>   
> >> -	extern int mdev_set_class(struct device *dev, u16 id);
> >> +	extern int mdev_set_class(struct device *dev, u16 id,
> >> +	                          const void *ops);  
> > Apologies if that has already been discussed, but do we want a 1:1
> > relationship between id and ops, or can different devices with the same
> > id register different ops?  
> 
> 
> I think we have a N:1 mapping between id and ops, e.g we want both 
> virtio-mdev and vhost-mdev use a single set of device ops.

The contents of the ops structure is essentially defined by the id,
which is why I was leaning towards them being defined together.  They
are effectively interlocked, the id defines which mdev "endpoint"
driver is loaded and that driver requires mdev_get_dev_ops() to return
the structure required by the driver.  I wish there was a way we could
incorporate type checking here.  We toyed with the idea of having the
class in the same structure as the ops, but I think this approach was
chosen for simplicity.  We could still do something like:

int mdev_set_class_struct(struct device *dev, const struct mdev_class_struct *class);

struct mdev_class_struct {
	u16	id;
	union {
		struct vfio_mdev_ops vfio_ops;
		struct virtio_mdev_ops virtio_ops;
	};
};

Maybe even:

struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev) {
	BUG_ON(mdev->class.id != MDEV_ID_VFIO);
	return &mdev->class.vfio_ops;
}

The match callback would of course just use the mdev->class.id value.
Functionally equivalent, but maybe better type characteristics.  Thanks,

Alex
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Cornelia Huck]

On Wed, 16 Oct 2019 05:50:08 +0000
Parav Pandit <parav@mellanox.com> wrote:

> Hi Alex,
> 
> > -----Original Message-----
> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Tuesday, October 15, 2019 12:27 PM
> > To: Jason Wang <jasowang@redhat.com>
> > Cc: Cornelia Huck <cohuck@redhat.com>; kvm@vger.kernel.org; linux-
> > s390@vger.kernel.org; linux-kernel@vger.kernel.org; dri-
> > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-gvt-
> > dev@lists.freedesktop.org; kwankhede@nvidia.com; mst@redhat.com;
> > tiwei.bie@intel.com; virtualization@lists.linux-foundation.org;
> > netdev@vger.kernel.org; maxime.coquelin@redhat.com;
> > cunming.liang@intel.com; zhihong.wang@intel.com;
> > rob.miller@broadcom.com; xiao.w.wang@intel.com;
> > haotian.wang@sifive.com; zhenyuw@linux.intel.com; zhi.a.wang@intel.com;
> > jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com;
> > rodrigo.vivi@intel.com; airlied@linux.ie; daniel@ffwll.ch;
> > farman@linux.ibm.com; pasic@linux.ibm.com; sebott@linux.ibm.com;
> > oberpar@linux.ibm.com; heiko.carstens@de.ibm.com; gor@linux.ibm.com;
> > borntraeger@de.ibm.com; akrowiak@linux.ibm.com; freude@linux.ibm.com;
> > lingshan.zhu@intel.com; Ido Shamay <idos@mellanox.com>;
> > eperezma@redhat.com; lulu@redhat.com; Parav Pandit
> > <parav@mellanox.com>; christophe.de.dinechin@gmail.com;
> > kevin.tian@intel.com
> > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> > 
> > On Tue, 15 Oct 2019 20:17:01 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >   
> > > On 2019/10/15 下午6:41, Cornelia Huck wrote:  
> > > > On Fri, 11 Oct 2019 16:15:54 +0800
> > > > Jason Wang <jasowang@redhat.com> wrote:

> > > >> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
> > > >>   	extern int  mdev_register_device(struct device *dev,
> > > >>   	                                 const struct mdev_parent_ops
> > > >> *ops);
> > > >>
> > > >> -It is also required to specify the class_id through::
> > > >> +It is also required to specify the class_id and device specific ops  
> > through::  
> > > >>
> > > >> -	extern int mdev_set_class(struct device *dev, u16 id);
> > > >> +	extern int mdev_set_class(struct device *dev, u16 id,
> > > >> +	                          const void *ops);  
> > > > Apologies if that has already been discussed, but do we want a 1:1
> > > > relationship between id and ops, or can different devices with the
> > > > same id register different ops?  
> > >
> > >
> > > I think we have a N:1 mapping between id and ops, e.g we want both
> > > virtio-mdev and vhost-mdev use a single set of device ops.  
> > 
> > The contents of the ops structure is essentially defined by the id, which is
> > why I was leaning towards them being defined together.  They are effectively
> > interlocked, the id defines which mdev "endpoint"
> > driver is loaded and that driver requires mdev_get_dev_ops() to return the
> > structure required by the driver.  I wish there was a way we could
> > incorporate type checking here.  We toyed with the idea of having the class
> > in the same structure as the ops, but I think this approach was chosen for
> > simplicity.  We could still do something like:
> > 
> > int mdev_set_class_struct(struct device *dev, const struct mdev_class_struct
> > *class);
> > 
> > struct mdev_class_struct {
> > 	u16	id;
> > 	union {
> > 		struct vfio_mdev_ops vfio_ops;
> > 		struct virtio_mdev_ops virtio_ops;
> > 	};
> > };
> > 
> > Maybe even:
> > 
> > struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev) {
> > 	BUG_ON(mdev->class.id != MDEV_ID_VFIO);
> > 	return &mdev->class.vfio_ops;
> > }
> > 
> > The match callback would of course just use the mdev->class.id value.
> > Functionally equivalent, but maybe better type characteristics.  Thanks,
> > 
> > Alex  
> 
> We have 3 use cases of mdev.
> 1. current mdev binding to vfio_mdev
> 2. mdev binding to virtio
> 3. mdev binding to mlx5_core without dev_ops
> 
> Also 
> (a) a given parent may serve multiple types of classes in future.
> (b) number of classes may not likely explode, they will be handful of them. (vfio_mdev, virtio)
> 
> So, instead of making copies of this dev_ops pointer in each mdev, it is better to keep const multiple ops in their parent device.
> Something like below,
> 
> struct mdev_parent {
> 	[..]
> 	struct mdev_parent_ops *parent_ops; /* create, remove */
> 	struct vfio_mdev_ops *vfio_ops; /* read,write, ioctl etc */
> 	struct virtio_mdev_ops *virtio_ops; /* virtio ops */
> };

That feels a bit odd. Why should the parent carry pointers to every
possible version of ops?

> 
> const struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_parent *parent);
> const struct virtio_mdev_ops *mdev_get_virtio_ops(struct mdev_parent *parent);
> 
> This way, 
> (a) we have strong type check support
> (b) ops pointer is not duplicated across several hundred mdev devices, and don't have to set on every mdev creation
> (c) all 3 classes of mdev are supported
> (d) one extra symbol table entry used per ops type, but there are not expected to grow a lot.
> (e) multiple classes per single parent is still supported
> (f) still extendible for multiple classes (well defined classes = vfio, virtio, and vendor class)

Yet another suggestion: have the class id derive from the function you
use to set up the ops.

void mdev_set_vfio_ops(struct mdev_device *mdev, const struct vfio_mdev_ops *vfio_ops)
{
	mdev->device_ops = vfio_ops;
	mdev->class_id = MDEV_ID_VFIO;
}

void mdev_set_virtio_ops(struct mdev_device *mdev, const struct virtio_mdev_ops *virtio_ops)
{
	mdev->device_ops = virtio_ops;
	mdev->class_id = MDEV_ID_VIRTIO;
}

void mdev_set_vhost_ops(struct mdev_device *mdev, const struct virtio_mdev_ops *virtio_ops)
{
	mdev->device_ops = virtio_ops;
	mdev->class_id = MDEV_ID_VHOST;
}

void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */
{
	mdev->class_id = MDEV_ID_VENDOR;
}
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Alex Williamson]

On Wed, 16 Oct 2019 15:31:25 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > -----Original Message-----
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Wednesday, October 16, 2019 3:53 AM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Alex Williamson <alex.williamson@redhat.com>; Jason Wang
> > <jasowang@redhat.com>; kvm@vger.kernel.org; linux-s390@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org;
> > kwankhede@nvidia.com; mst@redhat.com; tiwei.bie@intel.com;
> > virtualization@lists.linux-foundation.org; netdev@vger.kernel.org;
> > maxime.coquelin@redhat.com; cunming.liang@intel.com;
> > zhihong.wang@intel.com; rob.miller@broadcom.com; xiao.w.wang@intel.com;
> > haotian.wang@sifive.com; zhenyuw@linux.intel.com; zhi.a.wang@intel.com;
> > jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com;
> > rodrigo.vivi@intel.com; airlied@linux.ie; daniel@ffwll.ch;
> > farman@linux.ibm.com; pasic@linux.ibm.com; sebott@linux.ibm.com;
> > oberpar@linux.ibm.com; heiko.carstens@de.ibm.com; gor@linux.ibm.com;
> > borntraeger@de.ibm.com; akrowiak@linux.ibm.com; freude@linux.ibm.com;
> > lingshan.zhu@intel.com; Ido Shamay <idos@mellanox.com>;
> > eperezma@redhat.com; lulu@redhat.com; christophe.de.dinechin@gmail.com;
> > kevin.tian@intel.com
> > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> > 
> > On Wed, 16 Oct 2019 05:50:08 +0000
> > Parav Pandit <parav@mellanox.com> wrote:
> >   
> > > Hi Alex,
> > >  
> > > > -----Original Message-----
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > Sent: Tuesday, October 15, 2019 12:27 PM
> > > > To: Jason Wang <jasowang@redhat.com>
> > > > Cc: Cornelia Huck <cohuck@redhat.com>; kvm@vger.kernel.org; linux-
> > > > s390@vger.kernel.org; linux-kernel@vger.kernel.org; dri-
> > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
> > > > intel-gvt- dev@lists.freedesktop.org; kwankhede@nvidia.com;
> > > > mst@redhat.com; tiwei.bie@intel.com;
> > > > virtualization@lists.linux-foundation.org;
> > > > netdev@vger.kernel.org; maxime.coquelin@redhat.com;
> > > > cunming.liang@intel.com; zhihong.wang@intel.com;
> > > > rob.miller@broadcom.com; xiao.w.wang@intel.com;
> > > > haotian.wang@sifive.com; zhenyuw@linux.intel.com;
> > > > zhi.a.wang@intel.com; jani.nikula@linux.intel.com;
> > > > joonas.lahtinen@linux.intel.com; rodrigo.vivi@intel.com;
> > > > airlied@linux.ie; daniel@ffwll.ch; farman@linux.ibm.com;
> > > > pasic@linux.ibm.com; sebott@linux.ibm.com; oberpar@linux.ibm.com;
> > > > heiko.carstens@de.ibm.com; gor@linux.ibm.com;
> > > > borntraeger@de.ibm.com; akrowiak@linux.ibm.com;
> > > > freude@linux.ibm.com; lingshan.zhu@intel.com; Ido Shamay
> > > > <idos@mellanox.com>; eperezma@redhat.com; lulu@redhat.com; Parav
> > > > Pandit <parav@mellanox.com>; christophe.de.dinechin@gmail.com;
> > > > kevin.tian@intel.com
> > > > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> > > >
> > > > On Tue, 15 Oct 2019 20:17:01 +0800
> > > > Jason Wang <jasowang@redhat.com> wrote:
> > > >  
> > > > > On 2019/10/15 下午6:41, Cornelia Huck wrote:  
> > > > > > On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang
> > > > > > <jasowang@redhat.com> wrote:  
> >   
> > > > > >> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
> > > > > >>   	extern int  mdev_register_device(struct device *dev,
> > > > > >>   	                                 const struct
> > > > > >> mdev_parent_ops *ops);
> > > > > >>
> > > > > >> -It is also required to specify the class_id through::
> > > > > >> +It is also required to specify the class_id and device
> > > > > >> +specific ops  
> > > > through::  
> > > > > >>
> > > > > >> -	extern int mdev_set_class(struct device *dev, u16 id);
> > > > > >> +	extern int mdev_set_class(struct device *dev, u16 id,
> > > > > >> +	                          const void *ops);  
> > > > > > Apologies if that has already been discussed, but do we want a
> > > > > > 1:1 relationship between id and ops, or can different devices
> > > > > > with the same id register different ops?  
> > > > >
> > > > >
> > > > > I think we have a N:1 mapping between id and ops, e.g we want both
> > > > > virtio-mdev and vhost-mdev use a single set of device ops.  
> > > >
> > > > The contents of the ops structure is essentially defined by the id,
> > > > which is why I was leaning towards them being defined together.
> > > > They are effectively interlocked, the id defines which mdev "endpoint"
> > > > driver is loaded and that driver requires mdev_get_dev_ops() to
> > > > return the structure required by the driver.  I wish there was a way
> > > > we could incorporate type checking here.  We toyed with the idea of
> > > > having the class in the same structure as the ops, but I think this
> > > > approach was chosen for simplicity.  We could still do something like:
> > > >
> > > > int mdev_set_class_struct(struct device *dev, const struct
> > > > mdev_class_struct *class);
> > > >
> > > > struct mdev_class_struct {
> > > > 	u16	id;
> > > > 	union {
> > > > 		struct vfio_mdev_ops vfio_ops;
> > > > 		struct virtio_mdev_ops virtio_ops;
> > > > 	};
> > > > };
> > > >
> > > > Maybe even:
> > > >
> > > > struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev) {
> > > > 	BUG_ON(mdev->class.id != MDEV_ID_VFIO);
> > > > 	return &mdev->class.vfio_ops;
> > > > }
> > > >
> > > > The match callback would of course just use the mdev->class.id value.
> > > > Functionally equivalent, but maybe better type characteristics.
> > > > Thanks,
> > > >
> > > > Alex  
> > >
> > > We have 3 use cases of mdev.
> > > 1. current mdev binding to vfio_mdev
> > > 2. mdev binding to virtio
> > > 3. mdev binding to mlx5_core without dev_ops
> > >
> > > Also
> > > (a) a given parent may serve multiple types of classes in future.
> > > (b) number of classes may not likely explode, they will be handful of
> > > them. (vfio_mdev, virtio)
> > >
> > > So, instead of making copies of this dev_ops pointer in each mdev, it is better  
> > to keep const multiple ops in their parent device.  
> > > Something like below,
> > >
> > > struct mdev_parent {
> > > 	[..]
> > > 	struct mdev_parent_ops *parent_ops; /* create, remove */
> > > 	struct vfio_mdev_ops *vfio_ops; /* read,write, ioctl etc */
> > > 	struct virtio_mdev_ops *virtio_ops; /* virtio ops */ };  
> > 
> > That feels a bit odd. Why should the parent carry pointers to every possible
> > version of ops?
> >   
> How many are we expecting? I envisioned handful of them.
> It carries because parent is few, mdevs are several hundreds.
> It makes sense to keep few copies, instead of several hundred copies
> and it doesn't need to setup on every mdev creation.

It does need setup on every mdev creation, it's just a matter of the
scope, 'id and ops' vs 'id only' vs 'ops with implicit id'.  The other
argument is assuming a space vs time trade-off that I'm having a hard
time judging is necessarily the correct approach.  We potentially have
better data locality in the mdev device structure vs the parent.  The
caching of the ops structure itself is separate from how we get to it.
We might have hundreds of pointers to those ops structure, but the
space trade-off might we worth it if they're on the same cacheline as
the mdev device itself vs the indirection via the parent.

I see a couple other drawbacks to the parent hosted ops pointers as
well.  First, it imposes that per parent there can only be one device
ops structure per class id, but who's to say that different types of
mdev devices for a given parent all make the same callbacks into the
parent.  For instance, for a vfio-mdev we already support the concept
of an iommu backing device which makes the type1 iommu code behave a
little differently.  Those differences might be sufficient that the
parent driver would register a different device ops structure for an
iommu backed mdev vs a non-iommu backed device.  The other drawback is
that it implies a binary difference in all mdev parent drivers to add
any new device ids.  I know we don't guarantee binary compatibility,
but it's rather ugly.

Overall, I guess I tend to prefer Connie's proposal, the class id and
structure are tied together and the parent driver is only responsible
for one of them, the class id is hidden away in mdev-core and the mdev
driver itself.

> > > const struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_parent
> > > *parent); const struct virtio_mdev_ops *mdev_get_virtio_ops(struct
> > > mdev_parent *parent);
> > >
> > > This way,
> > > (a) we have strong type check support
> > > (b) ops pointer is not duplicated across several hundred mdev
> > > devices, and don't have to set on every mdev creation
> > > (c) all 3 classes of mdev are supported
> > > (d) one extra symbol table entry used per ops type, but there are
> > > not  
> > expected to grow a lot.  
> > > (e) multiple classes per single parent is still supported
> > > (f) still extendible for multiple classes (well defined classes =
> > > vfio, virtio, and vendor class)  
> > 
> > Yet another suggestion: have the class id derive from the function
> > you use to set up the ops.
> > 
> > void mdev_set_vfio_ops(struct mdev_device *mdev, const struct
> > vfio_mdev_ops *vfio_ops) {
> > 	mdev->device_ops = vfio_ops;
> > 	mdev->class_id = MDEV_ID_VFIO;
> > }
> > 
> > void mdev_set_virtio_ops(struct mdev_device *mdev, const struct
> > virtio_mdev_ops *virtio_ops) {
> > 	mdev->device_ops = virtio_ops;
> > 	mdev->class_id = MDEV_ID_VIRTIO;
> > }
> > 
> > void mdev_set_vhost_ops(struct mdev_device *mdev, const struct
> > virtio_mdev_ops *virtio_ops) {
> > 	mdev->device_ops = virtio_ops;
> > 	mdev->class_id = MDEV_ID_VHOST;
> > }
> > 
> > void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */ {
> > 	mdev->class_id = MDEV_ID_VENDOR;
> > }  

One further step towards making this hard to use incorrectly might be
to return an error if class_id is already set.  Thanks,

Alex
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Alex Williamson]

On Wed, 16 Oct 2019 20:48:06 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > From: Alex Williamson <alex.williamson@redhat.com>
> > On Wed, 16 Oct 2019 15:31:25 +0000
> > Parav Pandit <parav@mellanox.com> wrote:
> > > > From: Cornelia Huck <cohuck@redhat.com>
> > > > Parav Pandit <parav@mellanox.com> wrote:
> > > > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > > > On Tue, 15 Oct 2019 20:17:01 +0800 Jason Wang
> > > > > > <jasowang@redhat.com> wrote:
> > > > > >  
> > > > > > > On 2019/10/15 下午6:41, Cornelia Huck wrote:  
> > > > > > > > Apologies if that has already been discussed, but do we want
> > > > > > > > a
> > > > > > > > 1:1 relationship between id and ops, or can different
> > > > > > > > devices with the same id register different ops?  
> > > > > > >
> > > > > > >
> > > > > > > I think we have a N:1 mapping between id and ops, e.g we want
> > > > > > > both virtio-mdev and vhost-mdev use a single set of device ops.  
> > > > > >
> > > > > > The contents of the ops structure is essentially defined by the
> > > > > > id, which is why I was leaning towards them being defined together.
> > > > > > They are effectively interlocked, the id defines which mdev "endpoint"
> > > > > > driver is loaded and that driver requires mdev_get_dev_ops() to
> > > > > > return the structure required by the driver.  I wish there was a
> > > > > > way we could incorporate type checking here.  We toyed with the
> > > > > > idea of having the class in the same structure as the ops, but I
> > > > > > think this approach was chosen for simplicity.  We could still do  
> > something like:  
> > > > > >
> > > > > > int mdev_set_class_struct(struct device *dev, const struct
> > > > > > mdev_class_struct *class);
> > > > > >
> > > > > > struct mdev_class_struct {
> > > > > > 	u16	id;
> > > > > > 	union {
> > > > > > 		struct vfio_mdev_ops vfio_ops;
> > > > > > 		struct virtio_mdev_ops virtio_ops;
> > > > > > 	};
> > > > > > };
> > > > > >
> > > > > > Maybe even:
> > > > > >
> > > > > > struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev)  
> > {  
> > > > > > 	BUG_ON(mdev->class.id != MDEV_ID_VFIO);
> > > > > > 	return &mdev->class.vfio_ops;
> > > > > > }
> > > > > >
> > > > > > The match callback would of course just use the mdev->class.id value.
> > > > > > Functionally equivalent, but maybe better type characteristics.
> > > > > > Thanks,
> > > > > >
> > > > > > Alex  
> > > > >
> > > > > We have 3 use cases of mdev.
> > > > > 1. current mdev binding to vfio_mdev 2. mdev binding to virtio 3.
> > > > > mdev binding to mlx5_core without dev_ops
> > > > >
> > > > > Also
> > > > > (a) a given parent may serve multiple types of classes in future.
> > > > > (b) number of classes may not likely explode, they will be handful
> > > > > of them. (vfio_mdev, virtio)
> > > > >
> > > > > So, instead of making copies of this dev_ops pointer in each mdev,
> > > > > it is better  
> > > > to keep const multiple ops in their parent device.  
> > > > > Something like below,
> > > > >
> > > > > struct mdev_parent {
> > > > > 	[..]
> > > > > 	struct mdev_parent_ops *parent_ops; /* create, remove */
> > > > > 	struct vfio_mdev_ops *vfio_ops; /* read,write, ioctl etc */
> > > > > 	struct virtio_mdev_ops *virtio_ops; /* virtio ops */ };  
> > > >
> > > > That feels a bit odd. Why should the parent carry pointers to every
> > > > possible version of ops?
> > > >  
> > > How many are we expecting? I envisioned handful of them.
> > > It carries because parent is few, mdevs are several hundreds.
> > > It makes sense to keep few copies, instead of several hundred copies
> > > and it doesn't need to setup on every mdev creation.  
> > 
> > It does need setup on every mdev creation, it's just a matter of the scope, 'id
> > and ops' vs 'id only' vs 'ops with implicit id'.  The other argument is assuming a
> > space vs time trade-off that I'm having a hard time judging is necessarily the
> > correct approach.  We potentially have better data locality in the mdev device
> > structure vs the parent.  The caching of the ops structure itself is separate from
> > how we get to it.
> > We might have hundreds of pointers to those ops structure, but the space
> > trade-off might we worth it if they're on the same cacheline as the mdev
> > device itself vs the indirection via the parent.
> > 
> > I see a couple other drawbacks to the parent hosted ops pointers as well.  First,
> > it imposes that per parent there can only be one device ops structure per class
> > id, but who's to say that different types of mdev devices for a given parent all
> > make the same callbacks into the parent.   
> We should have driver who intent to use different device ops for each
> device with single parent that supports this claim.

Why?  Are we not allowed to identify restrictions implied by a given
proposal if we don't yet have a user?  I can't subscribe to that.

>  For instance, for a vfio-mdev we
> > already support the concept of an iommu backing device which makes
> > the type1 iommu code behave a little differently.  Those
> > differences might be sufficient that the parent driver would
> > register a different device ops structure for an iommu backed mdev
> > vs a non-iommu backed device.    
> I am not sure if this is really worth it.
> Which driver should I look which has if-else conditions sprinkled in
> these callbacks for different iommu types? If majority code is same,
> adding few branches looks ok vs creating new ops all together. So I
> need to educate myself first with the driver which desires this. Any
> pointers?

While the iommu backed vfio-mdevs is real, the example that a parent
driver might choose to register different device ops based on that is
theoretical.  Parent drivers don't have that option today, but as we're
making the device ops more modular and have stumbled onto this benefit
of per device ops, perhaps it might be useful.  The "is it worth it"
question can also be asked of the claimed benefits of a set of shared
devices ops per parent.
 
> > The other
> > drawback is that it implies a binary difference in all mdev parent
> > drivers to add any new device ids.  I know we don't guarantee
> > binary compatibility, but it's rather ugly.
> >   
> Yeah, we don't support and there is no requirement for binary
> compatibility.
> 
> > Overall, I guess I tend to prefer Connie's proposal, the class id
> > and structure are tied together and the parent driver is only
> > responsible for one of them, the class id is hidden away in
> > mdev-core and the mdev driver itself. 
> I am fine with Cornelia's approach.
> It comes with small cost of additional symbols and it is probably ok.
> I just find it over engineered given handful of dev ops types.

If the device ops types are limited, then so are the additional
symbols.  Those symbols also add a degree of explicitness to the
interface (ie. register this device as vfio-mdev with this set of
vfio-mdev-ops, versus register this device as vfio-mdev... which uses
the vfio-mdev-ops over in the parent ops structure).  I don't really
see what's over-engineered about former.  I like to think of Rusty's
old interface guidelines, particularly the one about making it
difficult to use incorrectly for these sorts of interfaces.  Thanks,

Alex
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Jason Wang]

On 2019/10/17 上午12:53, Alex Williamson wrote:
> On Wed, 16 Oct 2019 15:31:25 +0000
> Parav Pandit <parav@mellanox.com> wrote:
>
>>> -----Original Message-----
>>> From: Cornelia Huck <cohuck@redhat.com>
>>> Sent: Wednesday, October 16, 2019 3:53 AM
>>> To: Parav Pandit <parav@mellanox.com>
>>> Cc: Alex Williamson <alex.williamson@redhat.com>; Jason Wang
>>> <jasowang@redhat.com>; kvm@vger.kernel.org; linux-s390@vger.kernel.org;
>>> linux-kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; intel-
>>> gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org;
>>> kwankhede@nvidia.com; mst@redhat.com; tiwei.bie@intel.com;
>>> virtualization@lists.linux-foundation.org; netdev@vger.kernel.org;
>>> maxime.coquelin@redhat.com; cunming.liang@intel.com;
>>> zhihong.wang@intel.com; rob.miller@broadcom.com; xiao.w.wang@intel.com;
>>> haotian.wang@sifive.com; zhenyuw@linux.intel.com; zhi.a.wang@intel.com;
>>> jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com;
>>> rodrigo.vivi@intel.com; airlied@linux.ie; daniel@ffwll.ch;
>>> farman@linux.ibm.com; pasic@linux.ibm.com; sebott@linux.ibm.com;
>>> oberpar@linux.ibm.com; heiko.carstens@de.ibm.com; gor@linux.ibm.com;
>>> borntraeger@de.ibm.com; akrowiak@linux.ibm.com; freude@linux.ibm.com;
>>> lingshan.zhu@intel.com; Ido Shamay <idos@mellanox.com>;
>>> eperezma@redhat.com; lulu@redhat.com; christophe.de.dinechin@gmail.com;
>>> kevin.tian@intel.com
>>> Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
>>>
>>> On Wed, 16 Oct 2019 05:50:08 +0000
>>> Parav Pandit <parav@mellanox.com> wrote:
>>>    
>>>> Hi Alex,
>>>>   
>>>>> -----Original Message-----
>>>>> From: Alex Williamson <alex.williamson@redhat.com>
>>>>> Sent: Tuesday, October 15, 2019 12:27 PM
>>>>> To: Jason Wang <jasowang@redhat.com>
>>>>> Cc: Cornelia Huck <cohuck@redhat.com>; kvm@vger.kernel.org; linux-
>>>>> s390@vger.kernel.org; linux-kernel@vger.kernel.org; dri-
>>>>> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
>>>>> intel-gvt- dev@lists.freedesktop.org; kwankhede@nvidia.com;
>>>>> mst@redhat.com; tiwei.bie@intel.com;
>>>>> virtualization@lists.linux-foundation.org;
>>>>> netdev@vger.kernel.org; maxime.coquelin@redhat.com;
>>>>> cunming.liang@intel.com; zhihong.wang@intel.com;
>>>>> rob.miller@broadcom.com; xiao.w.wang@intel.com;
>>>>> haotian.wang@sifive.com; zhenyuw@linux.intel.com;
>>>>> zhi.a.wang@intel.com; jani.nikula@linux.intel.com;
>>>>> joonas.lahtinen@linux.intel.com; rodrigo.vivi@intel.com;
>>>>> airlied@linux.ie; daniel@ffwll.ch; farman@linux.ibm.com;
>>>>> pasic@linux.ibm.com; sebott@linux.ibm.com; oberpar@linux.ibm.com;
>>>>> heiko.carstens@de.ibm.com; gor@linux.ibm.com;
>>>>> borntraeger@de.ibm.com; akrowiak@linux.ibm.com;
>>>>> freude@linux.ibm.com; lingshan.zhu@intel.com; Ido Shamay
>>>>> <idos@mellanox.com>; eperezma@redhat.com; lulu@redhat.com; Parav
>>>>> Pandit <parav@mellanox.com>; christophe.de.dinechin@gmail.com;
>>>>> kevin.tian@intel.com
>>>>> Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
>>>>>
>>>>> On Tue, 15 Oct 2019 20:17:01 +0800
>>>>> Jason Wang <jasowang@redhat.com> wrote:
>>>>>   
>>>>>> On 2019/10/15 下午6:41, Cornelia Huck wrote:
>>>>>>> On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang
>>>>>>> <jasowang@redhat.com> wrote:
>>>    
>>>>>>>> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
>>>>>>>>    	extern int  mdev_register_device(struct device *dev,
>>>>>>>>    	                                 const struct
>>>>>>>> mdev_parent_ops *ops);
>>>>>>>>
>>>>>>>> -It is also required to specify the class_id through::
>>>>>>>> +It is also required to specify the class_id and device
>>>>>>>> +specific ops
>>>>> through::
>>>>>>>> -	extern int mdev_set_class(struct device *dev, u16 id);
>>>>>>>> +	extern int mdev_set_class(struct device *dev, u16 id,
>>>>>>>> +	                          const void *ops);
>>>>>>> Apologies if that has already been discussed, but do we want a
>>>>>>> 1:1 relationship between id and ops, or can different devices
>>>>>>> with the same id register different ops?
>>>>>>
>>>>>> I think we have a N:1 mapping between id and ops, e.g we want both
>>>>>> virtio-mdev and vhost-mdev use a single set of device ops.
>>>>> The contents of the ops structure is essentially defined by the id,
>>>>> which is why I was leaning towards them being defined together.
>>>>> They are effectively interlocked, the id defines which mdev "endpoint"
>>>>> driver is loaded and that driver requires mdev_get_dev_ops() to
>>>>> return the structure required by the driver.  I wish there was a way
>>>>> we could incorporate type checking here.  We toyed with the idea of
>>>>> having the class in the same structure as the ops, but I think this
>>>>> approach was chosen for simplicity.  We could still do something like:
>>>>>
>>>>> int mdev_set_class_struct(struct device *dev, const struct
>>>>> mdev_class_struct *class);
>>>>>
>>>>> struct mdev_class_struct {
>>>>> 	u16	id;
>>>>> 	union {
>>>>> 		struct vfio_mdev_ops vfio_ops;
>>>>> 		struct virtio_mdev_ops virtio_ops;
>>>>> 	};
>>>>> };
>>>>>
>>>>> Maybe even:
>>>>>
>>>>> struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev) {
>>>>> 	BUG_ON(mdev->class.id != MDEV_ID_VFIO);
>>>>> 	return &mdev->class.vfio_ops;
>>>>> }
>>>>>
>>>>> The match callback would of course just use the mdev->class.id value.
>>>>> Functionally equivalent, but maybe better type characteristics.
>>>>> Thanks,
>>>>>
>>>>> Alex
>>>> We have 3 use cases of mdev.
>>>> 1. current mdev binding to vfio_mdev
>>>> 2. mdev binding to virtio
>>>> 3. mdev binding to mlx5_core without dev_ops
>>>>
>>>> Also
>>>> (a) a given parent may serve multiple types of classes in future.
>>>> (b) number of classes may not likely explode, they will be handful of
>>>> them. (vfio_mdev, virtio)
>>>>
>>>> So, instead of making copies of this dev_ops pointer in each mdev, it is better
>>> to keep const multiple ops in their parent device.
>>>> Something like below,
>>>>
>>>> struct mdev_parent {
>>>> 	[..]
>>>> 	struct mdev_parent_ops *parent_ops; /* create, remove */
>>>> 	struct vfio_mdev_ops *vfio_ops; /* read,write, ioctl etc */
>>>> 	struct virtio_mdev_ops *virtio_ops; /* virtio ops */ };
>>> That feels a bit odd. Why should the parent carry pointers to every possible
>>> version of ops?
>>>    
>> How many are we expecting? I envisioned handful of them.
>> It carries because parent is few, mdevs are several hundreds.
>> It makes sense to keep few copies, instead of several hundred copies
>> and it doesn't need to setup on every mdev creation.
> It does need setup on every mdev creation, it's just a matter of the
> scope, 'id and ops' vs 'id only' vs 'ops with implicit id'.  The other
> argument is assuming a space vs time trade-off that I'm having a hard
> time judging is necessarily the correct approach.  We potentially have
> better data locality in the mdev device structure vs the parent.  The
> caching of the ops structure itself is separate from how we get to it.
> We might have hundreds of pointers to those ops structure, but the
> space trade-off might we worth it if they're on the same cacheline as
> the mdev device itself vs the indirection via the parent.
>
> I see a couple other drawbacks to the parent hosted ops pointers as
> well.  First, it imposes that per parent there can only be one device
> ops structure per class id, but who's to say that different types of
> mdev devices for a given parent all make the same callbacks into the
> parent.  For instance, for a vfio-mdev we already support the concept
> of an iommu backing device which makes the type1 iommu code behave a
> little differently.  Those differences might be sufficient that the
> parent driver would register a different device ops structure for an
> iommu backed mdev vs a non-iommu backed device.  The other drawback is
> that it implies a binary difference in all mdev parent drivers to add
> any new device ids.  I know we don't guarantee binary compatibility,
> but it's rather ugly.
>
> Overall, I guess I tend to prefer Connie's proposal, the class id and
> structure are tied together and the parent driver is only responsible
> for one of them, the class id is hidden away in mdev-core and the mdev
> driver itself.


Will go this way.


>
>>>> const struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_parent
>>>> *parent); const struct virtio_mdev_ops *mdev_get_virtio_ops(struct
>>>> mdev_parent *parent);
>>>>
>>>> This way,
>>>> (a) we have strong type check support
>>>> (b) ops pointer is not duplicated across several hundred mdev
>>>> devices, and don't have to set on every mdev creation
>>>> (c) all 3 classes of mdev are supported
>>>> (d) one extra symbol table entry used per ops type, but there are
>>>> not
>>> expected to grow a lot.
>>>> (e) multiple classes per single parent is still supported
>>>> (f) still extendible for multiple classes (well defined classes =
>>>> vfio, virtio, and vendor class)
>>> Yet another suggestion: have the class id derive from the function
>>> you use to set up the ops.
>>>
>>> void mdev_set_vfio_ops(struct mdev_device *mdev, const struct
>>> vfio_mdev_ops *vfio_ops) {
>>> 	mdev->device_ops = vfio_ops;
>>> 	mdev->class_id = MDEV_ID_VFIO;
>>> }
>>>
>>> void mdev_set_virtio_ops(struct mdev_device *mdev, const struct
>>> virtio_mdev_ops *virtio_ops) {
>>> 	mdev->device_ops = virtio_ops;
>>> 	mdev->class_id = MDEV_ID_VIRTIO;
>>> }
>>>
>>> void mdev_set_vhost_ops(struct mdev_device *mdev, const struct
>>> virtio_mdev_ops *virtio_ops) {
>>> 	mdev->device_ops = virtio_ops;
>>> 	mdev->class_id = MDEV_ID_VHOST;
>>> }
>>>
>>> void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */ {
>>> 	mdev->class_id = MDEV_ID_VENDOR;
>>> }
> One further step towards making this hard to use incorrectly might be
> to return an error if class_id is already set.  Thanks,
>
> Alex


I will add a BUG_ON() when class_id has already set.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Cornelia Huck]

On Thu, 17 Oct 2019 16:30:43 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2019/10/17 上午12:53, Alex Williamson wrote:

> >>> Yet another suggestion: have the class id derive from the function
> >>> you use to set up the ops.
> >>>
> >>> void mdev_set_vfio_ops(struct mdev_device *mdev, const struct
> >>> vfio_mdev_ops *vfio_ops) {
> >>> 	mdev->device_ops = vfio_ops;
> >>> 	mdev->class_id = MDEV_ID_VFIO;
> >>> }
> >>>
> >>> void mdev_set_virtio_ops(struct mdev_device *mdev, const struct
> >>> virtio_mdev_ops *virtio_ops) {
> >>> 	mdev->device_ops = virtio_ops;
> >>> 	mdev->class_id = MDEV_ID_VIRTIO;
> >>> }
> >>>
> >>> void mdev_set_vhost_ops(struct mdev_device *mdev, const struct
> >>> virtio_mdev_ops *virtio_ops) {
> >>> 	mdev->device_ops = virtio_ops;
> >>> 	mdev->class_id = MDEV_ID_VHOST;
> >>> }
> >>>
> >>> void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */ {
> >>> 	mdev->class_id = MDEV_ID_VENDOR;
> >>> }  
> > One further step towards making this hard to use incorrectly might be
> > to return an error if class_id is already set.  Thanks,
> >
> > Alex  
> 
> 
> I will add a BUG_ON() when class_id has already set.

Probably better a WARN_ON()?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/7] mdev: introduce device specific ops

[Jason Wang]

On 2019/10/17 下午4:45, Cornelia Huck wrote:
> On Thu, 17 Oct 2019 16:30:43 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2019/10/17 上午12:53, Alex Williamson wrote:
>>>>> Yet another suggestion: have the class id derive from the function
>>>>> you use to set up the ops.
>>>>>
>>>>> void mdev_set_vfio_ops(struct mdev_device *mdev, const struct
>>>>> vfio_mdev_ops *vfio_ops) {
>>>>> 	mdev->device_ops = vfio_ops;
>>>>> 	mdev->class_id = MDEV_ID_VFIO;
>>>>> }
>>>>>
>>>>> void mdev_set_virtio_ops(struct mdev_device *mdev, const struct
>>>>> virtio_mdev_ops *virtio_ops) {
>>>>> 	mdev->device_ops = virtio_ops;
>>>>> 	mdev->class_id = MDEV_ID_VIRTIO;
>>>>> }
>>>>>
>>>>> void mdev_set_vhost_ops(struct mdev_device *mdev, const struct
>>>>> virtio_mdev_ops *virtio_ops) {
>>>>> 	mdev->device_ops = virtio_ops;
>>>>> 	mdev->class_id = MDEV_ID_VHOST;
>>>>> }
>>>>>
>>>>> void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */ {
>>>>> 	mdev->class_id = MDEV_ID_VENDOR;
>>>>> }
>>> One further step towards making this hard to use incorrectly might be
>>> to return an error if class_id is already set.  Thanks,
>>>
>>> Alex
>>
>> I will add a BUG_ON() when class_id has already set.
> Probably better a WARN_ON()?


Right.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/7] mdev: class id support

[Cornelia Huck]

On Fri, 11 Oct 2019 16:15:51 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Mdev bus only supports vfio driver right now, so it doesn't implement
> match method. But in the future, we may add drivers other than vfio,
> the first driver could be virtio-mdev. This means we need to add
> device class id support in bus match method to pair the mdev device
> and mdev driver correctly.
> 
> So this patch adds id_table to mdev_driver and class_id for mdev
> device with the match method for mdev bus.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  Documentation/driver-api/vfio-mediated-device.rst |  7 ++++++-
>  drivers/gpu/drm/i915/gvt/kvmgt.c                  |  1 +
>  drivers/s390/cio/vfio_ccw_ops.c                   |  1 +
>  drivers/s390/crypto/vfio_ap_ops.c                 |  1 +
>  drivers/vfio/mdev/mdev_core.c                     | 11 +++++++++++
>  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                              |  8 ++++++++
>  include/linux/mod_devicetable.h                   |  8 ++++++++
>  samples/vfio-mdev/mbochs.c                        |  1 +
>  samples/vfio-mdev/mdpy.c                          |  1 +
>  samples/vfio-mdev/mtty.c                          |  1 +
>  13 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index 25eb7d5b834b..2035e48da7b2 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -102,12 +102,14 @@ structure to represent a mediated device's driver::
>        * @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 {
>  	     const char *name;
>  	     int  (*probe)  (struct device *dev);
>  	     void (*remove) (struct device *dev);
>  	     struct device_driver    driver;
> +	     const struct mdev_class_id *id_table;
>       };
>  
>  A mediated bus driver for mdev should use this structure in the function calls
> @@ -165,12 +167,15 @@ register itself with the mdev core driver::
>  	extern int  mdev_register_device(struct device *dev,
>  	                                 const struct mdev_parent_ops *ops);
>  
> +It is also required to specify the class_id through::
> +
> +	extern int mdev_set_class(struct device *dev, u16 id);

Should the document state explicitly that this should be done in the
->create() callback? Also, I think that the class_id might be different
for different mdevs (even if the parent is the same) -- should that be
mentioned explicitly?

> +
>  However, the mdev_parent_ops structure is not required in the function call
>  that a driver should use to unregister itself with the mdev core driver::
>  
>  	extern void mdev_unregister_device(struct device *dev);
>  
> -
>  Mediated Device Management Interface Through sysfs
>  ==================================================
>  
(...)

Looks reasonable to me.

Re: [PATCH V3 1/7] mdev: class id support

[Jason Wang]

On 2019/10/15 下午6:26, Cornelia Huck wrote:
> On Fri, 11 Oct 2019 16:15:51 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Mdev bus only supports vfio driver right now, so it doesn't implement
>> match method. But in the future, we may add drivers other than vfio,
>> the first driver could be virtio-mdev. This means we need to add
>> device class id support in bus match method to pair the mdev device
>> and mdev driver correctly.
>>
>> So this patch adds id_table to mdev_driver and class_id for mdev
>> device with the match method for mdev bus.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   Documentation/driver-api/vfio-mediated-device.rst |  7 ++++++-
>>   drivers/gpu/drm/i915/gvt/kvmgt.c                  |  1 +
>>   drivers/s390/cio/vfio_ccw_ops.c                   |  1 +
>>   drivers/s390/crypto/vfio_ap_ops.c                 |  1 +
>>   drivers/vfio/mdev/mdev_core.c                     | 11 +++++++++++
>>   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                              |  8 ++++++++
>>   include/linux/mod_devicetable.h                   |  8 ++++++++
>>   samples/vfio-mdev/mbochs.c                        |  1 +
>>   samples/vfio-mdev/mdpy.c                          |  1 +
>>   samples/vfio-mdev/mtty.c                          |  1 +
>>   13 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
>> index 25eb7d5b834b..2035e48da7b2 100644
>> --- a/Documentation/driver-api/vfio-mediated-device.rst
>> +++ b/Documentation/driver-api/vfio-mediated-device.rst
>> @@ -102,12 +102,14 @@ structure to represent a mediated device's driver::
>>         * @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 {
>>   	     const char *name;
>>   	     int  (*probe)  (struct device *dev);
>>   	     void (*remove) (struct device *dev);
>>   	     struct device_driver    driver;
>> +	     const struct mdev_class_id *id_table;
>>        };
>>   
>>   A mediated bus driver for mdev should use this structure in the function calls
>> @@ -165,12 +167,15 @@ register itself with the mdev core driver::
>>   	extern int  mdev_register_device(struct device *dev,
>>   	                                 const struct mdev_parent_ops *ops);
>>   
>> +It is also required to specify the class_id through::
>> +
>> +	extern int mdev_set_class(struct device *dev, u16 id);
> Should the document state explicitly that this should be done in the
> ->create() callback?


Yes, it's better.


> Also, I think that the class_id might be different
> for different mdevs (even if the parent is the same) -- should that be
> mentioned explicitly?


Yes, depends on the mdev_supported_type.

Thanks


>
>> +
>>   However, the mdev_parent_ops structure is not required in the function call
>>   that a driver should use to unregister itself with the mdev core driver::
>>   
>>   	extern void mdev_unregister_device(struct device *dev);
>>   
>> -
>>   Mediated Device Management Interface Through sysfs
>>   ==================================================
>>   
> (...)
>
> Looks reasonable to me.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/7] mdev: class id support

[Alex Williamson]

On Fri, 11 Oct 2019 16:15:51 +0800
Jason Wang <jasowang@redhat.com> wrote:
  
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b558d4cfd082..724e9b9841d8 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -45,6 +45,12 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data)
>  }
>  EXPORT_SYMBOL(mdev_set_drvdata);
>  
> +void mdev_set_class(struct mdev_device *mdev, u16 id)
> +{
> +	mdev->class_id = id;
> +}
> +EXPORT_SYMBOL(mdev_set_class);
> +
>  struct device *mdev_dev(struct mdev_device *mdev)
>  {
>  	return &mdev->dev;
> @@ -135,6 +141,7 @@ 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: class id.
>   *
>   * Add device to list of registered parent devices.
>   * Returns a negative value on error, otherwise 0.
> @@ -324,6 +331,9 @@ int mdev_device_create(struct kobject *kobj,
>  	if (ret)
>  		goto ops_create_fail;
>  
> +	if (!mdev->class_id)

This is a sanity test failure of the parent driver on a privileged
path, I think it's fair to print a warning when this occurs rather than
only return an errno to the user.  In fact, ret is not set to an error
value here, so it looks like this fails to create the device but
returns success.  Thanks,

Alex

> +		goto class_id_fail;
> +
>  	ret = device_add(&mdev->dev);
>  	if (ret)
>  		goto add_fail;
> @@ -340,6 +350,7 @@ int mdev_device_create(struct kobject *kobj,
>  
>  sysfs_fail:
>  	device_del(&mdev->dev);
> +class_id_fail:
>  add_fail:
>  	parent->ops->remove(mdev);
>  ops_create_fail:

Re: [PATCH V3 1/7] mdev: class id support

[Jason Wang]

On 2019/10/16 上午12:38, Alex Williamson wrote:
> On Fri, 11 Oct 2019 16:15:51 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>    
>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>> index b558d4cfd082..724e9b9841d8 100644
>> --- a/drivers/vfio/mdev/mdev_core.c
>> +++ b/drivers/vfio/mdev/mdev_core.c
>> @@ -45,6 +45,12 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data)
>>   }
>>   EXPORT_SYMBOL(mdev_set_drvdata);
>>   
>> +void mdev_set_class(struct mdev_device *mdev, u16 id)
>> +{
>> +	mdev->class_id = id;
>> +}
>> +EXPORT_SYMBOL(mdev_set_class);
>> +
>>   struct device *mdev_dev(struct mdev_device *mdev)
>>   {
>>   	return &mdev->dev;
>> @@ -135,6 +141,7 @@ 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: class id.
>>    *
>>    * Add device to list of registered parent devices.
>>    * Returns a negative value on error, otherwise 0.
>> @@ -324,6 +331,9 @@ int mdev_device_create(struct kobject *kobj,
>>   	if (ret)
>>   		goto ops_create_fail;
>>   
>> +	if (!mdev->class_id)
> This is a sanity test failure of the parent driver on a privileged
> path, I think it's fair to print a warning when this occurs rather than
> only return an errno to the user.  In fact, ret is not set to an error
> value here, so it looks like this fails to create the device but
> returns success.  Thanks,
>
> Alex


Will fix.

Thanks


>
>> +		goto class_id_fail;
>> +
>>   	ret = device_add(&mdev->dev);
>>   	if (ret)
>>   		goto add_fail;
>> @@ -340,6 +350,7 @@ int mdev_device_create(struct kobject *kobj,
>>   
>>   sysfs_fail:
>>   	device_del(&mdev->dev);
>> +class_id_fail:
>>   add_fail:
>>   	parent->ops->remove(mdev);
>>   ops_create_fail:
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/7] mdev: class id support

[Jason Wang]

On 2019/10/16 下午12:57, Parav Pandit wrote:
>
>> -----Original Message-----
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Friday, October 11, 2019 3:16 AM
>> To: kvm@vger.kernel.org; linux-s390@vger.kernel.org; linux-
>> kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; intel-
>> gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org;
>> kwankhede@nvidia.com; alex.williamson@redhat.com; mst@redhat.com;
>> tiwei.bie@intel.com
>> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org;
>> cohuck@redhat.com; maxime.coquelin@redhat.com;
>> cunming.liang@intel.com; zhihong.wang@intel.com;
>> rob.miller@broadcom.com; xiao.w.wang@intel.com;
>> haotian.wang@sifive.com; zhenyuw@linux.intel.com; zhi.a.wang@intel.com;
>> jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com;
>> rodrigo.vivi@intel.com; airlied@linux.ie; daniel@ffwll.ch;
>> farman@linux.ibm.com; pasic@linux.ibm.com; sebott@linux.ibm.com;
>> oberpar@linux.ibm.com; heiko.carstens@de.ibm.com; gor@linux.ibm.com;
>> borntraeger@de.ibm.com; akrowiak@linux.ibm.com; freude@linux.ibm.com;
>> lingshan.zhu@intel.com; Ido Shamay <idos@mellanox.com>;
>> eperezma@redhat.com; lulu@redhat.com; Parav Pandit
>> <parav@mellanox.com>; christophe.de.dinechin@gmail.com;
>> kevin.tian@intel.com; Jason Wang <jasowang@redhat.com>
>> Subject: [PATCH V3 1/7] mdev: class id support
>>
>> Mdev bus only supports vfio driver right now, so it doesn't implement match
>> method. But in the future, we may add drivers other than vfio, the first
>> driver could be virtio-mdev. This means we need to add device class id
>> support in bus match method to pair the mdev device and mdev driver
>> correctly.
>>
>> So this patch adds id_table to mdev_driver and class_id for mdev device
>> with the match method for mdev bus.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   Documentation/driver-api/vfio-mediated-device.rst |  7 ++++++-
>>   drivers/gpu/drm/i915/gvt/kvmgt.c                  |  1 +
>>   drivers/s390/cio/vfio_ccw_ops.c                   |  1 +
>>   drivers/s390/crypto/vfio_ap_ops.c                 |  1 +
>>   drivers/vfio/mdev/mdev_core.c                     | 11 +++++++++++
>>   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                              |  8 ++++++++
>>   include/linux/mod_devicetable.h                   |  8 ++++++++
>>   samples/vfio-mdev/mbochs.c                        |  1 +
>>   samples/vfio-mdev/mdpy.c                          |  1 +
>>   samples/vfio-mdev/mtty.c                          |  1 +
>>   13 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
>> b/Documentation/driver-api/vfio-mediated-device.rst
>> index 25eb7d5b834b..2035e48da7b2 100644
>> --- a/Documentation/driver-api/vfio-mediated-device.rst
>> +++ b/Documentation/driver-api/vfio-mediated-device.rst
>> @@ -102,12 +102,14 @@ structure to represent a mediated device's driver::
>>         * @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 {
>>   	     const char *name;
>>   	     int  (*probe)  (struct device *dev);
>>   	     void (*remove) (struct device *dev);
>>   	     struct device_driver    driver;
>> +	     const struct mdev_class_id *id_table;
>>        };
>>
>>   A mediated bus driver for mdev should use this structure in the function
>> calls @@ -165,12 +167,15 @@ register itself with the mdev core driver::
>>   	extern int  mdev_register_device(struct device *dev,
>>   	                                 const struct mdev_parent_ops *ops);
>>
>> +It is also required to specify the class_id through::
>> +
>> +	extern int mdev_set_class(struct device *dev, u16 id);
> Drop extern.
> In actual API you have correct signature, i.e. struct mdev_device.
> s/struct device/struct mdev_device.


Yes, will fix.


>
>> +
>>   However, the mdev_parent_ops structure is not required in the function call
>> that a driver should use to unregister itself with the mdev core driver::
>>
>>   	extern void mdev_unregister_device(struct device *dev);
>>
>> -
>>   Mediated Device Management Interface Through sysfs
>> ==================================================
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 343d79c1cb7e..17e9d4634c84 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -678,6 +678,7 @@ static int intel_vgpu_create(struct kobject *kobj,
>> struct mdev_device *mdev)
>>   		     dev_name(mdev_dev(mdev)));
>>   	ret = 0;
>>
>> +	mdev_set_class(mdev, MDEV_ID_VFIO);
>>   out:
>>   	return ret;
>>   }
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
>> b/drivers/s390/cio/vfio_ccw_ops.c index f0d71ab77c50..b5d223882c6c
>> 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -129,6 +129,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj,
>> struct mdev_device *mdev)
>>   			   private->sch->schid.ssid,
>>   			   private->sch->schid.sch_no);
>>
>> +	mdev_set_class(mdev, MDEV_ID_VFIO);
>>   	return 0;
>>   }
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 5c0f53c6dde7..47df1c593c35 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -343,6 +343,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj,
>> struct mdev_device *mdev)
>>   	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>>   	mutex_unlock(&matrix_dev->lock);
>>
>> +	mdev_set_class(mdev, MDEV_ID_VFIO);
>>   	return 0;
>>   }
>>
>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>> index b558d4cfd082..724e9b9841d8 100644
>> --- a/drivers/vfio/mdev/mdev_core.c
>> +++ b/drivers/vfio/mdev/mdev_core.c
>> @@ -45,6 +45,12 @@ void mdev_set_drvdata(struct mdev_device *mdev,
>> void *data)  }  EXPORT_SYMBOL(mdev_set_drvdata);
>>
>> +void mdev_set_class(struct mdev_device *mdev, u16 id) {
>> +	mdev->class_id = id;
>> +}
>> +EXPORT_SYMBOL(mdev_set_class);
>> +
> Usually with every exported symbol we have signature comment block that describes when to use an API etc.
> Please add it that describes that this API must be called during create() callback.


Ok.


>
>>   struct device *mdev_dev(struct mdev_device *mdev)  {
>>   	return &mdev->dev;
>> @@ -135,6 +141,7 @@ 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: class id.
>>    *
>>    * Add device to list of registered parent devices.
>>    * Returns a negative value on error, otherwise 0.
>> @@ -324,6 +331,9 @@ int mdev_device_create(struct kobject *kobj,
>>   	if (ret)
>>   		goto ops_create_fail;
>>
>> +	if (!mdev->class_id)
>> +		goto class_id_fail;
>> +
>>   	ret = device_add(&mdev->dev);
>>   	if (ret)
>>   		goto add_fail;
>> @@ -340,6 +350,7 @@ int mdev_device_create(struct kobject *kobj,
>>
>>   sysfs_fail:
>>   	device_del(&mdev->dev);
>> +class_id_fail:
> No need for new label. Just use add_fail label.


Ok.


>
>>   add_fail:
>>   	parent->ops->remove(mdev);
>>   ops_create_fail:
>> diff --git a/drivers/vfio/mdev/mdev_driver.c
>> b/drivers/vfio/mdev/mdev_driver.c index 0d3223aee20b..b7c40ce86ee3
>> 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_class_id *ids = mdrv->id_table;
>> +
>> +	for (i = 0; ids[i].id; i++)
>> +		if (ids[i].id == mdev->class_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..c65f436c1869 100644
>> --- a/drivers/vfio/mdev/mdev_private.h
>> +++ b/drivers/vfio/mdev/mdev_private.h
>> @@ -33,6 +33,7 @@ struct mdev_device {
>>   	struct kobject *type_kobj;
>>   	struct device *iommu_device;
>>   	bool active;
>> +	u16 class_id;
>>   };
>>
>>   #define to_mdev_device(dev)	container_of(dev, struct mdev_device, dev)
>> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
>> index 30964a4e0a28..fd2a4d9a3f26 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_class_id id_table[] = {
> static const


Fixed.


>
>> +	{ MDEV_ID_VFIO },
> I guess you don't need extra braces for each entry.
> Since this enum represents MDEV class id, it better to name it as MDEV_CLASS_ID_VFIO.
> (Similar to  PCI_VENDOR_ID, PCI_DEVICE_ID)..


Will do.


>> +	{ 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..a7570cf13ba4 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,12 +126,14 @@ struct mdev_driver {
>>   	int  (*probe)(struct device *dev);
>>   	void (*remove)(struct device *dev);
>>   	struct device_driver driver;
>> +	const struct mdev_class_id *id_table;
>>   };
>>
>>   #define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
>>
>>   void *mdev_get_drvdata(struct mdev_device *mdev);  void
>> mdev_set_drvdata(struct mdev_device *mdev, void *data);
>> +void mdev_set_class(struct mdev_device *mdev, u16 id);
> Better to insert new API after mdev_uuid().


Ok.

Thanks


>
>>   const guid_t *mdev_uuid(struct mdev_device *mdev);
>>
>>   extern struct bus_type mdev_bus_type;
>> @@ -145,4 +148,9 @@ 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);
>>
>> +enum {
>> +	MDEV_ID_VFIO = 1,
>> +	/* New entries must be added here */
>> +};
>> +
>>   #endif /* MDEV_H */
>> diff --git a/include/linux/mod_devicetable.h
>> b/include/linux/mod_devicetable.h index 5714fd35a83c..f32c6e44fb1a
>> 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -821,4 +821,12 @@ struct wmi_device_id {
>>   	const void *context;
>>   };
>>
>> +/**
>> + * struct mdev_class_id - MDEV device class identifier
>> + * @id: Used to identify a specific class of device, e.g vfio-mdev device.
>> + */
>> +struct mdev_class_id {
>> +	__u16 id;
>> +};
>> +
>>   #endif /* LINUX_MOD_DEVICETABLE_H */
>> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
>> index ac5c8c17b1ff..fd8491e58fe2 100644
>> --- a/samples/vfio-mdev/mbochs.c
>> +++ b/samples/vfio-mdev/mbochs.c
>> @@ -561,6 +561,7 @@ static int mbochs_create(struct kobject *kobj, struct
>> mdev_device *mdev)
>>   	mbochs_reset(mdev);
>>
>>   	mbochs_used_mbytes += type->mbytes;
>> +	mdev_set_class(mdev, MDEV_ID_VFIO);
>>   	return 0;
>>
>>   err_mem:
>> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index
>> cc86bf6566e4..889472b06708 100644
>> --- a/samples/vfio-mdev/mdpy.c
>> +++ b/samples/vfio-mdev/mdpy.c
>> @@ -269,6 +269,7 @@ static int mdpy_create(struct kobject *kobj, struct
>> mdev_device *mdev)
>>   	mdpy_reset(mdev);
>>
>>   	mdpy_count++;
>> +	mdev_set_class(mdev, MDEV_ID_VFIO);
>>   	return 0;
>>   }
>>
>> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index
>> ce84a300a4da..618810ca4331 100644
>> --- a/samples/vfio-mdev/mtty.c
>> +++ b/samples/vfio-mdev/mtty.c
>> @@ -755,6 +755,7 @@ static int mtty_create(struct kobject *kobj, struct
>> mdev_device *mdev)
>>   	list_add(&mdev_state->next, &mdev_devices_list);
>>   	mutex_unlock(&mdev_list_lock);
>>
>> +	mdev_set_class(mdev, MDEV_ID_VFIO);
>>   	return 0;
>>   }
>>
>> --
>> 2.19.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/7] mdev: class id support

[Jason Wang]

On 2019/10/16 下午12:57, Parav Pandit wrote:
>> +static struct mdev_class_id id_table[] = {
> static const
>
>> +	{ MDEV_ID_VFIO },
> I guess you don't need extra braces for each entry.
> Since this enum represents MDEV class id, it better to name it as MDEV_CLASS_ID_VFIO.
> (Similar to  PCI_VENDOR_ID, PCI_DEVICE_ID)..
>

Gcc start to complain like:

warning: missing braces around initializer [-Wmissing-braces]
  static const struct mdev_class_id id_table[] = {
                                                 ^
   MDEV_ID_VFIO, 0,
   {           } {
  };
  }

So I will keep this part untouched.

Thanks


>> +	{ 0 },
>> +};
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization