* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-18 18:38 [PATCH] KVM: x86: call irq notifiers with directed EOI Radim Krčmář
@ 2015-03-18 19:37 ` Bandan Das
2015-03-18 20:16 ` Radim Krčmář
2015-03-18 20:50 ` Bandan Das
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Bandan Das @ 2015-03-18 19:37 UTC (permalink / raw)
To: Radim Krčmář
Cc: linux-kernel, kvm, Paolo Bonzini, Marcelo Tosatti, chao.zhou
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;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-18 19:37 ` Bandan Das
@ 2015-03-18 20:16 ` Radim Krčmář
0 siblings, 0 replies; 9+ messages in thread
From: Radim Krčmář @ 2015-03-18 20:16 UTC (permalink / raw)
To: Bandan Das; +Cc: linux-kernel, kvm, Paolo Bonzini, Marcelo Tosatti, chao.zhou
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.)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-18 18:38 [PATCH] KVM: x86: call irq notifiers with directed EOI Radim Krčmář
2015-03-18 19:37 ` Bandan Das
@ 2015-03-18 20:50 ` Bandan Das
2015-03-19 17:24 ` Paolo Bonzini
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Bandan Das @ 2015-03-18 20:50 UTC (permalink / raw)
To: Radim Krčmář
Cc: linux-kernel, kvm, Paolo Bonzini, Marcelo Tosatti, chao.zhou
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-18 18:38 [PATCH] KVM: x86: call irq notifiers with directed EOI Radim Krčmář
2015-03-18 19:37 ` Bandan Das
2015-03-18 20:50 ` Bandan Das
@ 2015-03-19 17:24 ` Paolo Bonzini
2015-03-19 19:42 ` Radim Krčmář
2015-03-19 21:44 ` Marcelo Tosatti
2015-03-23 23:27 ` Marcelo Tosatti
4 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2015-03-19 17:24 UTC (permalink / raw)
To: Radim Krčmář, linux-kernel; +Cc: kvm, Marcelo Tosatti, chao.zhou
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;
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-19 17:24 ` Paolo Bonzini
@ 2015-03-19 19:42 ` Radim Krčmář
0 siblings, 0 replies; 9+ messages in thread
From: Radim Krčmář @ 2015-03-19 19:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, Marcelo Tosatti, chao.zhou
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-18 18:38 [PATCH] KVM: x86: call irq notifiers with directed EOI Radim Krčmář
` (2 preceding siblings ...)
2015-03-19 17:24 ` Paolo Bonzini
@ 2015-03-19 21:44 ` Marcelo Tosatti
2015-03-20 14:43 ` Radim Krčmář
2015-03-23 23:27 ` Marcelo Tosatti
4 siblings, 1 reply; 9+ messages in thread
From: Marcelo Tosatti @ 2015-03-19 21:44 UTC (permalink / raw)
To: Radim Krčmář; +Cc: linux-kernel, kvm, Paolo Bonzini, chao.zhou
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-19 21:44 ` Marcelo Tosatti
@ 2015-03-20 14:43 ` Radim Krčmář
0 siblings, 0 replies; 9+ messages in thread
From: Radim Krčmář @ 2015-03-20 14:43 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: linux-kernel, kvm, Paolo Bonzini, chao.zhou
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: x86: call irq notifiers with directed EOI
2015-03-18 18:38 [PATCH] KVM: x86: call irq notifiers with directed EOI Radim Krčmář
` (3 preceding siblings ...)
2015-03-19 21:44 ` Marcelo Tosatti
@ 2015-03-23 23:27 ` Marcelo Tosatti
4 siblings, 0 replies; 9+ messages in thread
From: Marcelo Tosatti @ 2015-03-23 23:27 UTC (permalink / raw)
To: Radim Krčmář; +Cc: linux-kernel, kvm, Paolo Bonzini, chao.zhou
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.
^ permalink raw reply [flat|nested] 9+ messages in thread