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

[Elliot Berman <quic_eberman@quicinc.com>]

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.