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

[Ryan Roberts]

On 14/04/2025 10:41, Ard Biesheuvel wrote:
> On Mon, 14 Apr 2025 at 09:52, Ryan Roberts <ryan.roberts at arm.com> 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 at arm.com>
>>> Cc: Will Deacon <will at kernel.org>
>>> Cc: Mark Rutland <mark.rutland at arm.com>
>>> Cc: Ard Biesheuvel <ardb at kernel.org>
>>> Cc: Ryan Roberts <ryan.roberts at arm.com>
>>> Cc: linux-arm-kernel at lists.infradead.org
>>> Cc: linux-kernel at vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual at arm.com>
>>> ---
>>> 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?

Thanks,
Ryan

> 
> I wouldn't mind just dropping that, i.e.,
> 
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -545,17 +545,9 @@ alternative_endif
>          *         magnitude and sign of the operand)
>          */
>         .macro  mov_q, reg, val
> -       .if (((\val) >> 31) == 0 || ((\val) >> 31) == 0x1ffffffff)
> -       movz    \reg, :abs_g1_s:\val
> -       .else
> -       .if (((\val) >> 47) == 0 || ((\val) >> 47) == 0x1ffff)
> -       movz    \reg, :abs_g2_s:\val
> -       .else
>         movz    \reg, :abs_g3:\val
>         movk    \reg, :abs_g2_nc:\val
> -       .endif
>         movk    \reg, :abs_g1_nc:\val
> -       .endif
>         movk    \reg, :abs_g0_nc:\val
>         .endm
> 
> Then, we can apply Ryan's trick to move these constants into
> asm-offsets.c, but you'll need to tweak the PTE_MAYBE macros as well:
> 
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -182,5 +182,22 @@ int main(void)
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>    DEFINE(FTRACE_OPS_DIRECT_CALL,       offsetof(struct ftrace_ops,
> direct_call));
>  #endif
> +#undef PTE_MAYBE_NG
> +#define PTE_MAYBE_NG           0
> +
> +#undef PTE_MAYBE_SHARED
> +#define PTE_MAYBE_SHARED       0
> +
> +  DEFINE(PIE_E0_ASM, PIE_E0);
> +  DEFINE(PIE_E1_ASM, PIE_E1);
>    return 0;
>  }
> 
> (We should also move the comment from proc.S to asm-offsets.c but I
> omitted that here for brevity.)
> 
> Then you should be able to use PIE_En_ASM in mov_q instructions without issue.
> 
> Alternatively, if changing the implementation of mov_q creates any
> problems, we can just add a variant of that macro that lacks the
> conditional directives and always emits MOVZ/MOVK/MOVK/MOVK