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

[Mark Rutland <mark.rutland@arm.com>]

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.