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 <[email protected]> > --- > 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 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
