diff --git a/docs/history/kernel.md b/docs/history/kernel.md index f1c8967..6b52cdb 100644 --- a/docs/history/kernel.md +++ b/docs/history/kernel.md @@ -1125,3 +1125,73 @@ and startup is 0.14 s. mksh keeps `/bin/sh`, so schinit's no-jobs fallback is a different program from the one that might have failed. Completion is the piece that waits on the filesystem. + +## 22. Every stack on every application processor was misaligned by eight + +`jmp mpenter`. + +x86-64 requires `rsp` to be 16-byte aligned immediately before a `call`, so a +function begins with `rsp` ending in 8 — the return address — and its +`push %rbp; mov %rsp,%rbp` prologue leaves `%rbp` 16-byte aligned. Compilers +build on that: an aligned vector store to a frame slot is `movaps`, and +`movaps` to a misaligned address is `#GP`. + +`entry64mp` loaded the AP's stack pointer, which is page-aligned, and *jumped*. +No return address was pushed, so `%rbp` came out eight bytes off — on every +frame, on every application processor, for the life of the machine. + +It cost nothing for as long as no function an AP ran needed an aligned vector +store. Then the console's glyph path started returning a small struct by value, +clang spilled it with `movaps`, and CPU 1 took a `#GP` the instant it announced +itself: + +``` +unexpected trap 13 from cpu 1 rip ffff800000120a3b +``` + +``` +ffff800000120a37: 0f 10 45 90 movups -0x70(%rbp), %xmm0 +ffff800000120a3b: 0f 29 45 b0 movaps %xmm0, -0x50(%rbp) <- #GP +``` + +`movups` — *unaligned* — loads fine on the line above; `movaps` stores and +faults. The displacement is a multiple of 16, so the misalignment is `%rbp` +itself, which is a fact about how the function was entered rather than +anything it did. + +`call mpenter` instead of `jmp`. Eight bytes for an ABI the whole toolchain +assumes. + +### It hid behind a second bug + +The first report of this was not a `#GP`, it was: + +``` +Stop code ACQUIRE + #2 cprintf+0xae + #3 trap+0x2c4 +``` + +`trap()` reported the fault with `cprintf()`, `cprintf()` takes the console +lock, and the faulting code was already holding it — every character drawn on +a verbose boot is drawn under that lock. So `acquire()` panicked about a lock +its own call chain owned, and the trap number, the faulting address and the +register state were all lost. + +A kernel-mode trap ends in `panic()` a dozen lines later regardless, so the +console is put into panic mode *before* the report rather than during it. The +same fault then prints what it actually was, which is the entire purpose of +the report. + +### And a third, in the same boot + +The console reported `JetBrains Mono, 8x16 cells` — a name from one font and +metrics from another. The built-in bitmap had become a provider like any other, +so `draw_glyph_px()` installed it lazily on the first glyph; on a verbose boot +that happens before `gfx_font_init()` runs, and `gfx_font_init()` returned early +because a font was already installed. The console spent the whole boot in the +fallback while claiming not to be. + +"A font is installed" and "a typeface was chosen" had been the same test +(`cfont == 0`) and quietly stopped being. They are two flags now, and +`gfx_font_ready()` answers the second one. diff --git a/kernel/arch/x86_64/entry.S b/kernel/arch/x86_64/entry.S index 64eabc3..c1e564f 100644 --- a/kernel/arch/x86_64/entry.S +++ b/kernel/arch/x86_64/entry.S @@ -233,7 +233,26 @@ __deadloop: entry64mp: mov $0x7000, %rax mov -16(%rax), %rsp - jmp mpenter + + # `call`, not `jmp`, and the difference is an ABI requirement rather than a + # style preference. + # + # x86-64 says rsp is 16-byte aligned immediately before a `call`, so a + # function starts with rsp ending in 8 -- the return address the call just + # pushed -- and its `push %rbp; mov %rsp,%rbp` prologue therefore leaves + # %rbp 16-byte aligned. Compilers rely on that: an aligned SSE store to a + # frame slot is `movaps`, and `movaps` to a misaligned address is #GP. + # + # This was a `jmp` into a 16-byte-aligned stack, so every frame on every + # application processor was off by eight, for the life of the machine. It + # cost nothing for as long as no function the APs ran happened to need an + # aligned vector store -- and then one did, deep in the console, and CPU 1 + # took a #GP the moment it announced itself on a verbose boot. + # + # mpenter never returns, so the pushed address is only ever a backtrace + # entry. Eight bytes for an ABI the whole toolchain assumes is a bargain. + call mpenter + jmp __deadloop .global wrmsr wrmsr: diff --git a/kernel/arch/x86_64/trap.c b/kernel/arch/x86_64/trap.c index 6622b49..1ade375 100644 --- a/kernel/arch/x86_64/trap.c +++ b/kernel/arch/x86_64/trap.c @@ -148,6 +148,18 @@ trap(struct trapframe *tf) unexpected: if(myproc() == 0 || (tf->cs&3) == 0){ + // A kernel-mode trap ends in panic() a dozen lines down, so the console + // can be put into its panic mode *now* -- and it has to be, because the + // fault may have happened inside the console itself. + // + // cprintf() takes cons.lock. If the faulting code was already holding + // it -- anything under console output, which on a verbose boot is most + // of the kernel -- then this report deadlocks on a lock its own call + // chain owns, acquire() panics with "acquire", and the trap that + // actually happened is never printed. That is a bad trade at the best + // of times and a worse one here: the whole point of the report is to + // name the fault. + cons_panic_mode(); // The rip is named where it can be. This is a kernel address, so // ksym_lookup() has something to say about it -- and the whole reason // docs/architecture/debugging.md puts D0 first is that "rip ffff8000001294c2" diff --git a/kernel/dev/gfx.c b/kernel/dev/gfx.c index 53cefa4..d9bc3a8 100644 --- a/kernel/dev/gfx.c +++ b/kernel/dev/gfx.c @@ -61,6 +61,17 @@ static int cell_h = BITMAP_H; // The typeface. Null only until the first glyph is drawn, which fills it in // with the built-in bitmap; gfx_font_init() replaces that with a real face // when there is one. +// Whether the font in `cfont` is the built-in bitmap standing in, or a real +// typeface that was chosen. +// +// The distinction was implicit in `cfont == 0` until the built-in became a +// provider like any other, and losing it cost a boot: draw_glyph_px() installs +// the fallback lazily, a verbose boot draws before gfx_font_init() runs, and +// gfx_font_init() then saw a font already installed and returned without +// loading anything. The console stayed 8x16 for the whole boot while reporting +// itself as JetBrains Mono. +static int cfont_is_builtin; + static struct ttf consttf; static struct font *cfont; static int cfont_baseline; @@ -318,6 +329,7 @@ draw_glyph_px(int x, int y, uint32 cp, uint32 fg, uint32 bg) if (!cfont) { cfont = font_builtin(); cfont_baseline = cfont->ops->baseline(cfont); + cfont_is_builtin = 1; } // Background first. A terminal cell owns its rectangle, and leaving the old @@ -366,7 +378,10 @@ gfx_font_init(void) uint32 len = (uint32)(_binary_consfont_end - _binary_consfont_start); struct glyph gl; - if (cfont) + // A built-in font standing in is not a font having been chosen. Returning + // here on that basis is what kept the bitmap on screen for an entire + // verbose boot. + if (cfont && !cfont_is_builtin) return; // A typeface off the boot volume, if the loader found one. @@ -430,6 +445,7 @@ gfx_font_init(void) cell_h = h; cfont_baseline = f->ops->baseline(f); cfont = f; + cfont_is_builtin = 0; } // **Nothing here prints.** gfx_font_init() runs before vt_init(), because @@ -443,7 +459,7 @@ gfx_font_init(void) int gfx_font_ready(void) { - return cfont != 0; + return cfont != 0 && !cfont_is_builtin; } void