On Sun, 23 Feb 2020 21:23:41 +1100
Jonathan Gray <j...@jsg.id.au> wrote:
> 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?
Because I thought moving uvm_setpagesize helps a panic() in the
function. But it doesn't help so much since there is still many other
panic()s. The updated diff doesn't move the function.
>> -
>> - /*
>> * 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.
Yes, that's right.
Updated 3rd diff is attached. I think this is the best way for this
moment. Since it's simple and it seems that we don't have any other
way to skip VGA than the way that skips VGA if the machine has efifb.
Index: sys/arch/amd64/amd64/machdep.c
===================================================================
RCS file: /var/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 27 Feb 2020 12:16:32 -0000
@@ -1394,11 +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();
@@ -1420,6 +1415,8 @@ init_x86_64(paddr_t first_avail)
} else
panic("invalid /boot");
+ cninit();
+
/*
* Memory on the AMD64 port is described by three different things.
*
@@ -1926,12 +1923,6 @@ getbootinfo(char *bootinfo, int bootinfo
bios_ddb_t *bios_ddb;
bios_bootduid_t *bios_bootduid;
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) &&
@@ -1941,24 +1932,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 */
@@ -1982,15 +1964,8 @@ getbootinfo(char *bootinfo, int bootinfo
comconsaddr = consaddr;
comconsrate = cdp->conspeed;
comconsiot = X86_BUS_SPACE_IO;
-
- /* Probe the serial port this time. */
- docninit++;
}
#endif
-#ifdef BOOTINFO_DEBUG
- printf(" console 0x%x:%d",
- cdp->consdev, cdp->conspeed);
-#endif
}
break;
case BOOTARG_BOOTMAC:
@@ -2022,8 +1997,6 @@ getbootinfo(char *bootinfo, int bootinfo
case BOOTARG_EFIINFO:
bios_efiinfo = (bios_efiinfo_t *)q->ba_arg;
- if (bios_efiinfo->fb_addr != 0)
- docninit++;
break;
case BOOTARG_UCODE:
@@ -2031,18 +2004,9 @@ 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
