On Sun, Feb 23, 2020 at 07:06:50PM +0900, YASUOKA Masahiko wrote:
> On Sun, 23 Feb 2020 18:50:54 +0900 (JST)
> YASUOKA Masahiko <yasu...@yasuoka.net> wrote:
> > On Sat, 22 Feb 2020 13:02:48 +1100
> > Jonathan Gray <j...@jsg.id.au> wrote:
> >> On Fri, Feb 21, 2020 at 02:09:07PM +0900, YASUOKA Masahiko wrote:
> >>> When efiboot starts the kernel, the video display becomes distorted
> >>> and never recovered until CPU reset.
> >>>
> >>> Our kernel tries to initialized console twice, first trial is done
> >>> before getting boot info and second trial is done after getting boot
> >>> info. Since EFI framebuffer needs "boot info", it is initialized on
> >>> second trial.
> >>>
> >>> On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
> >>> selects vga for the console, but actually it is broken. On usual
> >>> machines which boot with EFI, the problem doesn't happen since they
> >>> have no vga.
> >>>
> >>> The diff following fixes the problem by initializing efifb console
> >>> even if the VGA is probed.
> >>>
> >>> # Also, HP DL20 Gen10 has "UEFI optimized boot" setting on BIOS and
> >>> # disabling the setting avoids the problem happening. But since the
> >>> # setting seems to be for old Windows, I think we should fix our
> >>> # kernel.
> >>>
> >>> comment? ok?
> >>
> >> Is there a way to detect efi or bios before boot info is set?
> >> Ideally vga_cnattach() would never be called when booting via efi.
> >
> > Yes. I've tried to find such the way, I found 2 ways.
> >
> > 1) ACPI has FADT_NO_VGA flag which indicate the system has VGA, but
> > reading ACPI table at early of kernel boot is not good and difficult
> >
> > 2) Pass a flag from efiboot. A diff for this is attached.
> >
> >> Should the cninit() before the boot args are parsed be removed and just
> >> have cninit() unconditionally after? This would make the debug printfs
> >> in boot arg passing useless, but they already wouldn't work when booting
> >> via efi.
> >
> > I think this is a straight way and no downside for efi. For a system
> > booting via BIOS, there is a downside that panic or debug string isn't
> > shown at very early part of kernel boot.
>
> A diff for this is attached.
>
> 1st diff
> - initialize efifb even if vga is probed
>
> 2nd diff
> - pass a flag from efiboot, then initialize vga/efifb properly with it
>
> 3rd diff
> - parse bootarg first, then initialize vga/efifb properly
>
>
> I think 3rd diff is the best. Because it makes the code simple and
> the downside doesn't seem so serious.
>
>
> Index: sys/arch/amd64/amd64/machdep.c
> ===================================================================
> RCS file: /disk/cvs/openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.261
> diff -u -p -r1.261 machdep.c
> --- sys/arch/amd64/amd64/machdep.c 24 Jan 2020 05:27:31 -0000 1.261
> +++ sys/arch/amd64/amd64/machdep.c 23 Feb 2020 09:46:54 -0000
> @@ -1394,16 +1394,6 @@ init_x86_64(paddr_t first_avail)
> i8254_startclock();
>
> /*
> - * Attach the glass console early in case we need to display a panic.
> - */
> - cninit();
> -
> - /*
> - * Initialize PAGE_SIZE-dependent variables.
> - */
> - uvm_setpagesize();
why move uvm_setpagesize?
> -
> - /*
> * Boot arguments are in a single page specified by /boot.
> *
> * We require the "new" vector form, as well as memory ranges
> @@ -1420,6 +1410,16 @@ init_x86_64(paddr_t first_avail)
> } else
> panic("invalid /boot");
>
> + /*
> + * Attach the glass console early in case we need to display a panic.
> + */
> + cninit();
as cninit() is done here docninit and the cninit() call in getbootinfo()
could be removed.
> +
> + /*
> + * Initialize PAGE_SIZE-dependent variables.
> + */
> + uvm_setpagesize();
> +
> /*
> * Memory on the AMD64 port is described by three different things.
> *
> @@ -1928,11 +1928,6 @@ getbootinfo(char *bootinfo, int bootinfo
> bios_bootsr_t *bios_bootsr;
> int docninit = 0;
>
> -#undef BOOTINFO_DEBUG
> -#ifdef BOOTINFO_DEBUG
> - printf("bootargv:");
> -#endif
> -
> for (q = (bootarg32_t *)bootinfo;
> (q->ba_type != BOOTARG_END) &&
> ((((char *)q) - bootinfo) < bootinfo_size);
> @@ -1941,24 +1936,15 @@ getbootinfo(char *bootinfo, int bootinfo
> switch (q->ba_type) {
> case BOOTARG_MEMMAP:
> bios_memmap = (bios_memmap_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" memmap %p", bios_memmap);
> -#endif
> break;
> case BOOTARG_DISKINFO:
> bios_diskinfo = (bios_diskinfo_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" diskinfo %p", bios_diskinfo);
> -#endif
> break;
> case BOOTARG_APMINFO:
> /* generated by i386 boot loader */
> break;
> case BOOTARG_CKSUMLEN:
> bios_cksumlen = *(u_int32_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" cksumlen %d", bios_cksumlen);
> -#endif
> break;
> case BOOTARG_PCIINFO:
> /* generated by i386 boot loader */
> @@ -1987,10 +1973,6 @@ getbootinfo(char *bootinfo, int bootinfo
> docninit++;
> }
> #endif
> -#ifdef BOOTINFO_DEBUG
> - printf(" console 0x%x:%d",
> - cdp->consdev, cdp->conspeed);
> -#endif
> }
> break;
> case BOOTARG_BOOTMAC:
> @@ -2031,18 +2013,11 @@ getbootinfo(char *bootinfo, int bootinfo
> break;
>
> default:
> -#ifdef BOOTINFO_DEBUG
> - printf(" unsupported arg (%d) %p", q->ba_type,
> - q->ba_arg);
> -#endif
> break;
> }
> }
> if (docninit > 0)
> cninit();
> -#ifdef BOOTINFO_DEBUG
> - printf("\n");
> -#endif
> }
>
> int
>
>
