[PATCH v2 0/2] KVM : selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[PATCH v2 0/2] KVM : selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[Raghavendra Rao Ananta]

The series fixes a conflict in memory attributes in some
implementations,
such as Neoverse-N3, that causes a data abort in guest EL1 with FSC
0x35 (IMPLEMENTATION DEFINED fault (Unsupported Exclusive or Atomic
access)).

Patch-1 is a cleanup patch that replaces numbers (and comments) to
using proper macros for hardware configuration, such as registers and
page-table entries.

Patch-2 fixes the actual bug and sets the page attrs to Inner-Shareable
by default for the VMs created in the selftests. More details are
presented in the commit text.

v1: https://lore.kernel.org/all/20250404220659.1312465-1-rananta@google.com/

v1 -> v2: Addressed Oliver's comments (thank you)
 - Moved the TCR_* macros from tools' sysreg.h to selftests' local processor.h
   in patch-1.
 - Adjsted the citations to describe the issue more appropriately in
   patch-2.

Raghavendra Rao Ananta (2):
  KVM: selftests: arm64: Introduce and use hardware-definition macros
  KVM: selftests: arm64: Explicitly set the page attrs to
    Inner-Shareable

 .../selftests/kvm/arm64/page_fault_test.c     |  2 +-
 .../selftests/kvm/include/arm64/processor.h   | 67 +++++++++++++++++--
 .../selftests/kvm/lib/arm64/processor.c       | 60 ++++++++++-------
 3 files changed, 96 insertions(+), 33 deletions(-)


base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v2 1/2] KVM: selftests: arm64: Introduce and use hardware-definition macros

[Raghavendra Rao Ananta]

The kvm selftest library for arm64 currently configures the hardware
fields, such as shift and mask in the page-table entries and registers,
directly with numbers. While it add comments at places, it's better to
rewrite them with appropriate macros to improve the readability and
reduce the risk of errors. Hence, introduce macros to define the
hardware fields and use them in the arm64 processor library.

Most of the definitions are primary copied from the Linux's header,
arch/arm64/include/asm/pgtable-hwdef.h.

No functional change intended.

Suggested-by: Oliver Upton <oupton@google.com>
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/kvm/arm64/page_fault_test.c     |  2 +-
 .../selftests/kvm/include/arm64/processor.h   | 66 +++++++++++++++++--
 .../selftests/kvm/lib/arm64/processor.c       | 57 ++++++++--------
 3 files changed, 92 insertions(+), 33 deletions(-)

diff --git a/tools/testing/selftests/kvm/arm64/page_fault_test.c b/tools/testing/selftests/kvm/arm64/page_fault_test.c
index ec33a8f9c908..dc6559dad9d8 100644
--- a/tools/testing/selftests/kvm/arm64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/arm64/page_fault_test.c
@@ -199,7 +199,7 @@ static bool guest_set_ha(void)
 	if (hadbs == 0)
 		return false;
 
-	tcr = read_sysreg(tcr_el1) | TCR_EL1_HA;
+	tcr = read_sysreg(tcr_el1) | TCR_HA;
 	write_sysreg(tcr, tcr_el1);
 	isb();
 
diff --git a/tools/testing/selftests/kvm/include/arm64/processor.h b/tools/testing/selftests/kvm/include/arm64/processor.h
index 1e8d0d531fbd..7d88ff22013a 100644
--- a/tools/testing/selftests/kvm/include/arm64/processor.h
+++ b/tools/testing/selftests/kvm/include/arm64/processor.h
@@ -62,6 +62,66 @@
 	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |				\
 	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT))
 
+/* TCR_EL1 specific flags */
+#define TCR_T0SZ_OFFSET	0
+#define TCR_T0SZ(x)		((UL(64) - (x)) << TCR_T0SZ_OFFSET)
+
+#define TCR_IRGN0_SHIFT	8
+#define TCR_IRGN0_MASK		(UL(3) << TCR_IRGN0_SHIFT)
+#define TCR_IRGN0_NC		(UL(0) << TCR_IRGN0_SHIFT)
+#define TCR_IRGN0_WBWA		(UL(1) << TCR_IRGN0_SHIFT)
+#define TCR_IRGN0_WT		(UL(2) << TCR_IRGN0_SHIFT)
+#define TCR_IRGN0_WBnWA	(UL(3) << TCR_IRGN0_SHIFT)
+
+#define TCR_ORGN0_SHIFT	10
+#define TCR_ORGN0_MASK		(UL(3) << TCR_ORGN0_SHIFT)
+#define TCR_ORGN0_NC		(UL(0) << TCR_ORGN0_SHIFT)
+#define TCR_ORGN0_WBWA		(UL(1) << TCR_ORGN0_SHIFT)
+#define TCR_ORGN0_WT		(UL(2) << TCR_ORGN0_SHIFT)
+#define TCR_ORGN0_WBnWA	(UL(3) << TCR_ORGN0_SHIFT)
+
+#define TCR_SH0_SHIFT		12
+#define TCR_SH0_MASK		(UL(3) << TCR_SH0_SHIFT)
+#define TCR_SH0_INNER		(UL(3) << TCR_SH0_SHIFT)
+
+#define TCR_TG0_SHIFT		14
+#define TCR_TG0_MASK		(UL(3) << TCR_TG0_SHIFT)
+#define TCR_TG0_4K		(UL(0) << TCR_TG0_SHIFT)
+#define TCR_TG0_64K		(UL(1) << TCR_TG0_SHIFT)
+#define TCR_TG0_16K		(UL(2) << TCR_TG0_SHIFT)
+
+#define TCR_IPS_SHIFT		32
+#define TCR_IPS_MASK		(UL(7) << TCR_IPS_SHIFT)
+#define TCR_IPS_52_BITS	(UL(6) << TCR_IPS_SHIFT)
+#define TCR_IPS_48_BITS	(UL(5) << TCR_IPS_SHIFT)
+#define TCR_IPS_40_BITS	(UL(2) << TCR_IPS_SHIFT)
+#define TCR_IPS_36_BITS	(UL(1) << TCR_IPS_SHIFT)
+
+#define TCR_HA			(UL(1) << 39)
+#define TCR_DS			(UL(1) << 59)
+
+/*
+ * AttrIndx[2:0] encoding (mapping attributes defined in the MAIR* registers).
+ */
+#define PTE_ATTRINDX(t)	((t) << 2)
+#define PTE_ATTRINDX_MASK	GENMASK(4, 2)
+#define PTE_ATTRINDX_SHIFT	2
+
+#define PTE_VALID		BIT(0)
+#define PGD_TYPE_TABLE		BIT(1)
+#define PUD_TYPE_TABLE		BIT(1)
+#define PMD_TYPE_TABLE		BIT(1)
+#define PTE_TYPE_PAGE		BIT(1)
+
+#define PTE_AF			BIT(10)
+
+#define PTE_ADDR_MASK(page_shift)	GENMASK(47, (page_shift))
+#define PTE_ADDR_51_48			GENMASK(15, 12)
+#define PTE_ADDR_51_48_SHIFT		12
+#define PTE_ADDR_MASK_LPA2(page_shift)	GENMASK(49, (page_shift))
+#define PTE_ADDR_51_50_LPA2		GENMASK(9, 8)
+#define PTE_ADDR_51_50_LPA2_SHIFT	8
+
 void aarch64_vcpu_setup(struct kvm_vcpu *vcpu, struct kvm_vcpu_init *init);
 struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
 				  struct kvm_vcpu_init *init, void *guest_code);
@@ -102,12 +162,6 @@ enum {
 			   (v) == VECTOR_SYNC_LOWER_64    || \
 			   (v) == VECTOR_SYNC_LOWER_32)
 
-/* Access flag */
-#define PTE_AF			(1ULL << 10)
-
-/* Access flag update enable/disable */
-#define TCR_EL1_HA		(1ULL << 39)
-
 void aarch64_get_supported_page_sizes(uint32_t ipa, uint32_t *ipa4k,
 					uint32_t *ipa16k, uint32_t *ipa64k);
 
diff --git a/tools/testing/selftests/kvm/lib/arm64/processor.c b/tools/testing/selftests/kvm/lib/arm64/processor.c
index 7ba3aa3755f3..da5802c8a59c 100644
--- a/tools/testing/selftests/kvm/lib/arm64/processor.c
+++ b/tools/testing/selftests/kvm/lib/arm64/processor.c
@@ -72,13 +72,13 @@ static uint64_t addr_pte(struct kvm_vm *vm, uint64_t pa, uint64_t attrs)
 	uint64_t pte;
 
 	if (use_lpa2_pte_format(vm)) {
-		pte = pa & GENMASK(49, vm->page_shift);
-		pte |= FIELD_GET(GENMASK(51, 50), pa) << 8;
-		attrs &= ~GENMASK(9, 8);
+		pte = pa & PTE_ADDR_MASK_LPA2(vm->page_shift);
+		pte |= FIELD_GET(GENMASK(51, 50), pa) << PTE_ADDR_51_50_LPA2_SHIFT;
+		attrs &= ~PTE_ADDR_51_50_LPA2;
 	} else {
-		pte = pa & GENMASK(47, vm->page_shift);
+		pte = pa & PTE_ADDR_MASK(vm->page_shift);
 		if (vm->page_shift == 16)
-			pte |= FIELD_GET(GENMASK(51, 48), pa) << 12;
+			pte |= FIELD_GET(GENMASK(51, 48), pa) << PTE_ADDR_51_48_SHIFT;
 	}
 	pte |= attrs;
 
@@ -90,12 +90,12 @@ static uint64_t pte_addr(struct kvm_vm *vm, uint64_t pte)
 	uint64_t pa;
 
 	if (use_lpa2_pte_format(vm)) {
-		pa = pte & GENMASK(49, vm->page_shift);
-		pa |= FIELD_GET(GENMASK(9, 8), pte) << 50;
+		pa = pte & PTE_ADDR_MASK_LPA2(vm->page_shift);
+		pa |= FIELD_GET(PTE_ADDR_51_50_LPA2, pte) << 50;
 	} else {
-		pa = pte & GENMASK(47, vm->page_shift);
+		pa = pte & PTE_ADDR_MASK(vm->page_shift);
 		if (vm->page_shift == 16)
-			pa |= FIELD_GET(GENMASK(15, 12), pte) << 48;
+			pa |= FIELD_GET(PTE_ADDR_51_48, pte) << 48;
 	}
 
 	return pa;
@@ -128,7 +128,8 @@ void virt_arch_pgd_alloc(struct kvm_vm *vm)
 static void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 			 uint64_t flags)
 {
-	uint8_t attr_idx = flags & 7;
+	uint8_t attr_idx = flags & (PTE_ATTRINDX_MASK >> PTE_ATTRINDX_SHIFT);
+	uint64_t pg_attr;
 	uint64_t *ptep;
 
 	TEST_ASSERT((vaddr % vm->page_size) == 0,
@@ -147,18 +148,21 @@ static void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 
 	ptep = addr_gpa2hva(vm, vm->pgd) + pgd_index(vm, vaddr) * 8;
 	if (!*ptep)
-		*ptep = addr_pte(vm, vm_alloc_page_table(vm), 3);
+		*ptep = addr_pte(vm, vm_alloc_page_table(vm),
+				 PGD_TYPE_TABLE | PTE_VALID);
 
 	switch (vm->pgtable_levels) {
 	case 4:
 		ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pud_index(vm, vaddr) * 8;
 		if (!*ptep)
-			*ptep = addr_pte(vm, vm_alloc_page_table(vm), 3);
+			*ptep = addr_pte(vm, vm_alloc_page_table(vm),
+					 PUD_TYPE_TABLE | PTE_VALID);
 		/* fall through */
 	case 3:
 		ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pmd_index(vm, vaddr) * 8;
 		if (!*ptep)
-			*ptep = addr_pte(vm, vm_alloc_page_table(vm), 3);
+			*ptep = addr_pte(vm, vm_alloc_page_table(vm),
+					 PMD_TYPE_TABLE | PTE_VALID);
 		/* fall through */
 	case 2:
 		ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pte_index(vm, vaddr) * 8;
@@ -167,7 +171,8 @@ static void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 		TEST_FAIL("Page table levels must be 2, 3, or 4");
 	}
 
-	*ptep = addr_pte(vm, paddr, (attr_idx << 2) | (1 << 10) | 3);  /* AF */
+	pg_attr = PTE_AF | PTE_ATTRINDX(attr_idx) | PTE_TYPE_PAGE | PTE_VALID;
+	*ptep = addr_pte(vm, paddr, pg_attr);
 }
 
 void virt_arch_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr)
@@ -293,20 +298,20 @@ void aarch64_vcpu_setup(struct kvm_vcpu *vcpu, struct kvm_vcpu_init *init)
 	case VM_MODE_P48V48_64K:
 	case VM_MODE_P40V48_64K:
 	case VM_MODE_P36V48_64K:
-		tcr_el1 |= 1ul << 14; /* TG0 = 64KB */
+		tcr_el1 |= TCR_TG0_64K;
 		break;
 	case VM_MODE_P52V48_16K:
 	case VM_MODE_P48V48_16K:
 	case VM_MODE_P40V48_16K:
 	case VM_MODE_P36V48_16K:
 	case VM_MODE_P36V47_16K:
-		tcr_el1 |= 2ul << 14; /* TG0 = 16KB */
+		tcr_el1 |= TCR_TG0_16K;
 		break;
 	case VM_MODE_P52V48_4K:
 	case VM_MODE_P48V48_4K:
 	case VM_MODE_P40V48_4K:
 	case VM_MODE_P36V48_4K:
-		tcr_el1 |= 0ul << 14; /* TG0 = 4KB */
+		tcr_el1 |= TCR_TG0_4K;
 		break;
 	default:
 		TEST_FAIL("Unknown guest mode, mode: 0x%x", vm->mode);
@@ -319,35 +324,35 @@ void aarch64_vcpu_setup(struct kvm_vcpu *vcpu, struct kvm_vcpu_init *init)
 	case VM_MODE_P52V48_4K:
 	case VM_MODE_P52V48_16K:
 	case VM_MODE_P52V48_64K:
-		tcr_el1 |= 6ul << 32; /* IPS = 52 bits */
+		tcr_el1 |= TCR_IPS_52_BITS;
 		ttbr0_el1 |= FIELD_GET(GENMASK(51, 48), vm->pgd) << 2;
 		break;
 	case VM_MODE_P48V48_4K:
 	case VM_MODE_P48V48_16K:
 	case VM_MODE_P48V48_64K:
-		tcr_el1 |= 5ul << 32; /* IPS = 48 bits */
+		tcr_el1 |= TCR_IPS_48_BITS;
 		break;
 	case VM_MODE_P40V48_4K:
 	case VM_MODE_P40V48_16K:
 	case VM_MODE_P40V48_64K:
-		tcr_el1 |= 2ul << 32; /* IPS = 40 bits */
+		tcr_el1 |= TCR_IPS_40_BITS;
 		break;
 	case VM_MODE_P36V48_4K:
 	case VM_MODE_P36V48_16K:
 	case VM_MODE_P36V48_64K:
 	case VM_MODE_P36V47_16K:
-		tcr_el1 |= 1ul << 32; /* IPS = 36 bits */
+		tcr_el1 |= TCR_IPS_36_BITS;
 		break;
 	default:
 		TEST_FAIL("Unknown guest mode, mode: 0x%x", vm->mode);
 	}
 
-	sctlr_el1 |= (1 << 0) | (1 << 2) | (1 << 12) /* M | C | I */;
-	/* TCR_EL1 |= IRGN0:WBWA | ORGN0:WBWA | SH0:Inner-Shareable */;
-	tcr_el1 |= (1 << 8) | (1 << 10) | (3 << 12);
-	tcr_el1 |= (64 - vm->va_bits) /* T0SZ */;
+	sctlr_el1 |= SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_I;
+
+	tcr_el1 |= TCR_IRGN0_WBWA | TCR_ORGN0_WBWA | TCR_SH0_INNER;
+	tcr_el1 |= TCR_T0SZ(vm->va_bits);
 	if (use_lpa2_pte_format(vm))
-		tcr_el1 |= (1ul << 59) /* DS */;
+		tcr_el1 |= TCR_DS;
 
 	vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_SCTLR_EL1), sctlr_el1);
 	vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_TCR_EL1), tcr_el1);
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v2 2/2] KVM: selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[Raghavendra Rao Ananta]

Atomic instructions such as 'ldset' over (global) variables in the guest
is observed to cause an EL1 data abort with FSC 0x35 (IMPLEMENTATION
DEFINED fault (Unsupported Exclusive or Atomic access)). The observation
was particularly apparent on Neoverse-N3.

According to ARM ARM DDI0487L.a B2.2.6 (Possible implementation
restrictions on using atomic instructions), atomic instructions are
architecturally guaranteed for Inner Shareable and Outer Shareable
attributes. For Non-Shareable attribute, the atomic instructions are
not atomic and issuing such an instruction can lead to the FSC
mentioned in this case (among other things).

Moreover, according to DDI0487L.a C3.2.6 (Single-copy atomic 64-byte
load/store), it is implementation defined that a data abort with the
mentioned FSC is reported for the first stage of translation that
provides an inappropriate memory type. It's likely that Neoverse-N3
chose to implement these two and why we see an FSC of 0x35 in EL1 upon
executing atomic instructions.

ARM64 KVM selftests sets no shareable attributes, which makes them
Non-Shareable by default. Hence, explicitly set them as Inner-Shareable
to fix this issue.

Suggested-by: Oliver Upton <oupton@google.com>
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/testing/selftests/kvm/include/arm64/processor.h | 1 +
 tools/testing/selftests/kvm/lib/arm64/processor.c     | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/arm64/processor.h b/tools/testing/selftests/kvm/include/arm64/processor.h
index 7d88ff22013a..b0fc0f945766 100644
--- a/tools/testing/selftests/kvm/include/arm64/processor.h
+++ b/tools/testing/selftests/kvm/include/arm64/processor.h
@@ -113,6 +113,7 @@
 #define PMD_TYPE_TABLE		BIT(1)
 #define PTE_TYPE_PAGE		BIT(1)
 
+#define PTE_SHARED		(UL(3) << 8) /* SH[1:0], inner shareable */
 #define PTE_AF			BIT(10)
 
 #define PTE_ADDR_MASK(page_shift)	GENMASK(47, (page_shift))
diff --git a/tools/testing/selftests/kvm/lib/arm64/processor.c b/tools/testing/selftests/kvm/lib/arm64/processor.c
index da5802c8a59c..9d69904cb608 100644
--- a/tools/testing/selftests/kvm/lib/arm64/processor.c
+++ b/tools/testing/selftests/kvm/lib/arm64/processor.c
@@ -172,6 +172,9 @@ static void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 	}
 
 	pg_attr = PTE_AF | PTE_ATTRINDX(attr_idx) | PTE_TYPE_PAGE | PTE_VALID;
+	if (!use_lpa2_pte_format(vm))
+		pg_attr |= PTE_SHARED;
+
 	*ptep = addr_pte(vm, paddr, pg_attr);
 }
 
-- 
2.49.0.504.g3bcea36a83-goog

Re: [PATCH v2 2/2] KVM: selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[Mingwei Zhang]

.

On Fri, Apr 4, 2025 at 5:10 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
>
> Atomic instructions such as 'ldset' over (global) variables in the guest
> is observed to cause an EL1 data abort with FSC 0x35 (IMPLEMENTATION
> DEFINED fault (Unsupported Exclusive or Atomic access)). The observation
> was particularly apparent on Neoverse-N3.
>
> According to ARM ARM DDI0487L.a B2.2.6 (Possible implementation
> restrictions on using atomic instructions), atomic instructions are
> architecturally guaranteed for Inner Shareable and Outer Shareable
> attributes. For Non-Shareable attribute, the atomic instructions are
> not atomic and issuing such an instruction can lead to the FSC
> mentioned in this case (among other things).
>
> Moreover, according to DDI0487L.a C3.2.6 (Single-copy atomic 64-byte
> load/store), it is implementation defined that a data abort with the
> mentioned FSC is reported for the first stage of translation that
> provides an inappropriate memory type. It's likely that Neoverse-N3
> chose to implement these two and why we see an FSC of 0x35 in EL1 upon
> executing atomic instructions.

nit: It's likely that Neoverse-N3 chose to implement this option (the
first option) instead of reporting at the final enabled stage of
translation

I have minor question here: The DDI0487L C3.2.6 (Single-copy atomic
64-byte load/store) mentioned

"""
When the instructions access a memory type that is not one of the
following, a data abort for unsupported Exclusive or atomic access is
generated:

• Normal Inner Non-cacheable, Outer Non-cacheable.
"""

So, the above is the "Normal Inner Non-cacheable", but in our case we
have "Normal and non-shareable" in stage-1 mapping, right? I know it
is very close, but it seems the situation is still only "one bit" away
in my understanding...

>
> ARM64 KVM selftests sets no shareable attributes, which makes them
> Non-Shareable by default. Hence, explicitly set them as Inner-Shareable
> to fix this issue.
>
> Suggested-by: Oliver Upton <oupton@google.com>
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  tools/testing/selftests/kvm/include/arm64/processor.h | 1 +
>  tools/testing/selftests/kvm/lib/arm64/processor.c     | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/include/arm64/processor.h b/tools/testing/selftests/kvm/include/arm64/processor.h
> index 7d88ff22013a..b0fc0f945766 100644
> --- a/tools/testing/selftests/kvm/include/arm64/processor.h
> +++ b/tools/testing/selftests/kvm/include/arm64/processor.h
> @@ -113,6 +113,7 @@
>  #define PMD_TYPE_TABLE         BIT(1)
>  #define PTE_TYPE_PAGE          BIT(1)
>
> +#define PTE_SHARED             (UL(3) << 8) /* SH[1:0], inner shareable */
>  #define PTE_AF                 BIT(10)
>
>  #define PTE_ADDR_MASK(page_shift)      GENMASK(47, (page_shift))
> diff --git a/tools/testing/selftests/kvm/lib/arm64/processor.c b/tools/testing/selftests/kvm/lib/arm64/processor.c
> index da5802c8a59c..9d69904cb608 100644
> --- a/tools/testing/selftests/kvm/lib/arm64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/arm64/processor.c
> @@ -172,6 +172,9 @@ static void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
>         }
>
>         pg_attr = PTE_AF | PTE_ATTRINDX(attr_idx) | PTE_TYPE_PAGE | PTE_VALID;
> +       if (!use_lpa2_pte_format(vm))
> +               pg_attr |= PTE_SHARED;
> +
>         *ptep = addr_pte(vm, paddr, pg_attr);
>  }
>
> --
> 2.49.0.504.g3bcea36a83-goog
>

Re: [PATCH v2 2/2] KVM: selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[Oliver Upton]

On Fri, Apr 04, 2025 at 05:31:49PM -0700, Mingwei Zhang wrote:
> On Fri, Apr 4, 2025 at 5:10 PM Raghavendra Rao Ananta
> <rananta@google.com> wrote:
> >
> > Atomic instructions such as 'ldset' over (global) variables in the guest
> > is observed to cause an EL1 data abort with FSC 0x35 (IMPLEMENTATION
> > DEFINED fault (Unsupported Exclusive or Atomic access)). The observation
> > was particularly apparent on Neoverse-N3.
> >
> > According to ARM ARM DDI0487L.a B2.2.6 (Possible implementation
> > restrictions on using atomic instructions), atomic instructions are
> > architecturally guaranteed for Inner Shareable and Outer Shareable
> > attributes. For Non-Shareable attribute, the atomic instructions are
> > not atomic and issuing such an instruction can lead to the FSC
> > mentioned in this case (among other things).
> >
> > Moreover, according to DDI0487L.a C3.2.6 (Single-copy atomic 64-byte
> > load/store), it is implementation defined that a data abort with the
> > mentioned FSC is reported for the first stage of translation that
> > provides an inappropriate memory type. It's likely that Neoverse-N3
> > chose to implement these two and why we see an FSC of 0x35 in EL1 upon
> > executing atomic instructions.

Ok, can we please drop this second reference?

This is talking about something else (FEAT_LS64) that happens to share
the same FSC as an unsupported atomic instruction. I mentioned this to
you internally as an illustration of how different implementations may
behave when determining if the attributes support a particular access,
but it isn't actually relevant to this change.

> nit: It's likely that Neoverse-N3 chose to implement this option (the
> first option) instead of reporting at the final enabled stage of
> translation

I would much rather we rely on the language that describes what the
architecture guarantees rather than speculate as to how Neoverse-N3
behaves.

Mentioning that the breakage was observed on Neoverse-N3 is still useful
to add to the changelog.

> I have minor question here: The DDI0487L C3.2.6 (Single-copy atomic
> 64-byte load/store) mentioned
> 
> """
> When the instructions access a memory type that is not one of the
> following, a data abort for unsupported Exclusive or atomic access is
> generated:
> 
> • Normal Inner Non-cacheable, Outer Non-cacheable.
> """
> 
> So, the above is the "Normal Inner Non-cacheable", but in our case we
> have "Normal and non-shareable" in stage-1 mapping, right? I know it
> is very close, but it seems the situation is still only "one bit" away
> in my understanding...

This citation relates to FEAT_LS64. If you look at B2.2.6 instead, it
reads:

"""
The memory types for which it is architecturally guaranteed that the
atomic instructions will be atomic are:

 - Inner Shareable, Inner Write-Back, Outer Write-Back Normal memory
   with Read allocation hints and Write allocation hints and not
   transient.

 - Outer Shareable, Inner Write-Back, Outer Write-Back Normal memory
   with Read allocation hints and Write allocation hints and not
   transient.
"""

and

"""
If the atomic insturctions are not atomic in regard to other agents that
access memory, then performing an atomic instruction to such a location
can have one or more of the following effects:

[...]

 - The instruction generates an IMPLEMENTATION DEFINED fault reported
   using the Data Abort Fault status code of ESR_ELx.DFSC = 110101
"""

The memory type used by KVM selftests is *Non-Shareable*, which is not
one of the memory types guaranteed by the architecture to work.

Thanks,
Oliver

Re: [PATCH v2 2/2] KVM: selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[Mingwei Zhang]

On Fri, Apr 4, 2025 at 7:51 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Fri, Apr 04, 2025 at 05:31:49PM -0700, Mingwei Zhang wrote:
> > On Fri, Apr 4, 2025 at 5:10 PM Raghavendra Rao Ananta
> > <rananta@google.com> wrote:
> > >
> > > Atomic instructions such as 'ldset' over (global) variables in the guest
> > > is observed to cause an EL1 data abort with FSC 0x35 (IMPLEMENTATION
> > > DEFINED fault (Unsupported Exclusive or Atomic access)). The observation
> > > was particularly apparent on Neoverse-N3.
> > >
> > > According to ARM ARM DDI0487L.a B2.2.6 (Possible implementation
> > > restrictions on using atomic instructions), atomic instructions are
> > > architecturally guaranteed for Inner Shareable and Outer Shareable
> > > attributes. For Non-Shareable attribute, the atomic instructions are
> > > not atomic and issuing such an instruction can lead to the FSC
> > > mentioned in this case (among other things).
> > >
> > > Moreover, according to DDI0487L.a C3.2.6 (Single-copy atomic 64-byte
> > > load/store), it is implementation defined that a data abort with the
> > > mentioned FSC is reported for the first stage of translation that
> > > provides an inappropriate memory type. It's likely that Neoverse-N3
> > > chose to implement these two and why we see an FSC of 0x35 in EL1 upon
> > > executing atomic instructions.
>
> Ok, can we please drop this second reference?
>
> This is talking about something else (FEAT_LS64) that happens to share
> the same FSC as an unsupported atomic instruction. I mentioned this to
> you internally as an illustration of how different implementations may
> behave when determining if the attributes support a particular access,
> but it isn't actually relevant to this change.
>
> > nit: It's likely that Neoverse-N3 chose to implement this option (the
> > first option) instead of reporting at the final enabled stage of
> > translation
>
> I would much rather we rely on the language that describes what the
> architecture guarantees rather than speculate as to how Neoverse-N3
> behaves.
>
> Mentioning that the breakage was observed on Neoverse-N3 is still useful
> to add to the changelog.
>
> > I have minor question here: The DDI0487L C3.2.6 (Single-copy atomic
> > 64-byte load/store) mentioned
> >
> > """
> > When the instructions access a memory type that is not one of the
> > following, a data abort for unsupported Exclusive or atomic access is
> > generated:
> >
> > • Normal Inner Non-cacheable, Outer Non-cacheable.
> > """
> >
> > So, the above is the "Normal Inner Non-cacheable", but in our case we
> > have "Normal and non-shareable" in stage-1 mapping, right? I know it
> > is very close, but it seems the situation is still only "one bit" away
> > in my understanding...
>
> This citation relates to FEAT_LS64. If you look at B2.2.6 instead, it
> reads:
>
> """
> The memory types for which it is architecturally guaranteed that the
> atomic instructions will be atomic are:
>
>  - Inner Shareable, Inner Write-Back, Outer Write-Back Normal memory
>    with Read allocation hints and Write allocation hints and not
>    transient.
>
>  - Outer Shareable, Inner Write-Back, Outer Write-Back Normal memory
>    with Read allocation hints and Write allocation hints and not
>    transient.
> """

Agree that the above should be the right place to cite. C3.2.6 seems
to discuss atomic instruction with memory attributes bits(which points
to MAIR_EL1 and MAIR_EL2?). In this case, it is more related to
shareability bits instead.


>
> and
>
> """
> If the atomic insturctions are not atomic in regard to other agents that
> access memory, then performing an atomic instruction to such a location
> can have one or more of the following effects:
>
> [...]
>
>  - The instruction generates an IMPLEMENTATION DEFINED fault reported
>    using the Data Abort Fault status code of ESR_ELx.DFSC = 110101
> """
>
> The memory type used by KVM selftests is *Non-Shareable*, which is not
> one of the memory types guaranteed by the architecture to work.
>
> Thanks,
> Oliver

Re: [PATCH v2 2/2] KVM: selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[Marc Zyngier]

On Sat, 05 Apr 2025 08:24:30 +0100,
Mingwei Zhang <mizhang@google.com> wrote:
> 
> On Fri, Apr 4, 2025 at 7:51 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > On Fri, Apr 04, 2025 at 05:31:49PM -0700, Mingwei Zhang wrote:
> > > On Fri, Apr 4, 2025 at 5:10 PM Raghavendra Rao Ananta
> > > <rananta@google.com> wrote:
> > > >
> > > > Atomic instructions such as 'ldset' over (global) variables in the guest
> > > > is observed to cause an EL1 data abort with FSC 0x35 (IMPLEMENTATION
> > > > DEFINED fault (Unsupported Exclusive or Atomic access)). The observation
> > > > was particularly apparent on Neoverse-N3.
> > > >
> > > > According to ARM ARM DDI0487L.a B2.2.6 (Possible implementation
> > > > restrictions on using atomic instructions), atomic instructions are
> > > > architecturally guaranteed for Inner Shareable and Outer Shareable
> > > > attributes. For Non-Shareable attribute, the atomic instructions are
> > > > not atomic and issuing such an instruction can lead to the FSC
> > > > mentioned in this case (among other things).
> > > >
> > > > Moreover, according to DDI0487L.a C3.2.6 (Single-copy atomic 64-byte
> > > > load/store), it is implementation defined that a data abort with the
> > > > mentioned FSC is reported for the first stage of translation that
> > > > provides an inappropriate memory type. It's likely that Neoverse-N3
> > > > chose to implement these two and why we see an FSC of 0x35 in EL1 upon
> > > > executing atomic instructions.
> >
> > Ok, can we please drop this second reference?
> >
> > This is talking about something else (FEAT_LS64) that happens to share
> > the same FSC as an unsupported atomic instruction. I mentioned this to
> > you internally as an illustration of how different implementations may
> > behave when determining if the attributes support a particular access,
> > but it isn't actually relevant to this change.
> >
> > > nit: It's likely that Neoverse-N3 chose to implement this option (the
> > > first option) instead of reporting at the final enabled stage of
> > > translation
> >
> > I would much rather we rely on the language that describes what the
> > architecture guarantees rather than speculate as to how Neoverse-N3
> > behaves.
> >
> > Mentioning that the breakage was observed on Neoverse-N3 is still useful
> > to add to the changelog.
> >
> > > I have minor question here: The DDI0487L C3.2.6 (Single-copy atomic
> > > 64-byte load/store) mentioned
> > >
> > > """
> > > When the instructions access a memory type that is not one of the
> > > following, a data abort for unsupported Exclusive or atomic access is
> > > generated:
> > >
> > > • Normal Inner Non-cacheable, Outer Non-cacheable.
> > > """
> > >
> > > So, the above is the "Normal Inner Non-cacheable", but in our case we
> > > have "Normal and non-shareable" in stage-1 mapping, right? I know it
> > > is very close, but it seems the situation is still only "one bit" away
> > > in my understanding...
> >
> > This citation relates to FEAT_LS64. If you look at B2.2.6 instead, it
> > reads:
> >
> > """
> > The memory types for which it is architecturally guaranteed that the
> > atomic instructions will be atomic are:
> >
> >  - Inner Shareable, Inner Write-Back, Outer Write-Back Normal memory
> >    with Read allocation hints and Write allocation hints and not
> >    transient.
> >
> >  - Outer Shareable, Inner Write-Back, Outer Write-Back Normal memory
> >    with Read allocation hints and Write allocation hints and not
> >    transient.
> > """
> 
> Agree that the above should be the right place to cite. C3.2.6 seems
> to discuss atomic instruction with memory attributes bits(which points
> to MAIR_EL1 and MAIR_EL2?). In this case, it is more related to
> shareability bits instead.

Not sure what you mean by this reference to MAIR.

These constraints apply to any memory access, including those that are
*not* the direct effect of an instruction.  For example, an atomic
page table update generated by the HW (AF or DB update) is only
guaranteed to succeed if the PTW is itself configured to use ISH+WB or
OSH+WB, and could otherwise result in any of the ill effects described
in B2.2.6.

	M.

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

Re: [PATCH v2 0/2] KVM : selftests: arm64: Explicitly set the page attrs to Inner-Shareable

[Oliver Upton]

On Sat, 05 Apr 2025 00:10:40 +0000, Raghavendra Rao Ananta wrote:
> The series fixes a conflict in memory attributes in some
> implementations,
> such as Neoverse-N3, that causes a data abort in guest EL1 with FSC
> 0x35 (IMPLEMENTATION DEFINED fault (Unsupported Exclusive or Atomic
> access)).
> 
> Patch-1 is a cleanup patch that replaces numbers (and comments) to
> using proper macros for hardware configuration, such as registers and
> page-table entries.
> 
> [...]

I cleaned up the changelog on patch 2 per the discussion + added a note
about the interaction with LPA2.

Applied to fixes, thanks!

[1/2] KVM: selftests: arm64: Introduce and use hardware-definition macros
      https://git.kernel.org/kvmarm/kvmarm/c/d8d78398e550
[2/2] KVM: selftests: arm64: Explicitly set the page attrs to Inner-Shareable
      https://git.kernel.org/kvmarm/kvmarm/c/c8631ea59b65

--
Best,
Oliver