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

[Liran Alon]

> On 2 Apr 2019, at 12:06, Paolo Bonzini <address@hidden> wrote:
> 
> On 02/04/19 10:23, Liran Alon wrote:
>>> +#define SUCCESSIVE_IRQ_MAX_COUNT 10000
>>> +
>>> +static void ioapic_timer(void *opaque)
>> I suggest rename method to something such as 
>> “delayed_ioapic_service_timer_handler()”.
>> 
> 
> I renamed them to delayed_ioapic_service_{timer,cb} and queued the patch.

Thanks.

> 
>>> 
>>> +            if (((entry & IOAPIC_VECTOR_MASK) != vector) ||
>>> +                !(entry & IOAPIC_LVT_REMOTE_IRR)) {
>>> +                continue;
>>> +            }
>>> +
>>> +            trace_ioapic_clear_remote_irr(n, vector);
>>> +            s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
>> 
>> remote-irr is only set for level-triggered interrupt.
>> Thus, I think in the “if” above you should “continue;” in case trigger-mode 
>> != IOAPIC_TRIGGER_LEVEL.
>> i.e. Replace condition "!(entry & IOAPIC_LVT_REMOTE_IRR)” with “trigger-mode 
>> != IOAPIC_TRIGGER_LEVEL”.
>> Then you can also remove the “if” below.
> 
> Like this?
> 
> @@ -232,19 +232,18 @@ void ioapic_eoi_broadcast(int vector)
>         for (n = 0; n < IOAPIC_NUM_PINS; n++) {
>             entry = s->ioredtbl[n];
> 
> -            if (((entry & IOAPIC_VECTOR_MASK) != vector) ||
> -                !(entry & IOAPIC_LVT_REMOTE_IRR)) {
> +            if ((entry & IOAPIC_VECTOR_MASK) != vector ||
> +                ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != 
> IOAPIC_TRIGGER_LEVEL) {
>                 continue;
>             }
> 
> -            trace_ioapic_clear_remote_irr(n, vector);
> -            s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
> -
> -            if (((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) !=
> -                IOAPIC_TRIGGER_LEVEL) {
> +            if (!(entry & IOAPIC_LVT_REMOTE_IRR)) {
>                 continue;
>             }

I think above “if” of checking remote-irr should just be removed.
But the rest seems good :)

> 
> +            trace_ioapic_clear_remote_irr(n, vector);
> +            s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
> +
>             if (!(entry & IOAPIC_LVT_MASKED) && (s->irr & (1 << n))) {
>                 ++s->irq_eoi[vector];
>                 if (s->irq_eoi[vector] >= SUCCESSIVE_IRQ_MAX_COUNT) {
> 
> 
> Paolo