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@xxxxxxxxxx> > --- > 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"); > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
