Re: [PATCH 0/4] KVM: arm64: nv: HAF fixes

[Marc Zyngier <maz@kernel.org>]

On Fri, 28 Nov 2025 10:09:42 +0000,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Based on kvmarm's next branch.
> 
> HAF support for the software translation table walker was merged while I
> was in the process of reading the patches, so instead of comments I have
> these few fixes.
> 
> One thing I didn't touch is this sequence in hyp_set_prot_attr():
> 
> 	if (prot & KVM_PGTABLE_PROT_X) {
> 		/* don't set the XN bit */
> 	} else {
> 		attr |= KVM_PTE_LEAF_ATTR_HI_S1_XN;
> 	}
> 
> If the caller is executing in nVHE mode, the translation regime is EL2,
> which has only PrivExecute permission. Since KVM_PGTABLE_PROT_X is now the
> union of PrivExecute and UnprivExecute, if the caller requests only the
> UnprivExecute permission, but no PrivExecute permission, the function does
> not return an error code and sets the PrivExecute permission.

I don't think this is a huge problem *right now*, as long as we don't
have anything that looks like "hvhe hypervisor userspace" (yes, I
proposed that a while ago, and haven't completely dropped the
idea). But at the same time, the page-table code should probably be
built to the architecture and not to the use cases.

But it also outlines a rather bad bug in the hVHE case, where we set
the UXN bit instead of the PXN bit...

What I have in mind is something like this, untested. Thoughts?

	M.

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index d57c12f074a40..48305118ba3c5 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -88,6 +88,8 @@ typedef u64 kvm_pte_t;
 #define KVM_PTE_LEAF_ATTR_HI_SW		GENMASK(58, 55)
 
 #define KVM_PTE_LEAF_ATTR_HI_S1_XN	BIT(54)
+#define KVM_PTE_LEAF_ATTR_HI_S1_UXN	BIT(54)
+#define KVM_PTE_LEAF_ATTR_HI_S1_PXN	BIT(53)
 
 #define KVM_PTE_LEAF_ATTR_HI_S2_XN	GENMASK(54, 53)
 
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index e0bd6a0172729..cbf9b6b58e284 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -330,6 +330,11 @@ struct hyp_map_data {
 	kvm_pte_t			attr;
 };
 
+static bool el2_nvhe(void)
+{
+	return !has_vhe() && !cpus_have_final_cap(ARM64_KVM_PROTECTED_MODE);
+}
+
 static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep)
 {
 	bool device = prot & KVM_PGTABLE_PROT_DEVICE;
@@ -342,6 +347,9 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep)
 	if (!(prot & KVM_PGTABLE_PROT_R))
 		return -EINVAL;
 
+	if (el2_nvhe())
+		prot &= ~KVM_PGTABLE_PROT_UX;
+
 	if (prot & KVM_PGTABLE_PROT_X) {
 		if (prot & KVM_PGTABLE_PROT_W)
 			return -EINVAL;
@@ -351,8 +359,16 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep)
 
 		if (system_supports_bti_kernel())
 			attr |= KVM_PTE_LEAF_ATTR_HI_S1_GP;
+	}
+
+	if (el2_nvhe()) {
+		if (!(prot & KVM_PGTABLE_PROT_PX))
+			attr |= KVM_PTE_LEAF_ATTR_HI_S1_XN;
 	} else {
-		attr |= KVM_PTE_LEAF_ATTR_HI_S1_XN;
+		if (!(prot & KVM_PGTABLE_PROT_PX))
+			attr |= KVM_PTE_LEAF_ATTR_HI_S1_PXN;
+		if (!(prot & KVM_PGTABLE_PROT_UX))
+			attr |= KVM_PTE_LEAF_ATTR_HI_S1_UXN;
 	}
 
 	attr |= FIELD_PREP(KVM_PTE_LEAF_ATTR_LO_S1_AP, ap);
@@ -373,8 +389,15 @@ enum kvm_pgtable_prot kvm_pgtable_hyp_pte_prot(kvm_pte_t pte)
 	if (!kvm_pte_valid(pte))
 		return prot;
 
-	if (!(pte & KVM_PTE_LEAF_ATTR_HI_S1_XN))
-		prot |= KVM_PGTABLE_PROT_X;
+	if (el2_nvhe()) {
+		if (!(pte & KVM_PTE_LEAF_ATTR_HI_S1_XN))
+			prot |= KVM_PGTABLE_PROT_PX;
+	} else {
+		if (!(pte & KVM_PTE_LEAF_ATTR_HI_S1_PXN))
+			prot |= KVM_PGTABLE_PROT_PX;
+		if (!(pte & KVM_PTE_LEAF_ATTR_HI_S1_UXN))
+			prot |= KVM_PGTABLE_PROT_UX;
+	}
 
 	ap = FIELD_GET(KVM_PTE_LEAF_ATTR_LO_S1_AP, pte);
 	if (ap == KVM_PTE_LEAF_ATTR_LO_S1_AP_RO)

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