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

2015-03-23 Thread 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ář rkrc...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-20 Thread 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ář rkrc...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-19 Thread 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ář rkrc...@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 pbonz...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-19 Thread 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 pbonz...@redhat.com

Thanks.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-19 Thread 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ář rkrc...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-18 Thread Bandan Das 
Radim Krčmář rkrc...@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ář rkrc...@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 Dasb...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-18 Thread 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ář rkrc...@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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-18 Thread Bandan Das 
Radim Krčmář rkrc...@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ář rkrc...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-18 Thread Radim Krčmář 
2015-03-18 15:37-0400, Bandan Das:
 Radim Krčmář rkrc...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html