Fixes race where we'd ignore a kmsg when halting
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 31 Aug 2011 02:24:04 +0000 (19:24 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:06 +0000 (17:36 -0700)
If an interrupt came in at the wrong time, right after PRKM returns
(more specifically, right after it turns on interrupts but after it
thought the list was empty), then we'd halt and miss the message til the
next interrupt.

To fix this, we want cpu_halt() to mean "turn on interrupts and then
halt the core, atomically".  x86 does this via some irq_handler()
hacking, though from what I've seen, "sti;hlt" won't get interrupts
before the hlt (not sure if this is intentional or not).

Other archs (sparc/riscv) will need to support similar semantics in
their cpu_halt(), as the code is now.  Alternatively (and slightly
preferred), we can shut down the core completely and have it come back
up and come in through the top of smp_idle().

kern/arch/i686/arch.h
kern/arch/i686/trap.c
kern/src/smp.c
kern/src/testing.c

index 75bf445..a864f97 100644 (file)
@@ -131,10 +131,12 @@ cpu_relax(void)
        __cpu_relax();
 }
 
+/* This doesn't atomically enable interrupts and then halt, like we want, so
+ * x86 needs to use a custom helper in the irq handler in trap.c. */
 static __inline void
 cpu_halt(void)
 {
-       asm volatile("hlt" : : : "memory");
+       asm volatile("sti; hlt" : : : "memory");
 }
 
 static __inline void
index 3c095fb..a0e41f4 100644 (file)
@@ -296,6 +296,19 @@ static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe **tf)
        *tf = &pcpui->actual_tf;
 }
 
+/* If the interrupt interrupted a halt, we advance past it.  Made to work with
+ * x86's custom cpu_halt() in arch/arch.h.  Note this nearly never gets called.
+ * I needed to insert exactly one 'nop' in cpu_halt() (that isn't there now) to
+ * get the interrupt to trip on the hlt, o/w the hlt will execute before the
+ * interrupt arrives (even with a pending interrupt that should hit right after
+ * an interrupt_enable (sti)).  This was on the i7. */
+static void abort_halt(struct trapframe *tf)
+{
+       /* the halt instruction in 32 bit is 0xf4, and it's size is 1 byte */
+       if (*(uint8_t*)tf->tf_eip == 0xf4)
+               tf->tf_eip += 1;
+}
+
 void trap(struct trapframe *tf)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
@@ -325,7 +338,8 @@ void irq_handler(struct trapframe *tf)
        /* Copy out the TF for now, set tf to point to it. */
        if (!in_kernel(tf))
                set_current_tf(pcpui, &tf);
-
+       /* Coupled with cpu_halt() and smp_idle() */
+       abort_halt(tf);
        //if (core_id())
                printd("Incoming IRQ, ISR: %d on core %d\n", tf->tf_trapno, core_id());
 
index 66bb502..6b43226 100644 (file)
@@ -51,10 +51,15 @@ static void __smp_idle(void)
         * for now, it is possible to have a current loaded, even if we are idle
         * (and presumably about to execute a kmsg or fire up a vcore). */
        if (!management_core()) {
-               enable_irq();
                while (1) {
+                       disable_irq();
                        process_routine_kmsg(0);
+                       /* cpu_halt() atomically turns on interrupts and halts the core.
+                        * Important to do this, since we could have a RKM come in via an
+                        * interrupt right while PRKM is returning, and we wouldn't catch
+                        * it. */
                        cpu_halt();
+                       /* interrupts are back on now (given our current semantics) */
                }
        } else {
                enable_irqsave(&state);
index 226edb6..b7183c5 100644 (file)
@@ -721,7 +721,6 @@ void test_kernel_messages(void)
        return;
 }
 #endif // __i386__
-
 static void test_single_cache(int iters, size_t size, int align, int flags,
                               void (*ctor)(void *, size_t),
                               void (*dtor)(void *, size_t))
@@ -1421,3 +1420,29 @@ void test_atomics(void)
        test_cas_val(257);
        test_cas_val(1);
 }
+
+/* x86 test, making sure our cpu_halt() and irq_handler() work.  If you want to
+ * see it fail, you'll probably need to put a nop in the asm for cpu_halt(), and
+ * comment out abort_halt() in irq_handler(). */
+void test_abort_halt(void)
+{
+#ifdef __i386__
+       /* Core 1 does this, while core 0 hammers it with interrupts */
+       void test_try_halt(struct trapframe *tf, uint32_t srcid, long a0, long a1,
+                          long a2)
+       {
+               disable_irq();
+               /* wait 10 sec.  should have a bunch of ints pending */
+               udelay(10000000);
+               printk("Core 1 is about to halt\n");
+               cpu_halt();
+               printk("Returned from halting on core 1\n");
+       }
+       send_kernel_message(1, test_try_halt, 0, 0, 0, KMSG_ROUTINE);
+       /* wait 1 sec, enough time to for core 1 to be in its KMSG */
+       udelay(1000000);
+       /* Send an IPI */
+       send_ipi(get_hw_coreid(0x01), I_TESTING);
+       printk("Core 0 sent the IPI\n");
+#endif /* __i386__ */
+}