Re: [PATCH v3 18/24] KVM: arm64: Split S1 permission evaluation into direct and hierarchical parts

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

On Wed, Sep 11, 2024 at 02:51:45PM +0100, Marc Zyngier wrote:
> The AArch64.S1DirectBasePermissions() pseudocode deals with both
> direct and hierarchical S1 permission evaluation. While this is
> probably convenient in the pseudocode, we would like a bit more
> flexibility to slot things like indirect permissions.
> 
> To that effect, split the two permission check parts out of
> handle_at_slow() and into their own functions. The permissions
> are passed around in a new s1_perms type that contains the
> individual permissions across the flow.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/at.c | 164 ++++++++++++++++++++++++++------------------
>  1 file changed, 99 insertions(+), 65 deletions(-)
> 
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 73b2ee662f371..d6ee3a5c30bc2 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -47,6 +47,10 @@ struct s1_walk_result {
>  	bool	failed;
>  };
>  
> +struct s1_perms {
> +	bool	ur, uw, ux, pr, pw, px;
> +};
> +
>  static void fail_s1_walk(struct s1_walk_result *wr, u8 fst, bool ptw, bool s2)
>  {
>  	wr->fst		= fst;
> @@ -764,111 +768,141 @@ static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime)
>  	return sctlr & SCTLR_EL1_EPAN;
>  }
>  
> -static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> +static void compute_s1_direct_permissions(struct kvm_vcpu *vcpu,
> +					  struct s1_walk_info *wi,
> +					  struct s1_walk_result *wr,
> +					  struct s1_perms *s1p)
>  {
> -	bool perm_fail, ur, uw, ux, pr, pw, px;
> -	struct s1_walk_result wr = {};
> -	struct s1_walk_info wi = {};
> -	int ret, idx;
> -
> -	ret = setup_s1_walk(vcpu, op, &wi, &wr, vaddr);
> -	if (ret)
> -		goto compute_par;
> -
> -	if (wr.level == S1_MMU_DISABLED)
> -		goto compute_par;
> -
> -	idx = srcu_read_lock(&vcpu->kvm->srcu);
> -
> -	ret = walk_s1(vcpu, &wi, &wr, vaddr);
> -
> -	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> -
> -	if (ret)
> -		goto compute_par;
> -
> -	/* FIXME: revisit when adding indirect permission support */
> -	/* AArch64.S1DirectBasePermissions() */
> -	if (wi.regime != TR_EL2) {
> -		switch (FIELD_GET(PTE_USER | PTE_RDONLY, wr.desc)) {
> +	/* Non-hierarchical part of AArch64.S1DirectBasePermissions() */
> +	if (wi->regime != TR_EL2) {
> +		switch (FIELD_GET(PTE_USER | PTE_RDONLY, wr->desc)) {
>  		case 0b00:
> -			pr = pw = true;
> -			ur = uw = false;
> +			s1p->pr = s1p->pw = true;
> +			s1p->ur = s1p->uw = false;
>  			break;
>  		case 0b01:
> -			pr = pw = ur = uw = true;
> +			s1p->pr = s1p->pw = s1p->ur = s1p->uw = true;
>  			break;
>  		case 0b10:
> -			pr = true;
> -			pw = ur = uw = false;
> +			s1p->pr = true;
> +			s1p->pw = s1p->ur = s1p->uw = false;
>  			break;
>  		case 0b11:
> -			pr = ur = true;
> -			pw = uw = false;
> +			s1p->pr = s1p->ur = true;
> +			s1p->pw = s1p->uw = false;
>  			break;
>  		}
>  
> -		switch (wr.APTable) {
> +		/* We don't use px for anything yet, but hey... */
> +		s1p->px = !((wr->desc & PTE_PXN) || s1p->uw);
> +		s1p->ux = !(wr->desc & PTE_UXN);
> +	} else {
> +		s1p->ur = s1p->uw = s1p->ux = false;
> +
> +		if (!(wr->desc & PTE_RDONLY)) {
> +			s1p->pr = s1p->pw = true;
> +		} else {
> +			s1p->pr = true;
> +			s1p->pw = false;
> +		}
> +
> +		/* XN maps to UXN */
> +		s1p->px = !(wr->desc & PTE_UXN);
> +	}
> +}
> +
> +static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu,
> +						struct s1_walk_info *wi,
> +						struct s1_walk_result *wr,
> +						struct s1_perms *s1p)
> +{

How about:

	if (wi->hpd)
		return;

> +	/* Hierarchical part of AArch64.S1DirectBasePermissions() */
> +	if (wi->regime != TR_EL2) {
> +		switch (wr->APTable) {
>  		case 0b00:
>  			break;
>  		case 0b01:
> -			ur = uw = false;
> +			s1p->ur = s1p->uw = false;
>  			break;
>  		case 0b10:
> -			pw = uw = false;
> +			s1p->pw = s1p->uw = false;
>  			break;
>  		case 0b11:
> -			pw = ur = uw = false;
> +			s1p->pw = s1p->ur = s1p->uw = false;
>  			break;
>  		}
>  
> -		/* We don't use px for anything yet, but hey... */
> -		px = !((wr.desc & PTE_PXN) || wr.PXNTable || uw);
> -		ux = !((wr.desc & PTE_UXN) || wr.UXNTable);
> -
> -		if (op == OP_AT_S1E1RP || op == OP_AT_S1E1WP) {
> -			bool pan;
> -
> -			pan = *vcpu_cpsr(vcpu) & PSR_PAN_BIT;
> -			pan &= ur || uw || (pan3_enabled(vcpu, wi.regime) && ux);
> -			pw &= !pan;
> -			pr &= !pan;
> -		}
> +		s1p->px &= !wr->PXNTable;
> +		s1p->ux &= !wr->UXNTable;
>  	} else {
> -		ur = uw = ux = false;
> +		if (wr->APTable & BIT(1))
> +			s1p->pw = false;
>  
> -		if (!(wr.desc & PTE_RDONLY)) {
> -			pr = pw = true;
> -		} else {
> -			pr = true;
> -			pw = false;
> -		}
> +		/* XN maps to UXN */
> +		s1p->px &= !wr->UXNTable;
> +	}
> +}
>  
> -		if (wr.APTable & BIT(1))
> -			pw = 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);
> +	compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p);
>  
> -		/* XN maps to UXN */
> -		px = !((wr.desc & PTE_UXN) || wr.UXNTable);
> +	if (op == OP_AT_S1E1RP || op == OP_AT_S1E1WP) {
> +		bool pan;
> +
> +		pan = *vcpu_cpsr(vcpu) & PSR_PAN_BIT;
> +		pan &= s1p->ur || s1p->uw || (pan3_enabled(vcpu, wi->regime) && s1p->ux);
> +		s1p->pw &= !pan;
> +		s1p->pr &= !pan;
>  	}
> +}
> +
> +static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> +{
> +	struct s1_walk_result wr = {};
> +	struct s1_walk_info wi = {};
> +	struct s1_perms s1p = {};
> +	bool perm_fail = false;
> +	int ret, idx;
> +
> +	ret = setup_s1_walk(vcpu, op, &wi, &wr, vaddr);
> +	if (ret)
> +		goto compute_par;
> +
> +	if (wr.level == S1_MMU_DISABLED)
> +		goto compute_par;
> +
> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> +
> +	ret = walk_s1(vcpu, &wi, &wr, vaddr);
> +
> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +
> +	if (ret)
> +		goto compute_par;
>  
> -	perm_fail = false;
> +	compute_s1_permissions(vcpu, op, &wi, &wr, &s1p);
>  
>  	switch (op) {
>  	case OP_AT_S1E1RP:
>  	case OP_AT_S1E1R:
>  	case OP_AT_S1E2R:
> -		perm_fail = !pr;
> +		perm_fail = !s1p.pr;
>  		break;
>  	case OP_AT_S1E1WP:
>  	case OP_AT_S1E1W:
>  	case OP_AT_S1E2W:
> -		perm_fail = !pw;
> +		perm_fail = !s1p.pw;
>  		break;
>  	case OP_AT_S1E0R:
> -		perm_fail = !ur;
> +		perm_fail = !s1p.ur;
>  		break;
>  	case OP_AT_S1E0W:
> -		perm_fail = !uw;
> +		perm_fail = !s1p.uw;
>  		break;
>  	case OP_AT_S1E1A:
>  	case OP_AT_S1E2A:
> -- 
> 2.39.2
>