Re: [kvm-unit-tests PATCH v3 13/18] arm64: timer: Test behavior when timer disabled or masked

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 31 Dec 2019 16:09:44 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi,

> When the timer is disabled (the *_CTL_EL0.ENABLE bit is clear) or the
> timer interrupt is masked at the timer level (the *_CTL_EL0.IMASK bit is
> set), timer interrupts must not be pending or asserted by the VGIC.
> However, only when the timer interrupt is masked, we can still check
> that the timer condition is met by reading the *_CTL_EL0.ISTATUS bit.
> 
> This test was used to discover a bug and test the fix introduced by KVM
> commit 16e604a437c8 ("KVM: arm/arm64: vgic: Reevaluate level sensitive
> interrupts on enable").
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/timer.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arm/timer.c b/arm/timer.c
> index 67e95ede24ef..a0b57afd4fe4 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -230,9 +230,17 @@ static void test_timer(struct timer_info *info)
>  
>  	/* Disable the timer again and prepare to take interrupts */
>  	info->write_ctl(0);
> +	isb();
> +	info->irq_received = false;
>  	set_timer_irq_enabled(info, true);

Are we too impatient here? There does not seem to be a barrier after the write to the ISENABLER register, so I wonder if we need at least a dsb() here? I think in other occasions (GIC test) we even wait for some significant amount of time to allow interrupts to trigger (or not).

> +	report(!info->irq_received, "no interrupt when timer is disabled");
>  	report(!gic_timer_pending(info), "interrupt signal no longer pending");
>  
> +	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
> +	isb();
> +	report(!gic_timer_pending(info), "interrupt signal not pending");
> +	report(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS, "timer condition met");
> +
>  	report(test_cval_10msec(info), "latency within 10 ms");
>  	report(info->irq_received, "interrupt received");

Not part of your patch, but is this kind of evaluation of the irq_received actually valid? Does the compiler know that this gets set in another part of the code (the IRQ handler)? Do we need some synchronisation or barrier here to prevent the compiler from optimising or reordering the access to irq_received? 

Cheers,
Andre.