Re: [boot-wrapper 2/3] aarch64: Enable access into SCTLR2_ELx registers from EL2 and below

[Anshuman Khandual <anshuman.khandual@arm.com>]

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.