Re: [PATCH v2 03/14] arm64: realm: Query IPA size from the RMM

[Steven Price]

On 13/05/2024 15:03, Catalin Marinas wrote:
> On Fri, Apr 12, 2024 at 09:42:02AM +0100, Steven Price wrote:
>> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
>> index dd9ee67d1d87..15d8f0133af8 100644
>> --- a/arch/arm64/include/asm/pgtable-prot.h
>> +++ b/arch/arm64/include/asm/pgtable-prot.h
>> @@ -63,6 +63,9 @@
>>  #include <asm/pgtable-types.h>
>>  
>>  extern bool arm64_use_ng_mappings;
>> +extern unsigned long prot_ns_shared;
>> +
>> +#define PROT_NS_SHARED		((prot_ns_shared))
> 
> Nit: what's with the double parenthesis here?

No idea - I'll remove a set!

>>  #define PTE_MAYBE_NG		(arm64_use_ng_mappings ? PTE_NG : 0)
>>  #define PMD_MAYBE_NG		(arm64_use_ng_mappings ? PMD_SECT_NG : 0)
>> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
>> index 1076649ac082..b93252ed6fc5 100644
>> --- a/arch/arm64/kernel/rsi.c
>> +++ b/arch/arm64/kernel/rsi.c
>> @@ -7,6 +7,11 @@
>>  #include <linux/memblock.h>
>>  #include <asm/rsi.h>
>>  
>> +struct realm_config __attribute((aligned(PAGE_SIZE))) config;
> 
> Another nit: use __aligned(PAGE_SIZE).
> 
> However, does the spec require this to be page-size aligned? The spec
> says aligned to 0x1000 and that's not necessarily the kernel page size.
> It also states that the RsiRealmConfig structure is 4096 and I did not
> see this in the first patch, you only have 8 bytes in this structure.
> Some future spec may write more data here overriding your other
> variables in the data section.
> 
> So that's the wrong place to force the alignment. Just do this when you
> define the actual structure in the first patch:
> 
> struct realm_config {
> 	union {
> 		unsigned long ipa_bits; /* Width of IPA in bits */
> 		u8 __pad[4096];
> 	};
> } __aligned(4096);
> 
> and maybe with a comment on why the alignment and padding. You could
> also have an unnamed struct around ipa_bits in case you want to add more
> fields in the future (siginfo follows this pattern).

Good suggestion - I'll update this. It turns out struct realm_config is
already missing "hash_algo" (not that we use it yet), so I'll add that too.

Thanks,

Steve

>> +
>> +unsigned long prot_ns_shared;
>> +EXPORT_SYMBOL(prot_ns_shared);
>> +
>>  DEFINE_STATIC_KEY_FALSE_RO(rsi_present);
>>  EXPORT_SYMBOL(rsi_present);
>>  
>> @@ -53,6 +58,9 @@ void __init arm64_rsi_init(void)
>>  {
>>  	if (!rsi_version_matches())
>>  		return;
>> +	if (rsi_get_realm_config(&config))
>> +		return;
>> +	prot_ns_shared = BIT(config.ipa_bits - 1);
>>  
>>  	static_branch_enable(&rsi_present);
>>  }
>