[PATCH 1/5] arm64: mm: use a 48-bit ID map when possible on 52-bit VA builds

[Ard Biesheuvel]

On Thu, 11 Mar 2021 at 10:46, Will Deacon <will at kernel.org> wrote:
>
> On Wed, Mar 10, 2021 at 06:15:11PM +0100, Ard Biesheuvel wrote:
> > 52-bit VA kernels can run on hardware that is only 48-bit capable, but
> > configure the ID map as 52-bit by default. This was not a problem until
> > recently, because the special T0SZ value for a 52-bit VA space was never
> > programmed into the TCR register anwyay, and because a 52-bit ID map
> > happens to use the same number of translation levels as a 48-bit one.
> >
> > This behavior was changed by commit 1401bef703a4 ("arm64: mm: Always update
> > TCR_EL1 from __cpu_set_tcr_t0sz()"), which causes the unsupported T0SZ
> > value for a 52-bit VA to be programmed into TCR_EL1. While some hardware
> > simply ignores this, Mark reports that Amberwing systems choke on this,
> > resulting in a broken boot. But even before that commit, the unsupported
> > idmap_t0sz value was exposed to KVM and used to program TCR_EL2 incorrectly
> > as well.
> >
> > Given that we already have to deal with address spaces being either 48-bit
> > or 52-bit in size, the cleanest approach seems to be to simply default to
> > a 48-bit VA ID map, and only switch to a 52-bit one if the placement of the
> > kernel in DRAM requires it. This is guaranteed not to happen unless the
> > system is actually 52-bit VA capable.
> >
> > Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> > Reported-by: Mark Salter <msalter at redhat.com>
> > Link: http://lore.kernel.org/r/20210310003216.410037-1-msalter@redhat.com
> > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > ---
> >  arch/arm64/include/asm/mmu_context.h | 5 +----
> >  arch/arm64/kernel/head.S             | 2 +-
> >  arch/arm64/mm/mmu.c                  | 2 +-
> >  3 files changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> > index 70ce8c1d2b07..0f467d550f27 100644
> > --- a/arch/arm64/include/asm/mmu_context.h
> > +++ b/arch/arm64/include/asm/mmu_context.h
> > @@ -65,10 +65,7 @@ extern u64 idmap_ptrs_per_pgd;
> >
> >  static inline bool __cpu_uses_extended_idmap(void)
> >  {
> > -     if (IS_ENABLED(CONFIG_ARM64_VA_BITS_52))
> > -             return false;
> > -
> > -     return unlikely(idmap_t0sz != TCR_T0SZ(VA_BITS));
> > +     return unlikely(idmap_t0sz != TCR_T0SZ(vabits_actual));
>
> I'm a bit confused by this: if the idmap is using 48-bit VAs but
> vabits_actual is 52, then we'll return true despite not using the extended
> idmap. Have I missed something, or should that be < instead of != ?
>

Yeah, this is because before, we only ever extended the idmap, but in
this case, we are reducing it: the ID map is always 48 bits, and the
user space page tables could be 48 or 52. In the latter case, this
means we need to update TCR when we switch to the ID map or vice
versa, similar to how we switch to a 48-bit VA ID map on a 39-bit VA
kernel when physical DRAM cannot be covered with 39 bits.

The good news is that this function is dropped entirely in the next
patch. This hunk is only there for backports of this patch to -stable.
Therefore, I don't see the point to renaming it to sth like
'__cpu_has_different_idmap_size()'


> >  }
> >
> >  /*
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 66b0e0b66e31..0b0387644dc0 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -319,7 +319,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> >        */
> >       adrp    x5, __idmap_text_end
> >       clz     x5, x5
> > -     cmp     x5, TCR_T0SZ(VA_BITS)   // default T0SZ small enough?
> > +     cmp     x5, TCR_T0SZ(VA_BITS_MIN) // default T0SZ small enough?
> >       b.ge    1f                      // .. then skip VA range extension
>
> (nit: the comment immediately before this talks about VA_BITS and should
> probably be updated)
>
> Will