Re: [PATCH v9 21/27] gunyah: vm_mgr: Add framework to add VM Functions

[Elliot Berman]

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 <quic_eberman@xxxxxxxxxxx>
> > ---
[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.