Re: [PATCH v2 13/17] KVM: arm64: nv: Add SW walker for AT S1 emulation

[Marc Zyngier <maz@kernel.org>]

Hi Alex,

On Mon, 12 Aug 2024 16:11:02 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On Sat, Aug 10, 2024 at 11:16:15AM +0100, Marc Zyngier wrote:
> > Hi Alex,
> > 
> > @@ -136,12 +137,22 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
> >  	va = (u64)sign_extend64(va, 55);
> >  
> >  	/* Let's put the MMU disabled case aside immediately */
> > -	if (!(sctlr & SCTLR_ELx_M) ||
> > -	    (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_DC)) {
> > +	switch (wi->regime) {
> > +	case TR_EL10:
> > +		if (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_DC)
> > +			wr->level = S1_MMU_DISABLED;
> 
> In compute_translation_regime(), for AT instructions other than AT S1E2*, when
> {E2H,TGE} = {0,1}, regime is Regime_EL10. As far as I can tell, when regime is
> Regime_EL10 and TGE is set, stage 1 is disabled, according to
> AArch64.S1Enabled() and the decription of the TGE bit.

Grmbl... I really dislike E2H=0. May it die a painful death. How about
this on top?

diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index 10017d990bc3..870e77266f80 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -139,7 +139,19 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
 	/* Let's put the MMU disabled case aside immediately */
 	switch (wi->regime) {
 	case TR_EL10:
-		if (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_DC)
+		/*
+		 * If dealing with the EL1&0 translation regime, 3 things
+		 * can disable the S1 translation:
+		 *
+		 * - HCR_EL2.DC = 0
+		 * - HCR_EL2.{E2H,TGE} = {0,1}
+		 * - SCTLR_EL1.M = 0
+		 *
+		 * The TGE part is interesting. If we have decided that this
+		 * is EL1&0, then it means that either {E2H,TGE} == {1,0} or
+		 * {0,x}, and we only need to test for TGE == 1.
+		 */
+		if (__vcpu_sys_reg(vcpu, HCR_EL2) & (HCR_DC | HCR_TGE))
 			wr->level = S1_MMU_DISABLED;
 		fallthrough;
 	case TR_EL2:

[...]

>
> 	switch (desc & GENMASK_ULL(1, 0)) {
> 	case 0b00:
> 	case 0b10:
> 		goto transfault;
> 	case 0b01:
> 		/* Block mapping */
> 		break;
> 	default:
> 		if (level == 3)
> 			break;
> 	}
> 
> Is this better? Perhaps slightly easier to match against the descriptor layouts,
> but I'm not sure it's an improvement over your suggestion. Up to you, no point
> in bikeshedding over it.

I think I'll leave it as is for now. I'm getting sick of this code...

Thanks,

	M.

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