Re: [PATCH v5 21/37] KVM: arm64: Implement AT S1PIE support

[Marc Zyngier <maz@kernel.org>]

On Thu, 24 Oct 2024 14:59:25 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Wed, Oct 23, 2024 at 03:53:29PM +0100, Marc Zyngier wrote:
> > It doesn't take much effort to implement S1PIE support in AT.
> > 
> > It is only a matter of using the AArch64.S1IndirectBasePermissions()
> > encodings for the permission, ignoring GCS which has no impact on AT,
> > and enforce FEAT_PAN3 being enabled as this is a requirement of
> > FEAT_S1PIE.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/at.c | 117 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 116 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> > index f5bd750288ff5..3d93ed1795603 100644
> > --- a/arch/arm64/kvm/at.c
> > +++ b/arch/arm64/kvm/at.c
> > @@ -781,6 +781,9 @@ static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime)
> >  	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, PAN, PAN3))
> >  		return false;
> >  
> > +	if (s1pie_enabled(vcpu, regime))
> > +		return true;
> > +
> >  	if (regime == TR_EL10)
> >  		sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> >  	else
> > @@ -862,11 +865,123 @@ static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu,
> >  	}
> >  }
> >  
> > +#define perm_idx(v, r, i)	((vcpu_read_sys_reg((v), (r)) >> ((i) * 4)) & 0xf)
> > +
> > +#define set_priv_perms(wr, r, w, x)	\
> > +	do {				\
> > +		(wr)->pr = (r);		\
> > +		(wr)->pw = (w);		\
> > +		(wr)->px = (x);		\
> > +	} while (0)
> > +
> > +#define set_unpriv_perms(wr, r, w, x)	\
> > +	do {				\
> > +		(wr)->ur = (r);		\
> > +		(wr)->uw = (w);		\
> > +		(wr)->ux = (x);		\
> > +	} while (0)
> > +
> > +/* Similar to AArch64.S1IndirectBasePermissions(), without GCS  */
> > +#define set_perms(w, wr, ip)						\
> > +	do {								\
> > +		/* R_LLZDZ */						\
> > +		switch ((ip)) {						\
> > +		case 0b0000:						\
> > +			set_ ## w ## _perms((wr), false, false, false);	\
> > +			break;						\
> > +		case 0b0001:						\
> > +			set_ ## w ## _perms((wr), true , false, false);	\
> > +			break;						\
> > +		case 0b0010:						\
> > +			set_ ## w ## _perms((wr), false, false, true );	\
> > +			break;						\
> > +		case 0b0011:						\
> > +			set_ ## w ## _perms((wr), true , false, true );	\
> > +			break;						\
> > +		case 0b0100:						\
> > +			set_ ## w ## _perms((wr), false, false, false);	\
> > +			break;						\
> > +		case 0b0101:						\
> > +			set_ ## w ## _perms((wr), true , true , false);	\
> > +			break;						\
> > +		case 0b0110:						\
> > +			set_ ## w ## _perms((wr), true , true , true );	\
> > +			break;						\
> > +		case 0b0111:						\
> > +			set_ ## w ## _perms((wr), true , true , true );	\
> > +			break;						\
> > +		case 0b1000:						\
> > +			set_ ## w ## _perms((wr), true , false, false);	\
> > +			break;						\
> > +		case 0b1001:						\
> > +			set_ ## w ## _perms((wr), true , false, false);	\
> > +			break;						\
> > +		case 0b1010:						\
> > +			set_ ## w ## _perms((wr), true , false, true );	\
> > +			break;						\
> > +		case 0b1011:						\
> > +			set_ ## w ## _perms((wr), false, false, false);	\
> > +			break;						\
> > +		case 0b1100:						\
> > +			set_ ## w ## _perms((wr), true , true , false);	\
> > +			break;						\
> > +		case 0b1101:						\
> > +			set_ ## w ## _perms((wr), false, false, false);	\
> > +			break;						\
> > +		case 0b1110:						\
> > +			set_ ## w ## _perms((wr), true , true , true );	\
> > +			break;						\
> > +		case 0b1111:						\
> > +			set_ ## w ## _perms((wr), false, false, false);	\
> > +			break;						\
> > +		}							\
> > +	} while (0)
> > +
> > +static void compute_s1_indirect_permissions(struct kvm_vcpu *vcpu,
> > +					    struct s1_walk_info *wi,
> > +					    struct s1_walk_result *wr)
> > +{
> > +	u8 up, pp, idx;
> > +
> > +	idx = pte_pi_index(wr->desc);
> > +
> > +	switch (wi->regime) {
> > +	case TR_EL10:
> > +		pp = perm_idx(vcpu, PIR_EL1, idx);
> > +		up = perm_idx(vcpu, PIRE0_EL1, idx);
> > +		break;
> > +	case TR_EL20:
> > +		pp = perm_idx(vcpu, PIR_EL2, idx);
> > +		up = perm_idx(vcpu, PIRE0_EL2, idx);
> > +		break;
> > +	case TR_EL2:
> > +		pp = perm_idx(vcpu, PIR_EL2, idx);
> > +		up = 0;
> > +		break;
> > +	}
> 
> There seems to be inconsistent use of
> 
> default:
> 	BUG();
> 
> when switching on wi->regime.

True. Maybe I should drop them all apart from the one in
setup_s1_walk().

> 
> > +
> > +	set_perms(priv, wr, pp);
> > +
> > +	if (wi->regime != TR_EL2)
> > +		set_perms(unpriv, wr, up);
> > +	else
> > +		set_unpriv_perms(wr, false, false, false);
> 
> When regime == TR_EL2, up == 0, so the if/else should do the same thing? Maybe
> you've done that intentionally to be more explicit.

The reason for doing so was not to give the impression that we were
actively using the unprivileged indirect permissions for TR_EL2.

But maybe that's be just as clear with a comment.

> 
> Either way:
> 
> Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thanks!

	M.

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