Re: [PATCH v5 07/19] arm64: rsi: Add support for checking whether an MMIO is protected

[Gavin Shan <gshan@redhat.com>]

Hi Steven,

On 8/19/24 11:19 PM, Steven Price wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> 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@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> 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.

        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.

> +EXPORT_SYMBOL(arm64_rsi_is_protected_mmio);
> +
>   void __init arm64_rsi_init(void)
>   {
>   	/*

The unexpected calltrace is continuously observed with host/v4, guest/v5 and
latest upstream tf-rmm on 'WARN_ON(top <= base)' because @top is never updated
by the latest tf-rmm. The following call path indicates how SMC_RSI_IPA_STATE_GET
is handled by tf-rmm. I don't see RSI_RIPAS_IO is defined there and @top is
updated.

[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at arch/arm64/kernel/rsi.c:103 arm64_rsi_is_protected_mmio+0xf0/0x110
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.11.0-rc1-gavin-g3527d001084e #1
[    0.000000] Hardware name: linux,dummy-virt (DT)
[    0.000000] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.000000] pc : arm64_rsi_is_protected_mmio+0xf0/0x110
[    0.000000] lr : arm64_rsi_is_protected_mmio+0x80/0x110
[    0.000000] sp : ffffcd7097053bf0
[    0.000000] x29: ffffcd7097053c30 x28: 0000000000000000 x27: 0000000000000000
[    0.000000] x26: 00000000000003d0 x25: 00000000ffffff8e x24: ffffcd7096831bd0
[    0.000000] x23: ffffcd7097053c08 x22: 00000000c4000198 x21: 0000000000001000
[    0.000000] x20: 0000000001001000 x19: 0000000001000000 x18: 0000000000000002
[    0.000000] x17: 0000000000000000 x16: 0000000000000010 x15: 0001000080000000
[    0.000000] x14: 0068000000000703 x13: ffffffffff5fe000 x12: ffffcd7097053ba4
[    0.000000] x11: 00000000000003d0 x10: ffffcd7097053bc4 x9 : 0000000000000444
[    0.000000] x8 : ffffffffff5fe000 x7 : 0000000000000000 x6 : 0000000000000000
[    0.000000] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
[    0.000000] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000
[    0.000000] Call trace:
[    0.000000]  arm64_rsi_is_protected_mmio+0xf0/0x110
[    0.000000]  set_fixmap_io+0x8c/0xd0
[    0.000000]  of_setup_earlycon+0xa0/0x294
[    0.000000]  early_init_dt_scan_chosen_stdout+0x104/0x1dc
[    0.000000]  acpi_boot_table_init+0x1a4/0x2d8
[    0.000000]  setup_arch+0x240/0x610
[    0.000000]  start_kernel+0x6c/0x708
[    0.000000]  __primary_switched+0x80/0x88

===> tf-rmm: git@github.com:TF-RMM/tf-rmm.git

handle_realm_rsi
   handle_rsi_ipa_state_get
     realm_ipa_get_ripas

===> tf-rmm/lib/s2tt/include/ripas.h

enum ripas {
         RIPAS_EMPTY = RMI_EMPTY,        /* Unused IPA for Realm */
         RIPAS_RAM = RMI_RAM,            /* IPA used for Code/Data by Realm */
         RIPAS_DESTROYED = RMI_DESTROYED /* IPA is inaccessible to the Realm */
};

===> tf-rmm/runtime/rsi/memory.c

void handle_rsi_ipa_state_get(struct rec *rec,
                               struct rsi_result *res)
{
         unsigned long ipa = rec->regs[1];
         enum s2_walk_status ws;
         enum ripas ripas_val = RIPAS_EMPTY;

         res->action = UPDATE_REC_RETURN_TO_REALM;

         if (!GRANULE_ALIGNED(ipa) || !addr_in_rec_par(rec, ipa)) {
                 res->smc_res.x[0] = RSI_ERROR_INPUT;
                 return;
         }

         ws = realm_ipa_get_ripas(rec, ipa, &ripas_val);
         if (ws == WALK_SUCCESS) {
                 res->smc_res.x[0] = RSI_SUCCESS;
                 res->smc_res.x[1] = (unsigned long)ripas_val;
         } else {
                 res->smc_res.x[0] = RSI_ERROR_INPUT;
         }
}

Thanks,
Gavin