[PATCH kvm-unit-tests] arm64: timer: Avoid IRQ race in timer test

[PATCH kvm-unit-tests] arm64: timer: Avoid IRQ race in timer test

[Christoffer Dall]

The current timer test relies on testing the pending state of the timer
before the interrupt handler has run which could lower the pending
signal again (because it masks the timer output signal).

What we really want is to make sure the output signal from the timer as
perceived by the virtual interrupt controller is low when the timer is
programmed some time far in the future.  The proper way to do that is to
disable the timer interrupt on the distributor and then reading its
pending state.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 arm/timer.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 33dfc6f..e824338 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -96,6 +96,23 @@ static struct timer_info ptimer_info = {
 	.write_ctl = write_ptimer_ctl,
 };
 
+static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
+{
+	u32 val = 0;
+
+	if (enabled)
+		val = 1 << PPI(info->irq);
+
+	switch (gic_version()) {
+	case 2:
+		writel(val, gicv2_dist_base() + GICD_ISENABLER + 0);
+		break;
+	case 3:
+		writel(val, gicv3_sgi_base() + GICR_ISENABLER0);
+		break;
+	}
+}
+
 static void irq_handler(struct pt_regs *regs)
 {
 	struct timer_info *info;
@@ -133,6 +150,8 @@ static bool test_cval_10msec(struct timer_info *info)
 	/* Program timer to fire in 10 ms */
 	before_timer = info->read_counter();
 	info->write_cval(before_timer + time_10ms);
+	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
+	isb();
 
 	/* Wait for the timer to fire */
 	while (!(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS))
@@ -159,13 +178,25 @@ static void test_timer(struct timer_info *info)
 	u64 time_10s = read_sysreg(cntfrq_el0) * 10;
 	u64 later = now + time_10s;
 
+	/* We don't want the irq handler to fire because that will change the
+	 * timer state and we want to test the timer output signal.  We can
+	 * still read the pending state even if it's disabled. */
+	set_timer_irq_enabled(info, false);
 
-	/* Enable the timer, but schedule it for much later*/
+	/* Enable the timer, but schedule it for much later */
 	info->write_cval(later);
-	isb();
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
-
+	isb();
 	report("not pending before", !gic_timer_pending(info));
+
+	info->write_cval(now - 1);
+	isb();
+	report("interrupt signal pending", gic_timer_pending(info));
+
+	/* Disable the timer again and prepare to take interrupts */
+	info->write_ctl(0);
+	set_timer_irq_enabled(info, true);
+
 	report("latency within 10 ms", test_cval_10msec(info));
 	report("interrupt received", info->irq_received);
 
@@ -211,13 +242,9 @@ static void test_init(void)
 
 	switch (gic_version()) {
 	case 2:
-		writel(1 << PPI(vtimer_info.irq), gicv2_dist_base() + GICD_ISENABLER + 0);
-		writel(1 << PPI(ptimer_info.irq), gicv2_dist_base() + GICD_ISENABLER + 0);
 		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
 		break;
 	case 3:
-		writel(1 << PPI(vtimer_info.irq), gicv3_sgi_base() + GICR_ISENABLER0);
-		writel(1 << PPI(ptimer_info.irq), gicv3_sgi_base() + GICR_ISENABLER0);
 		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
 		break;
 	}
-- 
2.9.0

Re: [PATCH kvm-unit-tests] arm64: timer: Avoid IRQ race in timer test

[Paolo Bonzini]

On 26/07/2017 13:42, Christoffer Dall wrote:
> The current timer test relies on testing the pending state of the timer
> before the interrupt handler has run which could lower the pending
> signal again (because it masks the timer output signal).
> 
> What we really want is to make sure the output signal from the timer as
> perceived by the virtual interrupt controller is low when the timer is
> programmed some time far in the future.  The proper way to do that is to
> disable the timer interrupt on the distributor and then reading its
> pending state.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  arm/timer.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/arm/timer.c b/arm/timer.c
> index 33dfc6f..e824338 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -96,6 +96,23 @@ static struct timer_info ptimer_info = {
>  	.write_ctl = write_ptimer_ctl,
>  };
>  
> +static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
> +{
> +	u32 val = 0;
> +
> +	if (enabled)
> +		val = 1 << PPI(info->irq);
> +
> +	switch (gic_version()) {
> +	case 2:
> +		writel(val, gicv2_dist_base() + GICD_ISENABLER + 0);
> +		break;
> +	case 3:
> +		writel(val, gicv3_sgi_base() + GICR_ISENABLER0);
> +		break;
> +	}
> +}
> +
>  static void irq_handler(struct pt_regs *regs)
>  {
>  	struct timer_info *info;
> @@ -133,6 +150,8 @@ static bool test_cval_10msec(struct timer_info *info)
>  	/* Program timer to fire in 10 ms */
>  	before_timer = info->read_counter();
>  	info->write_cval(before_timer + time_10ms);
> +	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> +	isb();
>  
>  	/* Wait for the timer to fire */
>  	while (!(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS))
> @@ -159,13 +178,25 @@ static void test_timer(struct timer_info *info)
>  	u64 time_10s = read_sysreg(cntfrq_el0) * 10;
>  	u64 later = now + time_10s;
>  
> +	/* We don't want the irq handler to fire because that will change the
> +	 * timer state and we want to test the timer output signal.  We can
> +	 * still read the pending state even if it's disabled. */
> +	set_timer_irq_enabled(info, false);
>  
> -	/* Enable the timer, but schedule it for much later*/
> +	/* Enable the timer, but schedule it for much later */
>  	info->write_cval(later);
> -	isb();
>  	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> -
> +	isb();
>  	report("not pending before", !gic_timer_pending(info));
> +
> +	info->write_cval(now - 1);
> +	isb();
> +	report("interrupt signal pending", gic_timer_pending(info));
> +
> +	/* Disable the timer again and prepare to take interrupts */
> +	info->write_ctl(0);
> +	set_timer_irq_enabled(info, true);
> +
>  	report("latency within 10 ms", test_cval_10msec(info));
>  	report("interrupt received", info->irq_received);
>  
> @@ -211,13 +242,9 @@ static void test_init(void)
>  
>  	switch (gic_version()) {
>  	case 2:
> -		writel(1 << PPI(vtimer_info.irq), gicv2_dist_base() + GICD_ISENABLER + 0);
> -		writel(1 << PPI(ptimer_info.irq), gicv2_dist_base() + GICD_ISENABLER + 0);
>  		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
>  		break;
>  	case 3:
> -		writel(1 << PPI(vtimer_info.irq), gicv3_sgi_base() + GICR_ISENABLER0);
> -		writel(1 << PPI(ptimer_info.irq), gicv3_sgi_base() + GICR_ISENABLER0);
>  		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
>  		break;
>  	}
> 


Applied, thanks.

Paolo

Re: [PATCH kvm-unit-tests] arm64: timer: Avoid IRQ race in timer test

[Andrew Jones]

On Wed, Jul 26, 2017 at 01:42:49PM +0200, Christoffer Dall wrote:
> The current timer test relies on testing the pending state of the timer
> before the interrupt handler has run which could lower the pending
> signal again (because it masks the timer output signal).
> 
> What we really want is to make sure the output signal from the timer as
> perceived by the virtual interrupt controller is low when the timer is
> programmed some time far in the future.  The proper way to do that is to
> disable the timer interrupt on the distributor and then reading its
> pending state.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  arm/timer.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
>

Hi Christoffer,

With this patch the ptimer "not pending before" test always
fails on KVM for me (tested on mustang and thunderx). Have
you seen that?

Thanks,
drew

Re: [PATCH kvm-unit-tests] arm64: timer: Avoid IRQ race in timer test

[Christoffer Dall]

On Wed, Aug 02, 2017 at 05:53:12PM +0200, Andrew Jones wrote:
> On Wed, Jul 26, 2017 at 01:42:49PM +0200, Christoffer Dall wrote:
> > The current timer test relies on testing the pending state of the timer
> > before the interrupt handler has run which could lower the pending
> > signal again (because it masks the timer output signal).
> > 
> > What we really want is to make sure the output signal from the timer as
> > perceived by the virtual interrupt controller is low when the timer is
> > programmed some time far in the future.  The proper way to do that is to
> > disable the timer interrupt on the distributor and then reading its
> > pending state.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  arm/timer.c | 41 ++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 34 insertions(+), 7 deletions(-)
> >
> 
> Hi Christoffer,
> 
> With this patch the ptimer "not pending before" test always
> fails on KVM for me (tested on mustang and thunderx). Have
> you seen that?
> 

Doy, yeah, that was a silly one.  Patch incoming.

Thanks,
-Christoffer