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

[Paolo Bonzini]

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.

>>
>> +            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;
             }
 
+            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