| Message ID | 1577808589-31892-13-git-send-email-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Various fixes | expand |
On Tue, 31 Dec 2019 16:09:43 +0000 Alexandru Elisei <alexandru.elisei@arm.com> 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@arm.com> > --- > 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)
Hi, On 1/3/20 1:36 PM, Andre Przywara wrote: > On Tue, 31 Dec 2019 16:09:43 +0000 > Alexandru Elisei <alexandru.elisei@arm.com> 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@arm.com> >> --- >> 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)
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(); + info->irq_received = true; + + gic_write_eoir(irqstat); } static bool gic_timer_pending(struct timer_info *info)
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. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arm/timer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)