Messages in this thread | | | Date | Wed, 8 Feb 2023 11:34:10 -0800 | Subject | Re: [PATCH v9 21/27] gunyah: vm_mgr: Add framework to add VM Functions | From | 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@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.
| |