On 06/09/2024 05:32, Gavin Shan wrote:
> Hi Steven,
>
> On 8/19/24 11:19 PM, Steven Price wrote:
>> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>>
>> On Arm CCA, with RMM-v1.0, all MMIO regions are shared. However, in
>> the future, an Arm CCA-v1.0 compliant guest may be run in a lesser
>> privileged partition in the Realm World (with Arm CCA-v1.1 Planes
>> feature). In this case, some of the MMIO regions may be emulated
>> by a higher privileged component in the Realm world, i.e, protected.
>>
>> Thus the guest must decide today, whether a given MMIO region is shared
>> vs Protected and create the stage1 mapping accordingly. On Arm CCA, this
>> detection is based on the "IPA State" (RIPAS == RIPAS_IO). Provide a
>> helper to run this check on a given range of MMIO.
>>
>> Also, provide a arm64 helper which may be hooked in by other solutions.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Signed-off-by: Steven Price <steven.price@xxxxxxx>
>> ---
>> New patch for v5
>> ---
>> arch/arm64/include/asm/io.h | 8 ++++++++
>> arch/arm64/include/asm/rsi.h | 3 +++
>> arch/arm64/include/asm/rsi_cmds.h | 21 +++++++++++++++++++++
>> arch/arm64/kernel/rsi.c | 26 ++++++++++++++++++++++++++
>> 4 files changed, 58 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 1ada23a6ec19..a6c551c5e44e 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -17,6 +17,7 @@
>> #include <asm/early_ioremap.h>
>> #include <asm/alternative.h>
>> #include <asm/cpufeature.h>
>> +#include <asm/rsi.h>
>> /*
>> * Generic IO read/write. These perform native-endian accesses.
>> @@ -318,4 +319,11 @@ extern bool
>> arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
>> unsigned long flags);
>> #define arch_memremap_can_ram_remap arch_memremap_can_ram_remap
>> +static inline bool arm64_is_iomem_private(phys_addr_t phys_addr,
>> size_t size)
>> +{
>> + if (unlikely(is_realm_world()))
>> + return arm64_rsi_is_protected_mmio(phys_addr, size);
>> + return false;
>> +}
>> +
>> #endif /* __ASM_IO_H */
>> diff --git a/arch/arm64/include/asm/rsi.h b/arch/arm64/include/asm/rsi.h
>> index 2bc013badbc3..e31231b50b6a 100644
>> --- a/arch/arm64/include/asm/rsi.h
>> +++ b/arch/arm64/include/asm/rsi.h
>> @@ -13,6 +13,9 @@ DECLARE_STATIC_KEY_FALSE(rsi_present);
>> void __init arm64_rsi_init(void);
>> void __init arm64_rsi_setup_memory(void);
>> +
>> +bool arm64_rsi_is_protected_mmio(phys_addr_t base, size_t size);
>> +
>> static inline bool is_realm_world(void)
>> {
>> return static_branch_unlikely(&rsi_present);
>> diff --git a/arch/arm64/include/asm/rsi_cmds.h
>> b/arch/arm64/include/asm/rsi_cmds.h
>> index 968b03f4e703..c2363f36d167 100644
>> --- a/arch/arm64/include/asm/rsi_cmds.h
>> +++ b/arch/arm64/include/asm/rsi_cmds.h
>> @@ -45,6 +45,27 @@ static inline unsigned long
>> rsi_get_realm_config(struct realm_config *cfg)
>> return res.a0;
>> }
>> +static inline unsigned long rsi_ipa_state_get(phys_addr_t start,
>> + phys_addr_t end,
>> + enum ripas *state,
>> + phys_addr_t *top)
>> +{
>> + struct arm_smccc_res res;
>> +
>> + arm_smccc_smc(SMC_RSI_IPA_STATE_GET,
>> + start, end, 0, 0, 0, 0, 0,
>> + &res);
>> +
>> + if (res.a0 == RSI_SUCCESS) {
>> + if (top)
>> + *top = res.a1;
>> + if (state)
>> + *state = res.a2;
>> + }
>> +
>> + return res.a0;
>> +}
>> +
>> static inline unsigned long rsi_set_addr_range_state(phys_addr_t start,
>> phys_addr_t end,
>> enum ripas state,
>> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
>> index e968a5c9929e..381a5b9a5333 100644
>> --- a/arch/arm64/kernel/rsi.c
>> +++ b/arch/arm64/kernel/rsi.c
>> @@ -67,6 +67,32 @@ void __init arm64_rsi_setup_memory(void)
>> }
>> }
>> +bool arm64_rsi_is_protected_mmio(phys_addr_t base, size_t size)
>> +{
>> + enum ripas ripas;
>> + phys_addr_t end, top;
>> +
>> + /* Overflow ? */
>> + if (WARN_ON(base + size < base))
>> + return false;
>> +
>> + end = ALIGN(base + size, RSI_GRANULE_SIZE);
>> + base = ALIGN_DOWN(base, RSI_GRANULE_SIZE);
>> +
>> + while (base < end) {
>> + if (WARN_ON(rsi_ipa_state_get(base, end, &ripas, &top)))
>> + break;
>> + if (WARN_ON(top <= base))
>> + break;
>> + if (ripas != RSI_RIPAS_IO)
>> + break;
>> + base = top;
>> + }
>> +
>> + return (size && base >= end);
>> +}
>
> I don't understand why @size needs to be checked here. Its initial value
> taken from the input parameter should be larger than zero and its value
> is never updated in the loop. So I'm understanding @size is always larger
> than zero, and the condition would be something like below if I'm correct.
Yes you are correct. I'm not entirely sure why it was written that way.
The only change dropping 'size' as you suggest is that a zero-sized
region is considered protected. But I'd consider it a bug if this is
called with size=0. I'll drop 'size' here.
> return (base >= end); /* RSI_RIPAS_IO returned for all
> granules */
>
> Another issue is @top is always zero with the latest tf-rmm. More details
> are provided below.
That suggests that you are not actually using the 'latest' tf-rmm ;)
(for some definition of 'latest' which might not be obvious!)
