Re: [PATCH v2 10/16] KVM: arm64: Allow use of S1 PTW for non-NV vcpus

[Marc Zyngier <maz@kernel.org>]

On Fri, 19 Sep 2025 23:27:52 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Mon, Sep 15, 2025 at 12:44:45PM +0100, Marc Zyngier wrote:
> > As we are about to use the S1 PTW in non-NV contexts, we must make
> > sure that we don't evaluate the EL2 state when dealing with the EL1&0
> > translation regime.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/at.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> > index 1230907d0aa0a..4f6686f59d1c4 100644
> > --- a/arch/arm64/kvm/at.c
> > +++ b/arch/arm64/kvm/at.c
> > @@ -108,8 +108,9 @@ static bool s1pie_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime)
> >  	case TR_EL20:
> >  		return vcpu_read_sys_reg(vcpu, TCR2_EL2) & TCR2_EL2_PIE;
> >  	case TR_EL10:
> > -		return  (__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_TCR2En) &&
> > -			(__vcpu_sys_reg(vcpu, TCR2_EL1) & TCR2_EL1_PIE);
> > +		return ((!vcpu_has_nv(vcpu) ||
> > +			 (__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_TCR2En)) &&
> > +			(__vcpu_sys_reg(vcpu, TCR2_EL1) & TCR2_EL1_PIE));
> 
> Hmm, dealing with the effectiveness of bits gated by HCRX_EL2.xEN is a
> pain. Rather than open-coding this everywhere:
> 
> static bool __effective_tcr2_bit(struct kvm_vcpu *vcpu, enum trans_regime regime,
> 				 unsigned int idx)
> {
> 	bool bit;
> 
> 	if (tr != TR_EL10)
> 		return vcpu_read_sys_reg(vcpu, TCR2_EL2) & BIT(idx);
> 
> 	bit = __vcpu_read_sys_reg(vcpu, TCR2_EL1) & BIT(idx);
> 	if (vcpu_has_nv(vcpu))
> 		bit &= (__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_TCR2En);
> 
> 	return bit;
> }
> 
> static bool s1pie_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime)
> {
> 	return __effective_tcr2_bit(vcpu, regime, TCR2_EL1_PIE_SHIFT);
> }
> 
> static void compute_s1poe(struct kvm_vcpu *vcpu, struct s1_walk_info *wi)
> {
> 	if (!kvm_has_s1poe(vcpu->kvm)) {
> 		wi->poe = wi->e0poe = false;
> 		return;
> 	}
> 
> 	wi->poe = __effective_tcr2_bit(vcpu, wi->regime, TCR2_EL1_POE_SHIFT);
> 	if (wi->regime != TR_EL2)
> 		wi->poe = __effective_tcr2_bit(vcpu, wi->regime, TCR2_EL1_E0POE_SHIFT);
> }
> 
> Thoughts?

I quite like the idea, except for passing individual bit numbers to
the helper (I'd rather get the full value or 0, depending on TCR2En).
Based on this, I ended up with this:

static u64 effective_tcr2(struct kvm_vcpu *vcpu, enum trans_regime regime)
{
	if (regime == TR_EL10) {
		if (vcpu_has_nv(vcpu) &&
		    !(__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_TCR2En))
			return 0;

		return __vcpu_read_sys_reg(vcpu, TCR2_EL1);
	}

	return vcpu_read_sys_reg(vcpu, TCR2_EL2);
}

static bool s1pie_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime)
{
	if (!kvm_has_s1pie(vcpu->kvm))
		return false;

	/* Abuse TCR2_EL1_PIE and use it for EL2 as well */
	return effective_tcr2(vcpu, regime) & TCR2_EL1_PIE;
}

static void compute_s1poe(struct kvm_vcpu *vcpu, struct s1_walk_info *wi)
{
	u64 val;

	if (!kvm_has_s1poe(vcpu->kvm)) {
		wi->poe = wi->e0poe = false;
		return;
	}

	val = effective_tcr2(vcpu, wi->regime);

	/* Abuse TCR2_EL1_* for EL2 */
	wi->poe = val & TCR2_EL1_POE;
	wi->e0poe = (wi->regime != TR_EL2) && (val & TCR2_EL1_E0POE);
}

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.