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

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

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@arm.com> wrote:
>>>
>>> On 14/04/2025 10:41, Ard Biesheuvel wrote:
>>>> On Mon, 14 Apr 2025 at 09:52, Ryan Roberts <ryan.roberts@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@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-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
 
-       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