-----Original Message----- From: kvm-owner@vger.kernel.org kvm-owner@vger.kernel.org On Behalf Of Jason Wang Sent: Thursday, October 17, 2019 5:49 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; stefanha@redhat.com; Jason Wang jasowang@redhat.com Subject: [PATCH V4 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, 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

.../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 | 18 +++++++++++++++ drivers/vfio/mdev/mdev_driver.c | 22 +++++++++++++++++++ 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, 75 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index 25eb7d5b834b..f9a78d75a67a 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 in create() callback through::

• int mdev_set_class(struct mdev_device *mdev, 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..6420f0dbd31b 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_CLASS_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..cf2c013ae32f 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_CLASS_ID_VFIO); return 0;

}

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 5c0f53c6dde7..07c31070afeb 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_CLASS_ID_VFIO); return 0;

}

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index b558d4cfd082..3a9c52d71b4e 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -45,6 +45,16 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data) } EXPORT_SYMBOL(mdev_set_drvdata);

+/* Specify the class for the mdev device, this must be called during

• • create() callback.
• */

+void mdev_set_class(struct mdev_device *mdev, u16 id) {

• WARN_ON(mdev->class_id);
• mdev->class_id = id;

+} +EXPORT_SYMBOL(mdev_set_class);

struct device *mdev_dev(struct mdev_device *mdev) { return &mdev->dev; @@ -135,6 +145,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 +335,13 @@ int mdev_device_create(struct kobject *kobj, if (ret) goto ops_create_fail;

• if (!mdev->class_id) {

• ret = -EINVAL;
  

• WARN(1, "class id must be specified for device %s\n",
  

•      dev_name(dev));
  

• goto add_fail;
  

• }
• ret = device_add(&mdev->dev); if (ret) goto add_fail;

diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c index 0d3223aee20b..319d886ffaf7 100644 --- a/drivers/vfio/mdev/mdev_driver.c +++ b/drivers/vfio/mdev/mdev_driver.c @@ -69,8 +69,30 @@ 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;

+}

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

}; 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..7b24ee9cb8dd 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 const struct mdev_class_id id_table[] = {

• { MDEV_CLASS_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..78b69d09eb54 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) @@ -132,6 +134,7 @@ struct mdev_driver { void *mdev_get_drvdata(struct mdev_device *mdev); void mdev_set_drvdata(struct mdev_device *mdev, void *data); const guid_t *mdev_uuid(struct mdev_device *mdev); +void mdev_set_class(struct mdev_device *mdev, u16 id);

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_CLASS_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..115bc5074656 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_CLASS_ID_VFIO); return 0;

err_mem: diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index cc86bf6566e4..665614574d50 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_CLASS_ID_VFIO); return 0;

}

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index ce84a300a4da..90da12ff7fd9 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_CLASS_ID_VFIO); return 0;

}

-- 2.19.1

Reviewed-by: Parav Pandit parav@mellanox.com

-----Original Message----- From: Jason Wang jasowang@redhat.com Sent: Thursday, October 17, 2019 5:49 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; stefanha@redhat.com; Jason Wang jasowang@redhat.com Subject: [PATCH V4 2/6] modpost: add support for mdev class id

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 7b24ee9cb8dd..cb701cd646f0 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -125,6 +125,8 @@ static const 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

Reviewed-by: Parav Pandit parav@mellanox.com

On Thu, 17 Oct 2019 18:48:33 +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 | 25 +++++---- 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 | 18 +++++-- drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 37 ++++++------- include/linux/mdev.h | 45 ++++------------ include/linux/vfio_mdev.h | 52 +++++++++++++++++++ samples/vfio-mdev/mbochs.c | 20 ++++--- samples/vfio-mdev/mdpy.c | 20 ++++--- samples/vfio-mdev/mtty.c | 18 ++++--- 13 files changed, 184 insertions(+), 103 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 f9a78d75a67a..0cca84d19603 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,11 +152,22 @@ 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:

-* open: open callback of mediated device -* close: close callback of mediated device -* ioctl: ioctl callback of mediated device +As multiple types of mediated devices may be supported, the device +must set up the class id and the device specific callbacks in create()

s/in create()/in the create()/

+callback. E.g for vfio-mdev device it needs to be done through:

"Each class provides a helper function to do so; e.g. for vfio-mdev devices, the function to be called is:"

?

• int mdev_set_vfio_ops(struct mdev_device *mdev,

•                      const struct vfio_mdev_ops *vfio_ops);
  

+The class id (set to MDEV_CLASS_ID_VFIO) is used to match a device

"(set by this helper function to MDEV_CLASS_ID_VFIO)" ?

+with an mdev driver via its id table. The device specific callbacks +(specified in *ops) are obtainable via mdev_get_dev_ops() (for use by

"(specified in *vfio_ops by the caller)" ?

+the mdev bus driver). A vfio-mdev device (class id MDEV_CLASS_ID_VFIO) +uses the following device-specific ops:

+* open: open callback of vfio mediated device +* close: close callback of vfio mediated device +* ioctl: ioctl callback of vfio mediated device

• read : read emulation callback
• write: write emulation callback
• mmap: mmap emulation callback

@@ -167,10 +178,6 @@ 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 in create() callback through::

• int mdev_set_class(struct mdev_device *mdev, u16 id);

I'm wondering if this patch set should start out with introducing helper functions already (i.e. don't introduce mdev_set_class(), but start out with mdev_set_class_vfio() which will gain the *vfio_ops argument in this patch.)

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/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 3a9c52d71b4e..d0f3113c8071 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -45,15 +45,23 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data) } EXPORT_SYMBOL(mdev_set_drvdata);

-/* Specify the class for the mdev device, this must be called during

• • create() callback.

+/* Specify the VFIO device ops for the mdev device, this

• • must be called during create() callback for VFIO mdev device.
  */

/* * Specify the mdev device to be a VFIO mdev device, and set the * VFIO devices ops for it. This must be called from the create() * callback for VFIO mdev devices. */

?

-void mdev_set_class(struct mdev_device *mdev, u16 id) +void mdev_set_vfio_ops(struct mdev_device *mdev,

•        const struct vfio_mdev_device_ops *vfio_ops)
  

{ WARN_ON(mdev->class_id);

• mdev->class_id = id;

• mdev->class_id = MDEV_CLASS_ID_VFIO;
• mdev->device_ops = vfio_ops;

} -EXPORT_SYMBOL(mdev_set_class); +EXPORT_SYMBOL(mdev_set_vfio_ops);

+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) {

(...)

The code change looks good to me; I'm just wondering if we should introduce mdev_set_class() at all (see above).

On Thu, 17 Oct 2019 11:53:10 -0600 Alex Williamson alex.williamson@redhat.com wrote:

On Thu, 17 Oct 2019 17:07:55 +0200 Cornelia Huck cohuck@redhat.com wrote:

...

On Thu, 17 Oct 2019 18:48:33 +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 | 25 +++++---- 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 | 18 +++++-- drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 37 ++++++------- include/linux/mdev.h | 45 ++++------------ include/linux/vfio_mdev.h | 52 +++++++++++++++++++ samples/vfio-mdev/mbochs.c | 20 ++++--- samples/vfio-mdev/mdpy.c | 20 ++++--- samples/vfio-mdev/mtty.c | 18 ++++--- 13 files changed, 184 insertions(+), 103 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 f9a78d75a67a..0cca84d19603 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,11 +152,22 @@ 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:

-* open: open callback of mediated device -* close: close callback of mediated device -* ioctl: ioctl callback of mediated device +As multiple types of mediated devices may be supported, the device +must set up the class id and the device specific callbacks in create()

s/in create()/in the create()/

...

+callback. E.g for vfio-mdev device it needs to be done through:

"Each class provides a helper function to do so; e.g. for vfio-mdev devices, the function to be called is:"

?

...

• int mdev_set_vfio_ops(struct mdev_device *mdev,

•                      const struct vfio_mdev_ops *vfio_ops);
  

+The class id (set to MDEV_CLASS_ID_VFIO) is used to match a device

"(set by this helper function to MDEV_CLASS_ID_VFIO)" ?

...

+with an mdev driver via its id table. The device specific callbacks +(specified in *ops) are obtainable via mdev_get_dev_ops() (for use by

"(specified in *vfio_ops by the caller)" ?

...

+the mdev bus driver). A vfio-mdev device (class id MDEV_CLASS_ID_VFIO) +uses the following device-specific ops:

+* open: open callback of vfio mediated device +* close: close callback of vfio mediated device +* ioctl: ioctl callback of vfio mediated device

• read : read emulation callback
• write: write emulation callback
• mmap: mmap emulation callback

@@ -167,10 +178,6 @@ 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 in create() callback through::

• int mdev_set_class(struct mdev_device *mdev, u16 id);

I'm wondering if this patch set should start out with introducing helper functions already (i.e. don't introduce mdev_set_class(), but start out with mdev_set_class_vfio() which will gain the *vfio_ops argument in this patch.)

Yes, it would be cleaner, but is it really worth the churn? Correct me if I'm wrong, but I think we get to the same point after this patch and aside from the function name itself, the difference is really just that the class_id is briefly exposed to the parent driver, right? Thanks,

Yes, it does not really matter much. If there's no other reason to rework things, I'd just keep this as it is now.

-----Original Message----- From: Jason Wang jasowang@redhat.com Sent: Thursday, October 17, 2019 5:49 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; stefanha@redhat.com; Jason Wang jasowang@redhat.com Subject: [PATCH V4 3/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. 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 | 25 +++++---- 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 | 18 +++++-- drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 37 ++++++------- include/linux/mdev.h | 45 ++++------------ include/linux/vfio_mdev.h | 52 +++++++++++++++++++ samples/vfio-mdev/mbochs.c | 20 ++++--- samples/vfio-mdev/mdpy.c | 20 ++++--- samples/vfio-mdev/mtty.c | 18 ++++--- 13 files changed, 184 insertions(+), 103 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 f9a78d75a67a..0cca84d19603 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,11 +152,22 @@ 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:

-* open: open callback of mediated device -* close: close callback of mediated device -* ioctl: ioctl callback of mediated device +As multiple types of mediated devices may be supported, the device must +set up the class id and the device specific callbacks in create() +callback. E.g for vfio-mdev device it needs to be done through:

• int mdev_set_vfio_ops(struct mdev_device *mdev,

•                      const struct vfio_mdev_ops *vfio_ops);
  

+The class id (set to MDEV_CLASS_ID_VFIO) is used to match a device with +an mdev driver via its id table. The device specific callbacks +(specified in *ops) are obtainable via mdev_get_dev_ops() (for use by +the mdev bus driver). A vfio-mdev device (class id MDEV_CLASS_ID_VFIO) +uses the following device-specific ops:

+* open: open callback of vfio mediated device +* close: close callback of vfio mediated device +* ioctl: ioctl callback of vfio mediated device

• read : read emulation callback
• write: write emulation callback
• mmap: mmap emulation callback

@@ -167,10 +178,6 @@ 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 in create() callback through::

• int mdev_set_class(struct mdev_device *mdev, 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::

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 6420f0dbd31b..306f934c7857 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_CLASS_ID_VFIO);

• mdev_set_vfio_ops(mdev, &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 cf2c013ae32f..6e606cdb1aa9 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_CLASS_ID_VFIO);

• mdev_set_vfio_ops(mdev, &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 07c31070afeb..195c8f4c6f10 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_CLASS_ID_VFIO);

• mdev_set_vfio_ops(mdev, &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 3a9c52d71b4e..d0f3113c8071 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -45,15 +45,23 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data) } EXPORT_SYMBOL(mdev_set_drvdata);

-/* Specify the class for the mdev device, this must be called during

• • create() callback.

+/* Specify the VFIO device ops for the mdev device, this

• • must be called during create() callback for VFIO mdev device.
  */

-void mdev_set_class(struct mdev_device *mdev, u16 id)

I think you should keep the mdev_set_class API and let mdev_set_vfio_ops call it. Even though its two line function. The reason to avoid a code churn in short time with mlx5_core patch which will only call mdev_set_class(), as it doesn't have any ops yet.

+void mdev_set_vfio_ops(struct mdev_device *mdev,

•        const struct vfio_mdev_device_ops *vfio_ops)
  

{ WARN_ON(mdev->class_id);

• mdev->class_id = id;

• mdev->class_id = MDEV_CLASS_ID_VFIO;
• mdev->device_ops = vfio_ops;

} -EXPORT_SYMBOL(mdev_set_class); +EXPORT_SYMBOL(mdev_set_vfio_ops);

+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) { 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;

Instead of void *, this should be union vfio_ops and virtio_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 cb701cd646f0..6b6b92011436 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 78b69d09eb54..3d29e09e20c9 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -10,7 +10,13 @@ #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; +struct vfio_mdev_device_ops;

/*

• Called by the parent device driver to set the device which represents @@

-48,30 +54,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 +66,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,8 +107,11 @@ struct mdev_driver {

void *mdev_get_drvdata(struct mdev_device *mdev); void mdev_set_drvdata(struct mdev_device *mdev, void *data);

Unrelated line change for this patch. Please remove.

const guid_t *mdev_uuid(struct mdev_device *mdev); -void mdev_set_class(struct mdev_device *mdev, u16 id); +void mdev_set_vfio_ops(struct mdev_device *mdev,

•        const struct vfio_mdev_device_ops *vfio_ops); const
  

void +*mdev_get_dev_ops(struct mdev_device *mdev);

Since you have the set_vfio_ops, its better to have get_vfio_ops() and similarly get_virtio_ops.

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

This file name is confusing. vfio_mdev_ops.h is more accurate name of what it intends to achieve. If you see this file expanding scope beyond ops, this file name looks fine.

@@ -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 115bc5074656..37abdf1b27a8 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_CLASS_ID_VFIO);

• mdev_set_vfio_ops(mdev, &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 665614574d50..f21c795c40e4 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,7 @@ static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev) mdpy_reset(mdev);

mdpy_count++;

• mdev_set_class(mdev, MDEV_CLASS_ID_VFIO);

• mdev_set_vfio_ops(mdev, &vfio_mdev_ops); return 0;

}

@@ -726,12 +729,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 +738,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 90da12ff7fd9..b829f33b98f5 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_CLASS_ID_VFIO);

• mdev_set_vfio_ops(mdev, &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,

+};

Since hunk shows that vfio_dev_ops added new here, it is better to drop the extra white space mixed with tab alignment. Just have single white space like below.

.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

On 2019/10/18 上午1:53, Alex Williamson wrote:

On Thu, 17 Oct 2019 18:48:34 +0800 Jason Wangjasowang@redhat.com wrote:

...

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

Signed-off-by: Jason Wangjasowang@redhat.com

drivers/vfio/mdev/mdev_core.c | 12 +++ include/linux/mdev.h | 4 + include/linux/virtio_mdev.h | 151 ++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index d0f3113c8071..5834f6b7c7a5 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -57,6 +57,18 @@ void mdev_set_vfio_ops(struct mdev_device *mdev, } EXPORT_SYMBOL(mdev_set_vfio_ops);

+/* Specify the virtio device ops for the mdev device, this

• • must be called during create() callback for virtio mdev device.
• */

+void mdev_set_virtio_ops(struct mdev_device *mdev,

• 	 const struct virtio_mdev_device_ops *virtio_ops)
  

+{

• BUG_ON(mdev->class_id);

Nit, this one is a BUG_ON, but the vfio one is a WARN_ON. Thanks,

Alex

Let me fix in next version.

Thanks

On 2019/10/18 下午5:46, Cornelia Huck wrote:

On Thu, 17 Oct 2019 18:48:34 +0800 Jason Wang jasowang@redhat.com wrote:

...

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

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

drivers/vfio/mdev/mdev_core.c | 12 +++ include/linux/mdev.h | 4 + include/linux/virtio_mdev.h | 151 ++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index d0f3113c8071..5834f6b7c7a5 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -57,6 +57,18 @@ void mdev_set_vfio_ops(struct mdev_device *mdev, } EXPORT_SYMBOL(mdev_set_vfio_ops);

+/* Specify the virtio device ops for the mdev device, this

• • must be called during create() callback for virtio mdev device.
• */

Change this as for the vfio comment (last patch?)

Ok.

...

+void mdev_set_virtio_ops(struct mdev_device *mdev,

• 	 const struct virtio_mdev_device_ops *virtio_ops)
  

+{

• BUG_ON(mdev->class_id);

s/BUG_ON/WARN_ON/

Yes.

...

• mdev->class_id = MDEV_CLASS_ID_VIRTIO;
• mdev->device_ops = virtio_ops;

+} +EXPORT_SYMBOL(mdev_set_virtio_ops);

• const void *mdev_get_dev_ops(struct mdev_device *mdev) { return mdev->device_ops;

(...)

...

diff --git a/include/linux/virtio_mdev.h b/include/linux/virtio_mdev.h new file mode 100644 index 000000000000..b965b50f9b24 --- /dev/null +++ b/include/linux/virtio_mdev.h @@ -0,0 +1,151 @@ +/* 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)
    

Probably dumb question (have not read the next patches yet): Is this agnostic regarding split or packed queues?

Yes, it is to and to be more obvious, I use the terminology from recent spec.

...

• • @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 interrupt callback 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

s/get_features/set_features/

Will fix.

...

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

How is the vendor id defined? As for normal virtio-pci devices?

The vendor that provides this device. So something like this

I notice that MMIO also had this so it looks to me it's not pci specific.

...

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

What kind of 'features' are supposed to go in there? Are these bits, like you defined for VIRTIO_MDEV_F_VERSION_1 above?

It's the API or mdev features other than virtio features. It could be used by driver to determine the capability of the mdev device. Besides _F_VERSION_1, we may add dirty page tracking etc which means we need new device ops.

...

• • @get_generation: Get device generaton

• • 		@mdev: mediated device
    

• • 		Returns u32: device generation
    

Is that callback mandatory?

I think so, it's hard to emulate that completely in virtio-mdev transport.

Thanks

...

• */

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

+};

+void mdev_set_virtio_ops(struct mdev_device *mdev,

• 	 const struct virtio_mdev_device_ops *virtio_ops);
  

+#endif