[PATCH] arm64: mm: Ensure phys_to_ttbr on pgdir for idmap_cpu_replace_ttbr1

[PATCH] arm64: mm: Ensure phys_to_ttbr on pgdir for idmap_cpu_replace_ttbr1

[Weikang Guo]

Commit 5ffdfaedfa0a ("arm64: mm: Support Common Not Private translations")
changed the contract of idmap_cpu_replace_ttbr1, requiring that the TTBR
argument passed in should already be processed by phys_to_ttbr (i.e., in
TTBR format, not just a raw physical address).

However, the current map_kernel implementation does not always convert the
pgdir/ttbr argument via phys_to_ttbr before calling
idmap_cpu_replace_ttbr1. This can lead to issues on systems with
CONFIG_ARM64_PA_BITS_52 enabled, as the TTBR would not be properly folded
per the ARMv8.2+ requirements.

Signed-off-by: Weikang Guo <guoweikang.kernel@gmail.com>

---
Note: I do not currently have access to ARM64 hardware or an emulation
environment that supports 52-bit physical address (PA52). I would
greatly appreciate if anyone with such a platform could help test
this patch. Thank you!
---
 arch/arm64/kernel/pi/map_kernel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pi/map_kernel.c b/arch/arm64/kernel/pi/map_kernel.c
index 0f4bd7771859..05a04eb91e81 100644
--- a/arch/arm64/kernel/pi/map_kernel.c
+++ b/arch/arm64/kernel/pi/map_kernel.c
@@ -18,7 +18,7 @@
 
 extern const u8 __eh_frame_start[], __eh_frame_end[];
 
-extern void idmap_cpu_replace_ttbr1(void *pgdir);
+extern void idmap_cpu_replace_ttbr1(phys_addr_t);
 
 static void __init map_segment(pgd_t *pg_dir, u64 *pgd, u64 va_offset,
 			       void *start, void *end, pgprot_t prot,
@@ -90,7 +90,7 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
 		    true, root_level);
 	dsb(ishst);
 
-	idmap_cpu_replace_ttbr1(init_pg_dir);
+	idmap_cpu_replace_ttbr1(phys_to_ttbr((u64)init_pg_dir));
 
 	if (twopass) {
 		if (IS_ENABLED(CONFIG_RELOCATABLE))
@@ -129,7 +129,7 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
 	/* Copy the root page table to its final location */
 	memcpy((void *)swapper_pg_dir + va_offset, init_pg_dir, PAGE_SIZE);
 	dsb(ishst);
-	idmap_cpu_replace_ttbr1(swapper_pg_dir);
+	idmap_cpu_replace_ttbr1(phys_to_ttbr((u64)swapper_pg_dir));
 }
 
 static void noinline __section(".idmap.text") set_ttbr0_for_lpa2(u64 ttbr)
-- 
2.25.1

Re: [PATCH] arm64: mm: Ensure phys_to_ttbr on pgdir for idmap_cpu_replace_ttbr1

[Mark Rutland]

On Tue, Jul 22, 2025 at 04:21:13PM +0800, Weikang Guo wrote:
> Commit 5ffdfaedfa0a ("arm64: mm: Support Common Not Private translations")
> changed the contract of idmap_cpu_replace_ttbr1, requiring that the TTBR
> argument passed in should already be processed by phys_to_ttbr (i.e., in
> TTBR format, not just a raw physical address).
> 
> However, the current map_kernel implementation does not always convert the
> pgdir/ttbr argument via phys_to_ttbr before calling
> idmap_cpu_replace_ttbr1. This can lead to issues on systems with
> CONFIG_ARM64_PA_BITS_52 enabled, as the TTBR would not be properly folded
> per the ARMv8.2+ requirements.

For the cases below I don't believe that this is actually a problem.
Since commit:

  453dfcee70c5c344 ("arm64: booting: Require placement within 48-bit addressable memory")

... we require that the kernel Image (including any trailing unallocated
bytes accounted for in image_size) are below the 48-bit address limit,
and so there should be no difference between the PA and TTBR format.

We could probably test and enforce that in the early boot code somehow,
if we're not doing that already.

If we were going to change things to avoid accidents in future, I think
it would be better to enforce this with the type system. e.g. we could
have a ttbr_val type that's distinct from phys_addr_t. Even then, for
the idmap code I think it's better to avoid the phys_to_ttbr() dance,
since that has runtime patching.

Mark.

> 
> Signed-off-by: Weikang Guo <guoweikang.kernel@gmail.com>
> 
> ---
> Note: I do not currently have access to ARM64 hardware or an emulation
> environment that supports 52-bit physical address (PA52). I would
> greatly appreciate if anyone with such a platform could help test
> this patch. Thank you!
> ---
>  arch/arm64/kernel/pi/map_kernel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pi/map_kernel.c b/arch/arm64/kernel/pi/map_kernel.c
> index 0f4bd7771859..05a04eb91e81 100644
> --- a/arch/arm64/kernel/pi/map_kernel.c
> +++ b/arch/arm64/kernel/pi/map_kernel.c
> @@ -18,7 +18,7 @@
>  
>  extern const u8 __eh_frame_start[], __eh_frame_end[];
>  
> -extern void idmap_cpu_replace_ttbr1(void *pgdir);
> +extern void idmap_cpu_replace_ttbr1(phys_addr_t);
>  
>  static void __init map_segment(pgd_t *pg_dir, u64 *pgd, u64 va_offset,
>  			       void *start, void *end, pgprot_t prot,
> @@ -90,7 +90,7 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
>  		    true, root_level);
>  	dsb(ishst);
>  
> -	idmap_cpu_replace_ttbr1(init_pg_dir);
> +	idmap_cpu_replace_ttbr1(phys_to_ttbr((u64)init_pg_dir));
>  
>  	if (twopass) {
>  		if (IS_ENABLED(CONFIG_RELOCATABLE))
> @@ -129,7 +129,7 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
>  	/* Copy the root page table to its final location */
>  	memcpy((void *)swapper_pg_dir + va_offset, init_pg_dir, PAGE_SIZE);
>  	dsb(ishst);
> -	idmap_cpu_replace_ttbr1(swapper_pg_dir);
> +	idmap_cpu_replace_ttbr1(phys_to_ttbr((u64)swapper_pg_dir));
>  }
>  
>  static void noinline __section(".idmap.text") set_ttbr0_for_lpa2(u64 ttbr)
> -- 
> 2.25.1
> 
> 

Re: [PATCH] arm64: mm: Ensure phys_to_ttbr on pgdir for idmap_cpu_replace_ttbr1

[Weikang Guo]

On Tue, Jul 22, 2025 at 03:56:20PM +0100, Mark Rutland wrote:
> On Tue, Jul 22, 2025 at 04:21:13PM +0800, Weikang Guo wrote:
> > Commit 5ffdfaedfa0a ("arm64: mm: Support Common Not Private translations")
> > changed the contract of idmap_cpu_replace_ttbr1, requiring that the TTBR
> > argument passed in should already be processed by phys_to_ttbr (i.e., in
> > TTBR format, not just a raw physical address).
> > 
> > However, the current map_kernel implementation does not always convert the
> > pgdir/ttbr argument via phys_to_ttbr before calling
> > idmap_cpu_replace_ttbr1. This can lead to issues on systems with
> > CONFIG_ARM64_PA_BITS_52 enabled, as the TTBR would not be properly folded
> > per the ARMv8.2+ requirements.
> 
> For the cases below I don't believe that this is actually a problem.
> Since commit:
> 
>   453dfcee70c5c344 ("arm64: booting: Require placement within 48-bit addressable memory")
> 
> ... we require that the kernel Image (including any trailing unallocated
> bytes accounted for in image_size) are below the 48-bit address limit,
> and so there should be no difference between the PA and TTBR format.
> 
> We could probably test and enforce that in the early boot code somehow,
> if we're not doing that already.
> 
> If we were going to change things to avoid accidents in future, I think
> it would be better to enforce this with the type system. e.g. we could
> have a ttbr_val type that's distinct from phys_addr_t. Even then, for
> the idmap code I think it's better to avoid the phys_to_ttbr() dance,
> since that has runtime patching.
> 
> Mark.
>

Thank you for your detailed explanation.

As you mentioned, if we can guarantee that the kernel image is always within
the 48-bit PA range,then there is indeed no real difference between the PA
and TTBR formats in this context.
In that case, does it mean that the conversion of `reserved_pg_dir`here is
also redundant? (There may be other similar cases.)

If we already ensure the kernel is always mapped below 48 bits, it does
seem safe to remove `phys_to_ttbr`here as well.

.macro  __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
    adrp    \tmp1, reserved_pg_dir
    phys_to_ttbr \tmp2, \tmp1    // This might not be needed either?
    offset_ttbr1 \tmp2, \tmp1
    msr ttbr1_el1, \tmp2
    isb
    tlbi    vmalle1
    dsb nsh
    isb
.endm

Thanks again for the clarification!

WeiKang

Re: [PATCH] arm64: mm: Ensure phys_to_ttbr on pgdir for idmap_cpu_replace_ttbr1

[Mark Rutland]

On Wed, Jul 23, 2025 at 10:50:55AM +0800, Weikang Guo wrote:
> On Tue, Jul 22, 2025 at 03:56:20PM +0100, Mark Rutland wrote:
> > On Tue, Jul 22, 2025 at 04:21:13PM +0800, Weikang Guo wrote:
> > > Commit 5ffdfaedfa0a ("arm64: mm: Support Common Not Private translations")
> > > changed the contract of idmap_cpu_replace_ttbr1, requiring that the TTBR
> > > argument passed in should already be processed by phys_to_ttbr (i.e., in
> > > TTBR format, not just a raw physical address).
> > > 
> > > However, the current map_kernel implementation does not always convert the
> > > pgdir/ttbr argument via phys_to_ttbr before calling
> > > idmap_cpu_replace_ttbr1. This can lead to issues on systems with
> > > CONFIG_ARM64_PA_BITS_52 enabled, as the TTBR would not be properly folded
> > > per the ARMv8.2+ requirements.
> > 
> > For the cases below I don't believe that this is actually a problem.
> > Since commit:
> > 
> >   453dfcee70c5c344 ("arm64: booting: Require placement within 48-bit addressable memory")
> > 
> > ... we require that the kernel Image (including any trailing unallocated
> > bytes accounted for in image_size) are below the 48-bit address limit,
> > and so there should be no difference between the PA and TTBR format.
> > 
> > We could probably test and enforce that in the early boot code somehow,
> > if we're not doing that already.
> > 
> > If we were going to change things to avoid accidents in future, I think
> > it would be better to enforce this with the type system. e.g. we could
> > have a ttbr_val type that's distinct from phys_addr_t. Even then, for
> > the idmap code I think it's better to avoid the phys_to_ttbr() dance,
> > since that has runtime patching.
> > 
> > Mark.
> >
> 
> Thank you for your detailed explanation.
> 
> As you mentioned, if we can guarantee that the kernel image is always within
> the 48-bit PA range,then there is indeed no real difference between the PA
> and TTBR formats in this context.

Yep.

To be clear, I'm saying that there's no functional problem in practice,
and hence the description in the commit message is more alarming than
necessary.

Since the conversion is trivial I'm not against applying the conversion
consistently, but if we do that I think we should enforce that through
the type system so that missing conversions will be identified by the
compiler.

> In that case, does it mean that the conversion of `reserved_pg_dir`here is
> also redundant? (There may be other similar cases.)

Yes, 'reserved_pg_dir' should be part of the kernel Image and hence
below the 48-bit limit.

> If we already ensure the kernel is always mapped below 48 bits, it does
> seem safe to remove `phys_to_ttbr`here as well.

I assume for both instances of 'here' above you're referring to the
macro below.

> .macro  __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
>     adrp    \tmp1, reserved_pg_dir
>     phys_to_ttbr \tmp2, \tmp1    // This might not be needed either?
>     offset_ttbr1 \tmp2, \tmp1
>     msr ttbr1_el1, \tmp2
>     isb
>     tlbi    vmalle1
>     dsb nsh
>     isb
> .endm

Yes, the phys_to_ttbr conversion above isn't strictly necessary today.

Mark.

Re: [PATCH] arm64: mm: Ensure phys_to_ttbr on pgdir for idmap_cpu_replace_ttbr1

[Weikang Guo]

On Wed, Jul 23, 2025 at 09:48:53AM +0100, Mark Rutland wrote:
> On Wed, Jul 23, 2025 at 10:50:55AM +0800, Weikang Guo wrote:
> > On Tue, Jul 22, 2025 at 03:56:20PM +0100, Mark Rutland wrote:
> > > On Tue, Jul 22, 2025 at 04:21:13PM +0800, Weikang Guo wrote:
> > > > Commit 5ffdfaedfa0a ("arm64: mm: Support Common Not Private translations")
> > > > changed the contract of idmap_cpu_replace_ttbr1, requiring that the TTBR
> > > > argument passed in should already be processed by phys_to_ttbr (i.e., in
> > > > TTBR format, not just a raw physical address).
> > > > 
> > > > However, the current map_kernel implementation does not always convert the
> > > > pgdir/ttbr argument via phys_to_ttbr before calling
> > > > idmap_cpu_replace_ttbr1. This can lead to issues on systems with
> > > > CONFIG_ARM64_PA_BITS_52 enabled, as the TTBR would not be properly folded
> > > > per the ARMv8.2+ requirements.
> > > 
> > > For the cases below I don't believe that this is actually a problem.
> > > Since commit:
> > > 
> > >   453dfcee70c5c344 ("arm64: booting: Require placement within 48-bit addressable memory")
> > > 
> > > ... we require that the kernel Image (including any trailing unallocated
> > > bytes accounted for in image_size) are below the 48-bit address limit,
> > > and so there should be no difference between the PA and TTBR format.
> > > 
> > > We could probably test and enforce that in the early boot code somehow,
> > > if we're not doing that already.
> > > 
> > > If we were going to change things to avoid accidents in future, I think
> > > it would be better to enforce this with the type system. e.g. we could
> > > have a ttbr_val type that's distinct from phys_addr_t. Even then, for
> > > the idmap code I think it's better to avoid the phys_to_ttbr() dance,
> > > since that has runtime patching.
> > > 
> > > Mark.
> > >
> > 
> > Thank you for your detailed explanation.
> > 
> > As you mentioned, if we can guarantee that the kernel image is always within
> > the 48-bit PA range,then there is indeed no real difference between the PA
> > and TTBR formats in this context.
> 
> Yep.
> 
> To be clear, I'm saying that there's no functional problem in practice,
> and hence the description in the commit message is more alarming than
> necessary.

Okay, I understand what you mean! and this is clear if the image is always
guaranteed to be within the 48-bit address range, phys_to_ttbr will not change
anything.

> 
> Since the conversion is trivial I'm not against applying the conversion
> consistently, but if we do that I think we should enforce that through
> the type system so that missing conversions will be identified by the
> compiler.

For a function call it should work.

> 
> > In that case, does it mean that the conversion of `reserved_pg_dir`here is
> > also redundant? (There may be other similar cases.)
> 
> Yes, 'reserved_pg_dir' should be part of the kernel Image and hence
> below the 48-bit limit.
> 
> > If we already ensure the kernel is always mapped below 48 bits, it does
> > seem safe to remove `phys_to_ttbr`here as well.
> 
> I assume for both instances of 'here' above you're referring to the
> macro below.

Yes, I meant the `__idmap_cpu_set_reserved_ttbr1` macro.

> 
> > .macro  __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
> >     adrp    \tmp1, reserved_pg_dir
> >     phys_to_ttbr \tmp2, \tmp1    // This might not be needed either?
> >     offset_ttbr1 \tmp2, \tmp1
> >     msr ttbr1_el1, \tmp2
> >     isb
> >     tlbi    vmalle1
> >     dsb nsh
> >     isb
> > .endm
> 
> Yes, the phys_to_ttbr conversion above isn't strictly necessary today.

Thanks for confirming this!
> 
> Mark.

Mark, finally, would you prefer that we introduce a new type to ensure we
always get a properly converted TTBR, or keep things as they are, or perhaps
explicitly state that we do not support kernel images above the 48-bit range
and remove those unnecessary conversions?

Weikang