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

[Paolo Bonzini <pbonzini@redhat.com>]

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