Re: [PATCH v13 17/24] gunyah: vm_mgr: Add framework for VM Functions

[Alex Elder]

On 5/9/23 3:47 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/ioventfds/

Also, these aren't just examples of VM functions, they *are* the
VM functions implemented.

> 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.

This also allows the set of VM functions to be extended without
updating the API (like it or not).

> Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>

I have a few more comments, but this looks pretty good.

Reviewed-by: Alex Elder <elder@xxxxxxxxxx>

> ---
>   Documentation/virt/gunyah/vm-manager.rst |  18 ++
>   drivers/virt/gunyah/vm_mgr.c             | 216 ++++++++++++++++++++++-
>   drivers/virt/gunyah/vm_mgr.h             |   4 +
>   include/linux/gunyah_vm_mgr.h            |  87 +++++++++
>   include/uapi/linux/gunyah.h              |  18 ++
>   5 files changed, 340 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 50d8ae7fabcd..3b51bab9d793 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.

Is a function *type* (e.g., VCPU or ioeventfd) identified by a string?
Or a function *instance* (e.g. four VCPUs)?  Or both?

> +
> +Functions typically will always do at least one of these operations:

Typically, or always?

> +
> +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
> +   VM is starting.
> +

What I think this means is that tickets are used to allow functions
to be defined *before* the VM is actually started.  So once it starts,
the functions get added.  (I might have this slightly wrong, but in
any case I'm not sure the above sentence is very clear.)

> +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 a800061f56bf..56464451b262 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,172 @@
>   
>   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;
> +	int r;
> +
> +	fn = xa_load(&gh_vm_functions, type);
> +	if (!fn) {
> +		r = request_module("ghfunc:%d", type);
> +		if (r)
> +			return ERR_PTR(r > 0 ? -r : r);

Almost all callers of request_module() simply ignore the
return value.  What positive values are you expecting to
see here (and are you sure they're positive errno values)?

> +
> +		fn = xa_load(&gh_vm_functions, type);
> +	}
> +
> +	if (!fn || !try_module_get(fn->mod))
> +		fn = ERR_PTR(-ENOENT);
> +
> +	return fn;
> +}

. . .