KVM: arm64: timers: Program CVAL from the current count when the hardware won't apply the offset

Hi folks,

Changes since v1 [1]:
- Patch 3 programs the new case's CVAL as an absolute value near the wrap instead of a negative TVAL, so a host stall between the counter and timer writes can't make it fire. (Sashiko)

A guest timer can fail to fire, or fire early, once the guest's counter is far enough from the host's. Where the hardware won't apply the offset itself (the physical timer without CNTPOFF_EL2, a CNTPOFF_EL2 host while TGE is set, the virtual timer on x1e), KVM adds it to CVAL and lets the hardware compare the sum against the raw counter. The timer condition is an unsigned compare, and adding the offset to both sides of it preserves it only while both sums wrap or neither does: a guest whose counter is ahead of the host's by more than CVAL has an expired timer that never fires, and one behind it can have a far-future timer fire at once.

Patch 1 computes the programmed value from the current count instead, and returns a guest hypervisor's own physical CVAL from memory where it used to subtract the offset from the hardware value. Patch 2 does the same for the virtual CVAL read on x1e, which has returned CVAL + offset since the workaround landed. Patch 3 adds the second case, a guest behind the host with a far-future timer, to arch_timer_edge_cases.

So far it has only shown up in the selftest, which moves the guest counter half its range away from the host's. On a VHE host without CNTPOFF_EL2 arch_timer_edge_cases hangs in its physical cval = 0 cases until the vCPU is next loaded, which is what Zenghui saw on a Kunpeng920 [2]. On a CNTPOFF_EL2 host (QEMU, here) the same cases livelock instead. Both go away with patch 1. The arithmetic is wrong for any offset once one sum wraps and the other doesn't, and the fix for the selftest's case should be the fix for all of them.

Tested with arch_timer_edge_cases on QEMU with [3], which has since been merged, under VHE, nVHE and pKVM, and under VHE with CNTPOFF_EL2 removed by id_aa64mmfr0.ecv=1, the host class Zenghui hit. Under VHE with patch 1 reverted the new case fails. QEMU's TCG had a separate bug with the same offsets, which [3] fixes. Without it the test stalls under nVHE and pKVM as well, where KVM emulates the physical timer and patch 1 changes nothing. Not exercised here: patch 1's x1e paths in timer_save_state() and timer_restore_state() and all of patch 2, since nothing here runs on an x1e, and patch 1's CNTP_CVAL_EL0 read in kvm_hyp_handle_timer(), which only a guest hypervisor with a physical offset on a host without CNTPOFF_EL2 reaches.

Patch 1 carries Cc: stable for the hang and the livelock.

Based on Linux 7.3-rc3 (fd73f4a665989; also applies to kvmarm/next f91b534e81f23).

Cheers,
/fuad

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Fuad Tabba (3):
KVM: arm64: timers: Compute an offset-applied CVAL from the current
count
KVM: arm64: nv: Read a guest hypervisor's CNTV_CVAL_EL0 from memory on

    x1e

KVM: arm64: selftests: Test a timer set past the counter's wrap

 arch/arm64/kvm/arch_timer.c                   | 27 +++++++++----------
 arch/arm64/kvm/hyp/vhe/switch.c               | 21 +++++++++------
 include/kvm/arm_arch_timer.h                  | 15 +++++++++++
 .../kvm/arm64/arch_timer_edge_cases.c         | 25 +++++++++++++++++
 4 files changed, 65 insertions(+), 23 deletions(-)

base-commit: fd73f4a6659897191fa0d40695fe370925dd3780

2.39.5

When the hardware won't apply a timer's offset, KVM programs CVAL + offset and lets it compare that against the raw counter. The compare is unsigned (IsTimerConditionMet, DDI0487M_c J1.4.3.29):

  condition_met = (UInt(PhysicalCountInt() - offset) -
                   UInt(compare_value)) >= 0;

so adding the offset to both sides holds only while both sums wrap or neither does. A guest whose counter is ahead of the host's by more than CVAL has an expired timer that never fires, and one behind it can have a far-future timer fire at once.

On a CNTPOFF_EL2 host the first case livelocks: each entry reloads the guest's CVAL, the timer fires, and the exit rewrites it as CVAL + offset before the host can take the interrupt. Without CNTPOFF_EL2 the timer hangs until the vCPU is next loaded, which is what Zenghui reported on a Kunpeng920.

Program the value from the current count instead: CVAL + offset while the deadline is ahead, 0 once the timer has expired for the guest, ~0 past the counter's wrap. KVM recomputes it at load, at exit and on a trapped CVAL write, and an expired timer stays expired in between. The value can't be inverted, so with an offset timer_save_state() keeps the memory copy, and kvm_hyp_handle_timer() returns that copy for a guest hypervisor's read of its physical CVAL rather than subtracting the offset back out.

---
 arch/arm64/kvm/arch_timer.c     | 27 ++++++++++++---------------
 arch/arm64/kvm/hyp/vhe/switch.c | 11 ++++++-----
 include/kvm/arm_arch_timer.h    | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 20 deletions(-)

index 6ac3321f4c575..9278b5383c044 100644
@@ -528,17 +528,11 @@ static void timer_save_state(struct arch_timer_context *ctx)
 		goto out;
 
 	switch (index) {
-		u64 cval;
-
 	case TIMER_VTIMER:
 	case TIMER_HVTIMER:
 		timer_set_ctl(ctx, read_sysreg_el0(SYS_CNTV_CTL));
-		cval = read_sysreg_el0(SYS_CNTV_CVAL);
-
-		if (has_broken_cntvoff())
-			cval -= timer_get_offset(ctx);
-
-		timer_set_cval(ctx, cval);
+		if (!has_broken_cntvoff() || !timer_get_offset(ctx))
+			timer_set_cval(ctx, read_sysreg_el0(SYS_CNTV_CVAL));
 
 		/* Disable the timer */
 		write_sysreg_el0(0, SYS_CNTV_CTL);

@@ -564,11 +558,12 @@ static void timer_save_state(struct arch_timer_context *ctx)
 	case TIMER_PTIMER:
 	case TIMER_HPTIMER:
 		timer_set_ctl(ctx, read_sysreg_el0(SYS_CNTP_CTL));
-		cval = read_sysreg_el0(SYS_CNTP_CVAL);
-
-		cval -= timer_get_offset(ctx);
-
-		timer_set_cval(ctx, cval);
+		/*
+		 * With an offset, memory already holds the guest's CVAL (the
+		 * trap handler or __deactivate_traps() wrote it).
+		 */
+		if (!timer_get_offset(ctx))
+			timer_set_cval(ctx, read_sysreg_el0(SYS_CNTP_CVAL));
 
 		/* Disable the timer */
 		write_sysreg_el0(0, SYS_CNTP_CTL);

@@ -647,7 +642,8 @@ static void timer_restore_state(struct arch_timer_context *ctx)
 		offset = timer_get_offset(ctx);
 		if (has_broken_cntvoff()) {
 			set_cntvoff(0);
-			cval += offset;
+			if (offset)
+				cval = timer_apply_offset(cval, offset, kvm_phys_timer_read());
 		} else {
 			set_cntvoff(offset);
 		}

@@ -660,7 +656,8 @@ static void timer_restore_state(struct arch_timer_context *ctx)
 		cval = timer_get_cval(ctx);
 		offset = timer_get_offset(ctx);
 		set_cntpoff(offset);
-		cval += offset;
+		if (offset)
+			cval = timer_apply_offset(cval, offset, kvm_phys_timer_read());
 		write_sysreg_el0(cval, SYS_CNTP_CVAL);
 		isb();
 		write_sysreg_el0(timer_get_ctl(ctx), SYS_CNTP_CTL);

index 7875911c05063..14aada311bac4 100644
@@ -175,7 +175,8 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
 		offset = read_sysreg_s(SYS_CNTPOFF_EL2);
 
 		if (map.direct_ptimer && offset) {
-			write_sysreg_el0(val + offset, SYS_CNTP_CVAL);
+			val = timer_apply_offset(val, offset, arch_timer_read_cntpct_el0());
+			write_sysreg_el0(val, SYS_CNTP_CVAL);
 			isb();
 		}
 	}

@@ -296,10 +297,10 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code)
 		break;
 	case SYS_CNTP_CVAL_EL0:
 		if (vcpu_el2_e2h_is_set(vcpu)) {
-			val = read_sysreg_el0(SYS_CNTP_CVAL);
-
-			if (!has_cntpoff())
-				val -= timer_get_offset(vcpu_hptimer(vcpu));
+			if (!has_cntpoff() && timer_get_offset(vcpu_hptimer(vcpu)))
+				val = __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2);
+			else
+				val = read_sysreg_el0(SYS_CNTP_CVAL);
 		} else {
 			val = __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0);
 		}

index bc6f2fdd7ad33..80f96ea59f1ae 100644
@@ -195,4 +195,19 @@ static inline void timer_set_offset(struct arch_timer_context *ctxt, u64 offset)
 	WRITE_ONCE(*ctxt->offset.vm_offset, offset);
 }
 
+/*
+ * CVAL to program when the hardware won't apply the timer's offset, so that
+ * its compare against the raw counter matches the guest's at 'now'. An
+ * expired timer stays expired; one past the wrap never fires.
+ */
+static inline u64 timer_apply_offset(u64 cval, u64 offset, u64 now)
+{
+	u64 hw = cval + offset;
+
+	if (now - offset >= cval)
+		return 0;
+
+	return hw > now ? hw : U64_MAX;
+}
+
 #endif

2.39.5

On x1e KVM keeps CNTVOFF_EL2 at zero, traps the virtual timer whenever it has an offset and programs the hardware with the offset applied. kvm_hyp_handle_timer() returns that hardware value for a VHE guest hypervisor's read of its own CNTV_CVAL_EL0, instead of the CVAL it wrote. Its writes trap to the kernel and land in CNTHV_CVAL_EL2, so return that, as the physical timer's read does without CNTPOFF_EL2.

---
 arch/arm64/kvm/hyp/vhe/switch.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index 14aada311bac4..4c965f9b1e1de 100644
@@ -322,10 +322,14 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code)
 		val = __vcpu_sys_reg(vcpu, CNTV_CVAL_EL0);
 		break;
 	case SYS_CNTV_CVAL_EL0:
-		if (vcpu_el2_e2h_is_set(vcpu))
-			val = read_sysreg_el0(SYS_CNTV_CVAL);
-		else
+		if (vcpu_el2_e2h_is_set(vcpu)) {
+			if (has_broken_cntvoff() && timer_get_offset(vcpu_hvtimer(vcpu)))
+				val = __vcpu_sys_reg(vcpu, CNTHV_CVAL_EL2);
+			else
+				val = read_sysreg_el0(SYS_CNTV_CVAL);
+		} else {
 			val = __vcpu_sys_reg(vcpu, CNTV_CVAL_EL0);
+		}
 		break;
 	case SYS_CNTVCT_EL0:
 	case SYS_CNTVCTSS_EL0:

2.39.5

Program an unmasked timer whose cval sits within long_wait_ms of the counter's wrap while the guest's counter is at wait_ms, and expect no interrupt. The guest's counter can't reach it, but a host that applies the timer's offset by adding it to cval wraps the sum into the past, and the timer fires at once. The one existing case with a cval past the wrap, in the sanity checks, runs with the timer masked, so it gets no interrupt either way.

---
 .../kvm/arm64/arch_timer_edge_cases.c         | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

index d9c9377a63256..d46799ff91dc1 100644
@@ -787,6 +787,30 @@ static void test_timers_in_the_past(enum arch_timer timer)
 	}
 }
 
+/*
+ * A cval within long_wait_ms of the counter's wrap while the counter sits at
+ * wait_ms: a deadline further away than the counter can reach. Unmasked, so
+ * an early ISTATUS shows up as an IRQ.
+ */
+static void test_timers_past_counter_wrap(enum arch_timer timer)
+{
+	u64 cval = -msec_to_cycles(test_args.long_wait_ms);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(sleep_method); i++) {
+		sleep_method_t sm = sleep_method[i];
+
+		local_irq_disable();
+		set_counter(timer, msec_to_cycles(test_args.wait_ms));
+		set_cval_irq(timer, cval, CTL_ENABLE);
+		sm(timer, msecs_to_usecs(test_args.wait_ms) + TIMEOUT_NO_IRQ_US);
+		local_irq_enable();
+		isb();
+		assert_irqs_handled(0);
+		timer_set_ctl(timer, CTL_IMASK);
+	}
+}
+
 static void test_long_timer_delays(enum arch_timer timer)
 {
 	s32 tval = (s32)msec_to_cycles(test_args.long_wait_ms);

@@ -806,6 +830,7 @@ static void guest_run_iteration(enum arch_timer timer)
 	test_basic_functionality(timer);
 	test_timers_sanity_checks(timer);
 
+	test_timers_past_counter_wrap(timer);
 	test_timers_above_tval_max(timer);
 	test_timers_in_the_past(timer);
 

2.39.5

Applied to next, thanks!

[1/3] KVM: arm64: timers: Compute an offset-applied CVAL from the current count

      commit: 49930b1b557ceb698e0ab7f985ea41c99dc8bf22
[2/3] KVM: arm64: nv: Read a guest hypervisor's CNTV_CVAL_EL0 from memory on x1e
      commit: 1dbb5a2b5a21a62a223a2a2a0d775d417dca4242
[3/3] KVM: arm64: selftests: Test a timer set past the counter's wrap
      commit: 46e0f2c9d7d6f99a8f16290d4d4e2f3129edfa54

Cheers,

	M.

Without deviation from the norm, progress is not possible.