Date: Thu, 30 Mar 2023 18:45:19 -0500 From: Kyle Evans <kevans@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Gleb Smirnoff <glebius@freebsd.org>, Wei Hu <whu@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, imp@freebsd.org, gallatin@freebsd.org, kevans@freebsd.org Subject: Re: git: 927358dd98cb - main - amd64 loader: Use efiserialio for Hyper-V booted systems Message-ID: <CACNAnaH9g_WiNQziUoXMF-QUU=g2mpDR==70QDUBiCUs6WtMYw@mail.gmail.com> In-Reply-To: <CANCZdfpbWSbhJUDU_VUXzkqWE9Xc=MrE%2BgmEOqg%2ByOhG%2BTS6xw@mail.gmail.com> References: <202303180720.32I7KXOc030612@gitrepo.freebsd.org> <ZCYb4V4Mg8y%2BU7tu@FreeBSD.org> <CANCZdfpbWSbhJUDU_VUXzkqWE9Xc=MrE%2BgmEOqg%2ByOhG%2BTS6xw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Mar 30, 2023 at 6:40=E2=80=AFPM Warner Losh <imp@bsdimp.com> wrote: > > Let's back it out. I didn't get a chance to review it and the duplicated = name strikes me as massively unwise. > Of course, the efi serial driver never should have had the name comconsol= e in the first place, imho. It was OK > on arm where we couldn't conflict. > > So let's back it out and talk about how we should do this, including the = need to possibly just rename the efi > version of the console driver to something else. IMHO, it never should ha= ve been comconsole in the first > place because it's configured differently than comconsole.... > Sure, no objection. > Warner > > On Thu, Mar 30, 2023 at 5:31=E2=80=AFPM Gleb Smirnoff <glebius@freebsd.or= g> wrote: >> >> Wei, Kyle, >> >> this commit hangs loader on real hardware, at least on some >> of it. The loader prints list of consoles and hangs hard: >> >> [Thu Mar 30 20:46:12 2023]^M|^HLoading /boot/loader.conf^M >> ^M/^Hconsole vidconsole is invalid!^M >> ^MAvailable consoles:^M >> ^M efi^M >> ^M comconsole^M >> ^M comconsole^M >> ^M nullconsole^M >> ^M spinconsole^M >> >> Machine is unrecoverable unless you got alternate boot media >> and access to BMC console. >> >> First, please DO NOT MFC this as scheduled. Second, let's try >> to fix it or back it out if we hear from any other CURRENT >> user. >> >> Our configuration isn't special. This is what we got in >> loader.conf, related to consoles: >> >> console=3D"comconsole vidconsole efi" >> comconsole_speed=3D115200 >> comconsole_port=3D0x3e8 >> console=3D"efi" >> hw.uart.console=3D"io:1016,br:115200" >> >> On Sat, Mar 18, 2023 at 07:20:33AM +0000, Wei Hu wrote: >> W> The branch main has been updated by whu: >> W> >> W> URL: https://cgit.FreeBSD.org/src/commit/?id=3D927358dd98cb902160093e= 0dc0bac002d6b43858 >> W> >> W> commit 927358dd98cb902160093e0dc0bac002d6b43858 >> W> Author: Wei Hu <whu@FreeBSD.org> >> W> AuthorDate: 2023-03-14 15:13:46 +0000 >> W> Commit: Wei Hu <whu@FreeBSD.org> >> W> CommitDate: 2023-03-18 07:07:35 +0000 >> W> >> W> amd64 loader: Use efiserialio for Hyper-V booted systems >> W> >> W> UEFI provides ConIn/ConOut handles for consoles that it supports, >> W> which include the text-video and serial ports. When the serial po= rt >> W> is available, use the UEFI driver instead of direct io-port acces= ses >> W> to avoid conflicts between the firmware and direct hardware acces= s, as >> W> happens on Hyper-V (Azure) setups. >> W> >> W> This change enables efiserialio to be built for efi-amd64 and has >> W> higher order priority vs comconsole, and only uses efiserialio >> W> if the hypervisor is Hyper-V. When efiserialio successfully >> W> probes, it will set efi_comconsole_avail=3Dtrue which will preven= t >> W> comconsole from probing in this setup. >> W> >> W> Tested on Hyper-V, ESXi and Azure VMs. >> W> >> W> PR: 264267 >> W> Reviewed by: kevans, whu >> W> Tested by: whu >> W> Obtained from: Rubicon Communications, LLC (Netgate) >> W> MFC after: 2 weeks >> W> Sponsored by: Rubicon Communications, LLC (Netgate) >> W> --- >> W> stand/efi/loader/arch/amd64/Makefile.inc | 1 + >> W> stand/efi/loader/bootinfo.c | 11 ++++++-- >> W> stand/efi/loader/conf.c | 6 +++++ >> W> stand/efi/loader/efiserialio.c | 43 ++++++++++++++++++++++= ++++++---- >> W> stand/i386/libi386/comconsole.c | 14 +++++++++++ >> W> 5 files changed, 68 insertions(+), 7 deletions(-) >> W> >> W> diff --git a/stand/efi/loader/arch/amd64/Makefile.inc b/stand/efi/loa= der/arch/amd64/Makefile.inc >> W> index 0d9e2648cb59..bd89044bd6c7 100644 >> W> --- a/stand/efi/loader/arch/amd64/Makefile.inc >> W> +++ b/stand/efi/loader/arch/amd64/Makefile.inc >> W> @@ -5,6 +5,7 @@ SRCS+=3D amd64_tramp.S \ >> W> elf64_freebsd.c \ >> W> trap.c \ >> W> multiboot2.c \ >> W> + efiserialio.c \ >> W> exc.S >> W> >> W> .PATH: ${BOOTSRC}/i386/libi386 >> W> diff --git a/stand/efi/loader/bootinfo.c b/stand/efi/loader/bootinfo.= c >> W> index 939f2cf4c3fe..d79f59343af1 100644 >> W> --- a/stand/efi/loader/bootinfo.c >> W> +++ b/stand/efi/loader/bootinfo.c >> W> @@ -119,10 +119,17 @@ bi_getboothowto(char *kargs) >> W> if (tmp !=3D NULL) >> W> speed =3D strtol(tmp, NULL, 0); >> W> tmp =3D getenv("efi_com_port"); >> W> - if (tmp =3D=3D NULL) >> W> - tmp =3D getenv("comconsole_port"); >> W> if (tmp !=3D NULL) >> W> port =3D strtol(tmp, NULL, 0); >> W> + if (port <=3D 0) { >> W> + tmp =3D getenv("comconsole_port"); >> W> + if (tmp !=3D NULL) >> W> + port =3D strtol(tmp, NULL, 0); >> W> + else { >> W> + if (port =3D=3D 0) >> W> + port =3D 0x3f8; >> W> + } >> W> + } >> W> if (speed !=3D -1 && port !=3D -1) { >> W> snprintf(buf, sizeof(buf), "io:%d,br:%d"= , port, >> W> speed); >> W> diff --git a/stand/efi/loader/conf.c b/stand/efi/loader/conf.c >> W> index 863c9188c72c..051e1a3381d1 100644 >> W> --- a/stand/efi/loader/conf.c >> W> +++ b/stand/efi/loader/conf.c >> W> @@ -81,6 +81,9 @@ struct netif_driver *netif_drivers[] =3D { >> W> >> W> extern struct console efi_console; >> W> extern struct console comconsole; >> W> +#if defined(__amd64__) >> W> +extern struct console eficomconsole; >> W> +#endif >> W> #if defined(__amd64__) || defined(__i386__) >> W> extern struct console nullconsole; >> W> extern struct console spinconsole; >> W> @@ -88,6 +91,9 @@ extern struct console spinconsole; >> W> >> W> struct console *consoles[] =3D { >> W> &efi_console, >> W> +#if defined(__amd64__) >> W> + &eficomconsole, >> W> +#endif >> W> &comconsole, >> W> #if defined(__amd64__) || defined(__i386__) >> W> &nullconsole, >> W> diff --git a/stand/efi/loader/efiserialio.c b/stand/efi/loader/efiser= ialio.c >> W> index 375e679d2590..5fbc700f6ac2 100644 >> W> --- a/stand/efi/loader/efiserialio.c >> W> +++ b/stand/efi/loader/efiserialio.c >> W> @@ -69,6 +69,11 @@ static int comc_speed_set(struct env_var *,= int, const void *); >> W> >> W> static struct serial *comc_port; >> W> extern struct console efi_console; >> W> +bool efi_comconsole_avail =3D false; >> W> + >> W> +#if defined(__amd64__) >> W> +#define comconsole eficomconsole >> W> +#endif >> W> >> W> struct console comconsole =3D { >> W> .c_name =3D "comconsole", >> W> @@ -254,11 +259,22 @@ comc_probe(struct console *sc) >> W> char *env, *buf, *ep; >> W> size_t sz; >> W> >> W> +#if defined(__amd64__) >> W> + /* >> W> + * For x86-64, don't use this driver if not running in Hyper-V. >> W> + */ >> W> + env =3D getenv("smbios.bios.version"); >> W> + if (env =3D=3D NULL || strncmp(env, "Hyper-V", 7) !=3D 0) { >> W> + return; >> W> + } >> W> +#endif >> W> + >> W> if (comc_port =3D=3D NULL) { >> W> comc_port =3D calloc(1, sizeof (struct serial)); >> W> if (comc_port =3D=3D NULL) >> W> return; >> W> } >> W> + >> W> /* Use defaults from firmware */ >> W> comc_port->databits =3D 8; >> W> comc_port->parity =3D DefaultParity; >> W> @@ -308,6 +324,10 @@ comc_probe(struct console *sc) >> W> comc_port_set, env_nounset); >> W> >> W> env =3D getenv("efi_com_speed"); >> W> + if (env =3D=3D NULL) >> W> + /* fallback to comconsole setting */ >> W> + env =3D getenv("comconsole_speed"); >> W> + >> W> if (comc_parse_intval(env, &val) =3D=3D CMD_OK) >> W> comc_port->baudrate =3D val; >> W> >> W> @@ -318,8 +338,13 @@ comc_probe(struct console *sc) >> W> comc_speed_set, env_nounset); >> W> >> W> comconsole.c_flags =3D 0; >> W> - if (comc_setup()) >> W> + if (comc_setup()) { >> W> sc->c_flags =3D C_PRESENTIN | C_PRESENTOUT; >> W> + efi_comconsole_avail =3D true; >> W> + } else { >> W> + /* disable being seen as "comconsole" */ >> W> + comconsole.c_name =3D "efiserialio"; >> W> + } >> W> } >> W> >> W> static int >> W> @@ -489,6 +514,7 @@ comc_setup(void) >> W> { >> W> EFI_STATUS status; >> W> UINT32 control; >> W> + char *ev; >> W> >> W> /* port is not usable */ >> W> if (comc_port->sio =3D=3D NULL) >> W> @@ -498,10 +524,17 @@ comc_setup(void) >> W> if (EFI_ERROR(status)) >> W> return (false); >> W> >> W> - status =3D comc_port->sio->SetAttributes(comc_port->sio, >> W> - comc_port->baudrate, comc_port->receivefifodepth, >> W> - comc_port->timeout, comc_port->parity, >> W> - comc_port->databits, comc_port->stopbits); >> W> + ev =3D getenv("smbios.bios.version"); >> W> + if (ev !=3D NULL && strncmp(ev, "Hyper-V", 7) =3D=3D 0) { >> W> + status =3D comc_port->sio->SetAttributes(comc_port->sio, >> W> + 0, 0, 0, DefaultParity, 0, DefaultStopBits); >> W> + } else { >> W> + status =3D comc_port->sio->SetAttributes(comc_port->sio, >> W> + comc_port->baudrate, comc_port->receivefifodepth, >> W> + comc_port->timeout, comc_port->parity, >> W> + comc_port->databits, comc_port->stopbits); >> W> + } >> W> + >> W> if (EFI_ERROR(status)) >> W> return (false); >> W> >> W> diff --git a/stand/i386/libi386/comconsole.c b/stand/i386/libi386/com= console.c >> W> index ed1f1aa08ed7..3fbb6a292c19 100644 >> W> --- a/stand/i386/libi386/comconsole.c >> W> +++ b/stand/i386/libi386/comconsole.c >> W> @@ -85,6 +85,20 @@ comc_probe(struct console *cp) >> W> int speed, port; >> W> uint32_t locator; >> W> >> W> +#if defined(__amd64__) >> W> + extern bool efi_comconsole_avail; >> W> + >> W> + if (efi_comconsole_avail) { >> W> + /* >> W> + * If EFI provides serial I/O, then don't use this legac= y >> W> + * com driver to avoid conflicts with the firmware's dri= ver. >> W> + * Change c_name so that it cannot be found in the looku= p. >> W> + */ >> W> + comconsole.c_name =3D "xcomconsole"; >> W> + return; >> W> + } >> W> +#endif >> W> + >> W> if (comc_curspeed =3D=3D 0) { >> W> comc_curspeed =3D COMSPEED; >> W> /* >> >> -- >> Gleb Smirnoff
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaH9g_WiNQziUoXMF-QUU=g2mpDR==70QDUBiCUs6WtMYw>