Re: [PATCH v14 18/25] gunyah: vm_mgr: Add framework for VM Functions
From: Alex Elder <hidden>
Date: 2023-06-16 16:35:06
Also in:
linux-arm-msm, linux-devicetree, linux-doc, lkml
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 <redacted>
quoted hunk ↗ jump to hunk
Signed-off-by: Elliot Berman <redacted> --- 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.hdiff --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/
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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) + +#endifdiff --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
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel