diff mbox series

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

Message ID 20240819131924.372366-8-steven.price@arm.com (mailing list archive)
State New
Headers show
Series arm64: Support for running as a guest in Arm CCA | expand

Commit Message

Steven Price Aug. 19, 2024, 1:19 p.m. UTC
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(+)

Comments

Catalin Marinas Aug. 26, 2024, 10:04 a.m. UTC | #1
On Mon, Aug 19, 2024 at 02:19:12PM +0100, Steven Price wrote:
> +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;
> +}

I was wondering whether to return true in non-realm world. It doesn't
matter since the pgprot_decrypted() wouldn't do anything. Anyway:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Gavin Shan Sept. 6, 2024, 4:32 a.m. UTC | #2
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
Gavin Shan Sept. 6, 2024, 4:52 a.m. UTC | #3
On 9/6/24 2:32 PM, Gavin Shan wrote:
> 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);
>> +}
> 
> 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
> 

I have local changes like below to avoid the unexpected calltrace.

diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
index 9cb3353e5cbf..3d132d45fd83 100644
--- a/arch/arm64/kernel/rsi.c
+++ b/arch/arm64/kernel/rsi.c
@@ -100,14 +100,15 @@ bool arm64_rsi_is_protected_mmio(phys_addr_t base, size_t size)
         while (base < end) {
                 if (WARN_ON(rsi_ipa_state_get(base, end, &ripas, &top)))
                         break;
-               if (WARN_ON(top <= base))
+               if (WARN_ON(top && top <= base))
                         break;
                 if (ripas != RSI_RIPAS_IO)
                         break;
-               base = top;
+
+               base = top ? top : end;
         }
  
-       return (size && base >= end);
+       return base >= end;

Thanks,
Gavin
Steven Price Sept. 6, 2024, 1:55 p.m. UTC | #4
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@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.

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!)

From the cover letter:

> As mentioned above the new RMM specification means that corresponding
> changes need to be made in the RMM, at this time these changes are still
> in review (see 'topics/rmm-1.0-rel0-rc1'). So you'll need to fetch the
> changes[3] from the gerrit instance until they are pushed to the main
> branch.
>
> [3] https://review.trustedfirmware.org/c/TF-RMM/tf-rmm/+/30485

Sorry, I should probably have made this much more prominent in the cover
letter.

Running something like...

 git fetch https://git.trustedfirmware.org/TF-RMM/tf-rmm.git \
	refs/changes/85/30485/11

... should get you the latest. Hopefully these changes will get merged
to the main branch soon.

Steve

>> +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
>
Gavin Shan Sept. 8, 2024, 11:53 p.m. UTC | #5
On 9/6/24 11:55 PM, Steven Price wrote:
> On 06/09/2024 05:32, Gavin Shan wrote:
>> 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/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.
> 

The check 'size == 0' could be squeezed to the overflow check if you agree.

     /* size == 0 or overflow */
     if (WARN_ON(base + size) <= base)
         return false;
     :
     
     return (base >= end);


>>         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!)
> 
>>From the cover letter:
> 
>> As mentioned above the new RMM specification means that corresponding
>> changes need to be made in the RMM, at this time these changes are still
>> in review (see 'topics/rmm-1.0-rel0-rc1'). So you'll need to fetch the
>> changes[3] from the gerrit instance until they are pushed to the main
>> branch.
>>
>> [3] https://review.trustedfirmware.org/c/TF-RMM/tf-rmm/+/30485
> 
> Sorry, I should probably have made this much more prominent in the cover
> letter.
> 
> Running something like...
> 
>   git fetch https://git.trustedfirmware.org/TF-RMM/tf-rmm.git \
> 	refs/changes/85/30485/11
> 
> ... should get you the latest. Hopefully these changes will get merged
> to the main branch soon.
> 

My bad. I didn't check the cover letter in time. With this specific TF-RMM branch,
I'm able to boot the guest with cca/host-v4 and cca/guest-v5. However, there are
messages indicating unhandled system register accesses, as below.

# ./start.sh
   Info: # lkvm run -k Image -m 256 -c 2 --name guest-152
   Info: Removed ghost socket file "/root/.lkvm//guest-152.sock".
[   rmm ] SMC_RMI_REALM_CREATE          882860000 880856000 > RMI_SUCCESS
[   rmm ] SMC_RMI_REC_AUX_COUNT         882860000 > RMI_SUCCESS 10
[   rmm ] SMC_RMI_REC_CREATE            882860000 88bdc5000 88bdc4000 > RMI_SUCCESS
[   rmm ] SMC_RMI_REC_CREATE            882860000 88bdd7000 88bdc4000 > RMI_SUCCESS
[   rmm ] SMC_RMI_REALM_ACTIVATE        882860000 > RMI_SUCCESS
[   rmm ] Unhandled write S2_0_C0_C2_2
[   rmm ] Unhandled write S3_3_C9_C14_0
[   rmm ] SMC_RSI_VERSION               10000 > RSI_SUCCESS 10000 10000
[   rmm ] SMC_RSI_REALM_CONFIG          82b2b000 > RSI_SUCCESS
[   rmm ] SMC_RSI_IPA_STATE_SET         80000000 90000000 1 0 > RSI_SUCCESS 90000000 0
[   rmm ] SMC_RSI_IPA_STATE_GET         1000000 1001000 > RSI_SUCCESS 1001000 0
      :
[    1.835570] DMA: preallocated 128 KiB GFP_KERNEL|GFP_DMA32 pool for atomic allocations
[    1.865993] audit: initializing netlink subsys (disabled)
[    1.891218] audit: type=2000 audit(0.492:1): state=initialized audit_enabled=0 res=1
[    1.899066] thermal_sys: Registered thermal governor 'step_wise'
[    1.920869] thermal_sys: Registered thermal governor 'power_allocator'
[    1.944151] cpuidle: using governor menu
[    1.988588] hw-breakpoint: found 16 breakpoint and 16 watchpoint registers.
[   rmm ] Unhandled write S2_0_C0_C0_5
[   rmm ] Unhandled write S2_0_C0_C0_4
[   rmm ] Unhandled write S2_0_C0_C1_5
[   rmm ] Unhandled write S2_0_C0_C1_4
[   rmm ] Unhandled write S2_0_C0_C2_5
      :
[   rmm ] Unhandled write S2_0_C0_C13_6
[   rmm ] Unhandled write S2_0_C0_C14_7
[   rmm ] Unhandled write S2_0_C0_C14_6
[   rmm ] Unhandled write S2_0_C0_C15_7
[   rmm ] Unhandled write S2_0_C0_C15_6

Thanks,
Gavin
Steven Price Sept. 9, 2024, 9:31 a.m. UTC | #6
On 09/09/2024 00:53, Gavin Shan wrote:
> On 9/6/24 11:55 PM, Steven Price wrote:
>> On 06/09/2024 05:32, Gavin Shan wrote:
>>> 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/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.
>>
> 
> The check 'size == 0' could be squeezed to the overflow check if you agree.
> 
>     /* size == 0 or overflow */
>     if (WARN_ON(base + size) <= base)
>         return false;
>     :
>         return (base >= end);
> 

Yes that makes sense, thanks for the suggestion.

>>>         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!)
>>
>>> From the cover letter:
>>
>>> As mentioned above the new RMM specification means that corresponding
>>> changes need to be made in the RMM, at this time these changes are still
>>> in review (see 'topics/rmm-1.0-rel0-rc1'). So you'll need to fetch the
>>> changes[3] from the gerrit instance until they are pushed to the main
>>> branch.
>>>
>>> [3] https://review.trustedfirmware.org/c/TF-RMM/tf-rmm/+/30485
>>
>> Sorry, I should probably have made this much more prominent in the cover
>> letter.
>>
>> Running something like...
>>
>>   git fetch https://git.trustedfirmware.org/TF-RMM/tf-rmm.git \
>>     refs/changes/85/30485/11
>>
>> ... should get you the latest. Hopefully these changes will get merged
>> to the main branch soon.
>>
> 
> My bad. I didn't check the cover letter in time. With this specific
> TF-RMM branch,
> I'm able to boot the guest with cca/host-v4 and cca/guest-v5. However,
> there are
> messages indicating unhandled system register accesses, as below.

To some extent unhandled system register accesses are expected. The
kernel will probe for features, and if the RMM doesn't support them it
will be emulating those registers as RAZ/WI. I believe RAZ/WI is an
appropriate emulation, so Linux won't have any trouble here, and I don't
think there's anything wrong with Linux probing these registers.

So the question really is whether the RMM needs to have dummy handlers
to silence the 'warnings'. They are currently output using 'INFO' so
priority - so will only be visible in a 'debug' build (or if the log
level has been explicitly set).

Steve

> # ./start.sh
>   Info: # lkvm run -k Image -m 256 -c 2 --name guest-152
>   Info: Removed ghost socket file "/root/.lkvm//guest-152.sock".
> [   rmm ] SMC_RMI_REALM_CREATE          882860000 880856000 > RMI_SUCCESS
> [   rmm ] SMC_RMI_REC_AUX_COUNT         882860000 > RMI_SUCCESS 10
> [   rmm ] SMC_RMI_REC_CREATE            882860000 88bdc5000 88bdc4000 >
> RMI_SUCCESS
> [   rmm ] SMC_RMI_REC_CREATE            882860000 88bdd7000 88bdc4000 >
> RMI_SUCCESS
> [   rmm ] SMC_RMI_REALM_ACTIVATE        882860000 > RMI_SUCCESS
> [   rmm ] Unhandled write S2_0_C0_C2_2
> [   rmm ] Unhandled write S3_3_C9_C14_0
> [   rmm ] SMC_RSI_VERSION               10000 > RSI_SUCCESS 10000 10000
> [   rmm ] SMC_RSI_REALM_CONFIG          82b2b000 > RSI_SUCCESS
> [   rmm ] SMC_RSI_IPA_STATE_SET         80000000 90000000 1 0 >
> RSI_SUCCESS 90000000 0
> [   rmm ] SMC_RSI_IPA_STATE_GET         1000000 1001000 > RSI_SUCCESS
> 1001000 0
>      :
> [    1.835570] DMA: preallocated 128 KiB GFP_KERNEL|GFP_DMA32 pool for
> atomic allocations
> [    1.865993] audit: initializing netlink subsys (disabled)
> [    1.891218] audit: type=2000 audit(0.492:1): state=initialized
> audit_enabled=0 res=1
> [    1.899066] thermal_sys: Registered thermal governor 'step_wise'
> [    1.920869] thermal_sys: Registered thermal governor 'power_allocator'
> [    1.944151] cpuidle: using governor menu
> [    1.988588] hw-breakpoint: found 16 breakpoint and 16 watchpoint
> registers.
> [   rmm ] Unhandled write S2_0_C0_C0_5
> [   rmm ] Unhandled write S2_0_C0_C0_4
> [   rmm ] Unhandled write S2_0_C0_C1_5
> [   rmm ] Unhandled write S2_0_C0_C1_4
> [   rmm ] Unhandled write S2_0_C0_C2_5
>      :
> [   rmm ] Unhandled write S2_0_C0_C13_6
> [   rmm ] Unhandled write S2_0_C0_C14_7
> [   rmm ] Unhandled write S2_0_C0_C14_6
> [   rmm ] Unhandled write S2_0_C0_C15_7
> [   rmm ] Unhandled write S2_0_C0_C15_6
> 
> Thanks,
> Gavin
>
Gavin Shan Sept. 10, 2024, 3:51 a.m. UTC | #7
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;
> +}
> +

I guess it might be better to unify the function names here. The name of
arm64_is_iomem_private() indicates the IO region is private, while the
name of arm64_rsi_is_protected_mmio() indicates the IO region is protected.
I think it would be nice to rename arm64_is_iomem_private() arm64_is_protected_iomem(),
or rename arm64_rsi_is_protected_mmio() to arm64_rsi_is_private_iomem().

Thanks,
Gavin
diff mbox series

Patch

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);
+}
+EXPORT_SYMBOL(arm64_rsi_is_protected_mmio);
+
 void __init arm64_rsi_init(void)
 {
 	/*