Re: [PATCH 02/22] KVM: arm64: nv: Consolidate computation of stage-2 permissions

[Leonardo Bras <leo.bras@arm.com>]

On Tue, Jun 23, 2026 at 11:41:41AM -0700, Oliver Upton wrote:
> Computing the output permissions from a translation requires
> contextualization, as fields in the descriptors may change depending on
> the MMU context. Centralize the computation of stage-2 permissions
> within the table walk rather as opposed to inspecting the descriptor in
> the case of pX and uX permissions.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_nested.h  | 42 ++++++++--------------------
>  arch/arm64/include/asm/kvm_pgtable.h |  5 ++--
>  arch/arm64/kvm/nested.c              | 33 ++++++++++++++++++++--
>  3 files changed, 45 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index cbdaaa2a2903..aa27f12cf2d4 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -87,13 +87,15 @@ extern void kvm_nested_sync_hwstate(struct kvm_vcpu *vcpu);
>  extern void kvm_nested_setup_mdcr_el2(struct kvm_vcpu *vcpu);
>  
>  struct kvm_s2_trans {
> -	phys_addr_t output;
> -	unsigned long block_size;
> -	bool writable;
> -	bool readable;
> -	int level;
> -	u32 esr;
> -	u64 desc;
> +	u64		desc;
> +	phys_addr_t	output;
> +	unsigned long	block_size;
> +	int		level;
> +	u32		esr;
> +	bool		writable;
> +	bool		readable;
> +	bool		px;
> +	bool		ux;
>  };
>  
>  static inline phys_addr_t kvm_s2_trans_output(struct kvm_s2_trans *trans)
> @@ -129,34 +131,12 @@ static inline bool kvm_has_xnx(struct kvm *kvm)
>  
>  static inline bool kvm_s2_trans_exec_el0(struct kvm *kvm, struct kvm_s2_trans *trans)
>  {
> -	u8 xn = FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, trans->desc);
> -
> -	if (!kvm_has_xnx(kvm))
> -		xn &= FIELD_PREP(KVM_PTE_LEAF_ATTR_HI_S2_XN, 0b10);
> -
> -	switch (xn) {
> -	case 0b00:
> -	case 0b01:
> -		return true;
> -	default:
> -		return false;
> -	}
> +	return trans->ux;
>  }
>  
>  static inline bool kvm_s2_trans_exec_el1(struct kvm *kvm, struct kvm_s2_trans *trans)
>  {
> -	u8 xn = FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, trans->desc);
> -
> -	if (!kvm_has_xnx(kvm))
> -		xn &= FIELD_PREP(KVM_PTE_LEAF_ATTR_HI_S2_XN, 0b10);
> -
> -	switch (xn) {
> -	case 0b00:
> -	case 0b11:
> -		return true;
> -	default:
> -		return false;
> -	}
> +	return trans->px;
>  }
>  
>  extern int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa,
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 41a8687938eb..22aeb2ed18d1 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -79,6 +79,8 @@ typedef u64 kvm_pte_t;
>  #define KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR	GENMASK(5, 2)
>  #define KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R	BIT(6)
>  #define KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W	BIT(7)
> +#define KVM_PTE_LEAF_ATTR_LO_S2_S2AP	(KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R |	\
> +					 KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W)
>  #define KVM_PTE_LEAF_ATTR_LO_S2_SH	GENMASK(9, 8)
>  #define KVM_PTE_LEAF_ATTR_LO_S2_SH_IS	3
>  #define KVM_PTE_LEAF_ATTR_LO_S2_AF	BIT(10)
> @@ -95,8 +97,7 @@ typedef u64 kvm_pte_t;
>  
>  #define KVM_PTE_LEAF_ATTR_HI_S1_GP	BIT(50)
>  
> -#define KVM_PTE_LEAF_ATTR_S2_PERMS	(KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R | \
> -					 KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
> +#define KVM_PTE_LEAF_ATTR_S2_PERMS	(KVM_PTE_LEAF_ATTR_LO_S2_S2AP |	\
>  					 KVM_PTE_LEAF_ATTR_HI_S2_XN)
>  
>  /* pKVM invalid pte encodings */
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 9e60c7c822ae..c9300703bd0d 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -241,6 +241,36 @@ static int swap_guest_s2_desc(struct kvm_vcpu *vcpu, phys_addr_t pa, u64 old, u6
>  	return __kvm_qat_swap_desc(vcpu->kvm, pa, old, new);
>  }
>  
> +static void compute_s2_permissions(struct kvm_vcpu *vcpu, struct s2_walk_info *wi,
> +				   struct s2_walk_step *ws, struct kvm_s2_trans *trans)
> +{
> +	u8 s2ap = FIELD_GET(KVM_PTE_LEAF_ATTR_LO_S2_S2AP, ws->desc);
> +	u8 xn = FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, ws->desc);
> +
> +	if (!kvm_has_xnx(vcpu->kvm))
> +		xn &= 0b10;
> +
> +	switch (xn) {
> +	case 0b00:
> +		trans->px = trans->ux = true;
> +		break;
> +	case 0b01:
> +		trans->px = false;
> +		trans->ux = true;
> +		break;
> +	case 0b10:
> +		trans->px = trans->ux = false;
> +		break;
> +	case 0b11:
> +		trans->px = true;
> +		trans->ux = false;
> +		break;
> +	}
> +

Nit: couldn't the above be summarized as below?

	trans->ux = !(xn & BIT(1));

	if (kvm_has_xnx(vcpu->kvm) && xn & BIT(0)) 
		trans->px = !trans->ux;
	else
		trans->px = trans->ux;
					

or maybe even skip the FIELD_GET and do:

	trans->ux = !(ws->desc & KVM_PTE_LEAF_ATTR_HI_S1_UXN);

	if (kvm_has_xnx(vcpu->kvm) &&
	    ws->desc & KVM_PTE_LEAF_ATTR_HI_S1_PXN) 
		trans->px = !trans->ux;
	else
		trans->px = trans->ux;


Maybe it's just me, but that looks easier to understand what each bit does 
based on the feature.

In any case, LGTM.

FWIW:
Reviewed-by: Leonardo Bras <leo.bras@arm.com>

Thanks!
Leo