Re: [PATCH] KVM: Fix RTC interrupt coalescing tracking

2013-06-26 Thread Gleb Natapov 
On Wed, Jun 26, 2013 at 07:49:37AM +0200, Jan Kiszka wrote:
 On 2013-06-24 14:19, Gleb Natapov wrote:
  This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
  the commit kvm_apic_set_irq() no longer returns accurate information
  about interrupt injection status if injection is done into disabled
  APIC. RTC interrupt coalescing tracking relies on the information to be
  accurate and cannot recover if it is not.
  
  Signed-off-by: Gleb Natapov g...@redhat.com
  diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
  index 9d75193..9f4bea8 100644
  --- a/arch/x86/kvm/lapic.c
  +++ b/arch/x86/kvm/lapic.c
  @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
  return highest_irr;
   }
   
  -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
  - int vector, int level, int trig_mode,
  - unsigned long *dest_map);
  +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
  +int vector, int level, int trig_mode,
  +unsigned long *dest_map);
 
 I still think __apic_accept_irq should unconditionally inject, and the
 test for acpi_enabled belongs into kvm_apic_set_irq. Why should
 __apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off?
 See below for another reason to refactor this part of the interface.
 
10.4.7.2 Local APIC State After It Has Been Software Disabled

The local APIC will respond normally to INIT, NMI, SMI, and SIPI
messages.

   
  -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
  - unsigned long *dest_map)
  +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
  +   unsigned long *dest_map)
   {
  struct kvm_lapic *apic = vcpu-arch.apic;
   
  -   __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
  - irq-level, irq-trig_mode, dest_map);
  +   return __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
  +   irq-level, irq-trig_mode, dest_map);
   }
   
   static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
  @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
  struct kvm_lapic *src,
  *r = -1;
   
  if (irq-shorthand == APIC_DEST_SELF) {
  -   kvm_apic_set_irq(src-vcpu, irq, dest_map);
  -   *r = 1;
  +   *r = kvm_apic_set_irq(src-vcpu, irq, dest_map);
  return true;
  }
   
  @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
  struct kvm_lapic *src,
  continue;
  if (*r  0)
  *r = 0;
  -   kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
  -   *r += 1;
  +   *r += kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
  }
   
  ret = true;
  @@ -664,11 +662,15 @@ out:
  return ret;
   }
   
  -/* Set an IRQ pending in the lapic. */
  -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
  - int vector, int level, int trig_mode,
  - unsigned long *dest_map)
  +/*
  + * Add a pending IRQ into lapic.
  + * Return 1 if successfully added and 0 if discarded.
  + */
  +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
  +int vector, int level, int trig_mode,
  +unsigned long *dest_map)
   {
  +   int result = 0;
  struct kvm_vcpu *vcpu = apic-vcpu;
   
  switch (delivery_mode) {
  @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, 
  int delivery_mode,
  if (dest_map)
  __set_bit(vcpu-vcpu_id, dest_map);
   
  -   if (kvm_x86_ops-deliver_posted_interrupt)
  +   if (kvm_x86_ops-deliver_posted_interrupt) {
  +   result = 1;
  kvm_x86_ops-deliver_posted_interrupt(vcpu, vector);
  -   else {
  -   if (apic_test_and_set_irr(vector, apic)) {
  +   } else {
  +   result = !apic_test_and_set_irr(vector, apic);
 
 This part of the revert makes no sense. If deliver_posted_interrupt is
 on, we don't have this feedback anymore, thus we decided to remove it, no?
 
Agree, but I wanted to do clear revert and fix on top.

 Jan
 
  +
  +   if (!result) {
  if (trig_mode)
  apic_debug(level trig mode repeatedly 
  for vector %d, vector);
  @@ -697,7 +702,7 @@ static void __apic_accept_irq(struct kvm_lapic *apic, 
  int delivery_mode,
  }
   out:
  trace_kvm_apic_accept_irq(vcpu-vcpu_id, delivery_mode,
  - trig_mode, vector, false);
  +   trig_mode, vector, !result);
  break;
   
  case APIC_DM_REMRD:
  @@ -709,12

Re: [PATCH] KVM: Fix RTC interrupt coalescing tracking

2013-06-26 Thread Jan Kiszka 
On 2013-06-26 08:15, Gleb Natapov wrote:
 On Wed, Jun 26, 2013 at 07:49:37AM +0200, Jan Kiszka wrote:
 On 2013-06-24 14:19, Gleb Natapov wrote:
 This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
 the commit kvm_apic_set_irq() no longer returns accurate information
 about interrupt injection status if injection is done into disabled
 APIC. RTC interrupt coalescing tracking relies on the information to be
 accurate and cannot recover if it is not.

 Signed-off-by: Gleb Natapov g...@redhat.com
 diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
 index 9d75193..9f4bea8 100644
 --- a/arch/x86/kvm/lapic.c
 +++ b/arch/x86/kvm/lapic.c
 @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 return highest_irr;
  }
  
 -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 - int vector, int level, int trig_mode,
 - unsigned long *dest_map);
 +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 +int vector, int level, int trig_mode,
 +unsigned long *dest_map);

 I still think __apic_accept_irq should unconditionally inject, and the
 test for acpi_enabled belongs into kvm_apic_set_irq. Why should
 __apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off?
 See below for another reason to refactor this part of the interface.

 10.4.7.2 Local APIC State After It Has Been Software Disabled
 
 The local APIC will respond normally to INIT, NMI, SMI, and SIPI
 messages.

OK, I see.

 
  
 -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
 - unsigned long *dest_map)
 +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
 +   unsigned long *dest_map)
  {
 struct kvm_lapic *apic = vcpu-arch.apic;
  
 -   __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
 - irq-level, irq-trig_mode, dest_map);
 +   return __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
 +   irq-level, irq-trig_mode, dest_map);
  }
  
  static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
 @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
 struct kvm_lapic *src,
 *r = -1;
  
 if (irq-shorthand == APIC_DEST_SELF) {
 -   kvm_apic_set_irq(src-vcpu, irq, dest_map);
 -   *r = 1;
 +   *r = kvm_apic_set_irq(src-vcpu, irq, dest_map);
 return true;
 }
  
 @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
 struct kvm_lapic *src,
 continue;
 if (*r  0)
 *r = 0;
 -   kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
 -   *r += 1;
 +   *r += kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
 }
  
 ret = true;
 @@ -664,11 +662,15 @@ out:
 return ret;
  }
  
 -/* Set an IRQ pending in the lapic. */
 -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 - int vector, int level, int trig_mode,
 - unsigned long *dest_map)
 +/*
 + * Add a pending IRQ into lapic.
 + * Return 1 if successfully added and 0 if discarded.
 + */
 +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 +int vector, int level, int trig_mode,
 +unsigned long *dest_map)
  {
 +   int result = 0;
 struct kvm_vcpu *vcpu = apic-vcpu;
  
 switch (delivery_mode) {
 @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, 
 int delivery_mode,
 if (dest_map)
 __set_bit(vcpu-vcpu_id, dest_map);
  
 -   if (kvm_x86_ops-deliver_posted_interrupt)
 +   if (kvm_x86_ops-deliver_posted_interrupt) {
 +   result = 1;
 kvm_x86_ops-deliver_posted_interrupt(vcpu, vector);
 -   else {
 -   if (apic_test_and_set_irr(vector, apic)) {
 +   } else {
 +   result = !apic_test_and_set_irr(vector, apic);

 This part of the revert makes no sense. If deliver_posted_interrupt is
 on, we don't have this feedback anymore, thus we decided to remove it, no?

 Agree, but I wanted to do clear revert and fix on top.

Fine with me, let's write a separate fix.

Jan




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] KVM: Fix RTC interrupt coalescing tracking

2013-06-25 Thread Jan Kiszka 
On 2013-06-24 14:19, Gleb Natapov wrote:
 This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
 the commit kvm_apic_set_irq() no longer returns accurate information
 about interrupt injection status if injection is done into disabled
 APIC. RTC interrupt coalescing tracking relies on the information to be
 accurate and cannot recover if it is not.
 
 Signed-off-by: Gleb Natapov g...@redhat.com
 diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
 index 9d75193..9f4bea8 100644
 --- a/arch/x86/kvm/lapic.c
 +++ b/arch/x86/kvm/lapic.c
 @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
   return highest_irr;
  }
  
 -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 -   int vector, int level, int trig_mode,
 -   unsigned long *dest_map);
 +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 +  int vector, int level, int trig_mode,
 +  unsigned long *dest_map);

I still think __apic_accept_irq should unconditionally inject, and the
test for acpi_enabled belongs into kvm_apic_set_irq. Why should
__apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off?
See below for another reason to refactor this part of the interface.

  
 -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
 -   unsigned long *dest_map)
 +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
 + unsigned long *dest_map)
  {
   struct kvm_lapic *apic = vcpu-arch.apic;
  
 - __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
 -   irq-level, irq-trig_mode, dest_map);
 + return __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
 + irq-level, irq-trig_mode, dest_map);
  }
  
  static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
 @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
 struct kvm_lapic *src,
   *r = -1;
  
   if (irq-shorthand == APIC_DEST_SELF) {
 - kvm_apic_set_irq(src-vcpu, irq, dest_map);
 - *r = 1;
 + *r = kvm_apic_set_irq(src-vcpu, irq, dest_map);
   return true;
   }
  
 @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
 struct kvm_lapic *src,
   continue;
   if (*r  0)
   *r = 0;
 - kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
 - *r += 1;
 + *r += kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
   }
  
   ret = true;
 @@ -664,11 +662,15 @@ out:
   return ret;
  }
  
 -/* Set an IRQ pending in the lapic. */
 -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 -   int vector, int level, int trig_mode,
 -   unsigned long *dest_map)
 +/*
 + * Add a pending IRQ into lapic.
 + * Return 1 if successfully added and 0 if discarded.
 + */
 +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 +  int vector, int level, int trig_mode,
 +  unsigned long *dest_map)
  {
 + int result = 0;
   struct kvm_vcpu *vcpu = apic-vcpu;
  
   switch (delivery_mode) {
 @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, 
 int delivery_mode,
   if (dest_map)
   __set_bit(vcpu-vcpu_id, dest_map);
  
 - if (kvm_x86_ops-deliver_posted_interrupt)
 + if (kvm_x86_ops-deliver_posted_interrupt) {
 + result = 1;
   kvm_x86_ops-deliver_posted_interrupt(vcpu, vector);
 - else {
 - if (apic_test_and_set_irr(vector, apic)) {
 + } else {
 + result = !apic_test_and_set_irr(vector, apic);

This part of the revert makes no sense. If deliver_posted_interrupt is
on, we don't have this feedback anymore, thus we decided to remove it, no?

Jan

 +
 + if (!result) {
   if (trig_mode)
   apic_debug(level trig mode repeatedly 
   for vector %d, vector);
 @@ -697,7 +702,7 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int 
 delivery_mode,
   }
  out:
   trace_kvm_apic_accept_irq(vcpu-vcpu_id, delivery_mode,
 -   trig_mode, vector, false);
 + trig_mode, vector, !result);
   break;
  
   case APIC_DM_REMRD:
 @@ -709,12 +714,14 @@ out:
   break;
  
   case APIC_DM_NMI:
 + result = 1;
   kvm_inject_nmi(vcpu);
   kvm_vcpu_kick(vcpu);
   break;
  
   case APIC_DM_INIT:
   if (!trig_mode || level) {
 +

[PATCH] KVM: Fix RTC interrupt coalescing tracking

2013-06-24 Thread Gleb Natapov 
This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
the commit kvm_apic_set_irq() no longer returns accurate information
about interrupt injection status if injection is done into disabled
APIC. RTC interrupt coalescing tracking relies on the information to be
accurate and cannot recover if it is not.

Signed-off-by: Gleb Natapov g...@redhat.com
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 9d75193..9f4bea8 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
return highest_irr;
 }
 
-static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
- int vector, int level, int trig_mode,
- unsigned long *dest_map);
+static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
+int vector, int level, int trig_mode,
+unsigned long *dest_map);
 
-void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
- unsigned long *dest_map)
+int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
+   unsigned long *dest_map)
 {
struct kvm_lapic *apic = vcpu-arch.apic;
 
-   __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
- irq-level, irq-trig_mode, dest_map);
+   return __apic_accept_irq(apic, irq-delivery_mode, irq-vector,
+   irq-level, irq-trig_mode, dest_map);
 }
 
 static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
@@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct 
kvm_lapic *src,
*r = -1;
 
if (irq-shorthand == APIC_DEST_SELF) {
-   kvm_apic_set_irq(src-vcpu, irq, dest_map);
-   *r = 1;
+   *r = kvm_apic_set_irq(src-vcpu, irq, dest_map);
return true;
}
 
@@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct 
kvm_lapic *src,
continue;
if (*r  0)
*r = 0;
-   kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
-   *r += 1;
+   *r += kvm_apic_set_irq(dst[i]-vcpu, irq, dest_map);
}
 
ret = true;
@@ -664,11 +662,15 @@ out:
return ret;
 }
 
-/* Set an IRQ pending in the lapic. */
-static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
- int vector, int level, int trig_mode,
- unsigned long *dest_map)
+/*
+ * Add a pending IRQ into lapic.
+ * Return 1 if successfully added and 0 if discarded.
+ */
+static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
+int vector, int level, int trig_mode,
+unsigned long *dest_map)
 {
+   int result = 0;
struct kvm_vcpu *vcpu = apic-vcpu;
 
switch (delivery_mode) {
@@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int 
delivery_mode,
if (dest_map)
__set_bit(vcpu-vcpu_id, dest_map);
 
-   if (kvm_x86_ops-deliver_posted_interrupt)
+   if (kvm_x86_ops-deliver_posted_interrupt) {
+   result = 1;
kvm_x86_ops-deliver_posted_interrupt(vcpu, vector);
-   else {
-   if (apic_test_and_set_irr(vector, apic)) {
+   } else {
+   result = !apic_test_and_set_irr(vector, apic);
+
+   if (!result) {
if (trig_mode)
apic_debug(level trig mode repeatedly 
for vector %d, vector);
@@ -697,7 +702,7 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int 
delivery_mode,
}
 out:
trace_kvm_apic_accept_irq(vcpu-vcpu_id, delivery_mode,
- trig_mode, vector, false);
+   trig_mode, vector, !result);
break;
 
case APIC_DM_REMRD:
@@ -709,12 +714,14 @@ out:
break;
 
case APIC_DM_NMI:
+   result = 1;
kvm_inject_nmi(vcpu);
kvm_vcpu_kick(vcpu);
break;
 
case APIC_DM_INIT:
if (!trig_mode || level) {
+   result = 1;
/* assumes that there are only KVM_APIC_INIT/SIPI */
apic-pending_events = (1UL  KVM_APIC_INIT);
/* make sure pending_events is visible before sending
@@ -731,6 +738,7 @@ out:
case APIC_DM_STARTUP:
apic_debug(SIPI to vcpu %d vector 0x%02x\n,
   vcpu-vcpu_id, vector);
+   result = 1;