Alexandru Elisei <alexandru.elisei@xxxxxxx> writes: > 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"; OK, I'll fix that in v2. > > Also, are launchpad bug reports permanent? Will the link still work in > a years' time? They should be - they are a unique id and we use them in the QEMU source tree. > >> 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. I added it to differentiate with the message a little further above. >> + 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"); Yes it was purely informational to say tval decrements towards the next IRQ. I can make it a pure test. > >> >> + /* check pending once cval is before now */ > > This comment adds nothing to the test. dropped. > >> 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. ok. > >> >> /* 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? It can't without some sort of mechanism to set the hypervisor registers before running the test. The QEMU bug is an overflow when cval of UINT64_MAX with a non-zero CNTVOFF_EL2. Running under KVM the host kernel will have likely set CNTVOFF_EL2 to some sort of value with: update_vtimer_cntvoff(vcpu, kvm_phys_timer_read()); > >> + 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. Is it worth still checking the GIC or just replacing everything with calls to: static bool timer_pending(struct timer_info *info) { return info->read_ctl() & ARCH_TIMER_CTL_ISTATUS; } > > Thanks, > Alex >> + info->write_ctl(0); >> + >> report(test_cval_10msec(info), "latency within 10 ms"); >> report(info->irq_received, "interrupt received"); >> -- Alex Bennée
