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

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 4/24/25 17:35, Ryan Roberts wrote:
> On 24/04/2025 12:00, Anshuman Khandual wrote:
>>
>>
>> On 4/24/25 15:37, Ryan Roberts wrote:
>>> On 16/04/2025 04:56, 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,
>>>> that are required for D128 page tables. 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 accommodate.
>>>>
>>>> Instead pre-calculate these PIRE0_EL1/PIR_EL1 constants into asm-offsets.h
>>>> based PIE_E0_ASM/PIE_E1_ASM which can then be used in arch/arm64/mm/proc.S.
>>>>
>>>> While here also move PTE_MAYBE_NG/PTE_MAYBE_SHARED assembly overrides into
>>>> arch/arm64/kernel/asm-offsets.c to ensure PIRE0_EL1/PIR_EL1 are calculated
>>>> in assembly without arm64_use_ng_mappings and lpa2_is_enabled() symbols
>>>> being accessible. Also move the corresponding comment as well.
>>>>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>> This patch applies on v6.15-rc2
>>>>
>>>> Changes in V2:
>>>>
>>>> - Added asm-offsets.c based PIE_E0_ASM and PIE_E1_ASM symbols as per Ard
>>>> - Moved PTE_MAYBE_NG and PTE_MAYBE_SHARED overrides inside asm-offsets.c
>>>>   along with the corresponding comment as per Ard
>>>>
>>>> Changes in V1:
>>>>
>>>> https://lore.kernel.org/linux-arm-kernel/20250410074024.1545768-1-anshuman.khandual@arm.com/
>>>>
>>>>  arch/arm64/kernel/asm-offsets.c | 16 ++++++++++++++++
>>>>  arch/arm64/mm/proc.S            | 19 ++-----------------
>>>>  2 files changed, 18 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>>>> index eb1a840e4110..5b99a78f6882 100644
>>>> --- a/arch/arm64/kernel/asm-offsets.c
>>>> +++ b/arch/arm64/kernel/asm-offsets.c
>>>> @@ -182,5 +182,21 @@ int main(void)
>>>>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>>>>    DEFINE(FTRACE_OPS_DIRECT_CALL,	offsetof(struct ftrace_ops, direct_call));
>>>>  #endif
>>>> +	/*
>>>> +	 * The PROT_* macros describing the various memory types may resolve to
>>>> +	 * C expressions if they include the PTE_MAYBE_* macros, and so they
>>>> +	 * can only be used from C code. The PIE_E* constants below are also
>>>> +	 * defined in terms of those macros, but will mask out those
>>>> +	 * PTE_MAYBE_* constants, whether they are set or not. So #define them
>>>> +	 * as 0x0 here so we can evaluate the PIE_E* constants in asm context.
>>>> +	 */
>>>> +#undef PTE_MAYBE_NG
>>>> +#define PTE_MAYBE_NG		0
>>>> +
>>>> +#undef PTE_MAYBE_SHARED
>>>> +#define PTE_MAYBE_SHARED	0
>>>
>>> My toolchain at least is smart enough to figure out that the bits of interest in
>>> PIE_E1 are all contstant so it works without this hack.
>>>
>>> I'd prefer to drop this hack on that basis. Or if there are issues with other
>>> toolchains that mean we need to keep it, I think the wording of the comment
>>
>> So could we keep these for some more time and later drop them off just to be
>> on the safer side ?
> 
> What exactly is your concern? That some toolchains might not be so smart? But
> that same risk exists when we eventually drop the hack, so why not just do it now?
> 
> TBH, I imagine that it's spec'ed to work like this by the C standard, in which
> case you would definitely be safe.
> 
> I checked with and without the hack and it gives the same answer both times:
> 
> 	#define PIE_E0_ASM 5769270003974144000 /* PIE_E0 */
> 	#define PIE_E1_ASM -3708698853797527552 /* PIE_E1 */

Alright, will drop these PTE_MAYBE_* defines.

> 
>>
>>> should be changed since we are now in C code so the "can only be used from C
>>> code" bit doesn't really make sense.
>>
>> Dropped that. Does this look better ?
>>
>> 	/*
>> 	 * The PROT_* macros describing the various memory types may resolve
>> 	 * to C expressions if they include the PTE_MAYBE_* macros. The PIE_E*
>> 	 * constants below are also defined in terms of those macros, but will
>> 	 * mask out those PTE_MAYBE_* constants, whether they are set or not.
>> 	 * So #define them as 0x0 here so we can evaluate the PIE_E* constants
>> 	 * in asm context.
>>  	 */
> 
> Yeah that works if leaving it in.
> 
>>
>>>
>>> Thanks,
>>> Ryan
>>>
>>>> +
>>>> +  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 fb30c8804f87..80d470aa469d 100644
>>>> --- a/arch/arm64/mm/proc.S
>>>> +++ b/arch/arm64/mm/proc.S
>>>> @@ -512,26 +512,11 @@ alternative_else_nop_endif
>>>>  	ubfx	x1, x1, #ID_AA64MMFR3_EL1_S1PIE_SHIFT, #4
>>>>  	cbz	x1, .Lskip_indirection
>>>>  
>>>> -	/*
>>>> -	 * The PROT_* macros describing the various memory types may resolve to
>>>> -	 * C expressions if they include the PTE_MAYBE_* macros, and so they
>>>> -	 * can only be used from C code. The PIE_E* constants below are also
>>>> -	 * defined in terms of those macros, but will mask out those
>>>> -	 * PTE_MAYBE_* constants, whether they are set or not. So #define them
>>>> -	 * as 0x0 here so we can evaluate the PIE_E* constants in asm context.
>>>> -	 */
>>>> -
>>>> -#define PTE_MAYBE_NG		0
>>>> -#define PTE_MAYBE_SHARED	0
>>>> -
>>>> -	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
>>>> -#undef PTE_MAYBE_SHARED
>>>> -
>>>>  	orr	tcr2, tcr2, TCR2_EL1_PIE
>>>>  	msr	REG_TCR2_EL1, x0
>>>>  
>>>
>