On Wed, 23 Oct 2019 21:07:47 +0800 Jason Wang <jasowang at redhat.com> wrote:> Mdev bus only supports vfio driver right now, so it doesn't implement > match method. But in the future, we may add drivers other than vfio, > the first driver could be virtio-mdev. This means we need to add > device class id support in bus match method to pair the mdev device > and mdev driver correctly. > > So this patch adds id_table to mdev_driver and class_id for mdev > device with the match method for mdev bus. > > Signed-off-by: Jason Wang <jasowang at redhat.com> > --- > .../driver-api/vfio-mediated-device.rst | 5 +++++ > drivers/gpu/drm/i915/gvt/kvmgt.c | 1 + > drivers/s390/cio/vfio_ccw_ops.c | 1 + > drivers/s390/crypto/vfio_ap_ops.c | 1 + > drivers/vfio/mdev/mdev_core.c | 18 +++++++++++++++ > drivers/vfio/mdev/mdev_driver.c | 22 +++++++++++++++++++ > drivers/vfio/mdev/mdev_private.h | 1 + > drivers/vfio/mdev/vfio_mdev.c | 6 +++++ > include/linux/mdev.h | 8 +++++++ > include/linux/mod_devicetable.h | 8 +++++++ > samples/vfio-mdev/mbochs.c | 1 + > samples/vfio-mdev/mdpy.c | 1 + > samples/vfio-mdev/mtty.c | 1 + > 13 files changed, 74 insertions(+) > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst > index 25eb7d5b834b..6709413bee29 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 > @@ -170,6 +172,9 @@ that a driver should use to unregister itself with the mdev core driver:: > > extern void mdev_unregister_device(struct device *dev); > > +It is also required to specify the class_id in create() callback through:: > + > + int mdev_set_class(struct mdev_device *mdev, u16 id); > > Mediated Device Management Interface Through sysfs > =================================================> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > index 343d79c1cb7e..6420f0dbd31b 100644 > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > @@ -678,6 +678,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) > dev_name(mdev_dev(mdev))); > ret = 0; > > + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > out: > return ret; > } > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c > index f0d71ab77c50..cf2c013ae32f 100644 > --- a/drivers/s390/cio/vfio_ccw_ops.c > +++ b/drivers/s390/cio/vfio_ccw_ops.c > @@ -129,6 +129,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev) > private->sch->schid.ssid, > private->sch->schid.sch_no); > > + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > return 0; > } > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 5c0f53c6dde7..07c31070afeb 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -343,6 +343,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) > list_add(&matrix_mdev->node, &matrix_dev->mdev_list); > mutex_unlock(&matrix_dev->lock); > > + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > return 0; > } > > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > index b558d4cfd082..3a9c52d71b4e 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -45,6 +45,16 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data) > } > EXPORT_SYMBOL(mdev_set_drvdata); > > +/* Specify the class for the mdev device, this must be called during > + * create() callback. > + */ > +void mdev_set_class(struct mdev_device *mdev, u16 id) > +{ > + WARN_ON(mdev->class_id); > + mdev->class_id = id; > +} > +EXPORT_SYMBOL(mdev_set_class); > + > struct device *mdev_dev(struct mdev_device *mdev) > { > return &mdev->dev; > @@ -135,6 +145,7 @@ static int mdev_device_remove_cb(struct device *dev, void *data) > * mdev_register_device : Register a device > * @dev: device structure representing parent device. > * @ops: Parent device operation structure to be registered. > + * @id: class id. > * > * Add device to list of registered parent devices. > * Returns a negative value on error, otherwise 0. > @@ -324,6 +335,13 @@ int mdev_device_create(struct kobject *kobj, > if (ret) > goto ops_create_fail; > > + if (!mdev->class_id) { > + ret = -EINVAL; > + WARN(1, "class id must be specified for device %s\n", > + dev_name(dev));Nit, dev_warn(dev, "mdev vendor driver failed to specify device class\n");> + goto add_fail; > + } > + > ret = device_add(&mdev->dev); > if (ret) > goto add_fail; > diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c > index 0d3223aee20b..319d886ffaf7 100644 > --- a/drivers/vfio/mdev/mdev_driver.c > +++ b/drivers/vfio/mdev/mdev_driver.c > @@ -69,8 +69,30 @@ static int mdev_remove(struct device *dev) > return 0; > } > > +static int mdev_match(struct device *dev, struct device_driver *drv) > +{ > + unsigned int i; > + struct mdev_device *mdev = to_mdev_device(dev); > + struct mdev_driver *mdrv = to_mdev_driver(drv); > + const struct mdev_class_id *ids = mdrv->id_table; > +Nit, as we start to allow new mdev bus drivers, mdev-core might want to protect itself from a NULL id_table, by either failing the mdev_register_driver() or failing the match here. I think such a condition would segfault as written here, but clearly we don't have such external drivers yet. Thanks, Alex> + for (i = 0; ids[i].id; i++) > + if (ids[i].id == mdev->class_id) > + return 1; > + return 0; > +} > + > +static int mdev_uevent(struct device *dev, struct kobj_uevent_env *env) > +{ > + struct mdev_device *mdev = to_mdev_device(dev); > + > + return add_uevent_var(env, "MODALIAS=mdev:c%02X", mdev->class_id); > +} > + > struct bus_type mdev_bus_type = { > .name = "mdev", > + .match = mdev_match, > + .uevent = mdev_uevent, > .probe = mdev_probe, > .remove = mdev_remove, > }; > diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h > index 7d922950caaf..c65f436c1869 100644 > --- a/drivers/vfio/mdev/mdev_private.h > +++ b/drivers/vfio/mdev/mdev_private.h > @@ -33,6 +33,7 @@ struct mdev_device { > struct kobject *type_kobj; > struct device *iommu_device; > bool active; > + u16 class_id; > }; > > #define to_mdev_device(dev) container_of(dev, struct mdev_device, dev) > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > index 30964a4e0a28..7b24ee9cb8dd 100644 > --- a/drivers/vfio/mdev/vfio_mdev.c > +++ b/drivers/vfio/mdev/vfio_mdev.c > @@ -120,10 +120,16 @@ static void vfio_mdev_remove(struct device *dev) > vfio_del_group_dev(dev); > } > > +static const struct mdev_class_id id_table[] = { > + { MDEV_CLASS_ID_VFIO }, > + { 0 }, > +}; > + > static struct mdev_driver vfio_mdev_driver = { > .name = "vfio_mdev", > .probe = vfio_mdev_probe, > .remove = vfio_mdev_remove, > + .id_table = id_table, > }; > > static int __init vfio_mdev_init(void) > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > index 0ce30ca78db0..78b69d09eb54 100644 > --- a/include/linux/mdev.h > +++ b/include/linux/mdev.h > @@ -118,6 +118,7 @@ struct mdev_type_attribute mdev_type_attr_##_name = \ > * @probe: called when new device created > * @remove: called when device removed > * @driver: device driver structure > + * @id_table: the ids serviced by this driver > * > **/ > struct mdev_driver { > @@ -125,6 +126,7 @@ struct mdev_driver { > int (*probe)(struct device *dev); > void (*remove)(struct device *dev); > struct device_driver driver; > + const struct mdev_class_id *id_table; > }; > > #define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver) > @@ -132,6 +134,7 @@ struct mdev_driver { > void *mdev_get_drvdata(struct mdev_device *mdev); > void mdev_set_drvdata(struct mdev_device *mdev, void *data); > const guid_t *mdev_uuid(struct mdev_device *mdev); > +void mdev_set_class(struct mdev_device *mdev, u16 id); > > extern struct bus_type mdev_bus_type; > > @@ -145,4 +148,9 @@ struct device *mdev_parent_dev(struct mdev_device *mdev); > struct device *mdev_dev(struct mdev_device *mdev); > struct mdev_device *mdev_from_dev(struct device *dev); > > +enum { > + MDEV_CLASS_ID_VFIO = 1, > + /* New entries must be added here */ > +}; > + > #endif /* MDEV_H */ > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h > index 5714fd35a83c..f32c6e44fb1a 100644 > --- a/include/linux/mod_devicetable.h > +++ b/include/linux/mod_devicetable.h > @@ -821,4 +821,12 @@ struct wmi_device_id { > const void *context; > }; > > +/** > + * struct mdev_class_id - MDEV device class identifier > + * @id: Used to identify a specific class of device, e.g vfio-mdev device. > + */ > +struct mdev_class_id { > + __u16 id; > +}; > + > #endif /* LINUX_MOD_DEVICETABLE_H */ > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c > index ac5c8c17b1ff..115bc5074656 100644 > --- a/samples/vfio-mdev/mbochs.c > +++ b/samples/vfio-mdev/mbochs.c > @@ -561,6 +561,7 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev) > mbochs_reset(mdev); > > mbochs_used_mbytes += type->mbytes; > + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > return 0; > > err_mem: > diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c > index cc86bf6566e4..665614574d50 100644 > --- a/samples/vfio-mdev/mdpy.c > +++ b/samples/vfio-mdev/mdpy.c > @@ -269,6 +269,7 @@ static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev) > mdpy_reset(mdev); > > mdpy_count++; > + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > return 0; > } > > diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c > index ce84a300a4da..90da12ff7fd9 100644 > --- a/samples/vfio-mdev/mtty.c > +++ b/samples/vfio-mdev/mtty.c > @@ -755,6 +755,7 @@ static int mtty_create(struct kobject *kobj, struct mdev_device *mdev) > list_add(&mdev_state->next, &mdev_devices_list); > mutex_unlock(&mdev_list_lock); > > + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > return 0; > } >
On 2019/10/24 ??5:42, Alex Williamson wrote:> On Wed, 23 Oct 2019 21:07:47 +0800 > Jason Wang <jasowang at redhat.com> wrote: > >> Mdev bus only supports vfio driver right now, so it doesn't implement >> match method. But in the future, we may add drivers other than vfio, >> the first driver could be virtio-mdev. This means we need to add >> device class id support in bus match method to pair the mdev device >> and mdev driver correctly. >> >> So this patch adds id_table to mdev_driver and class_id for mdev >> device with the match method for mdev bus. >> >> Signed-off-by: Jason Wang <jasowang at redhat.com> >> --- >> .../driver-api/vfio-mediated-device.rst | 5 +++++ >> drivers/gpu/drm/i915/gvt/kvmgt.c | 1 + >> drivers/s390/cio/vfio_ccw_ops.c | 1 + >> drivers/s390/crypto/vfio_ap_ops.c | 1 + >> drivers/vfio/mdev/mdev_core.c | 18 +++++++++++++++ >> drivers/vfio/mdev/mdev_driver.c | 22 +++++++++++++++++++ >> drivers/vfio/mdev/mdev_private.h | 1 + >> drivers/vfio/mdev/vfio_mdev.c | 6 +++++ >> include/linux/mdev.h | 8 +++++++ >> include/linux/mod_devicetable.h | 8 +++++++ >> samples/vfio-mdev/mbochs.c | 1 + >> samples/vfio-mdev/mdpy.c | 1 + >> samples/vfio-mdev/mtty.c | 1 + >> 13 files changed, 74 insertions(+) >> >> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst >> index 25eb7d5b834b..6709413bee29 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 >> @@ -170,6 +172,9 @@ that a driver should use to unregister itself with the mdev core driver:: >> >> extern void mdev_unregister_device(struct device *dev); >> >> +It is also required to specify the class_id in create() callback through:: >> + >> + int mdev_set_class(struct mdev_device *mdev, u16 id); >> >> Mediated Device Management Interface Through sysfs >> =================================================>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c >> index 343d79c1cb7e..6420f0dbd31b 100644 >> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c >> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c >> @@ -678,6 +678,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) >> dev_name(mdev_dev(mdev))); >> ret = 0; >> >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); >> out: >> return ret; >> } >> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c >> index f0d71ab77c50..cf2c013ae32f 100644 >> --- a/drivers/s390/cio/vfio_ccw_ops.c >> +++ b/drivers/s390/cio/vfio_ccw_ops.c >> @@ -129,6 +129,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev) >> private->sch->schid.ssid, >> private->sch->schid.sch_no); >> >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); >> return 0; >> } >> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c >> index 5c0f53c6dde7..07c31070afeb 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -343,6 +343,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) >> list_add(&matrix_mdev->node, &matrix_dev->mdev_list); >> mutex_unlock(&matrix_dev->lock); >> >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); >> return 0; >> } >> >> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c >> index b558d4cfd082..3a9c52d71b4e 100644 >> --- a/drivers/vfio/mdev/mdev_core.c >> +++ b/drivers/vfio/mdev/mdev_core.c >> @@ -45,6 +45,16 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data) >> } >> EXPORT_SYMBOL(mdev_set_drvdata); >> >> +/* Specify the class for the mdev device, this must be called during >> + * create() callback. >> + */ >> +void mdev_set_class(struct mdev_device *mdev, u16 id) >> +{ >> + WARN_ON(mdev->class_id); >> + mdev->class_id = id; >> +} >> +EXPORT_SYMBOL(mdev_set_class); >> + >> struct device *mdev_dev(struct mdev_device *mdev) >> { >> return &mdev->dev; >> @@ -135,6 +145,7 @@ static int mdev_device_remove_cb(struct device *dev, void *data) >> * mdev_register_device : Register a device >> * @dev: device structure representing parent device. >> * @ops: Parent device operation structure to be registered. >> + * @id: class id. >> * >> * Add device to list of registered parent devices. >> * Returns a negative value on error, otherwise 0. >> @@ -324,6 +335,13 @@ int mdev_device_create(struct kobject *kobj, >> if (ret) >> goto ops_create_fail; >> >> + if (!mdev->class_id) { >> + ret = -EINVAL; >> + WARN(1, "class id must be specified for device %s\n", >> + dev_name(dev)); > Nit, dev_warn(dev, "mdev vendor driver failed to specify device class\n");Will fix.> >> + goto add_fail; >> + } >> + >> ret = device_add(&mdev->dev); >> if (ret) >> goto add_fail; >> diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c >> index 0d3223aee20b..319d886ffaf7 100644 >> --- a/drivers/vfio/mdev/mdev_driver.c >> +++ b/drivers/vfio/mdev/mdev_driver.c >> @@ -69,8 +69,30 @@ static int mdev_remove(struct device *dev) >> return 0; >> } >> >> +static int mdev_match(struct device *dev, struct device_driver *drv) >> +{ >> + unsigned int i; >> + struct mdev_device *mdev = to_mdev_device(dev); >> + struct mdev_driver *mdrv = to_mdev_driver(drv); >> + const struct mdev_class_id *ids = mdrv->id_table; >> + > Nit, as we start to allow new mdev bus drivers, mdev-core might want to > protect itself from a NULL id_table, by either failing the > mdev_register_driver() or failing the match here. I think such a > condition would segfault as written here, but clearly we don't have > such external drivers yet. Thanks,I'm not sure I get the point here. My understanding is that mdev-core won't try to be matched here since it was not a complete mdev device. Thanks> > Alex > >> + for (i = 0; ids[i].id; i++) >> + if (ids[i].id == mdev->class_id) >> + return 1; >> + return 0; >> +} >> + >> +static int mdev_uevent(struct device *dev, struct kobj_uevent_env *env) >> +{ >> + struct mdev_device *mdev = to_mdev_device(dev); >> + >> + return add_uevent_var(env, "MODALIAS=mdev:c%02X", mdev->class_id); >> +} >> + >> struct bus_type mdev_bus_type = { >> .name = "mdev", >> + .match = mdev_match, >> + .uevent = mdev_uevent, >> .probe = mdev_probe, >> .remove = mdev_remove, >> }; >> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h >> index 7d922950caaf..c65f436c1869 100644 >> --- a/drivers/vfio/mdev/mdev_private.h >> +++ b/drivers/vfio/mdev/mdev_private.h >> @@ -33,6 +33,7 @@ struct mdev_device { >> struct kobject *type_kobj; >> struct device *iommu_device; >> bool active; >> + u16 class_id; >> }; >> >> #define to_mdev_device(dev) container_of(dev, struct mdev_device, dev) >> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c >> index 30964a4e0a28..7b24ee9cb8dd 100644 >> --- a/drivers/vfio/mdev/vfio_mdev.c >> +++ b/drivers/vfio/mdev/vfio_mdev.c >> @@ -120,10 +120,16 @@ static void vfio_mdev_remove(struct device *dev) >> vfio_del_group_dev(dev); >> } >> >> +static const struct mdev_class_id id_table[] = { >> + { MDEV_CLASS_ID_VFIO }, >> + { 0 }, >> +}; >> + >> static struct mdev_driver vfio_mdev_driver = { >> .name = "vfio_mdev", >> .probe = vfio_mdev_probe, >> .remove = vfio_mdev_remove, >> + .id_table = id_table, >> }; >> >> static int __init vfio_mdev_init(void) >> diff --git a/include/linux/mdev.h b/include/linux/mdev.h >> index 0ce30ca78db0..78b69d09eb54 100644 >> --- a/include/linux/mdev.h >> +++ b/include/linux/mdev.h >> @@ -118,6 +118,7 @@ struct mdev_type_attribute mdev_type_attr_##_name = \ >> * @probe: called when new device created >> * @remove: called when device removed >> * @driver: device driver structure >> + * @id_table: the ids serviced by this driver >> * >> **/ >> struct mdev_driver { >> @@ -125,6 +126,7 @@ struct mdev_driver { >> int (*probe)(struct device *dev); >> void (*remove)(struct device *dev); >> struct device_driver driver; >> + const struct mdev_class_id *id_table; >> }; >> >> #define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver) >> @@ -132,6 +134,7 @@ struct mdev_driver { >> void *mdev_get_drvdata(struct mdev_device *mdev); >> void mdev_set_drvdata(struct mdev_device *mdev, void *data); >> const guid_t *mdev_uuid(struct mdev_device *mdev); >> +void mdev_set_class(struct mdev_device *mdev, u16 id); >> >> extern struct bus_type mdev_bus_type; >> >> @@ -145,4 +148,9 @@ struct device *mdev_parent_dev(struct mdev_device *mdev); >> struct device *mdev_dev(struct mdev_device *mdev); >> struct mdev_device *mdev_from_dev(struct device *dev); >> >> +enum { >> + MDEV_CLASS_ID_VFIO = 1, >> + /* New entries must be added here */ >> +}; >> + >> #endif /* MDEV_H */ >> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h >> index 5714fd35a83c..f32c6e44fb1a 100644 >> --- a/include/linux/mod_devicetable.h >> +++ b/include/linux/mod_devicetable.h >> @@ -821,4 +821,12 @@ struct wmi_device_id { >> const void *context; >> }; >> >> +/** >> + * struct mdev_class_id - MDEV device class identifier >> + * @id: Used to identify a specific class of device, e.g vfio-mdev device. >> + */ >> +struct mdev_class_id { >> + __u16 id; >> +}; >> + >> #endif /* LINUX_MOD_DEVICETABLE_H */ >> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c >> index ac5c8c17b1ff..115bc5074656 100644 >> --- a/samples/vfio-mdev/mbochs.c >> +++ b/samples/vfio-mdev/mbochs.c >> @@ -561,6 +561,7 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev) >> mbochs_reset(mdev); >> >> mbochs_used_mbytes += type->mbytes; >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); >> return 0; >> >> err_mem: >> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c >> index cc86bf6566e4..665614574d50 100644 >> --- a/samples/vfio-mdev/mdpy.c >> +++ b/samples/vfio-mdev/mdpy.c >> @@ -269,6 +269,7 @@ static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev) >> mdpy_reset(mdev); >> >> mdpy_count++; >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); >> return 0; >> } >> >> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c >> index ce84a300a4da..90da12ff7fd9 100644 >> --- a/samples/vfio-mdev/mtty.c >> +++ b/samples/vfio-mdev/mtty.c >> @@ -755,6 +755,7 @@ static int mtty_create(struct kobject *kobj, struct mdev_device *mdev) >> list_add(&mdev_state->next, &mdev_devices_list); >> mutex_unlock(&mdev_list_lock); >> >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); >> return 0; >> } >>
On Thu, 24 Oct 2019 11:27:36 +0800 Jason Wang <jasowang at redhat.com> wrote:> On 2019/10/24 ??5:42, Alex Williamson wrote: > > On Wed, 23 Oct 2019 21:07:47 +0800 > > Jason Wang <jasowang at redhat.com> wrote: > > > >> Mdev bus only supports vfio driver right now, so it doesn't implement > >> match method. But in the future, we may add drivers other than vfio, > >> the first driver could be virtio-mdev. This means we need to add > >> device class id support in bus match method to pair the mdev device > >> and mdev driver correctly. > >> > >> So this patch adds id_table to mdev_driver and class_id for mdev > >> device with the match method for mdev bus. > >> > >> Signed-off-by: Jason Wang <jasowang at redhat.com> > >> --- > >> .../driver-api/vfio-mediated-device.rst | 5 +++++ > >> drivers/gpu/drm/i915/gvt/kvmgt.c | 1 + > >> drivers/s390/cio/vfio_ccw_ops.c | 1 + > >> drivers/s390/crypto/vfio_ap_ops.c | 1 + > >> drivers/vfio/mdev/mdev_core.c | 18 +++++++++++++++ > >> drivers/vfio/mdev/mdev_driver.c | 22 +++++++++++++++++++ > >> drivers/vfio/mdev/mdev_private.h | 1 + > >> drivers/vfio/mdev/vfio_mdev.c | 6 +++++ > >> include/linux/mdev.h | 8 +++++++ > >> include/linux/mod_devicetable.h | 8 +++++++ > >> samples/vfio-mdev/mbochs.c | 1 + > >> samples/vfio-mdev/mdpy.c | 1 + > >> samples/vfio-mdev/mtty.c | 1 + > >> 13 files changed, 74 insertions(+) > >> > >> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst > >> index 25eb7d5b834b..6709413bee29 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 > >> @@ -170,6 +172,9 @@ that a driver should use to unregister itself with the mdev core driver:: > >> > >> extern void mdev_unregister_device(struct device *dev); > >> > >> +It is also required to specify the class_id in create() callback through:: > >> + > >> + int mdev_set_class(struct mdev_device *mdev, u16 id); > >> > >> Mediated Device Management Interface Through sysfs > >> =================================================> >> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > >> index 343d79c1cb7e..6420f0dbd31b 100644 > >> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > >> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > >> @@ -678,6 +678,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) > >> dev_name(mdev_dev(mdev))); > >> ret = 0; > >> > >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > >> out: > >> return ret; > >> } > >> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c > >> index f0d71ab77c50..cf2c013ae32f 100644 > >> --- a/drivers/s390/cio/vfio_ccw_ops.c > >> +++ b/drivers/s390/cio/vfio_ccw_ops.c > >> @@ -129,6 +129,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev) > >> private->sch->schid.ssid, > >> private->sch->schid.sch_no); > >> > >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > >> return 0; > >> } > >> > >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > >> index 5c0f53c6dde7..07c31070afeb 100644 > >> --- a/drivers/s390/crypto/vfio_ap_ops.c > >> +++ b/drivers/s390/crypto/vfio_ap_ops.c > >> @@ -343,6 +343,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) > >> list_add(&matrix_mdev->node, &matrix_dev->mdev_list); > >> mutex_unlock(&matrix_dev->lock); > >> > >> + mdev_set_class(mdev, MDEV_CLASS_ID_VFIO); > >> return 0; > >> } > >> > >> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > >> index b558d4cfd082..3a9c52d71b4e 100644 > >> --- a/drivers/vfio/mdev/mdev_core.c > >> +++ b/drivers/vfio/mdev/mdev_core.c > >> @@ -45,6 +45,16 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data) > >> } > >> EXPORT_SYMBOL(mdev_set_drvdata); > >> > >> +/* Specify the class for the mdev device, this must be called during > >> + * create() callback. > >> + */ > >> +void mdev_set_class(struct mdev_device *mdev, u16 id) > >> +{ > >> + WARN_ON(mdev->class_id); > >> + mdev->class_id = id; > >> +} > >> +EXPORT_SYMBOL(mdev_set_class); > >> + > >> struct device *mdev_dev(struct mdev_device *mdev) > >> { > >> return &mdev->dev; > >> @@ -135,6 +145,7 @@ static int mdev_device_remove_cb(struct device *dev, void *data) > >> * mdev_register_device : Register a device > >> * @dev: device structure representing parent device. > >> * @ops: Parent device operation structure to be registered. > >> + * @id: class id. > >> * > >> * Add device to list of registered parent devices. > >> * Returns a negative value on error, otherwise 0. > >> @@ -324,6 +335,13 @@ int mdev_device_create(struct kobject *kobj, > >> if (ret) > >> goto ops_create_fail; > >> > >> + if (!mdev->class_id) { > >> + ret = -EINVAL; > >> + WARN(1, "class id must be specified for device %s\n", > >> + dev_name(dev)); > > Nit, dev_warn(dev, "mdev vendor driver failed to specify device class\n"); > > > Will fix. > > > > > >> + goto add_fail; > >> + } > >> + > >> ret = device_add(&mdev->dev); > >> if (ret) > >> goto add_fail; > >> diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c > >> index 0d3223aee20b..319d886ffaf7 100644 > >> --- a/drivers/vfio/mdev/mdev_driver.c > >> +++ b/drivers/vfio/mdev/mdev_driver.c > >> @@ -69,8 +69,30 @@ static int mdev_remove(struct device *dev) > >> return 0; > >> } > >> > >> +static int mdev_match(struct device *dev, struct device_driver *drv) > >> +{ > >> + unsigned int i; > >> + struct mdev_device *mdev = to_mdev_device(dev); > >> + struct mdev_driver *mdrv = to_mdev_driver(drv); > >> + const struct mdev_class_id *ids = mdrv->id_table; > >> + > > Nit, as we start to allow new mdev bus drivers, mdev-core might want to > > protect itself from a NULL id_table, by either failing the > > mdev_register_driver() or failing the match here. I think such a > > condition would segfault as written here, but clearly we don't have > > such external drivers yet. Thanks, > > > I'm not sure I get the point here. My understanding is that mdev-core > won't try to be matched here since it was not a complete mdev device.The parent driver failing to set a type vs the parent driver failing to register with a struct mdev_driver where id_table is not null are different issues. I agree that if a vendor driver was not updated for this series that they'd never successfully create a device because the mdev-core would reject it for not setting a class, but mdev_match() is called for devices that might be created by other vendor drivers, so loading a parent driver with a null id_table potentially breaks matching for everyone. Thanks, Alex