Re: [PATCH v20 03/14] KVM: arm64: Manage GCS access and registers for guests

[Lorenzo Stoakes (ARM)]

On Thu, Sep 03, 2026 at 09:41:26PM +0100, Mark Brown wrote:
> On Thu, Sep 03, 2026 at 07:13:17PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Tue, Sep 01, 2026 at 10:47:01PM +0100, Mark Brown wrote:
>
> > > GCS introduces a number of system registers, on systems with GCS we need
> > > to context switch them and expose them to VMMs to allow guests to use
> > > GCS.
>
> > > +				ctxt_sys_reg(ctxt, GCSCR_EL1)	= read_sysreg_el1(SYS_GCSCR);
> > > +			}
>
> > I had AI check this (and I think sashiko also hit on) it but this is a chain of:
>
> > 	ctxt_has_tcrx() -> ctxt_has_s1pie() -> ctx_has_gcs()
>
> > Is it correct to make the gcs stuff conditional on tcrx + s1pie + gcs?
>
> There are architectural dependencies which mean it is not valid to
> configure GCS without S1PIE, and S1PIE depends on TCRX.  I had

Ack yeah I think I note that elsewhere, so it reduces only to the 'users doing
weird stuff' case :)

> originally written the code without expressing this dependency but on a
> previous version Marc asked for this nesting as an optimisation.  Since
> it's about optimisastion adding checks that don't otherwise exist on the
> restore path would doubtless get the similar complaints.  Exactly the
> same concerns were raised on v19.

Could you implement the nesting the same in the cases where there is a bare
ctx_has_gcs() as an alternative?

So then it'd always be tcrx -> s1pie -> gcs everywhere and that'd resolve things
also and make things symmetric.

You could also I think express the dependency if it makes sense to.

>
> > And it seems like that feature depends on ctxt_has_tcrx() so _architecturally_
> > fine, but it doesn't seem like KVM enforces the dependency at all and so in
> > theory somebody could KVM_SET_ONE_REG a GCS, !S1PIE configuration.
>
> > And nicer to be consistent everywhere also I think (+ shut sashiko up! :)
>
> FWIW I do agree but I'm not sure how else to implement Marc's feedback
> here.

Ack sure of course.

>
> We could do checking of the ID registers at vCPU creation so we could
> avoid worrying about them in the fast path but there was also feedback
> about not doing that.  One idea I had was to generate feature

Could you possibly check in sanitise_id_aa64pfr1_el1(), something like:

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index fb2c9fd42e24..ba8780745987 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2193,7 +2193,8 @@ static u64 sanitise_id_aa64pfr1_el1(const struct kvm_vcpu *vcpu, u64 val)
 	      SYS_FIELD_GET(ID_AA64PFR0_EL1, RAS, pfr0) == ID_AA64PFR0_EL1_RAS_IMP))
 		val &= ~ID_AA64PFR1_EL1_RAS_frac;

-	if (!system_supports_gcs())
+	if (!system_supports_gcs() || !kvm_has_tcr2(vcpu->kvm) ||
+	    !kvm_has_s1pie(vcpu->kvm))
 		val &= ~ID_AA64PFR1_EL1_GCS;

 	val &= ~ID_AA64PFR1_EL1_SME;
--

And that way it slots in next to the system check?

> combination validation from the MRS, I think the main complaint was
> about open coding things rather than having the validation per se but
> ICBW.  Last I heard we were very near to having code for working with
> the MRS released which will help a lot with uses like that.  There is

Yeah that sounds like a really neat way of solving it! And a useful idea in
general.

> the possibility that people are relying on doing architecturally
> invalid configurations though.

Yeah, I guess it's possible somebody could think they could just set a single
register and use it or something.

>
> > And it seems like it makes it possible for a silly VMM which sets up the
> > registers wrong + some unfortunate guest behaviour -> oops via:
>
> > el1h_64_sync_handler() -> el1_gcs() -> do_el1_gcs()
>
> > Because the host's restore is skipped for s1pie=0, gcs=1, so a naughty guest can
> > set gcsr_el1.pcrsel=1 and some value in gcspr_el1, then the host will get a
> > mismatch and trigger the kernel die().
>
> Yes, that's possible.  GCSCR_EL1.EXLOCKEN would create similar issues.

Yeah :(

--
Cheers, Lorenzo