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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f48868a9-313d-b3ee-d8b2-98774f0d1cd8@quicinc.com>
Date:   Wed, 8 Feb 2023 11:34:10 -0800
From:   Elliot Berman <quic_eberman@...cinc.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Bjorn Andersson <quic_bjorande@...cinc.com>,
        Alex Elder <elder@...aro.org>,
        Murali Nalajala <quic_mnalajal@...cinc.com>,
        Jonathan Corbet <corbet@....net>
CC:     Trilok Soni <quic_tsoni@...cinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        "Will Deacon" <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Sudeep Holla <sudeep.holla@....com>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v9 21/27] gunyah: vm_mgr: Add framework to add VM
 Functions



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