[kvm-unit-tests PATCH v1 0/7] arm64: support EL2

[kvm-unit-tests PATCH v1 0/7] arm64: support EL2

[Joey Gouly]

Hi all,

This series is for adding support to running the kvm-unit-tests at EL2. These
have been tested with Marc Zyngier's Linux kvm-arm64/nv-next branch [1] and
kvmtool branch arm64/nv-6.13 [2]

The goal is to later extend and add new tests for Nested Virtualisation,
however they should also work with bare metal as well.

- The EFI/ACPI change has not been tested yet.
- The PMU tests don't all work in NV, a patch has been submitted that fixes it
  [3].
- The debug tests fail in NV
- micro-bench ipi test fails in 'bare metal' QEMU
- PMU pmu-mem-access, pmu-chain-promotion, pmu-overflow-interrupt fail
  on FVP, but fail when run from EL1 already. So not an EL2 issue.

I will continue to debug the above, but wanted to send this series out to make
some progress. We could even drop the last patch (actually enabling EL2), and
merge the rest, if people don't want to have some tests broken.

This is a continuation/reworking of Alexandru's patches at [4].

Thanks,

Joey

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/nv-next (commit a35d752b17f4)
[2] https://git.kernel.org/pub/scm/linux/kernel/git/maz/kvmtool.git arm64/nv-6.13 (commit 5b6fe295ea7)
[3] https://lore.kernel.org/kvmarm/20250217112412.3963324-1-maz@kernel.org/T/#t
[4] https://lore.kernel.org/all/1577972806-16184-1-git-send-email-alexandru.elisei@arm.com/


Joey Gouly (7):
  arm64: drop to EL1 if booted at EL2
  arm64: timer: use hypervisor timers when at EL2
  arm64: micro-bench: fix timer IRQ
  arm64: micro-bench: use smc when at EL2
  arm64: selftest: update test for running at EL2
  arm64: pmu: count EL2 cycles
  arm64: run at EL2 if supported

 arm/cstart64.S         | 55 ++++++++++++++++++++++++++++++++++++++++--
 arm/micro-bench.c      | 26 +++++++++++++++++---
 arm/pmu.c              | 21 +++++++++++++---
 arm/selftest.c         | 18 ++++++++++----
 arm/timer.c            | 10 ++++++--
 lib/acpi.h             |  2 ++
 lib/arm/asm/timer.h    | 11 +++++++++
 lib/arm/timer.c        | 19 +++++++++++++--
 lib/arm64/asm/sysreg.h |  5 ++++
 9 files changed, 150 insertions(+), 17 deletions(-)

-- 
2.25.1

[kvm-unit-tests PATCH v1 1/7] arm64: drop to EL1 if booted at EL2

[Joey Gouly]

EL2 is not currently supported, drop to EL1 to conitnue booting.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 arm/cstart64.S | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/arm/cstart64.S b/arm/cstart64.S
index b480a552..3a305ad0 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -57,14 +57,25 @@ start:
 	add     x6, x6, :lo12:reloc_end
 1:
 	cmp	x5, x6
-	b.hs	1f
+	b.hs	reloc_done
 	ldr	x7, [x5]			// r_offset
 	ldr	x8, [x5, #16]			// r_addend
 	add	x8, x8, x4			// val = base + r_addend
 	str	x8, [x4, x7]			// base[r_offset] = val
 	add	x5, x5, #24
 	b	1b
-
+reloc_done:
+	mrs	x4, CurrentEL
+	cmp	x4, CurrentEL_EL2
+	b.ne	1f
+drop_to_el1:
+	mov	x4, 4
+	msr	spsr_el2, x4
+	adrp	x4, 1f
+	add	x4, x4, :lo12:1f
+	msr	elr_el2, x4
+	isb
+	eret
 1:
 	/* zero BSS */
 	adrp	x4, bss
@@ -186,6 +197,18 @@ get_mmu_off:
 
 .globl secondary_entry
 secondary_entry:
+	mrs	x0, CurrentEL
+	cmp	x0, CurrentEL_EL2
+	b.ne	1f
+drop_to_el1_secondary:
+	mov	x0, 4
+	msr	spsr_el2, x0
+	adrp	x0, 1f
+	add	x0, x0, :lo12:1f
+	msr	elr_el2, x0
+	isb
+	eret
+1:
 	/* enable FP/ASIMD and SVE */
 	mov	x0, #(3 << 20)
 	orr	x0, x0, #(3 << 16)
-- 
2.25.1

[kvm-unit-tests PATCH v1 2/7] arm64: timer: use hypervisor timers when at EL2

[Joey Gouly]

At EL2, with VHE:
  - CNTP_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0
  - CNTV_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0

Save the hypervisor physical and virtual timer IRQ numbers from the DT/ACPI.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 arm/timer.c         | 10 ++++++++--
 lib/acpi.h          |  2 ++
 lib/arm/asm/timer.h | 11 +++++++++++
 lib/arm/timer.c     | 19 +++++++++++++++++--
 4 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 2cb80518..c6287ca7 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -347,8 +347,14 @@ static void test_ptimer(void)
 static void test_init(void)
 {
 	assert(TIMER_PTIMER_IRQ != -1 && TIMER_VTIMER_IRQ != -1);
-	ptimer_info.irq = TIMER_PTIMER_IRQ;
-	vtimer_info.irq = TIMER_VTIMER_IRQ;
+	if (current_level() == CurrentEL_EL1) {
+		ptimer_info.irq = TIMER_PTIMER_IRQ;
+		vtimer_info.irq = TIMER_VTIMER_IRQ;
+	} else {
+		assert(TIMER_HPTIMER_IRQ != -1 && TIMER_HVTIMER_IRQ != -1);
+		ptimer_info.irq = TIMER_HPTIMER_IRQ;
+		vtimer_info.irq = TIMER_HVTIMER_IRQ;
+	}
 
 	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
 	ptimer_info.read_ctl();
diff --git a/lib/acpi.h b/lib/acpi.h
index c330c877..66e3062d 100644
--- a/lib/acpi.h
+++ b/lib/acpi.h
@@ -290,6 +290,8 @@ struct acpi_table_gtdt {
 	u64 counter_read_block_address;
 	u32 platform_timer_count;
 	u32 platform_timer_offset;
+	u32 virtual_el2_timer_interrupt;
+	u32 virtual_el2_timer_flags;
 };
 
 /* Reset to default packing */
diff --git a/lib/arm/asm/timer.h b/lib/arm/asm/timer.h
index aaf839fc..7dda0f4f 100644
--- a/lib/arm/asm/timer.h
+++ b/lib/arm/asm/timer.h
@@ -21,12 +21,23 @@ struct timer_state {
 		u32 irq;
 		u32 irq_flags;
 	} vtimer;
+	struct {
+		u32 irq;
+		u32 irq_flags;
+	} hptimer;
+	struct {
+		u32 irq;
+		u32 irq_flags;
+	} hvtimer;
 };
 extern struct timer_state __timer_state;
 
 #define TIMER_PTIMER_IRQ (__timer_state.ptimer.irq)
 #define TIMER_VTIMER_IRQ (__timer_state.vtimer.irq)
 
+#define TIMER_HPTIMER_IRQ (__timer_state.hptimer.irq)
+#define TIMER_HVTIMER_IRQ (__timer_state.hvtimer.irq)
+
 void timer_save_state(void);
 
 #endif /* !__ASSEMBLY__ */
diff --git a/lib/arm/timer.c b/lib/arm/timer.c
index ae702e41..57f504e2 100644
--- a/lib/arm/timer.c
+++ b/lib/arm/timer.c
@@ -38,10 +38,11 @@ static void timer_save_state_fdt(void)
 	 *      secure timer irq
 	 *      non-secure timer irq            (ptimer)
 	 *      virtual timer irq               (vtimer)
-	 *      hypervisor timer irq
+	 *      hypervisor timer irq            (hptimer)
+	 *      hypervisor virtual timer irq    (hvtimer)
 	 */
 	prop = fdt_get_property(fdt, node, "interrupts", &len);
-	assert(prop && len == (4 * 3 * sizeof(u32)));
+	assert(prop && len >= (4 * 3 * sizeof(u32)));
 
 	data = (u32 *) prop->data;
 	assert(fdt32_to_cpu(data[3]) == 1 /* PPI */ );
@@ -50,6 +51,14 @@ static void timer_save_state_fdt(void)
 	assert(fdt32_to_cpu(data[6]) == 1 /* PPI */ );
 	__timer_state.vtimer.irq = PPI(fdt32_to_cpu(data[7]));
 	__timer_state.vtimer.irq_flags = fdt32_to_cpu(data[8]);
+	if (len == (5 * 3 * sizeof(u32))) {
+		assert(fdt32_to_cpu(data[9]) == 1 /* PPI */ );
+		__timer_state.hptimer.irq = PPI(fdt32_to_cpu(data[10]));
+		__timer_state.hptimer.irq_flags = fdt32_to_cpu(data[11]);
+		assert(fdt32_to_cpu(data[12]) == 1 /* PPI */ );
+		__timer_state.hvtimer.irq = PPI(fdt32_to_cpu(data[13]));
+		__timer_state.hvtimer.irq_flags = fdt32_to_cpu(data[14]);
+	}
 }
 
 #ifdef CONFIG_EFI
@@ -72,6 +81,12 @@ static void timer_save_state_acpi(void)
 
 	__timer_state.vtimer.irq = gtdt->virtual_timer_interrupt;
 	__timer_state.vtimer.irq_flags = gtdt->virtual_timer_flags;
+
+	__timer_state.hptimer.irq = gtdt->non_secure_el2_interrupt;
+	__timer_state.hptimer.irq_flags = gtdt->non_secure_el2_flags;
+
+	__timer_state.hvtimer.irq = gtdt->virtual_el2_timer_interrupt;
+	__timer_state.hvtimer.irq_flags = gtdt->virtual_el2_timer_flags;
 }
 
 #else
-- 
2.25.1

[kvm-unit-tests PATCH v1 3/7] arm64: micro-bench: fix timer IRQ

[Joey Gouly]

Enable the correct (hvtimer) IRQ when at EL2.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 arm/micro-bench.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index 22408955..f47c5fc1 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -42,7 +42,7 @@ static void gic_irq_handler(struct pt_regs *regs)
 	irq_received = true;
 	gic_write_eoir(irqstat);
 
-	if (irqstat == TIMER_VTIMER_IRQ) {
+	if (irqstat == TIMER_VTIMER_IRQ || irqstat == TIMER_HVTIMER_IRQ) {
 		write_sysreg((ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE),
 			     cntv_ctl_el0);
 		isb();
@@ -215,7 +215,11 @@ static bool timer_prep(void)
 	install_irq_handler(EL1H_IRQ, gic_irq_handler);
 	local_irq_enable();
 
-	gic_enable_irq(TIMER_VTIMER_IRQ);
+	if (current_level() == CurrentEL_EL1)
+		gic_enable_irq(TIMER_VTIMER_IRQ);
+	else
+		gic_enable_irq(TIMER_HVTIMER_IRQ);
+
 	write_sysreg(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE, cntv_ctl_el0);
 	isb();
 
-- 
2.25.1

[kvm-unit-tests PATCH v1 4/7] arm64: micro-bench: use smc when at EL2

[Joey Gouly]

At EL2, hvc would target the current EL, use smc so that it targets EL3.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 arm/micro-bench.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index f47c5fc1..32029c5a 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -282,6 +282,11 @@ static bool mmio_read_user_prep(void)
 	return true;
 }
 
+static void smc_exec(void)
+{
+       asm volatile("mov w0, #0x4b000000; smc #0" ::: "w0");
+}
+
 static void mmio_read_user_exec(void)
 {
 	readl(userspace_emulated_addr);
@@ -300,6 +305,8 @@ static void eoi_exec(void)
 	write_eoir(spurious_id);
 }
 
+static bool exec_select(void);
+
 struct exit_test {
 	const char *name;
 	bool (*prep)(void);
@@ -310,7 +317,7 @@ struct exit_test {
 };
 
 static struct exit_test tests[] = {
-	{"hvc",			NULL,			hvc_exec,		NULL,		65536,		true},
+	{"hyp_call",		exec_select,		hvc_exec,		NULL,		65536,		true},
 	{"mmio_read_user",	mmio_read_user_prep,	mmio_read_user_exec,	NULL,		65536,		true},
 	{"mmio_read_vgic",	NULL,			mmio_read_vgic_exec,	NULL,		65536,		true},
 	{"eoi",			NULL,			eoi_exec,		NULL,		65536,		true},
@@ -320,6 +327,15 @@ static struct exit_test tests[] = {
 	{"timer_10ms",		timer_prep,		timer_exec,		timer_post,	256,		true},
 };
 
+static bool exec_select(void)
+{
+       if (current_level() == CurrentEL_EL2)
+               tests[0].exec = &smc_exec;
+       else
+               tests[0].exec = &hvc_exec;
+        return true;
+}
+
 struct ns_time {
 	uint64_t ns;
 	uint64_t ns_frac;
-- 
2.25.1

[kvm-unit-tests PATCH v1 5/7] arm64: selftest: update test for running at EL2

[Joey Gouly]

Remove some hard-coded assumptions that this test is running at EL1.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 arm/selftest.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arm/selftest.c b/arm/selftest.c
index 1553ed8e..eccdc3d4 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -232,6 +232,7 @@ static void user_psci_system_off(struct pt_regs *regs)
 	__user_psci_system_off();
 }
 #elif defined(__aarch64__)
+static unsigned long expected_level;
 
 /*
  * Capture the current register state and execute an instruction
@@ -276,8 +277,7 @@ static bool check_regs(struct pt_regs *regs)
 {
 	unsigned i;
 
-	/* exception handlers should always run in EL1 */
-	if (current_level() != CurrentEL_EL1)
+	if (current_level() != expected_level)
 		return false;
 
 	for (i = 0; i < ARRAY_SIZE(regs->regs); ++i) {
@@ -301,7 +301,11 @@ static enum vector check_vector_prep(void)
 		return EL0_SYNC_64;
 
 	asm volatile("mrs %0, daif" : "=r" (daif) ::);
-	expected_regs.pstate = daif | PSR_MODE_EL1h;
+        expected_regs.pstate = daif;
+        if (current_level() == CurrentEL_EL1)
+                expected_regs.pstate |= PSR_MODE_EL1h;
+        else
+                expected_regs.pstate |= PSR_MODE_EL2h;
 	return EL1H_SYNC;
 }
 
@@ -317,8 +321,8 @@ static bool check_und(void)
 
 	install_exception_handler(v, ESR_EL1_EC_UNKNOWN, unknown_handler);
 
-	/* try to read an el2 sysreg from el0/1 */
-	test_exception("", "mrs x0, sctlr_el2", "", "x0");
+	/* try to read an el3 sysreg from el0/1/2 */
+	test_exception("", "mrs x0, sctlr_el3", "", "x0");
 
 	install_exception_handler(v, ESR_EL1_EC_UNKNOWN, NULL);
 
@@ -429,6 +433,10 @@ int main(int argc, char **argv)
 	if (argc < 2)
 		report_abort("no test specified");
 
+#if defined(__aarch64__)
+        expected_level = current_level();
+#endif
+
 	report_prefix_push(argv[1]);
 
 	if (strcmp(argv[1], "setup") == 0) {
-- 
2.25.1

[kvm-unit-tests PATCH v1 6/7] arm64: pmu: count EL2 cycles

[Joey Gouly]

Count EL2 cycles if that's the EL kvm-unit-tests is running at!

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 arm/pmu.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index 2dc0822b..238e4628 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -206,6 +206,8 @@ static void test_overflow_interrupt(bool overflow_at_64bits) {}
 #define ID_DFR0_PMU_V3_8_5	0b0110
 #define ID_DFR0_PMU_IMPDEF	0b1111
 
+#define PMCCFILTR_EL0_NSH	BIT(27)
+
 static inline uint32_t get_id_aa64dfr0(void) { return read_sysreg(id_aa64dfr0_el1); }
 static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
 static inline void set_pmcr(uint32_t v) { write_sysreg(v, pmcr_el0); }
@@ -247,7 +249,7 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
 #define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
 
 #define PMEVTYPER_EXCLUDE_EL1 BIT(31)
-#define PMEVTYPER_EXCLUDE_EL0 BIT(30)
+#define PMEVTYPER_EXCLUDE_EL0 BIT(30) | BIT(27)
 
 static bool is_event_supported(uint32_t n, bool warn)
 {
@@ -1059,11 +1061,18 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 static bool check_cycles_increase(void)
 {
 	bool success = true;
+	u64 pmccfiltr = 0;
 
 	/* init before event access, this test only cares about cycle count */
 	pmu_reset();
 	set_pmcntenset(1 << PMU_CYCLE_IDX);
-	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
+
+#if defined(__aarch64__)
+	if (current_level() == CurrentEL_EL2)
+		// include EL2 cycle counts
+		pmccfiltr |= PMCCFILTR_EL0_NSH;
+#endif
+	set_pmccfiltr(pmccfiltr);
 
 	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
 	isb();
@@ -1114,11 +1123,17 @@ static void measure_instrs(int num, uint32_t pmcr)
 static bool check_cpi(int cpi)
 {
 	uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
+	u64 pmccfiltr = 0;
 
 	/* init before event access, this test only cares about cycle count */
 	pmu_reset();
 	set_pmcntenset(1 << PMU_CYCLE_IDX);
-	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
+#if defined(__aarch64__)
+	if (current_level() == CurrentEL_EL2)
+		// include EL2 cycle counts
+		pmccfiltr |= PMCCFILTR_EL0_NSH;
+#endif
+	set_pmccfiltr(pmccfiltr);
 
 	if (cpi > 0)
 		printf("Checking for CPI=%d.\n", cpi);
-- 
2.25.1

[kvm-unit-tests PATCH v1 7/7] arm64: run at EL2 if supported

[Joey Gouly]

If VHE is supported, continue booting at EL2, otherwise continue booting at
EL1.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 arm/cstart64.S         | 28 ++++++++++++++++++++++++++++
 lib/arm64/asm/sysreg.h |  5 +++++
 2 files changed, 33 insertions(+)

diff --git a/arm/cstart64.S b/arm/cstart64.S
index 3a305ad0..2a15c03d 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -68,6 +68,20 @@ reloc_done:
 	mrs	x4, CurrentEL
 	cmp	x4, CurrentEL_EL2
 	b.ne	1f
+	/* EL2 setup */
+	mrs	x4, mpidr_el1
+	msr	vmpidr_el2, x4
+	mrs	x4, midr_el1
+	msr	vpidr_el2, x4
+	/* check VHE is supported */
+	mrs	x4, ID_AA64MMFR1_EL1
+	ubfx	x4, x4, ID_AA64MMFR1_EL1_VH_SHIFT, #4
+	cmp	x4, #0
+	b.eq	drop_to_el1
+	ldr	x4, =(HCR_EL2_TGE | HCR_EL2_E2H)
+	msr	hcr_el2, x4
+	isb
+	b	1f
 drop_to_el1:
 	mov	x4, 4
 	msr	spsr_el2, x4
@@ -200,6 +214,20 @@ secondary_entry:
 	mrs	x0, CurrentEL
 	cmp	x0, CurrentEL_EL2
 	b.ne	1f
+	/* EL2 setup */
+	mrs	x0, mpidr_el1
+	msr	vmpidr_el2, x0
+	mrs	x0, midr_el1
+	msr	vpidr_el2, x0
+	/* check VHE is supported */
+	mrs	x0, ID_AA64MMFR1_EL1
+	ubfx	x0, x0, ID_AA64MMFR1_EL1_VH_SHIFT, #4
+	cmp	x0, #0
+	b.eq	drop_to_el1
+	ldr	x0, =(HCR_EL2_TGE | HCR_EL2_E2H)
+	msr	hcr_el2, x0
+	isb
+	b	1f
 drop_to_el1_secondary:
 	mov	x0, 4
 	msr	spsr_el2, x0
diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
index f214a4f0..d99ab5ec 100644
--- a/lib/arm64/asm/sysreg.h
+++ b/lib/arm64/asm/sysreg.h
@@ -75,6 +75,8 @@ asm(
 
 #define ID_AA64ISAR0_EL1_RNDR_SHIFT	60
 
+#define ID_AA64MMFR1_EL1_VH_SHIFT	8
+
 #define ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
 #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
 #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
@@ -99,6 +101,9 @@ asm(
 #define SCTLR_EL1_A		_BITULL(1)
 #define SCTLR_EL1_M		_BITULL(0)
 
+#define HCR_EL2_TGE    _BITULL(27)
+#define HCR_EL2_E2H    _BITULL(34)
+
 #define INIT_SCTLR_EL1_MMU_OFF	\
 			(SCTLR_EL1_ITD | SCTLR_EL1_SED | SCTLR_EL1_EOS | \
 			 SCTLR_EL1_TSCXT | SCTLR_EL1_EIS | SCTLR_EL1_SPAN | \
-- 
2.25.1

Re: [kvm-unit-tests PATCH v1 1/7] arm64: drop to EL1 if booted at EL2

[Marc Zyngier]

On Thu, 20 Feb 2025 14:13:48 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> EL2 is not currently supported, drop to EL1 to conitnue booting.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  arm/cstart64.S | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index b480a552..3a305ad0 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -57,14 +57,25 @@ start:
>  	add     x6, x6, :lo12:reloc_end
>  1:
>  	cmp	x5, x6
> -	b.hs	1f
> +	b.hs	reloc_done
>  	ldr	x7, [x5]			// r_offset
>  	ldr	x8, [x5, #16]			// r_addend
>  	add	x8, x8, x4			// val = base + r_addend
>  	str	x8, [x4, x7]			// base[r_offset] = val
>  	add	x5, x5, #24
>  	b	1b
> -
> +reloc_done:
> +	mrs	x4, CurrentEL
> +	cmp	x4, CurrentEL_EL2
> +	b.ne	1f
> +drop_to_el1:
> +	mov	x4, 4

It'd be nice to have a symbolic constant denoting EL1t.

> +	msr	spsr_el2, x4
> +	adrp	x4, 1f
> +	add	x4, x4, :lo12:1f
> +	msr	elr_el2, x4
> +	isb

You can drop this ISB, as the following ERET has the same context
synchronisation properties.

> +	eret
>  1:
>  	/* zero BSS */
>  	adrp	x4, bss
> @@ -186,6 +197,18 @@ get_mmu_off:
>  
>  .globl secondary_entry
>  secondary_entry:
> +	mrs	x0, CurrentEL
> +	cmp	x0, CurrentEL_EL2
> +	b.ne	1f
> +drop_to_el1_secondary:
> +	mov	x0, 4
> +	msr	spsr_el2, x0
> +	adrp	x0, 1f
> +	add	x0, x0, :lo12:1f
> +	msr	elr_el2, x0
> +	isb
> +	eret

Maybe move this into a macro and use it in the two instances of
EL2->EL1 drop.

> +1:
>  	/* enable FP/ASIMD and SVE */
>  	mov	x0, #(3 << 20)
>  	orr	x0, x0, #(3 << 16)

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [kvm-unit-tests PATCH v1 7/7] arm64: run at EL2 if supported

[Marc Zyngier]

On Thu, 20 Feb 2025 14:13:54 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> If VHE is supported, continue booting at EL2, otherwise continue booting at
> EL1.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  arm/cstart64.S         | 28 ++++++++++++++++++++++++++++
>  lib/arm64/asm/sysreg.h |  5 +++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index 3a305ad0..2a15c03d 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -68,6 +68,20 @@ reloc_done:
>  	mrs	x4, CurrentEL
>  	cmp	x4, CurrentEL_EL2
>  	b.ne	1f
> +	/* EL2 setup */
> +	mrs	x4, mpidr_el1
> +	msr	vmpidr_el2, x4
> +	mrs	x4, midr_el1
> +	msr	vpidr_el2, x4
> +	/* check VHE is supported */
> +	mrs	x4, ID_AA64MMFR1_EL1
> +	ubfx	x4, x4, ID_AA64MMFR1_EL1_VH_SHIFT, #4
> +	cmp	x4, #0
> +	b.eq	drop_to_el1

nit: you can replace these cmp/b.eq with a cbz.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [kvm-unit-tests PATCH v1 1/7] arm64: drop to EL1 if booted at EL2

[Alexandru Elisei]

Hi Joey,

On Thu, Feb 20, 2025 at 02:13:48PM +0000, Joey Gouly wrote:
> EL2 is not currently supported, drop to EL1 to conitnue booting.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  arm/cstart64.S | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index b480a552..3a305ad0 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -57,14 +57,25 @@ start:
>  	add     x6, x6, :lo12:reloc_end
>  1:
>  	cmp	x5, x6
> -	b.hs	1f
> +	b.hs	reloc_done
>  	ldr	x7, [x5]			// r_offset
>  	ldr	x8, [x5, #16]			// r_addend
>  	add	x8, x8, x4			// val = base + r_addend
>  	str	x8, [x4, x7]			// base[r_offset] = val
>  	add	x5, x5, #24
>  	b	1b
> -
> +reloc_done:
> +	mrs	x4, CurrentEL
> +	cmp	x4, CurrentEL_EL2
> +	b.ne	1f
> +drop_to_el1:
> +	mov	x4, 4
> +	msr	spsr_el2, x4
> +	adrp	x4, 1f
> +	add	x4, x4, :lo12:1f
> +	msr	elr_el2, x4

I'm going to assume this works because KVM is nice enough to initialise the
EL2 registers that affect execution at EL1 to some sane defaults. Is that
something that can be relied on going forward?

What about UEFI?

I was expecting some kind of initialization of the registers that affect
EL1.

Thanks,
Alex

> +	isb
> +	eret
>  1:
>  	/* zero BSS */
>  	adrp	x4, bss
> @@ -186,6 +197,18 @@ get_mmu_off:
>  
>  .globl secondary_entry
>  secondary_entry:
> +	mrs	x0, CurrentEL
> +	cmp	x0, CurrentEL_EL2
> +	b.ne	1f
> +drop_to_el1_secondary:
> +	mov	x0, 4
> +	msr	spsr_el2, x0
> +	adrp	x0, 1f
> +	add	x0, x0, :lo12:1f
> +	msr	elr_el2, x0
> +	isb
> +	eret
> +1:
>  	/* enable FP/ASIMD and SVE */
>  	mov	x0, #(3 << 20)
>  	orr	x0, x0, #(3 << 16)
> -- 
> 2.25.1
> 

Re: [kvm-unit-tests PATCH v1 2/7] arm64: timer: use hypervisor timers when at EL2

[Alexandru Elisei]

Hi Joey,

On Thu, Feb 20, 2025 at 02:13:49PM +0000, Joey Gouly wrote:
> At EL2, with VHE:
>   - CNTP_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0
>   - CNTV_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0

CNTH*V*_CVAL_EL0, right?

It also happens for the other two registers for the physical and virtual
timers (CNT{P,V}_{TVAL,CTL}_EL0). Just nitpicking here.

> 
> Save the hypervisor physical and virtual timer IRQ numbers from the DT/ACPI.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  arm/timer.c         | 10 ++++++++--
>  lib/acpi.h          |  2 ++
>  lib/arm/asm/timer.h | 11 +++++++++++
>  lib/arm/timer.c     | 19 +++++++++++++++++--
>  4 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/arm/timer.c b/arm/timer.c
> index 2cb80518..c6287ca7 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -347,8 +347,14 @@ static void test_ptimer(void)
>  static void test_init(void)
>  {
>  	assert(TIMER_PTIMER_IRQ != -1 && TIMER_VTIMER_IRQ != -1);
> -	ptimer_info.irq = TIMER_PTIMER_IRQ;
> -	vtimer_info.irq = TIMER_VTIMER_IRQ;
> +	if (current_level() == CurrentEL_EL1) {
> +		ptimer_info.irq = TIMER_PTIMER_IRQ;
> +		vtimer_info.irq = TIMER_VTIMER_IRQ;

You dropped the assert for TIMER_{P,V}TIMER_IRQ.

Otherwise, looks good to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

> +	} else {
> +		assert(TIMER_HPTIMER_IRQ != -1 && TIMER_HVTIMER_IRQ != -1);
> +		ptimer_info.irq = TIMER_HPTIMER_IRQ;
> +		vtimer_info.irq = TIMER_HVTIMER_IRQ;
> +	}
>  
>  	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
>  	ptimer_info.read_ctl();
> diff --git a/lib/acpi.h b/lib/acpi.h
> index c330c877..66e3062d 100644
> --- a/lib/acpi.h
> +++ b/lib/acpi.h
> @@ -290,6 +290,8 @@ struct acpi_table_gtdt {
>  	u64 counter_read_block_address;
>  	u32 platform_timer_count;
>  	u32 platform_timer_offset;
> +	u32 virtual_el2_timer_interrupt;
> +	u32 virtual_el2_timer_flags;
>  };
>  
>  /* Reset to default packing */
> diff --git a/lib/arm/asm/timer.h b/lib/arm/asm/timer.h
> index aaf839fc..7dda0f4f 100644
> --- a/lib/arm/asm/timer.h
> +++ b/lib/arm/asm/timer.h
> @@ -21,12 +21,23 @@ struct timer_state {
>  		u32 irq;
>  		u32 irq_flags;
>  	} vtimer;
> +	struct {
> +		u32 irq;
> +		u32 irq_flags;
> +	} hptimer;
> +	struct {
> +		u32 irq;
> +		u32 irq_flags;
> +	} hvtimer;
>  };
>  extern struct timer_state __timer_state;
>  
>  #define TIMER_PTIMER_IRQ (__timer_state.ptimer.irq)
>  #define TIMER_VTIMER_IRQ (__timer_state.vtimer.irq)
>  
> +#define TIMER_HPTIMER_IRQ (__timer_state.hptimer.irq)
> +#define TIMER_HVTIMER_IRQ (__timer_state.hvtimer.irq)
> +
>  void timer_save_state(void);
>  
>  #endif /* !__ASSEMBLY__ */
> diff --git a/lib/arm/timer.c b/lib/arm/timer.c
> index ae702e41..57f504e2 100644
> --- a/lib/arm/timer.c
> +++ b/lib/arm/timer.c
> @@ -38,10 +38,11 @@ static void timer_save_state_fdt(void)
>  	 *      secure timer irq
>  	 *      non-secure timer irq            (ptimer)
>  	 *      virtual timer irq               (vtimer)
> -	 *      hypervisor timer irq
> +	 *      hypervisor timer irq            (hptimer)
> +	 *      hypervisor virtual timer irq    (hvtimer)
>  	 */
>  	prop = fdt_get_property(fdt, node, "interrupts", &len);
> -	assert(prop && len == (4 * 3 * sizeof(u32)));
> +	assert(prop && len >= (4 * 3 * sizeof(u32)));
>  
>  	data = (u32 *) prop->data;
>  	assert(fdt32_to_cpu(data[3]) == 1 /* PPI */ );
> @@ -50,6 +51,14 @@ static void timer_save_state_fdt(void)
>  	assert(fdt32_to_cpu(data[6]) == 1 /* PPI */ );
>  	__timer_state.vtimer.irq = PPI(fdt32_to_cpu(data[7]));
>  	__timer_state.vtimer.irq_flags = fdt32_to_cpu(data[8]);
> +	if (len == (5 * 3 * sizeof(u32))) {
> +		assert(fdt32_to_cpu(data[9]) == 1 /* PPI */ );
> +		__timer_state.hptimer.irq = PPI(fdt32_to_cpu(data[10]));
> +		__timer_state.hptimer.irq_flags = fdt32_to_cpu(data[11]);
> +		assert(fdt32_to_cpu(data[12]) == 1 /* PPI */ );
> +		__timer_state.hvtimer.irq = PPI(fdt32_to_cpu(data[13]));
> +		__timer_state.hvtimer.irq_flags = fdt32_to_cpu(data[14]);
> +	}
>  }
>  
>  #ifdef CONFIG_EFI
> @@ -72,6 +81,12 @@ static void timer_save_state_acpi(void)
>  
>  	__timer_state.vtimer.irq = gtdt->virtual_timer_interrupt;
>  	__timer_state.vtimer.irq_flags = gtdt->virtual_timer_flags;
> +
> +	__timer_state.hptimer.irq = gtdt->non_secure_el2_interrupt;
> +	__timer_state.hptimer.irq_flags = gtdt->non_secure_el2_flags;
> +
> +	__timer_state.hvtimer.irq = gtdt->virtual_el2_timer_interrupt;
> +	__timer_state.hvtimer.irq_flags = gtdt->virtual_el2_timer_flags;
>  }
>  
>  #else
> -- 
> 2.25.1
> 

Re: [kvm-unit-tests PATCH v1 3/7] arm64: micro-bench: fix timer IRQ

[Alexandru Elisei]

Hi Joey,

On Thu, Feb 20, 2025 at 02:13:50PM +0000, Joey Gouly wrote:
> Enable the correct (hvtimer) IRQ when at EL2.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  arm/micro-bench.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index 22408955..f47c5fc1 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -42,7 +42,7 @@ static void gic_irq_handler(struct pt_regs *regs)
>  	irq_received = true;
>  	gic_write_eoir(irqstat);
>  
> -	if (irqstat == TIMER_VTIMER_IRQ) {
> +	if (irqstat == TIMER_VTIMER_IRQ || irqstat == TIMER_HVTIMER_IRQ) {
>  		write_sysreg((ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE),
>  			     cntv_ctl_el0);
>  		isb();
> @@ -215,7 +215,11 @@ static bool timer_prep(void)
>  	install_irq_handler(EL1H_IRQ, gic_irq_handler);
>  	local_irq_enable();
>  
> -	gic_enable_irq(TIMER_VTIMER_IRQ);
> +	if (current_level() == CurrentEL_EL1)
> +		gic_enable_irq(TIMER_VTIMER_IRQ);
> +	else
> +		gic_enable_irq(TIMER_HVTIMER_IRQ);
> +

Looks correct to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

>  	write_sysreg(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE, cntv_ctl_el0);
>  	isb();
>  
> -- 
> 2.25.1
> 

Re: [kvm-unit-tests PATCH v1 5/7] arm64: selftest: update test for running at EL2

[Alexandru Elisei]

Hi Joey,

On Thu, Feb 20, 2025 at 02:13:52PM +0000, Joey Gouly wrote:
> Remove some hard-coded assumptions that this test is running at EL1.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

We discussed this privately, and this patch was split from from the bigger
patch that added support to all tests to run at EL2 [1]. Joey kept my
Signed-off-by, but accidently dropped the authorship.

[1] https://lore.kernel.org/all/1577972806-16184-3-git-send-email-alexandru.elisei@arm.com/

> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  arm/selftest.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arm/selftest.c b/arm/selftest.c
> index 1553ed8e..eccdc3d4 100644
> --- a/arm/selftest.c
> +++ b/arm/selftest.c
> @@ -232,6 +232,7 @@ static void user_psci_system_off(struct pt_regs *regs)
>  	__user_psci_system_off();
>  }
>  #elif defined(__aarch64__)
> +static unsigned long expected_level;
>  
>  /*
>   * Capture the current register state and execute an instruction
> @@ -276,8 +277,7 @@ static bool check_regs(struct pt_regs *regs)
>  {
>  	unsigned i;
>  
> -	/* exception handlers should always run in EL1 */
> -	if (current_level() != CurrentEL_EL1)
> +	if (current_level() != expected_level)
>  		return false;
>  
>  	for (i = 0; i < ARRAY_SIZE(regs->regs); ++i) {
> @@ -301,7 +301,11 @@ static enum vector check_vector_prep(void)
>  		return EL0_SYNC_64;
>  
>  	asm volatile("mrs %0, daif" : "=r" (daif) ::);
> -	expected_regs.pstate = daif | PSR_MODE_EL1h;
> +        expected_regs.pstate = daif;
> +        if (current_level() == CurrentEL_EL1)
> +                expected_regs.pstate |= PSR_MODE_EL1h;
> +        else
> +                expected_regs.pstate |= PSR_MODE_EL2h;
>  	return EL1H_SYNC;
>  }
>  
> @@ -317,8 +321,8 @@ static bool check_und(void)
>  
>  	install_exception_handler(v, ESR_EL1_EC_UNKNOWN, unknown_handler);
>  
> -	/* try to read an el2 sysreg from el0/1 */
> -	test_exception("", "mrs x0, sctlr_el2", "", "x0");
> +	/* try to read an el3 sysreg from el0/1/2 */
> +	test_exception("", "mrs x0, sctlr_el3", "", "x0");
>  
>  	install_exception_handler(v, ESR_EL1_EC_UNKNOWN, NULL);
>  
> @@ -429,6 +433,10 @@ int main(int argc, char **argv)
>  	if (argc < 2)
>  		report_abort("no test specified");
>  
> +#if defined(__aarch64__)
> +        expected_level = current_level();
> +#endif
> +
>  	report_prefix_push(argv[1]);
>  
>  	if (strcmp(argv[1], "setup") == 0) {

Looks good to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

Re: [kvm-unit-tests PATCH v1 6/7] arm64: pmu: count EL2 cycles

[Alexandru Elisei]

Hi Joey,

On Thu, Feb 20, 2025 at 02:13:53PM +0000, Joey Gouly wrote:
> Count EL2 cycles if that's the EL kvm-unit-tests is running at!
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  arm/pmu.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 2dc0822b..238e4628 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -206,6 +206,8 @@ static void test_overflow_interrupt(bool overflow_at_64bits) {}
>  #define ID_DFR0_PMU_V3_8_5	0b0110
>  #define ID_DFR0_PMU_IMPDEF	0b1111
>  
> +#define PMCCFILTR_EL0_NSH	BIT(27)
> +
>  static inline uint32_t get_id_aa64dfr0(void) { return read_sysreg(id_aa64dfr0_el1); }
>  static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
>  static inline void set_pmcr(uint32_t v) { write_sysreg(v, pmcr_el0); }
> @@ -247,7 +249,7 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  #define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
>  
>  #define PMEVTYPER_EXCLUDE_EL1 BIT(31)

I think you can drop the macro. I was expecting to see an exclude EL2 macro
used in its place when the test is running at EL2, but it seems
PMEVTYPER_EXCLUDE_EL1 is not used anywhere. Unless I'm missing something here.

> -#define PMEVTYPER_EXCLUDE_EL0 BIT(30)
> +#define PMEVTYPER_EXCLUDE_EL0 BIT(30) | BIT(27)

I'm not really sure what that's supposed to achieve - if the test is
running at EL2, and events from both EL0 and EL2 are excluded, what's left
to count?

I also don't understand what PMEVTYPER_EXCLUDE_EL0 does in the non-nested
virt case (when kvm-unit-tests boots at El1). The tests run at EL1, so not
counting events at EL0 shouldn't affect anything. Am I missing something
obvious here?

>  
>  static bool is_event_supported(uint32_t n, bool warn)
>  {
> @@ -1059,11 +1061,18 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>  static bool check_cycles_increase(void)
>  {
>  	bool success = true;
> +	u64 pmccfiltr = 0;
>  
>  	/* init before event access, this test only cares about cycle count */
>  	pmu_reset();
>  	set_pmcntenset(1 << PMU_CYCLE_IDX);
> -	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> +
> +#if defined(__aarch64__)
> +	if (current_level() == CurrentEL_EL2)
> +		// include EL2 cycle counts
> +		pmccfiltr |= PMCCFILTR_EL0_NSH;
> +#endif
> +	set_pmccfiltr(pmccfiltr);
>  
>  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
>  	isb();
> @@ -1114,11 +1123,17 @@ static void measure_instrs(int num, uint32_t pmcr)
>  static bool check_cpi(int cpi)
>  {
>  	uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
> +	u64 pmccfiltr = 0;
>  
>  	/* init before event access, this test only cares about cycle count */
>  	pmu_reset();
>  	set_pmcntenset(1 << PMU_CYCLE_IDX);
> -	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> +#if defined(__aarch64__)
> +	if (current_level() == CurrentEL_EL2)
> +		// include EL2 cycle counts
> +		pmccfiltr |= PMCCFILTR_EL0_NSH;
> +#endif

It's called twice, so it could be abstracted in a function.

Also, I find it interesting that for PMCCFILTR_EL0 you set the NSH bit
based on current exception level, but for PMEVTYPER you set it
unconditionally. Why the different approaches? For convenience of is there
something more?

Thanks,
Alex

Re: [kvm-unit-tests PATCH v1 6/7] arm64: pmu: count EL2 cycles

[Joey Gouly]

Hi Alexandru,

On Thu, Feb 27, 2025 at 05:01:03PM +0000, Alexandru Elisei wrote:
> Hi Joey,
> 
> On Thu, Feb 20, 2025 at 02:13:53PM +0000, Joey Gouly wrote:
> > Count EL2 cycles if that's the EL kvm-unit-tests is running at!
> > 
> > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > ---
> >  arm/pmu.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 2dc0822b..238e4628 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -206,6 +206,8 @@ static void test_overflow_interrupt(bool overflow_at_64bits) {}
> >  #define ID_DFR0_PMU_V3_8_5	0b0110
> >  #define ID_DFR0_PMU_IMPDEF	0b1111
> >  
> > +#define PMCCFILTR_EL0_NSH	BIT(27)
> > +
> >  static inline uint32_t get_id_aa64dfr0(void) { return read_sysreg(id_aa64dfr0_el1); }
> >  static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
> >  static inline void set_pmcr(uint32_t v) { write_sysreg(v, pmcr_el0); }
> > @@ -247,7 +249,7 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
> >  #define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
> >  
> >  #define PMEVTYPER_EXCLUDE_EL1 BIT(31)
> 
> I think you can drop the macro. I was expecting to see an exclude EL2 macro
> used in its place when the test is running at EL2, but it seems
> PMEVTYPER_EXCLUDE_EL1 is not used anywhere. Unless I'm missing something here.

I will drop it.

> 
> > -#define PMEVTYPER_EXCLUDE_EL0 BIT(30)
> > +#define PMEVTYPER_EXCLUDE_EL0 BIT(30) | BIT(27)
> 
> I'm not really sure what that's supposed to achieve - if the test is
> running at EL2, and events from both EL0 and EL2 are excluded, what's left
> to count?

bit 27 of PMEVTYPER is the NSH bit, when it is 1 the events are not filtered.
This is the opposite polarity to the U (bit 30) and P (bit 31) bits. When they
are 1, the events are filtered.

> 
> I also don't understand what PMEVTYPER_EXCLUDE_EL0 does in the non-nested
> virt case (when kvm-unit-tests boots at El1). The tests run at EL1, so not
> counting events at EL0 shouldn't affect anything. Am I missing something
> obvious here?

I don't know why it's like that. AFAICT, it doesn't run anything at EL0.

> 
> >  
> >  static bool is_event_supported(uint32_t n, bool warn)
> >  {
> > @@ -1059,11 +1061,18 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> >  static bool check_cycles_increase(void)
> >  {
> >  	bool success = true;
> > +	u64 pmccfiltr = 0;
> >  
> >  	/* init before event access, this test only cares about cycle count */
> >  	pmu_reset();
> >  	set_pmcntenset(1 << PMU_CYCLE_IDX);
> > -	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> > +
> > +#if defined(__aarch64__)
> > +	if (current_level() == CurrentEL_EL2)
> > +		// include EL2 cycle counts
> > +		pmccfiltr |= PMCCFILTR_EL0_NSH;
> > +#endif
> > +	set_pmccfiltr(pmccfiltr);
> >  
> >  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> >  	isb();
> > @@ -1114,11 +1123,17 @@ static void measure_instrs(int num, uint32_t pmcr)
> >  static bool check_cpi(int cpi)
> >  {
> >  	uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
> > +	u64 pmccfiltr = 0;
> >  
> >  	/* init before event access, this test only cares about cycle count */
> >  	pmu_reset();
> >  	set_pmcntenset(1 << PMU_CYCLE_IDX);
> > -	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> > +#if defined(__aarch64__)
> > +	if (current_level() == CurrentEL_EL2)
> > +		// include EL2 cycle counts
> > +		pmccfiltr |= PMCCFILTR_EL0_NSH;
> > +#endif
> 
> It's called twice, so it could be abstracted in a function.
> 
> Also, I find it interesting that for PMCCFILTR_EL0 you set the NSH bit
> based on current exception level, but for PMEVTYPER you set it
> unconditionally. Why the different approaches? For convenience of is there
> something more?

No real reason. I don't really know about the PMU, just did some changes to get
this test working.  I'll look into if setting NSH always is fine too, then I'll
unconditionally do it.

Thanks,
Joey

Re: [kvm-unit-tests PATCH v1 1/7] arm64: drop to EL1 if booted at EL2

[Joey Gouly]

Hi,

On Thu, Feb 27, 2025 at 04:53:15PM +0000, Alexandru Elisei wrote:
> Hi Joey,
> 
> On Thu, Feb 20, 2025 at 02:13:48PM +0000, Joey Gouly wrote:
> > EL2 is not currently supported, drop to EL1 to conitnue booting.
> > 
> > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > ---
> >  arm/cstart64.S | 27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > index b480a552..3a305ad0 100644
> > --- a/arm/cstart64.S
> > +++ b/arm/cstart64.S
> > @@ -57,14 +57,25 @@ start:
> >  	add     x6, x6, :lo12:reloc_end
> >  1:
> >  	cmp	x5, x6
> > -	b.hs	1f
> > +	b.hs	reloc_done
> >  	ldr	x7, [x5]			// r_offset
> >  	ldr	x8, [x5, #16]			// r_addend
> >  	add	x8, x8, x4			// val = base + r_addend
> >  	str	x8, [x4, x7]			// base[r_offset] = val
> >  	add	x5, x5, #24
> >  	b	1b
> > -
> > +reloc_done:
> > +	mrs	x4, CurrentEL
> > +	cmp	x4, CurrentEL_EL2
> > +	b.ne	1f
> > +drop_to_el1:
> > +	mov	x4, 4
> > +	msr	spsr_el2, x4
> > +	adrp	x4, 1f
> > +	add	x4, x4, :lo12:1f
> > +	msr	elr_el2, x4
> 
> I'm going to assume this works because KVM is nice enough to initialise the
> EL2 registers that affect execution at EL1 to some sane defaults. Is that
> something that can be relied on going forward?

I was just trying to keep the changes minimal.

> 
> What about UEFI?

Haven't tested it yet.

> 
> I was expecting some kind of initialization of the registers that affect
> EL1.

I'll look into it.

Thanks,
Joey

> 
> Thanks,
> Alex
> 
> > +	isb
> > +	eret
> >  1:
> >  	/* zero BSS */
> >  	adrp	x4, bss
> > @@ -186,6 +197,18 @@ get_mmu_off:
> >  
> >  .globl secondary_entry
> >  secondary_entry:
> > +	mrs	x0, CurrentEL
> > +	cmp	x0, CurrentEL_EL2
> > +	b.ne	1f
> > +drop_to_el1_secondary:
> > +	mov	x0, 4
> > +	msr	spsr_el2, x0
> > +	adrp	x0, 1f
> > +	add	x0, x0, :lo12:1f
> > +	msr	elr_el2, x0
> > +	isb
> > +	eret
> > +1:
> >  	/* enable FP/ASIMD and SVE */
> >  	mov	x0, #(3 << 20)
> >  	orr	x0, x0, #(3 << 16)
> > -- 
> > 2.25.1
> > 

Re: [kvm-unit-tests PATCH v1 2/7] arm64: timer: use hypervisor timers when at EL2

[Joey Gouly]

On Thu, Feb 27, 2025 at 04:55:26PM +0000, Alexandru Elisei wrote:
> Hi Joey,
> 
> On Thu, Feb 20, 2025 at 02:13:49PM +0000, Joey Gouly wrote:
> > At EL2, with VHE:
> >   - CNTP_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0
> >   - CNTV_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0
> 
> CNTH*V*_CVAL_EL0, right?

Fixed.

> 
> It also happens for the other two registers for the physical and virtual
> timers (CNT{P,V}_{TVAL,CTL}_EL0). Just nitpicking here.

I'll write it like that, thanks!

> 
> > 
> > Save the hypervisor physical and virtual timer IRQ numbers from the DT/ACPI.
> > 
> > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > ---
> >  arm/timer.c         | 10 ++++++++--
> >  lib/acpi.h          |  2 ++
> >  lib/arm/asm/timer.h | 11 +++++++++++
> >  lib/arm/timer.c     | 19 +++++++++++++++++--
> >  4 files changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arm/timer.c b/arm/timer.c
> > index 2cb80518..c6287ca7 100644
> > --- a/arm/timer.c
> > +++ b/arm/timer.c
> > @@ -347,8 +347,14 @@ static void test_ptimer(void)
> >  static void test_init(void)
> >  {
> >  	assert(TIMER_PTIMER_IRQ != -1 && TIMER_VTIMER_IRQ != -1);
It's still there ^^
> > -	ptimer_info.irq = TIMER_PTIMER_IRQ;
> > -	vtimer_info.irq = TIMER_VTIMER_IRQ;
> > +	if (current_level() == CurrentEL_EL1) {
> > +		ptimer_info.irq = TIMER_PTIMER_IRQ;
> > +		vtimer_info.irq = TIMER_VTIMER_IRQ;
> 
> You dropped the assert for TIMER_{P,V}TIMER_IRQ.

The assertion is still there!

> 
> Otherwise, looks good to me:
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks!

> 
> Thanks,
> Alex
> 
> > +	} else {
> > +		assert(TIMER_HPTIMER_IRQ != -1 && TIMER_HVTIMER_IRQ != -1);
> > +		ptimer_info.irq = TIMER_HPTIMER_IRQ;
> > +		vtimer_info.irq = TIMER_HVTIMER_IRQ;
> > +	}
> >  
> >  	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
> >  	ptimer_info.read_ctl();
> > diff --git a/lib/acpi.h b/lib/acpi.h
> > index c330c877..66e3062d 100644
> > --- a/lib/acpi.h
> > +++ b/lib/acpi.h
> > @@ -290,6 +290,8 @@ struct acpi_table_gtdt {
> >  	u64 counter_read_block_address;
> >  	u32 platform_timer_count;
> >  	u32 platform_timer_offset;
> > +	u32 virtual_el2_timer_interrupt;
> > +	u32 virtual_el2_timer_flags;
> >  };
> >  
> >  /* Reset to default packing */
> > diff --git a/lib/arm/asm/timer.h b/lib/arm/asm/timer.h
> > index aaf839fc..7dda0f4f 100644
> > --- a/lib/arm/asm/timer.h
> > +++ b/lib/arm/asm/timer.h
> > @@ -21,12 +21,23 @@ struct timer_state {
> >  		u32 irq;
> >  		u32 irq_flags;
> >  	} vtimer;
> > +	struct {
> > +		u32 irq;
> > +		u32 irq_flags;
> > +	} hptimer;
> > +	struct {
> > +		u32 irq;
> > +		u32 irq_flags;
> > +	} hvtimer;
> >  };
> >  extern struct timer_state __timer_state;
> >  
> >  #define TIMER_PTIMER_IRQ (__timer_state.ptimer.irq)
> >  #define TIMER_VTIMER_IRQ (__timer_state.vtimer.irq)
> >  
> > +#define TIMER_HPTIMER_IRQ (__timer_state.hptimer.irq)
> > +#define TIMER_HVTIMER_IRQ (__timer_state.hvtimer.irq)
> > +
> >  void timer_save_state(void);
> >  
> >  #endif /* !__ASSEMBLY__ */
> > diff --git a/lib/arm/timer.c b/lib/arm/timer.c
> > index ae702e41..57f504e2 100644
> > --- a/lib/arm/timer.c
> > +++ b/lib/arm/timer.c
> > @@ -38,10 +38,11 @@ static void timer_save_state_fdt(void)
> >  	 *      secure timer irq
> >  	 *      non-secure timer irq            (ptimer)
> >  	 *      virtual timer irq               (vtimer)
> > -	 *      hypervisor timer irq
> > +	 *      hypervisor timer irq            (hptimer)
> > +	 *      hypervisor virtual timer irq    (hvtimer)
> >  	 */
> >  	prop = fdt_get_property(fdt, node, "interrupts", &len);
> > -	assert(prop && len == (4 * 3 * sizeof(u32)));
> > +	assert(prop && len >= (4 * 3 * sizeof(u32)));
> >  
> >  	data = (u32 *) prop->data;
> >  	assert(fdt32_to_cpu(data[3]) == 1 /* PPI */ );
> > @@ -50,6 +51,14 @@ static void timer_save_state_fdt(void)
> >  	assert(fdt32_to_cpu(data[6]) == 1 /* PPI */ );
> >  	__timer_state.vtimer.irq = PPI(fdt32_to_cpu(data[7]));
> >  	__timer_state.vtimer.irq_flags = fdt32_to_cpu(data[8]);
> > +	if (len == (5 * 3 * sizeof(u32))) {
> > +		assert(fdt32_to_cpu(data[9]) == 1 /* PPI */ );
> > +		__timer_state.hptimer.irq = PPI(fdt32_to_cpu(data[10]));
> > +		__timer_state.hptimer.irq_flags = fdt32_to_cpu(data[11]);
> > +		assert(fdt32_to_cpu(data[12]) == 1 /* PPI */ );
> > +		__timer_state.hvtimer.irq = PPI(fdt32_to_cpu(data[13]));
> > +		__timer_state.hvtimer.irq_flags = fdt32_to_cpu(data[14]);
> > +	}
> >  }
> >  
> >  #ifdef CONFIG_EFI
> > @@ -72,6 +81,12 @@ static void timer_save_state_acpi(void)
> >  
> >  	__timer_state.vtimer.irq = gtdt->virtual_timer_interrupt;
> >  	__timer_state.vtimer.irq_flags = gtdt->virtual_timer_flags;
> > +
> > +	__timer_state.hptimer.irq = gtdt->non_secure_el2_interrupt;
> > +	__timer_state.hptimer.irq_flags = gtdt->non_secure_el2_flags;
> > +
> > +	__timer_state.hvtimer.irq = gtdt->virtual_el2_timer_interrupt;
> > +	__timer_state.hvtimer.irq_flags = gtdt->virtual_el2_timer_flags;
> >  }
> >  
> >  #else
> > -- 
> > 2.25.1
> > 

Re: [kvm-unit-tests PATCH v1 1/7] arm64: drop to EL1 if booted at EL2

[Alexandru Elisei]

Hi Joey,

On Tue, Mar 04, 2025 at 05:02:13PM +0000, Joey Gouly wrote:
> Hi,
> 
> On Thu, Feb 27, 2025 at 04:53:15PM +0000, Alexandru Elisei wrote:
> > Hi Joey,
> > 
> > On Thu, Feb 20, 2025 at 02:13:48PM +0000, Joey Gouly wrote:
> > > EL2 is not currently supported, drop to EL1 to conitnue booting.
> > > 
> > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > ---
> > >  arm/cstart64.S | 27 +++++++++++++++++++++++++--
> > >  1 file changed, 25 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > > index b480a552..3a305ad0 100644
> > > --- a/arm/cstart64.S
> > > +++ b/arm/cstart64.S
> > > @@ -57,14 +57,25 @@ start:
> > >  	add     x6, x6, :lo12:reloc_end
> > >  1:
> > >  	cmp	x5, x6
> > > -	b.hs	1f
> > > +	b.hs	reloc_done
> > >  	ldr	x7, [x5]			// r_offset
> > >  	ldr	x8, [x5, #16]			// r_addend
> > >  	add	x8, x8, x4			// val = base + r_addend
> > >  	str	x8, [x4, x7]			// base[r_offset] = val
> > >  	add	x5, x5, #24
> > >  	b	1b
> > > -
> > > +reloc_done:
> > > +	mrs	x4, CurrentEL
> > > +	cmp	x4, CurrentEL_EL2
> > > +	b.ne	1f
> > > +drop_to_el1:
> > > +	mov	x4, 4
> > > +	msr	spsr_el2, x4
> > > +	adrp	x4, 1f
> > > +	add	x4, x4, :lo12:1f
> > > +	msr	elr_el2, x4
> > 
> > I'm going to assume this works because KVM is nice enough to initialise the
> > EL2 registers that affect execution at EL1 to some sane defaults. Is that
> > something that can be relied on going forward?
> 
> I was just trying to keep the changes minimal.

Sure, I appreciate that, and the series looks nice and small because of it, but
in the absence of an ABI from KVM, the reset values for the EL2 registers that
affect execution at EL1 are just an implemention choice that KVM made at a
particular point in time. I don't think robust software should rely on it.

> 
> > 
> > What about UEFI?
> 
> Haven't tested it yet.

Ok, I saw you made changes to the UEFI code so I had assumed to tested them.

> 
> > 
> > I was expecting some kind of initialization of the registers that affect
> > EL1.
> 
> I'll look into it.

Great, thanks. I think arch/arm64/incluse/asm/el2_setup.h might be a useful
starting point. You can put the initialization code in a macro and use it
for the primary and secondaries CPUs, like Marc suggested.

Thanks,
Alex

Re: [kvm-unit-tests PATCH v1 2/7] arm64: timer: use hypervisor timers when at EL2

[Alexandru Elisei]

Hi Joey,

On Tue, Mar 04, 2025 at 05:05:03PM +0000, Joey Gouly wrote:
> On Thu, Feb 27, 2025 at 04:55:26PM +0000, Alexandru Elisei wrote:
> > Hi Joey,
> > 
> > On Thu, Feb 20, 2025 at 02:13:49PM +0000, Joey Gouly wrote:
> > > At EL2, with VHE:
> > >   - CNTP_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0
> > >   - CNTV_CVAL_EL0 is forwarded to CNTHP_CVAL_EL0
> > 
> > CNTH*V*_CVAL_EL0, right?
> 
> Fixed.
> 
> > 
> > It also happens for the other two registers for the physical and virtual
> > timers (CNT{P,V}_{TVAL,CTL}_EL0). Just nitpicking here.
> 
> I'll write it like that, thanks!
> 
> > 
> > > 
> > > Save the hypervisor physical and virtual timer IRQ numbers from the DT/ACPI.
> > > 
> > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > ---
> > >  arm/timer.c         | 10 ++++++++--
> > >  lib/acpi.h          |  2 ++
> > >  lib/arm/asm/timer.h | 11 +++++++++++
> > >  lib/arm/timer.c     | 19 +++++++++++++++++--
> > >  4 files changed, 38 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arm/timer.c b/arm/timer.c
> > > index 2cb80518..c6287ca7 100644
> > > --- a/arm/timer.c
> > > +++ b/arm/timer.c
> > > @@ -347,8 +347,14 @@ static void test_ptimer(void)
> > >  static void test_init(void)
> > >  {
> > >  	assert(TIMER_PTIMER_IRQ != -1 && TIMER_VTIMER_IRQ != -1);
> It's still there ^^
> > > -	ptimer_info.irq = TIMER_PTIMER_IRQ;
> > > -	vtimer_info.irq = TIMER_VTIMER_IRQ;
> > > +	if (current_level() == CurrentEL_EL1) {
> > > +		ptimer_info.irq = TIMER_PTIMER_IRQ;
> > > +		vtimer_info.irq = TIMER_VTIMER_IRQ;
> > 
> > You dropped the assert for TIMER_{P,V}TIMER_IRQ.
> 
> The assertion is still there!

Yeah, my bad, sorry for that.

Alex

> 
> > 
> > Otherwise, looks good to me:
> > 
> > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> Thanks!
> 
> > 
> > Thanks,
> > Alex
> > 
> > > +	} else {
> > > +		assert(TIMER_HPTIMER_IRQ != -1 && TIMER_HVTIMER_IRQ != -1);
> > > +		ptimer_info.irq = TIMER_HPTIMER_IRQ;
> > > +		vtimer_info.irq = TIMER_HVTIMER_IRQ;
> > > +	}
> > >  
> > >  	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
> > >  	ptimer_info.read_ctl();
> > > diff --git a/lib/acpi.h b/lib/acpi.h
> > > index c330c877..66e3062d 100644
> > > --- a/lib/acpi.h
> > > +++ b/lib/acpi.h
> > > @@ -290,6 +290,8 @@ struct acpi_table_gtdt {
> > >  	u64 counter_read_block_address;
> > >  	u32 platform_timer_count;
> > >  	u32 platform_timer_offset;
> > > +	u32 virtual_el2_timer_interrupt;
> > > +	u32 virtual_el2_timer_flags;
> > >  };
> > >  
> > >  /* Reset to default packing */
> > > diff --git a/lib/arm/asm/timer.h b/lib/arm/asm/timer.h
> > > index aaf839fc..7dda0f4f 100644
> > > --- a/lib/arm/asm/timer.h
> > > +++ b/lib/arm/asm/timer.h
> > > @@ -21,12 +21,23 @@ struct timer_state {
> > >  		u32 irq;
> > >  		u32 irq_flags;
> > >  	} vtimer;
> > > +	struct {
> > > +		u32 irq;
> > > +		u32 irq_flags;
> > > +	} hptimer;
> > > +	struct {
> > > +		u32 irq;
> > > +		u32 irq_flags;
> > > +	} hvtimer;
> > >  };
> > >  extern struct timer_state __timer_state;
> > >  
> > >  #define TIMER_PTIMER_IRQ (__timer_state.ptimer.irq)
> > >  #define TIMER_VTIMER_IRQ (__timer_state.vtimer.irq)
> > >  
> > > +#define TIMER_HPTIMER_IRQ (__timer_state.hptimer.irq)
> > > +#define TIMER_HVTIMER_IRQ (__timer_state.hvtimer.irq)
> > > +
> > >  void timer_save_state(void);
> > >  
> > >  #endif /* !__ASSEMBLY__ */
> > > diff --git a/lib/arm/timer.c b/lib/arm/timer.c
> > > index ae702e41..57f504e2 100644
> > > --- a/lib/arm/timer.c
> > > +++ b/lib/arm/timer.c
> > > @@ -38,10 +38,11 @@ static void timer_save_state_fdt(void)
> > >  	 *      secure timer irq
> > >  	 *      non-secure timer irq            (ptimer)
> > >  	 *      virtual timer irq               (vtimer)
> > > -	 *      hypervisor timer irq
> > > +	 *      hypervisor timer irq            (hptimer)
> > > +	 *      hypervisor virtual timer irq    (hvtimer)
> > >  	 */
> > >  	prop = fdt_get_property(fdt, node, "interrupts", &len);
> > > -	assert(prop && len == (4 * 3 * sizeof(u32)));
> > > +	assert(prop && len >= (4 * 3 * sizeof(u32)));
> > >  
> > >  	data = (u32 *) prop->data;
> > >  	assert(fdt32_to_cpu(data[3]) == 1 /* PPI */ );
> > > @@ -50,6 +51,14 @@ static void timer_save_state_fdt(void)
> > >  	assert(fdt32_to_cpu(data[6]) == 1 /* PPI */ );
> > >  	__timer_state.vtimer.irq = PPI(fdt32_to_cpu(data[7]));
> > >  	__timer_state.vtimer.irq_flags = fdt32_to_cpu(data[8]);
> > > +	if (len == (5 * 3 * sizeof(u32))) {
> > > +		assert(fdt32_to_cpu(data[9]) == 1 /* PPI */ );
> > > +		__timer_state.hptimer.irq = PPI(fdt32_to_cpu(data[10]));
> > > +		__timer_state.hptimer.irq_flags = fdt32_to_cpu(data[11]);
> > > +		assert(fdt32_to_cpu(data[12]) == 1 /* PPI */ );
> > > +		__timer_state.hvtimer.irq = PPI(fdt32_to_cpu(data[13]));
> > > +		__timer_state.hvtimer.irq_flags = fdt32_to_cpu(data[14]);
> > > +	}
> > >  }
> > >  
> > >  #ifdef CONFIG_EFI
> > > @@ -72,6 +81,12 @@ static void timer_save_state_acpi(void)
> > >  
> > >  	__timer_state.vtimer.irq = gtdt->virtual_timer_interrupt;
> > >  	__timer_state.vtimer.irq_flags = gtdt->virtual_timer_flags;
> > > +
> > > +	__timer_state.hptimer.irq = gtdt->non_secure_el2_interrupt;
> > > +	__timer_state.hptimer.irq_flags = gtdt->non_secure_el2_flags;
> > > +
> > > +	__timer_state.hvtimer.irq = gtdt->virtual_el2_timer_interrupt;
> > > +	__timer_state.hvtimer.irq_flags = gtdt->virtual_el2_timer_flags;
> > >  }
> > >  
> > >  #else
> > > -- 
> > > 2.25.1
> > > 

Re: [kvm-unit-tests PATCH v1 6/7] arm64: pmu: count EL2 cycles

[Alexandru Elisei]

Hi Joey,

On Tue, Mar 04, 2025 at 04:56:04PM +0000, Joey Gouly wrote:
> Hi Alexandru,
> 
> On Thu, Feb 27, 2025 at 05:01:03PM +0000, Alexandru Elisei wrote:
> > Hi Joey,
> > 
> > On Thu, Feb 20, 2025 at 02:13:53PM +0000, Joey Gouly wrote:
> > > Count EL2 cycles if that's the EL kvm-unit-tests is running at!
> > > 
> > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > ---
> > >  arm/pmu.c | 21 ++++++++++++++++++---
> > >  1 file changed, 18 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index 2dc0822b..238e4628 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -206,6 +206,8 @@ static void test_overflow_interrupt(bool overflow_at_64bits) {}
> > >  #define ID_DFR0_PMU_V3_8_5	0b0110
> > >  #define ID_DFR0_PMU_IMPDEF	0b1111
> > >  
> > > +#define PMCCFILTR_EL0_NSH	BIT(27)
> > > +
> > >  static inline uint32_t get_id_aa64dfr0(void) { return read_sysreg(id_aa64dfr0_el1); }
> > >  static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
> > >  static inline void set_pmcr(uint32_t v) { write_sysreg(v, pmcr_el0); }
> > > @@ -247,7 +249,7 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
> > >  #define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
> > >  
> > >  #define PMEVTYPER_EXCLUDE_EL1 BIT(31)
> > 
> > I think you can drop the macro. I was expecting to see an exclude EL2 macro
> > used in its place when the test is running at EL2, but it seems
> > PMEVTYPER_EXCLUDE_EL1 is not used anywhere. Unless I'm missing something here.
> 
> I will drop it.
> 
> > 
> > > -#define PMEVTYPER_EXCLUDE_EL0 BIT(30)
> > > +#define PMEVTYPER_EXCLUDE_EL0 BIT(30) | BIT(27)
> > 
> > I'm not really sure what that's supposed to achieve - if the test is
> > running at EL2, and events from both EL0 and EL2 are excluded, what's left
> > to count?
> 
> bit 27 of PMEVTYPER is the NSH bit, when it is 1 the events are not filtered.
> This is the opposite polarity to the U (bit 30) and P (bit 31) bits. When they
> are 1, the events are filtered.

Aaah, I was reading the Arm ARM backwards. yeah, bit NSH needs to be set to
allow counting of events at EL2, my bad.

> 
> > 
> > I also don't understand what PMEVTYPER_EXCLUDE_EL0 does in the non-nested
> > virt case (when kvm-unit-tests boots at El1). The tests run at EL1, so not
> > counting events at EL0 shouldn't affect anything. Am I missing something
> > obvious here?
> 
> I don't know why it's like that. AFAICT, it doesn't run anything at EL0.

Ok, just wanted to make sure I'm not missing anything.

> 
> > 
> > >  
> > >  static bool is_event_supported(uint32_t n, bool warn)
> > >  {
> > > @@ -1059,11 +1061,18 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > >  static bool check_cycles_increase(void)
> > >  {
> > >  	bool success = true;
> > > +	u64 pmccfiltr = 0;
> > >  
> > >  	/* init before event access, this test only cares about cycle count */
> > >  	pmu_reset();
> > >  	set_pmcntenset(1 << PMU_CYCLE_IDX);
> > > -	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> > > +
> > > +#if defined(__aarch64__)
> > > +	if (current_level() == CurrentEL_EL2)
> > > +		// include EL2 cycle counts
> > > +		pmccfiltr |= PMCCFILTR_EL0_NSH;
> > > +#endif
> > > +	set_pmccfiltr(pmccfiltr);
> > >  
> > >  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> > >  	isb();
> > > @@ -1114,11 +1123,17 @@ static void measure_instrs(int num, uint32_t pmcr)
> > >  static bool check_cpi(int cpi)
> > >  {
> > >  	uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
> > > +	u64 pmccfiltr = 0;
> > >  
> > >  	/* init before event access, this test only cares about cycle count */
> > >  	pmu_reset();
> > >  	set_pmcntenset(1 << PMU_CYCLE_IDX);
> > > -	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> > > +#if defined(__aarch64__)
> > > +	if (current_level() == CurrentEL_EL2)
> > > +		// include EL2 cycle counts
> > > +		pmccfiltr |= PMCCFILTR_EL0_NSH;
> > > +#endif
> > 
> > It's called twice, so it could be abstracted in a function.
> > 
> > Also, I find it interesting that for PMCCFILTR_EL0 you set the NSH bit
> > based on current exception level, but for PMEVTYPER you set it
> > unconditionally. Why the different approaches? For convenience of is there
> > something more?
> 
> No real reason. I don't really know about the PMU, just did some changes to get
> this test working.  I'll look into if setting NSH always is fine too, then I'll
> unconditionally do it.

Cool, thanks.

Alex