Re: [PATCH] arm64/mm: Re-organise setting up FEAT_S1PIE registers PIRE0_EL1 and PIR_EL1

[Ryan Roberts]

On 15/04/2025 10:46, Anshuman Khandual wrote:
> 
> 
> On 4/15/25 13:23, Ryan Roberts wrote:
>> On 15/04/2025 07:27, Anshuman Khandual wrote:
>>>
>>>
>>> On 4/14/25 18:01, Ryan Roberts wrote:
>>>> On 14/04/2025 13:28, Ard Biesheuvel wrote:
>>>>> On Mon, 14 Apr 2025 at 14:04, Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
>>>>>>
>>>>>> On 14/04/2025 10:41, Ard Biesheuvel wrote:
>>>>>>> On Mon, 14 Apr 2025 at 09:52, Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
>>>>>>>>
>>>>>>>> On 10/04/2025 08:40, Anshuman Khandual wrote:
>>>>>>>>> mov_q cannot really move PIE_E[0|1] macros into a general purpose register
>>>>>>>>> as expected if those macro constants contain some 128 bit layout elements,
>>>>>>>>> required for D128 page tables. Fix this problem via first loading up these
>>>>>>>>> macro constants into a given memory location and then subsequently setting
>>>>>>>>> up registers PIRE0_EL1 and PIR_EL1 by retrieving the memory stored values.
>>>>>>>>
>>>>>>>> From memory, the primary issue is that for D128, PIE_E[0|1] are defined in terms
>>>>>>>> of 128-bit types with shifting and masking, which the assembler can't do? It
>>>>>>>> would be good to spell this out.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>>>>>>>>> Cc: Will Deacon <will@xxxxxxxxxx>
>>>>>>>>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>>>>>>>>> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
>>>>>>>>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
>>>>>>>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>>>>>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>>>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>>>>>>>> ---
>>>>>>>>> This patch applies on v6.15-rc1
>>>>>>>>>
>>>>>>>>>  arch/arm64/kernel/head.S         | 3 +++
>>>>>>>>>  arch/arm64/kernel/pi/map_range.c | 6 ++++++
>>>>>>>>>  arch/arm64/kernel/pi/pi.h        | 1 +
>>>>>>>>>  arch/arm64/mm/mmu.c              | 1 +
>>>>>>>>>  arch/arm64/mm/proc.S             | 5 +++--
>>>>>>>>>  5 files changed, 14 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>>>>>>>>> index 2ce73525de2c..4950d9cc638a 100644
>>>>>>>>> --- a/arch/arm64/kernel/head.S
>>>>>>>>> +++ b/arch/arm64/kernel/head.S
>>>>>>>>> @@ -126,6 +126,9 @@ SYM_CODE_START(primary_entry)
>>>>>>>>>        * On return, the CPU will be ready for the MMU to be turned on and
>>>>>>>>>        * the TCR will have been set.
>>>>>>>>>        */
>>>>>>>>> +     adr_l   x0, pir_data
>>>>>>>>> +     bl      __pi_load_pir_data
>>>>>>>>
>>>>>>>> Using C code to pre-calculate the values into global variables that the assembly
>>>>>>>> code then loads and stuffs into the PIR registers feels hacky. I wonder if we
>>>>>>>> can instead pre-calculate into asm-offsets.h? e.g. add the following to
>>>>>>>> asm-offsets.c:
>>>>>>>>
>>>>>>>> DEFINE(PIE_E0_ASM, PIE_E0);
>>>>>>>> DEFINE(PIE_E1_ASM, PIE_E1);
>>>>>>>>
>>>>>>>> Which will generate the asm-offsets.h header with PIE_E[0|1]_ASM with the
>>>>>>>> pre-calculated values that you can then use in proc.S?
>>>>>>>>
>>>>>>>
>>>>>>> There is another issue, which is that mov_q tries to be smart, and
>>>>>>> emit fewer than 4 MOVZ/MOVK instructions if possible. So the .if
>>>>>>> directive evaluates the argument, which does not work with symbolic
>>>>>>> constants.
>>>>>>
>>>>>> I'm not quite understanding the detail here; what do you mean by "symbolic
>>>>>> constants"? asm-offsets.h will provide something like:
>>>>>>
>>>>>> #define PIE_E0_ASM 1234567890
>>>>>>
>>>>>> The current code is using a hash-define and that's working fine:
>>>>>>
>>>>>> mov_q   x0, PIE_E0
>>>>>>
>>>>>>
>>>>>> Won't the C preprocessor just substitute and everything will work out?
>>>>>>
>>>>>
>>>>> Yeah, you're right. I was experimenting with something like
>>>>>
>>>>> .set .Lpie_e0, PIE_E0_ASM
>>>>> mov_q xN, .Lpie_e0
>>>>>
>>>>> where this problem does exist, but we can just use PIE_E0_ASM directly
>>>>> and things should work as expected.
>>>>
>>>> Ahh great, sounds like this should be pretty simple then!
>>>
>>> Following change works both on current and with D128 page tables.
>>>
>>> --- a/arch/arm64/kernel/asm-offsets.c
>>> +++ b/arch/arm64/kernel/asm-offsets.c
>>> @@ -182,5 +182,7 @@ int main(void)
>>>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>>>    DEFINE(FTRACE_OPS_DIRECT_CALL,       offsetof(struct ftrace_ops, direct_call));
>>>  #endif
>>> +  DEFINE(PIE_E0_ASM, PIE_E0);
>>> +  DEFINE(PIE_E1_ASM, PIE_E1);
>>>    return 0;
>>>  }
>>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>>> index 737c99d79833..f45494425d09 100644
>>> --- a/arch/arm64/mm/proc.S
>>> +++ b/arch/arm64/mm/proc.S
>>> @@ -536,9 +536,9 @@ alternative_else_nop_endif
>>>  #define PTE_MAYBE_NG           0
>>>  #define PTE_MAYBE_SHARED       0
>>
>> I think at minimum, you can remove this PTE_MAYBE_* hack from proc.S. But as Ard
>> says, you may need to add it to asm-offsets.c? I'm surprised asm-offsets.c even
> 
> Moving PTE_MAYBE_* inside asm-offsets.c works as well in both cases
> but still wondering why this is even required ? What am I missing ?

Without the overrides:

#define PTE_MAYBE_NG		(arm64_use_ng_mappings ? PTE_NG : 0)
#define PTE_MAYBE_SHARED	(lpa2_is_enabled() ? 0 : PTE_SHARED)

And these are used in the definition of PROT_DEFAULT, which is used in the
definition of _PAGE_KERNEL*, which are used in the definition of PIE_E1.

For the assembly code arm64_use_ng_mappings and lpa2_is_enabled() are not
accessible. But we don't actually care about those bits so its just hacked to
override the fields to 0.

I would have expected the asm-offsets.c case to have a similer problem because
we have not defined arm64_use_ng_mappings and lpa2_is_enabled(). asm-offsets.c
is compiled as a standalone object then the macros are exfiltrated and
asm-offsets.h is created.

Perhaps the compiler is smart enough to see that we don't care about the bits in
those fields (pte_pi_index() is only keeping selected bits) and asm-offsets.c
can be safely compiled without the hack?

> 
>> compiles without this hack since surely it doesn't have arm64_use_ng_mappings or
>> is_realm_world() available?
> Did not face any problem with defconfig for the mainline kernel and
> both these symbols were visible in the built code.
> 
>>
>> Thanks,
>> Ryan
>>
>>>  
>>> -       mov_q   x0, PIE_E0
>>> +       mov_q   x0, PIE_E0_ASM
>>>         msr     REG_PIRE0_EL1, x0
>>> -       mov_q   x0, PIE_E1
>>> +       mov_q   x0, PIE_E1_ASM
>>>         msr     REG_PIR_EL1, x0
>>>  
>>>  #undef PTE_MAYBE_NG
>>>
>>