Date: Tue, 16 May 2023 20:19:25 -0600 From: Warner Losh <imp@bsdimp.com> To: Gleb Smirnoff <glebius@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, "<dev-commits-src-main@freebsd.org>" <dev-commits-src-main@freebsd.org> Subject: Re: HEADS UP: broken boot! Was: git: 2f131435bc22 - main - stand: efi create eficom console device. Message-ID: <CANCZdfq%2BFx-nU5eCzhjF0=ujCAepVFw09_VpwXSRAQYRoRDiPA@mail.gmail.com> In-Reply-To: <ZGQ4GuTLcQhv9avt@FreeBSD.org> References: <202305112006.34BK6kPw019492@gitrepo.freebsd.org> <ZGQ4GuTLcQhv9avt@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Tue, May 16, 2023, 10:12 PM Gleb Smirnoff <glebius@freebsd.org> wrote: > Hi, > > this commit breaks boot on some machines with EFI. Warner already knows > and he > provided a quick hack to make it bootable. The proper fix will be > available later. > > Meanwhile you are advised to not update your loader past 2f131435bc22, or > use the patch (attached). > It breaks dual console loaders. I should have a fix in the morning. Too much bsdcan cheer tonight to think straight. Warner On Thu, May 11, 2023 at 08:06:46PM +0000, Warner Losh wrote: > W> The branch main has been updated by imp: > W> > W> URL: > https://cgit.FreeBSD.org/src/commit/?id=2f131435bc22540db2d3f6bf97e5f9fe7039f889 > W> > W> commit 2f131435bc22540db2d3f6bf97e5f9fe7039f889 > W> Author: Warner Losh <imp@FreeBSD.org> > W> AuthorDate: 2023-05-11 20:03:17 +0000 > W> Commit: Warner Losh <imp@FreeBSD.org> > W> CommitDate: 2023-05-11 20:06:03 +0000 > W> > W> stand: efi create eficom console device. > W> > W> Fix the 'renaming kludge' that we absolutely cannot do going forward > W> (it's cost us days of engineering time). > W> > W> console=comconsole talks to the hardware directly. This is available > W> only on amd64. It is not available anywhere else (and so requires > W> changes for people doing comconsole on aarch64) > W> > W> console=eficom talks to the console via EFI protocols. It's > available > W> on amd64, aarch64 and riscv64. It's the first port that we find, > though > W> it can be overriden by efi_com_port (which should be set to the UID > of > W> the serial port, not the I/O port, despite the name). devinfo -v > W> will give the UID to uartX mapping. > W> > W> This is an incompatible change for HYPER-V on amd64. It only works > with > W> eficom console, so you'll need to change your configuration in > W> loader.conf. No compatibility hack will ever be provided for this > (since > W> it requires renamig, which the loader cannot reliably do). > W> > W> It's also an incompatible change for aarch64. comconsole will need > to > W> change to eficom. There might be a comconsole "shim" for this. > W> > W> All the interlock to keep only eficom and comconsole from both > attaching > W> have been removed. > W> > W> RelNotes: Yes > W> Sponsored by: Netflix > W> Discussed with: kevans > W> Differential Revision: https://reviews.freebsd.org/D39982 > W> --- > W> stand/efi/loader/conf.c | 12 ++---- > W> stand/efi/loader/efiserialio.c | 85 > +++++++++++++++++------------------------ > W> stand/i386/libi386/comconsole.c | 14 ------- > W> 3 files changed, 39 insertions(+), 72 deletions(-) > W> > W> diff --git a/stand/efi/loader/conf.c b/stand/efi/loader/conf.c > W> index 051e1a3381d1..e9ae01d19270 100644 > W> --- a/stand/efi/loader/conf.c > W> +++ b/stand/efi/loader/conf.c > W> @@ -80,22 +80,18 @@ struct netif_driver *netif_drivers[] = { > W> }; > W> > W> extern struct console efi_console; > W> -extern struct console comconsole; > W> -#if defined(__amd64__) > W> -extern struct console eficomconsole; > W> -#endif > W> +extern struct console eficom; > W> #if defined(__amd64__) || defined(__i386__) > W> +extern struct console comconsole; > W> extern struct console nullconsole; > W> extern struct console spinconsole; > W> #endif > W> > W> struct console *consoles[] = { > W> &efi_console, > W> -#if defined(__amd64__) > W> - &eficomconsole, > W> -#endif > W> - &comconsole, > W> + &eficom, > W> #if defined(__amd64__) || defined(__i386__) > W> + &comconsole, > W> &nullconsole, > W> &spinconsole, > W> #endif > W> diff --git a/stand/efi/loader/efiserialio.c > b/stand/efi/loader/efiserialio.c > W> index 0f37ef8b87dd..de4d6b3e34c1 100644 > W> --- a/stand/efi/loader/efiserialio.c > W> +++ b/stand/efi/loader/efiserialio.c > W> @@ -69,14 +69,9 @@ 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> +struct console eficom = { > W> + .c_name = "eficom", > W> .c_desc = "serial port", > W> .c_flags = 0, > W> .c_probe = comc_probe, > W> @@ -259,18 +254,6 @@ 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> - /* Disable being seen as "comconsole". */ > W> - comconsole.c_name = "efiserialio"; > 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> @@ -339,13 +322,9 @@ comc_probe(struct console *sc) > W> env_setenv("efi_com_speed", EV_VOLATILE, value, > W> comc_speed_set, env_nounset); > W> > W> - comconsole.c_flags = 0; > W> + eficom.c_flags = 0; > 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> @@ -356,7 +335,7 @@ comc_init(int arg __unused) > W> if (comc_setup()) > W> return (CMD_OK); > W> > W> - comconsole.c_flags = 0; > W> + eficom.c_flags = 0; > W> return (CMD_ERROR); > W> } > W> > W> @@ -522,35 +501,41 @@ comc_setup(void) > W> if (comc_port->sio == NULL) > W> return (false); > W> > W> - status = comc_port->sio->Reset(comc_port->sio); > W> - if (EFI_ERROR(status)) > W> - return (false); > W> - > 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> + if (comc_port->sio->Reset != NULL) { > W> + status = comc_port->sio->Reset(comc_port->sio); > W> + if (EFI_ERROR(status)) > W> + return (false); > W> } > W> > W> - if (EFI_ERROR(status)) > W> - return (false); > W> + if (comc_port->sio->SetAttributes != NULL) { > 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> - status = comc_port->sio->GetControl(comc_port->sio, &control); > W> - if (EFI_ERROR(status)) > W> - return (false); > W> - if (comc_port->rtsdtr_off) { > W> - control &= ~(EFI_SERIAL_REQUEST_TO_SEND | > W> - EFI_SERIAL_DATA_TERMINAL_READY); > W> - } else { > W> - control |= EFI_SERIAL_REQUEST_TO_SEND; > W> + if (EFI_ERROR(status)) > W> + return (false); > W> + } > W> + > W> + if (comc_port->sio->GetControl != NULL && > comc_port->sio->SetControl != NULL) { > W> + status = comc_port->sio->GetControl(comc_port->sio, > &control); > W> + if (EFI_ERROR(status)) > W> + return (false); > W> + if (comc_port->rtsdtr_off) { > W> + control &= ~(EFI_SERIAL_REQUEST_TO_SEND | > W> + EFI_SERIAL_DATA_TERMINAL_READY); > W> + } else { > W> + control |= EFI_SERIAL_REQUEST_TO_SEND; > W> + } > W> + (void) comc_port->sio->SetControl(comc_port->sio, control); > W> } > W> - (void) comc_port->sio->SetControl(comc_port->sio, control); > W> /* Mark this port usable. */ > W> - comconsole.c_flags |= (C_PRESENTIN | C_PRESENTOUT); > W> + eficom.c_flags |= (C_PRESENTIN | C_PRESENTOUT); > W> return (true); > W> } > W> diff --git a/stand/i386/libi386/comconsole.c > b/stand/i386/libi386/comconsole.c > W> index 507cd0ec922f..6d48e876fa37 100644 > W> --- a/stand/i386/libi386/comconsole.c > W> +++ b/stand/i386/libi386/comconsole.c > W> @@ -85,20 +85,6 @@ 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 > [-- Attachment #2 --] <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 16, 2023, 10:12 PM Gleb Smirnoff <<a href="mailto:glebius@freebsd.org">glebius@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi,<br> <br> this commit breaks boot on some machines with EFI. Warner already knows and he<br> provided a quick hack to make it bootable. The proper fix will be available later.<br> <br> Meanwhile you are advised to not update your loader past 2f131435bc22, or <br> use the patch (attached).<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">It breaks dual console loaders. I should have a fix in the morning. Too much bsdcan cheer tonight to think straight. </div><div dir="auto"><br></div><div dir="auto">Warner </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Thu, May 11, 2023 at 08:06:46PM +0000, Warner Losh wrote:<br> W> The branch main has been updated by imp:<br> W> <br> W> URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=2f131435bc22540db2d3f6bf97e5f9fe7039f889" rel="noreferrer noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=2f131435bc22540db2d3f6bf97e5f9fe7039f889</a><br>; W> <br> W> commit 2f131435bc22540db2d3f6bf97e5f9fe7039f889<br> W> Author: Warner Losh <imp@FreeBSD.org><br> W> AuthorDate: 2023-05-11 20:03:17 +0000<br> W> Commit: Warner Losh <imp@FreeBSD.org><br> W> CommitDate: 2023-05-11 20:06:03 +0000<br> W> <br> W> stand: efi create eficom console device.<br> W> <br> W> Fix the 'renaming kludge' that we absolutely cannot do going forward<br> W> (it's cost us days of engineering time).<br> W> <br> W> console=comconsole talks to the hardware directly. This is available<br> W> only on amd64. It is not available anywhere else (and so requires<br> W> changes for people doing comconsole on aarch64)<br> W> <br> W> console=eficom talks to the console via EFI protocols. It's available<br> W> on amd64, aarch64 and riscv64. It's the first port that we find, though<br> W> it can be overriden by efi_com_port (which should be set to the UID of<br> W> the serial port, not the I/O port, despite the name). devinfo -v<br> W> will give the UID to uartX mapping.<br> W> <br> W> This is an incompatible change for HYPER-V on amd64. It only works with<br> W> eficom console, so you'll need to change your configuration in<br> W> loader.conf. No compatibility hack will ever be provided for this (since<br> W> it requires renamig, which the loader cannot reliably do).<br> W> <br> W> It's also an incompatible change for aarch64. comconsole will need to<br> W> change to eficom. There might be a comconsole "shim" for this.<br> W> <br> W> All the interlock to keep only eficom and comconsole from both attaching<br> W> have been removed.<br> W> <br> W> RelNotes: Yes<br> W> Sponsored by: Netflix<br> W> Discussed with: kevans<br> W> Differential Revision: <a href="https://reviews.freebsd.org/D39982" rel="noreferrer noreferrer" target="_blank">https://reviews.freebsd.org/D39982</a><br>; W> ---<br> W> stand/efi/loader/conf.c | 12 ++----<br> W> stand/efi/loader/efiserialio.c | 85 +++++++++++++++++------------------------<br> W> stand/i386/libi386/comconsole.c | 14 -------<br> W> 3 files changed, 39 insertions(+), 72 deletions(-)<br> W> <br> W> diff --git a/stand/efi/loader/conf.c b/stand/efi/loader/conf.c<br> W> index 051e1a3381d1..e9ae01d19270 100644<br> W> --- a/stand/efi/loader/conf.c<br> W> +++ b/stand/efi/loader/conf.c<br> W> @@ -80,22 +80,18 @@ struct netif_driver *netif_drivers[] = {<br> W> };<br> W> <br> W> extern struct console efi_console;<br> W> -extern struct console comconsole;<br> W> -#if defined(__amd64__)<br> W> -extern struct console eficomconsole;<br> W> -#endif<br> W> +extern struct console eficom;<br> W> #if defined(__amd64__) || defined(__i386__)<br> W> +extern struct console comconsole;<br> W> extern struct console nullconsole;<br> W> extern struct console spinconsole;<br> W> #endif<br> W> <br> W> struct console *consoles[] = {<br> W> &efi_console,<br> W> -#if defined(__amd64__)<br> W> - &eficomconsole,<br> W> -#endif<br> W> - &comconsole,<br> W> + &eficom,<br> W> #if defined(__amd64__) || defined(__i386__)<br> W> + &comconsole,<br> W> &nullconsole,<br> W> &spinconsole,<br> W> #endif<br> W> diff --git a/stand/efi/loader/efiserialio.c b/stand/efi/loader/efiserialio.c<br> W> index 0f37ef8b87dd..de4d6b3e34c1 100644<br> W> --- a/stand/efi/loader/efiserialio.c<br> W> +++ b/stand/efi/loader/efiserialio.c<br> W> @@ -69,14 +69,9 @@ static int comc_speed_set(struct env_var *, int, const void *);<br> W> <br> W> static struct serial *comc_port;<br> W> extern struct console efi_console;<br> W> -bool efi_comconsole_avail = false;<br> W> <br> W> -#if defined(__amd64__)<br> W> -#define comconsole eficomconsole<br> W> -#endif<br> W> -<br> W> -struct console comconsole = {<br> W> - .c_name = "comconsole",<br> W> +struct console eficom = {<br> W> + .c_name = "eficom",<br> W> .c_desc = "serial port",<br> W> .c_flags = 0,<br> W> .c_probe = comc_probe,<br> W> @@ -259,18 +254,6 @@ comc_probe(struct console *sc)<br> W> char *env, *buf, *ep;<br> W> size_t sz;<br> W> <br> W> -#if defined(__amd64__)<br> W> - /*<br> W> - * For x86-64, don't use this driver if not running in Hyper-V.<br> W> - */<br> W> - env = getenv("smbios.bios.version");<br> W> - if (env == NULL || strncmp(env, "Hyper-V", 7) != 0) {<br> W> - /* Disable being seen as "comconsole". */<br> W> - comconsole.c_name = "efiserialio";<br> W> - return;<br> W> - }<br> W> -#endif<br> W> -<br> W> if (comc_port == NULL) {<br> W> comc_port = calloc(1, sizeof (struct serial));<br> W> if (comc_port == NULL)<br> W> @@ -339,13 +322,9 @@ comc_probe(struct console *sc)<br> W> env_setenv("efi_com_speed", EV_VOLATILE, value,<br> W> comc_speed_set, env_nounset);<br> W> <br> W> - comconsole.c_flags = 0;<br> W> + eficom.c_flags = 0;<br> W> if (comc_setup()) {<br> W> sc->c_flags = C_PRESENTIN | C_PRESENTOUT;<br> W> - efi_comconsole_avail = true;<br> W> - } else {<br> W> - /* disable being seen as "comconsole" */<br> W> - comconsole.c_name = "efiserialio";<br> W> }<br> W> }<br> W> <br> W> @@ -356,7 +335,7 @@ comc_init(int arg __unused)<br> W> if (comc_setup())<br> W> return (CMD_OK);<br> W> <br> W> - comconsole.c_flags = 0;<br> W> + eficom.c_flags = 0;<br> W> return (CMD_ERROR);<br> W> }<br> W> <br> W> @@ -522,35 +501,41 @@ comc_setup(void)<br> W> if (comc_port->sio == NULL)<br> W> return (false);<br> W> <br> W> - status = comc_port->sio->Reset(comc_port->sio);<br> W> - if (EFI_ERROR(status))<br> W> - return (false);<br> W> -<br> W> - ev = getenv("smbios.bios.version");<br> W> - if (ev != NULL && strncmp(ev, "Hyper-V", 7) == 0) {<br> W> - status = comc_port->sio->SetAttributes(comc_port->sio,<br> W> - 0, 0, 0, DefaultParity, 0, DefaultStopBits);<br> W> - } else {<br> W> - status = comc_port->sio->SetAttributes(comc_port->sio,<br> W> - comc_port->baudrate, comc_port->receivefifodepth,<br> W> - comc_port->timeout, comc_port->parity,<br> W> - comc_port->databits, comc_port->stopbits);<br> W> + if (comc_port->sio->Reset != NULL) {<br> W> + status = comc_port->sio->Reset(comc_port->sio);<br> W> + if (EFI_ERROR(status))<br> W> + return (false);<br> W> }<br> W> <br> W> - if (EFI_ERROR(status))<br> W> - return (false);<br> W> + if (comc_port->sio->SetAttributes != NULL) {<br> W> + ev = getenv("smbios.bios.version");<br> W> + if (ev != NULL && strncmp(ev, "Hyper-V", 7) == 0) {<br> W> + status = comc_port->sio->SetAttributes(comc_port->sio,<br> W> + 0, 0, 0, DefaultParity, 0, DefaultStopBits);<br> W> + } else {<br> W> + status = comc_port->sio->SetAttributes(comc_port->sio,<br> W> + comc_port->baudrate, comc_port->receivefifodepth,<br> W> + comc_port->timeout, comc_port->parity,<br> W> + comc_port->databits, comc_port->stopbits);<br> W> + }<br> W> <br> W> - status = comc_port->sio->GetControl(comc_port->sio, &control);<br> W> - if (EFI_ERROR(status))<br> W> - return (false);<br> W> - if (comc_port->rtsdtr_off) {<br> W> - control &= ~(EFI_SERIAL_REQUEST_TO_SEND |<br> W> - EFI_SERIAL_DATA_TERMINAL_READY);<br> W> - } else {<br> W> - control |= EFI_SERIAL_REQUEST_TO_SEND;<br> W> + if (EFI_ERROR(status))<br> W> + return (false);<br> W> + }<br> W> +<br> W> + if (comc_port->sio->GetControl != NULL && comc_port->sio->SetControl != NULL) {<br> W> + status = comc_port->sio->GetControl(comc_port->sio, &control);<br> W> + if (EFI_ERROR(status))<br> W> + return (false);<br> W> + if (comc_port->rtsdtr_off) {<br> W> + control &= ~(EFI_SERIAL_REQUEST_TO_SEND |<br> W> + EFI_SERIAL_DATA_TERMINAL_READY);<br> W> + } else {<br> W> + control |= EFI_SERIAL_REQUEST_TO_SEND;<br> W> + }<br> W> + (void) comc_port->sio->SetControl(comc_port->sio, control);<br> W> }<br> W> - (void) comc_port->sio->SetControl(comc_port->sio, control);<br> W> /* Mark this port usable. */<br> W> - comconsole.c_flags |= (C_PRESENTIN | C_PRESENTOUT);<br> W> + eficom.c_flags |= (C_PRESENTIN | C_PRESENTOUT);<br> W> return (true);<br> W> }<br> W> diff --git a/stand/i386/libi386/comconsole.c b/stand/i386/libi386/comconsole.c<br> W> index 507cd0ec922f..6d48e876fa37 100644<br> W> --- a/stand/i386/libi386/comconsole.c<br> W> +++ b/stand/i386/libi386/comconsole.c<br> W> @@ -85,20 +85,6 @@ comc_probe(struct console *cp)<br> W> int speed, port;<br> W> uint32_t locator;<br> W> <br> W> -#if defined(__amd64__)<br> W> - extern bool efi_comconsole_avail;<br> W> -<br> W> - if (efi_comconsole_avail) {<br> W> - /*<br> W> - * If EFI provides serial I/O, then don't use this legacy<br> W> - * com driver to avoid conflicts with the firmware's driver.<br> W> - * Change c_name so that it cannot be found in the lookup.<br> W> - */<br> W> - comconsole.c_name = "xcomconsole";<br> W> - return;<br> W> - }<br> W> -#endif<br> W> -<br> W> if (comc_curspeed == 0) {<br> W> comc_curspeed = COMSPEED;<br> W> /*<br> <br> -- <br> Gleb Smirnoff<br> </blockquote></div></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfq%2BFx-nU5eCzhjF0=ujCAepVFw09_VpwXSRAQYRoRDiPA>