Re: [bootwrapper PATCH 2/2] aarch64: Disable trapping into EL3 while accessing FEAT_FGT2 registers

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

On 4/18/24 16:10, Mark Rutland wrote:
> On Thu, Apr 04, 2024 at 01:07:26PM +0530, Anshuman Khandual wrote:
>> This disables trapping into EL3 while accessing Fine Grained Traps Enable 2
>> (i.e FEAT_FGT2) registers such as HDFGRTR2_EL2, HDFGWTR2_EL2, HFGITR2_EL2,
>> HFGRTR2_EL2 and HFGWTR2_EL2 via setting SCR_EL3.FGTEN2. But first ensure
>> that FEAT_FGT2 feature is implemented looking into ID_AA64MMFR0_EL1.
> 
> I think for clarity that should be:
> 
> | aarch64: Enable access to FGT2 registers from EL2 and below
> | 
> | FEAT_FGT2 adds a number of registers: HFGITR2_EL2, HFGRTR2_EL2,
> | HFGWTR_EL2, HDFGRTR2_EL2, and HDFGWTR2_EL2. Acceses to these from EL2
> | trap to EL3 unless SCR_EL3.FGTEN2 is set.
> | 
> | Enable access to the FGT2 registers when they are implemented.
> 
> However, I note that those registers have fields that reset to UNKNOWN values

Are you referring to the following rules in the register descriptions ?


The reset behavior of this field is:

On a Warm reset:
— When the highest implemented Exception level is EL2, this field resets to 0 .
— Otherwise, this field resets to an architecturally UNKNOWN value. <========

> when resets are taken to EL3. That means we must initialise those to sane
> values to avoid breaking existing EL2 code with unexpected traps.

FEAT_FGT2 is a new feature, so are its corresponding registers as listed
above. Hence I guess resetting them all to zero should be fine ?

        if (mrs_field(ID_AA64MMFR0_EL1, FGT) >= 2) {
                scr |= SCR_EL3_FGTEN2;
                msr(HDFGRTR2_EL2, 0);
                msr(HDFGWTR2_EL2, 0);
                msr(HFGITR2_EL2, 0);
                msr(HFGRTR2_EL2, 0);
                msr(HFGWTR2_EL2, 0);
        }
> 
> I know that we didn't do that for FGT, which really was a mistake.
> 
> I reckon we should reset all of the FGT regsiters to sane values (i.e. existing
> features shouldn't trap, new features should trap). We'll also need to ensure
> that Trusted Firmware does likewise...

Sure, will audit for existing FEAT_FGT register reset values depending on what
is currently supported in the kernel and follow up on with TFA changes as well.

> 
> Mark.
> 
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/aarch64/include/asm/cpu.h | 1 +
>>  arch/aarch64/init.c            | 3 +++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
>> index 124ef91..56f319a 100644
>> --- a/arch/aarch64/include/asm/cpu.h
>> +++ b/arch/aarch64/include/asm/cpu.h
>> @@ -57,6 +57,7 @@
>>  #define SCR_EL3_EnTP2			BIT(41)
>>  #define SCR_EL3_TCR2EN			BIT(43)
>>  #define SCR_EL3_PIEN			BIT(45)
>> +#define SCR_EL3_FGTEN2			BIT(59)
>>  
>>  #define HCR_EL2_RES1			BIT(1)
>>  
>> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
>> index 37cb45f..557266b 100644
>> --- a/arch/aarch64/init.c
>> +++ b/arch/aarch64/init.c
>> @@ -68,6 +68,9 @@ void cpu_init_el3(void)
>>  	if (mrs_field(ID_AA64MMFR0_EL1, FGT))
>>  		scr |= SCR_EL3_FGTEN;
>>  
>> +	if (mrs_field(ID_AA64MMFR0_EL1, FGT) >= 2)
>> +		scr |= SCR_EL3_FGTEN2;
>> +
>>  	if (mrs_field(ID_AA64MMFR0_EL1, ECV) >= 2)
>>  		scr |= SCR_EL3_ECVEN;
>>  
>> -- 
>> 2.25.1
>>

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