[PATCH v14 18/25] gunyah: vm_mgr: Add framework for VM Functions
Alex Elder
elder at linaro.org
Fri Jun 16 09:34:29 PDT 2023
On 6/13/23 12:20 PM, Elliot Berman wrote:
> Introduce a framework for Gunyah userspace to install VM functions. VM
> functions are optional interfaces to the virtual machine. vCPUs,
> ioeventfs, and irqfds are examples of such VM functions and are
s/ioventfs/ioeventfds/
> implemented in subsequent patches.
>
> A generic framework is implemented instead of individual ioctls to
> create vCPUs, irqfds, etc., in order to simplify the VM manager core
> implementation and allow dynamic loading of VM function modules.
I have some comments, and one question about the user space
argument passed during function instance removal. Assuming
you explain/address that:
Reviewed-by: Alex Elder <elder at linaro.org>
>
> Signed-off-by: Elliot Berman <quic_eberman at quicinc.com>
> ---
> Documentation/virt/gunyah/vm-manager.rst | 18 ++
> drivers/virt/gunyah/vm_mgr.c | 213 ++++++++++++++++++++++-
> drivers/virt/gunyah/vm_mgr.h | 4 +
> include/linux/gunyah_vm_mgr.h | 97 +++++++++++
> include/uapi/linux/gunyah.h | 18 ++
> 5 files changed, 347 insertions(+), 3 deletions(-)
> create mode 100644 include/linux/gunyah_vm_mgr.h
>
> diff --git a/Documentation/virt/gunyah/vm-manager.rst b/Documentation/virt/gunyah/vm-manager.rst
> index df0e1a8279bf5..cbc79bd8749b4 100644
> --- a/Documentation/virt/gunyah/vm-manager.rst
> +++ b/Documentation/virt/gunyah/vm-manager.rst
> @@ -15,6 +15,24 @@ such as vCPUs for the VM. Memory can be shared with the VM with
> `GH_VM_SET_USER_MEM_REGION`_. Userspace can interact with the resources in Linux
> by adding "functions" to the VM.
>
> +Gunyah Functions
> +================
> +
> +Components of a Gunyah VM's configuration that need kernel configuration are
> +called "functions" and are built on top of a framework. Functions are identified
> +by a string and have some argument(s) to configure them. They are typically
> +created by the `GH_VM_ADD_FUNCTION`_ ioctl.
Regarding "typically": when are they not aded by this IOCTL?
> +
> +Functions typically will always do at least one of these operations:
Again, "typically" or "always"? I just find using both of those
in the same sentence a little odd.
> +
> +1. Create resource ticket(s). Resource tickets allow a function to register
> + itself as the client for a Gunyah resource (e.g. doorbell or vCPU) and
> + the function is given the pointer to the &struct gh_resource when the
s/when the/when a/
> + VM is starting.
> +
> +2. Register IO handler(s). IO handlers allow a function to handle stage-2 faults
> + from the virtual machine.
> +
> Sample Userspace VMM
> ====================
>
> diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c
> index 562ae6ed4a5f1..cfd79bc9900fd 100644
> --- a/drivers/virt/gunyah/vm_mgr.c
> +++ b/drivers/virt/gunyah/vm_mgr.c
> @@ -6,10 +6,13 @@
> #define pr_fmt(fmt) "gh_vm_mgr: " fmt
>
> #include <linux/anon_inodes.h>
> +#include <linux/compat.h>
> #include <linux/file.h>
> #include <linux/gunyah_rsc_mgr.h>
> +#include <linux/gunyah_vm_mgr.h>
> #include <linux/miscdevice.h>
> #include <linux/module.h>
> +#include <linux/xarray.h>
>
> #include <uapi/linux/gunyah.h>
>
> @@ -17,6 +20,169 @@
>
> static void gh_vm_free(struct work_struct *work);
>
> +static DEFINE_XARRAY(gh_vm_functions);
> +
> +static void gh_vm_put_function(struct gh_vm_function *fn)
> +{
> + module_put(fn->mod);
> +}
> +
> +static struct gh_vm_function *gh_vm_get_function(u32 type)
> +{
> + struct gh_vm_function *fn;
> +
> + fn = xa_load(&gh_vm_functions, type);
> + if (!fn) {
> + request_module("ghfunc:%d", type);
> +
> + fn = xa_load(&gh_vm_functions, type);
> + }
> +
> + if (!fn || !try_module_get(fn->mod))
> + fn = ERR_PTR(-ENOENT);
> +
> + return fn;
> +}
> +
> +static void gh_vm_remove_function_instance(struct gh_vm_function_instance *inst)
> + __must_hold(&inst->ghvm->fn_lock)
> +{
> + inst->fn->unbind(inst);
> + list_del(&inst->vm_list);
> + gh_vm_put_function(inst->fn);
> + kfree(inst->argp);
> + kfree(inst);
> +}
> +
> +static void gh_vm_remove_functions(struct gh_vm *ghvm)
> +{
> + struct gh_vm_function_instance *inst, *iiter;
> +
> + mutex_lock(&ghvm->fn_lock);
> + list_for_each_entry_safe(inst, iiter, &ghvm->functions, vm_list) {
> + gh_vm_remove_function_instance(inst);
> + }
> + mutex_unlock(&ghvm->fn_lock);
> +}
> +
> +static long gh_vm_add_function_instance(struct gh_vm *ghvm, struct gh_fn_desc *f)
> +{
> + struct gh_vm_function_instance *inst;
> + void __user *argp;
> + long r = 0;
> +
> + if (f->arg_size > GH_FN_MAX_ARG_SIZE) {
> + dev_err_ratelimited(ghvm->parent, "%s: arg_size > %d\n",
> + __func__, GH_FN_MAX_ARG_SIZE);
> + return -EINVAL;
> + }
> +
> + inst = kzalloc(sizeof(*inst), GFP_KERNEL);
> + if (!inst)
> + return -ENOMEM;
> +
> + inst->arg_size = f->arg_size;
> + if (inst->arg_size) {
> + inst->argp = kzalloc(inst->arg_size, GFP_KERNEL);
> + if (!inst->argp) {
> + r = -ENOMEM;
> + goto free;
> + }
> +
> + argp = u64_to_user_ptr(f->arg);
> + if (copy_from_user(inst->argp, argp, f->arg_size)) {
> + r = -EFAULT;
> + goto free_arg;
> + }
> + }
> +
> + inst->fn = gh_vm_get_function(f->type);
> + if (IS_ERR(inst->fn)) {
> + r = PTR_ERR(inst->fn);
> + goto free_arg;
> + }
> +
> + inst->ghvm = ghvm;
> + inst->rm = ghvm->rm;
> +
> + mutex_lock(&ghvm->fn_lock);
> + r = inst->fn->bind(inst);
> + if (r < 0) {
> + mutex_unlock(&ghvm->fn_lock);
> + gh_vm_put_function(inst->fn);
> + goto free_arg;
> + }
> +
> + list_add(&inst->vm_list, &ghvm->functions);
> + mutex_unlock(&ghvm->fn_lock);
> +
> + return r;
> +free_arg:
> + kfree(inst->argp);
> +free:
> + kfree(inst);
> + return r;
> +}
> +
> +static long gh_vm_rm_function_instance(struct gh_vm *ghvm, struct gh_fn_desc *f)
> +{
> + struct gh_vm_function_instance *inst, *iter;
> + void __user *user_argp;
> + void *argp;
> + long r = 0;
> +
> + r = mutex_lock_interruptible(&ghvm->fn_lock);
> + if (r)
> + return r;
> +
> + if (f->arg_size) {
> + argp = kzalloc(f->arg_size, GFP_KERNEL);
> + if (!argp) {
> + r = -ENOMEM;
> + goto out;
> + }
> +
> + user_argp = u64_to_user_ptr(f->arg);
What is the user pointer even needed for here? What
is the purpose of copying it into the local buffer?
You never reference it after this point.
I could envision it being usable (and so it makes
some sense being part of the user space API), but
at least at this time it serves no purpose.
> + if (copy_from_user(argp, user_argp, f->arg_size)) {
> + r = -EFAULT;
> + kfree(argp);
> + goto out;
> + }
> +
> + r = -ENOENT;
> + list_for_each_entry_safe(inst, iter, &ghvm->functions, vm_list) {
> + if (inst->fn->type == f->type &&
> + inst->fn->compare(inst, argp, f->arg_size)) {
> + gh_vm_remove_function_instance(inst);
> + r = 0;
> + }
> + }
> +
> + kfree(argp);
> + }
> +
> +out:
> + mutex_unlock(&ghvm->fn_lock);
> + return r;
> +}
> +
> +int gh_vm_function_register(struct gh_vm_function *fn)
> +{
> + if (!fn->bind || !fn->unbind)
> + return -EINVAL;
> +
> + return xa_err(xa_store(&gh_vm_functions, fn->type, fn, GFP_KERNEL));
> +}
> +EXPORT_SYMBOL_GPL(gh_vm_function_register);
> +
> +void gh_vm_function_unregister(struct gh_vm_function *fn)
> +{
> + /* Expecting unregister to only come when unloading a module */
> + WARN_ON(fn->mod && module_refcount(fn->mod));
> + xa_erase(&gh_vm_functions, fn->type);
> +}
> +EXPORT_SYMBOL_GPL(gh_vm_function_unregister);
> +
> static int gh_vm_rm_notification_status(struct gh_vm *ghvm, void *data)
> {
> struct gh_rm_vm_status_payload *payload = data;
> @@ -98,6 +264,8 @@ static __must_check struct gh_vm *gh_vm_alloc(struct gh_rm *rm)
> init_rwsem(&ghvm->status_lock);
> init_waitqueue_head(&ghvm->vm_status_wait);
> INIT_WORK(&ghvm->free_work, gh_vm_free);
> + kref_init(&ghvm->kref);
> + INIT_LIST_HEAD(&ghvm->functions);
> ghvm->vm_status = GH_RM_VM_STATUS_NO_STATE;
>
> return ghvm;
> @@ -254,6 +422,24 @@ static long gh_vm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> r = gh_vm_ensure_started(ghvm);
> break;
> }
> + case GH_VM_ADD_FUNCTION: {
> + struct gh_fn_desc f;
> +
> + if (copy_from_user(&f, argp, sizeof(f)))
> + return -EFAULT;
> +
> + r = gh_vm_add_function_instance(ghvm, &f);
> + break;
> + }
> + case GH_VM_REMOVE_FUNCTION: {
> + struct gh_fn_desc f;
> +
> + if (copy_from_user(&f, argp, sizeof(f)))
> + return -EFAULT;
> +
> + r = gh_vm_rm_function_instance(ghvm, &f);
> + break;
> + }
> default:
> r = -ENOTTY;
> break;
> @@ -270,6 +456,8 @@ static void gh_vm_free(struct work_struct *work)
> if (ghvm->vm_status == GH_RM_VM_STATUS_RUNNING)
> gh_vm_stop(ghvm);
>
> + gh_vm_remove_functions(ghvm);
> +
> if (ghvm->vm_status != GH_RM_VM_STATUS_NO_STATE &&
> ghvm->vm_status != GH_RM_VM_STATUS_LOAD &&
> ghvm->vm_status != GH_RM_VM_STATUS_RESET) {
> @@ -294,14 +482,33 @@ static void gh_vm_free(struct work_struct *work)
> kfree(ghvm);
> }
>
> -static int gh_vm_release(struct inode *inode, struct file *filp)
> +int __must_check gh_vm_get(struct gh_vm *ghvm)
> {
> - struct gh_vm *ghvm = filp->private_data;
> + return kref_get_unless_zero(&ghvm->kref);
> +}
> +EXPORT_SYMBOL_GPL(gh_vm_get);
> +
> +static void _gh_vm_put(struct kref *kref)
> +{
> + struct gh_vm *ghvm = container_of(kref, struct gh_vm, kref);
>
> /* VM will be reset and make RM calls which can interruptible sleep.
> * Defer to a work so this thread can receive signal.
> */
> schedule_work(&ghvm->free_work);
> +}
> +
> +void gh_vm_put(struct gh_vm *ghvm)
> +{
> + kref_put(&ghvm->kref, _gh_vm_put);
> +}
> +EXPORT_SYMBOL_GPL(gh_vm_put);
> +
> +static int gh_vm_release(struct inode *inode, struct file *filp)
> +{
> + struct gh_vm *ghvm = filp->private_data;
> +
> + gh_vm_put(ghvm);
> return 0;
> }
>
> @@ -346,7 +553,7 @@ static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long arg)
> err_put_fd:
> put_unused_fd(fd);
> err_destroy_vm:
> - gh_vm_free(&ghvm->free_work);
> + gh_vm_put(ghvm);
> return err;
> }
>
> diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h
> index 4173bd51f83fe..c4bec1469ae8c 100644
> --- a/drivers/virt/gunyah/vm_mgr.h
> +++ b/drivers/virt/gunyah/vm_mgr.h
> @@ -8,6 +8,7 @@
>
> #include <linux/gunyah_rsc_mgr.h>
> #include <linux/list.h>
> +#include <linux/kref.h>
> #include <linux/miscdevice.h>
> #include <linux/mutex.h>
> #include <linux/rwsem.h>
> @@ -45,9 +46,12 @@ struct gh_vm {
> struct rw_semaphore status_lock;
>
> struct work_struct free_work;
> + struct kref kref;
> struct mm_struct *mm; /* userspace tied to this vm */
> struct mutex mm_lock;
> struct list_head memory_mappings;
> + struct mutex fn_lock;
> + struct list_head functions;
> };
>
> int gh_vm_mem_alloc(struct gh_vm *ghvm, struct gh_userspace_memory_region *region);
> diff --git a/include/linux/gunyah_vm_mgr.h b/include/linux/gunyah_vm_mgr.h
> new file mode 100644
> index 0000000000000..6e4dd6fa5f96d
> --- /dev/null
> +++ b/include/linux/gunyah_vm_mgr.h
> @@ -0,0 +1,97 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _GUNYAH_VM_MGR_H
> +#define _GUNYAH_VM_MGR_H
> +
> +#include <linux/compiler_types.h>
> +#include <linux/gunyah.h>
> +#include <linux/gunyah_rsc_mgr.h>
> +#include <linux/list.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/notifier.h>
> +
> +#include <uapi/linux/gunyah.h>
> +
> +struct gh_vm;
> +
> +int __must_check gh_vm_get(struct gh_vm *ghvm);
> +void gh_vm_put(struct gh_vm *ghvm);
> +
> +struct gh_vm_function_instance;
> +/**
> + * struct gh_vm_function - Represents a function type
> + * @type: value from &enum gh_fn_type
> + * @name: friendly name for debug purposes
> + * @mod: owner of the function type
> + * @bind: Called when a new function of this type has been allocated.
> + * @unbind: Called when the function instance is being destroyed.
> + * @compare: Compare function instance @f's argument to the provided arg.
> + * Return true if they are equivalent. Used on GH_VM_REMOVE_FUNCTION.
> + */
> +struct gh_vm_function {
> + u32 type;
> + const char *name;
> + struct module *mod;
> + long (*bind)(struct gh_vm_function_instance *f);
> + void (*unbind)(struct gh_vm_function_instance *f);
> + bool (*compare)(const struct gh_vm_function_instance *f, const void *arg, size_t size);
> +};
> +
> +/**
> + * struct gh_vm_function_instance - Represents one function instance
> + * @arg_size: size of user argument
> + * @argp: pointer to user argument
> + * @ghvm: Pointer to VM instance
> + * @rm: Pointer to resource manager for the VM instance
> + * @fn: The ops for the function
> + * @data: Private data for function
> + * @vm_list: for gh_vm's functions list
> + * @fn_list: for gh_vm_function's instances list
> + */
> +struct gh_vm_function_instance {
> + size_t arg_size;
> + void *argp;
> + struct gh_vm *ghvm;
> + struct gh_rm *rm;
> + struct gh_vm_function *fn;
> + void *data;
> + struct list_head vm_list;
> +};
> +
> +int gh_vm_function_register(struct gh_vm_function *f);
> +void gh_vm_function_unregister(struct gh_vm_function *f);
> +
> +/* Since the function identifiers were setup in a uapi header as an
> + * enum and we do no want to change that, the user must supply the expanded
> + * constant as well and the compiler checks they are the same.
> + * See also MODULE_ALIAS_RDMA_NETLINK.
> + */
> +#define MODULE_ALIAS_GH_VM_FUNCTION(_type, _idx) \
> + static inline void __maybe_unused __chk##_idx(void) \
> + { \
> + BUILD_BUG_ON(_type != _idx); \
> + } \
> + MODULE_ALIAS("ghfunc:" __stringify(_idx))
> +
> +#define DECLARE_GH_VM_FUNCTION(_name, _type, _bind, _unbind, _compare) \
> + static struct gh_vm_function _name = { \
> + .type = _type, \
> + .name = __stringify(_name), \
> + .mod = THIS_MODULE, \
> + .bind = _bind, \
> + .unbind = _unbind, \
> + .compare = _compare, \
> + }
> +
> +#define module_gh_vm_function(__gf) \
> + module_driver(__gf, gh_vm_function_register, gh_vm_function_unregister)
> +
> +#define DECLARE_GH_VM_FUNCTION_INIT(_name, _type, _idx, _bind, _unbind, _compare) \
> + DECLARE_GH_VM_FUNCTION(_name, _type, _bind, _unbind, _compare); \
> + module_gh_vm_function(_name); \
> + MODULE_ALIAS_GH_VM_FUNCTION(_type, _idx)
> +
> +#endif
> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
> index 4b63d0b9b8ba7..bb07118a351fd 100644
> --- a/include/uapi/linux/gunyah.h
> +++ b/include/uapi/linux/gunyah.h
> @@ -72,4 +72,22 @@ struct gh_vm_dtb_config {
>
> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3)
>
> +#define GH_FN_MAX_ARG_SIZE 256
> +
> +/**
> + * struct gh_fn_desc - Arguments to create a VM function
> + * @type: Type of the function. See &enum gh_fn_type.
> + * @arg_size: Size of argument to pass to the function. arg_size <= GH_FN_MAX_ARG_SIZE
> + * @arg: Pointer to argument given to the function. See &enum gh_fn_type for expected
> + * arguments for a function type.
> + */
> +struct gh_fn_desc {
> + __u32 type;
> + __u32 arg_size;
> + __u64 arg;
> +};
> +
> +#define GH_VM_ADD_FUNCTION _IOW(GH_IOCTL_TYPE, 0x4, struct gh_fn_desc)
> +#define GH_VM_REMOVE_FUNCTION _IOW(GH_IOCTL_TYPE, 0x7, struct gh_fn_desc)
> +
> #endif
More information about the linux-arm-kernel
mailing list