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

[Mark Rutland <mark.rutland@arm.com>]

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).

On Mon, Jul 07, 2025 at 11:54:30AM +0200, Tamas Kaman wrote:
> Signed-off-by: Tamas Kaman <tamas.kaman@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.

> +
>  	if (!kernel_is_32bit())
>  		scr |= SCR_EL3_RW;
>  
> -- 
> 2.34.1
>