[Qemu-devel] [PATCH] APIC: add NMI and SMI IPI support

[Qemu-devel] [PATCH] APIC: add NMI and SMI IPI support

[Jan Kiszka]

While testing KGDB (yeah, it actually seem to make it into mainline!)
under QEMU, I failed to get it running in SMP mode. Reason: NMI IPIs are
not correctly handled by QEMU's emulated APIC.

To overcome this, the patch below introduces a new interruption request,
CPU_INTERRUPT_NMI, so that a VCPU can cleanly send this special
interrupt to other VCPUs. It also introduces HF_NMI_MASK which shall
ensure that NMIs are not recursively triggered, but I must confess that
this particular property was not really tested yet.

CPU_INTERRUPT_NMI is then trivially exploited by apic_bus_deliver to
send out both NMIs and (for the sake of completeness - it's untested as
well SMIs).

With this patch applied, I'm finally able to run (and potentially debug)
KGDB for Linux SMP guests.

Jan

---
 cpu-all.h            |    1 +
 cpu-exec.c           |    6 ++++++
 hw/apic.c            |    8 +++++++-
 target-i386/cpu.h    |    2 ++
 target-i386/helper.c |    2 ++
 5 files changed, 18 insertions(+), 1 deletion(-)

Index: b/hw/apic.c
===================================================================
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -216,8 +216,14 @@ static void apic_bus_deliver(const uint3
             break;
 
         case APIC_DM_SMI:
+            foreach_apic(apic_iter, deliver_bitmask,
+                cpu_interrupt(apic_iter->cpu_env, CPU_INTERRUPT_SMI) );
+            return;
+
         case APIC_DM_NMI:
-            break;
+            foreach_apic(apic_iter, deliver_bitmask,
+                cpu_interrupt(apic_iter->cpu_env, CPU_INTERRUPT_NMI) );
+            return;
 
         case APIC_DM_INIT:
             /* normal INIT IPI sent to processors */
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -748,6 +748,7 @@ extern int code_copy_enabled;
 #define CPU_INTERRUPT_SMI    0x40 /* (x86 only) SMI interrupt pending */
 #define CPU_INTERRUPT_DEBUG  0x80 /* Debug event occured.  */
 #define CPU_INTERRUPT_VIRQ   0x100 /* virtual interrupt pending.  */
+#define CPU_INTERRUPT_NMI    0x200 /* NMI pending. */
 
 void cpu_interrupt(CPUState *s, int mask);
 void cpu_reset_interrupt(CPUState *env, int mask);
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -505,6 +505,12 @@ int cpu_exec(CPUState *env1)
                         env->interrupt_request &= ~CPU_INTERRUPT_SMI;
                         do_smm_enter();
                         BREAK_CHAIN;
+                    } else if ((interrupt_request & CPU_INTERRUPT_NMI) &&
+                        !(env->hflags & HF_NMI_MASK)) {
+                        env->interrupt_request &= ~CPU_INTERRUPT_NMI;
+                        env->hflags |= HF_NMI_MASK;
+                        do_interrupt(EXCP02_NMI, 0, 0, 0, 1);
+                        BREAK_CHAIN;
                     } else if ((interrupt_request & CPU_INTERRUPT_HARD) &&
                         (env->eflags & IF_MASK || env->hflags & HF_HIF_MASK) &&
                         !(env->hflags & HF_INHIBIT_IRQ_MASK)) {
Index: b/target-i386/cpu.h
===================================================================
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -148,6 +148,7 @@
 #define HF_SMM_SHIFT        19 /* CPU in SMM mode */
 #define HF_GIF_SHIFT        20 /* if set CPU takes interrupts */
 #define HF_HIF_SHIFT        21 /* shadow copy of IF_MASK when in SVM */
+#define HF_NMI_SHIFT        22 /* CPU serving NMI */
 
 #define HF_CPL_MASK          (3 << HF_CPL_SHIFT)
 #define HF_SOFTMMU_MASK      (1 << HF_SOFTMMU_SHIFT)
@@ -167,6 +168,7 @@
 #define HF_SMM_MASK          (1 << HF_SMM_SHIFT)
 #define HF_GIF_MASK          (1 << HF_GIF_SHIFT)
 #define HF_HIF_MASK          (1 << HF_HIF_SHIFT)
+#define HF_NMI_MASK          (1 << HF_NMI_SHIFT)
 
 #define CR0_PE_MASK  (1 << 0)
 #define CR0_MP_MASK  (1 << 1)
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -2382,6 +2382,7 @@ void helper_iret_real(int shift)
     if (shift == 0)
         eflags_mask &= 0xffff;
     load_eflags(new_eflags, eflags_mask);
+    env->hflags &= ~HF_NMI_MASK;
 }
 
 static inline void validate_seg(int seg_reg, int cpl)
@@ -2633,6 +2634,7 @@ void helper_iret_protected(int shift, in
     } else {
         helper_ret_protected(shift, 1, 0);
     }
+    env->hflags &= ~HF_NMI_MASK;
 #ifdef USE_KQEMU
     if (kqemu_is_ok(env)) {
         CC_OP = CC_OP_EFLAGS;

Re: [Qemu-devel] [PATCH] APIC: add NMI and SMI IPI support

[Robi Yagel]

[-- Attachment #1: Type: text/plain, Size: 4840 bytes --]

Hello,
Thanks for the patch, if you can, please advice on the proper place to add
periodic generation of SMI/NMIs in order to simulate, e.g., a watchdog (and the
needed parameters - except for CPU_INTERRUPT_NMI...)
Thanks in advance, Robi



>> original message from Jan Kiszka on Mon, 28 Jan 2008 12:38:19 -0800

While testing KGDB (yeah, it actually seem to make it into mainline!)
under QEMU, I failed to get it running in SMP mode. Reason: NMI IPIs are
not correctly handled by QEMU's emulated APIC.

To overcome this, the patch below introduces a new interruption request,
CPU_INTERRUPT_NMI, so that a VCPU can cleanly send this special
interrupt to other VCPUs. It also introduces HF_NMI_MASK which shall
ensure that NMIs are not recursively triggered, but I must confess that
this particular property was not really tested yet.

CPU_INTERRUPT_NMI is then trivially exploited by apic_bus_deliver to
send out both NMIs and (for the sake of completeness - it's untested as
well SMIs).

With this patch applied, I'm finally able to run (and potentially debug)
KGDB for Linux SMP guests.

Jan

---
 cpu-all.h            |    1 +
 cpu-exec.c           |    6 ++++++
 hw/apic.c            |    8 +++++++-
 target-i386/cpu.h    |    2 ++
 target-i386/helper.c |    2 ++
 5 files changed, 18 insertions(+), 1 deletion(-)

Index: b/hw/apic.c
===================================================================
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -216,8 +216,14 @@ static void apic_bus_deliver(const uint3
             break;

         case APIC_DM_SMI:
+            foreach_apic(apic_iter, deliver_bitmask,
+                cpu_interrupt(apic_iter->cpu_env, CPU_INTERRUPT_SMI) );
+            return;
+
         case APIC_DM_NMI:
-            break;
+            foreach_apic(apic_iter, deliver_bitmask,
+                cpu_interrupt(apic_iter->cpu_env, CPU_INTERRUPT_NMI) );
+            return;

         case APIC_DM_INIT:
             /* normal INIT IPI sent to processors */
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -748,6 +748,7 @@ extern int code_copy_enabled;
 #define CPU_INTERRUPT_SMI    0x40 /* (x86 only) SMI interrupt pending */
 #define CPU_INTERRUPT_DEBUG  0x80 /* Debug event occured.  */
 #define CPU_INTERRUPT_VIRQ   0x100 /* virtual interrupt pending.  */
+#define CPU_INTERRUPT_NMI    0x200 /* NMI pending. */

 void cpu_interrupt(CPUState *s, int mask);
 void cpu_reset_interrupt(CPUState *env, int mask);
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -505,6 +505,12 @@ int cpu_exec(CPUState *env1)
                         env->interrupt_request &= ~CPU_INTERRUPT_SMI;
                         do_smm_enter();
                         BREAK_CHAIN;
+                    } else if ((interrupt_request & CPU_INTERRUPT_NMI) &&
+                        !(env->hflags & HF_NMI_MASK)) {
+                        env->interrupt_request &= ~CPU_INTERRUPT_NMI;
+                        env->hflags |= HF_NMI_MASK;
+                        do_interrupt(EXCP02_NMI, 0, 0, 0, 1);
+                        BREAK_CHAIN;
                     } else if ((interrupt_request & CPU_INTERRUPT_HARD) &&
                         (env->eflags & IF_MASK || env->hflags & HF_HIF_MASK) &&
                         !(env->hflags & HF_INHIBIT_IRQ_MASK)) {
Index: b/target-i386/cpu.h
===================================================================
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -148,6 +148,7 @@
 #define HF_SMM_SHIFT        19 /* CPU in SMM mode */
 #define HF_GIF_SHIFT        20 /* if set CPU takes interrupts */
 #define HF_HIF_SHIFT        21 /* shadow copy of IF_MASK when in SVM */
+#define HF_NMI_SHIFT        22 /* CPU serving NMI */

 #define HF_CPL_MASK          (3 << HF_CPL_SHIFT)
 #define HF_SOFTMMU_MASK      (1 << HF_SOFTMMU_SHIFT)
@@ -167,6 +168,7 @@
 #define HF_SMM_MASK          (1 << HF_SMM_SHIFT)
 #define HF_GIF_MASK          (1 << HF_GIF_SHIFT)
 #define HF_HIF_MASK          (1 << HF_HIF_SHIFT)
+#define HF_NMI_MASK          (1 << HF_NMI_SHIFT)

 #define CR0_PE_MASK  (1 << 0)
 #define CR0_MP_MASK  (1 << 1)
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -2382,6 +2382,7 @@ void helper_iret_real(int shift)
     if (shift == 0)
         eflags_mask &= 0xffff;
     load_eflags(new_eflags, eflags_mask);
+    env->hflags &= ~HF_NMI_MASK;
 }

 static inline void validate_seg(int seg_reg, int cpl)
@@ -2633,6 +2634,7 @@ void helper_iret_protected(int shift, in
     } else {
         helper_ret_protected(shift, 1, 0);
     }
+    env->hflags &= ~HF_NMI_MASK;
 #ifdef USE_KQEMU
     if (kqemu_is_ok(env)) {
         CC_OP = CC_OP_EFLAGS;

[-- Attachment #2: Type: text/html, Size: 5479 bytes --]

Re: [Qemu-devel] [PATCH] APIC: add NMI and SMI IPI support

[Jan Kiszka]

Robi Yagel wrote:
> Hello,
> Thanks for the patch,

Nice to hear that there is interest in this. :)

> if you can, please advice on the proper place to add
> periodic generation of SMI/NMIs in order to simulate, e.g., a watchdog (and the
> needed parameters - except for CPU_INTERRUPT_NMI...)

Yeah, I also thought about NMI watchdog emulation while adding
CPU_INTERRUPT_NMI, but it took a bit more effort to understand what
pieces are missing. It should be a fairly useful feature for testing NMI
interaction with new kernel code and maybe even catching nasty races
that way (whenever you happen to work on such things, like I do ;) ).
Also, we need NMI delivery for custom hardware emulation here. So there
might be broader use for CPU_INTERRUPT_NMI in the future.

However, find some experimental watchdog-enabler patch below. It allows
to use nmi_watchdog=1 (i.e. the IO-APIC variant) with Linux, without any
visible regression of normal IRQ delivery (but I only lightly tested it
on top of the kvm-userspace qemu, that's why "experimental").

A performance counter based NMI watchdog is surely doable as well, but
it takes a bit more effort. You would have to decide first what
perf-counter features should be emulated and how cleanly, even if you
only want to (mis-)use it for watchdog services. I'm lacking time for
this right now.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux


---
 hw/apic.c |   38 +++++++++++++++++++++++++++++++++-----
 hw/pc.c   |    2 +-
 hw/pc.h   |    3 +++
 3 files changed, 37 insertions(+), 6 deletions(-)

Index: b/hw/apic.c
===================================================================
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -166,6 +166,37 @@ static inline void reset_bit(uint32_t *t
     tab[i] &= ~mask;
 }
 
+void apic_local_deliver(CPUState *env, int vector)
+{
+    APICState *s = env->apic_state;
+    uint32_t lvt = s->lvt[vector];
+    int trigger_mode;
+
+    if (lvt & APIC_LVT_MASKED)
+        return;
+
+    switch ((lvt >> 8) & 7) {
+    case APIC_DM_SMI:
+        cpu_interrupt(env, CPU_INTERRUPT_SMI);
+        break;
+
+    case APIC_DM_NMI:
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+        break;
+
+    case APIC_DM_EXTINT:
+        cpu_interrupt(env, CPU_INTERRUPT_HARD);
+        break;
+
+    case APIC_DM_FIXED:
+        trigger_mode = APIC_TRIGGER_EDGE;
+        if ((vector == APIC_LVT_LINT0 || vector == APIC_LVT_LINT1) &&
+            (lvt & APIC_LVT_LEVEL_TRIGGER))
+            trigger_mode = APIC_TRIGGER_LEVEL;
+        apic_set_irq(s, lvt & 0xff, trigger_mode);
+    }
+}
+
 #define foreach_apic(apic, deliver_bitmask, code) \
 {\
     int __i, __j, __mask;\
@@ -504,8 +535,7 @@ int apic_accept_pic_intr(CPUState *env)
 
     if (s->id == 0 &&
         ((s->apicbase & MSR_IA32_APICBASE_ENABLE) == 0 ||
-         ((lvt0 & APIC_LVT_MASKED) == 0 &&
-          ((lvt0 >> 8) & 0x7) == APIC_DM_EXTINT)))
+         (lvt0 & APIC_LVT_MASKED) == 0))
         return 1;
 
     return 0;
@@ -556,9 +586,7 @@ static void apic_timer(void *opaque)
 {
     APICState *s = opaque;
 
-    if (!(s->lvt[APIC_LVT_TIMER] & APIC_LVT_MASKED)) {
-        apic_set_irq(s, s->lvt[APIC_LVT_TIMER] & 0xff, APIC_TRIGGER_EDGE);
-    }
+    apic_local_deliver(s->cpu_env, APIC_LVT_TIMER);
     apic_timer_update(s, s->next_time);
 }
 
Index: b/hw/pc.c
===================================================================
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -115,7 +115,7 @@ static void pic_irq_request(void *opaque
 {
     CPUState *env = opaque;
     if (level && apic_accept_pic_intr(env))
-        cpu_interrupt(env, CPU_INTERRUPT_HARD);
+        apic_local_deliver(env, APIC_LINT0);
 }
 
 /* PC cmos mappings */
Index: b/hw/pc.h
===================================================================
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -39,8 +39,11 @@ void irq_info(void);
 /* APIC */
 typedef struct IOAPICState IOAPICState;
 
+#define APIC_LINT0	3
+
 int apic_init(CPUState *env);
 int apic_accept_pic_intr(CPUState *env);
+void apic_local_deliver(CPUState *env, int vector);
 int apic_get_interrupt(CPUState *env);
 IOAPICState *ioapic_init(void);
 void ioapic_set_irq(void *opaque, int vector, int level);

[Qemu-devel] [PATCH] Rework local IRQ delivery for APICs (was:[PATCH] APIC: add NMI and SMI IPI support)

[Jan Kiszka]

Jan Kiszka wrote:
> However, find some experimental watchdog-enabler patch below. It allows
> to use nmi_watchdog=1 (i.e. the IO-APIC variant) with Linux, without any
> visible regression of normal IRQ delivery (but I only lightly tested it
> on top of the kvm-userspace qemu, that's why "experimental").

As I was lacking NMIs on CPUs > 0, I thought about IRQ delivery again
and came to the conclusion, that we may better broadcast the PIC IRQ to
all APICs. After fixing the current APIC initialization, this actually
works as desired. Updated patch below.

Jan

---
 hw/apic.c |   63 ++++++++++++++++++++++++++++++++++++++++++--------------------
 hw/pc.c   |   15 ++++++++++----
 hw/pc.h   |    3 ++
 3 files changed, 57 insertions(+), 24 deletions(-)

Index: b/hw/apic.c
===================================================================
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -166,6 +166,37 @@ static inline void reset_bit(uint32_t *t
     tab[i] &= ~mask;
 }
 
+void apic_local_deliver(CPUState *env, int vector)
+{
+    APICState *s = env->apic_state;
+    uint32_t lvt = s->lvt[vector];
+    int trigger_mode;
+
+    if (lvt & APIC_LVT_MASKED)
+        return;
+
+    switch ((lvt >> 8) & 7) {
+    case APIC_DM_SMI:
+        cpu_interrupt(env, CPU_INTERRUPT_SMI);
+        break;
+
+    case APIC_DM_NMI:
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+        break;
+
+    case APIC_DM_EXTINT:
+        cpu_interrupt(env, CPU_INTERRUPT_HARD);
+        break;
+
+    case APIC_DM_FIXED:
+        trigger_mode = APIC_TRIGGER_EDGE;
+        if ((vector == APIC_LVT_LINT0 || vector == APIC_LVT_LINT1) &&
+            (lvt & APIC_LVT_LEVEL_TRIGGER))
+            trigger_mode = APIC_TRIGGER_LEVEL;
+        apic_set_irq(s, lvt & 0xff, trigger_mode);
+    }
+}
+
 #define foreach_apic(apic, deliver_bitmask, code) \
 {\
     int __i, __j, __mask;\
@@ -502,10 +533,8 @@ int apic_accept_pic_intr(CPUState *env)
 
     lvt0 = s->lvt[APIC_LVT_LINT0];
 
-    if (s->id == 0 &&
-        ((s->apicbase & MSR_IA32_APICBASE_ENABLE) == 0 ||
-         ((lvt0 & APIC_LVT_MASKED) == 0 &&
-          ((lvt0 >> 8) & 0x7) == APIC_DM_EXTINT)))
+    if ((s->apicbase & MSR_IA32_APICBASE_ENABLE) == 0 ||
+        (lvt0 & APIC_LVT_MASKED) == 0)
         return 1;
 
     return 0;
@@ -556,9 +585,7 @@ static void apic_timer(void *opaque)
 {
     APICState *s = opaque;
 
-    if (!(s->lvt[APIC_LVT_TIMER] & APIC_LVT_MASKED)) {
-        apic_set_irq(s, s->lvt[APIC_LVT_TIMER] & 0xff, APIC_TRIGGER_EDGE);
-    }
+    apic_local_deliver(s->cpu_env, APIC_LVT_TIMER);
     apic_timer_update(s, s->next_time);
 }
 
@@ -818,12 +845,14 @@ static void apic_reset(void *opaque)
     APICState *s = opaque;
     apic_init_ipi(s);
 
-    /*
-     * LINT0 delivery mode is set to ExtInt at initialization time
-     * typically by BIOS, so PIC interrupt can be delivered to the
-     * processor when local APIC is enabled.
-     */
-    s->lvt[APIC_LVT_LINT0] = 0x700;
+    if (s->id == 0) {
+        /*
+         * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
+         * time typically by BIOS, so PIC interrupt can be delivered to the
+         * processor when local APIC is enabled.
+         */
+        s->lvt[APIC_LVT_LINT0] = 0x700;
+    }
 }
 
 static CPUReadMemoryFunc *apic_mem_read[3] = {
@@ -848,19 +877,13 @@ int apic_init(CPUState *env)
     if (!s)
         return -1;
     env->apic_state = s;
-    apic_init_ipi(s);
     s->id = last_apic_id++;
     env->cpuid_apic_id = s->id;
     s->cpu_env = env;
     s->apicbase = 0xfee00000 |
         (s->id ? 0 : MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE;
 
-    /*
-     * LINT0 delivery mode is set to ExtInt at initialization time
-     * typically by BIOS, so PIC interrupt can be delivered to the
-     * processor when local APIC is enabled.
-     */
-    s->lvt[APIC_LVT_LINT0] = 0x700;
+    apic_reset(s);
 
     /* XXX: mapping more APICs at the same memory location */
     if (apic_io_memory == 0) {
Index: b/hw/pc.c
===================================================================
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -113,9 +113,16 @@ int cpu_get_pic_interrupt(CPUState *env)
 
 static void pic_irq_request(void *opaque, int irq, int level)
 {
-    CPUState *env = opaque;
-    if (level && apic_accept_pic_intr(env))
-        cpu_interrupt(env, CPU_INTERRUPT_HARD);
+    CPUState *env = first_cpu;
+
+    if (!level)
+        return;
+
+    while (env) {
+        if (apic_accept_pic_intr(env))
+            apic_local_deliver(env, APIC_LINT0);
+        env = env->next_cpu;
+    }
 }
 
 /* PC cmos mappings */
@@ -841,7 +848,7 @@ static void pc_init1(int ram_size, int v
     if (linux_boot)
 	load_linux(kernel_filename, initrd_filename, kernel_cmdline);
 
-    cpu_irq = qemu_allocate_irqs(pic_irq_request, first_cpu, 1);
+    cpu_irq = qemu_allocate_irqs(pic_irq_request, NULL, 1);
     i8259 = i8259_init(cpu_irq[0]);
     ferr_irq = i8259[13];
 
Index: b/hw/pc.h
===================================================================
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -39,8 +39,11 @@ void irq_info(void);
 /* APIC */
 typedef struct IOAPICState IOAPICState;
 
+#define APIC_LINT0	3
+
 int apic_init(CPUState *env);
 int apic_accept_pic_intr(CPUState *env);
+void apic_local_deliver(CPUState *env, int vector);
 int apic_get_interrupt(CPUState *env);
 IOAPICState *ioapic_init(void);
 void ioapic_set_irq(void *opaque, int vector, int level);