Re: [BOOT-WRAPPER PATCH 3/3] aarch64: Enable use of GCS for EL2 and below

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

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.