Re: [PATCH v3 14/18] KVM: arm64: nv: Add SW walker for AT S1 emulation

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Marc,

On Thu, Aug 15, 2024 at 07:28:41PM +0100, Marc Zyngier wrote:
> 
> Hi Alex,
> 
> On Thu, 15 Aug 2024 17:44:02 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> [..]
> 
> > > +static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
> > > +			 struct s1_walk_result *wr, u64 va)
> > > +{
> > > +	u64 sctlr, tcr, tg, ps, ia_bits, ttbr;
> > > +	unsigned int stride, x;
> > > +	bool va55, tbi, lva, as_el0;
> > > +
> > > +	wi->regime = compute_translation_regime(vcpu, op);
> > > +	as_el0 = (op == OP_AT_S1E0R || op == OP_AT_S1E0W);
> > > +
> > > +	va55 = va & BIT(55);
> > > +
> > > +	if (wi->regime == TR_EL2 && va55)
> > > +		goto addrsz;
> > > +
> > > +	wi->s2 = (wi->regime == TR_EL10 &&
> > > +		  (__vcpu_sys_reg(vcpu, HCR_EL2) & (HCR_VM | HCR_DC)));
> > 
> > This could be written on one line if there were a local variable for the HCR_EL2
> > register (which is already read multiple times in the function).
> 
> Sure thing.
> 
> [...]
> 
> > > +	/* Let's put the MMU disabled case aside immediately */
> > > +	switch (wi->regime) {
> > > +	case TR_EL10:
> > > +		/*
> > > +		 * If dealing with the EL1&0 translation regime, 3 things
> > > +		 * can disable the S1 translation:
> > > +		 *
> > > +		 * - HCR_EL2.DC = 1
> > > +		 * - 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;
> > 
> > There's no need to fallthrough and check SCTLR_ELx.M if the MMU disabled check
> > here is true.
> 
> I'm not sure it makes the code more readable. But if you do, why not.
> 
> [...]
> 
> > > +	/* Someone was silly enough to encode TG0/TG1 differently */
> > > +	if (va55) {
> > > +		wi->txsz = FIELD_GET(TCR_T1SZ_MASK, tcr);
> > > +		tg = FIELD_GET(TCR_TG1_MASK, tcr);
> > > +
> > > +		switch (tg << TCR_TG1_SHIFT) {
> > > +		case TCR_TG1_4K:
> > > +			wi->pgshift = 12;	 break;
> > > +		case TCR_TG1_16K:
> > > +			wi->pgshift = 14;	 break;
> > > +		case TCR_TG1_64K:
> > > +		default:	    /* IMPDEF: treat any other value as 64k */
> > > +			wi->pgshift = 16;	 break;
> > > +		}
> > 
> > Just a thought, wi->pgshift is used in several places to identify the guest page
> > size, might be useful to have something like PAGE_SHIFT_{4K,16K,64K}. That would
> > also make its usage consistent, because in some places wi->pgshift is compared
> > directly to 12, 14 or 16, in other places the page size is computed from
> > wi->pgshift and compared to SZ_4K, SZ_16K or SZ_64K.
> 
> I only found a single place where we compare wi->pgshift to a
> non-symbolic integer (as part of the R_YXNYW handling). Everywhere
> else, we use BIT(wi->pgshift) and compare it to SZ_*K. We moved away
> from the various PAGE_SHIFT_* macros some years ago, and I don't think
> we want them back.

Oh, I wasn't aware of that bit of history. No need to change the current code
then, it's readable enough.

> 
> What I can do is to convert the places where we init pgshift to use an
> explicit size using const_ilog2():
> 
> @@ -185,12 +188,12 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
>  
>  		switch (tg << TCR_TG1_SHIFT) {
>  		case TCR_TG1_4K:
> -			wi->pgshift = 12;	 break;
> +			wi->pgshift = const_ilog2(SZ_4K);	 break;
>  		case TCR_TG1_16K:
> -			wi->pgshift = 14;	 break;
> +			wi->pgshift = const_ilog2(SZ_16K);	 break;
>  		case TCR_TG1_64K:
>  		default:	    /* IMPDEF: treat any other value as 64k */
> -			wi->pgshift = 16;	 break;
> +			wi->pgshift = const_ilog2(SZ_64K);	 break;
>  		}
>  	} else {
>  		wi->txsz = FIELD_GET(TCR_T0SZ_MASK, tcr);
> @@ -198,12 +201,12 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
>  
>  		switch (tg << TCR_TG0_SHIFT) {
>  		case TCR_TG0_4K:
> -			wi->pgshift = 12;	 break;
> +			wi->pgshift = const_ilog2(SZ_4K);	 break;
>  		case TCR_TG0_16K:
> -			wi->pgshift = 14;	 break;
> +			wi->pgshift = const_ilog2(SZ_16K);	 break;
>  		case TCR_TG0_64K:
>  		default:	    /* IMPDEF: treat any other value as 64k */
> -			wi->pgshift = 16;	 break;
> +			wi->pgshift = const_ilog2(SZ_64K);	 break;
>  		}
>  	}
>  
> @@ -212,7 +215,7 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
>  		if (wi->txsz > 39)
>  			goto transfault_l0;
>  	} else {
> -		if (wi->txsz > 48 || (wi->pgshift == 16 && wi->txsz > 47))
> +		if (wi->txsz > 48 || (BIT(wi->pgshift) == SZ_64K && wi->txsz > 47))
>  			goto transfault_l0;
>  	}
> 
> 
> > 
> > > +	} else {
> > > +		wi->txsz = FIELD_GET(TCR_T0SZ_MASK, tcr);
> > > +		tg = FIELD_GET(TCR_TG0_MASK, tcr);
> > > +
> > > +		switch (tg << TCR_TG0_SHIFT) {
> > > +		case TCR_TG0_4K:
> > > +			wi->pgshift = 12;	 break;
> > > +		case TCR_TG0_16K:
> > > +			wi->pgshift = 14;	 break;
> > > +		case TCR_TG0_64K:
> > > +		default:	    /* IMPDEF: treat any other value as 64k */
> > > +			wi->pgshift = 16;	 break;
> > > +		}
> > > +	}
> > > +
> > > +	/* R_PLCGL, R_YXNYW */
> > > +	if (!kvm_has_feat_enum(vcpu->kvm, ID_AA64MMFR2_EL1, ST, 48_47)) {
> > > +		if (wi->txsz > 39)
> > > +			goto transfault_l0;
> > > +	} else {
> > > +		if (wi->txsz > 48 || (wi->pgshift == 16 && wi->txsz > 47))
> > > +			goto transfault_l0;
> > > +	}
> > > +
> > > +	/* R_GTJBY, R_SXWGM */
> > > +	switch (BIT(wi->pgshift)) {
> > > +	case SZ_4K:
> > > +		lva = kvm_has_feat(vcpu->kvm, ID_AA64MMFR0_EL1, TGRAN4, 52_BIT);
> > > +		lva &= tcr & (wi->regime == TR_EL2 ? TCR_EL2_DS : TCR_DS);
> > > +		break;
> > > +	case SZ_16K:
> > > +		lva = kvm_has_feat(vcpu->kvm, ID_AA64MMFR0_EL1, TGRAN16, 52_BIT);
> > > +		lva &= tcr & (wi->regime == TR_EL2 ? TCR_EL2_DS : TCR_DS);
> > > +		break;
> > > +	case SZ_64K:
> > > +		lva = kvm_has_feat(vcpu->kvm, ID_AA64MMFR2_EL1, VARange, 52);
> > > +		break;
> > > +	}
> > > +
> > > +	if ((lva && wi->txsz < 12) || wi->txsz < 16)
> > > +		goto transfault_l0;
> > 
> > Let's assume lva = true, wi->txsz greater than 12, but smaller than 16, which is
> > architecturally allowed according to R_GTJBY and AArch64.S1MinTxSZ().
> > 
> > (lva && wi->txsz < 12) = false
> > wi->txsz < 16 = true
> > 
> > KVM treats it as a fault.
> 
> Gah... Fixed with:
> 
> @@ -231,7 +234,7 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
>  		break;
>  	}
>  
> -	if ((lva && wi->txsz < 12) || wi->txsz < 16)
> +	if ((lva && wi->txsz < 12) || (!lva && wi->txsz < 16))
>  		goto transfault_l0;
>  
>  	ia_bits = get_ia_size(wi);
> 
> Not that it has an impact yet, given that we don't support any of the
> 52bit stuff yet, but thanks for spotting this!

The change looks correct to me.

> 
> [...]
> 
> > > +	/* R_VPBBF */
> > > +	if (check_output_size(wi->baddr, wi))
> > > +		goto transfault_l0;
> > 
> > I think R_VPBBF says that an Address size fault is generated here, not a
> > translation fault.
> 
> Indeed, another one fixed.
> 
> > 
> > > +
> > > +	wi->baddr &= GENMASK_ULL(wi->max_oa_bits - 1, x);
> > > +
> > > +	return 0;
> > > +
> > > +addrsz:				/* Address Size Fault level 0 */
> > > +	fail_s1_walk(wr, ESR_ELx_FSC_ADDRSZ_L(0), false, false);
> > > +	return -EFAULT;
> > > +
> > > +transfault_l0:			/* Translation Fault level 0 */
> > > +	fail_s1_walk(wr, ESR_ELx_FSC_FAULT_L(0), false, false);
> > > +	return -EFAULT;
> > > +}
> > 
> > [..]
> > 
> > > +static bool par_check_s1_perm_fault(u64 par)
> > > +{
> > > +	u8 fst = FIELD_GET(SYS_PAR_EL1_FST, par);
> > > +
> > > +	return  ((fst & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_PERM &&
> > > +		 !(par & SYS_PAR_EL1_S));
> > 
> > ESR_ELx_FSC_PERM = 0x0c is a permission fault, level 0, which Arm ARM says can
> > only happen when FEAT_LPA2. I think the code should check that the value for
> > PAR_EL1.FST is in the interval (ESR_ELx_FSC_PERM_L(0), ESR_ELx_FSC_PERM_L(3)].
> 
> I honestly don't want to second-guess the HW. If it reports something
> that is the wrong level, why should we trust the FSC at all?

Sorry, I should have been clearer.

It's not about the hardware reporting a fault on level 0 of the translation
tables, it's about the function returning false if the hardware reports a
permission fault on levels 1, 2 or 3 of the translation tables.

For example, on a permssion fault on level 3, PAR_EL1. FST = 0b001111 = 0x0F,
which means that the condition:

(fst & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_PERM (which is 0x0C) is false and KVM
will fall back to the software walker.

Does that make sense to you?

Thanks,
Alex