Re: [libvirt] [PATCH v2 1/4] vfio/mdev: Add new instances parameter for mdev create

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 20 Jul 2018 10:19:25 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:

> For special mdev type which can aggregate instances for mdev device,
> this extends mdev create interface by allowing extra "instances=xxx"
> parameter, which is passed to mdev device model to be able to create
> arbitrary bundled number of instances for target mdev device.
> 
> v2: create new create_with_instances operator for vendor driver
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/vfio/mdev/mdev_core.c    | 18 +++++++++++++----
>  drivers/vfio/mdev/mdev_private.h |  5 ++++-
>  drivers/vfio/mdev/mdev_sysfs.c   | 34 ++++++++++++++++++++++++++------
>  include/linux/mdev.h             | 10 ++++++++++
>  4 files changed, 56 insertions(+), 11 deletions(-)
> 

(...)

> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index 249472f05509..a06e5b7c69d3 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -54,11 +54,15 @@ static const struct sysfs_ops mdev_type_sysfs_ops = {
>  static ssize_t create_store(struct kobject *kobj, struct device *dev,
>  			    const char *buf, size_t count)
>  {
> -	char *str;
> +	char *str, *opt = NULL;
>  	uuid_le uuid;
>  	int ret;
> +	unsigned int instances = 1;
>  
> -	if ((count < UUID_STRING_LEN) || (count > UUID_STRING_LEN + 1))
> +	if (count < UUID_STRING_LEN)
> +		return -EINVAL;
> +
> +	if (count > UUID_STRING_LEN + 1 + MDEV_CREATE_OPT_LEN)

Do you plan to have other optional parameters? If you don't, you could
probably do a quick exit here if count is between UUID_STRING_LEN + 1
and UUID_STRING_LEN + 12 (for ",instances=<one digit>")?

>  		return -EINVAL;
>  
>  	str = kstrndup(buf, count, GFP_KERNEL);

(...)

> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index b6e048e1045f..cfb702600f95 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -30,6 +30,13 @@ struct mdev_device;
>   *			@kobj: kobject of type for which 'create' is called.
>   *			@mdev: mdev_device structure on of mediated device
>   *			      that is being created
> + * @create_with_instances: Allocate aggregated instances' resources in parent device's
> + *			driver for a particular mediated device. It is optional
> + *			if doesn't support aggregated resources.

"Optional if aggregated resources are not supported"

> + *			@kobj: kobject of type for which 'create' is called.
> + *			@mdev: mdev_device structure on of mediated device
> + *			      that is being created
> + *                      @instances: number of instances to aggregate
>   *			Returns integer: success (0) or error (< 0)

You need that "Returns" line for both the old and the new ops.

>   * @remove:		Called to free resources in parent device's driver for a
>   *			a mediated device. It is mandatory to provide 'remove'
> @@ -71,6 +78,9 @@ struct mdev_parent_ops {
>  	struct attribute_group **supported_type_groups;
>  
>  	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
> +	int     (*create_with_instances)(struct kobject *kobj,
> +					 struct mdev_device *mdev,
> +					 unsigned int instances);
>  	int     (*remove)(struct mdev_device *mdev);
>  	int     (*open)(struct mdev_device *mdev);
>  	void    (*release)(struct mdev_device *mdev);