Re: [PATCH v2 12/16] KVM: arm64: Implement AT S1PIE support

[Joey Gouly <joey.gouly@arm.com>]

Hello Marc!

On Tue, Sep 03, 2024 at 04:38:30PM +0100, Marc Zyngier wrote:
> It doesn't take much effort to imple,emt S1PIE support in AT.
> This 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 | 136 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 135 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 68f5b89598ec..bd7e1b32b049 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -736,6 +736,23 @@ static u64 compute_par_s1(struct kvm_vcpu *vcpu, struct s1_walk_result *wr,
>  	return par;
>  }
>  
> +static bool s1pie_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime)
> +{
> +	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
> +		return false;
> +
> +	switch (regime) {
> +	case TR_EL2:
> +	case TR_EL20:
> +		return __vcpu_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_EL1x_PIE);
> +	default:
> +		BUG();
> +	}
> +}
> +
>  static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime)
>  {
>  	u64 sctlr;
> @@ -743,6 +760,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
> @@ -826,12 +846,126 @@ static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +#define pi_idx(v, r, i)	((__vcpu_sys_reg((v), (r)) >> ((i) * 4)) & 0xf)
> +
> +#define set_priv_perms(p, r, w, x)	\
> +	do {				\
> +		(p)->pr = (r);		\
> +		(p)->pw = (w);		\
> +		(p)->px = (x);		\
> +	} while (0)
> +
> +#define set_unpriv_perms(p, r, w, x)	\
> +	do {				\
> +		(p)->ur = (r);		\
> +		(p)->uw = (w);		\
> +		(p)->ux = (x);		\
> +	} while (0)
> +
> +/* Similar to AArch64.S1IndirectBasePermissions(), without GCS  */
> +#define set_perms(w, p, ip)						\
> +	do {								\
> +		switch ((ip)) {						\
> +		case 0b0000:						\
> +			set_ ## w ## _perms((p), false, false, false);	\
> +			break;						\
> +		case 0b0001:						\
> +			set_ ## w ## _perms((p), true , false, false);	\
> +			break;						\
> +		case 0b0010:						\
> +			set_ ## w ## _perms((p), false, false, true );	\
> +			break;						\
> +		case 0b0011:						\
> +			set_ ## w ## _perms((p), true , false, true );	\
> +			break;						\
> +		case 0b0100:						\
> +			set_ ## w ## _perms((p), false, false, false);	\
> +			break;						\
> +		case 0b0101:						\
> +			set_ ## w ## _perms((p), true , true , false);	\
> +			break;						\
> +		case 0b0110:						\
> +			set_ ## w ## _perms((p), true , true , true );	\
> +			break;						\
> +		case 0b0111:						\
> +			set_ ## w ## _perms((p), true , true , true );	\
> +			break;						\
> +		case 0b1000:						\
> +			set_ ## w ## _perms((p), true , false, false);	\
> +			break;						\
> +		case 0b1001:						\
> +			set_ ## w ## _perms((p), true , false, false);	\
> +			break;						\
> +		case 0b1010:						\
> +			set_ ## w ## _perms((p), true , false, true );	\
> +			break;						\
> +		case 0b1011:						\
> +			set_ ## w ## _perms((p), false, false, false);	\
> +			break;						\
> +		case 0b1100:						\
> +			set_ ## w ## _perms((p), true , true , false);	\
> +			break;						\
> +		case 0b1101:						\
> +			set_ ## w ## _perms((p), false, false, false);	\
> +			break;						\
> +		case 0b1110:						\
> +			set_ ## w ## _perms((p), true , true , true );	\
> +			break;						\
> +		case 0b1111:						\
> +			set_ ## w ## _perms((p), 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,
> +					    struct s1_perms *s1p)
> +{
> +	u8 up, pp, idx;
> +
> +	idx = (FIELD_GET(GENMASK(54, 53), wr->desc) << 2	|
> +	       FIELD_GET(BIT(51), wr->desc) << 1		|
> +	       FIELD_GET(BIT(6), wr->desc));
> +
> +	switch (wi->regime) {
> +	case TR_EL10:
> +		pp = pi_idx(vcpu, PIR_EL1, idx);
> +		up = pi_idx(vcpu, PIRE0_EL1, idx);
> +		break;
> +	case TR_EL20:
> +		pp = pi_idx(vcpu, PIR_EL2, idx);
> +		up = pi_idx(vcpu, PIRE0_EL2, idx);
> +		break;
> +	case TR_EL2:
> +		pp = pi_idx(vcpu, PIR_EL2, idx);
> +		up = 0;
> +		break;
> +	}
> +
> +	set_perms(priv, s1p, pp);
> +
> +	if (wi->regime != TR_EL2)
> +		set_perms(unpriv, s1p, up);
> +	else
> +		set_unpriv_perms(s1p, false, false, false);
> +
> +	if (s1p->px && s1p->uw) {
> +		set_priv_perms(s1p, false, false, false);
> +		set_unpriv_perms(s1p, false, false, false);
> +	}
> +}
> +
>  static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op,
>  				   struct s1_walk_info *wi,
>  				   struct s1_walk_result *wr,
>  				   struct s1_perms *s1p)
>  {
> -	compute_s1_direct_permissions(vcpu, wi, wr, s1p);
> +	if (!s1pie_enabled(vcpu, wi->regime))
> +		compute_s1_direct_permissions(vcpu, wi, wr, s1p);
> +	else
> +		compute_s1_indirect_permissions(vcpu, wi, wr, s1p);
> +
>  	compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p);

Is this (and the previous patch to split this up) right?

Looking at this from the ARM ARM (ARM DDI 0487K.a):

	R JHSVW If Indirect permissions are used, then hierarchical permissions are disabled and TCR_ELx.HPDn are RES 1.

>  
>  	if (op == OP_AT_S1E1RP || op == OP_AT_S1E1WP) {
> -- 

Thanks,
Joey