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

[Weikang Guo <guoweikang.kernel@gmail.com>]

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