[PATCH v2] arm64: arch_timer: XGene-1: TVAL register math error breaks timer expiry calculation.

* [PATCH v2] arm64: arch_timer: XGene-1: TVAL register math error breaks timer expiry calculation.
@ 2022-10-24 16:54 Joe Korty
  2022-10-25 12:31 ` Marc Zyngier
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Joe Korty @ 2022-10-24 16:54 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, linux-arm-kernel,
	linux-kernel

arm64: arch_timer: XGene-1: TVAL register math error breaks timer expiry calculation.

The TVAL register is 32 bit signed.  Thus only the lower 31 bits are
available to specify when an interrupt is to occur at some time in the
near future.  Attempting to specify a larger interval with TVAL results
in a negative time delta which means the timer fires immediately upon
being programmed, rather than firing at that expected future time.

The solution is for linux to declare that TVAL is a 31 bit register rather
than give its true size of 32 bits.  This prevents linux from programming
TVAL with a too-large value.  Note that, prior to 5.16, this little trick
was the standard way to handle TVAL in linux, so there is nothing new
happening here on that front.

Test procedure: for some reason, the lockup watchdog is sensitive to
this bug.  When we turn the watchdog off, then run a little 'hello world'
program in each of the CPUs, there will often be one that 1) hangs up
forever, 2) hangs up what seems like forever, but actully contines after
a few minutes.  In either case, the program cannot be freed by a ^C.
This test sequence requires CONFIG_SOFTLOCKUP_DETECTOR, and probably
requires that one of the NO_HZ Kconfig options be specified.

The sequence is, for an 8 cpu Mustang XGene-1:

   echo 0 >/proc/sys/kernel/watchdog
   for i in {0..7}; do taskset -c $i echo hi there $i; done

Note that though the hangup usually happens, it does not
always happen.

Some comments on the v1 version of this patch by Marc Zyngier:

  XGene implements CVAL (a 64bit comparator) in terms of TVAL (a countdown
  register) instead of the other way around. TVAL being a 32bit register,
  the width of the counter should equally be 32.  However, TVAL is a
  *signed* value, and keeps counting down in the negative range once the
  timer fires.

  It means that any TVAL value with bit 31 set will fire immediately,
  as it cannot be distinguished from an already expired timer. Reducing
  the timer range back to a paltry 31 bits papers over the issue.

  Another problem cannot be fixed though, which is that the timer interrupt
  *must* be handled within the negative countdown period, or the interrupt
  will be lost (TVAL will rollover to a positive value, indicative of a
  new timer deadline).

 [ v2: Expanded CC list - jak ]
 [ v2: Revamped changelog - jak ] 
 [ v2: streamlined inlined comments - jak ] 

Cc: stable@vger.kernel.org # 5.16+
Fixes: 012f18850452 ("clocksource/drivers/arm_arch_timer: Work around broken CVAL implementations")
Signed-off-by: Joe Korty <joe.korty@concurrent-rt.com>
---
base-commit: v6.0
Index: b/drivers/clocksource/arm_arch_timer.c
===================================================================

--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -804,6 +804,9 @@ static u64 __arch_timer_check_delta(void
 		/*
 		 * XGene-1 implements CVAL in terms of TVAL, meaning
 		 * that the maximum timer range is 32bit. Shame on them.
+		 *
+		 * Note that TVAL is signed, thus has only 31 of its
+		 * 32 bits to express magnitude.
 		 */
 		MIDR_ALL_VERSIONS(MIDR_CPU_MODEL(ARM_CPU_IMP_APM,
 						 APM_CPU_PART_POTENZA)),
@@ -811,8 +814,8 @@ static u64 __arch_timer_check_delta(void
 	};
 
 	if (is_midr_in_range_list(read_cpuid_id(), broken_cval_midrs)) {
-		pr_warn_once("Broken CNTx_CVAL_EL1, limiting width to 32bits");
-		return CLOCKSOURCE_MASK(32);
+		pr_warn_once("Broken CNTx_CVAL_EL1, using 32 bit TVAL instead.\n");
+		return CLOCKSOURCE_MASK(31);
 	}
 #endif
 	return CLOCKSOURCE_MASK(arch_counter_get_width());

^ permalink raw reply	[flat|nested] 7+ messages in thread