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

[Alexandru Elisei]

Hi Marc,

On Mon, Aug 12, 2024 at 06:58:24PM +0100, Marc Zyngier wrote:
> Hi Alex,
> 
> On Mon, 12 Aug 2024 16:11:02 +0100,
> Alexandru Elisei <alexandru.elisei at 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;

The condition looks good now.

>  		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...

Agreed!

Thanks,
Alex