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);
>> }
>
