| Message ID | 1426703902-16818-1-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Radim Kr?má? <rkrcmar@redhat.com> writes: > kvm_ioapic_update_eoi() wasn't called if directed EOI was enabled. > We need to do that for irq notifiers. (Like with edge interrupts.) Wow! It's interesting that this path is only hit with Xen as guest. I always thought of directed EOI as a "security feature" since broadcast could lead to interrupt storms (or something like that) :) Bandan > Fix it by skipping EOI broadcast only. > > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=82211 > Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> > --- > arch/x86/kvm/ioapic.c | 4 +++- > arch/x86/kvm/lapic.c | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index b1947e0f3e10..46d4449772bc 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -422,6 +422,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > struct kvm_ioapic *ioapic, int vector, int trigger_mode) > { > int i; > + struct kvm_lapic *apic = vcpu->arch.apic; > > for (i = 0; i < IOAPIC_NUM_PINS; i++) { > union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i]; > @@ -443,7 +444,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > spin_lock(&ioapic->lock); > > - if (trigger_mode != IOAPIC_LEVEL_TRIG) > + if (trigger_mode != IOAPIC_LEVEL_TRIG || > + kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) > continue; > > ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index bd4e34de24c7..4ee827d7bf36 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -833,8 +833,7 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2) > > static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector) > { > - if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) && > - kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > + if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > int trigger_mode; > if (apic_test_vector(vector, apic->regs + APIC_TMR)) > trigger_mode = IOAPIC_LEVEL_TRIG; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-03-18 15:37-0400, Bandan Das: > Radim Kr?má? <rkrcmar@redhat.com> writes: > > kvm_ioapic_update_eoi() wasn't called if directed EOI was enabled. > > We need to do that for irq notifiers. (Like with edge interrupts.) > > Wow! It's interesting that this path is only hit with Xen as guest. Linux doesn't use directed EOI ... KVM should fail with anything that depends on PIT, so probably only Xen bothered to implement it :) > I always thought of directed EOI as a "security feature" since broadcast > could lead to interrupt storms (or something like that) :) I think it is just an unpopular optimization for large systems. (With multiple IO-APICs: IRQ handler knows which ones need the EOI, but LAPIC doesn't, hence we avoid some useless poking if OS does it ... EOI interrupt storm happens because right after EOI, the IO-APIC can send another interrupt and real hardware is slow, so CPU manages some cycles before receiving the next one, but KVM works instantaneously.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Radim Kr?má? <rkrcmar@redhat.com> writes: > 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@redhat.com> > --- > arch/x86/kvm/ioapic.c | 4 +++- > arch/x86/kvm/lapic.c | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index b1947e0f3e10..46d4449772bc 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -422,6 +422,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > struct kvm_ioapic *ioapic, int vector, int trigger_mode) > { > int i; > + struct kvm_lapic *apic = vcpu->arch.apic; > > for (i = 0; i < IOAPIC_NUM_PINS; i++) { > union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i]; > @@ -443,7 +444,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > spin_lock(&ioapic->lock); > > - if (trigger_mode != IOAPIC_LEVEL_TRIG) > + if (trigger_mode != IOAPIC_LEVEL_TRIG || > + kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) > continue; > > ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index bd4e34de24c7..4ee827d7bf36 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -833,8 +833,7 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2) > > static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector) > { > - if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) && > - kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > + if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > int trigger_mode; > if (apic_test_vector(vector, apic->regs + APIC_TMR)) > trigger_mode = IOAPIC_LEVEL_TRIG; Works on my Xen 4.4 L1 setup with Intel E5 v2 host. Without this patch, L1 panics as reported in the bug referenced above. Tested-by: Bandan Das<bsd@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/03/2015 19:38, 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@redhat.com> > --- > arch/x86/kvm/ioapic.c | 4 +++- > arch/x86/kvm/lapic.c | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index b1947e0f3e10..46d4449772bc 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -422,6 +422,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > struct kvm_ioapic *ioapic, int vector, int trigger_mode) > { > int i; > + struct kvm_lapic *apic = vcpu->arch.apic; > > for (i = 0; i < IOAPIC_NUM_PINS; i++) { > union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i]; > @@ -443,7 +444,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > spin_lock(&ioapic->lock); > > - if (trigger_mode != IOAPIC_LEVEL_TRIG) > + if (trigger_mode != IOAPIC_LEVEL_TRIG || > + kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) > continue; > > ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index bd4e34de24c7..4ee827d7bf36 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -833,8 +833,7 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2) > > static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector) > { > - if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) && > - kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > + if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { Can you look into making kvm_ioapic_handles_vector inline in ioapic.h? Anyway, Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo > int trigger_mode; > if (apic_test_vector(vector, apic->regs + APIC_TMR)) > trigger_mode = IOAPIC_LEVEL_TRIG; > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-03-19 18:24+0100, Paolo Bonzini: > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > @@ -833,8 +833,7 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2) > > - if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) && > > - kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > > + if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > > Can you look into making kvm_ioapic_handles_vector inline in ioapic.h? Will do. > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> > --- > arch/x86/kvm/ioapic.c | 4 +++- > arch/x86/kvm/lapic.c | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index b1947e0f3e10..46d4449772bc 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -422,6 +422,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > struct kvm_ioapic *ioapic, int vector, int trigger_mode) > { > int i; > + struct kvm_lapic *apic = vcpu->arch.apic; > > for (i = 0; i < IOAPIC_NUM_PINS; i++) { > union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i]; > @@ -443,7 +444,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > spin_lock(&ioapic->lock); > > - 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? > ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); This assert can now fail? > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index bd4e34de24c7..4ee827d7bf36 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -833,8 +833,7 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2) > > static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector) > { > - if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) && > - kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > + if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { > int trigger_mode; > if (apic_test_vector(vector, apic->regs + APIC_TMR)) > trigger_mode = IOAPIC_LEVEL_TRIG; > -- > 2.3.3 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> > --- > arch/x86/kvm/ioapic.c | 4 +++- > arch/x86/kvm/lapic.c | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) Applied to master, thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c index b1947e0f3e10..46d4449772bc 100644 --- a/arch/x86/kvm/ioapic.c +++ b/arch/x86/kvm/ioapic.c @@ -422,6 +422,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, struct kvm_ioapic *ioapic, int vector, int trigger_mode) { int i; + struct kvm_lapic *apic = vcpu->arch.apic; for (i = 0; i < IOAPIC_NUM_PINS; i++) { union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i]; @@ -443,7 +444,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); spin_lock(&ioapic->lock); - if (trigger_mode != IOAPIC_LEVEL_TRIG) + if (trigger_mode != IOAPIC_LEVEL_TRIG || + kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) continue; ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index bd4e34de24c7..4ee827d7bf36 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -833,8 +833,7 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2) static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector) { - if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) && - kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { + if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) { int trigger_mode; if (apic_test_vector(vector, apic->regs + APIC_TMR)) trigger_mode = IOAPIC_LEVEL_TRIG;
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@redhat.com> --- arch/x86/kvm/ioapic.c | 4 +++- arch/x86/kvm/lapic.c | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-)