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

On Tue, 24 Sep 2019 21:53: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, 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 parent with the match method for mdev bus.

Description needs to be revised from v1, class_id is no longer on the parent.

Yes, will fix.

...

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

Documentation/driver-api/vfio-mediated-device.rst | 3 +++ 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 | 7 +++++++ drivers/vfio/mdev/mdev_driver.c | 14 ++++++++++++++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 6 ++++++ include/linux/mdev.h | 8 ++++++++ include/linux/mod_devicetable.h | 8 ++++++++ samples/vfio-mdev/mbochs.c | 1 + samples/vfio-mdev/mdpy.c | 1 + samples/vfio-mdev/mtty.c | 1 + 13 files changed, 53 insertions(+)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index 25eb7d5b834b..a5bdc60d62a1 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,6 +167,7 @@ register itself with the mdev core driver:: extern int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops);

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

Unintended extra line? Doesn't seem to match surrounding formatting.

Calling mdev_set_class_id() as part of create seems relatively fundamental to the vendor driver with this change, it should be added to the documentation.

Right.

...

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 23aa3e50cbf8..f793252a3d2a 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_id(mdev, MDEV_ID_VFIO);

out: return ret; } diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index f0d71ab77c50..d258ef1fedb9 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_id(mdev, MDEV_ID_VFIO); return 0;

}

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 5c0f53c6dde7..2cfd96112aa0 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_id(mdev, MDEV_ID_VFIO); return 0;

}

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

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

• mdev->class_id = id;

+} +EXPORT_SYMBOL(mdev_set_class_id);

struct device *mdev_dev(struct mdev_device *mdev) { return &mdev->dev; @@ -135,6 +141,7 @@ static int mdev_device_remove_cb(struct device *dev, void *data)

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

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

diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c index 0d3223aee20b..b7c40ce86ee3 100644 --- a/drivers/vfio/mdev/mdev_driver.c +++ b/drivers/vfio/mdev/mdev_driver.c @@ -69,8 +69,22 @@ static int mdev_remove(struct device *dev) return 0; }

+static int mdev_match(struct device *dev, struct device_driver *drv) +{

• unsigned int i;
• struct mdev_device *mdev = to_mdev_device(dev);
• struct mdev_driver *mdrv = to_mdev_driver(drv);
• const struct mdev_class_id *ids = mdrv->id_table;
• for (i = 0; ids[i].id; i++)

• if (ids[i].id == mdev->class_id)
  

With the proposed API here, mdev->class_id can be NULL, do we allow devices to exist in that state or should mdev_device_create() generate an error if the .create() callback doesn't register a type? Or a warn_once and assume MDEV_ID_VFIO? The latter seems pretty sketchy when we get to patch 5/8. Thanks,

It looks to me the error from mdev_device_create() is better.

Thanks

Alex

...

• 	return 1;
  

• return 0;

+}

struct bus_type mdev_bus_type = { .name = "mdev",

• .match = mdev_match, .probe = mdev_probe, .remove = mdev_remove,

}; diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h index 7d922950caaf..c65f436c1869 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -33,6 +33,7 @@ struct mdev_device { struct kobject *type_kobj; struct device *iommu_device; bool active;

• u16 class_id;

};

#define to_mdev_device(dev) container_of(dev, struct mdev_device, dev) diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c index 30964a4e0a28..fd2a4d9a3f26 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -120,10 +120,16 @@ static void vfio_mdev_remove(struct device *dev) vfio_del_group_dev(dev); }

+static struct mdev_class_id id_table[] = {

• { MDEV_ID_VFIO },
• { 0 },

+};

static struct mdev_driver vfio_mdev_driver = { .name = "vfio_mdev", .probe = vfio_mdev_probe, .remove = vfio_mdev_remove,

• .id_table = id_table,

};

static int __init vfio_mdev_init(void) diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 0ce30ca78db0..3974650c074f 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -118,6 +118,7 @@ struct mdev_type_attribute mdev_type_attr_##_name = \

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

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

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

• const struct mdev_class_id *id_table;

};

#define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver)

void *mdev_get_drvdata(struct mdev_device *mdev); void mdev_set_drvdata(struct mdev_device *mdev, void *data); +void mdev_set_class_id(struct mdev_device *mdev, u16 id); const guid_t *mdev_uuid(struct mdev_device *mdev);

extern struct bus_type mdev_bus_type; @@ -145,4 +148,9 @@ struct device *mdev_parent_dev(struct mdev_device *mdev); struct device *mdev_dev(struct mdev_device *mdev); struct mdev_device *mdev_from_dev(struct device *dev);

+enum {

• MDEV_ID_VFIO = 1,
• /* New entries must be added here */

+};

#endif /* MDEV_H */ diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 5714fd35a83c..f32c6e44fb1a 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -821,4 +821,12 @@ struct wmi_device_id { const void *context; };

+/**

• • struct mdev_class_id - MDEV device class identifier
• • @id: Used to identify a specific class of device, e.g vfio-mdev device.
• */

+struct mdev_class_id {

• __u16 id;

+};

#endif /* LINUX_MOD_DEVICETABLE_H */ diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index ac5c8c17b1ff..8a8583c892b2 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_id(mdev, MDEV_ID_VFIO); return 0;

err_mem: diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index cc86bf6566e4..88d7e76f3836 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_id(mdev, MDEV_ID_VFIO); return 0;

}

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index 92e770a06ea2..4e0735143b69 100644 --- a/samples/vfio-mdev/mtty.c +++ b/samples/vfio-mdev/mtty.c @@ -770,6 +770,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_id(mdev, MDEV_ID_VFIO); return 0;

}

On 2019/9/25 下午4:28, Tian, Kevin wrote:

...

From: Jason Wang Sent: Tuesday, September 24, 2019 9:53 PM

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 parent with the match method for mdev bus.

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

Documentation/driver-api/vfio-mediated-device.rst | 3 +++ 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 | 7 +++++++ drivers/vfio/mdev/mdev_driver.c | 14 ++++++++++++++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 6 ++++++ include/linux/mdev.h | 8 ++++++++ include/linux/mod_devicetable.h | 8 ++++++++ samples/vfio-mdev/mbochs.c | 1 + samples/vfio-mdev/mdpy.c | 1 + samples/vfio-mdev/mtty.c | 1 + 13 files changed, 53 insertions(+)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index 25eb7d5b834b..a5bdc60d62a1 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,6 +167,7 @@ register itself with the mdev core driver:: extern int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops);

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

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 23aa3e50cbf8..f793252a3d2a 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_id(mdev, MDEV_ID_VFIO);

out: return ret; } diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index f0d71ab77c50..d258ef1fedb9 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_id(mdev, MDEV_ID_VFIO); return 0;

}

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 5c0f53c6dde7..2cfd96112aa0 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_id(mdev, MDEV_ID_VFIO); return 0;

}

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

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

• mdev->class_id = id;

+} +EXPORT_SYMBOL(mdev_set_class_id);

struct device *mdev_dev(struct mdev_device *mdev) { return &mdev->dev; @@ -135,6 +141,7 @@ static int mdev_device_remove_cb(struct device *dev, void *data)

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

• • @id: device id.

class id.

Right, will fix.

Thanks

On Tue, 24 Sep 2019 21:53:29 +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 | 4 +- MAINTAINERS | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 17 +++--- drivers/s390/cio/vfio_ccw_ops.c | 17 ++++-- drivers/s390/crypto/vfio_ap_ops.c | 13 +++-- drivers/vfio/mdev/mdev_core.c | 12 +++++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 37 ++++++------- include/linux/mdev.h | 42 ++++----------- include/linux/vfio_mdev.h | 52 +++++++++++++++++++ samples/vfio-mdev/mbochs.c | 19 ++++--- samples/vfio-mdev/mdpy.c | 19 ++++--- samples/vfio-mdev/mtty.c | 17 ++++-- 13 files changed, 168 insertions(+), 83 deletions(-) create mode 100644 include/linux/vfio_mdev.h

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index a5bdc60d62a1..d50425b368bb 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,7 +152,9 @@ callbacks per mdev parent device, per mdev type, or any other categorization. Vendor drivers are expected to be fully asynchronous in this respect or provide their own internal resource protection.)

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

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

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

• open: open callback of mediated device
• close: close callback of mediated device

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

VFIO PLATFORM DRIVER diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index f793252a3d2a..b274f5ee481f 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -42,6 +42,7 @@ #include <linux/kvm_host.h> #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/vfio_mdev.h> #include <linux/debugfs.h>

#include <linux/nospec.h> @@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu) vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device); }

+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops;

static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) { struct intel_vgpu *vgpu = NULL; @@ -679,6 +682,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) ret = 0;

mdev_set_class_id(mdev, MDEV_ID_VFIO);

• mdev_set_dev_ops(mdev, &intel_vfio_vgpu_dev_ops);

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

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

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

out: return ret; } @@ -1601,20 +1605,21 @@ static const struct attribute_group *intel_vgpu_groups[] = { NULL, };

-static struct mdev_parent_ops intel_vgpu_ops = {

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

+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = { .open = intel_vgpu_open, .release = intel_vgpu_release,

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

};

+static struct mdev_parent_ops intel_vgpu_ops = {

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

Alex

• .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 d258ef1fedb9..329d53c1f46b 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 = @@ -130,6 +133,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev) private->sch->schid.sch_no);

mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

}

@@ -575,11 +579,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 +587,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 2cfd96112aa0..3a89933f0d3e 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; @@ -344,6 +347,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) mutex_unlock(&matrix_dev->lock);

mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

}

@@ -1281,15 +1285,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 8764cf4a276d..6f35f2ced2c9 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -51,6 +51,18 @@ void mdev_set_class_id(struct mdev_device *mdev, u16 id) } EXPORT_SYMBOL(mdev_set_class_id);

+const void *mdev_get_dev_ops(struct mdev_device *mdev) +{

• return mdev->device_ops;

+} +EXPORT_SYMBOL(mdev_get_dev_ops);

+void mdev_set_dev_ops(struct mdev_device *mdev, const void *ops) +{

• mdev->device_ops = ops;

+} +EXPORT_SYMBOL(mdev_set_dev_ops);

struct device *mdev_dev(struct mdev_device *mdev) { return &mdev->dev; diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h index c65f436c1869..b666805f0b1f 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -34,6 +34,7 @@ struct mdev_device { struct device *iommu_device; bool active; u16 class_id;

• const void *device_ops;

};

#define to_mdev_device(dev) container_of(dev, struct mdev_device, dev) diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c index 891cf83a2d9a..95efa054442f 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -14,6 +14,7 @@ #include <linux/slab.h> #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/vfio_mdev.h>

#include "mdev_private.h"

@@ -24,16 +25,16 @@ static int vfio_mdev_open(void *device_data) { struct mdev_device *mdev = device_data;

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); int ret;

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

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

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

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

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

@@ -43,10 +44,10 @@ static int vfio_mdev_open(void *device_data) static void vfio_mdev_release(void *device_data) { struct mdev_device *mdev = device_data;

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);

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

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

• if (likely(ops->release))

• ops->release(mdev);
  
  

  module_put(THIS_MODULE);

} @@ -55,47 +56,47 @@ static long vfio_mdev_unlocked_ioctl(void *device_data, unsigned int cmd, unsigned long arg) { struct mdev_device *mdev = device_data;

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);

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

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

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

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

}

static ssize_t vfio_mdev_read(void *device_data, char __user *buf, size_t count, loff_t *ppos) { struct mdev_device *mdev = device_data;

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);

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

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

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

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

}

static ssize_t vfio_mdev_write(void *device_data, const char __user *buf, size_t count, loff_t *ppos) { struct mdev_device *mdev = device_data;

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);

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

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

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

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

}

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

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);

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

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

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

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

}

static const struct vfio_device_ops vfio_mdev_dev_ops = { diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3974650c074f..3414307311f1 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -10,6 +10,11 @@ #ifndef MDEV_H #define MDEV_H

+#include <linux/types.h> +#include <linux/device.h> +#include <linux/mdev.h> +#include <uapi/linux/uuid.h>

struct mdev_device;

/* @@ -48,30 +53,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 +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,6 +107,8 @@ struct mdev_driver {

void *mdev_get_drvdata(struct mdev_device *mdev); void mdev_set_drvdata(struct mdev_device *mdev, void *data); +const void *mdev_get_dev_ops(struct mdev_device *mdev); +void mdev_set_dev_ops(struct mdev_device *mdev, const void *ops); void mdev_set_class_id(struct mdev_device *mdev, u16 id); const guid_t *mdev_uuid(struct mdev_device *mdev);

diff --git a/include/linux/vfio_mdev.h b/include/linux/vfio_mdev.h new file mode 100644 index 000000000000..3907c5371c2b --- /dev/null +++ b/include/linux/vfio_mdev.h @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • VFIO Mediated device definition
• */

+#ifndef VFIO_MDEV_H +#define VFIO_MDEV_H

+#include <linux/mdev.h>

+/**

• • struct vfio_mdev_device_ops - Structure to be registered for each
• • mdev device to register the device to vfio-mdev module.
• • @open: Open mediated device.

• • 	@mdev: mediated device.
    

• • 	Returns integer: success (0) or error (< 0)
    

• • @release: release mediated device

• • 	@mdev: mediated device.
    

• • @read: Read emulation callback

• • 	@mdev: mediated device structure
    

• • 	@buf: read buffer
    

• • 	@count: number of bytes to read
    

• • 	@ppos: address.
    

• • 	Retuns number on bytes read on success or error.
    

• • @write: Write emulation callback

• • 	@mdev: mediated device structure
    

• • 	@buf: write buffer
    

• • 	@count: number of bytes to be written
    

• • 	@ppos: address.
    

• • 	Retuns number on bytes written on success or error.
    

• • @ioctl: IOCTL callback

• • 	@mdev: mediated device structure
    

• • 	@cmd: ioctl command
    

• • 	@arg: arguments to ioctl
    

• • @mmap: mmap callback

• • 	@mdev: mediated device structure
    

• • 	@vma: vma structure
    

• */

+struct vfio_mdev_device_ops {

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

• 	size_t count, loff_t *ppos);
  

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

• 	 size_t count, loff_t *ppos);
  

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

• 	 unsigned long arg);
  

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

+};

+#endif diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index 8a8583c892b2..b4bf29c6136c 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); @@ -562,6 +565,7 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev)

mbochs_used_mbytes += type->mbytes; mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

err_mem: @@ -1419,12 +1423,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 +1432,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 88d7e76f3836..80c2df531326 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); @@ -270,6 +273,7 @@ static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev)

mdpy_count++; mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

}

@@ -726,12 +730,7 @@ static struct attribute_group *mdev_type_groups[] = { NULL, };

-static const struct mdev_parent_ops mdev_fops = {

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

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

+static const struct mdev_parent_ops mdev_fops = {

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

+};

static const struct file_operations vd_fops = { .owner = THIS_MODULE, }; diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index 4e0735143b69..2db860ccb02c 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> @@ -723,6 +724,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; @@ -771,6 +774,7 @@ static int mtty_create(struct kobject *kobj, struct mdev_device *mdev) mutex_unlock(&mdev_list_lock);

mdev_set_class_id(mdev, MDEV_ID_VFIO);

• mdev_set_dev_ops(mdev, &vfio_dev_ops); return 0;

}

@@ -1411,6 +1415,14 @@ static struct attribute_group *mdev_type_groups[] = { NULL, };

+static const struct vfio_mdev_device_ops vfio_dev_ops = {

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

+};

static const struct mdev_parent_ops mdev_fops = { .owner = THIS_MODULE, .dev_attr_groups = mtty_dev_groups, @@ -1418,11 +1430,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)

On Wed, Sep 25, 2019 at 4:52 AM Tian, Kevin kevin.tian@intel.com wrote:

...

From: Alex Williamson Sent: Wednesday, September 25, 2019 7:07 AM

On Tue, 24 Sep 2019 21:53:29 +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 | 4 +- MAINTAINERS | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 17 +++--- drivers/s390/cio/vfio_ccw_ops.c | 17 ++++-- drivers/s390/crypto/vfio_ap_ops.c | 13 +++-- drivers/vfio/mdev/mdev_core.c | 12 +++++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 37 ++++++------- include/linux/mdev.h | 42 ++++----------- include/linux/vfio_mdev.h | 52 +++++++++++++++++++ samples/vfio-mdev/mbochs.c | 19 ++++--- samples/vfio-mdev/mdpy.c | 19 ++++--- samples/vfio-mdev/mtty.c | 17 ++++-- 13 files changed, 168 insertions(+), 83 deletions(-) create mode 100644 include/linux/vfio_mdev.h

diff --git a/Documentation/driver-api/vfio-mediated-device.rst

b/Documentation/driver-api/vfio-mediated-device.rst

...

index a5bdc60d62a1..d50425b368bb 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,7 +152,9 @@ callbacks per mdev parent device, per mdev type,

or any other categorization.

...

Vendor drivers are expected to be fully asynchronous in this respect

or

...

...

provide their own internal resource protection.)

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

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

there is also a mistake in include/Linux/mdev.h, where device_ops is still part of mdev_parent_ops in the comment line.

...

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

...

• open: open callback of mediated device
• close: close callback of mediated device

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

VFIO PLATFORM DRIVER diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c

b/drivers/gpu/drm/i915/gvt/kvmgt.c

...

index f793252a3d2a..b274f5ee481f 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -42,6 +42,7 @@ #include <linux/kvm_host.h> #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/vfio_mdev.h> #include <linux/debugfs.h>

#include <linux/nospec.h> @@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu) vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device); }

+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops;

static int intel_vgpu_create(struct kobject *kobj, struct mdev_device

*mdev)

...

{ struct intel_vgpu *vgpu = NULL; @@ -679,6 +682,7 @@ static int intel_vgpu_create(struct kobject *kobj,

struct mdev_device *mdev)

...

ret = 0;

mdev_set_class_id(mdev, MDEV_ID_VFIO);

• mdev_set_dev_ops(mdev, &intel_vfio_vgpu_dev_ops);

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

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

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

or just make it explicit? e.g.

mdev_set_class(mdev, MDEV_ID_VFIO, &intel_vfio_vgpu_dev_ops);

...

...

out: return ret; } @@ -1601,20 +1605,21 @@ static const struct attribute_group

*intel_vgpu_groups[] = {

...

NULL,

};

-static struct mdev_parent_ops intel_vgpu_ops = {

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

+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = { .open = intel_vgpu_open, .release = intel_vgpu_release,

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

};

+static struct mdev_parent_ops intel_vgpu_ops = {

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

Alex

...

• .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 d258ef1fedb9..329d53c1f46b 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 = @@ -130,6 +133,7 @@ static int vfio_ccw_mdev_create(struct kobject

*kobj, struct mdev_device *mdev)

...

                   private->sch->schid.sch_no);

mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

}

@@ -575,11 +579,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 +587,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 2cfd96112aa0..3a89933f0d3e 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; @@ -344,6 +347,7 @@ static int vfio_ap_mdev_create(struct kobject

*kobj, struct mdev_device *mdev)

...

mutex_unlock(&matrix_dev->lock);

mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

}

@@ -1281,15 +1285,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 8764cf4a276d..6f35f2ced2c9 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -51,6 +51,18 @@ void mdev_set_class_id(struct mdev_device *mdev,

u16 id)

...

} EXPORT_SYMBOL(mdev_set_class_id);

+const void *mdev_get_dev_ops(struct mdev_device *mdev) +{

• return mdev->device_ops;

+} +EXPORT_SYMBOL(mdev_get_dev_ops);

+void mdev_set_dev_ops(struct mdev_device *mdev, const void *ops) +{

• mdev->device_ops = ops;

+} +EXPORT_SYMBOL(mdev_set_dev_ops);

struct device *mdev_dev(struct mdev_device *mdev) { return &mdev->dev; diff --git a/drivers/vfio/mdev/mdev_private.h

b/drivers/vfio/mdev/mdev_private.h

...

index c65f436c1869..b666805f0b1f 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -34,6 +34,7 @@ struct mdev_device { struct device *iommu_device; bool active; u16 class_id;

• const void *device_ops;

};

#define to_mdev_device(dev) container_of(dev, struct

mdev_device, dev)

...

diff --git a/drivers/vfio/mdev/vfio_mdev.c

b/drivers/vfio/mdev/vfio_mdev.c

...

index 891cf83a2d9a..95efa054442f 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -14,6 +14,7 @@ #include <linux/slab.h> #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/vfio_mdev.h>

#include "mdev_private.h"

@@ -24,16 +25,16 @@ static int vfio_mdev_open(void *device_data) { struct mdev_device *mdev = device_data;

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops =

mdev_get_dev_ops(mdev);

...

int ret;

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

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

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

RJM>] My understanding lately is that this call to try_module_get(THIS_MODULE) is no longer needed as is considered as a latent bug. Quote from https://stackoverflow.com/questions/1741415/linux-kernel-modules-when-to-use... : There are a number of uses of try_module_get(THIS_MODULE) in the kernel source but most if not all of them are latent bugs that should be cleaned up.

...

...

• 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 3974650c074f..3414307311f1 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -10,6 +10,11 @@ #ifndef MDEV_H #define MDEV_H

+#include <linux/types.h> +#include <linux/device.h> +#include <linux/mdev.h> +#include <uapi/linux/uuid.h>

struct mdev_device;

/* @@ -48,30 +53,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 +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,6 +107,8 @@ struct mdev_driver {

void *mdev_get_drvdata(struct mdev_device *mdev); void mdev_set_drvdata(struct mdev_device *mdev, void *data); +const void *mdev_get_dev_ops(struct mdev_device *mdev); +void mdev_set_dev_ops(struct mdev_device *mdev, const void *ops); void mdev_set_class_id(struct mdev_device *mdev, u16 id); const guid_t *mdev_uuid(struct mdev_device *mdev);

diff --git a/include/linux/vfio_mdev.h b/include/linux/vfio_mdev.h new file mode 100644 index 000000000000..3907c5371c2b --- /dev/null +++ b/include/linux/vfio_mdev.h @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • VFIO Mediated device definition
• */

+#ifndef VFIO_MDEV_H +#define VFIO_MDEV_H

+#include <linux/mdev.h>

+/**

• • struct vfio_mdev_device_ops - Structure to be registered for each
• • mdev device to register the device to vfio-mdev module.
• • @open: Open mediated device.

• •             @mdev: mediated device.
    

• •             Returns integer: success (0) or error (< 0)
    

• • @release: release mediated device

• •             @mdev: mediated device.
    

• • @read: Read emulation callback

• •             @mdev: mediated device structure
    

• •             @buf: read buffer
    

• •             @count: number of bytes to read
    

• •             @ppos: address.
    

• •             Retuns number on bytes read on success or error.
    

• • @write: Write emulation callback

• •             @mdev: mediated device structure
    

• •             @buf: write buffer
    

• •             @count: number of bytes to be written
    

• •             @ppos: address.
    

• •             Retuns number on bytes written on success or error.
    

• • @ioctl: IOCTL callback

• •             @mdev: mediated device structure
    

• •             @cmd: ioctl command
    

• •             @arg: arguments to ioctl
    

• • @mmap: mmap callback

• •             @mdev: mediated device structure
    

• •             @vma: vma structure
    

• */

+struct vfio_mdev_device_ops {

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

•               size_t count, loff_t *ppos);
  

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

•                size_t count, loff_t *ppos);
  

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

•                unsigned long arg);
  

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

*vma);

...

+};

+#endif diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index 8a8583c892b2..b4bf29c6136c 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); @@ -562,6 +565,7 @@ static int mbochs_create(struct kobject *kobj,

struct mdev_device *mdev)

...

mbochs_used_mbytes += type->mbytes;
mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

err_mem: @@ -1419,12 +1423,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 +1432,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 88d7e76f3836..80c2df531326 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); @@ -270,6 +273,7 @@ static int mdpy_create(struct kobject *kobj, struct

mdev_device *mdev)

...

mdpy_count++;
mdev_set_class_id(mdev, MDEV_ID_VFIO);

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

}

@@ -726,12 +730,7 @@ static struct attribute_group

*mdev_type_groups[] = {

...

NULL,

};

-static const struct mdev_parent_ops mdev_fops = {

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

+static const struct vfio_mdev_device_ops vfio_mdev_ops = { .open = mdpy_open, .release = mdpy_close, .read = mdpy_read, @@ -740,6 +739,14 @@ static const struct mdev_parent_ops mdev_fops

= {

...

.mmap                   = mdpy_mmap,

};

+static const struct mdev_parent_ops mdev_fops = {

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

+};

static const struct file_operations vd_fops = { .owner = THIS_MODULE, }; diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index 4e0735143b69..2db860ccb02c 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> @@ -723,6 +724,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; @@ -771,6 +774,7 @@ static int mtty_create(struct kobject *kobj, struct

mdev_device *mdev)

...

mutex_unlock(&mdev_list_lock);

mdev_set_class_id(mdev, MDEV_ID_VFIO);

• mdev_set_dev_ops(mdev, &vfio_dev_ops); return 0;

}

@@ -1411,6 +1415,14 @@ static struct attribute_group

*mdev_type_groups[] = {

...

NULL,

};

+static const struct vfio_mdev_device_ops vfio_dev_ops = {

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

+};

static const struct mdev_parent_ops mdev_fops = { .owner = THIS_MODULE, .dev_attr_groups = mtty_dev_groups, @@ -1418,11 +1430,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)

---

intel-gvt-dev mailing list intel-gvt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

On Wed, Sep 25, 2019 at 10:30:28AM -0600, Alex Williamson wrote:

On Wed, 25 Sep 2019 10:11:00 -0400 Rob Miller rob.miller@broadcom.com wrote:

...

...

...

On Tue, 24 Sep 2019 21:53:29 +0800 Jason Wang jasowang@redhat.com wrote:

...

diff --git a/drivers/vfio/mdev/vfio_mdev.c

b/drivers/vfio/mdev/vfio_mdev.c

...

index 891cf83a2d9a..95efa054442f 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -14,6 +14,7 @@ #include <linux/slab.h> #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/vfio_mdev.h>

#include "mdev_private.h"

@@ -24,16 +25,16 @@ static int vfio_mdev_open(void *device_data) { struct mdev_device *mdev = device_data;

• struct mdev_parent *parent = mdev->parent;

• const struct vfio_mdev_device_ops *ops =

mdev_get_dev_ops(mdev);

...

int ret;

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

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

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

RJM>] My understanding lately is that this call to try_module_get(THIS_MODULE) is no longer needed as is considered as a latent bug. Quote from https://stackoverflow.com/questions/1741415/linux-kernel-modules-when-to-use... : There are a number of uses of try_module_get(THIS_MODULE) in the kernel source but most if not all of them are latent bugs that should be cleaned up.

This use seems to fall exactly into the case where it is necessary, the open here is not a direct VFS call, it's an internal interface between modules. The user is interacting with filesystem objects from the vfio module and the module reference we're trying to acquire here is to the vfio-mdev module. Thanks,

Alex

I think the latent bug refers not to module get per se, but to the module_put tied to it. E.g.:

static void vfio_mdev_release(void *device_data) { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

if (likely(parent->ops->release)) parent->ops->release(mdev);

module_put(THIS_MODULE);

Does anything prevent the module from unloading at this point? if not then ...

}

it looks like the implicit return (with instructions for argument pop and functuon return) here can get overwritten on module unload, causing a crash when executed.

IOW there's generally no way for module to keep a reference to itself: it can take a reference but it needs someone else to keep it and put.

On Thu, Sep 26, 2019 at 10:26:08AM -0600, Alex Williamson wrote:

On Thu, 26 Sep 2019 11:46:55 -0400 "Michael S. Tsirkin" mst@redhat.com wrote:

...

On Wed, Sep 25, 2019 at 10:30:28AM -0600, Alex Williamson wrote:

...

On Wed, 25 Sep 2019 10:11:00 -0400 Rob Miller rob.miller@broadcom.com wrote:

...

...

...

On Tue, 24 Sep 2019 21:53:29 +0800 Jason Wang jasowang@redhat.com wrote: > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > index 891cf83a2d9a..95efa054442f 100644 > --- a/drivers/vfio/mdev/vfio_mdev.c > +++ b/drivers/vfio/mdev/vfio_mdev.c > @@ -14,6 +14,7 @@ > #include <linux/slab.h> > #include <linux/vfio.h> > #include <linux/mdev.h> > +#include <linux/vfio_mdev.h> > > #include "mdev_private.h" > > @@ -24,16 +25,16 @@ > static int vfio_mdev_open(void *device_data) > { > struct mdev_device *mdev = device_data; > - struct mdev_parent *parent = mdev->parent; > + const struct vfio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); > int ret; > > - if (unlikely(!parent->ops->open)) > + if (unlikely(!ops->open)) > return -EINVAL; > > if (!try_module_get(THIS_MODULE)) > return -ENODEV;

RJM>] My understanding lately is that this call to try_module_get(THIS_MODULE) is no longer needed as is considered as a latent bug. Quote from https://stackoverflow.com/questions/1741415/linux-kernel-modules-when-to-use... : There are a number of uses of try_module_get(THIS_MODULE) in the kernel source but most if not all of them are latent bugs that should be cleaned up.

This use seems to fall exactly into the case where it is necessary, the open here is not a direct VFS call, it's an internal interface between modules. The user is interacting with filesystem objects from the vfio module and the module reference we're trying to acquire here is to the vfio-mdev module. Thanks,

Alex

I think the latent bug refers not to module get per se, but to the module_put tied to it. E.g.:

static void vfio_mdev_release(void *device_data) { struct mdev_device *mdev = device_data; struct mdev_parent *parent = mdev->parent;

    if (likely(parent->ops->release))
            parent->ops->release(mdev);

    module_put(THIS_MODULE);

Does anything prevent the module from unloading at this point? if not then ...

}

it looks like the implicit return (with instructions for argument pop and functuon return) here can get overwritten on module unload, causing a crash when executed.

IOW there's generally no way for module to keep a reference to itself: it can take a reference but it needs someone else to keep it and put.

I'd always assumed this would exit cleanly, but perhaps there is a latent race there. In any case, taking a module reference within the module in this case is better than not doing so, as the latter would potentially allow the module to be removed at any point in time, while the former only seems to expose acquire and release gaps. Add it to the todo list. Thanks,

Alex

Right. I agree with the stack overflow quote: as this example seems to show this is a latent bug.

But I also agree that just removing the reference isn't the right way to clean it up.

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

On Tue, 24 Sep 2019 21:53:29 +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 | 4 +- MAINTAINERS | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 17 +++--- drivers/s390/cio/vfio_ccw_ops.c | 17 ++++-- drivers/s390/crypto/vfio_ap_ops.c | 13 +++-- drivers/vfio/mdev/mdev_core.c | 12 +++++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 37 ++++++------- include/linux/mdev.h | 42 ++++----------- include/linux/vfio_mdev.h | 52 +++++++++++++++++++ samples/vfio-mdev/mbochs.c | 19 ++++--- samples/vfio-mdev/mdpy.c | 19 ++++--- samples/vfio-mdev/mtty.c | 17 ++++-- 13 files changed, 168 insertions(+), 83 deletions(-) create mode 100644 include/linux/vfio_mdev.h

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index a5bdc60d62a1..d50425b368bb 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,7 +152,9 @@ callbacks per mdev parent device, per mdev type, or any other categorization. Vendor drivers are expected to be fully asynchronous in this respect or provide their own internal resource protection.)

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

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

Yes.

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

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

...

• open: open callback of mediated device
• close: close callback of mediated device

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

VFIO PLATFORM DRIVER diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index f793252a3d2a..b274f5ee481f 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -42,6 +42,7 @@ #include <linux/kvm_host.h> #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/vfio_mdev.h> #include <linux/debugfs.h>

#include <linux/nospec.h> @@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu) vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device); }

+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops;

static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) { struct intel_vgpu *vgpu = NULL; @@ -679,6 +682,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) ret = 0;

mdev_set_class_id(mdev, MDEV_ID_VFIO);

• mdev_set_dev_ops(mdev, &intel_vfio_vgpu_dev_ops);

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

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

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

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

...

out: return ret; } @@ -1601,20 +1605,21 @@ static const struct attribute_group *intel_vgpu_groups[] = { NULL, };

-static struct mdev_parent_ops intel_vgpu_ops = {

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

+static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = { .open = intel_vgpu_open, .release = intel_vgpu_release,

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

};

+static struct mdev_parent_ops intel_vgpu_ops = {

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

Alex

Ok, let me fix.

Thanks

On Tue, 24 Sep 2019 21:53:30 +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

include/linux/mdev.h | 2 + include/linux/virtio_mdev.h | 145 ++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3414307311f1..73ac27b3b868 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -126,6 +126,8 @@ struct mdev_device *mdev_from_dev(struct device *dev);

enum { MDEV_ID_VFIO = 1,

• MDEV_ID_VIRTIO = 2,
• MDEV_ID_VHOST = 3,

MDEV_ID_VHOST isn't used yet here. Also, given the strong interdependence between the class_id and the ops structure, we might wand to define them in the same place. Thanks,

Alex

/* New entries must be added here */ };

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

+struct virtio_mdev_callback {

• irqreturn_t (*callback)(void *data);
• void *private;

+};

+/**

• • struct vfio_mdev_device_ops - Structure to be registered for each
• • mdev device to register the device to virtio-mdev module.
• • @set_vq_address: Set the address of virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@desc_area: address of desc area
    

• • 		@driver_area: address of driver area
    

• • 		@device_area: address of device area
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @set_vq_num: Set the size of virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@num: the size of virtqueue
    

• • @kick_vq: Kick the virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • @set_vq_cb: Set the interrut calback function for

• • 		a virtqueue
    

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@cb: virtio-mdev interrupt callback structure
    

• • @set_vq_ready: Set ready status for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@ready: ready (true) not ready(false)
    

• • @get_vq_ready: Get ready status for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		Returns boolean: ready (true) or not (false)
    

• • @set_vq_state: Set the state for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@state: virtqueue state (last_avail_idx)
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @get_vq_state: Get the state for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		Returns virtqueue state (last_avail_idx)
    

• • @get_vq_align: Get the virtqueue align requirement

• • 		for the device
    

• • 		@mdev: mediated device
    

• • 		Returns virtqueue algin requirement
    

• • @get_features: Get virtio features supported by the device

• • 		@mdev: mediated device
    

• • 		Returns the features support by the
    

• • 		device
    

• • @get_features: Set virtio features supported by the driver

• • 		@mdev: mediated device
    

• • 		@features: feature support by the driver
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @set_config_cb: Set the config interrupt callback

• • 		@mdev: mediated device
    

• • 		@cb: virtio-mdev interrupt callback structure
    

• • @get_device_id: Get virtio device id

• • 		@mdev: mediated device
    

• • 		Returns u32: virtio device id
    

• • @get_vendor_id: Get virtio vendor id

• • 		@mdev: mediated device
    

• • 		Returns u32: virtio vendor id
    

• • @get_status: Get the device status

• • 		@mdev: mediated device
    

• • 		Returns u8: virtio device status
    

• • @set_status: Set the device status

• • 		@mdev: mediated device
    

• • 		@status: virtio device status
    

• • @get_config: Read from device specific confiugration 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 confiugration 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_version: Get the version of virtio mdev device

• • 		@mdev: mediated device
    

• • 		Returns integer: version of the device
    

• • @get_generation: Get device generaton

• • 		@mdev: mediated device
    

• • 		Returns u32: device generation
    

• */

+struct virtio_mdev_device_ops {

• /* Virtqueue ops */
• int (*set_vq_address)(struct mdev_device *mdev,

• 	      u16 idx, u64 desc_area, u64 driver_area,
  

• 	      u64 device_area);
  

• void (*set_vq_num)(struct mdev_device *mdev, u16 idx, u32 num);
• void (*kick_vq)(struct mdev_device *mdev, u16 idx);
• void (*set_vq_cb)(struct mdev_device *mdev, u16 idx,

• 	  struct virtio_mdev_callback *cb);
  

• void (*set_vq_ready)(struct mdev_device *mdev, u16 idx, bool ready);
• bool (*get_vq_ready)(struct mdev_device *mdev, u16 idx);
• int (*set_vq_state)(struct mdev_device *mdev, u16 idx, u64 state);
• u64 (*get_vq_state)(struct mdev_device *mdev, u16 idx);
• /* Device ops */
• u16 (*get_vq_align)(struct mdev_device *mdev);
• u64 (*get_features)(struct mdev_device *mdev);
• int (*set_features)(struct mdev_device *mdev, u64 features);
• void (*set_config_cb)(struct mdev_device *mdev,

• 	      struct virtio_mdev_callback *cb);
  

• u16 (*get_queue_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);
  

• int (*get_version)(struct mdev_device *mdev);
• u32 (*get_generation)(struct mdev_device *mdev);

+};

+#endif

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

On Tue, 24 Sep 2019 21:53:30 +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

include/linux/mdev.h | 2 + include/linux/virtio_mdev.h | 145 ++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3414307311f1..73ac27b3b868 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -126,6 +126,8 @@ struct mdev_device *mdev_from_dev(struct device *dev);

enum { MDEV_ID_VFIO = 1,

• MDEV_ID_VIRTIO = 2,
• MDEV_ID_VHOST = 3,

MDEV_ID_VHOST isn't used yet here. Also, given the strong interdependence between the class_id and the ops structure, we might wand to define them in the same place. Thanks,

Alex

Rethink about this,  I believe it's better to define the device ops in their own headers, and one set of device ops could be used for two classes (e.g both virtio and vhost). And to avoid a duplicated ID definition. I tend to keep this in the common mdev.h header.

Thanks

On Fri, 27 Sep 2019 16:25:13 +0000 Parav Pandit parav@mellanox.com wrote:

Hi Alex,

...

-----Original Message----- From: Alex Williamson alex.williamson@redhat.com Sent: Tuesday, September 24, 2019 6:07 PM To: Jason Wang jasowang@redhat.com Cc: kvm@vger.kernel.org; linux-s390@vger.kernel.org; linux- kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; intel- gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; kwankhede@nvidia.com; mst@redhat.com; tiwei.bie@intel.com; virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; 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 Subject: Re: [PATCH V2 6/8] mdev: introduce virtio device and its device ops

On Tue, 24 Sep 2019 21:53:30 +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

include/linux/mdev.h | 2 + include/linux/virtio_mdev.h | 145 ++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3414307311f1..73ac27b3b868 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -126,6 +126,8 @@ struct mdev_device *mdev_from_dev(struct device *dev);

enum { MDEV_ID_VFIO = 1,

• MDEV_ID_VIRTIO = 2,
• MDEV_ID_VHOST = 3,

MDEV_ID_VHOST isn't used yet here. Also, given the strong interdependence between the class_id and the ops structure, we might wand to define them in the same place. Thanks,

When mlx5_core creates mdevs (parent->ops->create() and it wants to bind to mlx5 mdev driver (which does mdev_register_driver()), mlx5 core driver will publish MDEV_ID_MLX5_NET defined in central place as include/linux/mdev.h without any ops structure. Because such ops are not relevant. It uses usual, standard ops probe() remove() on the mdev (similar to a regular PCI device). So for VHOST case ops may be closely related to ID, but not for other type of ID.

Just want to make sure, that scope of ID covers this case.

AIUI, these device-ops are primarily meant to have 1:N multiplexing of the mdev bus driver. One mdev bus driver supports N vendor drivers via a common "protocol" defined by this structure. vfio-mdev supports GVT-g, GRID, and several sample drivers. I think Jason and Tiwei are attempting something similar if we have multiple vendors that may provide virtio/vhost parent drivers. If you have a 1:1 model with mlx5 where you're not trying to abstract a common channel between the mdev bus driver and the mdev vendor driver, then I suppose you might not use the device-ops capabilities of the mdev-core. Did I interpret the question correctly? I think that's probably fine, mdev-core shouldn't have any dependencies on the device-ops and we shouldn't really be dictating the bus/vendor link through mdev. Thanks,

Alex

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

On Tue, 24 Sep 2019 21:53:30 +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

include/linux/mdev.h | 2 + include/linux/virtio_mdev.h | 145 ++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3414307311f1..73ac27b3b868 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -126,6 +126,8 @@ struct mdev_device *mdev_from_dev(struct device *dev);

enum { MDEV_ID_VFIO = 1,

• MDEV_ID_VIRTIO = 2,
• MDEV_ID_VHOST = 3,

MDEV_ID_VHOST isn't used yet here. Also, given the strong interdependence between the class_id and the ops structure, we might wand to define them in the same place. Thanks,

Alex

Rethink about this, consider we may have more types of devices supported in the future, moving all device_ops to mdev.h seems unnecessary. I prefer to keep the device_ops into their own headers.

Thanks

On 2019/9/25 下午5:09, Tian, Kevin wrote:

...

From: Jason Wang [mailto:jasowang@redhat.com] Sent: Tuesday, September 24, 2019 9:54 PM

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

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

include/linux/mdev.h | 2 + include/linux/virtio_mdev.h | 145 ++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3414307311f1..73ac27b3b868 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -126,6 +126,8 @@ struct mdev_device *mdev_from_dev(struct device *dev);

enum { MDEV_ID_VFIO = 1,

• MDEV_ID_VIRTIO = 2,
• MDEV_ID_VHOST = 3, /* New entries must be added here */ };

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

Just be curious. is this version identical to virtio spec version that below callbacks are created for, or just irrelevant?

It could be a hint but basically it's a way for userspace driver compatibility. For kernel we don't need this.

...

+struct virtio_mdev_callback {

• irqreturn_t (*callback)(void *data);
• void *private;

+};

+/**

• • struct vfio_mdev_device_ops - Structure to be registered for each
• • mdev device to register the device to virtio-mdev module.
• • @set_vq_address: Set the address of virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@desc_area: address of desc area
    

• • 		@driver_area: address of driver area
    

• • 		@device_area: address of device area
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @set_vq_num: Set the size of virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@num: the size of virtqueue
    

• • @kick_vq: Kick the virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • @set_vq_cb: Set the interrut calback function for

• • 		a virtqueue
    

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@cb: virtio-mdev interrupt callback
    

structure

• • @set_vq_ready: Set ready status for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@ready: ready (true) not ready(false)
    

• • @get_vq_ready: Get ready status for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		Returns boolean: ready (true) or not (false)
    

• • @set_vq_state: Set the state for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@state: virtqueue state (last_avail_idx)
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @get_vq_state: Get the state for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		Returns virtqueue state (last_avail_idx)
    

• • @get_vq_align: Get the virtqueue align requirement

• • 		for the device
    

• • 		@mdev: mediated device
    

• • 		Returns virtqueue algin requirement
    

• • @get_features: Get virtio features supported by the device

• • 		@mdev: mediated device
    

• • 		Returns the features support by the
    

• • 		device
    

• • @get_features: Set virtio features supported by the driver

• • 		@mdev: mediated device
    

• • 		@features: feature support by the driver
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @set_config_cb: Set the config interrupt callback

• • 		@mdev: mediated device
    

• • 		@cb: virtio-mdev interrupt callback
    

structure

• • @get_device_id: Get virtio device id

• • 		@mdev: mediated device
    

• • 		Returns u32: virtio device id
    

• • @get_vendor_id: Get virtio vendor id

• • 		@mdev: mediated device
    

• • 		Returns u32: virtio vendor id
    

• • @get_status: Get the device status

• • 		@mdev: mediated device
    

• • 		Returns u8: virtio device status
    

• • @set_status: Set the device status

• • 		@mdev: mediated device
    

• • 		@status: virtio device status
    

• • @get_config: Read from device specific confiugration

space

configuration (and similar typos downward)

Let me fix.

...

• • 		@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 confiugration 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_version: Get the version of virtio mdev device

• • 		@mdev: mediated device
    

• • 		Returns integer: version of the device
    

• • @get_generation: Get device generaton

• • 		@mdev: mediated device
    

• • 		Returns u32: device generation
    

• */

+struct virtio_mdev_device_ops {

• /* Virtqueue ops */
• int (*set_vq_address)(struct mdev_device *mdev,

• 	      u16 idx, u64 desc_area, u64 driver_area,
  

• 	      u64 device_area);
  

• void (*set_vq_num)(struct mdev_device *mdev, u16 idx, u32 num);
• void (*kick_vq)(struct mdev_device *mdev, u16 idx);
• void (*set_vq_cb)(struct mdev_device *mdev, u16 idx,

• 	  struct virtio_mdev_callback *cb);
  

• void (*set_vq_ready)(struct mdev_device *mdev, u16 idx, bool

ready);

• bool (*get_vq_ready)(struct mdev_device *mdev, u16 idx);
• int (*set_vq_state)(struct mdev_device *mdev, u16 idx, u64 state);
• u64 (*get_vq_state)(struct mdev_device *mdev, u16 idx);
• /* Device ops */
• u16 (*get_vq_align)(struct mdev_device *mdev);
• u64 (*get_features)(struct mdev_device *mdev);
• int (*set_features)(struct mdev_device *mdev, u64 features);
• void (*set_config_cb)(struct mdev_device *mdev,

• 	      struct virtio_mdev_callback *cb);
  

• u16 (*get_queue_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);
  

• int (*get_version)(struct mdev_device *mdev);
• u32 (*get_generation)(struct mdev_device *mdev);

+};

I'm not sure how stable above ops are.

It's the kernel internal API, so there's no strict requirement for this. We will export a version value for userspace for compatibility.

Does it make sense if defining just two callbacks here, e.g. vq_ctrl and device_ctrl, and then let the vendor driver to handle specific ops in each category (similar to how ioctl works)?

My understanding is that it introduce another indirection, you still need to differ from different command, and it's less flexible than direct callback.

What's the value of doing this?

Thanks

Thanks Kevin

On 2019/9/25 下午9:21, Michael S. Tsirkin wrote:

On Wed, Sep 25, 2019 at 08:45:21PM +0800, Jason Wang wrote:

...

On 2019/9/25 下午5:09, Tian, Kevin wrote:

...

...

From: Jason Wang [mailto:jasowang@redhat.com] Sent: Tuesday, September 24, 2019 9:54 PM

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

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

include/linux/mdev.h | 2 + include/linux/virtio_mdev.h | 145 ++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 include/linux/virtio_mdev.h

diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 3414307311f1..73ac27b3b868 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -126,6 +126,8 @@ struct mdev_device *mdev_from_dev(struct device *dev);

enum { MDEV_ID_VFIO = 1,

• MDEV_ID_VIRTIO = 2,
• MDEV_ID_VHOST = 3, /* New entries must be added here */ };

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

Just be curious. is this version identical to virtio spec version that below callbacks are created for, or just irrelevant?

It could be a hint but basically it's a way for userspace driver compatibility. For kernel we don't need this.

...

...

+struct virtio_mdev_callback {

• irqreturn_t (*callback)(void *data);
• void *private;

+};

+/**

• • struct vfio_mdev_device_ops - Structure to be registered for each
• • mdev device to register the device to virtio-mdev module.
• • @set_vq_address: Set the address of virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@desc_area: address of desc area
    

• • 		@driver_area: address of driver area
    

• • 		@device_area: address of device area
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @set_vq_num: Set the size of virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@num: the size of virtqueue
    

• • @kick_vq: Kick the virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • @set_vq_cb: Set the interrut calback function for

• • 		a virtqueue
    

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@cb: virtio-mdev interrupt callback
    

structure

• • @set_vq_ready: Set ready status for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@ready: ready (true) not ready(false)
    

• • @get_vq_ready: Get ready status for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		Returns boolean: ready (true) or not (false)
    

• • @set_vq_state: Set the state for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		@state: virtqueue state (last_avail_idx)
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @get_vq_state: Get the state for a virtqueue

• • 		@mdev: mediated device
    

• • 		@idx: virtqueue index
    

• • 		Returns virtqueue state (last_avail_idx)
    

• • @get_vq_align: Get the virtqueue align requirement

• • 		for the device
    

• • 		@mdev: mediated device
    

• • 		Returns virtqueue algin requirement
    

• • @get_features: Get virtio features supported by the device

• • 		@mdev: mediated device
    

• • 		Returns the features support by the
    

• • 		device
    

• • @get_features: Set virtio features supported by the driver

• • 		@mdev: mediated device
    

• • 		@features: feature support by the driver
    

• • 		Returns integer: success (0) or error (< 0)
    

• • @set_config_cb: Set the config interrupt callback

• • 		@mdev: mediated device
    

• • 		@cb: virtio-mdev interrupt callback
    

structure

• • @get_device_id: Get virtio device id

• • 		@mdev: mediated device
    

• • 		Returns u32: virtio device id
    

• • @get_vendor_id: Get virtio vendor id

• • 		@mdev: mediated device
    

• • 		Returns u32: virtio vendor id
    

• • @get_status: Get the device status

• • 		@mdev: mediated device
    

• • 		Returns u8: virtio device status
    

• • @set_status: Set the device status

• • 		@mdev: mediated device
    

• • 		@status: virtio device status
    

• • @get_config: Read from device specific confiugration

space

configuration (and similar typos downward)

Let me fix.

...

...

• • 		@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 confiugration 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_version: Get the version of virtio mdev device

• • 		@mdev: mediated device
    

• • 		Returns integer: version of the device
    

• • @get_generation: Get device generaton

• • 		@mdev: mediated device
    

• • 		Returns u32: device generation
    

• */

+struct virtio_mdev_device_ops {

• /* Virtqueue ops */
• int (*set_vq_address)(struct mdev_device *mdev,

• 	      u16 idx, u64 desc_area, u64 driver_area,
  

• 	      u64 device_area);
  

• void (*set_vq_num)(struct mdev_device *mdev, u16 idx, u32 num);
• void (*kick_vq)(struct mdev_device *mdev, u16 idx);
• void (*set_vq_cb)(struct mdev_device *mdev, u16 idx,

• 	  struct virtio_mdev_callback *cb);
  

• void (*set_vq_ready)(struct mdev_device *mdev, u16 idx, bool

ready);

• bool (*get_vq_ready)(struct mdev_device *mdev, u16 idx);
• int (*set_vq_state)(struct mdev_device *mdev, u16 idx, u64 state);
• u64 (*get_vq_state)(struct mdev_device *mdev, u16 idx);
• /* Device ops */
• u16 (*get_vq_align)(struct mdev_device *mdev);
• u64 (*get_features)(struct mdev_device *mdev);
• int (*set_features)(struct mdev_device *mdev, u64 features);
• void (*set_config_cb)(struct mdev_device *mdev,

• 	      struct virtio_mdev_callback *cb);
  

• u16 (*get_queue_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);
  

• int (*get_version)(struct mdev_device *mdev);
• u32 (*get_generation)(struct mdev_device *mdev);

+};

I'm not sure how stable above ops are.

It's the kernel internal API, so there's no strict requirement for this. We will export a version value for userspace for compatibility.

Given it's tied to virtio we probably want kernel+userspace feature bits?

Yes, then I think we could probably have a version field inside e.g device_ops structure. Then it could be fetched from both kernel and userspace driver.

Thanks