2015-03-19 18:44-0300, Marcelo Tosatti: > On Wed, Mar 18, 2015 at 07:38:22PM +0100, Radim Krčmář wrote: > > kvm_ioapic_update_eoi() wasn't called if directed EOI was enabled. > > We need to do that for irq notifiers. (Like with edge interrupts.) > > > > Fix it by skipping EOI broadcast only. > > > > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=82211 > > Signed-off-by: Radim Krčmář <rkrcmar@xxxxxxxxxx> > > --- > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > > @@ -443,7 +444,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > > - if (trigger_mode != IOAPIC_LEVEL_TRIG) > > + if (trigger_mode != IOAPIC_LEVEL_TRIG || > > + kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) > > continue; > > Don't you have to handle kvm_ioapic_eoi_inject_work as well? It works without that: ent->fields.remote_irr == 1, thus kvm_ioapic_eoi_inject_work() will do nothing. Adding a check would be better for clarity, though. We could add the EOI register (implement IO-APIC version 0x20), because kernels are forced to do ugly hacks otherwise (switching to edge-triggered mode and back). We also clear remote_irr on a different occasion (just a write to ioreg). I'll take a closer look at the second one. > > ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); > > This assert can now fail? I think it can't (nothing changed), but that is how asserts should be. It checks a different variable than the condition above. ('trigger_mode' is sourced from APIC_TMR, which should correctly match 'ent->fields.trig_mode'.) The assert would be more useful before 'continue;', and modified: ASSERT(ent->fields.trig_mode == trigger_mode) Thanks for the review, I'll incorporate the your comments to v2. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
