Re: [PATCH 2/2] KVM: arm64: Fix vcpu_{read,write}_sys_reg() accessors

[Marc Zyngier <maz@kernel.org>]

Hey Oliver,

Thanks for looking into this.

On Tue, 12 Aug 2025 21:23:33 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Sat, Aug 09, 2025 at 03:48:11PM +0100, Marc Zyngier wrote:
> > @@ -144,125 +156,120 @@ static bool get_el2_to_el1_mapping(unsigned int reg,
> >  		MAPPED_EL2_SYSREG(ZCR_EL2,     ZCR_EL1,     NULL	     );
> >  		MAPPED_EL2_SYSREG(CONTEXTIDR_EL2, CONTEXTIDR_EL1, NULL	     );
> >  		MAPPED_EL2_SYSREG(SCTLR2_EL2,  SCTLR2_EL1,  NULL	     );
> > +	case CNTHCTL_EL2:
> > +		/* CNTHCTL_EL2 is super special, until we support NV2.1 */
> > +		loc->loc = ((is_hyp_ctxt(vcpu) && vcpu_el2_e2h_is_set(vcpu)) ?
> > +			    SR_LOC_SPECIAL : SR_LOC_MEMORY);
> > +		break;
> > +	case TPIDR_EL0:
> > +	case TPIDRRO_EL0:
> > +	case TPIDR_EL1:
> > +	case PAR_EL1:
> > +		/* These registers are always loaded, no matter what */
> > +		loc->loc = SR_LOC_LOADED;
> > +		break;
> >  	default:
> > -		return false;
> > +		/*
> > +		 * Non-mapped EL2 registers are by definition in memory, but
> > +		 * we don't need to distinguish them here, as the CPU
> > +		 * register accessors will bail out and we'll end-up using
> > +		 * the backing store.
> > +		 *
> > +		 * EL1 registers are, however, only loaded if we're
> > +		 * not in hypervisor context.
> > +		 */
> > +		loc->loc = is_hyp_ctxt(vcpu) ? SR_LOC_MEMORY : SR_LOC_LOADED;
> 
> Hmm... I get the feeling that this flow is becoming even more subtle.
> There's some implicit coupling between this switch statement and the
> __vcpu_{read,write}_sys_reg_from_cpu() which feels like it could be
> error prone. Especially since we're gonna lose the WARN() that would
> inform us if an on-CPU register was actually redirected to memory.

This implicit behaviour was already present, and nobody noticed it.
See how the FGT2 registers are currently missing from the list of
"pure" registers. We didn't see the problem because the fallback saves
us. This is what decided me to throw away the "pure" annotation and
lump both non-remapped EL2 and EL1 registers together.

> I'm wondering if we need some macro hell containing the block of
> registers we handle on-CPU and expand that can be expanded into this
> triage switch case as well as the sysreg accessor.

That's an interesting approach. A bit tricky, because we do rely on
heavy inlining and constant propagation in the CPU accessors, but
maybe there's a way to deal with that...

> What you have definitely seems correct, though. I'll twiddle a bit and
> see if I come up with something, although I imagine what you have is
> what we'll use in the end anyway.

I'll have a look in parallel and post my findings, if any.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.