On Mon, 26 Nov 2018 16:48:51 +0800
Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx> wrote:
> For special mdev type which can aggregate instances for mdev device,
> this extends mdev create interface by allowing extra "aggregate=xxx"
> parameter, which is passed to mdev device model to be able to create
> bundled number of instances for target mdev device.
>
> v2: create new create_with_instances operator for vendor driver
> v3:
> - Change parameter name as "aggregate="
> - Fix new interface comments.
> - Parameter checking for new option, pass UUID string only to
> parse and properly end parameter for kstrtouint() conversion.
Please put the change log under the divider, so git am can
automatically strip it.
>
> Cc: Kirti Wankhede <kwankhede@xxxxxxxxxx>
> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: Cornelia Huck <cohuck@xxxxxxxxxx>
> Signed-off-by: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
> ---
> drivers/vfio/mdev/mdev_core.c | 21 +++++++++++++++++----
> drivers/vfio/mdev/mdev_private.h | 4 +++-
> drivers/vfio/mdev/mdev_sysfs.c | 32 ++++++++++++++++++++++++++++----
> include/linux/mdev.h | 11 +++++++++++
> 4 files changed, 59 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index 0212f0ee8aea..545c52ec7618 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -104,12 +104,17 @@ static inline void mdev_put_parent(struct mdev_parent *parent)
> }
>
> static int mdev_device_create_ops(struct kobject *kobj,
> - struct mdev_device *mdev)
> + struct mdev_device *mdev,
> + unsigned int instances)
> {
> struct mdev_parent *parent = mdev->parent;
> int ret;
>
> - ret = parent->ops->create(kobj, mdev);
> + if (instances > 1) {
> + ret = parent->ops->create_with_instances(kobj, mdev,
> + instances);
> + } else
> + ret = parent->ops->create(kobj, mdev);
So, that implies that a driver that supports aggregation needs to
provide both ->create_with_instances and ->create? Won't that lead
to code duplication in those drivers?
(I'd probably just call ->create_with_instances if it exists and else
use ->create, as you already do sanity checks in mdev_device_create().)
> if (ret)
> return ret;
>
> @@ -276,7 +281,8 @@ static void mdev_device_release(struct device *dev)
> kfree(mdev);
> }
>
> -int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
> +int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid,
> + unsigned int instances)
> {
> int ret;
> struct mdev_device *mdev, *tmp;
> @@ -287,6 +293,12 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
> if (!parent)
> return -EINVAL;
>
> + if (instances > 1 &&
> + !parent->ops->create_with_instances) {
Maybe log a message here, so that the user can get an idea what went
wrong?
> + ret = -EINVAL;
> + goto mdev_fail;
> + }
> +
> mutex_lock(&mdev_list_lock);
>
> /* Check for duplicate */
(...)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index b6e048e1045f..c12c0bfc5598 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -31,6 +31,14 @@ struct mdev_device;
> * @mdev: mdev_device structure on of mediated device
> * that is being created
> * Returns integer: success (0) or error (< 0)
> + * @create_with_instances: Allocate aggregated instances' resources in parent device's
> + * driver for a particular mediated device. Optional if aggregated
> + * resources are not supported.
> + * @kobj: kobject of type for which 'create' is called.
> + * @mdev: mdev_device structure on of mediated device
s/on of/of/
(Yes, I know that @create has the same typo :)
> + * that is being created
> + * @instances: number of instances to aggregate
> + * Returns integer: success (0) or error (< 0)
> * @remove: Called to free resources in parent device's driver for a
> * a mediated device. It is mandatory to provide 'remove'
> * ops.
> @@ -71,6 +79,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);
