[PATCH 0/5] arm64: some 52-bit cleanups

[PATCH 0/5] arm64: some 52-bit cleanups

[Ard Biesheuvel]

This work was triggered by Mark Salter's report about a regression
regarding the programmed size of the ID map page tables.

The first patch switches the early ID map population code to default to
a 48-bit range on kernels built with 52-bit VA support, and only switch
to 52-bit if the placement of the kernel requires it. This should address
the regressions reported by Mark. This patch should be suitable for being
backported to -stable.

The remaining patches are cleanups that remove and/or simplify the 52-bit
VA support, and disable some parts in configurations that don't need it.

Patch #5 adds back the logic to use a 52-bit VA ID map on 52-bit VA capable
hardware, which we might consider if it is worthwhile for some reason to
elide the TCR_EL1 updates that result from having ID map and user space VA
ranges of different size.

Cc: Mark Salter <msalter@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Steve Capper <steve.capper@arm.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>

Ard Biesheuvel (5):
  arm64: mm: use a 48-bit ID map when possible on 52-bit VA builds
  arm64: mm: remove unused __cpu_uses_extended_idmap[_level()]
  arm64: mm: use a compile time constant for vabits_actual when possible
  arm64: mm: get rid of idmap_ptrs_per_pgd handling
  arm64: mm: switch to 52-bit ID map on 52-bit VA capable systems

 arch/arm64/include/asm/memory.h      |  4 +++
 arch/arm64/include/asm/mmu_context.h | 18 --------------
 arch/arm64/kernel/head.S             | 26 +++++++++-----------
 arch/arm64/mm/mmu.c                  |  5 ++--
 4 files changed, 18 insertions(+), 35 deletions(-)

-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ard Biesheuvel]

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@redhat.com>
Link: http://lore.kernel.org/r/20210310003216.410037-1-msalter@redhat.com
Signed-off-by: Ard Biesheuvel <ardb@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));
 }
 
 /*
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
 
 	adr_l	x6, idmap_t0sz
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 3802cfbdd20d..4c5603c41870 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -40,7 +40,7 @@
 #define NO_BLOCK_MAPPINGS	BIT(0)
 #define NO_CONT_MAPPINGS	BIT(1)
 
-u64 idmap_t0sz = TCR_T0SZ(VA_BITS);
+u64 idmap_t0sz = TCR_T0SZ(VA_BITS_MIN);
 u64 idmap_ptrs_per_pgd = PTRS_PER_PGD;
 
 u64 __section(".mmuoff.data.write") vabits_actual;
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Will Deacon]

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@redhat.com>
> Link: http://lore.kernel.org/r/20210310003216.410037-1-msalter@redhat.com
> Signed-off-by: Ard Biesheuvel <ardb@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 != ?

>  }
>  
>  /*
> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@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@redhat.com>
> > Link: http://lore.kernel.org/r/20210310003216.410037-1-msalter@redhat.com
> > Signed-off-by: Ard Biesheuvel <ardb@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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] arm64: mm: remove unused __cpu_uses_extended_idmap[_level()]

[Ard Biesheuvel]

These routines lost all existing users during the latest merge window so
we can remove them. This avoids the need to fix them in the context of
fixing a regression related to the ID map on 52-bit VA kernels.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/mmu_context.h | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index 0f467d550f27..bd02e99b1a4c 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -63,20 +63,6 @@ static inline void cpu_switch_mm(pgd_t *pgd, struct mm_struct *mm)
 extern u64 idmap_t0sz;
 extern u64 idmap_ptrs_per_pgd;
 
-static inline bool __cpu_uses_extended_idmap(void)
-{
-	return unlikely(idmap_t0sz != TCR_T0SZ(vabits_actual));
-}
-
-/*
- * True if the extended ID map requires an extra level of translation table
- * to be configured.
- */
-static inline bool __cpu_uses_extended_idmap_level(void)
-{
-	return ARM64_HW_PGTABLE_LEVELS(64 - idmap_t0sz) > CONFIG_PGTABLE_LEVELS;
-}
-
 /*
  * Ensure TCR.T0SZ is set to the provided value.
  */
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] arm64: mm: use a compile time constant for vabits_actual when possible

[Ard Biesheuvel]

The size of the kernel VA space is a compile time constant unless the
kernel is built to support 52-bit virtual addressing, which today is
only supported on 64k page size kernels (although this has recently
changed in the architecture).

This means that in many configurations, vabits_actual can never deviate
from its build time default, making it rather pointless to carry this
value in a variable. So use a compile time constant for vabits_actual
unless it can really assume different values.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/memory.h |  4 ++++
 arch/arm64/kernel/head.S        | 12 ++++++------
 arch/arm64/mm/mmu.c             |  2 ++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index c759faf7a1ff..501c5c87ec0a 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -179,7 +179,11 @@
 #include <linux/types.h>
 #include <asm/bug.h>
 
+#ifdef CONFIG_ARM64_VA_BITS_52
 extern u64			vabits_actual;
+#else
+#define vabits_actual		((u64)VA_BITS)
+#endif
 
 extern s64			memstart_addr;
 /* PHYS_OFFSET - the physical address of the start of memory. */
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 0b0387644dc0..f65d17a90204 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -294,16 +294,16 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
 
 #ifdef CONFIG_ARM64_VA_BITS_52
 	mrs_s	x6, SYS_ID_AA64MMFR2_EL1
-	and	x6, x6, #(0xf << ID_AA64MMFR2_LVA_SHIFT)
-	mov	x5, #52
-	cbnz	x6, 1f
-#endif
-	mov	x5, #VA_BITS_MIN
-1:
+	tst	x6, #(0xf << ID_AA64MMFR2_LVA_SHIFT)
+	mov	x5, #VA_BITS
+	mov	x6, #VA_BITS_MIN
+	csel	x5, x5, x6, ne
+
 	adr_l	x6, vabits_actual
 	str	x5, [x6]
 	dmb	sy
 	dc	ivac, x6		// Invalidate potentially stale cache line
+#endif
 
 	/*
 	 * VA_BITS may be too small to allow for an ID mapping to be created
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 4c5603c41870..338658cd1bea 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -43,8 +43,10 @@
 u64 idmap_t0sz = TCR_T0SZ(VA_BITS_MIN);
 u64 idmap_ptrs_per_pgd = PTRS_PER_PGD;
 
+#ifdef CONFIG_ARM64_VA_BITS_52
 u64 __section(".mmuoff.data.write") vabits_actual;
 EXPORT_SYMBOL(vabits_actual);
+#endif
 
 u64 kimage_voffset __ro_after_init;
 EXPORT_SYMBOL(kimage_voffset);
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] arm64: mm: use a compile time constant for vabits_actual when possible

[Will Deacon]

On Wed, Mar 10, 2021 at 06:15:13PM +0100, Ard Biesheuvel wrote:
> The size of the kernel VA space is a compile time constant unless the
> kernel is built to support 52-bit virtual addressing, which today is
> only supported on 64k page size kernels (although this has recently
> changed in the architecture).
> 
> This means that in many configurations, vabits_actual can never deviate
> from its build time default, making it rather pointless to carry this
> value in a variable. So use a compile time constant for vabits_actual
> unless it can really assume different values.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/include/asm/memory.h |  4 ++++
>  arch/arm64/kernel/head.S        | 12 ++++++------
>  arch/arm64/mm/mmu.c             |  2 ++
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index c759faf7a1ff..501c5c87ec0a 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -179,7 +179,11 @@
>  #include <linux/types.h>
>  #include <asm/bug.h>
>  
> +#ifdef CONFIG_ARM64_VA_BITS_52
>  extern u64			vabits_actual;
> +#else
> +#define vabits_actual		((u64)VA_BITS)
> +#endif

Maybe we should have VA_BITS_MIN, VA_BITS and VA_BITS_MAX instead of
the current VA_BITS_MIN, VA_BITS and vabits_actual? The current naming is
definitely a source of confusion for me.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] arm64: mm: use a compile time constant for vabits_actual when possible

[Ard Biesheuvel]

On Thu, 11 Mar 2021 at 10:49, Will Deacon <will@kernel.org> wrote:
>
> On Wed, Mar 10, 2021 at 06:15:13PM +0100, Ard Biesheuvel wrote:
> > The size of the kernel VA space is a compile time constant unless the
> > kernel is built to support 52-bit virtual addressing, which today is
> > only supported on 64k page size kernels (although this has recently
> > changed in the architecture).
> >
> > This means that in many configurations, vabits_actual can never deviate
> > from its build time default, making it rather pointless to carry this
> > value in a variable. So use a compile time constant for vabits_actual
> > unless it can really assume different values.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/arm64/include/asm/memory.h |  4 ++++
> >  arch/arm64/kernel/head.S        | 12 ++++++------
> >  arch/arm64/mm/mmu.c             |  2 ++
> >  3 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index c759faf7a1ff..501c5c87ec0a 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -179,7 +179,11 @@
> >  #include <linux/types.h>
> >  #include <asm/bug.h>
> >
> > +#ifdef CONFIG_ARM64_VA_BITS_52
> >  extern u64                   vabits_actual;
> > +#else
> > +#define vabits_actual                ((u64)VA_BITS)
> > +#endif
>
> Maybe we should have VA_BITS_MIN, VA_BITS and VA_BITS_MAX instead of
> the current VA_BITS_MIN, VA_BITS and vabits_actual? The current naming is
> definitely a source of confusion for me.
>



We will still need build time and runtime versions, and at build time,
we sometimes need the minimum value (e.g, for sizing the vmalloc
region) and sometimes the maximum value (e.g., in the definition of
PTRS_PER_PGD). We could rename VA_BITS to VA_BITS_MAX, as you suggest,
and find another name for vabits_actual, but we definitely need all
these three quantities in one way or another.

However, renaming vabits_actual to VA_BITS is likely to cause
confusion as well, due to the change in meaning, as well as the fact
that all-caps identifiers are usually build time constants.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] arm64: mm: use a compile time constant for vabits_actual when possible

[Will Deacon]

On Thu, Mar 11, 2021 at 06:23:33PM +0100, Ard Biesheuvel wrote:
> On Thu, 11 Mar 2021 at 10:49, Will Deacon <will@kernel.org> wrote:
> >
> > On Wed, Mar 10, 2021 at 06:15:13PM +0100, Ard Biesheuvel wrote:
> > > The size of the kernel VA space is a compile time constant unless the
> > > kernel is built to support 52-bit virtual addressing, which today is
> > > only supported on 64k page size kernels (although this has recently
> > > changed in the architecture).
> > >
> > > This means that in many configurations, vabits_actual can never deviate
> > > from its build time default, making it rather pointless to carry this
> > > value in a variable. So use a compile time constant for vabits_actual
> > > unless it can really assume different values.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  arch/arm64/include/asm/memory.h |  4 ++++
> > >  arch/arm64/kernel/head.S        | 12 ++++++------
> > >  arch/arm64/mm/mmu.c             |  2 ++
> > >  3 files changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > index c759faf7a1ff..501c5c87ec0a 100644
> > > --- a/arch/arm64/include/asm/memory.h
> > > +++ b/arch/arm64/include/asm/memory.h
> > > @@ -179,7 +179,11 @@
> > >  #include <linux/types.h>
> > >  #include <asm/bug.h>
> > >
> > > +#ifdef CONFIG_ARM64_VA_BITS_52
> > >  extern u64                   vabits_actual;
> > > +#else
> > > +#define vabits_actual                ((u64)VA_BITS)
> > > +#endif
> >
> > Maybe we should have VA_BITS_MIN, VA_BITS and VA_BITS_MAX instead of
> > the current VA_BITS_MIN, VA_BITS and vabits_actual? The current naming is
> > definitely a source of confusion for me.
> >
> 
> 
> 
> We will still need build time and runtime versions, and at build time,
> we sometimes need the minimum value (e.g, for sizing the vmalloc
> region) and sometimes the maximum value (e.g., in the definition of
> PTRS_PER_PGD). We could rename VA_BITS to VA_BITS_MAX, as you suggest,
> and find another name for vabits_actual, but we definitely need all
> these three quantities in one way or another.

Right, I was thinking that for !52-bit VA we've have:

#define VA_BITS		CONFIG_ARM64_VA_BITS
#define VA_BITS_MIN	VA_BITS_MIN
#define	VA_BITS_MAX	VA_BITS_MIN

and then for 52-bit VA we'd have:

#define VA_BITS		(vabits_actual)
#define VA_BITS_MIN	48
#define VA_BITS_MAX	52

> However, renaming vabits_actual to VA_BITS is likely to cause
> confusion as well, due to the change in meaning, as well as the fact
> that all-caps identifiers are usually build time constants.

Yeah, fair enough, but it reads a tonne better imo.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] arm64: mm: use a compile time constant for vabits_actual when possible

[Ard Biesheuvel]

On Thu, 11 Mar 2021 at 19:44, Will Deacon <will@kernel.org> wrote:
>
> On Thu, Mar 11, 2021 at 06:23:33PM +0100, Ard Biesheuvel wrote:
> > On Thu, 11 Mar 2021 at 10:49, Will Deacon <will@kernel.org> wrote:
> > >
> > > On Wed, Mar 10, 2021 at 06:15:13PM +0100, Ard Biesheuvel wrote:
> > > > The size of the kernel VA space is a compile time constant unless the
> > > > kernel is built to support 52-bit virtual addressing, which today is
> > > > only supported on 64k page size kernels (although this has recently
> > > > changed in the architecture).
> > > >
> > > > This means that in many configurations, vabits_actual can never deviate
> > > > from its build time default, making it rather pointless to carry this
> > > > value in a variable. So use a compile time constant for vabits_actual
> > > > unless it can really assume different values.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >  arch/arm64/include/asm/memory.h |  4 ++++
> > > >  arch/arm64/kernel/head.S        | 12 ++++++------
> > > >  arch/arm64/mm/mmu.c             |  2 ++
> > > >  3 files changed, 12 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > > index c759faf7a1ff..501c5c87ec0a 100644
> > > > --- a/arch/arm64/include/asm/memory.h
> > > > +++ b/arch/arm64/include/asm/memory.h
> > > > @@ -179,7 +179,11 @@
> > > >  #include <linux/types.h>
> > > >  #include <asm/bug.h>
> > > >
> > > > +#ifdef CONFIG_ARM64_VA_BITS_52
> > > >  extern u64                   vabits_actual;
> > > > +#else
> > > > +#define vabits_actual                ((u64)VA_BITS)
> > > > +#endif
> > >
> > > Maybe we should have VA_BITS_MIN, VA_BITS and VA_BITS_MAX instead of
> > > the current VA_BITS_MIN, VA_BITS and vabits_actual? The current naming is
> > > definitely a source of confusion for me.
> > >
> >
> >
> >
> > We will still need build time and runtime versions, and at build time,
> > we sometimes need the minimum value (e.g, for sizing the vmalloc
> > region) and sometimes the maximum value (e.g., in the definition of
> > PTRS_PER_PGD). We could rename VA_BITS to VA_BITS_MAX, as you suggest,
> > and find another name for vabits_actual, but we definitely need all
> > these three quantities in one way or another.
>
> Right, I was thinking that for !52-bit VA we've have:
>
> #define VA_BITS         CONFIG_ARM64_VA_BITS
> #define VA_BITS_MIN     VA_BITS_MIN
> #define VA_BITS_MAX     VA_BITS_MIN
>
> and then for 52-bit VA we'd have:
>
> #define VA_BITS         (vabits_actual)
> #define VA_BITS_MIN     48
> #define VA_BITS_MAX     52
>
> > However, renaming vabits_actual to VA_BITS is likely to cause
> > confusion as well, due to the change in meaning, as well as the fact
> > that all-caps identifiers are usually build time constants.
>
> Yeah, fair enough, but it reads a tonne better imo.
>

Can't we just stop using VA_BITS altogether, and only keep the _MIN
and _MAX variants?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] arm64: mm: get rid of idmap_ptrs_per_pgd handling

[Ard Biesheuvel]

idmap_ptrs_per_pgd is stored and re-loaded in the very early boot path,
and never referenced again. So a variable is not needed here.

Furthermore, given that on the code path in question, PTRS_PER_PGD and
'1 << (PHYS_MASK_SHIFT - PGDIR_SHIFT)' are guaranteed to be equal unless
- the kernel was built for 48-bit VAs and 52-bit PAs, and
- the kernel was loaded outside of the 48-bit addressable physical memory
  space (which is not supported by EFI to begin with),
it seems reasonable to simply drop this code path altogether.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/mmu_context.h | 1 -
 arch/arm64/kernel/head.S             | 9 +--------
 arch/arm64/mm/mmu.c                  | 1 -
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index bd02e99b1a4c..1307f5e27a5a 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -61,7 +61,6 @@ static inline void cpu_switch_mm(pgd_t *pgd, struct mm_struct *mm)
  * physical memory, in which case it will be smaller.
  */
 extern u64 idmap_t0sz;
-extern u64 idmap_ptrs_per_pgd;
 
 /*
  * Ensure TCR.T0SZ is set to the provided value.
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index f65d17a90204..da6e99fa4e08 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -344,16 +344,9 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
 
 	mov	x4, EXTRA_PTRS
 	create_table_entry x0, x3, EXTRA_SHIFT, x4, x5, x6
-#else
-	/*
-	 * If VA_BITS == 48, we don't have to configure an additional
-	 * translation level, but the top-level table has more entries.
-	 */
-	mov	x4, #1 << (PHYS_MASK_SHIFT - PGDIR_SHIFT)
-	str_l	x4, idmap_ptrs_per_pgd, x5
 #endif
 1:
-	ldr_l	x4, idmap_ptrs_per_pgd
+	mov	x4, PTRS_PER_PGD
 	mov	x5, x3				// __pa(__idmap_text_start)
 	adr_l	x6, __idmap_text_end		// __pa(__idmap_text_end)
 
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 338658cd1bea..569cf07ddb91 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -41,7 +41,6 @@
 #define NO_CONT_MAPPINGS	BIT(1)
 
 u64 idmap_t0sz = TCR_T0SZ(VA_BITS_MIN);
-u64 idmap_ptrs_per_pgd = PTRS_PER_PGD;
 
 #ifdef CONFIG_ARM64_VA_BITS_52
 u64 __section(".mmuoff.data.write") vabits_actual;
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] arm64: mm: switch to 52-bit ID map on 52-bit VA capable systems

[Ard Biesheuvel]

Now that we cleaned up the code a bit, add back the code path to use a
52-bit VA ID map on hardware that is 52-bit VA capable. This removes
a TCR update from the cpuidle path.

At the same time, make the 52-bit VA handling mutually exclusive with
the extended ID map code: a 52-bit VA kernel running on 48-bit VA only
hardware never has a need for it.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/head.S | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index da6e99fa4e08..06f1ddeef821 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -303,8 +303,10 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
 	str	x5, [x6]
 	dmb	sy
 	dc	ivac, x6		// Invalidate potentially stale cache line
-#endif
 
+	mov	x5, TCR_T0SZ(VA_BITS)
+	b.eq	1f			// skip VA range extension on !LVA hardware
+#else
 	/*
 	 * VA_BITS may be too small to allow for an ID mapping to be created
 	 * that covers system RAM if that is located sufficiently high in the
@@ -321,6 +323,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
 	clz	x5, x5
 	cmp	x5, TCR_T0SZ(VA_BITS_MIN) // default T0SZ small enough?
 	b.ge	1f			// .. then skip VA range extension
+#endif
 
 	adr_l	x6, idmap_t0sz
 	str	x5, [x6]
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] arm64: some 52-bit cleanups

[Will Deacon]

On Wed, 10 Mar 2021 18:15:10 +0100, Ard Biesheuvel wrote:
> This work was triggered by Mark Salter's report about a regression
> regarding the programmed size of the ID map page tables.
> 
> The first patch switches the early ID map population code to default to
> a 48-bit range on kernels built with 52-bit VA support, and only switch
> to 52-bit if the placement of the kernel requires it. This should address
> the regressions reported by Mark. This patch should be suitable for being
> backported to -stable.
> 
> [...]

Applied the first two patches to arm64 (for-next/fixes), thanks!

[1/5] arm64: mm: use a 48-bit ID map when possible on 52-bit VA builds
      https://git.kernel.org/arm64/c/7ba8f2b2d652
[2/5] arm64: mm: remove unused __cpu_uses_extended_idmap[_level()]
      https://git.kernel.org/arm64/c/30b2675761b8

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel