Re: [RFC PATCH v2 11/14] x86/watchdog/hardlockup: Add an HPET-based hardlockup detector

["Suthikulpanit, Suravee" <Suravee.Suthikulpanit@amd.com>]

Hi Neri,

While trying out this patch series, I found that it does not work when the HPET timer
is in periodic mode.

On 2/27/19 11:05 PM, Ricardo Neri wrote:
> ......
> diff --git a/arch/x86/kernel/watchdog_hld_hpet.c b/arch/x86/kernel/watchdog_hld_hpet.c
> new file mode 100644
> index 000000000000..cfa284da4bf6
> --- /dev/null
> +++ b/arch/x86/kernel/watchdog_hld_hpet.c
> @@ -0,0 +1,405 @@
> .....
> +
> +/**
> + * set_comparator() - Update the comparator in an HPET timer instance
> + * @hdata:	A data structure with the timer instance to update
> + * @cmp:	The value to write in the in the comparator registere
> + *
> + * Returns:
> + *
> + * None
> + */
> +static inline void set_comparator(struct hpet_hld_data *hdata,
> +				  unsigned long cmp)
> +{
> +	hpet_writeq(cmp, HPET_Tn_CMP(hdata->num));
> +}
> +
> +/**
> + * kick_timer() - Reprogram timer to expire in the future
> + * @hdata:	A data structure with the timer instance to update
> + * @force:	Force reprogram. Useful enabling or re-enabling detector.
> + *
> + * Reprogram the timer to expire within watchdog_thresh seconds in the future.
> + *
> + * Returns:
> + *
> + * None
> + */
> +static void kick_timer(struct hpet_hld_data *hdata, bool force)
> +{
> +	bool kick_needed = force || !(hdata->flags & HPET_DEV_PERI_CAP);
> +	unsigned long new_compare, count;
> +
> +	/*
> +	 * Update the comparator in increments of watch_thresh seconds relative
> +	 * to the current count. Since watch_thresh is given in seconds, we
> +	 * are able to update the comparator before the counter reaches such new
> +	 * value.
> +	 *
> +	 * Let it wrap around if needed.
> +	 */
> +
> +	if (kick_needed) {
> +		count = get_count();
> +
> +		new_compare = count + watchdog_thresh * hdata->ticks_per_second;
> +
> +		set_comparator(hdata, new_compare);
> +	}
> +}

It turns out that the set_comparator() does not seem to support periodic mode,
in which it should have been setting the accumulator/period via the comparator register.

Instead, what if we re-factor the code in the arch/x86/kernel/hpet.c:hpet_set_periodic()
to hpet_set_comparator(). Then we can also reuse it in the arch/x86/kernel/watchdog_hld_pet.c:
kick_timer() as shown below.

With these changes, I can test the series on AMD systems, and see all cores
in /proc/interrupts are receiving NMI interrupts.

Regards,
Suravee


diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
index 0976334..f30957f 100644
--- a/arch/x86/include/asm/hpet.h
+++ b/arch/x86/include/asm/hpet.h
@@ -115,6 +115,7 @@ extern int hpet_rtc_timer_init(void);
  extern irqreturn_t hpet_rtc_interrupt(int irq, void *dev_id);
  extern int hpet_register_irq_handler(rtc_irq_handler handler);
  extern void hpet_unregister_irq_handler(rtc_irq_handler handler);
+extern void hpet_set_comparator(int num, unsigned int cmp, unsigned int period);

  #endif /* CONFIG_HPET_EMULATE_RTC */

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 03b05dae..f3b2351 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -328,6 +328,41 @@ static void hpet_legacy_clockevent_register(void)
  	printk(KERN_DEBUG "hpet clockevent registered\n");
  }

+/*
+ * hpet_set_comparator() - Helper function for setting comparator register
+ * @num:	The timer ID
+ * @cmp:	The value to be written to the comparator/accumulator
+ * @period:	The value to be written to the period (0 = oneshot mode)
+ *
+ * Helper function for updating comparator, accumulator and period values.
+ *
+ * In periodic mode, HPET needs HPET_TN_SETVAL to be set before writing
+ * to the Tn_CMP to update the accumulator. Then, HPET needs a second
+ * write (with HPET_TN_SETVAL cleared) to Tn_CMP to set the period.
+ * The HPET_TN_SETVAL bit is automatically cleared after the first write.
+ *
+ * For one-shot mode, HPET_TN_SETVAL does not need to be set.
+ *
+ * See the following documents:
+ *   - Intel IA-PC HPET (High Precision Event Timers) Specification
+ *   - AMD-8111 HyperTransport I/O Hub Data Sheet, Publication # 24674
+ */
+void hpet_set_comparator(int num, unsigned int cmp, unsigned int period)
+{
+	if (period) {
+		unsigned int v = hpet_readl(HPET_Tn_CFG(num));
+		hpet_writel(v | HPET_TN_SETVAL, HPET_Tn_CFG(num));
+	}
+
+	hpet_writel(cmp, HPET_Tn_CMP(num));
+	if (!period)
+		return;
+
+	udelay(1);
+	hpet_writel(period, HPET_Tn_CMP(num));
+}
+EXPORT_SYMBOL_GPL(hpet_set_comparator);
+
  static int hpet_set_periodic(struct clock_event_device *evt, int timer)
  {
  	unsigned int cfg, cmp, now;
@@ -339,19 +374,11 @@ static int hpet_set_periodic(struct clock_event_device *evt, int timer)
  	now = hpet_readl(HPET_COUNTER);
  	cmp = now + (unsigned int)delta;
  	cfg = hpet_readl(HPET_Tn_CFG(timer));
-	cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
-	       HPET_TN_32BIT;
+	cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_32BIT;
  	hpet_writel(cfg, HPET_Tn_CFG(timer));
-	hpet_writel(cmp, HPET_Tn_CMP(timer));
-	udelay(1);
-	/*
-	 * HPET on AMD 81xx needs a second write (with HPET_TN_SETVAL
-	 * cleared) to T0_CMP to set the period. The HPET_TN_SETVAL
-	 * bit is automatically cleared after the first write.
-	 * (See AMD-8111 HyperTransport I/O Hub Data Sheet,
-	 * Publication # 24674)
-	 */
-	hpet_writel((unsigned int)delta, HPET_Tn_CMP(timer));
+
+	hpet_set_comparator(timer, cmp, (unsigned int)delta);
+
  	hpet_start_counter();
  	hpet_print_config();

diff --git a/arch/x86/kernel/watchdog_hld_hpet.c b/arch/x86/kernel/watchdog_hld_hpet.c
index 4402def..de33c50 100644
--- a/arch/x86/kernel/watchdog_hld_hpet.c
+++ b/arch/x86/kernel/watchdog_hld_hpet.c
@@ -35,21 +35,6 @@ static inline unsigned long get_count(void)
  }

  /**
- * set_comparator() - Update the comparator in an HPET timer instance
- * @hdata:	A data structure with the timer instance to update
- * @cmp:	The value to write in the in the comparator registere
- *
- * Returns:
- *
- * None
- */
-static inline void set_comparator(struct hpet_hld_data *hdata,
-				  unsigned long cmp)
-{
-	hpet_writeq(cmp, HPET_Tn_CMP(hdata->num));
-}
-
-/**
   * kick_timer() - Reprogram timer to expire in the future
   * @hdata:	A data structure with the timer instance to update
   * @force:	Force reprogram. Useful enabling or re-enabling detector.
@@ -68,7 +53,7 @@ static inline void set_comparator(struct hpet_hld_data *hdata,
  static void kick_timer(struct hpet_hld_data *hdata, bool force)
  {
  	bool kick_needed = force || !(hdata->flags & HPET_DEV_PERI_CAP);
-	unsigned long tsc_curr, tsc_delta, new_compare, count;
+	unsigned long tsc_curr, tsc_delta, new_compare, count, period = 0;

  	/* Start obtaining the current TSC and HPET counts. */
  	tsc_curr = rdtsc();
@@ -81,6 +66,9 @@ static void kick_timer(struct hpet_hld_data *hdata, bool force)
  	hdata->tsc_next = tsc_curr + tsc_delta;
  	hdata->tsc_next_error = tsc_delta >> 6;

+	if (!kick_needed)
+		return;
+
  	/*
  	 * Update the comparator in increments of watch_thresh seconds relative
  	 * to the current count. Since watch_thresh is given in seconds, we
@@ -89,12 +77,11 @@ static void kick_timer(struct hpet_hld_data *hdata, bool force)
  	 *
  	 * Let it wrap around if needed.
  	 */
+	if (hdata->flags & HPET_DEV_PERI_CAP)
+		period = watchdog_thresh * hdata->ticks_per_second;

-	if (kick_needed) {
-		new_compare = count + watchdog_thresh * hdata->ticks_per_second;
-
-		set_comparator(hdata, new_compare);
-	}
+	new_compare = count + period;
+	hpet_set_comparator(hdata->num, new_compare, period);
  }

  /**
-- 
2.7.4