* [boot-wrapper-aarch64 PATCH] Enable GCS if it is present in the HW @ 2025-07-07 9:54 Tamas Kaman 2025-07-07 12:32 ` Mark Rutland 0 siblings, 1 reply; 3+ messages in thread From: Tamas Kaman @ 2025-07-07 9:54 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Mark Rutland, Tamas Kaman 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) #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) + /* * 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); + } + if (!kernel_is_32bit()) scr |= SCR_EL3_RW; -- 2.34.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [boot-wrapper-aarch64 PATCH] Enable GCS if it is present in the HW 2025-07-07 9:54 [boot-wrapper-aarch64 PATCH] Enable GCS if it is present in the HW Tamas Kaman @ 2025-07-07 12:32 ` Mark Rutland 2025-07-11 7:28 ` Tamas Kaman 0 siblings, 1 reply; 3+ messages in thread From: Mark Rutland @ 2025-07-07 12:32 UTC (permalink / raw) To: Tamas Kaman; +Cc: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [boot-wrapper-aarch64 PATCH] Enable GCS if it is present in the HW 2025-07-07 12:32 ` Mark Rutland @ 2025-07-11 7:28 ` Tamas Kaman 0 siblings, 0 replies; 3+ messages in thread From: Tamas Kaman @ 2025-07-11 7:28 UTC (permalink / raw) To: Mark Rutland; +Cc: linux-arm-kernel@lists.infradead.org Hi Mark >From: Mark Rutland mailto:mark.rutland@arm.com >Date: Monday, 2025. July 7. at 14:32 >To: Tamas Kaman mailto:Tamas.Kaman@arm.com >Cc: mailto:linux-arm-kernel@lists.infradead.org mailto:linux-arm-kernel@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@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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-07-11 7:48 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-07 9:54 [boot-wrapper-aarch64 PATCH] Enable GCS if it is present in the HW Tamas Kaman 2025-07-07 12:32 ` Mark Rutland 2025-07-11 7:28 ` Tamas Kaman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox