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

Elliot Berman
2023-02-08T19:34:37Z

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@quicinc.com>
>> ---
[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.