[PATCH] KVM: x86: call irq notifiers with directed EOI

[PATCH] KVM: x86: call irq notifiers with directed EOI

[Radim Krčmář]

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;
-- 
2.3.3

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[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.
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;

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[Radim Krčmář]

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.)

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[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.)
>
> 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>

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[Paolo Bonzini]

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

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[Radim Krčmář]

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.

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[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>
> ---
>  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

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[Radim Krčmář]

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.

Re: [PATCH] KVM: x86: call irq notifiers with directed EOI

[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>
> ---
>  arch/x86/kvm/ioapic.c | 4 +++-
>  arch/x86/kvm/lapic.c  | 3 +--
>  2 files changed, 4 insertions(+), 3 deletions(-)

Applied to master, thanks.