On Tue, 31 Dec 2019 16:09:43 +0000
Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
Hi,
> Writing to the EOIR register before masking the HW mapped timer
> interrupt can cause taking another timer interrupt immediately after
> exception return. This doesn't happen all the time, because KVM
> reevaluates the state of pending HW mapped level sensitive interrupts on
> each guest exit. If the second interrupt is pending and a guest exit
> occurs after masking the timer interrupt and before the ERET (which
> restores PSTATE.I), then KVM removes it.
>
> Move the write after the IMASK bit has been set to prevent this from
> happening.
Sounds about right, just one comment below:
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> ---
> arm/timer.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index f390e8e65d31..67e95ede24ef 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -149,8 +149,8 @@ static void irq_handler(struct pt_regs *regs)
> u32 irqstat = gic_read_iar();
> u32 irqnr = gic_iar_irqnr(irqstat);
>
> - if (irqnr != GICC_INT_SPURIOUS)
> - gic_write_eoir(irqstat);
> + if (irqnr == GICC_INT_SPURIOUS)
> + return;
>
> if (irqnr == PPI(vtimer_info.irq)) {
> info = &vtimer_info;
> @@ -162,7 +162,11 @@ static void irq_handler(struct pt_regs *regs)
> }
>
> info->write_ctl(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE);
> + isb();
Shall this isb() actually go into the write_[pv]timer_ctl() implementation? I see one other call where we enable the timer without an isb() afterwards. Plus I am not sure we wouldn't need it as well for disabling the timers?
Cheers,
Andre.
> +
> info->irq_received = true;
> +
> + gic_write_eoir(irqstat);
> }
>
> static bool gic_timer_pending(struct timer_info *info)
