display: a 64-bit framebuffer address, no mirroring onto itself

Three fixes for real machines that broke at any resolution but 800x600:

- kmain() took the loader's framebuffer address as uint32, and
  vga_fb_init() re-published the scanout from that and the 16-bit VBE
  block. An aperture above 4 GB moved the console onto the wrong memory
  right after the vga: line. Both now use the loader's 64-bit display
  block, mapped with ioremap().
- The loader mirrored onto the console splitter's GOP, which forwards to
  the primary itself, and onto GOPs that share the primary's aperture.
  After a mode change that rewrote the screen over itself with another
  mode's stride. Handles without a device path and overlapping apertures
  are skipped, and a mode change re-matches the remaining mirrors.
- A mode whose reported size, stride or aperture does not add up after
  SetMode is refused, and the loader returns to the previous one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S1giNvrHTrPMPw7V6x1kQ4

boot/uefi/efi.h

@@ -218,6 +218,15 @@ typedef struct {
     void     *VendorTable;
 } EFI_CONFIGURATION_TABLE;
 
+// --- device path ----------------------------------------------------------
+//
+// Only ever asked whether a handle *has* one: a protocol installed by a real
+// device's driver sits on a handle with a path to that device, and a virtual
+// one -- the console splitter's GOP -- sits on a handle with none.
+
+#define EFI_DEVICE_PATH_PROTOCOL_GUID \
+    { 0x09576E91, 0x6D3F, 0x11D2, { 0x8E, 0x39, 0x00, 0xA0, 0xC9, 0x69, 0x72, 0x3B } }
+
 // --- loaded image ---------------------------------------------------------
 
 #define EFI_LOADED_IMAGE_PROTOCOL_GUID \

boot/uefi/main.c

@@ -88,6 +88,8 @@ static EFI_BOOT_SERVICES *BS;
 // ---------------------------------------------------------------------------
 
 static void mirrors_show(void);   /* see "the other displays", further down */
+static void mirrors_match(EFI_GRAPHICS_OUTPUT_PROTOCOL *primary);
+static void describe_gop(EFI_GRAPHICS_OUTPUT_PROTOCOL *g, const CHAR16 *tag);
 
 /* Once the splash is up, nothing may write to the console.
  *
@@ -793,6 +795,64 @@ res_read_number(const char *prompt)
 
 
 
+/* Set a mode, and keep it only if the firmware's own account of it adds up.
+ *
+ * A GOP can accept a mode and then describe it in a way the display does not
+ * scan out: a row length that is not the one being shown, or an aperture too
+ * small for the rows it claims. Drawn into anyway, that is the picture sheared
+ * into dotted lines, or a logo repeated across the screen, both of which a
+ * desktop with an NVIDIA card showed at sizes other than 800x600. The numbers
+ * cannot prove a mode is right, but they can prove one is wrong, and a wrong
+ * one is refused here, in the menu, where there is still a screen to say so on.
+ */
+static int
+mode_set_checked(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINT32 mode)
+{
+    UINT32 was = gop->Mode->Mode;
+    EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *want = 0;
+    UINTN sz = 0;
+
+    if (EFI_ERROR(gop->QueryMode(gop, mode, &sz, &want)) || !want)
+        return 0;
+    if (EFI_ERROR(gop->SetMode(gop, mode)))
+        return 0;
+
+    EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *got = gop->Mode->Info;
+    UINT64 need = (UINT64)got->PixelsPerScanLine * 4 * got->VerticalResolution;
+    if (got->HorizontalResolution == want->HorizontalResolution &&
+        got->VerticalResolution == want->VerticalResolution &&
+        got->PixelsPerScanLine >= got->HorizontalResolution &&
+        gop->Mode->FrameBufferBase &&
+        gop->Mode->FrameBufferSize >= need)
+        return 1;
+
+    describe_gop(gop, L"schos: mode does not add up, going back:");
+    gop->SetMode(gop, was);
+    return 0;
+}
+
+/* Said on the screen that is still working, then back to the menu. */
+static void
+res_refused(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop)
+{
+    struct ui_fb uifb;
+    EFI_INPUT_KEY key;
+    const char *msg[1] = { "The display refused that size -- keeping this one" };
+
+    uifb.base   = (ui_u8 *)(UINTN)gop->Mode->FrameBufferBase;
+    uifb.width  = gop->Mode->Info->HorizontalResolution;
+    uifb.height = gop->Mode->Info->VerticalResolution;
+    uifb.stride = gop->Mode->Info->PixelsPerScanLine * 4;
+    ui_init(&uifb);
+    ui_menu("Resolution", msg, 1, 0, "any key to go back");
+    mirrors_show();
+    for (;;) {
+        if (ST->ConIn && !EFI_ERROR(ST->ConIn->ReadKeyStroke(ST->ConIn, &key)))
+            break;
+        BS->Stall(10000);
+    }
+}
+
 static void
 res_choose(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop)
 {
@@ -850,7 +910,13 @@ res_choose(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop)
         }
         if (c == L'\r' || c == L'\n') {
             if (sel < RES_N) {
-                gop->SetMode(gop, RES_MODE[sel]);
+                if (!mode_set_checked(gop, RES_MODE[sel])) {
+                    res_refused(gop);
+                    return;
+                }
+                // The one other time the mirrors' modes are worth touching:
+                // the person just said what size the display is.
+                mirrors_match(gop);
                 return;
             }
             /* Typed. Matched against the same filtered list, so a size that
@@ -865,7 +931,11 @@ res_choose(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop)
                     continue;
                 if (mi->HorizontalResolution == w &&
                     mi->VerticalResolution == h) {
-                    gop->SetMode(gop, i);
+                    if (!mode_set_checked(gop, i)) {
+                        res_refused(gop);
+                        return;
+                    }
+                    mirrors_match(gop);
                     return;
                 }
             }
@@ -1236,6 +1306,7 @@ static EFI_GRAPHICS_OUTPUT_PROTOCOL *primary_gop;
 static UINTN mirror_n;
 
 static void mirrors_match(EFI_GRAPHICS_OUTPUT_PROTOCOL *primary);
+static void describe_gop(EFI_GRAPHICS_OUTPUT_PROTOCOL *g, const CHAR16 *tag);
 
 static void
 mirrors_find(EFI_GRAPHICS_OUTPUT_PROTOCOL *primary)
@@ -1269,6 +1340,32 @@ mirrors_find(EFI_GRAPHICS_OUTPUT_PROTOCOL *primary)
             continue;
         if (!g || g == primary || !g->Blt || !g->Mode || !g->Mode->Info)
             continue;
+
+        // Not a second display: the console splitter. Firmware installs one
+        // GOP of its own on a virtual handle and forwards every Blt to all of
+        // the real ones -- the primary included. Mirroring onto it copied the
+        // primary onto itself through the splitter's idea of the mode, which
+        // stops being the primary's the moment the resolution menu changes
+        // it: the picture landed again, offset and with the wrong row length,
+        // over the one already there. That was the doubled, sheared screen with
+        // the logo's edges repeated down it, at every size but the one the
+        // firmware booted in. A real adapter's GOP is on a handle with a path
+        // to the device; the splitter's has none.
+        void *dp = 0;
+        EFI_GUID dp_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
+        if (EFI_ERROR(BS->HandleProtocol(handles[i], &dp_guid, &dp)) || !dp)
+            continue;
+
+        // Nor is a second GOP whose aperture is the primary's: one panel
+        // reached twice, which laptops do. Blitting onto it rewrites the
+        // primary's own memory in another mode's layout.
+        UINT64 pb = primary->Mode->FrameBufferBase;
+        UINT64 pe = pb + primary->Mode->FrameBufferSize;
+        UINT64 gb = g->Mode->FrameBufferBase;
+        UINT64 ge = gb + g->Mode->FrameBufferSize;
+        if (gb && pb && gb < pe && pb < ge)
+            continue;
+
         mirror_gop[mirror_n++] = g;
     }
 
@@ -1292,8 +1389,9 @@ mirrors_find(EFI_GRAPHICS_OUTPUT_PROTOCOL *primary)
 static void
 mirrors_match(EFI_GRAPHICS_OUTPUT_PROTOCOL *primary)
 {
-    // Called exactly once, from mirrors_find(), before anything has been
+    // Called from mirrors_find(), before anything has been drawn, and again
-    // drawn. It used to be called from mirrors_show() -- which is to say on
+    // when the resolution menu sets a mode -- never per frame. It used to be
+    // called from mirrors_show() -- which is to say on
     // every menu redraw and on every segment of the kernel load -- and that is
     // not a safe thing to do to another display adapter.
     //

docs/history/kernel.md

@@ -1212,3 +1212,46 @@ 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.
+
+---
+
+## 23. Every resolution but 800x600, on two real machines
+
+Two reports in one afternoon. A desktop with an NVIDIA card booted fine at
+800x600 and at anything else showed the kernel log as dotted lines across the
+screen, and the loader's splash as four squashed copies of the logo side by
+side. A laptop stopped dead after the kernel's `vga:` line. Three separate
+defects were found under them; the third is a guard rather than a cure, and
+the desktop has not yet been retested.
+
+**The framebuffer address was cut in half.** The loader hands the kernel all
+sixty-four bits of the aperture address in RBX, and `kmain()` took it as a
+`uint32`. Below 4 GB that is harmless. Above it -- where a laptop's GPU is
+entitled to put its aperture -- the low half is a plausible address pointing
+at nothing. `kmain()` itself reads the loader's display block and puts the
+early console on the right memory, but `vga_fb_init()` then re-published the
+scanout from the truncated argument and the BIOS-era VBE block (32-bit
+address, 16-bit sizes). The second publication won, and every line after
+`vga:` went somewhere else. Both are fixed: `kmain(uint64)`, and
+`vga_fb_init()` reads the same 64-bit block `kmain()` does and maps it with
+`ioremap()` instead of `P2V()`.
+
+**The loader mirrored the screen onto itself.** It copies its picture to
+every other GOP it finds, for machines with two displays. But the firmware's
+console splitter is itself a GOP, on a virtual handle, and its `Blt` forwards
+to every real adapter -- the primary included. A laptop that reaches one panel
+through two GOPs is the same thing with a shared aperture. Once the resolution
+menu changed the primary's mode, the splitter still described the old one, so
+each mirror pass wrote the primary's own pixels back over themselves at an
+offset and with another mode's row length. Handles without a device path (the
+splitter) and GOPs whose aperture overlaps the primary's are no longer mirror
+targets, and a mode chosen in the menu re-matches the mirrors that remain.
+QEMU's OVMF has the splitter too: `schos: 2 display adapter(s)`, both at the
+same address.
+
+**A mode the firmware cannot describe is refused.** After `SetMode`, the
+loader now checks that the mode it got is the size it asked for, that the row
+is at least as long as the width, and that the aperture holds that many rows.
+If not, it goes back to the previous mode and says so in the menu. This proves
+nothing about a mode that passes, but it stops a lying one from reaching the
+kernel.

kernel/core/main.c

@@ -42,8 +42,13 @@ int boot_debug;
 // Bootstrap processor starts running C code here.
 // Allocate a real stack and switch to it, first
 // doing some setup required for memory allocator to work.
+// fb_addr is RBX from the loader, all sixty-four bits of it. It was declared
+// uint32, which cut an aperture above 4 GB down to whatever its low half
+// happened to be -- a plausible address below the hole, pointing at nothing
+// in particular -- and vga_fb_init() moved the console onto it. On a laptop
+// that is the screen stopping dead at the "vga:" line.
 int
-kmain(uint32 fb_addr)
+kmain(uint64 fb_addr)
 {
   // Carried out of the block below, for the framebuffer that could not be
   // attached there because it needs page tables that do not exist yet.

kernel/drivers/gpu/vga/vga_fb.c

@@ -69,15 +69,29 @@ vga_fb_init(uint64 phys_addr)
 {
   struct fb_info info;
 
+  // The UEFI loader's display block first, and the VBE block only without it.
+  //
+  // This read the VBE block alone, which is a BIOS structure: a 32-bit
+  // address and 16-bit sizes. kmain() reads the loader's block and puts the
+  // console on the right framebuffer; this then re-published the scanout from
+  // the other one, so the two disagreed on any machine the VBE block cannot
+  // describe, and the second one won.
+  uint64 blk = *(uint64*)P2V(SCHBOOT_FB_BASE);
+  if (blk) {
+    phys_addr = blk;
+    fb_stride = *(uint32*)P2V(SCHBOOT_FB_STRIDE);
+    fb_width  = *(uint32*)P2V(SCHBOOT_FB_WIDTH);
+    fb_height = *(uint32*)P2V(SCHBOOT_FB_HEIGHT);
+    fb_bpp    = 32;
+  } else {
     if (phys_addr == 0)
       phys_addr = *(uint32*)(P2V(SCHBOOT_VBE_LFB));
-
-  fb        = (volatile uchar*)P2V(phys_addr);
-  fb_phys   = phys_addr;
     fb_stride = *(ushort*)(P2V(SCHBOOT_VBE_STRIDE));
     fb_width  = *(ushort*)(P2V(SCHBOOT_VBE_WIDTH));
     fb_height = *(ushort*)(P2V(SCHBOOT_VBE_HEIGHT));
     fb_bpp    = *(uchar*)(P2V(SCHBOOT_VBE_BPP));
+  }
+  fb_phys = phys_addr;
 
   if (fb_width  == 0) fb_width  = 800;
   if (fb_height == 0) fb_height = 600;
@@ -100,8 +114,16 @@ vga_fb_init(uint64 phys_addr)
     return;
   }
 
-  kinfo("vga: %dx%d, %d bpp, stride %d, LFB: 0x%x\n",
+  // ioremap() rather than P2V(): the same address below 4 GB, and the window
-          fb_width, fb_height, fb_bpp, fb_stride, (uint32)phys_addr);
+  // kmain() already opened for an aperture above it.
+  fb = (volatile uchar*)ioremap(phys_addr, (uint64)fb_stride * fb_height);
+  if (fb == 0) {
+    cprintf("vga: cannot map the framebuffer at %p\n", (void *)phys_addr);
+    return;
+  }
+
+  kinfo("vga: %dx%d, %d bpp, stride %d, LFB: %p\n",
+          fb_width, fb_height, fb_bpp, fb_stride, (void *)phys_addr);
 
   info.ptr    = fb;
   info.phys   = fb_phys;