[boot-wrapper-aarch64 PATCH] Enable GCS if it is present in the HW

[Tamas Kaman]

Hi Mark
 
>From: Mark Rutland mailto:mark.rutland at arm.com
>Date: Monday, 2025. July 7. at 14:32
>To: Tamas Kaman mailto:Tamas.Kaman at arm.com
>Cc: mailto:linux-arm-kernel at lists.infradead.org mailto:linux-arm-kernel at lists.infradead.org
>Subject: Re: [boot-wrapper-aarch64 PATCH] Enable GCS if it is present in the HW
>Hi Tamas,
> 
>Usually we'd have a commit log here, mentioning and documents referenced
>(e.g. ARM ARM, booting.rst), and any interdependencies (e.g.  other
>features that might need to be enabled first).
 
Sure, I added some more details and reference to ARM ARM
 
> 
>On Mon, Jul 07, 2025 at 11:54:30AM +0200, Tamas Kaman wrote:
>> Signed-off-by: Tamas Kaman mailto:tamas.kaman at arm.com
>> ---
>>  arch/aarch64/include/asm/cpu.h | 5 +++++
>>  arch/aarch64/init.c            | 7 +++++++
>>  2 files changed, 12 insertions(+)
>> 
>> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
>> index ac50474..9d6f0fc 100644
>> --- a/arch/aarch64/include/asm/cpu.h
>> +++ b/arch/aarch64/include/asm/cpu.h
>> @@ -70,6 +70,7 @@
>>  #define SCR_EL3_ECVEN                        BIT(28)
>>  #define SCR_EL3_TME                  BIT(34)
>>  #define SCR_EL3_HXEn                 BIT(38)
>> +#define SCR_EL3_GCSEn                        BIT(39)
>>  #define SCR_EL3_EnTP2                        BIT(41)
>>  #define SCR_EL3_RCWMASKEn            BIT(42)
>>  #define SCR_EL3_TCR2EN                       BIT(43)
>> @@ -125,6 +126,7 @@
>>  #define ID_AA64PFR1_EL1_MTE          BITS(11, 8)
>>  #define ID_AA64PFR1_EL1_SME          BITS(27, 24)
>>  #define ID_AA64PFR1_EL1_CSV2_frac    BITS(35, 32)
>> +#define ID_AA64PFR1_EL1_GCS          BITS(47, 44)
> 
> 
>The two new definitions above look right to me per ARM DDI 0487 L.a
>sections D24.2.163 and D24.2.88.
> 
>>  #define ID_AA64PFR1_EL1_THE          BITS(51, 48)
>>  
>>  #define ID_AA64PFR2_EL1                      s3_0_c0_c4_2
>> @@ -133,6 +135,9 @@
>>  #define ID_AA64SMFR0_EL1             s3_0_c0_c4_5
>>  #define ID_AA64SMFR0_EL1_FA64                BIT(63)
>>  
>> +#define HCRX_EL2                     s3_4_c1_c2_2
>> +#define HCRX_EL2_GCSEn                       BIT(22)
> 
>This looks a bit suspicious, given it wasn't mentioned earlier, and
>given we don't poke this for anythign else so far.
> 
>More on that below.
> 
>> +
>>  /*
>>   * Initial register values required for the boot-wrapper to run out-of-reset.
>>   */
>> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
>> index cb24f4e..f815e6a 100644
>> --- a/arch/aarch64/init.c
>> +++ b/arch/aarch64/init.c
>> @@ -118,6 +118,13 @@ static void cpu_init_el3(void)
>>        if (mrs_field(ID_AA64PFR1_EL1, MTE) >= 2)
>>                scr |= SCR_EL3_ATA;
>>  
>> +     if (mrs_field(ID_AA64PFR1_EL1, GCS)) {
>> +             scr |= SCR_EL3_GCSEn;
>> +             unsigned long hcrx_el2 = mrs(HCRX_EL2);
>> +             hcrx_el2 |= HCRX_EL2_GCSEn;
>> +             msr(HCRX_EL2, hcrx_el2);
>> +     }
> 
>As a general policy, the boot wrapper avoids RMW sequences, and
>initializes all bits in registers to specific values, so this doesn't
>look right.
> 
>We genreally follow a pattern of accumulating control bits with a final
>write, e.g.
> 
>        unsigned long ctrl_x = DEFAULT_VALUE_X;
> 
>        if (featute_exists(a)) {
>                ctrl_x |= CTRL_X_ENABLE_A;
>                ...
>        }
>        ...
>        if (feature_exists(b)) {
>                ctrl_x |= CTRL_X_ENABLE_B;
>                ...
>        }
>        ...
>        msr(ctrl_x_el3, ctrl_x);
> 
>... but for HCRX_EL2, we haven't done that for any other features (e.g.
>FPMR, MOPS). Either that's a latent bug, or  we don't need to initialize
>HCRX_EL2 here.
> 
>Can you describe why you're initializing HCRx_EL2.GCSEn? e.g. are you
>trying to boot a kernel at EL1?
> 
>Thanks,
>Mark.
 
I made this patch a long time ago, when the GCS kernel code was still in development, I just did not upstream the patch at that time.
Now I can’t remember exactly why I added setting that bit in HCRx_EL2 register but I am sure it had its reason.
Anyway, I checked and Android boots without this bit, my tests pass, so I removed that part.
Thanks for spotting this.
 
> 
>> +
>>        if (!kernel_is_32bit())
>>                scr |= SCR_EL3_RW;
>>  
>> -- 
>> 2.34.1
>> 
> 
 
Thanks,
Tamas