[PATCH 0/3] apic: Fixes for userspace model

[PATCH 0/3] apic: Fixes for userspace model

[Jan Kiszka]

As Avi noted recently, there is a problem in way we inject interrupts
into the userspace APIC under KVM: The TRP check over the iothread may
race with the VCPU raising the TPR value while in KVM mode. Patch 3
addresses this issue.

The other two patches fix problems I came across while thinking about
the first one.

Who would like to process this series, up/master? Or should it go in
directly?

Jan Kiszka (3):
  apic: Resolve potential endless loop around apic_update_irq
  apic: Reevaluate pending interrupts on LVT_LINT0 changes
  apic: Defer interrupt updates to VCPU thread

 cpu-exec.c         |    6 ++++++
 hw/apic.c          |   27 ++++++++++++++++++++++-----
 hw/apic.h          |    1 +
 hw/apic_internal.h |    1 -
 target-i386/cpu.h  |    4 +++-
 target-i386/kvm.c  |    4 ++++
 6 files changed, 36 insertions(+), 7 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 0/3] apic: Fixes for userspace model

[Jan Kiszka]

As Avi noted recently, there is a problem in way we inject interrupts
into the userspace APIC under KVM: The TRP check over the iothread may
race with the VCPU raising the TPR value while in KVM mode. Patch 3
addresses this issue.

The other two patches fix problems I came across while thinking about
the first one.

Who would like to process this series, up/master? Or should it go in
directly?

Jan Kiszka (3):
  apic: Resolve potential endless loop around apic_update_irq
  apic: Reevaluate pending interrupts on LVT_LINT0 changes
  apic: Defer interrupt updates to VCPU thread

 cpu-exec.c         |    6 ++++++
 hw/apic.c          |   27 ++++++++++++++++++++++-----
 hw/apic.h          |    1 +
 hw/apic_internal.h |    1 -
 target-i386/cpu.h  |    4 +++-
 target-i386/kvm.c  |    4 ++++
 6 files changed, 36 insertions(+), 7 deletions(-)

-- 
1.7.3.4

[PATCH 1/3] apic: Resolve potential endless loop around apic_update_irq

[Jan Kiszka]

Commit d96e173769 refactored the reinjection of pending PIC interrupts.
However, it missed the potential loop of apic_update_irq ->
apic_deliver_pic_intr -> apic_local_deliver -> apic_set_irq ->
apic_update_irq that /could/ occur if LINT0 is injected as APIC_DM_FIXED
and that vector is currently blocked via TPR.

Resolve this by reinjecting only where it matters: inside
apic_get_interrupt. This function may clear a vector while a
PIC-originated reason still exists.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/apic.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 60552df..e65a35f 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -363,9 +363,6 @@ static void apic_update_irq(APICCommonState *s)
     }
     if (apic_irq_pending(s) > 0) {
         cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
-    } else if (apic_accept_pic_intr(&s->busdev.qdev) &&
-               pic_get_output(isa_pic)) {
-        apic_deliver_pic_intr(&s->busdev.qdev, 1);
     }
 }
 
@@ -560,7 +557,14 @@ int apic_get_interrupt(DeviceState *d)
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);
     apic_sync_vapic(s, SYNC_TO_VAPIC);
+
+    /* re-inject if there is still a pending PIC interrupt */
+    if (apic_accept_pic_intr(&s->busdev.qdev) && pic_get_output(isa_pic)) {
+        apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    }
+
     apic_update_irq(s);
+
     return intno;
 }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 1/3] apic: Resolve potential endless loop around apic_update_irq

[Jan Kiszka]

Commit d96e173769 refactored the reinjection of pending PIC interrupts.
However, it missed the potential loop of apic_update_irq ->
apic_deliver_pic_intr -> apic_local_deliver -> apic_set_irq ->
apic_update_irq that /could/ occur if LINT0 is injected as APIC_DM_FIXED
and that vector is currently blocked via TPR.

Resolve this by reinjecting only where it matters: inside
apic_get_interrupt. This function may clear a vector while a
PIC-originated reason still exists.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/apic.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 60552df..e65a35f 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -363,9 +363,6 @@ static void apic_update_irq(APICCommonState *s)
     }
     if (apic_irq_pending(s) > 0) {
         cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
-    } else if (apic_accept_pic_intr(&s->busdev.qdev) &&
-               pic_get_output(isa_pic)) {
-        apic_deliver_pic_intr(&s->busdev.qdev, 1);
     }
 }
 
@@ -560,7 +557,14 @@ int apic_get_interrupt(DeviceState *d)
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);
     apic_sync_vapic(s, SYNC_TO_VAPIC);
+
+    /* re-inject if there is still a pending PIC interrupt */
+    if (apic_accept_pic_intr(&s->busdev.qdev) && pic_get_output(isa_pic)) {
+        apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    }
+
     apic_update_irq(s);
+
     return intno;
 }
 
-- 
1.7.3.4

[PATCH 2/3] apic: Reevaluate pending interrupts on LVT_LINT0 changes

[Jan Kiszka]

When the guest modifies the LVT_LINT0 register, we need to check if some
pending PIC interrupt can now be delivered.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/apic.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index e65a35f..5b8f3e8 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -532,6 +532,15 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
     apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, trigger_mode);
 }
 
+static bool apic_check_pic(APICCommonState *s)
+{
+    if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+        return false;
+    }
+    apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    return true;
+}
+
 int apic_get_interrupt(DeviceState *d)
 {
     APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
@@ -559,9 +568,7 @@ int apic_get_interrupt(DeviceState *d)
     apic_sync_vapic(s, SYNC_TO_VAPIC);
 
     /* re-inject if there is still a pending PIC interrupt */
-    if (apic_accept_pic_intr(&s->busdev.qdev) && pic_get_output(isa_pic)) {
-        apic_deliver_pic_intr(&s->busdev.qdev, 1);
-    }
+    apic_check_pic(s);
 
     apic_update_irq(s);
 
@@ -804,8 +811,11 @@ static void apic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
         {
             int n = index - 0x32;
             s->lvt[n] = val;
-            if (n == APIC_LVT_TIMER)
+            if (n == APIC_LVT_TIMER) {
                 apic_timer_update(s, qemu_get_clock_ns(vm_clock));
+            } else if (n == APIC_LVT_LINT0 && apic_check_pic(s)) {
+                apic_update_irq(s);
+            }
         }
         break;
     case 0x38:
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/3] apic: Reevaluate pending interrupts on LVT_LINT0 changes

[Jan Kiszka]

When the guest modifies the LVT_LINT0 register, we need to check if some
pending PIC interrupt can now be delivered.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/apic.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index e65a35f..5b8f3e8 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -532,6 +532,15 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
     apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, trigger_mode);
 }
 
+static bool apic_check_pic(APICCommonState *s)
+{
+    if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+        return false;
+    }
+    apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    return true;
+}
+
 int apic_get_interrupt(DeviceState *d)
 {
     APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
@@ -559,9 +568,7 @@ int apic_get_interrupt(DeviceState *d)
     apic_sync_vapic(s, SYNC_TO_VAPIC);
 
     /* re-inject if there is still a pending PIC interrupt */
-    if (apic_accept_pic_intr(&s->busdev.qdev) && pic_get_output(isa_pic)) {
-        apic_deliver_pic_intr(&s->busdev.qdev, 1);
-    }
+    apic_check_pic(s);
 
     apic_update_irq(s);
 
@@ -804,8 +811,11 @@ static void apic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
         {
             int n = index - 0x32;
             s->lvt[n] = val;
-            if (n == APIC_LVT_TIMER)
+            if (n == APIC_LVT_TIMER) {
                 apic_timer_update(s, qemu_get_clock_ns(vm_clock));
+            } else if (n == APIC_LVT_LINT0 && apic_check_pic(s)) {
+                apic_update_irq(s);
+            }
         }
         break;
     case 0x38:
-- 
1.7.3.4

[PATCH 3/3] apic: Defer interrupt updates to VCPU thread

[Jan Kiszka]

KVM performs TPR raising asynchronously to QEMU, specifically outside
QEMU's global lock. When an interrupt is injected into the APIC and TPR
is checked to decide if this can be delivered, a stale TPR value may be
used, causing spurious interrupts in the end.

Fix this by deferring apic_update_irq to the context of the target VCPU.
We introduce a new interrupt flag for this, CPU_INTERRUPT_POLL. When it
is set, the VCPU calls apic_poll_irq before checking for further pending
interrupts. To avoid special-casing KVM, we also implement this logic
for TCG mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-exec.c         |    6 ++++++
 hw/apic.c          |    5 ++++-
 hw/apic.h          |    1 +
 hw/apic_internal.h |    1 -
 target-i386/cpu.h  |    4 +++-
 target-i386/kvm.c  |    4 ++++
 6 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 08c35f7..fc185a4 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -288,6 +288,12 @@ int cpu_exec(CPUArchState *env)
                     }
 #endif
 #if defined(TARGET_I386)
+#if !defined(CONFIG_USER_ONLY)
+                    if (interrupt_request & CPU_INTERRUPT_POLL) {
+                        env->interrupt_request &= ~CPU_INTERRUPT_POLL;
+                        apic_poll_irq(env->apic_state);
+                    }
+#endif
                     if (interrupt_request & CPU_INTERRUPT_INIT) {
                             cpu_svm_check_intercept_param(env, SVM_EXIT_INIT,
                                                           0);
diff --git a/hw/apic.c b/hw/apic.c
index 5b8f3e8..385555e 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -16,6 +16,7 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>
  */
+#include "qemu-thread.h"
 #include "apic_internal.h"
 #include "apic.h"
 #include "ioapic.h"
@@ -361,7 +362,9 @@ static void apic_update_irq(APICCommonState *s)
     if (!(s->spurious_vec & APIC_SV_ENABLE)) {
         return;
     }
-    if (apic_irq_pending(s) > 0) {
+    if (!qemu_cpu_is_self(s->cpu_env)) {
+        cpu_interrupt(s->cpu_env, CPU_INTERRUPT_POLL);
+    } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
     }
 }
diff --git a/hw/apic.h b/hw/apic.h
index 62179ce..a89542b 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,6 +20,7 @@ void apic_init_reset(DeviceState *s);
 void apic_sipi(DeviceState *s);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
+void apic_poll_irq(DeviceState *d);
 
 /* pc.c */
 int cpu_is_bsp(CPUX86State *env);
diff --git a/hw/apic_internal.h b/hw/apic_internal.h
index 60a6a8b..4d8ff49 100644
--- a/hw/apic_internal.h
+++ b/hw/apic_internal.h
@@ -141,7 +141,6 @@ void apic_report_irq_delivered(int delivered);
 bool apic_next_timer(APICCommonState *s, int64_t current_time);
 void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
 void apic_enable_vapic(DeviceState *d, target_phys_addr_t paddr);
-void apic_poll_irq(DeviceState *d);
 
 void vapic_report_tpr_access(DeviceState *dev, void *cpu, target_ulong ip,
                              TPRAccess access);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f257c97..1f6f14f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -477,6 +477,7 @@
                                  for syscall instruction */
 
 /* i386-specific interrupt pending bits.  */
+#define CPU_INTERRUPT_POLL      CPU_INTERRUPT_TGT_EXT_1
 #define CPU_INTERRUPT_SMI       CPU_INTERRUPT_TGT_EXT_2
 #define CPU_INTERRUPT_NMI       CPU_INTERRUPT_TGT_EXT_3
 #define CPU_INTERRUPT_MCE       CPU_INTERRUPT_TGT_EXT_4
@@ -1047,7 +1048,8 @@ static inline void cpu_clone_regs(CPUX86State *env, target_ulong newsp)
 
 static inline bool cpu_has_work(CPUX86State *env)
 {
-    return ((env->interrupt_request & CPU_INTERRUPT_HARD) &&
+    return ((env->interrupt_request & (CPU_INTERRUPT_HARD |
+                                       CPU_INTERRUPT_POLL)) &&
             (env->eflags & IF_MASK)) ||
            (env->interrupt_request & (CPU_INTERRUPT_NMI |
                                       CPU_INTERRUPT_INIT |
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0d0d8f6..cfe60bc 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1727,6 +1727,10 @@ int kvm_arch_process_async_events(CPUX86State *env)
         return 0;
     }
 
+    if (env->interrupt_request & CPU_INTERRUPT_POLL) {
+        env->interrupt_request &= ~CPU_INTERRUPT_POLL;
+        apic_poll_irq(env->apic_state);
+    }
     if (((env->interrupt_request & CPU_INTERRUPT_HARD) &&
          (env->eflags & IF_MASK)) ||
         (env->interrupt_request & CPU_INTERRUPT_NMI)) {
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/3] apic: Defer interrupt updates to VCPU thread

[Jan Kiszka]

KVM performs TPR raising asynchronously to QEMU, specifically outside
QEMU's global lock. When an interrupt is injected into the APIC and TPR
is checked to decide if this can be delivered, a stale TPR value may be
used, causing spurious interrupts in the end.

Fix this by deferring apic_update_irq to the context of the target VCPU.
We introduce a new interrupt flag for this, CPU_INTERRUPT_POLL. When it
is set, the VCPU calls apic_poll_irq before checking for further pending
interrupts. To avoid special-casing KVM, we also implement this logic
for TCG mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-exec.c         |    6 ++++++
 hw/apic.c          |    5 ++++-
 hw/apic.h          |    1 +
 hw/apic_internal.h |    1 -
 target-i386/cpu.h  |    4 +++-
 target-i386/kvm.c  |    4 ++++
 6 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 08c35f7..fc185a4 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -288,6 +288,12 @@ int cpu_exec(CPUArchState *env)
                     }
 #endif
 #if defined(TARGET_I386)
+#if !defined(CONFIG_USER_ONLY)
+                    if (interrupt_request & CPU_INTERRUPT_POLL) {
+                        env->interrupt_request &= ~CPU_INTERRUPT_POLL;
+                        apic_poll_irq(env->apic_state);
+                    }
+#endif
                     if (interrupt_request & CPU_INTERRUPT_INIT) {
                             cpu_svm_check_intercept_param(env, SVM_EXIT_INIT,
                                                           0);
diff --git a/hw/apic.c b/hw/apic.c
index 5b8f3e8..385555e 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -16,6 +16,7 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>
  */
+#include "qemu-thread.h"
 #include "apic_internal.h"
 #include "apic.h"
 #include "ioapic.h"
@@ -361,7 +362,9 @@ static void apic_update_irq(APICCommonState *s)
     if (!(s->spurious_vec & APIC_SV_ENABLE)) {
         return;
     }
-    if (apic_irq_pending(s) > 0) {
+    if (!qemu_cpu_is_self(s->cpu_env)) {
+        cpu_interrupt(s->cpu_env, CPU_INTERRUPT_POLL);
+    } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
     }
 }
diff --git a/hw/apic.h b/hw/apic.h
index 62179ce..a89542b 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,6 +20,7 @@ void apic_init_reset(DeviceState *s);
 void apic_sipi(DeviceState *s);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
+void apic_poll_irq(DeviceState *d);
 
 /* pc.c */
 int cpu_is_bsp(CPUX86State *env);
diff --git a/hw/apic_internal.h b/hw/apic_internal.h
index 60a6a8b..4d8ff49 100644
--- a/hw/apic_internal.h
+++ b/hw/apic_internal.h
@@ -141,7 +141,6 @@ void apic_report_irq_delivered(int delivered);
 bool apic_next_timer(APICCommonState *s, int64_t current_time);
 void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
 void apic_enable_vapic(DeviceState *d, target_phys_addr_t paddr);
-void apic_poll_irq(DeviceState *d);
 
 void vapic_report_tpr_access(DeviceState *dev, void *cpu, target_ulong ip,
                              TPRAccess access);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f257c97..1f6f14f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -477,6 +477,7 @@
                                  for syscall instruction */
 
 /* i386-specific interrupt pending bits.  */
+#define CPU_INTERRUPT_POLL      CPU_INTERRUPT_TGT_EXT_1
 #define CPU_INTERRUPT_SMI       CPU_INTERRUPT_TGT_EXT_2
 #define CPU_INTERRUPT_NMI       CPU_INTERRUPT_TGT_EXT_3
 #define CPU_INTERRUPT_MCE       CPU_INTERRUPT_TGT_EXT_4
@@ -1047,7 +1048,8 @@ static inline void cpu_clone_regs(CPUX86State *env, target_ulong newsp)
 
 static inline bool cpu_has_work(CPUX86State *env)
 {
-    return ((env->interrupt_request & CPU_INTERRUPT_HARD) &&
+    return ((env->interrupt_request & (CPU_INTERRUPT_HARD |
+                                       CPU_INTERRUPT_POLL)) &&
             (env->eflags & IF_MASK)) ||
            (env->interrupt_request & (CPU_INTERRUPT_NMI |
                                       CPU_INTERRUPT_INIT |
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0d0d8f6..cfe60bc 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1727,6 +1727,10 @@ int kvm_arch_process_async_events(CPUX86State *env)
         return 0;
     }
 
+    if (env->interrupt_request & CPU_INTERRUPT_POLL) {
+        env->interrupt_request &= ~CPU_INTERRUPT_POLL;
+        apic_poll_irq(env->apic_state);
+    }
     if (((env->interrupt_request & CPU_INTERRUPT_HARD) &&
          (env->eflags & IF_MASK)) ||
         (env->interrupt_request & CPU_INTERRUPT_NMI)) {
-- 
1.7.3.4

Re: [PATCH 0/3] apic: Fixes for userspace model

[Avi Kivity]

On 07/09/2012 05:42 PM, Jan Kiszka wrote:
> As Avi noted recently, there is a problem in way we inject interrupts
> into the userspace APIC under KVM: The TRP check over the iothread may
> race with the VCPU raising the TPR value while in KVM mode. Patch 3
> addresses this issue.
>
> The other two patches fix problems I came across while thinking about
> the first one.
>
> Who would like to process this series, up/master? Or should it go in
> directly?

I applied to uq/master, where it can get some extra testing.  Thanks.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 0/3] apic: Fixes for userspace model

[Avi Kivity]

On 07/09/2012 05:42 PM, Jan Kiszka wrote:
> As Avi noted recently, there is a problem in way we inject interrupts
> into the userspace APIC under KVM: The TRP check over the iothread may
> race with the VCPU raising the TPR value while in KVM mode. Patch 3
> addresses this issue.
>
> The other two patches fix problems I came across while thinking about
> the first one.
>
> Who would like to process this series, up/master? Or should it go in
> directly?

I applied to uq/master, where it can get some extra testing.  Thanks.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.