x86: fixes lock debug issues with the new core_id
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 17 Jun 2014 22:34:51 +0000 (15:34 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 17 Jun 2014 23:33:55 +0000 (16:33 -0700)
When spinlock debugging, we would do a core_id_early call and get gibberish
back.  This was because core_id_ready was set (it was being set once the LAPIC
was available, and not when the segmentation stuff was available).  Instead,
we'd get a garbage address and fault enough to reboot the machine.

The fix is to not start using core_id too early.

In doing so, we also expose an issue with the lock debugger being used.  The
older LAPIC styles were ready earlier than smp_boot (in vm_init()).  Now,
core_id isn't ready til the smp boot is mostly done.  In the meantime, the
non-zero cores will think they are core 0, which means all of them share a
pcpui->lock_depth.  Those cores are running concurrently, and the
pcpui->lock_depth isn't protected in any way.  Eventually, we'd screw up the
lock depth.  The fix there is to just not debug locks for a little while.

kern/arch/x86/apic.c
kern/arch/x86/apic.h
kern/arch/x86/coreid.h
kern/arch/x86/pmap32.c
kern/arch/x86/pmap64.c
kern/arch/x86/smp_boot.c
kern/src/kthread.c

index 31602f3..c3effdd 100644 (file)
@@ -20,8 +20,6 @@
 #include <bitmask.h>
 #include <arch/coreid.h>
 
-bool core_id_ready = FALSE;
-
 bool lapic_check_spurious(int trap_nr)
 {
 #ifndef CONFIG_ENABLE_MPTABLES
index b85f61d..c13d0de 100644 (file)
 #define LAPIC_ISR                                      (LAPIC_BASE + 0x100)
 #define LAPIC_IRR                                      (LAPIC_BASE + 0x200)
 
-/* Tracks whether it is safe to execute core_id() or not.  If we're using the
- * LAPIC, we need to have the LAPIC mapped into VM.  vm_init() sets this to
- * TRUE.
- *
- * If we're using rdtscp, if the instruction is supported, we can call core_id()
- * without rebooting.  cpuinfo should have panic'd if we're running on a machine
- * that doesn't support rdtscp, before vm_init().
- *
- * If we're using something else (like segmentation), then that will need to get
- * set up before vm_init(), at least for core 0.
- *
- * Note that core_id() will return 0 (or possibly another wrong answer) on cores
- * other than core 0 when it is called before smp_boot completes. */
-extern bool core_id_ready;
 struct irq_handler;    /* include loops */
 
 bool lapic_check_spurious(int trap_nr);
index 2f383e3..5c319f8 100644 (file)
@@ -59,6 +59,8 @@ static inline int core_id(void)
 }
 #endif /* CONFIG_X86_64 */
 
+/* Tracks whether it is safe to execute core_id() or not. */
+extern bool core_id_ready;
 static inline int core_id_early(void)
 {
        if (!core_id_ready)
index 526202b..263ecd5 100644 (file)
@@ -276,7 +276,6 @@ vm_init(void)
 
        // Flush the TLB for good measure, to kill the pgdir[0] mapping.
        tlb_flush_global();
-       core_id_ready = TRUE;
 }
 
 void x86_cleanup_bootmem(void)
index 009c1d1..698ab83 100644 (file)
@@ -452,11 +452,6 @@ void vm_init(void)
        gdt_pd = (pseudodesc_t) {sizeof(segdesc_t) * SEG_COUNT - 1,
                                 (uintptr_t)gdt};
        asm volatile("lgdt %0" : : "m"(gdt_pd));
-       /* LAPIC is mapped, if we're using that.  if we're using rdtscp or some
-        * other non-LAPIC method, we can (probably) start using it right away.  we
-        * may get 0 back on other cores, if smp_boot hasn't completed, though
-        * that's no different than before this is TRUE. */
-       core_id_ready = TRUE;
 }
 
 void x86_cleanup_bootmem(void)
index 64d8714..981dc00 100644 (file)
@@ -60,6 +60,8 @@ static void init_smp_call_function(void)
 
 /******************************************************************************/
 
+bool core_id_ready = FALSE;
+
 static void setup_rdtscp(int coreid)
 {
        uint32_t edx;
@@ -80,13 +82,16 @@ void smp_final_core_init(void)
 {
        /* It is possible that the non-0 cores will wake up before the broadcast
         * ipi.  this can be due to spurious IRQs or some such.  anyone other than
-        * core 0 that comes in here will wait til core 0 has set everything up */
+        * core 0 that comes in here will wait til core 0 has set everything up.
+        * those other cores might have come up before core 0 remapped the coreids,
+        * so we can only look at the HW coreid, which is only 0 for core 0. */
        static bool wait = TRUE;
-       if (get_os_coreid(hw_core_id()) == 0)
+       if (hw_core_id() == 0)
                wait = FALSE;
        while (wait)
                cpu_relax();
 #ifdef CONFIG_X86_64
+       /* at this point, it is safe to get the OS coreid */
        int coreid = get_os_coreid(hw_core_id());
        struct per_cpu_info *pcpui = &per_cpu_info[coreid];
        pcpui->coreid = coreid;
@@ -95,6 +100,18 @@ void smp_final_core_init(void)
 #endif
        /* don't need this for the kernel anymore, but userspace can still use it */
        setup_rdtscp(coreid);
+       /* After this point, all cores have set up their segmentation and whatnot to
+        * be able to do a proper core_id().  As a note to posterity, using the
+        * LAPIC coreid (like get_hw_coreid()) needs the LAPIC set up, which happens
+        * by the end of vm_init() */
+       waiton_barrier(&generic_barrier);
+       if (hw_core_id() == 0) {
+               core_id_ready = TRUE;
+               cmb();
+               pcpui->__lock_checking_enabled = 1;
+       }
+       /* being paranoid with this, it's all a bit ugly */
+       waiton_barrier(&generic_barrier);
        setup_default_mtrrs(&generic_barrier);
        smp_percpu_init();
        waiton_barrier(&generic_barrier);
@@ -145,6 +162,7 @@ static void smp_remap_coreids(void)
 
 void smp_boot(void)
 {
+       struct per_cpu_info *pcpui0 = &per_cpu_info[0];
        /* set core0's mappings */
        assert(lapic_get_id() == 0);
        os_coreid_lookup[0] = 0;
@@ -161,7 +179,8 @@ void smp_boot(void)
 #ifndef CONFIG_X86_64
        // This mapping allows access to the trampoline with paging on and off
        // via trampoline_pg
-       page_insert(boot_pgdir, pa2page(trampoline_pg), (void*SNT)trampoline_pg, PTE_W);
+       page_insert(boot_pgdir, pa2page(trampoline_pg), (void*SNT)trampoline_pg,
+                   PTE_W);
 #endif
 
        // Allocate a stack for the cores starting up.  One for all, must share
@@ -169,6 +188,10 @@ void smp_boot(void)
                panic("No memory for SMP boot stack!");
        smp_stack_top = SINIT((uintptr_t)(page2kva(smp_stack) + PGSIZE));
 
+       /* During SMP boot, core_id_early() returns 0, so all of the cores, which
+        * grab locks concurrently, share the same pcpui and thus the same
+        * lock_depth.  We need to disable checking until core_id works properly. */
+       pcpui0->__lock_checking_enabled = 0;
        // Start the IPI process (INIT, wait, SIPI, wait, SIPI, wait)
        send_init_ipi();
        // SDM 3A is a little wonky wrt the proper delays.  These are my best guess.
index 3fa7b81..7a8738e 100644 (file)
@@ -282,7 +282,8 @@ void sem_down(struct semaphore *sem)
 
        assert(can_block(pcpui));
        /* Make sure we aren't holding any locks (only works if SPINLOCK_DEBUG) */
-       assert(!pcpui->lock_depth);
+       if (pcpui->lock_depth)
+               panic("Kthread tried to sleep, with lockdepth %d\n", pcpui->lock_depth);
        assert(pcpui->cur_kthread);
        /* Try to down the semaphore.  If there is a signal there, we can skip all
         * of the sleep prep and just return. */