Re: [PATCH v10 15/26] gunyah: rsc_mgr: Add platform ops on mem_lend/mem_reclaim

[Elliot Berman <quic_eberman@quicinc.com>]

On 2/22/2023 2:21 AM, Srinivas Kandagatla wrote:
> 
> 
> On 21/02/2023 21:22, Elliot Berman wrote:
>>
>>
>> On 2/21/2023 6:51 AM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 14/02/2023 21:24, Elliot Berman wrote:
>> [snip]
>>>> +
>>>> +static struct gunyah_rm_platform_ops *rm_platform_ops;
>>>> +static DECLARE_RWSEM(rm_platform_ops_lock);
>>>
>>> Why do we need this read/write lock or this global rm_platform_ops 
>>> here, AFAIU, there will be only one instance of platform_ops per 
>>> platform.
>>>
>>> This should be a core part of the gunyah and its driver early setup, 
>>> that should give us pretty much lock less behaviour.
>>>
>>> We should be able to determine by Hypervisor UUID that its on 
>>> Qualcomm platform or not, during early gunyah setup which should help 
>>> us setup the platfrom ops accordingly.
>>>
>>> This should also help cleanup some of the gunyah code that was added 
>>> futher down in this patchset.
>>
>> I'm guessing the direction to take is:
>>
>>    config GUNYAH
>>      select QCOM_SCM if ARCH_QCOM
> 
> This is how other kernel drivers use SCM.
> 
>>
>> and have vm_mgr call directly into qcom_scm driver if the UID matches?
> 
> Yes that is the plan, we could have these callbacks as part key data 
> structure like struct gh_rm and update it at very early in setup stage 
> based on UUID match.
> 
> 
>>
>> We have an Android requirement to enable CONFIG_GUNYAH=y and 
>> CONFIG_QCOM_SCM=m, but it wouldn't be possible with this design. The 
> 
> Am not sure how this will work, if gunyah for QCOM Platform is depended 
> on SCM then there is no way that gunyah could be a inbuilt and make scm 
> a module. >
> On the other hand with the existing design gunyah will not be functional 
> until scm driver is loaded and platform hooks are registered. This 
> runtime dependency design does not express the dependency correctly and 
> the only way to know if gunyah is functional is keep trying which can 
> only work after scm driver is probed.
> 
> This also raises the design question on how much of platform hooks 
> dependency is captured at gunyah core and api level, with state of 
> current code /dev/gunyah will be created even without platform hooks and 
> let the userspace use it which then only fail at hyp call level.
> 
> Other issue with current design is, scm module can be unloaded under the 
> hood leaving gunyah with NULL pointers to those platform hook functions. 


This is not possible because SCM module can't be unloaded (except with 
CONFIG_MODULE_FORCE_UNLOAD). I can also increase refcount of qcom_scm.ko 
module to be more correct.

> This is the kind of issues we could see if the dependency is not 
> expressed from bottom up. >
> The current design is not really capturing the depended components 
> accurately.
> 
> Considering platform hooks as a core resource to gunyah on Qualcomm 
> platform is something that needs attention. If we can fix that then it 
> might be doable to have QCOM_SCM=m and CONFIG_GUNYAH=y.
> 

I'm open to ideas. I don't see this as being a real-world issue because 
default defconfig has QCOM_SCM=y and all Qualcomm platforms enable 
QCOM_SCM at least as =m.

Thanks,
Elliot

> 
> --srini
>> platform hooks implementation allows GUNYAH and QCOM_SCM to be enabled 
>> without setting lower bound of the other.
>>
>> - Elliot