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

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

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