| Message ID | 20240723110630.483871-3-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aarch64: Enable access for FEAT_D128 registers in EL1/EL2 | expand |
On Tue, Jul 23, 2024 at 04:36:29PM +0530, Anshuman Khandual wrote: > FEAT_SCTLR2 adds SCTLR2_EL1 and SCTLR2_EL2 system registers But access into > these register from EL2 and below trap to EL3 unless SCR_EL3.D128En is set. > > Enable access to SCTLR2_ELx registers when they are implemented. > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> I think this should be first in the series, since SCTLR2 can exist without D128, but not vice-versa. > --- > arch/aarch64/include/asm/cpu.h | 4 +++- > arch/aarch64/init.c | 3 +++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h > index 0b8b463..57d66e4 100644 > --- a/arch/aarch64/include/asm/cpu.h > +++ b/arch/aarch64/include/asm/cpu.h > @@ -56,6 +56,7 @@ > #define SCR_EL3_HXEn BIT(38) > #define SCR_EL3_EnTP2 BIT(41) > #define SCR_EL3_TCR2EN BIT(43) > +#define SCR_EL3_SCTLR2En BIT(44) > #define SCR_EL3_PIEN BIT(45) > #define SCR_EL3_D128En BIT(47) > > @@ -81,7 +82,8 @@ > > #define ID_AA64MMFR1_EL1_HCX BITS(43, 40) > > -#define ID_AA64MMFR3_EL1_TCRX BITS(4, 0) > +#define ID_AA64MMFR3_EL1_TCRX BITS(3, 0) > +#define ID_AA64MMFR3_EL1_SCTLRX BITS(7, 4) > #define ID_AA64MMFR3_EL1_S1PIE BITS(11, 8) > #define ID_AA64MMFR3_EL1_S2PIE BITS(15, 12) > #define ID_AA64MMFR3_EL1_S1POE BITS(19, 16) > diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c > index 7d9d0d9..5b21cb8 100644 > --- a/arch/aarch64/init.c > +++ b/arch/aarch64/init.c > @@ -92,6 +92,9 @@ void cpu_init_el3(void) > if (mrs_field(ID_AA64MMFR3_EL1, D128)) > scr |= SCR_EL3_D128En; > > + if (mrs_field(ID_AA64MMFR3_EL1, SCTLRX)) > + scr |= SCR_EL3_SCTLR2En; > + The SCTLR2_ELx registers reset to UNKNOWN values when the highest implemented exception level is not ELx, so we need to initialize those to safe values. Otherwise a kernel which is not aware of SCTLR2_ELx will be subject to arbitrary behaviour as a result of the SCTLR2_ELx bits which it will not have configured. I know that we've failed to do that for other things (FGT and HCRX), and those are latent bugs / mistakes in our appraoch that I'll see about fixing. Mark.
On 7/25/24 14:10, Mark Rutland wrote: > On Tue, Jul 23, 2024 at 04:36:29PM +0530, Anshuman Khandual wrote: >> FEAT_SCTLR2 adds SCTLR2_EL1 and SCTLR2_EL2 system registers But access into >> these register from EL2 and below trap to EL3 unless SCR_EL3.D128En is set. Will fix a small nit here, s/SCR_EL3.D128En/SCR_EL3.SCTLR2En/ ^^^ >> >> Enable access to SCTLR2_ELx registers when they are implemented. >> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > > I think this should be first in the series, since SCTLR2 can exist > without D128, but not vice-versa. Sure, will move it as the first patch in the series. > >> --- >> arch/aarch64/include/asm/cpu.h | 4 +++- >> arch/aarch64/init.c | 3 +++ >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h >> index 0b8b463..57d66e4 100644 >> --- a/arch/aarch64/include/asm/cpu.h >> +++ b/arch/aarch64/include/asm/cpu.h >> @@ -56,6 +56,7 @@ >> #define SCR_EL3_HXEn BIT(38) >> #define SCR_EL3_EnTP2 BIT(41) >> #define SCR_EL3_TCR2EN BIT(43) >> +#define SCR_EL3_SCTLR2En BIT(44) >> #define SCR_EL3_PIEN BIT(45) >> #define SCR_EL3_D128En BIT(47) >> >> @@ -81,7 +82,8 @@ >> >> #define ID_AA64MMFR1_EL1_HCX BITS(43, 40) >> >> -#define ID_AA64MMFR3_EL1_TCRX BITS(4, 0) >> +#define ID_AA64MMFR3_EL1_TCRX BITS(3, 0) >> +#define ID_AA64MMFR3_EL1_SCTLRX BITS(7, 4) >> #define ID_AA64MMFR3_EL1_S1PIE BITS(11, 8) >> #define ID_AA64MMFR3_EL1_S2PIE BITS(15, 12) >> #define ID_AA64MMFR3_EL1_S1POE BITS(19, 16) >> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c >> index 7d9d0d9..5b21cb8 100644 >> --- a/arch/aarch64/init.c >> +++ b/arch/aarch64/init.c >> @@ -92,6 +92,9 @@ void cpu_init_el3(void) >> if (mrs_field(ID_AA64MMFR3_EL1, D128)) >> scr |= SCR_EL3_D128En; >> >> + if (mrs_field(ID_AA64MMFR3_EL1, SCTLRX)) >> + scr |= SCR_EL3_SCTLR2En; >> + > > The SCTLR2_ELx registers reset to UNKNOWN values when the highest > implemented exception level is not ELx, so we need to initialize those > to safe values. Otherwise a kernel which is not aware of SCTLR2_ELx will > be subject to arbitrary behaviour as a result of the SCTLR2_ELx bits > which it will not have configured. Both SCTLR2_EL1 and SCTLR2_EL2 has the same register fields layout except the very last bit i.e SCTLR2_EL2.EMEC which is available in SCTLR2_EL2 but not in SCTLR2_EL1. AFAICT all the above register fields are applicable for newer arch features which the current kernel is not even aware about. So even if the kernel is not ware about SCTLR2_EL2 or SCTLR2_EL1 registers, there will not be any difference in behaviour related to these new arch features. Search for the registers in the current mainline kernel. $git grep SCTLR2_EL arch/arm64/include/asm/sysreg.h:#define SYS_SCTLR2_EL2 sys_reg(3, 4, 1, 0, 3) arch/arm64/include/asm/sysreg.h:#define SYS_SCTLR2_EL12 sys_reg(3, 5, 1, 0, 3) arch/arm64/kvm/emulate-nested.c: SR_TRAP(SYS_SCTLR2_EL2, CGT_HCR_NV), $git grep SCTLR2En arch/arm64/kvm/nested.c: res0 |= HCRX_EL2_SCTLR2En; arch/arm64/tools/sysreg:Field 15 SCTLR2En Although if we are looking for safer values, guess resetting these two registers might be sufficient here ? + if (mrs_field(ID_AA64MMFR3_EL1, SCTLRX)) { + scr |= SCR_EL3_SCTLR2En; + msr(SCTLR2_EL2, 0); + msr(SCTLR2_EL1, 0); + } + > > I know that we've failed to do that for other things (FGT and HCRX), and > those are latent bugs / mistakes in our appraoch that I'll see about > fixing. Sure.
On Fri, Jul 26, 2024 at 12:25:14PM +0530, Anshuman Khandual wrote: > On 7/25/24 14:10, Mark Rutland wrote: > > On Tue, Jul 23, 2024 at 04:36:29PM +0530, Anshuman Khandual wrote: > >> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c > >> index 7d9d0d9..5b21cb8 100644 > >> --- a/arch/aarch64/init.c > >> +++ b/arch/aarch64/init.c > >> @@ -92,6 +92,9 @@ void cpu_init_el3(void) > >> if (mrs_field(ID_AA64MMFR3_EL1, D128)) > >> scr |= SCR_EL3_D128En; > >> > >> + if (mrs_field(ID_AA64MMFR3_EL1, SCTLRX)) > >> + scr |= SCR_EL3_SCTLR2En; > >> + > > > > The SCTLR2_ELx registers reset to UNKNOWN values when the highest > > implemented exception level is not ELx, so we need to initialize those > > to safe values. Otherwise a kernel which is not aware of SCTLR2_ELx will > > be subject to arbitrary behaviour as a result of the SCTLR2_ELx bits > > which it will not have configured. > > Both SCTLR2_EL1 and SCTLR2_EL2 has the same register fields layout > except the very last bit i.e SCTLR2_EL2.EMEC which is available in > SCTLR2_EL2 but not in SCTLR2_EL1. > > AFAICT all the above register fields are applicable for newer arch > features which the current kernel is not even aware about. So even > if the kernel is not ware about SCTLR2_EL2 or SCTLR2_EL1 registers, > there will not be any difference in behaviour related to these new > arch features. There several are changes to existing behaviours. Looking at ARM DDI 0487K.a: * EASE changes the way external aborts are routed, which could surprise the exception handling code. * NMEA causes SError to be taken regardless of PSTATE.A. This *will* break exception handling. ... and regardless we have no idea how any of the RES0 bits will be used in future. Looking at DDI 0601 ID070124 from: https://developer.arm.com/documentation/ddi0601/2024-06/?lang=en ... there are other bits that would be problematic too. Consider how EnPACM0 works with a kernel that is not PACM-aware but a userspace that is, especially if CPUs have mismatched reset values. > Search for the registers in the current mainline kernel. > > $git grep SCTLR2_EL > > arch/arm64/include/asm/sysreg.h:#define SYS_SCTLR2_EL2 sys_reg(3, 4, 1, 0, 3) > arch/arm64/include/asm/sysreg.h:#define SYS_SCTLR2_EL12 sys_reg(3, 5, 1, 0, 3) > arch/arm64/kvm/emulate-nested.c: SR_TRAP(SYS_SCTLR2_EL2, CGT_HCR_NV), > > $git grep SCTLR2En > arch/arm64/kvm/nested.c: res0 |= HCRX_EL2_SCTLR2En; > arch/arm64/tools/sysreg:Field 15 SCTLR2En > > Although if we are looking for safer values, guess resetting these > two registers might be sufficient here ? > > + if (mrs_field(ID_AA64MMFR3_EL1, SCTLRX)) { > + scr |= SCR_EL3_SCTLR2En; > + msr(SCTLR2_EL2, 0); > + msr(SCTLR2_EL1, 0); > + } Using zero for both looks fine to me. Mark.
diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h index 0b8b463..57d66e4 100644 --- a/arch/aarch64/include/asm/cpu.h +++ b/arch/aarch64/include/asm/cpu.h @@ -56,6 +56,7 @@ #define SCR_EL3_HXEn BIT(38) #define SCR_EL3_EnTP2 BIT(41) #define SCR_EL3_TCR2EN BIT(43) +#define SCR_EL3_SCTLR2En BIT(44) #define SCR_EL3_PIEN BIT(45) #define SCR_EL3_D128En BIT(47) @@ -81,7 +82,8 @@ #define ID_AA64MMFR1_EL1_HCX BITS(43, 40) -#define ID_AA64MMFR3_EL1_TCRX BITS(4, 0) +#define ID_AA64MMFR3_EL1_TCRX BITS(3, 0) +#define ID_AA64MMFR3_EL1_SCTLRX BITS(7, 4) #define ID_AA64MMFR3_EL1_S1PIE BITS(11, 8) #define ID_AA64MMFR3_EL1_S2PIE BITS(15, 12) #define ID_AA64MMFR3_EL1_S1POE BITS(19, 16) diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c index 7d9d0d9..5b21cb8 100644 --- a/arch/aarch64/init.c +++ b/arch/aarch64/init.c @@ -92,6 +92,9 @@ void cpu_init_el3(void) if (mrs_field(ID_AA64MMFR3_EL1, D128)) scr |= SCR_EL3_D128En; + if (mrs_field(ID_AA64MMFR3_EL1, SCTLRX)) + scr |= SCR_EL3_SCTLR2En; + msr(SCR_EL3, scr); msr(CPTR_EL3, cptr);
FEAT_SCTLR2 adds SCTLR2_EL1 and SCTLR2_EL2 system registers But access into these register from EL2 and below trap to EL3 unless SCR_EL3.D128En is set. Enable access to SCTLR2_ELx registers when they are implemented. Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/aarch64/include/asm/cpu.h | 4 +++- arch/aarch64/init.c | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-)