On Mon, 23 Sep 2019 21:03:26 +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, one example is virtio-mdev[1] driver. 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 parent 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 | 2 +- drivers/s390/cio/vfio_ccw_ops.c | 2 +- drivers/s390/crypto/vfio_ap_ops.c | 3 ++- drivers/vfio/mdev/mdev_core.c | 14 ++++++++++++-- drivers/vfio/mdev/mdev_driver.c | 14 ++++++++++++++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 6 ++++++ include/linux/mdev.h | 7 ++++++- include/linux/mod_devicetable.h | 8 ++++++++ samples/vfio-mdev/mbochs.c | 2 +- samples/vfio-mdev/mdpy.c | 2 +- samples/vfio-mdev/mtty.c | 2 +- 13 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index 25eb7d5b834b..0e052072e1d8 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 @@ -116,7 +118,7 @@ to register and unregister itself with the core driver:

• Register::

  extern int mdev_register_driver(struct mdev_driver *drv,

• 		   struct module *owner);
  

•                                 struct module *owner);
  
  

• Unregister::

@@ -163,7 +165,8 @@ A driver should use the mdev_parent_ops structure in the function call to register itself with the mdev core driver::

extern int mdev_register_device(struct device *dev,

•                                 const struct mdev_parent_ops *ops);
  

•                                 const struct mdev_parent_ops *ops,
  

•                                 u8 class_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:: diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 23aa3e50cbf8..19d51a35f019 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1625,7 +1625,7 @@ static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops) return -EFAULT; intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;

• return mdev_register_device(dev, &intel_vgpu_ops);

• return mdev_register_vfio_device(dev, &intel_vgpu_ops);

}

static void kvmgt_host_exit(struct device *dev) diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index f0d71ab77c50..246ff0f80944 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -588,7 +588,7 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = {

int vfio_ccw_mdev_reg(struct subchannel *sch) {

• return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);

• return mdev_register_vfio_device(&sch->dev, &vfio_ccw_mdev_ops);

}

void vfio_ccw_mdev_unreg(struct subchannel *sch) diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 5c0f53c6dde7..7487fc39d2c5 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -1295,7 +1295,8 @@ int vfio_ap_mdev_register(void) { atomic_set(&matrix_dev->available_instances, MAX_ZDEV_ENTRIES_EXT);

• return mdev_register_device(&matrix_dev->device, &vfio_ap_matrix_ops);

• return mdev_register_vfio_device(&matrix_dev->device,

• 			 &vfio_ap_matrix_ops);
  

}

void vfio_ap_mdev_unregister(void) diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index b558d4cfd082..a02c256a3514 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -135,11 +135,14 @@ static int mdev_device_remove_cb(struct device *dev, void *data)

• mdev_register_device : Register a device
• @dev: device structure representing parent device.
• @ops: Parent device operation structure to be registered.

• • @id: device id.
  • Add device to list of registered parent devices.
  • Returns a negative value on error, otherwise 0.
  */

-int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops) +int mdev_register_device(struct device *dev,

• 	 const struct mdev_parent_ops *ops,
  

• 	 u8 class_id)
  

{ int ret; struct mdev_parent *parent; @@ -175,6 +178,7 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)

parent->dev = dev; parent->ops = ops;

• parent->class_id = class_id;

I don't think we want to tie the class_id to the parent. mdev parent devices can create various types of devices, some might be virtio, some might be vfio. I think the cover letter even suggests that's a direction these virtio devices might be headed. It seems the class should be on the resulting device itself. That also suggests that at the parent we cannot have a single device_ops, the ops used will depend on the type of device created. Perhaps that means we need vfio_ops alongside virtio_ops, rather than a common device_ops. Thanks,

Alex

if (!mdev_bus_compat_class) { mdev_bus_compat_class = class_compat_register("mdev_bus"); @@ -208,7 +212,13 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops) put_device(dev); return ret; } -EXPORT_SYMBOL(mdev_register_device);

+int mdev_register_vfio_device(struct device *dev,

• 	      const struct mdev_parent_ops *ops)
  

+{

• return mdev_register_device(dev, ops, MDEV_ID_VFIO);

+} +EXPORT_SYMBOL(mdev_register_vfio_device);

/*

• mdev_unregister_device : Unregister a parent device

diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c index 0d3223aee20b..b70bbebc9dd3 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->parent->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..e58b07c866b1 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -22,6 +22,7 @@ struct mdev_parent { struct list_head type_list; /* Synchronize device creation/removal with parent unregistration */ struct rw_semaphore unreg_sem;

• u8 class_id;

};

struct mdev_device { 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..3ebae310f599 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -118,6 +118,7 @@ struct mdev_type_attribute mdev_type_attr_##_name = \

• @probe: called when new device created
• @remove: called when device removed
• @driver: device driver structure

• • @id_table: the ids serviced by this driver.
  **/

struct mdev_driver { @@ -125,6 +126,7 @@ struct mdev_driver { int (*probe)(struct device *dev); void (*remove)(struct device *dev); struct device_driver driver;

• const struct mdev_class_id *id_table;

};

#define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver) @@ -135,7 +137,8 @@ const guid_t *mdev_uuid(struct mdev_device *mdev);

extern struct bus_type mdev_bus_type;

-int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops); +int mdev_register_vfio_device(struct device *dev,

• 	      const struct mdev_parent_ops *ops);
  

void mdev_unregister_device(struct device *dev);

int mdev_register_driver(struct mdev_driver *drv, struct module *owner); @@ -145,4 +148,6 @@ struct device *mdev_parent_dev(struct mdev_device *mdev); struct device *mdev_dev(struct mdev_device *mdev); struct mdev_device *mdev_from_dev(struct device *dev);

+#define MDEV_ID_VFIO 1 /* VFIO device */

#endif /* MDEV_H */ diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 5714fd35a83c..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..71a4469be85d 100644 --- a/samples/vfio-mdev/mbochs.c +++ b/samples/vfio-mdev/mbochs.c @@ -1468,7 +1468,7 @@ static int __init mbochs_dev_init(void) if (ret) goto failed2;

• ret = mdev_register_device(&mbochs_dev, &mdev_fops);

• ret = mdev_register_vfio_device(&mbochs_dev, &mdev_fops); if (ret) goto failed3;

diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index cc86bf6566e4..d3029dd27d91 100644 --- a/samples/vfio-mdev/mdpy.c +++ b/samples/vfio-mdev/mdpy.c @@ -775,7 +775,7 @@ static int __init mdpy_dev_init(void) if (ret) goto failed2;

• ret = mdev_register_device(&mdpy_dev, &mdev_fops);

• ret = mdev_register_vfio_device(&mdpy_dev, &mdev_fops); if (ret) goto failed3;

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index 92e770a06ea2..744c88a6b22c 100644 --- a/samples/vfio-mdev/mtty.c +++ b/samples/vfio-mdev/mtty.c @@ -1468,7 +1468,7 @@ static int __init mtty_dev_init(void) if (ret) goto failed2;

• ret = mdev_register_device(&mtty_dev.dev, &mdev_fops);

• ret = mdev_register_vfio_device(&mtty_dev.dev, &mdev_fops); if (ret) goto failed3;

Hi Jason,

-----Original Message----- From: Jason Wang jasowang@redhat.com Sent: Monday, September 23, 2019 8:03 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; Jason Wang jasowang@redhat.com Subject: [PATCH 1/6] 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, one example is virtio-mdev[1] driver. 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 parent 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 | 2 +- drivers/s390/cio/vfio_ccw_ops.c | 2 +- drivers/s390/crypto/vfio_ap_ops.c | 3 ++- drivers/vfio/mdev/mdev_core.c | 14 ++++++++++++-- drivers/vfio/mdev/mdev_driver.c | 14 ++++++++++++++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 6 ++++++ include/linux/mdev.h | 7 ++++++- include/linux/mod_devicetable.h | 8 ++++++++ samples/vfio-mdev/mbochs.c | 2 +- samples/vfio-mdev/mdpy.c | 2 +- samples/vfio-mdev/mtty.c | 2 +- 13 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index 25eb7d5b834b..0e052072e1d8 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.
  

No full stop at the end.

   */
  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 @@ -116,7 +118,7 @@ to register and unregister itself with the core driver:

• Register::

  extern int mdev_register_driver(struct mdev_driver *drv,

• 		   struct module *owner);
  

•                                 struct module *owner);
  
  

Unrelated change in this patch.

• Unregister::

@@ -163,7 +165,8 @@ A driver should use the mdev_parent_ops structure in the function call to register itself with the mdev core driver::

extern int mdev_register_device(struct device *dev,

•                                 const struct mdev_parent_ops *ops);
  

•                                 const struct mdev_parent_ops *ops,
  

•                                 u8 class_id);
  
  

Cover letter from Change-V2 says that it class_id changed from " use u16 instead u8 for class id" But it is still u8 here?

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/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 23aa3e50cbf8..19d51a35f019 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1625,7 +1625,7 @@ static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops) return -EFAULT; intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;

• return mdev_register_device(dev, &intel_vgpu_ops);

• return mdev_register_vfio_device(dev, &intel_vgpu_ops);

}

static void kvmgt_host_exit(struct device *dev) diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index f0d71ab77c50..246ff0f80944 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -588,7 +588,7 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = {

int vfio_ccw_mdev_reg(struct subchannel *sch) {

• return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);

• return mdev_register_vfio_device(&sch->dev, &vfio_ccw_mdev_ops);

}

void vfio_ccw_mdev_unreg(struct subchannel *sch) diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 5c0f53c6dde7..7487fc39d2c5 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -1295,7 +1295,8 @@ int vfio_ap_mdev_register(void) { atomic_set(&matrix_dev->available_instances, MAX_ZDEV_ENTRIES_EXT);

• return mdev_register_device(&matrix_dev->device,

&vfio_ap_matrix_ops);

• return mdev_register_vfio_device(&matrix_dev->device,

• 			 &vfio_ap_matrix_ops);
  

}

void vfio_ap_mdev_unregister(void) diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index b558d4cfd082..a02c256a3514 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -135,11 +135,14 @@ static int mdev_device_remove_cb(struct device *dev, void *data)

• mdev_register_device : Register a device
• @dev: device structure representing parent device.
• @ops: Parent device operation structure to be registered.

• • @id: device id.

It device id here, but in below its class_id. Please make them uniform.

• Add device to list of registered parent devices.
• Returns a negative value on error, otherwise 0.

*/ -int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops) +int mdev_register_device(struct device *dev,

• 	 const struct mdev_parent_ops *ops,
  

• 	 u8 class_id)
  

{

u16 class_id?

int ret; struct mdev_parent *parent; @@ -175,6 +178,7 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)

parent->dev = dev; parent->ops = ops;

• parent->class_id = class_id;

  if (!mdev_bus_compat_class) { mdev_bus_compat_class =

class_compat_register("mdev_bus"); @@ -208,7 +212,13 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops) put_device(dev); return ret; } -EXPORT_SYMBOL(mdev_register_device);

+int mdev_register_vfio_device(struct device *dev,

• 	      const struct mdev_parent_ops *ops) {
  

• return mdev_register_device(dev, ops, MDEV_ID_VFIO); }

+EXPORT_SYMBOL(mdev_register_vfio_device);

/*

• mdev_unregister_device : Unregister a parent device diff --git

a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c index 0d3223aee20b..b70bbebc9dd3 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->parent->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..e58b07c866b1 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -22,6 +22,7 @@ struct mdev_parent { struct list_head type_list; /* Synchronize device creation/removal with parent unregistration */ struct rw_semaphore unreg_sem;

• u8 class_id;

};

struct mdev_device { 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..3ebae310f599 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -118,6 +118,7 @@ struct mdev_type_attribute mdev_type_attr_##_name = \

• @probe: called when new device created
• @remove: called when device removed
• @driver: device driver structure

• • @id_table: the ids serviced by this driver.
  **/

struct mdev_driver { @@ -125,6 +126,7 @@ struct mdev_driver { int (*probe)(struct device *dev); void (*remove)(struct device *dev); struct device_driver driver;

• const struct mdev_class_id *id_table;

};

#define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver) @@ -135,7 +137,8 @@ const guid_t *mdev_uuid(struct mdev_device *mdev);

extern struct bus_type mdev_bus_type;

-int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops); +int mdev_register_vfio_device(struct device *dev,

• 	      const struct mdev_parent_ops *ops);
  

void mdev_unregister_device(struct device *dev);

int mdev_register_driver(struct mdev_driver *drv, struct module *owner); @@ -145,4 +148,6 @@ struct device *mdev_parent_dev(struct mdev_device *mdev); struct device *mdev_dev(struct mdev_device *mdev); struct mdev_device *mdev_from_dev(struct device *dev);

+#define MDEV_ID_VFIO 1 /* VFIO device */

Instead of define, please put them as enum

enum mdev_class/device_id { 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;

+};

This is u16 as I guess you wanted as opposed to u8 in other places in patch.

#endif /* LINUX_MOD_DEVICETABLE_H */ diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index ac5c8c17b1ff..71a4469be85d 100644 --- a/samples/vfio-mdev/mbochs.c +++ b/samples/vfio-mdev/mbochs.c @@ -1468,7 +1468,7 @@ static int __init mbochs_dev_init(void) if (ret) goto failed2;

• ret = mdev_register_device(&mbochs_dev, &mdev_fops);

• ret = mdev_register_vfio_device(&mbochs_dev, &mdev_fops); if (ret) goto failed3;

diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index cc86bf6566e4..d3029dd27d91 100644 --- a/samples/vfio-mdev/mdpy.c +++ b/samples/vfio-mdev/mdpy.c @@ -775,7 +775,7 @@ static int __init mdpy_dev_init(void) if (ret) goto failed2;

• ret = mdev_register_device(&mdpy_dev, &mdev_fops);

• ret = mdev_register_vfio_device(&mdpy_dev, &mdev_fops); if (ret) goto failed3;

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index 92e770a06ea2..744c88a6b22c 100644 --- a/samples/vfio-mdev/mtty.c +++ b/samples/vfio-mdev/mtty.c @@ -1468,7 +1468,7 @@ static int __init mtty_dev_init(void) if (ret) goto failed2;

• ret = mdev_register_device(&mtty_dev.dev, &mdev_fops);

• ret = mdev_register_vfio_device(&mtty_dev.dev, &mdev_fops); if (ret) goto failed3;

-- 2.19.1

-----Original Message----- From: Jason Wang jasowang@redhat.com Sent: Monday, September 23, 2019 8:03 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; Jason Wang jasowang@redhat.com Subject: [PATCH 2/6] mdev: introduce device specific ops

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. Follow the class id support by previous patch, this patch introduces device specific ops pointer inside parent ops which points to device specific ops (e.g vfio ops). This allows the future drivers like virtio-mdev to implement its own device specific ops.

Signed-off-by: Jason Wang jasowang@redhat.com

.../driver-api/vfio-mediated-device.rst | 4 +- MAINTAINERS | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 15 +++--- drivers/s390/cio/vfio_ccw_ops.c | 15 ++++-- drivers/s390/crypto/vfio_ap_ops.c | 11 ++-- drivers/vfio/mdev/vfio_mdev.c | 31 ++++++----- include/linux/mdev.h | 36 ++----------- include/linux/vfio_mdev.h | 53 +++++++++++++++++++ samples/vfio-mdev/mbochs.c | 17 +++--- samples/vfio-mdev/mdpy.c | 17 +++--- samples/vfio-mdev/mtty.c | 15 ++++-- 11 files changed, 138 insertions(+), 77 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 0e052072e1d8..3ab00e48212f 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,7 +152,9 @@ callbacks per mdev parent device, per mdev type, or any other categorization. Vendor drivers are expected to be fully asynchronous in this respect or provide their own internal resource protection.)

-The callbacks in the mdev_parent_ops structure are as follows: +The device specific callbacks are referred through device_ops pointer +in mdev_parent_ops. For vfio-mdev device, its callbacks in device_ops +are as follows:

• open: open callback of mediated device
• close: close callback of mediated device diff --git a/MAINTAINERS

b/MAINTAINERS index b2326dece28e..89832b316500 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17075,6 +17075,7 @@ S: Maintained F: Documentation/driver-api/vfio-mediated-device.rst F: drivers/vfio/mdev/ F: include/linux/mdev.h +F: include/linux/vfio_mdev.h F: samples/vfio-mdev/

VFIO PLATFORM DRIVER diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 19d51a35f019..8ea86b1e69f1 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> @@ -1600,20 +1601,22 @@ static const struct attribute_group *intel_vgpu_groups[] = { NULL, };

-static struct mdev_parent_ops intel_vgpu_ops = {

• .mdev_attr_groups = intel_vgpu_groups,
• .create = intel_vgpu_create,
• .remove = intel_vgpu_remove,

+static struct vfio_mdev_parent_ops intel_vfio_vgpu_ops = {

Naming it with _dev prefix as intel_vfio_vgpu_dev_ops is better to differentiate with parent_ops.

.open = intel_vgpu_open, .release = intel_vgpu_release,

• .read = intel_vgpu_read, .write = intel_vgpu_write, .mmap = intel_vgpu_mmap, .ioctl = intel_vgpu_ioctl,

};

+static struct mdev_parent_ops intel_vgpu_ops = {

• .mdev_attr_groups = intel_vgpu_groups,
• .create = intel_vgpu_create,
• .remove = intel_vgpu_remove,
• .device_ops = &intel_vfio_vgpu_ops,

+};

static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops) { struct attribute **kvm_type_attrs; diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index 246ff0f80944..02122bbc213e 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>

@@ -574,11 +575,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev, } }

-static const struct mdev_parent_ops vfio_ccw_mdev_ops = {

• .owner = THIS_MODULE,
• .supported_type_groups = mdev_type_groups,
• .create = vfio_ccw_mdev_create,
• .remove = vfio_ccw_mdev_remove,

+static const struct vfio_mdev_parent_ops vfio_mdev_ops = { .open = vfio_ccw_mdev_open, .release = vfio_ccw_mdev_release, .read = vfio_ccw_mdev_read, @@ -586,6 +583,14 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = { .ioctl = vfio_ccw_mdev_ioctl, };

+static const struct mdev_parent_ops vfio_ccw_mdev_ops = {

• .owner = THIS_MODULE,
• .supported_type_groups = mdev_type_groups,
• .create = vfio_ccw_mdev_create,
• .remove = vfio_ccw_mdev_remove,
• .device_ops = &vfio_mdev_ops,

+};

int vfio_ccw_mdev_reg(struct subchannel *sch) { return mdev_register_vfio_device(&sch->dev, &vfio_ccw_mdev_ops); diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 7487fc39d2c5..4251becc7a6d 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>

@@ -1280,15 +1281,19 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev, return ret; }

+static const struct vfio_mdev_parent_ops vfio_mdev_ops = {

• .open = vfio_ap_mdev_open,
• .release = vfio_ap_mdev_release,
• .ioctl = vfio_ap_mdev_ioctl,

+};

static const struct mdev_parent_ops vfio_ap_matrix_ops = { .owner = THIS_MODULE, .supported_type_groups = vfio_ap_mdev_type_groups, .mdev_attr_groups = vfio_ap_mdev_attr_groups, .create = vfio_ap_mdev_create, .remove = vfio_ap_mdev_remove,

• .open = vfio_ap_mdev_open,
• .release = vfio_ap_mdev_release,
• .ioctl = vfio_ap_mdev_ioctl,

• .device_ops = &vfio_mdev_ops,

};

int vfio_ap_mdev_register(void) diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c index fd2a4d9a3f26..d23c9f58c84f 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"

@@ -25,15 +26,16 @@ static int vfio_mdev_open(void *device_data) { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops; int ret;

• if (unlikely(!parent->ops->open))

• if (unlikely(!ops->open)) return -EINVAL;

device_ops is optional and can be NULL for mdev devices which are not required to be mapped via vfio. So please change to,

If (!ops || !ops->open) return -EINVAL;

and rest of the below places.

if (!try_module_get(THIS_MODULE)) return -ENODEV;

• ret = parent->ops->open(mdev);

• ret = ops->open(mdev); if (ret) module_put(THIS_MODULE);

@@ -44,9 +46,10 @@ static void vfio_mdev_release(void *device_data) { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;

• if (likely(parent->ops->release))

• parent->ops->release(mdev);
  

• if (likely(ops->release))

• ops->release(mdev);
  
  

  module_put(THIS_MODULE);

} @@ -56,11 +59,12 @@ static long vfio_mdev_unlocked_ioctl(void *device_data, { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;

• if (unlikely(!parent->ops->ioctl))

• if (unlikely(!ops->ioctl)) return -EINVAL;

• return parent->ops->ioctl(mdev, cmd, arg);

• return ops->ioctl(mdev, cmd, arg);

}

static ssize_t vfio_mdev_read(void *device_data, char __user *buf, @@ -68,11 +72,12 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf, { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;

• if (unlikely(!parent->ops->read))

• if (unlikely(!ops->read)) return -EINVAL;

• return parent->ops->read(mdev, buf, count, ppos);

• return ops->read(mdev, buf, count, ppos);

}

static ssize_t vfio_mdev_write(void *device_data, const char __user *buf, @@ -80,22 +85,24 @@ static ssize_t vfio_mdev_write(void *device_data, const char __user *buf, { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;

• if (unlikely(!parent->ops->write))

• if (unlikely(!ops->write)) return -EINVAL;

• return parent->ops->write(mdev, buf, count, ppos);

• return ops->write(mdev, buf, count, ppos);

}

static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma) { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;

• if (unlikely(!parent->ops->mmap))

• if (unlikely(!ops->mmap)) return -EINVAL;

• return parent->ops->mmap(mdev, vma);

• return ops->mmap(mdev, vma);

}

static const struct vfio_device_ops vfio_mdev_dev_ops = { diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3ebae310f599..fa167bcb81e1 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -48,30 +48,8 @@ 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
    

• • @device_ops: Device specific emulation callback.
• • Parent device that support mediated device should be registered with mdev
  • module with mdev_parent_ops structure.
  **/

@@ -83,15 +61,7 @@ struct mdev_parent_ops {

int (*create)(struct kobject *kobj, struct mdev_device *mdev); int (*remove)(struct mdev_device *mdev);

• int (*open)(struct mdev_device *mdev);
• void (*release)(struct mdev_device *mdev);
• ssize_t (*read)(struct mdev_device *mdev, char __user *buf,

• 	size_t count, loff_t *ppos);
  

• ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,

• 	 size_t count, loff_t *ppos);
  

• long (*ioctl)(struct mdev_device *mdev, unsigned int cmd,

• 	 unsigned long arg);
  

• int (*mmap)(struct mdev_device *mdev, struct vm_area_struct

*vma);

• const void *device_ops;

};

/* interface for exporting mdev supported type attributes */ diff --git a/include/linux/vfio_mdev.h b/include/linux/vfio_mdev.h new file mode 100644 index 000000000000..0c1b34f98f5d --- /dev/null +++ b/include/linux/vfio_mdev.h @@ -0,0 +1,53 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • VFIO Mediated device definition
• */

+#ifndef VFIO_MDEV_H +#define VFIO_MDEV_H

+#include <linux/types.h> +#include <linux/mdev.h>

+/**

• • struct vfio_mdev_parent_ops - Structure to be registered for each
• • parent 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_parent_ops {

• int (*open)(struct mdev_device *mdev);
• void (*release)(struct mdev_device *mdev);
• ssize_t (*read)(struct mdev_device *mdev, char __user *buf,

• 	size_t count, loff_t *ppos);
  

• ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,

• 	 size_t count, loff_t *ppos);
  

• long (*ioctl)(struct mdev_device *mdev, unsigned int cmd,

• 	 unsigned long arg);
  

• int (*mmap)(struct mdev_device *mdev, struct vm_area_struct

*vma); +};

+#endif diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index 71a4469be85d..107cc30d0f45 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> @@ -1418,12 +1419,7 @@ static struct attribute_group *mdev_type_groups[] = { NULL, };

-static const struct mdev_parent_ops mdev_fops = {

• .owner = THIS_MODULE,
• .mdev_attr_groups = mdev_dev_groups,
• .supported_type_groups = mdev_type_groups,
• .create = mbochs_create,
• .remove = mbochs_remove,

+static const struct vfio_mdev_parent_ops vfio_mdev_ops = { .open = mbochs_open, .release = mbochs_close, .read = mbochs_read, @@ -1432,6 +1428,15 @@ static const struct mdev_parent_ops mdev_fops = { .mmap = mbochs_mmap, };

+static const struct mdev_parent_ops mdev_fops = {

• .owner = THIS_MODULE,
• .mdev_attr_groups = mdev_dev_groups,
• .supported_type_groups = mdev_type_groups,
• .create = mbochs_create,
• .remove = mbochs_remove,
• .device_ops = &vfio_mdev_ops,

+};

static const struct file_operations vd_fops = { .owner = THIS_MODULE, }; diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index d3029dd27d91..2cd2018a53f9 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" @@ -725,12 +726,7 @@ static struct attribute_group *mdev_type_groups[] = { NULL, };

-static const struct mdev_parent_ops mdev_fops = {

• .owner = THIS_MODULE,
• .mdev_attr_groups = mdev_dev_groups,
• .supported_type_groups = mdev_type_groups,
• .create = mdpy_create,
• .remove = mdpy_remove,

+static const struct vfio_mdev_parent_ops vfio_mdev_ops = { .open = mdpy_open, .release = mdpy_close, .read = mdpy_read, @@ -739,6 +735,15 @@ static const struct mdev_parent_ops mdev_fops = { .mmap = mdpy_mmap, };

+static const struct mdev_parent_ops mdev_fops = {

• .owner = THIS_MODULE,
• .mdev_attr_groups = mdev_dev_groups,
• .supported_type_groups = mdev_type_groups,
• .create = mdpy_create,
• .remove = mdpy_remove,
• .device_ops = &vfio_mdev_ops,

+};

static const struct file_operations vd_fops = { .owner = THIS_MODULE, }; diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index 744c88a6b22c..e427425b5daf 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> @@ -1410,6 +1411,14 @@ static struct attribute_group *mdev_type_groups[] = { NULL, };

+static const struct vfio_mdev_parent_ops vfio_mdev_ops = {

• .open = mtty_open,
• .release = mtty_close,
• .read = mtty_read,
• .write = mtty_write,
• .ioctl = mtty_ioctl,

+};

static const struct mdev_parent_ops mdev_fops = { .owner = THIS_MODULE, .dev_attr_groups = mtty_dev_groups, @@ -1417,11 +1426,7 @@ static const struct mdev_parent_ops mdev_fops = { .supported_type_groups = mdev_type_groups, .create = mtty_create, .remove = mtty_remove,

• .open = mtty_open,
• .release = mtty_close,
• .read = mtty_read,
• .write = mtty_write,
• .ioctl = mtty_ioctl,

• .device_ops = &vfio_mdev_ops,

};

static void mtty_device_release(struct device *dev)

2.19.1

-----Original Message----- From: Jason Wang jasowang@redhat.com Sent: Monday, September 23, 2019 8:03 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; Jason Wang jasowang@redhat.com Subject: [PATCH 4/6] virtio: introduce a mdev based transport

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

MAINTAINERS | 1 + drivers/vfio/mdev/Kconfig | 7 + drivers/vfio/mdev/Makefile | 1 + drivers/vfio/mdev/virtio_mdev.c | 416 ++++++++++++++++++++++++++++++++ 4 files changed, 425 insertions(+) create mode 100644 drivers/vfio/mdev/virtio_mdev.c

diff --git a/MAINTAINERS b/MAINTAINERS index 89832b316500..820ec250cc52 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17202,6 +17202,7 @@ F: include/linux/virtio*.h F: include/uapi/linux/virtio_*.h F: drivers/crypto/virtio/ F: mm/balloon_compaction.c +F: drivers/vfio/mdev/virtio_mdev.c

VIRTIO BLOCK AND SCSI DRIVERS M: "Michael S. Tsirkin" mst@redhat.com diff --git a/drivers/vfio/mdev/Kconfig b/drivers/vfio/mdev/Kconfig index 5da27f2100f9..c488c31fc137 100644 --- a/drivers/vfio/mdev/Kconfig +++ b/drivers/vfio/mdev/Kconfig @@ -16,3 +16,10 @@ config VFIO_MDEV_DEVICE default n help VFIO based driver for Mediated devices.

+config VIRTIO_MDEV_DEVICE

• tristate "VIRTIO driver for Mediated devices"
• depends on VFIO_MDEV && VIRTIO
• default n
• help

•  VIRTIO based driver for Mediated devices.
  

diff --git a/drivers/vfio/mdev/Makefile b/drivers/vfio/mdev/Makefile index 101516fdf375..99d31e29c23e 100644 --- a/drivers/vfio/mdev/Makefile +++ b/drivers/vfio/mdev/Makefile @@ -4,3 +4,4 @@ mdev-y := mdev_core.o mdev_sysfs.o mdev_driver.o

obj-$(CONFIG_VFIO_MDEV) += mdev.o obj-$(CONFIG_VFIO_MDEV_DEVICE) += vfio_mdev.o +obj-$(CONFIG_VIRTIO_MDEV_DEVICE) += virtio_mdev.o diff --git a/drivers/vfio/mdev/virtio_mdev.c b/drivers/vfio/mdev/virtio_mdev.c new file mode 100644 index 000000000000..919a082adc9c --- /dev/null +++ b/drivers/vfio/mdev/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> +#include "mdev_private.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;

Every lock must need a comment to describe what it locks. I don't see this lock is used in this patch. Please introduce in the patch that uses it.

• spinlock_t lock;

+};

+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 const struct virtio_mdev_parent_ops *mdev_get_parent_ops(struct +mdev_device *mdev) {

• struct mdev_parent *parent = mdev->parent;
• return parent->ops->device_ops;

+}

+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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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 mdev_device *mdev = vm_get_mdev(vdev);
• const struct virtio_mdev_parent_ops *ops =

mdev_get_parent_ops(mdev);

• struct virtio_mdev_vq_info *info;
• struct virtio_mdev_callback cb;
• struct virtqueue *vq;
• u32 align, num;
• u64 desc_addr, driver_addr, device_addr;
• int err;
• if (!name)

• return NULL;
  

• /* Queue shouldn't already be set up. */
• if (ops->get_vq_ready(mdev, index)) {

• err = -ENOENT;
  

• goto error_available;
  

• }

No need for a goto, to single return done later. Just do if (ops->get_vq_ready(mdev, index)) return -ENOENT;

• /* Allocate and fill out our active queue description */
• info = kmalloc(sizeof(*info), GFP_KERNEL);
• if (!info) {

• err = -ENOMEM;
  

• goto error_kmalloc;
  

• }

Similar to above one.

• num = ops->get_queue_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;
• 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);

+error_kmalloc: +error_available:

• return ERR_PTR(err);

+}

+static void virtio_mdev_del_vq(struct virtqueue *vq) {

• struct mdev_device *mdev = vm_get_mdev(vq->vdev);
• const struct virtio_mdev_parent_ops *ops =

mdev_get_parent_ops(mdev);

• struct virtio_mdev_vq_info *info = vq->priv;
• unsigned int index = vq->index;
• /* 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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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_parent_ops *ops =

mdev_get_parent_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);

+}

+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 = to_mdev_device(dev);
• const struct virtio_mdev_parent_ops *ops =

mdev_get_parent_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;
• spin_lock_init(&vm_dev->lock);
• vm_dev->version = ops->get_version(mdev);
• if (vm_dev->version != 1) {

• dev_err(dev, "Version %ld not supported!\n",
  

• 	vm_dev->version);
  

• 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);
  

• dev_set_drvdata(dev, vm_dev);

No need to set drvdata when there is error returned from register_virtio_device().

• return rc;

Extra line not needed.

+}

+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 },

+};

+static struct mdev_driver virtio_mdev_driver = {

• .name = "virtio_mdev",
• .probe = virtio_mdev_probe,
• .remove = virtio_mdev_remove,

No need for tab, just do single white space.

• .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

On Mon, Sep 23, 2019 at 09:45:59AM -0600, Alex Williamson wrote:

On Mon, 23 Sep 2019 21:03:30 +0800 Jason Wang jasowang@redhat.com wrote:

...

We want to copy from iov to buf, so the direction was wrong.

Signed-off-by: Jason Wang jasowang@redhat.com

drivers/vhost/vringh.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)

Why is this included in the series? Seems like an unrelated fix being held up within a proposal for a new feature. Thanks,

Alex

It's better to have it as patch 1/6, but it's a dependency of the example driver in the series. I can reorder when I apply.

...

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c index 08ad0d1f0476..a0a2d74967ef 100644 --- a/drivers/vhost/vringh.c +++ b/drivers/vhost/vringh.c @@ -852,6 +852,12 @@ static inline int xfer_kern(void *src, void *dst, size_t len) return 0; }

+static inline int kern_xfer(void *dst, void *src, size_t len) +{

• memcpy(dst, src, len);
• return 0;

+}

/**

• vringh_init_kern - initialize a vringh for a kernelspace vring.
• @vrh: the vringh to initialize.

@@ -958,7 +964,7 @@ EXPORT_SYMBOL(vringh_iov_pull_kern); ssize_t vringh_iov_push_kern(struct vringh_kiov *wiov, const void *src, size_t len) {

• return vringh_iov_xfer(wiov, (void *)src, len, xfer_kern);

• return vringh_iov_xfer(wiov, (void *)src, len, kern_xfer);

} EXPORT_SYMBOL(vringh_iov_push_kern);