diff --git a/boot/uefi/efi.h b/boot/uefi/efi.h index 7cad782..f1df84d 100644 --- a/boot/uefi/efi.h +++ b/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 \ diff --git a/boot/uefi/main.c b/boot/uefi/main.c index 31c0a28..280f966 100644 --- a/boot/uefi/main.c +++ b/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 - // drawn. It used to be called from mirrors_show() -- which is to say on + // Called from mirrors_find(), before anything has been drawn, and again + // 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. // diff --git a/docs/history/kernel.md b/docs/history/kernel.md index 1598f51..ff72a21 100644 --- a/docs/history/kernel.md +++ b/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. diff --git a/kernel/core/main.c b/kernel/core/main.c index 6c74c34..475e171 100644 --- a/kernel/core/main.c +++ b/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. diff --git a/kernel/drivers/gpu/vga/vga_fb.c b/kernel/drivers/gpu/vga/vga_fb.c index 0f9aab4..40017bb 100644 --- a/kernel/drivers/gpu/vga/vga_fb.c +++ b/kernel/drivers/gpu/vga/vga_fb.c @@ -69,15 +69,29 @@ vga_fb_init(uint64 phys_addr) { struct fb_info info; - 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)); + // 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_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", - fb_width, fb_height, fb_bpp, fb_stride, (uint32)phys_addr); + // ioremap() rather than P2V(): the same address below 4 GB, and the window + // 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;