diff --git a/app/display.c b/app/display.c index 72919e0..4b73590 100644 --- a/app/display.c +++ b/app/display.c @@ -6,6 +6,7 @@ #include #include "cpuid.h" +#include "cpulocal.h" #include "cpuinfo.h" #include "hwctrl.h" #include "i2c_x86.h" @@ -15,6 +16,7 @@ #include "serial.h" #include "pmem.h" #include "smbios.h" +#include "smp.h" #include "spd.h" #include "temperature.h" #include "tsc.h" @@ -539,16 +541,20 @@ void scroll(void) if (scroll_message_row < ROW_SCROLL_B) { scroll_message_row++; } else { - if (scroll_lock) { - display_footer_message(" Single step "); - } - scroll_wait = true; - do { - check_input(); - } while (scroll_wait && scroll_lock); + // Only the master CPU may poll the keyboard, so the scroll-lock + // single-step wait is only available to it. + if (smp_my_cpu_num() == master_cpu) { + if (scroll_lock) { + display_footer_message(" Single step "); + } + scroll_wait = true; + do { + check_input(); + } while (scroll_wait && scroll_lock); - scroll_wait = false; - clear_footer_message(); + scroll_wait = false; + clear_footer_message(); + } scroll_screen_region(ROW_SCROLL_T, 0, ROW_SCROLL_B, SCREEN_WIDTH - 1); } } @@ -632,6 +638,15 @@ void do_tick(int my_cpu) // This only tick one time per second if (!timed_update_done) { + // A corrupted stack canary means a CPU overran its stack slot and may + // have corrupted the thread-local barrier flags below it (see boot.h). + static int last_overflow_cpu = -1; + int overflow_cpu = stack_canary_check(); + if (overflow_cpu >= 0 && overflow_cpu != last_overflow_cpu) { + last_overflow_cpu = overflow_cpu; + do_trace(overflow_cpu, "CPU stack overflow detected - test results are unreliable"); + } + // Display FAIL banner if (new) errors detected if (err_banner_redraw && !big_status_displayed && error_count > 1) { display_big_status(false); diff --git a/app/error.c b/app/error.c index 75d9eb8..3bf7a66 100644 --- a/app/error.c +++ b/app/error.c @@ -14,6 +14,7 @@ #include #include "smp.h" + #include "vmem.h" #include "badram.h" @@ -276,7 +277,11 @@ static void common_err(error_type_t type, uintptr_t addr, testword_t good, testw #endif } if (new_address) { - check_input(); + // Only the master CPU may poll the keyboard: polling from other + // CPUs would race the master's polling. + if (smp_my_cpu_num() == master_cpu) { + check_input(); + } scroll(); set_foreground_colour(YELLOW); diff --git a/app/loongarch/interrupt.c b/app/loongarch/interrupt.c index 66b9792..5afcc86 100644 --- a/app/loongarch/interrupt.c +++ b/app/loongarch/interrupt.c @@ -186,7 +186,9 @@ void interrupt(struct system_context *system_context) ecode = (system_context->estat >> 16) & 0x3F; - spin_lock(error_mutex); + // Don't wait for the error mutex: it may be held by a stopped CPU, or by + // this very CPU if the interrupt was taken inside the error reporting path. + spin_trylock(error_mutex); clear_message_area(); diff --git a/app/main.c b/app/main.c index 0724b63..bb21a73 100644 --- a/app/main.c +++ b/app/main.c @@ -21,6 +21,7 @@ #include "acpi.h" #include "cache.h" #include "cpuid.h" +#include "cpulocal.h" #include "cpuinfo.h" #include "heap.h" #include "hwctrl.h" @@ -177,10 +178,50 @@ static void run_at(uintptr_t addr, int my_cpu) #endif } +// A BSP-only version of run_at(), used before the APs have been started. The +// thread-local storage is zeroed, as its original copy may not be backed by RAM. +static void relocate_to(uintptr_t addr) +{ + uintptr_t *new_start_addr = (uintptr_t *)(addr + startup - _start); + + // Copy the program code and all data except the stacks. + memmove((void *)addr, (void *)_start, _stacks - _start); + // Zero the thread-local storage. + size_t locals_offset = _stacks - _start + BSP_STACK_SIZE - LOCALS_SIZE; + for (int cpu_num = 0; cpu_num < num_available_cpus; cpu_num++) { + memset((void *)(addr + locals_offset), 0, LOCALS_SIZE); + locals_offset += AP_STACK_SIZE; + } + + // Jump to new_start_addr. +#ifdef __i386__ + // The 32-bit startup code needs to know where it is located. + __asm__ __volatile__("movl %0, %%edi; jmp *%0" : : "r" (new_start_addr)); + __builtin_unreachable(); +#else + ((void (*)(void))new_start_addr)(); +#endif +} + +// Checks that the given address range lies entirely within a single region of +// usable RAM (the BIOS bootloader may have loaded us straddling the VGA/ROM hole). +static bool addr_range_is_usable(uintptr_t start, size_t size) +{ + for (int i = 0; i < pm_map_size; i++) { + uintptr_t region_start = pm_map[i].start << PAGE_SHIFT; + uintptr_t region_end = pm_map[i].end << PAGE_SHIFT; + if (start >= region_start && (start + size) <= region_end) { + return true; + } + } + return false; +} + static bool set_load_addr(uintptr_t *load_addr, size_t program_size, uintptr_t lower_limit, uintptr_t upper_limit) { uintptr_t current_start = (uintptr_t)_start; - if (current_start >= lower_limit && (current_start + program_size) <= upper_limit) { + if (current_start >= lower_limit && (current_start + program_size) <= upper_limit + && addr_range_is_usable(current_start, program_size)) { *load_addr = current_start; return true; } @@ -206,6 +247,13 @@ static bool set_load_addr(uintptr_t *load_addr, size_t program_size, uintptr_t l static void global_init(void) { + // Set once initialisation is complete; the early relocation below + // restarts the program and re-enters here. + static bool init_complete = false; + if (init_complete) { + return; + } + floppy_off(); cpuid_init(); @@ -335,6 +383,20 @@ static void global_init(void) start_run = true; dummy_run = true; restart = false; + + init_complete = true; + + // If the bootloader placed us so that the stack area extends beyond usable RAM, + // move before the APs start; avoid a target that overlaps the running code. + uintptr_t current_start = (uintptr_t)_start; + if (!addr_range_is_usable(current_start, program_size)) { + uintptr_t target = low_load_addr; + if (target < (current_start + program_size) && current_start < (target + program_size)) { + target = high_load_addr; + } + trace(0, "relocating to %0*x before starting CPUs", 2*sizeof(uintptr_t), target); + relocate_to(target); + } } static void ap_enumerate(int my_cpu) @@ -452,6 +514,10 @@ static void test_all_windows(int my_cpu) } } if (i_am_master) { + // CPUs not taking part in this test won't re-arm their stack + // canaries, and the coming relocations will invalidate them. + stack_canary_disarm_all(); + num_active_cpus = 1; if (!dummy_run) { if (parallel_test) { diff --git a/app/x86/interrupt.c b/app/x86/interrupt.c index d367480..b80dba0 100644 --- a/app/x86/interrupt.c +++ b/app/x86/interrupt.c @@ -202,7 +202,9 @@ ISR_GP_REGS_ONLY void interrupt(struct trap_regs *trap_regs) } } - spin_lock(error_mutex); + // Don't wait for the error mutex: it may be held by a stopped CPU, or by + // this very CPU if the interrupt was taken inside the error reporting path. + spin_trylock(error_mutex); clear_message_area(); diff --git a/boot/boot.h b/boot/boot.h index 9c84804..ce1af30 100644 --- a/boot/boot.h +++ b/boot/boot.h @@ -19,11 +19,16 @@ #define MAX_APS 511 /* Maximum number of active APs */ #define BSP_STACK_SIZE 16384 /* Stack size for the BSP */ -#ifdef __loongarch_lp64 -#define AP_STACK_SIZE 2048 /* Stack size for each AP */ -#else -#define AP_STACK_SIZE 1024 /* Stack size for each AP */ -#endif + +/* + * Stack size for each AP. Each CPU's thread-local storage (the barrier + * waiting flags) occupies the top LOCALS_SIZE bytes of its own stack slot, + * directly below the bottom of the next CPU's stack: an AP overrunning its + * stack silently corrupts its neighbour's flags and freezes the whole test. + * 1024 bytes proved too small for the error reporting path; the canary in + * system/cpulocal.c turns any future overrun into a visible report. + */ +#define AP_STACK_SIZE 2048 #define STACKS_SIZE (BSP_STACK_SIZE + MAX_APS * AP_STACK_SIZE) diff --git a/boot/x86/startup32.S b/boot/x86/startup32.S index a558061..12d5e6a 100644 --- a/boot/x86/startup32.S +++ b/boot/x86/startup32.S @@ -167,13 +167,14 @@ init_idt_descr: lidt idt_descr@GOTOFF(%ebx) - # Zero the BSS (if first boot). + # Zero the BSS (if first boot). The stack area is excluded: it needs no + # init and, when loaded in low memory, may extend into the video/ROM region. cmpl $1, first_boot@GOTOFF(%ebx) jne 1f xorl %eax, %eax leal _bss@GOTOFF(%ebx), %edi - leal _end@GOTOFF(%ebx), %ecx + leal _stacks@GOTOFF(%ebx), %ecx subl %edi, %ecx 0: movl %eax, (%edi) addl $4, %edi diff --git a/boot/x86/startup64.S b/boot/x86/startup64.S index ad5ad8c..0892e01 100644 --- a/boot/x86/startup64.S +++ b/boot/x86/startup64.S @@ -253,13 +253,17 @@ flush: movw $KERNEL_DS, %ax lidt idt_descr(%rip) - # Zero the BSS (if first boot). + # Zero the BSS (if first boot). The stack area is excluded: it doesn't + # need initialising (the thread-local storage it contains is always + # written before use), and when loaded in low memory it can extend + # beyond the end of usable RAM, into the video/ROM region, where the + # ACPI and SMBIOS structures would be destroyed by the write sweep. cmpl $1, first_boot(%rip) jne 1f xorq %rax, %rax leaq _bss(%rip), %rdi - leaq _end(%rip), %rcx + leaq _stacks(%rip), %rcx subq %rdi, %rcx 0: movq %rax, (%rdi) addq $8, %rdi diff --git a/lib/spinlock.h b/lib/spinlock.h index fd80317..6717e7d 100644 --- a/lib/spinlock.h +++ b/lib/spinlock.h @@ -70,6 +70,18 @@ static inline void spin_lock(spinlock_t *lock) } } +/** + * Locks the mutex if it is not currently locked, without waiting. Returns + * true if the mutex was locked by this call, false if it was already held. + */ +static inline bool spin_trylock(spinlock_t *lock) +{ + if (lock) { + return __sync_bool_compare_and_swap(lock, false, true); + } + return true; +} + /** * Unlocks the mutex. */ diff --git a/system/cpulocal.c b/system/cpulocal.c index ceea148..b804d37 100644 --- a/system/cpulocal.c +++ b/system/cpulocal.c @@ -2,17 +2,43 @@ // Copyright (C) 2022 Martin Whitaker. #include +#include #include "boot.h" +#include "smp.h" #include "cpulocal.h" +//------------------------------------------------------------------------------ +// Constants +//------------------------------------------------------------------------------ + +// An arbitrary value that is unlikely to appear in a stack frame. +#define STACK_CANARY UINT32_C(0x446D6153) + //------------------------------------------------------------------------------ // Variables //------------------------------------------------------------------------------ int local_bytes_used = 0; +// Whether each stack canary is currently valid. The stack area is not +// preserved across program relocations, hence the arm/disarm cycle. +static bool canary_armed[MAX_CPUS]; + +//------------------------------------------------------------------------------ +// Private Functions +//------------------------------------------------------------------------------ + +static uint32_t *stack_canary_addr(int cpu_num) +{ + uintptr_t slot_bottom = (uintptr_t)_stacks; + if (cpu_num > 0) { + slot_bottom += BSP_STACK_SIZE + (uintptr_t)(cpu_num - 1) * AP_STACK_SIZE; + } + return (uint32_t *)slot_bottom; +} + //------------------------------------------------------------------------------ // Public Functions //------------------------------------------------------------------------------ @@ -24,3 +50,29 @@ int allocate_local_flag(void) } return local_bytes_used += sizeof(bool); } + +void stack_canary_arm(int cpu_num) +{ + if (cpu_num < 0 || cpu_num >= MAX_CPUS) { + return; + } + *stack_canary_addr(cpu_num) = STACK_CANARY; + canary_armed[cpu_num] = true; +} + +void stack_canary_disarm_all(void) +{ + for (int i = 0; i < MAX_CPUS; i++) { + canary_armed[i] = false; + } +} + +int stack_canary_check(void) +{ + for (int i = 0; i < num_available_cpus; i++) { + if (canary_armed[i] && *stack_canary_addr(i) != STACK_CANARY) { + return i; + } + } + return -1; +} diff --git a/system/cpulocal.h b/system/cpulocal.h index 747c69f..2d9455c 100644 --- a/system/cpulocal.h +++ b/system/cpulocal.h @@ -43,4 +43,21 @@ static inline local_flag_t *local_flags(int flag_num) return (local_flag_t *)(_stacks + BSP_STACK_SIZE - LOCALS_SIZE + flag_num); } +/** + * Writes a canary value at the bottom of the given CPU's stack slot and + * marks it as armed. Must be called again after each program relocation. + */ +void stack_canary_arm(int cpu_num); + +/** + * Marks all stack canaries as no longer valid. + */ +void stack_canary_disarm_all(void); + +/** + * Checks all armed stack canaries. Returns the number of the first CPU + * whose canary has been overwritten, or -1 if all are intact. + */ +int stack_canary_check(void); + #endif // CPULOCAL_H diff --git a/system/smp.c b/system/smp.c index 96bea14..3ab4232 100644 --- a/system/smp.c +++ b/system/smp.c @@ -1076,14 +1076,8 @@ void smp_init(bool smp_enable) smp_heap_page = heap_alloc(HEAP_TYPE_LM_1, PAGE_SIZE, PAGE_SIZE) >> PAGE_SHIFT; #if defined(__i386__) || defined(__x86_64__) - ap_startup_addr = (uintptr_t)startup; - - size_t ap_trampoline_size = ap_trampoline_end - ap_trampoline; - memcpy((uint8_t *)HEAP_BASE_ADDR, ap_trampoline, ap_trampoline_size); - - alloc_addr = HEAP_BASE_ADDR + ap_trampoline_size; + alloc_addr = HEAP_BASE_ADDR + (ap_trampoline_end - ap_trampoline); #elif defined(__loongarch_lp64) - ap_startup_addr = (uintptr_t)startup64; alloc_addr = HEAP_BASE_ADDR; #endif } @@ -1092,6 +1086,16 @@ int smp_start(cpu_state_t cpu_state[MAX_CPUS]) { int cpu_num; + // Set up the AP startup vector here rather than in smp_init(): the program + // may have been relocated in between, and the APs must enter the running copy. +#if defined(__i386__) || defined(__x86_64__) + ap_startup_addr = (uintptr_t)startup; + + memcpy((uint8_t *)HEAP_BASE_ADDR, ap_trampoline, ap_trampoline_end - ap_trampoline); +#elif defined(__loongarch_lp64) + ap_startup_addr = (uintptr_t)startup64; +#endif + cpu_state[0] = CPU_STATE_RUNNING; // we don't support disabling the boot CPU for (cpu_num = 1; cpu_num < num_available_cpus; cpu_num++) { diff --git a/tests/tests.c b/tests/tests.c index ed47de2..b62cec1 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -15,6 +15,7 @@ #include "cache.h" #include "cpuid.h" +#include "cpulocal.h" #include "memsize.h" #include "simd.h" #include "tsc.h" @@ -122,6 +123,10 @@ void test_list_init(void) int run_test(int my_cpu, int test, int stage, int iterations) { + // (Re)arm the canary guarding against this CPU overrunning its stack. + // Needed on every call: relocation invalidates the stack area. + stack_canary_arm(my_cpu); + if (my_cpu == master_cpu) { if (window_num == 0) { // First window, so we need to test all selected lower memory.