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
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.
Signed-off-by: Elliot Berman <[email protected]>
---
Documentation/virt/gunyah/vm-manager.rst | 18 +++
drivers/virt/gunyah/vm_mgr.c | 187 ++++++++++++++++++++++-
drivers/virt/gunyah/vm_mgr.h | 5 +
include/linux/gunyah_vm_mgr.h | 68 +++++++++
include/uapi/linux/gunyah.h | 13 ++
5 files changed, 289 insertions(+), 2 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 62513af09cbf..d929f8f20b3b 100644
--- a/Documentation/virt/gunyah/vm-manager.rst
+++ b/Documentation/virt/gunyah/vm-manager.rst
@@ -17,6 +17,24 @@ sharing userspace memory with a VM is done via the GH_VM_SET_USER_MEM_REGION
ioctl. The VM itself is configured to use the memory region via the
devicetree.
+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.
+
+Functions typically will always do at least one of these operations:
+
+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 gunyah_resource` when the
+ 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 48bd3f06fb6c..1e795f3d19d5 100644
--- a/drivers/virt/gunyah/vm_mgr.c
+++ b/drivers/virt/gunyah/vm_mgr.c
@@ -16,7 +16,147 @@
#include "vm_mgr.h"
-static __must_check struct gunyah_vm *gunyah_vm_alloc(struct gh_rm_rpc *rm)
+static DEFINE_MUTEX(functions_lock);
+static LIST_HEAD(functions);
+
+static struct gunyah_vm_function_driver *__find_function(const char name[GUNYAH_FUNCTION_NAME_SIZE])
+ __must_hold(functions_lock)
+{
+ struct gunyah_vm_function_driver *iter, *drv = NULL;
+
+ list_for_each_entry(iter, &functions, list) {
+ if (!strncmp(iter->name, name, GUNYAH_FUNCTION_NAME_SIZE)) {
+ drv = iter;
+ break;
+ }
+ }
+
+ return drv;
+}
+
+int gunyah_vm_function_register(struct gunyah_vm_function_driver *drv)
+{
+ int ret = 0;
+
+ mutex_lock(&functions_lock);
+ if (__find_function(drv->name)) {
+ ret = -EEXIST;
+ goto out;
+ }
+
+ INIT_LIST_HEAD(&drv->instances);
+ list_add(&drv->list, &functions);
+out:
+ mutex_unlock(&functions_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gunyah_vm_function_register);
+
+static void gh_vm_remove_function(struct gunyah_vm_function *f)
+ __must_hold(functions_lock)
+{
+ f->drv->release(f);
+ list_del(&f->vm_list);
+ list_del(&f->drv_list);
+ module_put(f->drv->mod);
+ kfree(f);
+}
+
+void gunyah_vm_function_unregister(struct gunyah_vm_function_driver *drv)
+{
+ struct gunyah_vm_function *f, *iter;
+
+ mutex_lock(&functions_lock);
+ list_for_each_entry_safe(f, iter, &drv->instances, drv_list)
+ gh_vm_remove_function(f);
+ list_del(&drv->list);
+ mutex_unlock(&functions_lock);
+}
+EXPORT_SYMBOL_GPL(gunyah_vm_function_unregister);
+
+static long gh_vm_add_function(struct gunyah_vm *ghvm, struct gunyah_vm_function *f)
+{
+ long r = 0;
+
+ mutex_lock(&functions_lock);
+ f->drv = __find_function(f->fn.name);
+ if (!f->drv) {
+ mutex_unlock(&functions_lock);
+ r = request_module("ghfunc:%s", f->fn.name);
+ if (r)
+ return r;
+
+ mutex_lock(&functions_lock);
+ f->drv = __find_function(f->fn.name);
+ }
+
+ if (!f->drv) {
+ r = -ENOENT;
+ goto out;
+ }
+
+ if (!try_module_get(f->drv->mod)) {
+ r = -ENOENT;
+ f->drv = NULL;
+ goto out;
+ }
+
+ f->ghvm = ghvm;
+ f->rm = ghvm->rm;
+
+ r = f->drv->bind(f);
+ if (r < 0) {
+ module_put(f->drv->mod);
+ goto out;
+ }
+
+ list_add(&f->vm_list, &ghvm->functions);
+ list_add(&f->drv_list, &f->drv->instances);
+out:
+ mutex_unlock(&functions_lock);
+ return r;
+}
+
+static long gh_vm_rm_function(struct gunyah_vm *ghvm, struct gh_vm_function *fn)
+{
+ long r = 0;
+ struct gunyah_vm_function *f, *iter;
+
+ r = mutex_lock_interruptible(&functions_lock);
+ if (r)
+ return r;
+
+ list_for_each_entry_safe(f, iter, &ghvm->functions, vm_list) {
+ if (!memcmp(&f->fn, fn, sizeof(*fn)))
+ gh_vm_remove_function(f);
+ }
+
+ mutex_unlock(&functions_lock);
+ return 0;
+}
+
+static void ghvm_put(struct kref *kref)
+{
+ struct gunyah_vm *ghvm = container_of(kref, struct gunyah_vm, kref);
+
+ gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid);
+ put_gh_rm(ghvm->rm);
+ kfree(ghvm);
+}
+
+int __must_check get_gunyah_vm(struct gunyah_vm *ghvm)
+{
+ return kref_get_unless_zero(&ghvm->kref);
+}
+EXPORT_SYMBOL_GPL(get_gunyah_vm);
+
+void put_gunyah_vm(struct gunyah_vm *ghvm)
+{
+ kref_put(&ghvm->kref, ghvm_put);
+}
+EXPORT_SYMBOL_GPL(put_gunyah_vm);
+
+static __must_check struct gunyah_vm *gunyah_vm_alloc(struct gh_rm *rm)
{
struct gunyah_vm *ghvm;
int vmid;
@@ -39,6 +179,8 @@ static __must_check struct gunyah_vm *gunyah_vm_alloc(struct gh_rm_rpc *rm)
mutex_init(&ghvm->mm_lock);
INIT_LIST_HEAD(&ghvm->memory_mappings);
init_rwsem(&ghvm->status_lock);
+ kref_init(&ghvm->kref);
+ INIT_LIST_HEAD(&ghvm->functions);
return ghvm;
}
@@ -192,6 +334,39 @@ static long gh_vm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
r = -EINVAL;
break;
}
+ case GH_VM_ADD_FUNCTION: {
+ struct gunyah_vm_function *f;
+
+ r = -ENOMEM;
+ f = kzalloc(sizeof(*f), GFP_KERNEL);
+ if (!f)
+ break;
+
+ r = -EFAULT;
+ if (copy_from_user(&f->fn, argp, sizeof(f->fn)))
+ break;
+
+ r = gh_vm_add_function(ghvm, f);
+ if (r < 0)
+ kfree(f);
+ break;
+ }
+ case GH_VM_REMOVE_FUNCTION: {
+ struct gh_vm_function *fn;
+
+ r = -ENOMEM;
+ fn = kzalloc(sizeof(*fn), GFP_KERNEL);
+ if (!fn)
+ break;
+
+ r = -EFAULT;
+ if (copy_from_user(fn, argp, sizeof(*fn)))
+ break;
+
+ r = gh_vm_rm_function(ghvm, fn);
+ kfree(fn);
+ break;
+ }
default:
r = -ENOTTY;
break;
@@ -204,15 +379,23 @@ static int gh_vm_release(struct inode *inode, struct file *filp)
{
struct gunyah_vm *ghvm = filp->private_data;
struct gunyah_vm_memory_mapping *mapping, *tmp;
+ struct gunyah_vm_function *f, *fiter;
gh_vm_stop(ghvm);
+ mutex_lock(&functions_lock);
+ list_for_each_entry_safe(f, fiter, &ghvm->functions, vm_list) {
+ gh_vm_remove_function(f);
+ }
+ mutex_unlock(&functions_lock);
+
list_for_each_entry_safe(mapping, tmp, &ghvm->memory_mappings, list) {
gh_vm_mem_mapping_reclaim(ghvm, mapping);
kfree(mapping);
}
+
put_gh_rm(ghvm->rm);
- kfree(ghvm);
+ put_gunyah_vm(ghvm);
return 0;
}
diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h
index 5c02fb305893..8d3b0678fb96 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>
@@ -41,8 +42,12 @@ struct gunyah_vm {
enum gh_rm_vm_status vm_status;
struct rw_semaphore status_lock;
+ struct kref kref;
+
struct mutex mm_lock;
struct list_head memory_mappings;
+
+ struct list_head functions;
};
struct gunyah_vm_memory_mapping *gh_vm_mem_mapping_alloc(struct gunyah_vm *ghvm,
diff --git a/include/linux/gunyah_vm_mgr.h b/include/linux/gunyah_vm_mgr.h
new file mode 100644
index 000000000000..69f98eb503e9
--- /dev/null
+++ b/include/linux/gunyah_vm_mgr.h
@@ -0,0 +1,68 @@
+/* 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 gunyah_vm;
+
+int __must_check get_gunyah_vm(struct gunyah_vm *ghvm);
+void put_gunyah_vm(struct gunyah_vm *ghvm);
+
+struct gunyah_vm_function;
+struct gunyah_vm_function_driver {
+ const char name[GUNYAH_FUNCTION_NAME_SIZE];
+ struct module *mod;
+ long (*bind)(struct gunyah_vm_function *f);
+ void (*release)(struct gunyah_vm_function *f);
+ struct list_head list;
+ struct list_head instances;
+};
+
+struct gunyah_vm_function {
+ struct gh_vm_function fn;
+ struct gunyah_vm *ghvm;
+ struct gh_rm *rm;
+ struct gunyah_vm_function_driver *drv;
+ void *data;
+ struct list_head vm_list;
+ struct list_head drv_list;
+};
+
+int gunyah_vm_function_register(struct gunyah_vm_function_driver *f);
+void gunyah_vm_function_unregister(struct gunyah_vm_function_driver *f);
+
+#define DECLARE_GUNYAH_VM_FUNCTION(_name, _bind, _release) \
+ static struct gunyah_vm_function_driver _name = { \
+ .name = __stringify(_name), \
+ .mod = THIS_MODULE, \
+ .bind = _bind, \
+ .release = _release, \
+ }; \
+ MODULE_ALIAS("ghfunc:"__stringify(_name))
+
+#define DECLARE_GUNYAH_VM_FUNCTION_INIT(_name, _bind, _release) \
+ DECLARE_GUNYAH_VM_FUNCTION(_name, _bind, _release); \
+ static int __init _name##_mod_init(void) \
+ { \
+ return gunyah_vm_function_register(&(_name)); \
+ } \
+ module_init(_name##_mod_init); \
+ static void __exit _name##_mod_exit(void) \
+ { \
+ gunyah_vm_function_unregister(&(_name)); \
+ } \
+ module_exit(_name##_mod_exit)
+
+#endif
diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
index 36359ad2175e..ec8da6fde045 100644
--- a/include/uapi/linux/gunyah.h
+++ b/include/uapi/linux/gunyah.h
@@ -50,4 +50,17 @@ struct gh_vm_dtb_config {
#define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3)
+#define GUNYAH_FUNCTION_NAME_SIZE 32
+#define GUNYAH_FUNCTION_MAX_ARG_SIZE 1024
+
+struct gh_vm_function {
+ char name[GUNYAH_FUNCTION_NAME_SIZE];
+ union {
+ char data[GUNYAH_FUNCTION_MAX_ARG_SIZE];
+ };
+};
+
+#define GH_VM_ADD_FUNCTION _IOW(GH_IOCTL_TYPE, 0x4, struct gh_vm_function)
+#define GH_VM_REMOVE_FUNCTION _IOW(GH_IOCTL_TYPE, 0x7, struct gh_vm_function)
+
#endif
--
2.39.0
* Elliot Berman <[email protected]> [2023-01-20 14:46:20]:
> +static struct gunyah_vm_function_driver *__find_function(const char name[GUNYAH_FUNCTION_NAME_SIZE])
> + __must_hold(functions_lock)
> +{
> + struct gunyah_vm_function_driver *iter, *drv = NULL;
> +
> + list_for_each_entry(iter, &functions, list) {
> + if (!strncmp(iter->name, name, GUNYAH_FUNCTION_NAME_SIZE)) {
> + drv = iter;
> + break;
> + }
> + }
Not sure how much of a hot path this is going to sit in. I can imagine VM boot
to be in fast path for some cases (VMs spawned on usecase boundaries - I think
some VMs like in Amazon firecracker boot in fraction of a second). This
indirection could cost that a bit (linear search + strcmp for the right
function). IMHO a direct interface (ex: ADD_IOEVENTFD) will be more efficient.
> +void gunyah_vm_function_unregister(struct gunyah_vm_function_driver *drv)
> +{
> + struct gunyah_vm_function *f, *iter;
> +
> + mutex_lock(&functions_lock);
> + list_for_each_entry_safe(f, iter, &drv->instances, drv_list)
> + gh_vm_remove_function(f);
> + list_del(&drv->list);
> + mutex_unlock(&functions_lock);
This seems to allow essential functions to be unregistered while there are still
active users. For example, it would allow ioeventfd or irqfd module to be
unloaded while there are VMs depending on it. I think it would be better if we
allow module unload (aka function_unregister) only after dependent VMs are stopped.
> +static long gh_vm_rm_function(struct gunyah_vm *ghvm, struct gh_vm_function *fn)
> +{
> + long r = 0;
> + struct gunyah_vm_function *f, *iter;
> +
> + r = mutex_lock_interruptible(&functions_lock);
> + if (r)
> + return r;
> +
> + list_for_each_entry_safe(f, iter, &ghvm->functions, vm_list) {
> + if (!memcmp(&f->fn, fn, sizeof(*fn)))
> + gh_vm_remove_function(f);
> + }
> +
I think we should return some error (for ioctl atleast) if given function was
not found.
> +struct gh_vm_function {
> + char name[GUNYAH_FUNCTION_NAME_SIZE];
> + union {
> + char data[GUNYAH_FUNCTION_MAX_ARG_SIZE];
> + };
Can you find a way to optimize this memory/time usage? For example, in case of
ioevents we strictly need 28 bytes, but end up consuming 1kB.
Also we end up comparing 1024 bytes during REMOVE_FUNCTION when strictly 28
bytes or fewer bytes comparison was required in case of ioeventfd.
* Srivatsa Vaddagiri <[email protected]> [2023-02-03 15:07:14]:
> * Elliot Berman <[email protected]> [2023-01-20 14:46:20]:
>
> > +static struct gunyah_vm_function_driver *__find_function(const char name[GUNYAH_FUNCTION_NAME_SIZE])
> > + __must_hold(functions_lock)
> > +{
> > + struct gunyah_vm_function_driver *iter, *drv = NULL;
> > +
> > + list_for_each_entry(iter, &functions, list) {
> > + if (!strncmp(iter->name, name, GUNYAH_FUNCTION_NAME_SIZE)) {
> > + drv = iter;
> > + break;
> > + }
> > + }
>
> Not sure how much of a hot path this is going to sit in. I can imagine VM boot
> to be in fast path for some cases (VMs spawned on usecase boundaries - I think
> some VMs like in Amazon firecracker boot in fraction of a second). This
> indirection could cost that a bit (linear search + strcmp for the right
> function). IMHO a direct interface (ex: ADD_IOEVENTFD) will be more efficient.
At the minimum, I think you can make iter->name an enum, which will make the
search faster.
-vatsa
On 20/01/2023 22:46, 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
> 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.
>
> Signed-off-by: Elliot Berman <[email protected]>
> ---
> Documentation/virt/gunyah/vm-manager.rst | 18 +++
> drivers/virt/gunyah/vm_mgr.c | 187 ++++++++++++++++++++++-
> drivers/virt/gunyah/vm_mgr.h | 5 +
> include/linux/gunyah_vm_mgr.h | 68 +++++++++
> include/uapi/linux/gunyah.h | 13 ++
> 5 files changed, 289 insertions(+), 2 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 62513af09cbf..d929f8f20b3b 100644
> --- a/Documentation/virt/gunyah/vm-manager.rst
> +++ b/Documentation/virt/gunyah/vm-manager.rst
> @@ -17,6 +17,24 @@ sharing userspace memory with a VM is done via the GH_VM_SET_USER_MEM_REGION
> ioctl. The VM itself is configured to use the memory region via the
> devicetree.
>
> +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.
> +
> +Functions typically will always do at least one of these operations:
> +
> +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 gunyah_resource` when the
> + 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 48bd3f06fb6c..1e795f3d19d5 100644
> --- a/drivers/virt/gunyah/vm_mgr.c
> +++ b/drivers/virt/gunyah/vm_mgr.c
> @@ -16,7 +16,147 @@
>
> #include "vm_mgr.h"
>
> -static __must_check struct gunyah_vm *gunyah_vm_alloc(struct gh_rm_rpc *rm)
> +static DEFINE_MUTEX(functions_lock);
> +static LIST_HEAD(functions);
> +
> +static struct gunyah_vm_function_driver *__find_function(const char name[GUNYAH_FUNCTION_NAME_SIZE])
> + __must_hold(functions_lock)
> +{
> + struct gunyah_vm_function_driver *iter, *drv = NULL;
> +
> + list_for_each_entry(iter, &functions, list) {
> + if (!strncmp(iter->name, name, GUNYAH_FUNCTION_NAME_SIZE)) {
> + drv = iter;
> + break;
> + }
> + }
> +
> + return drv;
> +}
> +
> +int gunyah_vm_function_register(struct gunyah_vm_function_driver *drv)
> +{
> + int ret = 0;
> +
> + mutex_lock(&functions_lock);
> + if (__find_function(drv->name)) {
> + ret = -EEXIST;
> + goto out;
> + }
> +
> + INIT_LIST_HEAD(&drv->instances);
> + list_add(&drv->list, &functions);
> +out:
> + mutex_unlock(&functions_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(gunyah_vm_function_register);
> +
> +static void gh_vm_remove_function(struct gunyah_vm_function *f)
> + __must_hold(functions_lock)
> +{
> + f->drv->release(f);
> + list_del(&f->vm_list);
> + list_del(&f->drv_list);
> + module_put(f->drv->mod);
> + kfree(f);
> +}
> +
> +void gunyah_vm_function_unregister(struct gunyah_vm_function_driver *drv)
> +{
> + struct gunyah_vm_function *f, *iter;
> +
> + mutex_lock(&functions_lock);
> + list_for_each_entry_safe(f, iter, &drv->instances, drv_list)
> + gh_vm_remove_function(f);
> + list_del(&drv->list);
> + mutex_unlock(&functions_lock);
> +}
> +EXPORT_SYMBOL_GPL(gunyah_vm_function_unregister);
> +
> +static long gh_vm_add_function(struct gunyah_vm *ghvm, struct gunyah_vm_function *f)
> +{
> + long r = 0;
> +
> + mutex_lock(&functions_lock);
> + f->drv = __find_function(f->fn.name);
> + if (!f->drv) {
> + mutex_unlock(&functions_lock);
> + r = request_module("ghfunc:%s", f->fn.name);
> + if (r)
> + return r;
> +
> + mutex_lock(&functions_lock);
> + f->drv = __find_function(f->fn.name);
> + }
> +
> + if (!f->drv) {
> + r = -ENOENT;
> + goto out;
> + }
> +
> + if (!try_module_get(f->drv->mod)) {
> + r = -ENOENT;
> + f->drv = NULL;
> + goto out;
> + }
> +
> + f->ghvm = ghvm;
> + f->rm = ghvm->rm;
> +
> + r = f->drv->bind(f);
> + if (r < 0) {
> + module_put(f->drv->mod);
> + goto out;
> + }
> +
> + list_add(&f->vm_list, &ghvm->functions);
> + list_add(&f->drv_list, &f->drv->instances);
> +out:
> + mutex_unlock(&functions_lock);
> + return r;
> +}
> +
> +static long gh_vm_rm_function(struct gunyah_vm *ghvm, struct gh_vm_function *fn)
> +{
> + long r = 0;
> + struct gunyah_vm_function *f, *iter;
> +
> + r = mutex_lock_interruptible(&functions_lock);
> + if (r)
> + return r;
> +
> + list_for_each_entry_safe(f, iter, &ghvm->functions, vm_list) {
> + if (!memcmp(&f->fn, fn, sizeof(*fn)))
> + gh_vm_remove_function(f);
> + }
> +
> + mutex_unlock(&functions_lock);
> + return 0;
> +}
> +
> +static void ghvm_put(struct kref *kref)
> +{
> + struct gunyah_vm *ghvm = container_of(kref, struct gunyah_vm, kref);
> +
> + gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid);
> + put_gh_rm(ghvm->rm);
> + kfree(ghvm);
> +}
> +
> +int __must_check get_gunyah_vm(struct gunyah_vm *ghvm)
> +{
> + return kref_get_unless_zero(&ghvm->kref);
> +}
> +EXPORT_SYMBOL_GPL(get_gunyah_vm);
> +
> +void put_gunyah_vm(struct gunyah_vm *ghvm)
> +{
> + kref_put(&ghvm->kref, ghvm_put);
> +}
> +EXPORT_SYMBOL_GPL(put_gunyah_vm);
> +
> +static __must_check struct gunyah_vm *gunyah_vm_alloc(struct gh_rm *rm)
> {
> struct gunyah_vm *ghvm;
> int vmid;
> @@ -39,6 +179,8 @@ static __must_check struct gunyah_vm *gunyah_vm_alloc(struct gh_rm_rpc *rm)
> mutex_init(&ghvm->mm_lock);
> INIT_LIST_HEAD(&ghvm->memory_mappings);
> init_rwsem(&ghvm->status_lock);
> + kref_init(&ghvm->kref);
> + INIT_LIST_HEAD(&ghvm->functions);
>
> return ghvm;
> }
> @@ -192,6 +334,39 @@ static long gh_vm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> r = -EINVAL;
> break;
> }
> + case GH_VM_ADD_FUNCTION: {
> + struct gunyah_vm_function *f;
> +
> + r = -ENOMEM;
> + f = kzalloc(sizeof(*f), GFP_KERNEL);
> + if (!f)
> + break;
> +
> + r = -EFAULT;
> + if (copy_from_user(&f->fn, argp, sizeof(f->fn)))
> + break;
> +
> + r = gh_vm_add_function(ghvm, f);
> + if (r < 0)
> + kfree(f);
> + break;
> + }
> + case GH_VM_REMOVE_FUNCTION: {
> + struct gh_vm_function *fn;
> +
> + r = -ENOMEM;
> + fn = kzalloc(sizeof(*fn), GFP_KERNEL);
> + if (!fn)
> + break;
> +
> + r = -EFAULT;
> + if (copy_from_user(fn, argp, sizeof(*fn)))
> + break;
> +
> + r = gh_vm_rm_function(ghvm, fn);
> + kfree(fn);
> + break;
> + }
> default:
> r = -ENOTTY;
> break;
> @@ -204,15 +379,23 @@ static int gh_vm_release(struct inode *inode, struct file *filp)
> {
> struct gunyah_vm *ghvm = filp->private_data;
> struct gunyah_vm_memory_mapping *mapping, *tmp;
> + struct gunyah_vm_function *f, *fiter;
>
> gh_vm_stop(ghvm);
>
> + mutex_lock(&functions_lock);
> + list_for_each_entry_safe(f, fiter, &ghvm->functions, vm_list) {
> + gh_vm_remove_function(f);
> + }
> + mutex_unlock(&functions_lock);
> +
> list_for_each_entry_safe(mapping, tmp, &ghvm->memory_mappings, list) {
> gh_vm_mem_mapping_reclaim(ghvm, mapping);
> kfree(mapping);
> }
> +
> put_gh_rm(ghvm->rm);
> - kfree(ghvm);
> + put_gunyah_vm(ghvm);
> return 0;
> }
>
> diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h
> index 5c02fb305893..8d3b0678fb96 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>
> @@ -41,8 +42,12 @@ struct gunyah_vm {
> enum gh_rm_vm_status vm_status;
> struct rw_semaphore status_lock;
>
> + struct kref kref;
> +
> struct mutex mm_lock;
> struct list_head memory_mappings;
> +
> + struct list_head functions;
> };
>
> struct gunyah_vm_memory_mapping *gh_vm_mem_mapping_alloc(struct gunyah_vm *ghvm,
> diff --git a/include/linux/gunyah_vm_mgr.h b/include/linux/gunyah_vm_mgr.h
> new file mode 100644
> index 000000000000..69f98eb503e9
> --- /dev/null
> +++ b/include/linux/gunyah_vm_mgr.h
> @@ -0,0 +1,68 @@
> +/* 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 gunyah_vm;
> +
> +int __must_check get_gunyah_vm(struct gunyah_vm *ghvm);
> +void put_gunyah_vm(struct gunyah_vm *ghvm);
> +
> +struct gunyah_vm_function;
> +struct gunyah_vm_function_driver {
> + const char name[GUNYAH_FUNCTION_NAME_SIZE];
> + struct module *mod;
> + long (*bind)(struct gunyah_vm_function *f);
> + void (*release)(struct gunyah_vm_function *f);
> + struct list_head list;
> + struct list_head instances;
> +};
> +
> +struct gunyah_vm_function {
> + struct gh_vm_function fn;
> + struct gunyah_vm *ghvm;
> + struct gh_rm *rm;
> + struct gunyah_vm_function_driver *drv;
> + void *data;
> + struct list_head vm_list;
> + struct list_head drv_list;
> +};
> +
> +int gunyah_vm_function_register(struct gunyah_vm_function_driver *f);
> +void gunyah_vm_function_unregister(struct gunyah_vm_function_driver *f);
> +
> +#define DECLARE_GUNYAH_VM_FUNCTION(_name, _bind, _release) \
> + static struct gunyah_vm_function_driver _name = { \
> + .name = __stringify(_name), \
> + .mod = THIS_MODULE, \
> + .bind = _bind, \
> + .release = _release, \
> + }; \
> + MODULE_ALIAS("ghfunc:"__stringify(_name))
lets not over kill this by having DECLARE_GUNYAH_VM_FUNCTION, this will
make the drivers readable in a more familar way. let the driver define
this static struct.
> +
> +#define DECLARE_GUNYAH_VM_FUNCTION_INIT(_name, _bind, _release) \
> + DECLARE_GUNYAH_VM_FUNCTION(_name, _bind, _release); \
> + static int __init _name##_mod_init(void) \
> + { \
> + return gunyah_vm_function_register(&(_name)); \
> + } \
> + module_init(_name##_mod_init); \
> + static void __exit _name##_mod_exit(void) \
> + { \
> + gunyah_vm_function_unregister(&(_name)); \
> + } \
> + module_exit(_name##_mod_exit)
> +
How about:
#define module_gunyah_function_driver(__gf_driver)
module_driver(__gf_driver, gunyah_vm_function_register, \
gunyah_vm_function_unregister)
Having relook at the patch, I think modeling the gunyah_vm_function as a
proper device and driver model will scale, you could leverage most of
this manual management to the existing driver model. May I suggest to
you take a look at include/linux/auxiliary_bus.h
with that you could model add_functions as
auxiliary_device_add and the respecitive drivers as module_auxiliary_driver.
> +#endif
> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
> index 36359ad2175e..ec8da6fde045 100644
> --- a/include/uapi/linux/gunyah.h
> +++ b/include/uapi/linux/gunyah.h
> @@ -50,4 +50,17 @@ struct gh_vm_dtb_config {
>
> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3)
>
> +#define GUNYAH_FUNCTION_NAME_SIZE 32
> +#define GUNYAH_FUNCTION_MAX_ARG_SIZE 1024
> +
> +struct gh_vm_function {
> + char name[GUNYAH_FUNCTION_NAME_SIZE];
> + union {
> + char data[GUNYAH_FUNCTION_MAX_ARG_SIZE];
Are we missing any thing here, its odd to see a single member union like
this.
if other memembers are part of another patch please move them to this
one as its confusing.
> + };
> +};
> +
> +#define GH_VM_ADD_FUNCTION _IOW(GH_IOCTL_TYPE, 0x4, struct gh_vm_function)
> +#define GH_VM_REMOVE_FUNCTION _IOW(GH_IOCTL_TYPE, 0x7, struct gh_vm_function)
> +
> #endif
On 2/7/2023 5:15 AM, Srinivas Kandagatla wrote:
>
>
> On 20/01/2023 22:46, 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
>> 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.
>>
>> Signed-off-by: Elliot Berman <[email protected]>
>> ---
[snip]
>> +#define DECLARE_GUNYAH_VM_FUNCTION(_name, _bind, _release) \
>> + static struct gunyah_vm_function_driver _name = { \
>> + .name = __stringify(_name), \
>> + .mod = THIS_MODULE, \
>> + .bind = _bind, \
>> + .release = _release, \
>> + }; \
>> + MODULE_ALIAS("ghfunc:"__stringify(_name))
>
> lets not over kill this by having DECLARE_GUNYAH_VM_FUNCTION, this will
> make the drivers readable in a more familar way. let the driver define
> this static struct.
>
>
>> +
>> +#define DECLARE_GUNYAH_VM_FUNCTION_INIT(_name, _bind, _release) \
>> + DECLARE_GUNYAH_VM_FUNCTION(_name, _bind, _release); \
>> + static int __init _name##_mod_init(void) \
>> + { \
>> + return gunyah_vm_function_register(&(_name)); \
>> + } \
>> + module_init(_name##_mod_init); \
>> + static void __exit _name##_mod_exit(void) \
>> + { \
>> + gunyah_vm_function_unregister(&(_name)); \
>> + } \
>> + module_exit(_name##_mod_exit)
>> +
>
> How about:
>
> #define module_gunyah_function_driver(__gf_driver)
> module_driver(__gf_driver, gunyah_vm_function_register, \
> gunyah_vm_function_unregister)
>
> Having relook at the patch, I think modeling the gunyah_vm_function as a
> proper device and driver model will scale, you could leverage most of
> this manual management to the existing driver model. May I suggest to
> you take a look at include/linux/auxiliary_bus.h
> with that you could model add_functions as
> auxiliary_device_add and the respecitive drivers as
> module_auxiliary_driver.
>
I'm not sure if device model can fit well here. I wanted to make sure
that the VM function actually bound to a driver when user requests it
and the driver to be able to return some info about it to the user --
vCPUs return a file descriptor for instance. I could probably make it
work with a device/driver model, but I'm not sure if it should be done
like that.
>> +#endif
>> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
>> index 36359ad2175e..ec8da6fde045 100644
>> --- a/include/uapi/linux/gunyah.h
>> +++ b/include/uapi/linux/gunyah.h
>> @@ -50,4 +50,17 @@ struct gh_vm_dtb_config {
>> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3)
>> +#define GUNYAH_FUNCTION_NAME_SIZE 32
>> +#define GUNYAH_FUNCTION_MAX_ARG_SIZE 1024
>> +
>> +struct gh_vm_function {
>> + char name[GUNYAH_FUNCTION_NAME_SIZE];
>> + union {
>> + char data[GUNYAH_FUNCTION_MAX_ARG_SIZE];
>
> Are we missing any thing here, its odd to see a single member union like
> this.
> if other memembers are part of another patch please move them to this
> one as its confusing.
I can add a comment that members will be added as new functions are
added. If I put it in this patch, it raises questions about where those
other members are being used.