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

[Elliot Berman]

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