| Message ID | 20241126153955.477569-4-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for FEAT_FPMR and FEAT_GCS | expand |
On Tue, Nov 26, 2024 at 03:39:55PM +0000, Mark Rutland wrote: > + if (mrs_field(ID_AA64PFR1_EL1, GCS)) { > + scr |= SCR_EL3_GCSEn; > + msr(GCSCR_EL2, 0); > + msr(GCSCR_EL1, 0); > + msr(GCSCRE0_EL1, 0); > + } > + We're not doing anything for GCSCR_EL3 where (as for the other ELs) RVCHKEN resets to an architecturally UNKNOWN value: https://developer.arm.com/documentation/ddi0601/2024-09/AArch64-Registers/GCSCR-EL3--Guarded-Control-Stack-Control-Register--EL3-?lang=en This is also an oversight in the TF-A GCS support. One might question the decisions of an implementation which doesn't reset it to 0 but it should be safer to initialise.
On Tue, Nov 26, 2024 at 05:20:47PM +0000, Mark Brown wrote: > On Tue, Nov 26, 2024 at 03:39:55PM +0000, Mark Rutland wrote: > > > + if (mrs_field(ID_AA64PFR1_EL1, GCS)) { > > + scr |= SCR_EL3_GCSEn; > > + msr(GCSCR_EL2, 0); > > + msr(GCSCR_EL1, 0); > > + msr(GCSCRE0_EL1, 0); > > + } > > + > > We're not doing anything for GCSCR_EL3 where (as for the other ELs) > RVCHKEN resets to an architecturally UNKNOWN value: > > https://developer.arm.com/documentation/ddi0601/2024-09/AArch64-Registers/GCSCR-EL3--Guarded-Control-Stack-Control-Register--EL3-?lang=en > > This is also an oversight in the TF-A GCS support. One might question > the decisions of an implementation which doesn't reset it to 0 but it > should be safer to initialise. A need to initialize GCSCR_EL3.RVCHKEN would be an *architecture bug*, not an oversight in TF-A. That bit should only reset to an UNKNOWN value if it doesn't have a functional effect on EL3 code that doesn't touch GCS functionality. If it has no functional effect it's fine to leave it in an UNKNOWN state. GCSCR_EL3.PCRSEL resets to 0, so the HW won't push to the GCS for BL/BLR and won't pop from the GCS for a RET. That menas there's no GCS value for a return value check to compare with... That does raise the question of what specifically happens for a return at EL3 when GCSCR_EL3.{PCRSEL,RVCHKEN} == {1,0}. Can you enlighten me? Mark.
On Tue, Nov 26, 2024 at 06:01:56PM +0000, Mark Rutland wrote: > GCSCR_EL3.PCRSEL resets to 0, so the HW won't push to the GCS for BL/BLR > and won't pop from the GCS for a RET. That menas there's no GCS value > for a return value check to compare with... Oh, good point - I'd trusted that the initialisations were required and misremembered which of PCRSEL and RVCHKEN was which. Sorry about that. The values on reset follow the same pattern in all the GCS control registers down to GCSCRE0_EL1 so the initialisations of the other control registers are equally redundant. We should be consistent here, either initialising all the GCS control registers or relying on the architecture defaults for all of them, and the note in the changelog about them needs an update if the initialisation is there. > That does raise the question of what specifically happens for a return > at EL3 when GCSCR_EL3.{PCRSEL,RVCHKEN} == {1,0}. Can you enlighten me? RET will attempt to load and use a GCS record, the pseudocode is in LoadCheckGCSRecord() which isn't EL dependent other than the selection of which GCSPR and GCSCR to use and setting the access as privileged if we're not at EL0.
On Tue, Nov 26, 2024 at 06:53:03PM +0000, Mark Brown wrote: > On Tue, Nov 26, 2024 at 06:01:56PM +0000, Mark Rutland wrote: > > > GCSCR_EL3.PCRSEL resets to 0, so the HW won't push to the GCS for BL/BLR > > and won't pop from the GCS for a RET. That menas there's no GCS value > > for a return value check to compare with... > > Oh, good point - I'd trusted that the initialisations were required and > misremembered which of PCRSEL and RVCHKEN was which. Sorry about that. > The values on reset follow the same pattern in all the GCS control > registers down to GCSCRE0_EL1 so the initialisations of the other > control registers are equally redundant. We should be consistent here, > either initialising all the GCS control registers or relying on the > architecture defaults for all of them, and the note in the changelog > about them needs an update if the initialisation is there. Hmm... that's somewhat unusual, usually the architecture has a "minimal reset policy", where _ELx register fields are only reset when ELx is the highest EL after the reset, leaving it to SW to initialise lower ELs. It's not clear to me if not following that here was deliberate or an oversight. I can drop those for now. In future the boot-wrapper will probably need to poke those (and many other register fields) to properly handle PSCI CPU_OFF -> CPU_ON sequences, as we currently don't reset anything, and that should really involve a true reset. > > That does raise the question of what specifically happens for a return > > at EL3 when GCSCR_EL3.{PCRSEL,RVCHKEN} == {1,0}. Can you enlighten me? > > RET will attempt to load and use a GCS record, the pseudocode is in > LoadCheckGCSRecord() which isn't EL dependent other than the selection > of which GCSPR and GCSCR to use and setting the access as privileged if > we're not at EL0. Sorry, I got the bits backwards, and had meant the case where: GCSCR_EL3.{PCRSEL,RVCHKEN} == {0,1} ... where I assume the RVCHKEN bit has no effect given we don't have a GCS return value to compare against, but wanted to confirm that there wasn't an architectural edge-case that we might need to feed back to architects. It *seems* like RVCHKEN has no effect for RET when PCRSEL is clear. In ARM DDI 0487K.a, C6.2.291, the pseudocode for RET starts with: | if (IsFeatureImplemented(FEAT_GCS) && GCSPCREnabled(PSTATE.EL)) then | target = LoadCheckGCSRecord(target, GCSInstType_PRET); | SetCurrentGCSPointer(GetCurrentGCSPointer() + 8); ... and RVCHKEN is consumed under LoadCheckGCSRecord(), which has: | if GCSReturnValueCheckEnabled(PSTATE.EL) && (recorded_va != vaddress) then | GCSDataCheckException(gcsinst_type); ... where GCSReturnValueCheckEnabled() is: | boolean GCSReturnValueCheckEnabled(bits(2) el) | if UsingAArch32() then | return FALSE; | case el of | when EL0 return GCSCRE0_EL1.RVCHKEN == '1'; | when EL1 return GCSCR_EL1.RVCHKEN == '1'; | when EL2 return GCSCR_EL2.RVCHKEN == '1'; | when EL3 return GCSCR_EL3.RVCHKEN == '1'; AFAICT GCSReturnValueCheckEnabled() is only used by LoadCheckGCSRecord(), and that's only used by the pseudocode for RET/RETAA/RETAB, so it looks like we're good. Mark.
On Wed, Nov 27, 2024 at 10:25:54AM +0000, Mark Rutland wrote: > On Tue, Nov 26, 2024 at 06:53:03PM +0000, Mark Brown wrote: > > The values on reset follow the same pattern in all the GCS control > > registers down to GCSCRE0_EL1 so the initialisations of the other > > control registers are equally redundant. We should be consistent here, > > either initialising all the GCS control registers or relying on the > > architecture defaults for all of them, and the note in the changelog > > about them needs an update if the initialisation is there. > Hmm... that's somewhat unusual, usually the architecture has a "minimal > reset policy", where _ELx register fields are only reset when ELx is the > highest EL after the reset, leaving it to SW to initialise lower ELs. > It's not clear to me if not following that here was deliberate or an > oversight. Yeah, I remember being a bit surprised by the choice. > > > That does raise the question of what specifically happens for a return > > > at EL3 when GCSCR_EL3.{PCRSEL,RVCHKEN} == {1,0}. Can you enlighten me? > > RET will attempt to load and use a GCS record, the pseudocode is in > > LoadCheckGCSRecord() which isn't EL dependent other than the selection > > of which GCSPR and GCSCR to use and setting the access as privileged if > > we're not at EL0. > Sorry, I got the bits backwards, and had meant the case where: > GCSCR_EL3.{PCRSEL,RVCHKEN} == {0,1} > ... where I assume the RVCHKEN bit has no effect given we don't have a > GCS return value to compare against, but wanted to confirm that there > wasn't an architectural edge-case that we might need to feed back to > architects. Oh, good - I was wondering why you weren't asking that rather than what you asked. > It *seems* like RVCHKEN has no effect for RET when PCRSEL is clear. > In ARM DDI 0487K.a, C6.2.291, the pseudocode for RET starts with: ... > AFAICT GCSReturnValueCheckEnabled() is only used by > LoadCheckGCSRecord(), and that's only used by the pseudocode for > RET/RETAA/RETAB, so it looks like we're good. Yes, we only do a return value check if PCRSEL is active - it is only meaningful as part of a GCS load since it's a comparison of the value we got from the GCS with the value passed into the ret.
diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h index 3ef58f3..4cf0ff7 100644 --- a/arch/aarch64/include/asm/cpu.h +++ b/arch/aarch64/include/asm/cpu.h @@ -62,6 +62,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) @@ -115,6 +116,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 @@ -169,6 +171,10 @@ #define SMCR_EL3_FA64 BIT(31) #define SMCR_EL3_LEN_MAX 0xf +#define GCSCRE0_EL1 s3_0_c2_c5_2 +#define GCSCR_EL1 s3_0_c2_c5_0 +#define GCSCR_EL2 s3_4_c2_c5_0 + #define ID_AA64ISAR2_EL1 s3_0_c0_c6_2 #define ID_AA64MMFR3_EL1 s3_0_c0_c7_3 diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c index 1f38516..61b55f9 100644 --- a/arch/aarch64/init.c +++ b/arch/aarch64/init.c @@ -124,6 +124,13 @@ static void cpu_init_el3(void) if (mrs_field(ID_AA64PFR1_EL1, THE)) scr |= SCR_EL3_RCWMASKEn; + if (mrs_field(ID_AA64PFR1_EL1, GCS)) { + scr |= SCR_EL3_GCSEn; + msr(GCSCR_EL2, 0); + msr(GCSCR_EL1, 0); + msr(GCSCRE0_EL1, 0); + } + if (mrs_field(ID_AA64PFR2_EL1, FPMR)) scr |= SCR_EL3_EnFPM;
FEAT_GCS adds a number of new system registers and instructions. Usage of some of these can trap to EL3 unless SCR_EL3.GCSEn is set, and so boot-wrapper support is necessary. Support for FEAT_GCS was added to Linux in the v6.13-rc1 merge window without any boot-wrapper support. Consequently when GCS is enabled in a model, the kernel will hang when attempting to write to GCS control registers, which happens early in boot when the kernel configures EL2, before any console output is produced. FEAT_GCS is described in the latest ARM ARM (ARM DDI 0487K.a), which can be found at: https://developer.arm.com/documentation/ddi0487/ka/?lang=en Add boot-wrapper support for FEAT_GCS. In addition to setting SCR_EL3.GCSEn, it's necessary to initialize GCSCR_EL2, GCSCR_EL1, and GCSCRE0_EL1 such that older kernel which are not aware of GCS don't find GCS enabled unexpectedly. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Mark Brown <broonie@kernel.org> --- arch/aarch64/include/asm/cpu.h | 6 ++++++ arch/aarch64/init.c | 7 +++++++ 2 files changed, 13 insertions(+)