> Date: Thu, 28 May 2020 20:49:18 +0900 (JST)
> From: YASUOKA Masahiko <yasu...@openbsd.org>
>
> On Thu, 28 May 2020 12:31:31 +0200 (CEST)
> Mark Kettenis <mark.kette...@xs4all.nl> wrote:
> >> Date: Thu, 28 May 2020 17:01:48 +0900 (JST)
> >> From: YASUOKA Masahiko <yasu...@openbsd.org>
> >>
> >> Hi,
> >>
> >> I'd like to conclude this issue.
> >>
> >> On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
> >> YASUOKA Masahiko <yasu...@openbsd.org> wrote:
> >> > I am testing a new hardware, HPE DL20 Gen10.
> >> >
> >> > 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.
> >>
> >> If we have a way to detect whether the machine has VGA. ACPI
> >> FADT_NO_VGA was a candidate. But that bit is cleard both on my "HPE
> >> DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230. Also the
> >> problem newly posted at misc@ (*) might be the same problem.
> >>
> >> (*) https://marc.info/?l=openbsd-misc&m=159064773219779&w=2
> >>
> >> I think having the diff folowing is the best for this momemnt.
> >> The diff does:
> >>
> >> - move cninit() after parsing bootinfo
> >> - give up the debug message which can be enabled if BOOTINFO_DEBUG is
> >> defined
> >>
> >> ok?
> >
> > I suspect we have to accept that there is too much broken hardware out
> > there.
>
> Finally we might have no way other than having a configuration in
> boot.conf...
>
> > There is no real reason to drop the debug messages.
>
> OK, the debug messages are reverted.
>
> > I'd prefer to call cninit() directly from init_x86_64, so I'd just
> > move the call immediately after the block that calls getbootinfo().
> > And emove the call from getbootinfo() of course.
>
> I think the last diff already satisfied these things.
>
> >> @@ -1395,11 +1395,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();
> >> @@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
> >> } else
> >> panic("invalid /boot");
> >>
> >> + cninit();
> >> +
> >> /*
> >> * Memory on the AMD64 port is described by three different things.
> >> *
>
> A hidden line which calls getbootinfo() is at just before second
> chunk. The updated diff was created with "-U 4" to clarify this.
>
> Alternatively, are you suggesting
>
> getbootinfo(bootinfo, bootinfo_size);
> + cninit();
> } else
> panic("invalid /boot");
>
> ?
>
> Both is OK for me.
>
> How about this?
The attached diff looks good to me.
ok kettenis@
> Index: sys/arch/amd64/amd64/machdep.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.264
> diff -u -p -U4 -r1.264 machdep.c
> --- sys/arch/amd64/amd64/machdep.c 16 May 2020 14:44:44 -0000 1.264
> +++ sys/arch/amd64/amd64/machdep.c 28 May 2020 11:34:39 -0000
> @@ -1394,13 +1394,8 @@ 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,8 +1415,10 @@ init_x86_64(paddr_t first_avail)
> getbootinfo(bootinfo, bootinfo_size);
> } else
> panic("invalid /boot");
>
> + cninit();
> +
> /*
> * Memory on the AMD64 port is described by three different things.
> *
> * 1. biosbasemem - This is outdated, and should really only be used to
> @@ -1926,10 +1923,8 @@ getbootinfo(char *bootinfo, int bootinfo
> bootarg32_t *q;
> 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
> @@ -1982,11 +1977,8 @@ getbootinfo(char *bootinfo, int bootinfo
> comconsunit = unit;
> 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",
> @@ -2022,10 +2014,8 @@ getbootinfo(char *bootinfo, int bootinfo
> break;
>
> case BOOTARG_EFIINFO:
> bios_efiinfo = (bios_efiinfo_t *)q->ba_arg;
> - if (bios_efiinfo->fb_addr != 0)
> - docninit++;
> break;
>
> case BOOTARG_UCODE:
> bios_ucode = (bios_ucode_t *)q->ba_arg;
> @@ -2038,10 +2028,8 @@ getbootinfo(char *bootinfo, int bootinfo
> #endif
> break;
> }
> }
> - if (docninit > 0)
> - cninit();
> #ifdef BOOTINFO_DEBUG
> printf("\n");
> #endif
> }
>
