* [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
@ 2021-03-10 0:32 ` Mark Salter
0 siblings, 0 replies; 10+ messages in thread
From: Mark Salter @ 2021-03-10 0:32 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel; +Cc: Catalin Marinas, Will Deacon, Mark Rutland
I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
kernel.
The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
to call idmap_cpu_replace_ttbr1().
Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
VA seem to handle the setting of an unsupported t0sz without any apparent
problems. Indeed, if one reads back the tcr written with t0sz==12, the
value read has t0sz==16. Not so with Amberwing.
Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
Signed-off-by: Mark Salter <msalter@redhat.com>
---
arch/arm64/kernel/head.S | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 66b0e0b66e31..2bcbbb26292e 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
*/
adrp x0, idmap_pg_dir
adrp x3, __idmap_text_start // __pa(__idmap_text_start)
+ mov x4, TCR_T0SZ(VA_BITS)
#ifdef CONFIG_ARM64_VA_BITS_52
mrs_s x6, SYS_ID_AA64MMFR2_EL1
@@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
cbnz x6, 1f
#endif
mov x5, #VA_BITS_MIN
+#ifdef CONFIG_ARM64_VA_BITS_52
+ mov x4, TCR_T0SZ(VA_BITS_MIN)
+ adr_l x6, idmap_t0sz
+ str x4, [x6]
+ dmb sy
+ dc ivac, x6 // Invalidate potentially stale cache line
+#endif
1:
adr_l x6, vabits_actual
str x5, [x6]
@@ -319,7 +327,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, x4 // default T0SZ small enough?
b.ge 1f // .. then skip VA range extension
adr_l x6, idmap_t0sz
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
@ 2021-03-10 0:32 ` Mark Salter
0 siblings, 0 replies; 10+ messages in thread
From: Mark Salter @ 2021-03-10 0:32 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel; +Cc: Catalin Marinas, Will Deacon, Mark Rutland
I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
kernel.
The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
to call idmap_cpu_replace_ttbr1().
Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
VA seem to handle the setting of an unsupported t0sz without any apparent
problems. Indeed, if one reads back the tcr written with t0sz==12, the
value read has t0sz==16. Not so with Amberwing.
Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
Signed-off-by: Mark Salter <msalter@redhat.com>
---
arch/arm64/kernel/head.S | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 66b0e0b66e31..2bcbbb26292e 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
*/
adrp x0, idmap_pg_dir
adrp x3, __idmap_text_start // __pa(__idmap_text_start)
+ mov x4, TCR_T0SZ(VA_BITS)
#ifdef CONFIG_ARM64_VA_BITS_52
mrs_s x6, SYS_ID_AA64MMFR2_EL1
@@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
cbnz x6, 1f
#endif
mov x5, #VA_BITS_MIN
+#ifdef CONFIG_ARM64_VA_BITS_52
+ mov x4, TCR_T0SZ(VA_BITS_MIN)
+ adr_l x6, idmap_t0sz
+ str x4, [x6]
+ dmb sy
+ dc ivac, x6 // Invalidate potentially stale cache line
+#endif
1:
adr_l x6, vabits_actual
str x5, [x6]
@@ -319,7 +327,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, x4 // default T0SZ small enough?
b.ge 1f // .. then skip VA range extension
adr_l x6, idmap_t0sz
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
2021-03-10 0:32 ` Mark Salter
@ 2021-03-10 11:16 ` Will Deacon
-1 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2021-03-10 11:16 UTC (permalink / raw)
To: Mark Salter; +Cc: linux-arm-kernel, linux-kernel, Catalin Marinas, Mark Rutland
On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> kernel.
>
> The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> to call idmap_cpu_replace_ttbr1().
>
> Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> VA seem to handle the setting of an unsupported t0sz without any apparent
> problems. Indeed, if one reads back the tcr written with t0sz==12, the
> value read has t0sz==16. Not so with Amberwing.
Nice, you have one of those elusive platforms!
> Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
> arch/arm64/kernel/head.S | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 66b0e0b66e31..2bcbbb26292e 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> */
> adrp x0, idmap_pg_dir
> adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> + mov x4, TCR_T0SZ(VA_BITS)
>
> #ifdef CONFIG_ARM64_VA_BITS_52
> mrs_s x6, SYS_ID_AA64MMFR2_EL1
> @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> cbnz x6, 1f
> #endif
> mov x5, #VA_BITS_MIN
> +#ifdef CONFIG_ARM64_VA_BITS_52
> + mov x4, TCR_T0SZ(VA_BITS_MIN)
> + adr_l x6, idmap_t0sz
> + str x4, [x6]
> + dmb sy
> + dc ivac, x6 // Invalidate potentially stale cache line
> +#endif
> 1:
> adr_l x6, vabits_actual
> str x5, [x6]
> @@ -319,7 +327,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, x4 // default T0SZ small enough?
> b.ge 1f // .. then skip VA range extension
Could we instead have the default value be 48-bit, and then avoid having
to update the variable in both cases? e.g. something along the lines of
the entirely untested diff below?
Cheers,
Will
--->8
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 66b0e0b66e31..fb795123896f 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;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
@ 2021-03-10 11:16 ` Will Deacon
0 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2021-03-10 11:16 UTC (permalink / raw)
To: Mark Salter; +Cc: linux-arm-kernel, linux-kernel, Catalin Marinas, Mark Rutland
On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> kernel.
>
> The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> to call idmap_cpu_replace_ttbr1().
>
> Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> VA seem to handle the setting of an unsupported t0sz without any apparent
> problems. Indeed, if one reads back the tcr written with t0sz==12, the
> value read has t0sz==16. Not so with Amberwing.
Nice, you have one of those elusive platforms!
> Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
> arch/arm64/kernel/head.S | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 66b0e0b66e31..2bcbbb26292e 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> */
> adrp x0, idmap_pg_dir
> adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> + mov x4, TCR_T0SZ(VA_BITS)
>
> #ifdef CONFIG_ARM64_VA_BITS_52
> mrs_s x6, SYS_ID_AA64MMFR2_EL1
> @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> cbnz x6, 1f
> #endif
> mov x5, #VA_BITS_MIN
> +#ifdef CONFIG_ARM64_VA_BITS_52
> + mov x4, TCR_T0SZ(VA_BITS_MIN)
> + adr_l x6, idmap_t0sz
> + str x4, [x6]
> + dmb sy
> + dc ivac, x6 // Invalidate potentially stale cache line
> +#endif
> 1:
> adr_l x6, vabits_actual
> str x5, [x6]
> @@ -319,7 +327,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, x4 // default T0SZ small enough?
> b.ge 1f // .. then skip VA range extension
Could we instead have the default value be 48-bit, and then avoid having
to update the variable in both cases? e.g. something along the lines of
the entirely untested diff below?
Cheers,
Will
--->8
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 66b0e0b66e31..fb795123896f 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;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
2021-03-10 11:16 ` Will Deacon
@ 2021-03-10 12:41 ` Ard Biesheuvel
-1 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2021-03-10 12:41 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Salter, Linux ARM, Linux Kernel Mailing List,
Catalin Marinas, Mark Rutland
On Wed, 10 Mar 2021 at 12:18, Will Deacon <will@kernel.org> wrote:
>
> On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> > I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> > kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> > kernel.
> >
> > The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> > updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> > hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> > from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> > to call idmap_cpu_replace_ttbr1().
> >
> > Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> > VA seem to handle the setting of an unsupported t0sz without any apparent
> > problems. Indeed, if one reads back the tcr written with t0sz==12, the
> > value read has t0sz==16. Not so with Amberwing.
>
> Nice, you have one of those elusive platforms!
>
> > Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > ---
> > arch/arm64/kernel/head.S | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 66b0e0b66e31..2bcbbb26292e 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > */
> > adrp x0, idmap_pg_dir
> > adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> > + mov x4, TCR_T0SZ(VA_BITS)
> >
> > #ifdef CONFIG_ARM64_VA_BITS_52
> > mrs_s x6, SYS_ID_AA64MMFR2_EL1
> > @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > cbnz x6, 1f
> > #endif
> > mov x5, #VA_BITS_MIN
> > +#ifdef CONFIG_ARM64_VA_BITS_52
> > + mov x4, TCR_T0SZ(VA_BITS_MIN)
> > + adr_l x6, idmap_t0sz
> > + str x4, [x6]
> > + dmb sy
> > + dc ivac, x6 // Invalidate potentially stale cache line
> > +#endif
> > 1:
> > adr_l x6, vabits_actual
> > str x5, [x6]
> > @@ -319,7 +327,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, x4 // default T0SZ small enough?
> > b.ge 1f // .. then skip VA range extension
>
> Could we instead have the default value be 48-bit, and then avoid having
> to update the variable in both cases? e.g. something along the lines of
> the entirely untested diff below?
>
There is one other occurrence that needs to be updated. I will have a
stab at fixing this along these lines, and there are a couple of other
things that need cleaning up.
> --->8
>
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 66b0e0b66e31..fb795123896f 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;
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
@ 2021-03-10 12:41 ` Ard Biesheuvel
0 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2021-03-10 12:41 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Salter, Linux ARM, Linux Kernel Mailing List,
Catalin Marinas, Mark Rutland
On Wed, 10 Mar 2021 at 12:18, Will Deacon <will@kernel.org> wrote:
>
> On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> > I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> > kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> > kernel.
> >
> > The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> > updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> > hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> > from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> > to call idmap_cpu_replace_ttbr1().
> >
> > Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> > VA seem to handle the setting of an unsupported t0sz without any apparent
> > problems. Indeed, if one reads back the tcr written with t0sz==12, the
> > value read has t0sz==16. Not so with Amberwing.
>
> Nice, you have one of those elusive platforms!
>
> > Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > ---
> > arch/arm64/kernel/head.S | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 66b0e0b66e31..2bcbbb26292e 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > */
> > adrp x0, idmap_pg_dir
> > adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> > + mov x4, TCR_T0SZ(VA_BITS)
> >
> > #ifdef CONFIG_ARM64_VA_BITS_52
> > mrs_s x6, SYS_ID_AA64MMFR2_EL1
> > @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > cbnz x6, 1f
> > #endif
> > mov x5, #VA_BITS_MIN
> > +#ifdef CONFIG_ARM64_VA_BITS_52
> > + mov x4, TCR_T0SZ(VA_BITS_MIN)
> > + adr_l x6, idmap_t0sz
> > + str x4, [x6]
> > + dmb sy
> > + dc ivac, x6 // Invalidate potentially stale cache line
> > +#endif
> > 1:
> > adr_l x6, vabits_actual
> > str x5, [x6]
> > @@ -319,7 +327,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, x4 // default T0SZ small enough?
> > b.ge 1f // .. then skip VA range extension
>
> Could we instead have the default value be 48-bit, and then avoid having
> to update the variable in both cases? e.g. something along the lines of
> the entirely untested diff below?
>
There is one other occurrence that needs to be updated. I will have a
stab at fixing this along these lines, and there are a couple of other
things that need cleaning up.
> --->8
>
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 66b0e0b66e31..fb795123896f 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;
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
2021-03-10 12:41 ` Ard Biesheuvel
@ 2021-03-10 13:29 ` Ard Biesheuvel
-1 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2021-03-10 13:29 UTC (permalink / raw)
To: Will Deacon, James Morse
Cc: Mark Salter, Linux ARM, Linux Kernel Mailing List,
Catalin Marinas, Mark Rutland
(+ James)
On Wed, 10 Mar 2021 at 13:41, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 10 Mar 2021 at 12:18, Will Deacon <will@kernel.org> wrote:
> >
> > On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> > > I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> > > kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> > > kernel.
> > >
> > > The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> > > updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> > > hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> > > from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> > > to call idmap_cpu_replace_ttbr1().
> > >
> > > Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> > > VA seem to handle the setting of an unsupported t0sz without any apparent
> > > problems. Indeed, if one reads back the tcr written with t0sz==12, the
> > > value read has t0sz==16. Not so with Amberwing.
> >
> > Nice, you have one of those elusive platforms!
> >
> > > Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > ---
> > > arch/arm64/kernel/head.S | 10 +++++++++-
> > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > > index 66b0e0b66e31..2bcbbb26292e 100644
> > > --- a/arch/arm64/kernel/head.S
> > > +++ b/arch/arm64/kernel/head.S
> > > @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > */
> > > adrp x0, idmap_pg_dir
> > > adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> > > + mov x4, TCR_T0SZ(VA_BITS)
> > >
> > > #ifdef CONFIG_ARM64_VA_BITS_52
> > > mrs_s x6, SYS_ID_AA64MMFR2_EL1
> > > @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > cbnz x6, 1f
> > > #endif
> > > mov x5, #VA_BITS_MIN
> > > +#ifdef CONFIG_ARM64_VA_BITS_52
> > > + mov x4, TCR_T0SZ(VA_BITS_MIN)
> > > + adr_l x6, idmap_t0sz
> > > + str x4, [x6]
> > > + dmb sy
> > > + dc ivac, x6 // Invalidate potentially stale cache line
> > > +#endif
> > > 1:
> > > adr_l x6, vabits_actual
> > > str x5, [x6]
> > > @@ -319,7 +327,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, x4 // default T0SZ small enough?
> > > b.ge 1f // .. then skip VA range extension
> >
> > Could we instead have the default value be 48-bit, and then avoid having
> > to update the variable in both cases? e.g. something along the lines of
> > the entirely untested diff below?
> >
>
> There is one other occurrence that needs to be updated. I will have a
> stab at fixing this along these lines, and there are a couple of other
> things that need cleaning up.
>
Actually, it seems like this breakage may have been introduced by
commit 1401bef703a48cf79c674215f702a1435362beae
Author: James Morse <james.morse@arm.com>
Date: Mon Jan 25 14:19:12 2021 -0500
arm64: mm: Always update TCR_EL1 from __cpu_set_tcr_t0sz()
as before that patch, we would never actually load idmap_t0sz into TCR
on 52-bit VA kernels, AFAICT.
It seems to me that the sanest approach is to default to a 48-bit
idmap on 52-bit VA capable kernels (as Will suggests below). This
means that the notion of 'extended ID map' now also includes cases
where the ID map is smaller than the default VA_BITS range. However,
the number of levels is going to be the same, so it does not affect
the ID map page table population code in head.S.
Also, __cpu_uses_extended_idmap() no longer has any users anyway, so
we can remove it rather than bikeshed over a better name for it.
>
>
> > --->8
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 66b0e0b66e31..fb795123896f 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;
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
@ 2021-03-10 13:29 ` Ard Biesheuvel
0 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2021-03-10 13:29 UTC (permalink / raw)
To: Will Deacon, James Morse
Cc: Mark Salter, Linux ARM, Linux Kernel Mailing List,
Catalin Marinas, Mark Rutland
(+ James)
On Wed, 10 Mar 2021 at 13:41, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 10 Mar 2021 at 12:18, Will Deacon <will@kernel.org> wrote:
> >
> > On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> > > I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> > > kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> > > kernel.
> > >
> > > The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> > > updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> > > hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> > > from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> > > to call idmap_cpu_replace_ttbr1().
> > >
> > > Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> > > VA seem to handle the setting of an unsupported t0sz without any apparent
> > > problems. Indeed, if one reads back the tcr written with t0sz==12, the
> > > value read has t0sz==16. Not so with Amberwing.
> >
> > Nice, you have one of those elusive platforms!
> >
> > > Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > ---
> > > arch/arm64/kernel/head.S | 10 +++++++++-
> > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > > index 66b0e0b66e31..2bcbbb26292e 100644
> > > --- a/arch/arm64/kernel/head.S
> > > +++ b/arch/arm64/kernel/head.S
> > > @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > */
> > > adrp x0, idmap_pg_dir
> > > adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> > > + mov x4, TCR_T0SZ(VA_BITS)
> > >
> > > #ifdef CONFIG_ARM64_VA_BITS_52
> > > mrs_s x6, SYS_ID_AA64MMFR2_EL1
> > > @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > cbnz x6, 1f
> > > #endif
> > > mov x5, #VA_BITS_MIN
> > > +#ifdef CONFIG_ARM64_VA_BITS_52
> > > + mov x4, TCR_T0SZ(VA_BITS_MIN)
> > > + adr_l x6, idmap_t0sz
> > > + str x4, [x6]
> > > + dmb sy
> > > + dc ivac, x6 // Invalidate potentially stale cache line
> > > +#endif
> > > 1:
> > > adr_l x6, vabits_actual
> > > str x5, [x6]
> > > @@ -319,7 +327,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, x4 // default T0SZ small enough?
> > > b.ge 1f // .. then skip VA range extension
> >
> > Could we instead have the default value be 48-bit, and then avoid having
> > to update the variable in both cases? e.g. something along the lines of
> > the entirely untested diff below?
> >
>
> There is one other occurrence that needs to be updated. I will have a
> stab at fixing this along these lines, and there are a couple of other
> things that need cleaning up.
>
Actually, it seems like this breakage may have been introduced by
commit 1401bef703a48cf79c674215f702a1435362beae
Author: James Morse <james.morse@arm.com>
Date: Mon Jan 25 14:19:12 2021 -0500
arm64: mm: Always update TCR_EL1 from __cpu_set_tcr_t0sz()
as before that patch, we would never actually load idmap_t0sz into TCR
on 52-bit VA kernels, AFAICT.
It seems to me that the sanest approach is to default to a 48-bit
idmap on 52-bit VA capable kernels (as Will suggests below). This
means that the notion of 'extended ID map' now also includes cases
where the ID map is smaller than the default VA_BITS range. However,
the number of levels is going to be the same, so it does not affect
the ID map page table population code in head.S.
Also, __cpu_uses_extended_idmap() no longer has any users anyway, so
we can remove it rather than bikeshed over a better name for it.
>
>
> > --->8
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 66b0e0b66e31..fb795123896f 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;
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
2021-03-10 13:29 ` Ard Biesheuvel
@ 2021-03-10 14:08 ` Mark Salter
-1 siblings, 0 replies; 10+ messages in thread
From: Mark Salter @ 2021-03-10 14:08 UTC (permalink / raw)
To: Ard Biesheuvel, Will Deacon, James Morse
Cc: Linux ARM, Linux Kernel Mailing List, Catalin Marinas, Mark Rutland
On Wed, 2021-03-10 at 14:29 +0100, Ard Biesheuvel wrote:
> (+ James)
>
> On Wed, 10 Mar 2021 at 13:41, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 10 Mar 2021 at 12:18, Will Deacon <will@kernel.org> wrote:
> > >
> > > On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> > > > I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> > > > kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> > > > kernel.
> > > >
> > > > The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> > > > updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> > > > hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> > > > from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> > > > to call idmap_cpu_replace_ttbr1().
> > > >
> > > > Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> > > > VA seem to handle the setting of an unsupported t0sz without any apparent
> > > > problems. Indeed, if one reads back the tcr written with t0sz==12, the
> > > > value read has t0sz==16. Not so with Amberwing.
> > >
> > > Nice, you have one of those elusive platforms!
> > >
> > > > Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> > > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > > ---
> > > > arch/arm64/kernel/head.S | 10 +++++++++-
> > > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > > > index 66b0e0b66e31..2bcbbb26292e 100644
> > > > --- a/arch/arm64/kernel/head.S
> > > > +++ b/arch/arm64/kernel/head.S
> > > > @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > > */
> > > > adrp x0, idmap_pg_dir
> > > > adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> > > > + mov x4, TCR_T0SZ(VA_BITS)
> > > >
> > > > #ifdef CONFIG_ARM64_VA_BITS_52
> > > > mrs_s x6, SYS_ID_AA64MMFR2_EL1
> > > > @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > > cbnz x6, 1f
> > > > #endif
> > > > mov x5, #VA_BITS_MIN
> > > > +#ifdef CONFIG_ARM64_VA_BITS_52
> > > > + mov x4, TCR_T0SZ(VA_BITS_MIN)
> > > > + adr_l x6, idmap_t0sz
> > > > + str x4, [x6]
> > > > + dmb sy
> > > > + dc ivac, x6 // Invalidate potentially stale cache line
> > > > +#endif
> > > > 1:
> > > > adr_l x6, vabits_actual
> > > > str x5, [x6]
> > > > @@ -319,7 +327,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, x4 // default T0SZ small enough?
> > > > b.ge 1f // .. then skip VA range extension
> > >
> > > Could we instead have the default value be 48-bit, and then avoid having
> > > to update the variable in both cases? e.g. something along the lines of
> > > the entirely untested diff below?
> > >
> >
> > There is one other occurrence that needs to be updated. I will have a
> > stab at fixing this along these lines, and there are a couple of other
> > things that need cleaning up.
> >
>
> Actually, it seems like this breakage may have been introduced by
>
> commit 1401bef703a48cf79c674215f702a1435362beae
> Author: James Morse <james.morse@arm.com>
> Date: Mon Jan 25 14:19:12 2021 -0500
>
> arm64: mm: Always update TCR_EL1 from __cpu_set_tcr_t0sz()
>
> as before that patch, we would never actually load idmap_t0sz into TCR
> on 52-bit VA kernels, AFAICT.
Right, but even without that patch, the kvm hypervisor code would
use idmap_t0sz for its mapping in cpu_init_hyp_mode(). That leads
to a soft lockup rather than a panic with the above patch.
I can boot on amberwing with the below patch.
>
> It seems to me that the sanest approach is to default to a 48-bit
> idmap on 52-bit VA capable kernels (as Will suggests below). This
> means that the notion of 'extended ID map' now also includes cases
> where the ID map is smaller than the default VA_BITS range. However,
> the number of levels is going to be the same, so it does not affect
> the ID map page table population code in head.S.
>
> Also, __cpu_uses_extended_idmap() no longer has any users anyway, so
> we can remove it rather than bikeshed over a better name for it.
>
> >
> >
> > > --->8
> > >
> > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > > index 66b0e0b66e31..fb795123896f 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;
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled
@ 2021-03-10 14:08 ` Mark Salter
0 siblings, 0 replies; 10+ messages in thread
From: Mark Salter @ 2021-03-10 14:08 UTC (permalink / raw)
To: Ard Biesheuvel, Will Deacon, James Morse
Cc: Linux ARM, Linux Kernel Mailing List, Catalin Marinas, Mark Rutland
On Wed, 2021-03-10 at 14:29 +0100, Ard Biesheuvel wrote:
> (+ James)
>
> On Wed, 10 Mar 2021 at 13:41, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 10 Mar 2021 at 12:18, Will Deacon <will@kernel.org> wrote:
> > >
> > > On Tue, Mar 09, 2021 at 07:32:16PM -0500, Mark Salter wrote:
> > > > I ran into an early boot soft lockup on a Qualcomm Amberwing using a v5.11
> > > > kernel configured for 52-bit VA. This turned into a panic with a v5.12-rc2
> > > > kernel.
> > > >
> > > > The problem is that when we fall back to 48-bit VA, idmap_t0sz is not
> > > > updated. Later, the kvm hypervisor uses idmap_t0sz to set its tcr_el2 and
> > > > hangs (v5.11). After commit 1401bef703a4 ("arm64: mm: Always update TCR_EL1
> > > > from __cpu_set_tcr_t0sz()"), the kernel panics when trying to use the idmap
> > > > to call idmap_cpu_replace_ttbr1().
> > > >
> > > > Oddly, other systems (thunderX2 and Ampere eMag) which don't support 52-bit
> > > > VA seem to handle the setting of an unsupported t0sz without any apparent
> > > > problems. Indeed, if one reads back the tcr written with t0sz==12, the
> > > > value read has t0sz==16. Not so with Amberwing.
> > >
> > > Nice, you have one of those elusive platforms!
> > >
> > > > Fixes: 90ec95cda91a ("arm64: mm: Introduce VA_BITS_MIN")
> > > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > > ---
> > > > arch/arm64/kernel/head.S | 10 +++++++++-
> > > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > > > index 66b0e0b66e31..2bcbbb26292e 100644
> > > > --- a/arch/arm64/kernel/head.S
> > > > +++ b/arch/arm64/kernel/head.S
> > > > @@ -291,6 +291,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > > */
> > > > adrp x0, idmap_pg_dir
> > > > adrp x3, __idmap_text_start // __pa(__idmap_text_start)
> > > > + mov x4, TCR_T0SZ(VA_BITS)
> > > >
> > > > #ifdef CONFIG_ARM64_VA_BITS_52
> > > > mrs_s x6, SYS_ID_AA64MMFR2_EL1
> > > > @@ -299,6 +300,13 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> > > > cbnz x6, 1f
> > > > #endif
> > > > mov x5, #VA_BITS_MIN
> > > > +#ifdef CONFIG_ARM64_VA_BITS_52
> > > > + mov x4, TCR_T0SZ(VA_BITS_MIN)
> > > > + adr_l x6, idmap_t0sz
> > > > + str x4, [x6]
> > > > + dmb sy
> > > > + dc ivac, x6 // Invalidate potentially stale cache line
> > > > +#endif
> > > > 1:
> > > > adr_l x6, vabits_actual
> > > > str x5, [x6]
> > > > @@ -319,7 +327,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, x4 // default T0SZ small enough?
> > > > b.ge 1f // .. then skip VA range extension
> > >
> > > Could we instead have the default value be 48-bit, and then avoid having
> > > to update the variable in both cases? e.g. something along the lines of
> > > the entirely untested diff below?
> > >
> >
> > There is one other occurrence that needs to be updated. I will have a
> > stab at fixing this along these lines, and there are a couple of other
> > things that need cleaning up.
> >
>
> Actually, it seems like this breakage may have been introduced by
>
> commit 1401bef703a48cf79c674215f702a1435362beae
> Author: James Morse <james.morse@arm.com>
> Date: Mon Jan 25 14:19:12 2021 -0500
>
> arm64: mm: Always update TCR_EL1 from __cpu_set_tcr_t0sz()
>
> as before that patch, we would never actually load idmap_t0sz into TCR
> on 52-bit VA kernels, AFAICT.
Right, but even without that patch, the kvm hypervisor code would
use idmap_t0sz for its mapping in cpu_init_hyp_mode(). That leads
to a soft lockup rather than a panic with the above patch.
I can boot on amberwing with the below patch.
>
> It seems to me that the sanest approach is to default to a 48-bit
> idmap on 52-bit VA capable kernels (as Will suggests below). This
> means that the notion of 'extended ID map' now also includes cases
> where the ID map is smaller than the default VA_BITS range. However,
> the number of levels is going to be the same, so it does not affect
> the ID map page table population code in head.S.
>
> Also, __cpu_uses_extended_idmap() no longer has any users anyway, so
> we can remove it rather than bikeshed over a better name for it.
>
> >
> >
> > > --->8
> > >
> > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > > index 66b0e0b66e31..fb795123896f 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;
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-10 14:09 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 0:32 [PATCH] arm64: mm: fix runtime fallback to 48-bt VA when 52-bit VA is enabled Mark Salter
2021-03-10 0:32 ` Mark Salter
2021-03-10 11:16 ` Will Deacon
2021-03-10 11:16 ` Will Deacon
2021-03-10 12:41 ` Ard Biesheuvel
2021-03-10 12:41 ` Ard Biesheuvel
2021-03-10 13:29 ` Ard Biesheuvel
2021-03-10 13:29 ` Ard Biesheuvel
2021-03-10 14:08 ` Mark Salter
2021-03-10 14:08 ` Mark Salter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.