Re: [kvm-unit-tests PATCH] arm: expand the timer tests

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On 1/10/20 4:05 PM, Alex Bennée wrote:
> This was an attempt to replicate a QEMU bug. However to trigger the
> bug you need to have an offset set in EL2 which kvm-unit-tests is
> unable to do. However it does exercise some more corner cases.
>
> Bug: https://bugs.launchpad.net/bugs/1859021

I'm not aware of any Bug: tags in the Linux kernel. If you want people to follow
the link to the bug, how about referencing something like this:

"This was an attempt to replicate a QEMU bug [1]. [..]

[1] https://bugs.launchpad.net/qemu/+bug/1859021"

Also, are launchpad bug reports permanent? Will the link still work in a years' time?

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arm/timer.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index f390e8e..ae1d299 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -214,21 +214,46 @@ static void test_timer(struct timer_info *info)
>  	 * still read the pending state even if it's disabled. */
>  	set_timer_irq_enabled(info, false);
>  
> +	/* Verify count goes up */
> +	report(info->read_counter() >= now, "counter increments");
> +
>  	/* Enable the timer, but schedule it for much later */
>  	info->write_cval(later);
>  	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
>  	isb();
> -	report(!gic_timer_pending(info), "not pending before");
> +	report(!gic_timer_pending(info), "not pending before 10s");
> +
> +	/* Check with a maximum possible cval */
> +	info->write_cval(UINT64_MAX);
> +	isb();
> +	report(!gic_timer_pending(info), "not pending before UINT64_MAX");
> +
> +	/* also by setting tval */

All the comments in this file seem to start with a capital letter.

> +	info->write_tval(time_10s);
> +	isb();
> +	report(!gic_timer_pending(info), "not pending before 10s (via tval)");

You can remove the "(via tval)" part - the message is unique enough to figure out
which part of the test it refers to.

> +	report_info("TVAL is %d (delta CVAL %ld) ticks",
> +		    info->read_tval(), info->read_cval() - info->read_counter());

I'm not sure what you are trying to achieve with this. You can transform it to
check that TVAL is indeed positive and (almost) equal to cval - cntpct, something
like this:

+	s32 tval = info->read_tval();
+	report(tval > 0 && tval <= info->read_cval() - info->read_counter(), "TVAL measures time to next interrupt");

>  
> +        /* check pending once cval is before now */

This comment adds nothing to the test.

>  	info->write_cval(now - 1);
>  	isb();
>  	report(gic_timer_pending(info), "interrupt signal pending");
> +	report_info("TVAL is %d ticks", info->read_tval());

You can test that TVAL is negative here instead of printing the value.

>  
>  	/* Disable the timer again and prepare to take interrupts */
>  	info->write_ctl(0);
>  	set_timer_irq_enabled(info, true);
>  	report(!gic_timer_pending(info), "interrupt signal no longer pending");
>  
> +	/* QEMU bug when cntvoff_el2 > 0
> +	 * https://bugs.launchpad.net/bugs/1859021 */

This looks confusing to me. From the commit message, I got that kvm-unit-tests
needs qemu to set a special value for CNTVOFF_EL2. But the comments seems to
suggest that kvm-unit-tests can trigger the bug without qemu doing anything
special. Can you elaborate under which condition kvm-unit-tests can trigger the bug?

> +	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> +	info->write_cval(UINT64_MAX);

The order is wrong - you write CVAL first, *then* enable to timer. Otherwise you
might get an interrupt because of the previous CVAL value.

The previous value for CVAL was now -1, so your change triggers an unwanted
interrupt after enabling the timer. The interrupt handler masks the timer
interrupt at the timer level, which means that as far as the gic is concerned the
interrupt is not pending, making the report call afterwards useless.

> +	isb();
> +	report(!gic_timer_pending(info), "not pending before UINT64_MAX (irqs on)");

This check can be improved. You want to check the timer CTL.ISTATUS here, not the
gic. A device (in this case, the timer) can assert the interrupt, but the gic does
not sample it immediately. Come to think of it, the entire timer test is wrong
because of this.

Thanks,
Alex
> +	info->write_ctl(0);
> +
>  	report(test_cval_10msec(info), "latency within 10 ms");
>  	report(info->irq_received, "interrupt received");
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel