Re: git: 927358dd98cb - main - amd64 loader: Use efiserialio for Hyper-V booted systems
Date: Thu, 30 Mar 2023 23:45:19 UTC
On Thu, Mar 30, 2023 at 6:40 PM 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 comconsole 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 have 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 PM Gleb Smirnoff <glebius@freebsd.org> 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="comconsole vidconsole efi"
>> comconsole_speed=115200
>> comconsole_port=0x3e8
>> console="efi"
>> hw.uart.console="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=927358dd98cb902160093e0dc0bac002d6b43858
>> 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 port
>> W> is available, use the UEFI driver instead of direct io-port accesses
>> W> to avoid conflicts between the firmware and direct hardware access, 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=true which will prevent
>> 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/loader/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+= 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 != NULL)
>> W> speed = strtol(tmp, NULL, 0);
>> W> tmp = getenv("efi_com_port");
>> W> - if (tmp == NULL)
>> W> - tmp = getenv("comconsole_port");
>> W> if (tmp != NULL)
>> W> port = strtol(tmp, NULL, 0);
>> W> + if (port <= 0) {
>> W> + tmp = getenv("comconsole_port");
>> W> + if (tmp != NULL)
>> W> + port = strtol(tmp, NULL, 0);
>> W> + else {
>> W> + if (port == 0)
>> W> + port = 0x3f8;
>> W> + }
>> W> + }
>> W> if (speed != -1 && port != -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[] = {
>> 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[] = {
>> 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/efiserialio.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 = false;
>> W> +
>> W> +#if defined(__amd64__)
>> W> +#define comconsole eficomconsole
>> W> +#endif
>> W>
>> W> struct console comconsole = {
>> W> .c_name = "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 = getenv("smbios.bios.version");
>> W> + if (env == NULL || strncmp(env, "Hyper-V", 7) != 0) {
>> W> + return;
>> W> + }
>> W> +#endif
>> W> +
>> W> if (comc_port == NULL) {
>> W> comc_port = calloc(1, sizeof (struct serial));
>> W> if (comc_port == NULL)
>> W> return;
>> W> }
>> W> +
>> W> /* Use defaults from firmware */
>> W> comc_port->databits = 8;
>> W> comc_port->parity = DefaultParity;
>> W> @@ -308,6 +324,10 @@ comc_probe(struct console *sc)
>> W> comc_port_set, env_nounset);
>> W>
>> W> env = getenv("efi_com_speed");
>> W> + if (env == NULL)
>> W> + /* fallback to comconsole setting */
>> W> + env = getenv("comconsole_speed");
>> W> +
>> W> if (comc_parse_intval(env, &val) == CMD_OK)
>> W> comc_port->baudrate = val;
>> W>
>> W> @@ -318,8 +338,13 @@ comc_probe(struct console *sc)
>> W> comc_speed_set, env_nounset);
>> W>
>> W> comconsole.c_flags = 0;
>> W> - if (comc_setup())
>> W> + if (comc_setup()) {
>> W> sc->c_flags = C_PRESENTIN | C_PRESENTOUT;
>> W> + efi_comconsole_avail = true;
>> W> + } else {
>> W> + /* disable being seen as "comconsole" */
>> W> + comconsole.c_name = "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 == NULL)
>> W> @@ -498,10 +524,17 @@ comc_setup(void)
>> W> if (EFI_ERROR(status))
>> W> return (false);
>> W>
>> W> - status = 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 = getenv("smbios.bios.version");
>> W> + if (ev != NULL && strncmp(ev, "Hyper-V", 7) == 0) {
>> W> + status = comc_port->sio->SetAttributes(comc_port->sio,
>> W> + 0, 0, 0, DefaultParity, 0, DefaultStopBits);
>> W> + } else {
>> W> + status = 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/comconsole.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 legacy
>> W> + * com driver to avoid conflicts with the firmware's driver.
>> W> + * Change c_name so that it cannot be found in the lookup.
>> W> + */
>> W> + comconsole.c_name = "xcomconsole";
>> W> + return;
>> W> + }
>> W> +#endif
>> W> +
>> W> if (comc_curspeed == 0) {
>> W> comc_curspeed = COMSPEED;
>> W> /*
>>
>> --
>> Gleb Smirnoff