Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 13 Jul 2018 20:13:00 -0600
From:      Warner Losh <imp@bsdimp.com>
To:        Warner Losh <imp@freebsd.org>
Cc:        src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r336270 - head/stand/efi/loader
Message-ID:  <CANCZdfqpaxdKnpTD8MGaqDgt5K9b1cFTF2PsQf5d1Y3DOWonJA@mail.gmail.com>
In-Reply-To: <201807140040.w6E0eddV033638@repo.freebsd.org>
References:  <201807140040.w6E0eddV033638@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Uggg. This commit message is slightly wrong.

Added a printf I wanted to add anyway to a new version and made a note
about what's wrong with it. I changed the behavior based on review
feedback, but didn't change the commit message before the commit.

Warner

On Fri, Jul 13, 2018 at 6:40 PM, Warner Losh <imp@freebsd.org> wrote:

> Author: imp
> Date: Sat Jul 14 00:40:38 2018
> New Revision: 336270
> URL: https://svnweb.freebsd.org/changeset/base/336270
>
> Log:
>   uefi stand: Guess the console better
>
>   For server machines, ComOut is set to the set of devices that the efi
>   console suppots. Parse it to see if we have serial, video or both.
>   Make that take precidence over the command line args. boot1.efi parses
>   them, but loader.efi doesn't. It's not clear where to read boot.conf
>   from, so we don't do that. The command line args can still be set via
>   efibootmgr, which is more inline with the UEFI boot manager to replace
>   that. These args are typically used only to set serial vs video and
>   the com speed line. We can infer that from ComOut, so do so.
>   Remember the com speed and hw.uart.console to match.
>
>   RelNotes: yes
>   Sponsored by: Netflix
>   Differential Revision: https://reviews.freebsd.org/D15917
>
> Modified:
>   head/stand/efi/loader/bootinfo.c
>   head/stand/efi/loader/main.c
>
> Modified: head/stand/efi/loader/bootinfo.c
> ============================================================
> ==================
> --- head/stand/efi/loader/bootinfo.c    Fri Jul 13 23:46:07 2018
> (r336269)
> +++ head/stand/efi/loader/bootinfo.c    Sat Jul 14 00:40:38 2018
> (r336270)
> @@ -67,10 +67,11 @@ extern EFI_SYSTEM_TABLE     *ST;
>  static int
>  bi_getboothowto(char *kargs)
>  {
> -       const char *sw;
> +       const char *sw, *tmp;
>         char *opts;
>         char *console;
> -       int howto;
> +       int howto, speed, port;
> +       char buf[50];
>
>         howto = boot_parse_cmdline(kargs);
>         howto |= boot_env_to_howto();
> @@ -81,6 +82,35 @@ bi_getboothowto(char *kargs)
>                         howto |= RB_SERIAL;
>                 if (strcmp(console, "nullconsole") == 0)
>                         howto |= RB_MUTE;
> +               if (strcmp(console, "efi") == 0) {
> +                       /*
> +                        * If we found a com port and com speed, we need
> to tell
> +                        * the kernel where the serial port is, and how
> +                        * fast. Ideally, we'd get the port from ACPI, but
> that
> +                        * isn't running in the loader. Do the next best
> thing
> +                        * by allowing it to be set by a loader.conf
> variable,
> +                        * either a EFI specific one, or the compatible
> +                        * comconsole_port if not. PCI support is needed,
> but
> +                        * for that we'd ideally refactor the
> +                        * libi386/comconsole.c code to have identical
> behavior.
> +                        */
> +                       tmp = getenv("efi_com_speed");
> +                       if (tmp != NULL) {
> +                               speed = strtol(tmp, NULL, 0);
> +                               tmp = getenv("efi_com_port");
> +                               if (tmp == NULL)
> +                                       tmp = getenv("comconsole_port");
> +                               /* XXX fallback to EFI variable set in
> rc.d? */
> +                               if (tmp != NULL)
> +                                       port = strtol(tmp, NULL, 0);
> +                               else
> +                                       port = 0x3f8;
> +                               snprintf(buf, sizeof(buf), "io:%d,br:%d",
> port,
> +                                   speed);
> +                               env_setenv("hw.uart.console", EV_VOLATILE,
> buf,
> +                                   NULL, NULL);
> +                       }
> +               }
>         }
>
>         return (howto);
>
> Modified: head/stand/efi/loader/main.c
> ============================================================
> ==================
> --- head/stand/efi/loader/main.c        Fri Jul 13 23:46:07 2018
> (r336269)
> +++ head/stand/efi/loader/main.c        Sat Jul 14 00:40:38 2018
> (r336270)
> @@ -400,8 +400,8 @@ interactive_interrupt(const char *msg)
>         return (false);
>  }
>
> -int
> -parse_args(int argc, CHAR16 *argv[], bool has_kbd)
> +static int
> +parse_args(int argc, CHAR16 *argv[])
>  {
>         int i, j, howto;
>         bool vargood;
> @@ -429,12 +429,98 @@ parse_args(int argc, CHAR16 *argv[], bool has_kbd)
>         return (howto);
>  }
>
> +/*
> + * Parse ConOut (the list of consoles active) and see if we can find a
> + * serial port and/or a video port. It would be nice to also walk the
> + * ACPI name space to map the UID for the serial port to a port. The
> + * latter is especially hard.
> + */
> +static int
> +parse_uefi_con_out(void)
> +{
> +       int how, rv;
> +       int vid_seen = 0, com_seen = 0, seen = 0;
> +       size_t sz;
> +       char buf[4096], *ep;
> +       EFI_DEVICE_PATH *node;
> +       ACPI_HID_DEVICE_PATH  *acpi;
> +       UART_DEVICE_PATH  *uart;
> +       bool pci_pending;
>
> +       how = 0;
> +       sz = sizeof(buf);
> +       rv = efi_global_getenv("ConOut", buf, &sz);
> +       if (rv != EFI_SUCCESS)
> +               goto out;
> +       ep = buf + sz;
> +       node = (EFI_DEVICE_PATH *)buf;
> +       while ((char *)node < ep && !IsDevicePathEndType(node)) {
> +               pci_pending = false;
> +               if (DevicePathType(node) == ACPI_DEVICE_PATH &&
> +                   DevicePathSubType(node) == ACPI_DP) {
> +                       /* Check for Serial node */
> +                       acpi = (void *)node;
> +                       if (EISA_ID_TO_NUM(acpi->HID) == 0x501)
> +                               com_seen = ++seen;
> +               } else if (DevicePathType(node) == MESSAGING_DEVICE_PATH &&
> +                   DevicePathSubType(node) == MSG_UART_DP) {
> +                       char bd[16];
> +
> +                       uart = (void *)node;
> +                       snprintf(bd, sizeof(bd), "%d", uart->BaudRate);
> +                       setenv("efi_com_speed", bd, 1);
> +                       printf("Console speed is %d\n", uart->BaudRate);
> +               } else if (DevicePathType(node) == ACPI_DEVICE_PATH &&
> +                   DevicePathSubType(node) == ACPI_ADR_DP) {
> +                       /* Check for AcpiAdr() Node for video */
> +                       vid_seen = ++seen;
> +               } else if (DevicePathType(node) == HARDWARE_DEVICE_PATH &&
> +                   DevicePathSubType(node) == HW_PCI_DP) {
> +                       /*
> +                        * Note, vmware fusion has a funky console device
> +                        *      PciRoot(0x0)/Pci(0xf,0x0)
> +                        * which we can only detect at the end since we
> also
> +                        * have to cope with:
> +                        *      PciRoot(0x0)/Pci(0x1f,0x0)/Serial(0x1)
> +                        * so only match it if it's last.
> +                        */
> +                       pci_pending = true;
> +               }
> +               node = NextDevicePathNode(node); /* Skip the end node */
> +       }
> +       if (pci_pending && vid_seen == 0)
> +               vid_seen = ++seen;
> +
> +       /*
> +        * Truth table for RB_MULTIPLE | RB_SERIAL
> +        * Value                Result
> +        * 0                    Use only video console
> +        * RB_SERIAL            Use only serial console
> +        * RB_MULTIPLE          Use both video and serial console
> +        *                      (but video is primary so gets rc messages)
> +        * both                 Use both video and serial console
> +        *                      (but serial is primary so gets rc messages)
> +        *
> +        * Try to honor this as best we can. If only one of serial / video
> +        * found, then use that. Otherwise, use the first one we found.
> +        * This also implies if we found nothing, default to video.
> +        */
> +       how = 0;
> +       if (vid_seen && com_seen) {
> +               how |= RB_MULTIPLE;
> +               if (com_seen < vid_seen)
> +                       how |= RB_SERIAL;
> +       } else if (com_seen)
> +               how |= RB_SERIAL;
> +out:
> +       return (how);
> +}
> +
>  EFI_STATUS
>  main(int argc, CHAR16 *argv[])
>  {
>         EFI_GUID *guid;
> -       int howto, i;
> +       int howto, i, uhowto;
>         UINTN k;
>         bool has_kbd;
>         char *s;
> @@ -481,23 +567,61 @@ main(int argc, CHAR16 *argv[])
>          */
>         bcache_init(32768, 512);
>
> -       howto = parse_args(argc, argv, has_kbd);
> +       howto = parse_args(argc, argv);
> +       if (!has_kbd && (howto & RB_PROBE))
> +               howto |= RB_SERIAL | RB_MULTIPLE;
> +       howto &= ~RB_PROBE;
>
> -       boot_howto_to_env(howto);
> +       uhowto = parse_uefi_con_out();
>
>         /*
> -        * XXX we need fallback to this stuff after looking at the ConIn,
> ConOut and ConErr variables
> +        * We now have two notions of console. howto should be viewed as
> +        * overrides.
>          */
> -       if (howto & RB_MULTIPLE) {
> -               if (howto & RB_SERIAL)
> -                       setenv("console", "comconsole efi" , 1);
> -               else
> -                       setenv("console", "efi comconsole" , 1);
> -       } else if (howto & RB_SERIAL) {
> -               setenv("console", "comconsole" , 1);
> -       } else
> -               setenv("console", "efi", 1);
> +#define        VIDEO_ONLY      0
> +#define        SERIAL_ONLY     RB_SERIAL
> +#define        VID_SER_BOTH    RB_MULTIPLE
> +#define        SER_VID_BOTH    (RB_SERIAL | RB_MULTIPLE)
> +#define        CON_MASK        (RB_SERIAL | RB_MULTIPLE)
>
> +       if ((howto & CON_MASK) == 0) {
> +               /* No override, uhowto is controlling and efi cons is
> perfect */
> +               howto = howto | (uhowto & CON_MASK);
> +               setenv("console", "efi", 1);
> +       } else if ((howto & CON_MASK) == (uhowto & CON_MASK)) {
> +               /* override matches what UEFI told us, efi console is
> perfect */
> +               setenv("console", "efi", 1);
> +       } else if ((uhowto & (CON_MASK)) != 0) {
> +               /*
> +                * We detected a serial console on ConOut. All possible
> +                * overrides include serial. We can't really override what
> efi
> +                * gives us, so we use it knowing it's the best choice.
> +                */
> +               setenv("console", "efi", 1);
> +       } else {
> +               /*
> +                * We detected some kind of serial in the override, but
> ConOut
> +                * has no serial, so we have to sort out which case it
> really is.
> +                */
> +               switch (howto & CON_MASK) {
> +               case SERIAL_ONLY:
> +                       setenv("console", "comconsole", 1);
> +                       break;
> +               case VID_SER_BOTH:
> +                       setenv("console", "efi comconsole", 1);
> +                       break;
> +               case SER_VID_BOTH:
> +                       setenv("console", "comconsole efi", 1);
> +                       break;
> +               /* case VIDEO_ONLY can't happen -- it's the first if above
> */
> +               }
> +       }
> +       /*
> +        * howto is set now how we want to export the flags to the kernel,
> so
> +        * set the env based on it.
> +        */
> +       boot_howto_to_env(howto);
> +
>         if (efi_copy_init()) {
>                 printf("failed to allocate staging area\n");
>                 return (EFI_BUFFER_TOO_SMALL);
>
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqpaxdKnpTD8MGaqDgt5K9b1cFTF2PsQf5d1Y3DOWonJA>