Message-ID: <f48868a9-313d-b3ee-d8b2-98774f0d1cd8@quicinc.com>
Date:   Wed, 8 Feb 2023 11:34:10 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v9 21/27] gunyah: vm_mgr: Add framework to add VM
 Functions
Content-Language: en-US
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Bjorn Andersson <quic_bjorande@quicinc.com>,
        Alex Elder <elder@linaro.org>,
        Murali Nalajala <quic_mnalajal@quicinc.com>,
        Jonathan Corbet <corbet@lwn.net>
CC:     Trilok Soni <quic_tsoni@quicinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
        Carl van Schaik <quic_cvanscha@quicinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        "Will Deacon" <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Jassi Brar <jassisinghbrar@gmail.com>,
        Sudeep Holla <sudeep.holla@arm.com>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
References: <20230120224627.4053418-1-quic_eberman@quicinc.com>
 <20230120224627.4053418-22-quic_eberman@quicinc.com>
 <c1564a80-d1be-f31d-2db3-1ec0b847e921@linaro.org>
From:   Elliot Berman <quic_eberman@quicinc.com>
In-Reply-To: <c1564a80-d1be-f31d-2db3-1ec0b847e921@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.