Re: [PATCH v2 02/14] KVM: arm64: Add support for FEAT_XNX stage-2 permissions

[Marc Zyngier <maz@kernel.org>]

On Mon, 17 Nov 2025 22:43:13 +0000,
Oliver Upton <oupton@kernel.org> wrote:
> 
> FEAT_XNX adds support for encoding separate execute permissions for EL0
> and EL1 at stage-2. Add support for this to the page table library,
> hiding the unintuitive encoding scheme behind generic pX and uX
> permission flags.

This patch consistently breaks running VMs on my M2, see below for a
possible fix.

> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_pgtable.h | 17 +++++----
>  arch/arm64/kvm/hyp/pgtable.c         | 54 ++++++++++++++++++++++++----
>  2 files changed, 58 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 2888b5d03757..c72149a607d6 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -89,7 +89,7 @@ typedef u64 kvm_pte_t;
>  
>  #define KVM_PTE_LEAF_ATTR_HI_S1_XN	BIT(54)
>  
> -#define KVM_PTE_LEAF_ATTR_HI_S2_XN	BIT(54)
> +#define KVM_PTE_LEAF_ATTR_HI_S2_XN	GENMASK(54, 53)
>  
>  #define KVM_PTE_LEAF_ATTR_HI_S1_GP	BIT(50)
>  
> @@ -251,12 +251,15 @@ enum kvm_pgtable_stage2_flags {
>   * @KVM_PGTABLE_PROT_SW3:	Software bit 3.
>   */
>  enum kvm_pgtable_prot {
> -	KVM_PGTABLE_PROT_X			= BIT(0),
> -	KVM_PGTABLE_PROT_W			= BIT(1),
> -	KVM_PGTABLE_PROT_R			= BIT(2),
> -
> -	KVM_PGTABLE_PROT_DEVICE			= BIT(3),
> -	KVM_PGTABLE_PROT_NORMAL_NC		= BIT(4),
> +	KVM_PGTABLE_PROT_PX			= BIT(0),
> +	KVM_PGTABLE_PROT_UX			= BIT(1),
> +	KVM_PGTABLE_PROT_X			= KVM_PGTABLE_PROT_PX	|
> +						  KVM_PGTABLE_PROT_UX,
> +	KVM_PGTABLE_PROT_W			= BIT(2),
> +	KVM_PGTABLE_PROT_R			= BIT(3),
> +
> +	KVM_PGTABLE_PROT_DEVICE			= BIT(4),
> +	KVM_PGTABLE_PROT_NORMAL_NC		= BIT(5),
>  
>  	KVM_PGTABLE_PROT_SW0			= BIT(55),
>  	KVM_PGTABLE_PROT_SW1			= BIT(56),
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index c351b4abd5db..8c813bf70b38 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -661,11 +661,36 @@ void kvm_tlb_flush_vmid_range(struct kvm_s2_mmu *mmu,
>  
>  #define KVM_S2_MEMATTR(pgt, attr) PAGE_S2_MEMATTR(attr, stage2_has_fwb(pgt))
>  
> +static int stage2_set_xn_attr(enum kvm_pgtable_prot prot, kvm_pte_t *attr)
> +{
> +	bool px, ux;
> +	u8 xn;
> +
> +	px = prot & KVM_PGTABLE_PROT_PX;
> +	ux = prot & KVM_PGTABLE_PROT_UX;
> +
> +	if (!cpus_have_final_cap(ARM64_HAS_XNX) && px != ux)
> +		return -EINVAL;
> +
> +	if (px && ux)
> +		xn = 0b00;
> +	else if (!px && ux)
> +		xn = 0b01;
> +	else if (!px && !ux)
> +		xn = 0b10;
> +	else
> +		xn = 0b11;
> +
> +	*attr |= FIELD_PREP(KVM_PTE_LEAF_ATTR_HI_S2_XN, xn);

Notice how this is orr'ing bits into an existing descriptor, and...

> +	return 0;
> +}
> +
>  static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot prot,
>  				kvm_pte_t *ptep)
>  {
>  	kvm_pte_t attr;
>  	u32 sh = KVM_PTE_LEAF_ATTR_LO_S2_SH_IS;
> +	int r;
>  
>  	switch (prot & (KVM_PGTABLE_PROT_DEVICE |
>  			KVM_PGTABLE_PROT_NORMAL_NC)) {
> @@ -685,8 +710,9 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p
>  		attr = KVM_S2_MEMATTR(pgt, NORMAL);
>  	}
>  
> -	if (!(prot & KVM_PGTABLE_PROT_X))
> -		attr |= KVM_PTE_LEAF_ATTR_HI_S2_XN;
> +	r = stage2_set_xn_attr(prot, &attr);
> +	if (r)
> +		return r;
>  
>  	if (prot & KVM_PGTABLE_PROT_R)
>  		attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
> @@ -715,8 +741,19 @@ enum kvm_pgtable_prot kvm_pgtable_stage2_pte_prot(kvm_pte_t pte)
>  		prot |= KVM_PGTABLE_PROT_R;
>  	if (pte & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W)
>  		prot |= KVM_PGTABLE_PROT_W;
> -	if (!(pte & KVM_PTE_LEAF_ATTR_HI_S2_XN))
> -		prot |= KVM_PGTABLE_PROT_X;
> +
> +	switch (FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, pte)) {
> +	case 0b00:
> +		prot |= KVM_PGTABLE_PROT_PX | KVM_PGTABLE_PROT_UX;
> +		break;
> +	case 0b01:
> +		prot |= KVM_PGTABLE_PROT_UX;
> +		break;
> +	case 0b11:
> +		prot |= KVM_PGTABLE_PROT_PX;
> +		break;
> +	default:
> +	}
>  
>  	return prot;
>  }
> @@ -1293,6 +1330,7 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
>  	int ret;
>  	s8 level;
>  	kvm_pte_t set = 0, clr = 0;
> +	kvm_pte_t xn;

... how this variable is left uninitialised...

>  
>  	if (prot & KVM_PTE_LEAF_ATTR_HI_SW)
>  		return -EINVAL;
> @@ -1303,8 +1341,12 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
>  	if (prot & KVM_PGTABLE_PROT_W)
>  		set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
>  
> -	if (prot & KVM_PGTABLE_PROT_X)
> -		clr |= KVM_PTE_LEAF_ATTR_HI_S2_XN;
> +	ret = stage2_set_xn_attr(prot, &xn);
> +	if (ret)
> +		return ret;
> +
> +	set |= xn & KVM_PTE_LEAF_ATTR_HI_S2_XN;
> +	clr |= ~xn & KVM_PTE_LEAF_ATTR_HI_S2_XN;

... and finally consumed here. Depending on how the stack variable is
allocated, you may get interesting surprises. I fixed it with this
(belt and braces):

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 8c813bf70b385..d57134d69ea82 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -681,6 +681,7 @@ static int stage2_set_xn_attr(enum kvm_pgtable_prot prot, kvm_pte_t *attr)
 	else
 		xn = 0b11;
 
+	*attr &= ~KVM_PTE_LEAF_ATTR_HI_S2_XN;
 	*attr |= FIELD_PREP(KVM_PTE_LEAF_ATTR_HI_S2_XN, xn);
 	return 0;
 }
@@ -1329,8 +1330,7 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
 {
 	int ret;
 	s8 level;
-	kvm_pte_t set = 0, clr = 0;
-	kvm_pte_t xn;
+	kvm_pte_t xn = 0, set = 0, clr = 0;
 
 	if (prot & KVM_PTE_LEAF_ATTR_HI_SW)
 		return -EINVAL;


Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.