Re: [PATCH v1 13/18] vfio/mdev: Consolidate all the device_api sysfs into the core code

Kirti Wankhede
2022-06-06T19:44:02Z

Reviewed By: Kirti Wankhede <kwankhede@nvidia.com>

On 6/2/2022 10:49 PM, Eric Farman wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
>
> Every driver just emits a static string, simply feed it through the ops
> and provide a standard sysfs show function.
>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/6-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> [farman: added Cc: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> .../driver-api/vfio-mediated-device.rst | 4 ++-
> drivers/gpu/drm/i915/gvt/kvmgt.c | 9 +------
> drivers/s390/cio/vfio_ccw_ops.c | 9 +------
> drivers/s390/crypto/vfio_ap_ops.c | 9 +------
> drivers/vfio/mdev/mdev_core.c | 2 +-
> drivers/vfio/mdev/mdev_sysfs.c | 27 ++++++++++++++++---
> include/linux/mdev.h | 7 ++---
> samples/vfio-mdev/mbochs.c | 9 +------
> samples/vfio-mdev/mdpy.c | 9 +------
> samples/vfio-mdev/mtty.c | 10 +------
> 10 files changed, 36 insertions(+), 59 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index 9f26079cacae..f410a1cd98bb 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -137,6 +137,7 @@ The structures in the mdev_parent_ops structure are as follows:
> * mdev_attr_groups: attributes of the mediated device
> * supported_config: attributes to define supported configurations
> * device_driver: device driver to bind for mediated device instances
> +* device_api: String to pass through the sysfs file below
>
> The mdev_parent_ops also still has various functions pointers. Theses exist
> for historical reasons only and shall not be used for new drivers.
> @@ -225,7 +226,8 @@ Directories and files under the sysfs for Each Physical Device
> * device_api
>
> This attribute should show which device API is being created, for example,
> - "vfio-pci" for a PCI device.
> + "vfio-pci" for a PCI device. The core code maintins this sysfs using the
> + device_api member of mdev_parent_ops.
>
> * available_instances
>
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 057ec4490104..752d7a1211e6 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -163,12 +163,6 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
> return sprintf(buf, "%u\n", num);
> }
>
> -static ssize_t device_api_show(struct mdev_type *mtype,
> - struct mdev_type_attribute *attr, char *buf)
> -{
> - return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -
> static ssize_t description_show(struct mdev_type *mtype,
> struct mdev_type_attribute *attr, char *buf)
> {
> @@ -202,13 +196,11 @@ static ssize_t name_show(struct mdev_type *mtype,
> }
>
> static MDEV_TYPE_ATTR_RO(available_instances);
> -static MDEV_TYPE_ATTR_RO(device_api);
> static MDEV_TYPE_ATTR_RO(description);
> static MDEV_TYPE_ATTR_RO(name);
>
> static struct attribute *gvt_type_attrs[] = {
> &mdev_type_attr_available_instances.attr,
> - &mdev_type_attr_device_api.attr,
> &mdev_type_attr_description.attr,
> &mdev_type_attr_name.attr,
> NULL,
> @@ -1767,6 +1759,7 @@ static const struct attribute_group *intel_vgpu_groups[] = {
>
> static struct mdev_parent_ops intel_vgpu_ops = {
> .mdev_attr_groups = intel_vgpu_groups,
> + .device_api = VFIO_DEVICE_API_PCI_STRING,
> .create = intel_vgpu_create,
> .remove = intel_vgpu_remove,
>
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 2afb8f13739f..6793c8b3c58b 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -66,13 +66,6 @@ static ssize_t name_show(struct mdev_type *mtype,
> }
> static MDEV_TYPE_ATTR_RO(name);
>
> -static ssize_t device_api_show(struct mdev_type *mtype,
> - struct mdev_type_attribute *attr, char *buf)
> -{
> - return sprintf(buf, "%s\n", VFIO_DEVICE_API_CCW_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
> static ssize_t available_instances_show(struct mdev_type *mtype,
> struct mdev_type_attribute *attr,
> char *buf)
> @@ -86,7 +79,6 @@ static MDEV_TYPE_ATTR_RO(available_instances);
>
> static struct attribute *mdev_types_attrs[] = {
> &mdev_type_attr_name.attr,
> - &mdev_type_attr_device_api.attr,
> &mdev_type_attr_available_instances.attr,
> NULL,
> };
> @@ -644,5 +636,6 @@ struct mdev_driver vfio_ccw_mdev_driver = {
> const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> .owner = THIS_MODULE,
> .device_driver = &vfio_ccw_mdev_driver,
> + .device_api = VFIO_DEVICE_API_CCW_STRING,
> .supported_type_groups = mdev_type_groups,
> };
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6e08d04b605d..838b1a3eac8a 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -530,17 +530,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>
> static MDEV_TYPE_ATTR_RO(available_instances);
>
> -static ssize_t device_api_show(struct mdev_type *mtype,
> - struct mdev_type_attribute *attr, char *buf)
> -{
> - return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING);
> -}
> -
> -static MDEV_TYPE_ATTR_RO(device_api);
>
> static struct attribute *vfio_ap_mdev_type_attrs[] = {
> &mdev_type_attr_name.attr,
> - &mdev_type_attr_device_api.attr,
> &mdev_type_attr_available_instances.attr,
> NULL,
> };
> @@ -1501,6 +1493,7 @@ static struct mdev_driver vfio_ap_matrix_driver = {
> static const struct mdev_parent_ops vfio_ap_matrix_ops = {
> .owner = THIS_MODULE,
> .device_driver = &vfio_ap_matrix_driver,
> + .device_api = VFIO_DEVICE_API_AP_STRING,
> .supported_type_groups = vfio_ap_mdev_type_groups,
> };
>
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b314101237fe..c3018e8e6d32 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -129,7 +129,7 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> char *envp[] = { env_string, NULL };
>
> /* check for mandatory ops */
> - if (!ops || !ops->supported_type_groups)
> + if (!ops || !ops->supported_type_groups || !ops->device_api)
> return -EINVAL;
> if (!ops->device_driver && (!ops->create || !ops->remove))
> return -EINVAL;
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index f5cf1931c54e..d4b99440d19e 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -74,9 +74,30 @@ static ssize_t create_store(struct mdev_type *mtype,
>
> return count;
> }
> -
> static MDEV_TYPE_ATTR_WO(create);
>
> +static ssize_t device_api_show(struct mdev_type *mtype,
> + struct mdev_type_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "%s\n", mtype->parent->ops->device_api);
> +}
> +static MDEV_TYPE_ATTR_RO(device_api);
> +
> +static struct attribute *mdev_types_std_attrs[] = {
> + &mdev_type_attr_create.attr,
> + &mdev_type_attr_device_api.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mdev_type_std_group = {
> + .attrs = mdev_types_std_attrs,
> +};
> +
> +static const struct attribute_group *mdev_type_groups[] = {
> + &mdev_type_std_group,
> + NULL,
> +};
> +
> static void mdev_type_release(struct kobject *kobj)
> {
> struct mdev_type *type = to_mdev_type(kobj);
> @@ -123,7 +144,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
> return ERR_PTR(ret);
> }
>
> - ret = sysfs_create_file(&type->kobj, &mdev_type_attr_create.attr);
> + ret = sysfs_create_groups(&type->kobj, mdev_type_groups);
> if (ret)
> goto attr_create_failed;
>
> @@ -144,7 +165,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
> attrs_failed:
> kobject_put(type->devices_kobj);
> attr_devices_failed:
> - sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr);
> + sysfs_remove_groups(&type->kobj, mdev_type_groups);
> attr_create_failed:
> kobject_del(&type->kobj);
> kobject_put(&type->kobj);
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index a5788f592817..14655215417b 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -36,6 +36,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
> *
> * @owner: The module owner.
> * @device_driver: Which device driver to probe() on newly created devices
> + * @device_api: String to return for the device_api sysfs
> * @dev_attr_groups: Attributes of the parent device.
> * @mdev_attr_groups: Attributes of the mediated device.
> * @supported_type_groups: Attributes to define supported types. It is mandatory
> @@ -80,6 +81,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
> struct mdev_parent_ops {
> struct module *owner;
> struct mdev_driver *device_driver;
> + const char *device_api;
> const struct attribute_group **dev_attr_groups;
> const struct attribute_group **mdev_attr_groups;
> struct attribute_group **supported_type_groups;
> @@ -108,11 +110,6 @@ struct mdev_type_attribute {
> size_t count);
> };
>
> -#define MDEV_TYPE_ATTR(_name, _mode, _show, _store) \
> -struct mdev_type_attribute mdev_type_attr_##_name = \
> - __ATTR(_name, _mode, _show, _store)
> -#define MDEV_TYPE_ATTR_RW(_name) \
> - struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RW(_name)
> #define MDEV_TYPE_ATTR_RO(_name) \
> struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RO(_name)
> #define MDEV_TYPE_ATTR_WO(_name) \
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index e90c8552cc31..8d3ae97d9d6e 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -1358,17 +1358,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
> }
> static MDEV_TYPE_ATTR_RO(available_instances);
>
> -static ssize_t device_api_show(struct mdev_type *mtype,
> - struct mdev_type_attribute *attr, char *buf)
> -{
> - return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
> static struct attribute *mdev_types_attrs[] = {
> &mdev_type_attr_name.attr,
> &mdev_type_attr_description.attr,
> - &mdev_type_attr_device_api.attr,
> &mdev_type_attr_available_instances.attr,
> NULL,
> };
> @@ -1417,6 +1409,7 @@ static struct mdev_driver mbochs_driver = {
> static const struct mdev_parent_ops mdev_fops = {
> .owner = THIS_MODULE,
> .device_driver = &mbochs_driver,
> + .device_api = VFIO_DEVICE_API_PCI_STRING,
> .supported_type_groups = mdev_type_groups,
> };
>
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index fe5d43e797b6..402a7ebe6563 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -670,17 +670,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
> }
> static MDEV_TYPE_ATTR_RO(available_instances);
>
> -static ssize_t device_api_show(struct mdev_type *mtype,
> - struct mdev_type_attribute *attr, char *buf)
> -{
> - return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
> static struct attribute *mdev_types_attrs[] = {
> &mdev_type_attr_name.attr,
> &mdev_type_attr_description.attr,
> - &mdev_type_attr_device_api.attr,
> &mdev_type_attr_available_instances.attr,
> NULL,
> };
> @@ -728,6 +720,7 @@ static struct mdev_driver mdpy_driver = {
> static const struct mdev_parent_ops mdev_fops = {
> .owner = THIS_MODULE,
> .device_driver = &mdpy_driver,
> + .device_api = VFIO_DEVICE_API_PCI_STRING,
> .supported_type_groups = mdev_type_groups,
> };
>
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index a0e1a469bd47..5dc1b6a4c02c 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -1281,17 +1281,8 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>
> static MDEV_TYPE_ATTR_RO(available_instances);
>
> -static ssize_t device_api_show(struct mdev_type *mtype,
> - struct mdev_type_attribute *attr, char *buf)
> -{
> - return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
> static struct attribute *mdev_types_attrs[] = {
> &mdev_type_attr_name.attr,
> - &mdev_type_attr_device_api.attr,
> &mdev_type_attr_available_instances.attr,
> NULL,
> };
> @@ -1333,6 +1324,7 @@ static struct mdev_driver mtty_driver = {
> static const struct mdev_parent_ops mdev_fops = {
> .owner = THIS_MODULE,
> .device_driver = &mtty_driver,
> + .device_api = VFIO_DEVICE_API_PCI_STRING,
> .dev_attr_groups = mtty_dev_groups,
> .supported_type_groups = mdev_type_groups,
> };