| Message ID | 11-v1-7dedf20b2b75+4f785-vfio2_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make vfio_mdev type safe | expand |
On Tue, Mar 23, 2021 at 02:55:28PM -0300, Jason Gunthorpe wrote: > +/* > + * Return the index in supported_type_groups that this mdev_device was created > + * from. > + */ > +unsigned int mdev_get_type_group_id(struct mdev_device *mdev) > +{ > + return mdev->type->type_group_id; > +} > +EXPORT_SYMBOL(mdev_get_type_group_id); > + > +/* > + * Used in mdev_type_attribute sysfs functions to return the index in the > + * supported_type_groups that the sysfs is called from. > + */ > +unsigned int mtype_get_type_group_id(struct kobject *mtype_kobj) > +{ > + return container_of(mtype_kobj, struct mdev_type, kobj)->type_group_id; > +} > +EXPORT_SYMBOL(mtype_get_type_group_id); The single field accessors are a little silly.. Otherwise this looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, March 24, 2021 1:55 AM > > This returns the index in the supported_type_groups array that is > associated with the mdev_type attached to the struct mdev_device or its > containing struct kobject. > > Each mdev_device can be spawned from exactly one mdev_type, which in > turn > originates from exactly one supported_type_group. > > Drivers are using weird string calculations to try and get back to this > index, providing a direct access to the index removes a bunch of wonky > driver code. > > mdev_type->group can be deleted as the group is obtained using the > type_group_id. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > drivers/vfio/mdev/mdev_core.c | 20 ++++++++++++++++++++ > drivers/vfio/mdev/mdev_private.h | 2 +- > drivers/vfio/mdev/mdev_sysfs.c | 15 +++++++++------ > include/linux/mdev.h | 3 +++ > 4 files changed, 33 insertions(+), 7 deletions(-) > > diff --git a/drivers/vfio/mdev/mdev_core.c > b/drivers/vfio/mdev/mdev_core.c > index 493df3da451339..3ba5e9464b4d20 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -33,6 +33,26 @@ struct device *mdev_parent_dev(struct mdev_device > *mdev) > } > EXPORT_SYMBOL(mdev_parent_dev); > > +/* > + * Return the index in supported_type_groups that this mdev_device was > created > + * from. > + */ > +unsigned int mdev_get_type_group_id(struct mdev_device *mdev) > +{ > + return mdev->type->type_group_id; > +} > +EXPORT_SYMBOL(mdev_get_type_group_id); > + > +/* > + * Used in mdev_type_attribute sysfs functions to return the index in the > + * supported_type_groups that the sysfs is called from. > + */ > +unsigned int mtype_get_type_group_id(struct kobject *mtype_kobj) > +{ > + return container_of(mtype_kobj, struct mdev_type, kobj)- > >type_group_id; > +} > +EXPORT_SYMBOL(mtype_get_type_group_id); > + > /* Should be called holding parent_list_lock */ > static struct mdev_parent *__find_parent_device(struct device *dev) > { > diff --git a/drivers/vfio/mdev/mdev_private.h > b/drivers/vfio/mdev/mdev_private.h > index 10eccc35782c4d..a656cfe0346c33 100644 > --- a/drivers/vfio/mdev/mdev_private.h > +++ b/drivers/vfio/mdev/mdev_private.h > @@ -29,7 +29,7 @@ struct mdev_type { > struct kobject *devices_kobj; > struct mdev_parent *parent; > struct list_head next; > - struct attribute_group *group; > + unsigned int type_group_id; > }; > > #define to_mdev_type_attr(_attr) \ > diff --git a/drivers/vfio/mdev/mdev_sysfs.c > b/drivers/vfio/mdev/mdev_sysfs.c > index d43775bd0ba340..91ecccdc2f2ec6 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -92,9 +92,11 @@ static struct kobj_type mdev_type_ktype = { > }; > > static struct mdev_type *add_mdev_supported_type(struct mdev_parent > *parent, > - struct attribute_group > *group) > + unsigned int type_group_id) > { > struct mdev_type *type; > + struct attribute_group *group = > + parent->ops->supported_type_groups[type_group_id]; > int ret; > > if (!group->name) { > @@ -110,6 +112,7 @@ static struct mdev_type > *add_mdev_supported_type(struct mdev_parent *parent, > type->parent = parent; > /* Pairs with the put in mdev_type_release() */ > mdev_get_parent(parent); > + type->type_group_id = type_group_id; > > ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL, > "%s-%s", dev_driver_string(parent->dev), > @@ -135,8 +138,6 @@ static struct mdev_type > *add_mdev_supported_type(struct mdev_parent *parent, > ret = -ENOMEM; > goto attrs_failed; > } > - > - type->group = group; > return type; > > attrs_failed: > @@ -151,8 +152,11 @@ static struct mdev_type > *add_mdev_supported_type(struct mdev_parent *parent, > > static void remove_mdev_supported_type(struct mdev_type *type) > { > + struct attribute_group *group = > + type->parent->ops->supported_type_groups[type- > >type_group_id]; > + > sysfs_remove_files(&type->kobj, > - (const struct attribute **)type->group->attrs); > + (const struct attribute **)group->attrs); > kobject_put(type->devices_kobj); > sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr); > kobject_del(&type->kobj); > @@ -166,8 +170,7 @@ static int add_mdev_supported_type_groups(struct > mdev_parent *parent) > for (i = 0; parent->ops->supported_type_groups[i]; i++) { > struct mdev_type *type; > > - type = add_mdev_supported_type(parent, > - parent->ops- > >supported_type_groups[i]); > + type = add_mdev_supported_type(parent, i); > if (IS_ERR(type)) { > struct mdev_type *ltype, *tmp; > > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > index fb582adda28a9b..41e91936522394 100644 > --- a/include/linux/mdev.h > +++ b/include/linux/mdev.h > @@ -46,6 +46,9 @@ static inline struct device > *mdev_get_iommu_device(struct mdev_device *mdev) > return mdev->iommu_device; > } > > +unsigned int mdev_get_type_group_id(struct mdev_device *mdev); > +unsigned int mtype_get_type_group_id(struct kobject *mtype_kobj); > + > /** > * struct mdev_parent_ops - Structure to be registered for each parent > device to > * register the device to mdev module. > -- > 2.31.0
On Tue, 23 Mar 2021 14:55:28 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > This returns the index in the supported_type_groups array that is > associated with the mdev_type attached to the struct mdev_device or its > containing struct kobject. > > Each mdev_device can be spawned from exactly one mdev_type, which in turn > originates from exactly one supported_type_group. > > Drivers are using weird string calculations to try and get back to this > index, providing a direct access to the index removes a bunch of wonky > driver code. > > mdev_type->group can be deleted as the group is obtained using the > type_group_id. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/mdev/mdev_core.c | 20 ++++++++++++++++++++ > drivers/vfio/mdev/mdev_private.h | 2 +- > drivers/vfio/mdev/mdev_sysfs.c | 15 +++++++++------ > include/linux/mdev.h | 3 +++ > 4 files changed, 33 insertions(+), 7 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On Tue, Mar 23, 2021 at 08:23:30PM +0100, Christoph Hellwig wrote: > On Tue, Mar 23, 2021 at 02:55:28PM -0300, Jason Gunthorpe wrote: > > +/* > > + * Return the index in supported_type_groups that this mdev_device was created > > + * from. > > + */ > > +unsigned int mdev_get_type_group_id(struct mdev_device *mdev) > > +{ > > + return mdev->type->type_group_id; > > +} > > +EXPORT_SYMBOL(mdev_get_type_group_id); > > + > > +/* > > + * Used in mdev_type_attribute sysfs functions to return the index in the > > + * supported_type_groups that the sysfs is called from. > > + */ > > +unsigned int mtype_get_type_group_id(struct kobject *mtype_kobj) > > +{ > > + return container_of(mtype_kobj, struct mdev_type, kobj)->type_group_id; > > +} > > +EXPORT_SYMBOL(mtype_get_type_group_id); > > The single field accessors are a little silly.. Looked at this for a while, and to fix it I'd have to pull in the struct mdev_parent into the public header too (for mtype_get_parent_dev()) and I think the two silly accessors are the lessor evil at that point. Jason
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 493df3da451339..3ba5e9464b4d20 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -33,6 +33,26 @@ struct device *mdev_parent_dev(struct mdev_device *mdev) } EXPORT_SYMBOL(mdev_parent_dev); +/* + * Return the index in supported_type_groups that this mdev_device was created + * from. + */ +unsigned int mdev_get_type_group_id(struct mdev_device *mdev) +{ + return mdev->type->type_group_id; +} +EXPORT_SYMBOL(mdev_get_type_group_id); + +/* + * Used in mdev_type_attribute sysfs functions to return the index in the + * supported_type_groups that the sysfs is called from. + */ +unsigned int mtype_get_type_group_id(struct kobject *mtype_kobj) +{ + return container_of(mtype_kobj, struct mdev_type, kobj)->type_group_id; +} +EXPORT_SYMBOL(mtype_get_type_group_id); + /* Should be called holding parent_list_lock */ static struct mdev_parent *__find_parent_device(struct device *dev) { diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h index 10eccc35782c4d..a656cfe0346c33 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -29,7 +29,7 @@ struct mdev_type { struct kobject *devices_kobj; struct mdev_parent *parent; struct list_head next; - struct attribute_group *group; + unsigned int type_group_id; }; #define to_mdev_type_attr(_attr) \ diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c index d43775bd0ba340..91ecccdc2f2ec6 100644 --- a/drivers/vfio/mdev/mdev_sysfs.c +++ b/drivers/vfio/mdev/mdev_sysfs.c @@ -92,9 +92,11 @@ static struct kobj_type mdev_type_ktype = { }; static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, - struct attribute_group *group) + unsigned int type_group_id) { struct mdev_type *type; + struct attribute_group *group = + parent->ops->supported_type_groups[type_group_id]; int ret; if (!group->name) { @@ -110,6 +112,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, type->parent = parent; /* Pairs with the put in mdev_type_release() */ mdev_get_parent(parent); + type->type_group_id = type_group_id; ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL, "%s-%s", dev_driver_string(parent->dev), @@ -135,8 +138,6 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, ret = -ENOMEM; goto attrs_failed; } - - type->group = group; return type; attrs_failed: @@ -151,8 +152,11 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, static void remove_mdev_supported_type(struct mdev_type *type) { + struct attribute_group *group = + type->parent->ops->supported_type_groups[type->type_group_id]; + sysfs_remove_files(&type->kobj, - (const struct attribute **)type->group->attrs); + (const struct attribute **)group->attrs); kobject_put(type->devices_kobj); sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr); kobject_del(&type->kobj); @@ -166,8 +170,7 @@ static int add_mdev_supported_type_groups(struct mdev_parent *parent) for (i = 0; parent->ops->supported_type_groups[i]; i++) { struct mdev_type *type; - type = add_mdev_supported_type(parent, - parent->ops->supported_type_groups[i]); + type = add_mdev_supported_type(parent, i); if (IS_ERR(type)) { struct mdev_type *ltype, *tmp; diff --git a/include/linux/mdev.h b/include/linux/mdev.h index fb582adda28a9b..41e91936522394 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -46,6 +46,9 @@ static inline struct device *mdev_get_iommu_device(struct mdev_device *mdev) return mdev->iommu_device; } +unsigned int mdev_get_type_group_id(struct mdev_device *mdev); +unsigned int mtype_get_type_group_id(struct kobject *mtype_kobj); + /** * struct mdev_parent_ops - Structure to be registered for each parent device to * register the device to mdev module.
This returns the index in the supported_type_groups array that is associated with the mdev_type attached to the struct mdev_device or its containing struct kobject. Each mdev_device can be spawned from exactly one mdev_type, which in turn originates from exactly one supported_type_group. Drivers are using weird string calculations to try and get back to this index, providing a direct access to the index removes a bunch of wonky driver code. mdev_type->group can be deleted as the group is obtained using the type_group_id. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/mdev/mdev_core.c | 20 ++++++++++++++++++++ drivers/vfio/mdev/mdev_private.h | 2 +- drivers/vfio/mdev/mdev_sysfs.c | 15 +++++++++------ include/linux/mdev.h | 3 +++ 4 files changed, 33 insertions(+), 7 deletions(-)