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

  On Qualcomm platforms, there is a firmware entity which controls access
to physical pages. In order to share memory with another VM, this entity
needs to be informed that the guest VM should have access to the memory.

Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 drivers/virt/gunyah/Kconfig                 |  4 ++
 drivers/virt/gunyah/Makefile                |  1 +
 drivers/virt/gunyah/gunyah_platform_hooks.c | 80 +++++++++++++++++++++
 drivers/virt/gunyah/rsc_mgr.h               |  3 +
 drivers/virt/gunyah/rsc_mgr_rpc.c           | 12 +++-
 include/linux/gunyah_rsc_mgr.h              | 17 +++++
 6 files changed, 115 insertions(+), 2 deletions(-)
 create mode 100644 drivers/virt/gunyah/gunyah_platform_hooks.c
  

On 14/02/2023 21:24, Elliot Berman wrote:
> 
> On Qualcomm platforms, there is a firmware entity which controls access
> to physical pages. In order to share memory with another VM, this entity
> needs to be informed that the guest VM should have access to the memory.
> 
> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   drivers/virt/gunyah/Kconfig                 |  4 ++
>   drivers/virt/gunyah/Makefile                |  1 +
>   drivers/virt/gunyah/gunyah_platform_hooks.c | 80 +++++++++++++++++++++
>   drivers/virt/gunyah/rsc_mgr.h               |  3 +
>   drivers/virt/gunyah/rsc_mgr_rpc.c           | 12 +++-
>   include/linux/gunyah_rsc_mgr.h              | 17 +++++
>   6 files changed, 115 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/virt/gunyah/gunyah_platform_hooks.c
> 
> diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
> index 1a737694c333..de815189dab6 100644
> --- a/drivers/virt/gunyah/Kconfig
> +++ b/drivers/virt/gunyah/Kconfig
> @@ -4,6 +4,7 @@ config GUNYAH
>   	tristate "Gunyah Virtualization drivers"
>   	depends on ARM64
>   	depends on MAILBOX
> +	select GUNYAH_PLATFORM_HOOKS
>   	help
>   	  The Gunyah drivers are the helper interfaces that run in a guest VM
>   	  such as basic inter-VM IPC and signaling mechanisms, and higher level
> @@ -11,3 +12,6 @@ config GUNYAH
>   
>   	  Say Y/M here to enable the drivers needed to interact in a Gunyah
>   	  virtual environment.
> +
> +config GUNYAH_PLATFORM_HOOKS
> +	tristate
> diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile
> index ff8bc4925392..6b8f84dbfe0d 100644
> --- a/drivers/virt/gunyah/Makefile
> +++ b/drivers/virt/gunyah/Makefile
> @@ -1,6 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   
>   obj-$(CONFIG_GUNYAH) += gunyah.o
> +obj-$(CONFIG_GUNYAH_PLATFORM_HOOKS) += gunyah_platform_hooks.o
>   
>   gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o vm_mgr_mm.o
>   obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o
> diff --git a/drivers/virt/gunyah/gunyah_platform_hooks.c b/drivers/virt/gunyah/gunyah_platform_hooks.c
> new file mode 100644
> index 000000000000..e67e2361b592
> --- /dev/null
> +++ b/drivers/virt/gunyah/gunyah_platform_hooks.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/rwsem.h>
> +#include <linux/gunyah_rsc_mgr.h>
> +
> +#include "rsc_mgr.h"
> +
> +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.


--srini

> +
> +int gh_rm_platform_pre_mem_share(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel)
> +{
> +	int ret = 0;
> +
> +	down_read(&rm_platform_ops_lock);
> +	if (rm_platform_ops && rm_platform_ops->pre_mem_share)
> +		ret = rm_platform_ops->pre_mem_share(rm, mem_parcel);
> +	up_read(&rm_platform_ops_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gh_rm_platform_pre_mem_share);
> +
> +int gh_rm_platform_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel)
> +{
> +	int ret = 0;
> +
> +	down_read(&rm_platform_ops_lock);
> +	if (rm_platform_ops && rm_platform_ops->post_mem_reclaim)
> +		ret = rm_platform_ops->post_mem_reclaim(rm, mem_parcel);
> +	up_read(&rm_platform_ops_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gh_rm_platform_post_mem_reclaim);
> +
> +int gh_rm_register_platform_ops(struct gunyah_rm_platform_ops *platform_ops)
> +{
> +	int ret = 0;
> +
> +	down_write(&rm_platform_ops_lock);
> +	if (!rm_platform_ops)
> +		rm_platform_ops = platform_ops;
> +	else
> +		ret = -EEXIST;
> +	up_write(&rm_platform_ops_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gh_rm_register_platform_ops);
> +
> +void gh_rm_unregister_platform_ops(struct gunyah_rm_platform_ops *platform_ops)
> +{
> +	down_write(&rm_platform_ops_lock);
> +	if (rm_platform_ops == platform_ops)
> +		rm_platform_ops = NULL;
> +	up_write(&rm_platform_ops_lock);
> +}
> +EXPORT_SYMBOL_GPL(gh_rm_unregister_platform_ops);
> +
> +static void _devm_gh_rm_unregister_platform_ops(void *data)
> +{
> +	gh_rm_unregister_platform_ops(data);
> +}
> +
> +int devm_gh_rm_register_platform_ops(struct device *dev, struct gunyah_rm_platform_ops *ops)
> +{
> +	int ret;
> +
> +	ret = gh_rm_register_platform_ops(ops);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action(dev, _devm_gh_rm_unregister_platform_ops, ops);
> +}
> +EXPORT_SYMBOL_GPL(devm_gh_rm_register_platform_ops);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Gunyah Platform Hooks");
> diff --git a/drivers/virt/gunyah/rsc_mgr.h b/drivers/virt/gunyah/rsc_mgr.h
> index 9b23cefe02b0..e536169df41e 100644
> --- a/drivers/virt/gunyah/rsc_mgr.h
> +++ b/drivers/virt/gunyah/rsc_mgr.h
> @@ -74,6 +74,9 @@ struct gh_rm;
>   int gh_rm_call(struct gh_rm *rsc_mgr, u32 message_id, void *req_buff, size_t req_buff_size,
>   		void **resp_buf, size_t *resp_buff_size);
>   
> +int gh_rm_platform_pre_mem_share(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel);
> +int gh_rm_platform_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel);
> +
>   /* Message IDs: Memory Management */
>   #define GH_RM_RPC_MEM_LEND			0x51000012
>   #define GH_RM_RPC_MEM_SHARE			0x51000013
> diff --git a/drivers/virt/gunyah/rsc_mgr_rpc.c b/drivers/virt/gunyah/rsc_mgr_rpc.c
> index 0c83b097fec9..0b12696bf069 100644
> --- a/drivers/virt/gunyah/rsc_mgr_rpc.c
> +++ b/drivers/virt/gunyah/rsc_mgr_rpc.c
> @@ -116,6 +116,12 @@ static int gh_rm_mem_lend_common(struct gh_rm *rm, u32 message_id, struct gh_rm_
>   	if (!msg)
>   		return -ENOMEM;
>   
> +	ret = gh_rm_platform_pre_mem_share(rm, p);
> +	if (ret) {
> +		kfree(msg);
> +		return ret;
> +	}
> +
>   	req_header = msg;
>   	acl_section = (void *)req_header + sizeof(*req_header);
>   	mem_section = (void *)acl_section + struct_size(acl_section, entries, p->n_acl_entries);
> @@ -139,8 +145,10 @@ static int gh_rm_mem_lend_common(struct gh_rm *rm, u32 message_id, struct gh_rm_
>   	ret = gh_rm_call(rm, message_id, msg, msg_size, (void **)&resp, &resp_size);
>   	kfree(msg);
>   
> -	if (ret)
> +	if (ret) {
> +		gh_rm_platform_post_mem_reclaim(rm, p);
>   		return ret;
> +	}
>   
>   	p->mem_handle = le32_to_cpu(*resp);
>   
> @@ -204,7 +212,7 @@ int gh_rm_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_parcel *parcel)
>   	if (ret)
>   		return ret;
>   
> -	return ret;
> +	return gh_rm_platform_post_mem_reclaim(rm, parcel);
>   }
>   
>   /**
> diff --git a/include/linux/gunyah_rsc_mgr.h b/include/linux/gunyah_rsc_mgr.h
> index 9cffee6f9b4e..dc05d5b1e1a3 100644
> --- a/include/linux/gunyah_rsc_mgr.h
> +++ b/include/linux/gunyah_rsc_mgr.h
> @@ -147,4 +147,21 @@ int gh_rm_get_hyp_resources(struct gh_rm *rm, u16 vmid,
>   				struct gh_rm_hyp_resources **resources);
>   int gh_rm_get_vmid(struct gh_rm *rm, u16 *vmid);
>   
> +struct gunyah_rm_platform_ops {
> +	int (*pre_mem_share)(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel);
> +	int (*post_mem_reclaim)(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel);
> +};
> +
> +#if IS_ENABLED(CONFIG_GUNYAH_PLATFORM_HOOKS)
> +int gh_rm_register_platform_ops(struct gunyah_rm_platform_ops *platform_ops);
> +void gh_rm_unregister_platform_ops(struct gunyah_rm_platform_ops *platform_ops);
> +int devm_gh_rm_register_platform_ops(struct device *dev, struct gunyah_rm_platform_ops *ops);
> +#else
> +static inline int gh_rm_register_platform_ops(struct gunyah_rm_platform_ops *platform_ops)
> +	{ return 0; }
> +static inline void gh_rm_unregister_platform_ops(struct gunyah_rm_platform_ops *platform_ops) { }
> +static inline int devm_gh_rm_register_platform_ops(struct device *dev,
> +	struct gunyah_rm_platform_ops *ops) { return 0; }
> +#endif
> +
>   #endif
  

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

and have vm_mgr call directly into qcom_scm driver if the UID matches?

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 
platform hooks implementation allows GUNYAH and QCOM_SCM to be enabled 
without setting lower bound of the other.

- Elliot
  

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


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

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