Re: [Qemu-devel] [PATCH] ioapic: allow buggy guests mishandling level-triggered interrupts to make progress

[Paolo Bonzini]

On 01/04/19 15:36, Vitaly Kuznetsov wrote:
> It was found that Hyper-V 2016 on KVM in some configurations (q35 machine +
> piix4-usb-uhci) hangs on boot. Trace analysis led us to the conclusion that
> it is mishandling level-triggered interrupt performing EOI without fixing
> the root cause. This causes immediate re-assertion and L2 VM (which is
> supposedly expected to fix the cause of the interrupt) is not making any
> progress.
> 
> Gory details: https://www.spinics.net/lists/kvm/msg184484.html
> 
> Turns out we were dealing with similar issues before; in-kernel IOAPIC
> implementation has commit 184564efae4d ("kvm: ioapic: conditionally delay
> irq delivery duringeoi broadcast") which describes a very similar issue.
> 
> Steal the idea from the above mentioned commit for IOAPIC implementation in
> QEMU. SUCCESSIVE_IRQ_MAX_COUNT, delay and the comment are borrowed as well.
> 
> Signed-off-by: Vitaly Kuznetsov <address@hidden>
> ---
>  hw/intc/ioapic.c                  | 43 ++++++++++++++++++++++++++++++-
>  hw/intc/trace-events              |  1 +
>  include/hw/i386/ioapic_internal.h |  3 +++
>  3 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 9d75f84d3b..daf45cc8a8 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -139,6 +139,15 @@ static void ioapic_service(IOAPICCommonState *s)
>      }
>  }
>  
> +#define SUCCESSIVE_IRQ_MAX_COUNT 10000
> +
> +static void ioapic_timer(void *opaque)
> +{
> +    IOAPICCommonState *s = opaque;
> +
> +    ioapic_service(s);
> +}
> +
>  static void ioapic_set_irq(void *opaque, int vector, int level)
>  {
>      IOAPICCommonState *s = opaque;
> @@ -227,7 +236,28 @@ void ioapic_eoi_broadcast(int vector)
>                  trace_ioapic_clear_remote_irr(n, vector);
>                  s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
>                  if (!(entry & IOAPIC_LVT_MASKED) && (s->irr & (1 << n))) {
> -                    ioapic_service(s);
> +                    bool level = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 
> 1)
> +                        == IOAPIC_TRIGGER_LEVEL;
> +
> +                    ++s->irq_reassert[vector];
> +                    if (!level ||
> +                        s->irq_reassert[vector] < SUCCESSIVE_IRQ_MAX_COUNT) {
> +                        ioapic_service(s);
> +                    } else {
> +                        /*
> +                         * Real hardware does not deliver the interrupt
> +                         * immediately during eoi broadcast, and this lets a
> +                         * buggy guest make slow progress even if it does not
> +                         * correctly handle a level-triggered interrupt. 
> Emulate
> +                         * this behavior if we detect an interrupt storm.
> +                         */
> +                        trace_ioapic_eoi_delayed_reassert(vector);
> +                        timer_mod(s->timer,
> +                                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +                                  NANOSECONDS_PER_SECOND / 100);

Should this be done only if the timer isn't pending?

Paolo