On Thu, May 28, 2020 at 02:06:28PM +0200, Mark Kettenis wrote:
> > 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@
ok jsg@ on this one as well
>
> > 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
> > }
> >
>
>
