Hi,
On 1/3/20 1:36 PM, Andre Przywara wrote:
> 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?
Good catch. From ARM DDI 0487E.a glossary, the section "Context synchronization
event":
"All direct and indirect writes to System registers that are made before the
Context synchronization event affect any instruction, including a direct read,
that appears in program order after the instruction causing the Context
synchronization event."
Based on that, I'll add an ISB after a control register write.
Thanks,
Alex
>
> Cheers,
> Andre.
>
>> +
>> info->irq_received = true;
>> +
>> + gic_write_eoir(irqstat);
>> }
>>
>> static bool gic_timer_pending(struct timer_info *info)
