Page MenuHomeFreeBSD

uefi stand: Guess the console better
ClosedPublic

Authored by imp on Jun 20 2018, 12:35 AM.

Details

Summary

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.

RelNotes: yes

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp created this revision.Jun 20 2018, 12:35 AM
imp added a reviewer: kevans.
kevans added inline comments.Jun 20 2018, 12:44 AM
stand/efi/loader/main.c
586 ↗(On Diff #44111)

Are we eventually going to do something with the com_speed, other than write to it just for printf sake?

imp added inline comments.Jun 20 2018, 12:59 AM
stand/efi/loader/main.c
586 ↗(On Diff #44111)

I hope so. We should set hw.uart.console based on this. However, we're missing bits to do that. If comconsole_pcidev is set, we need to find it one way. If we have comconsole_port set, we know directly. Trouble is, both of these variables are set later so we can't count on them here. And they are only set for comconsole, console not efi console. We could get this information from ACPI, but that's a tree walk, and I'm not really keen to include that.

We likely could do something in the EFI console to export this information, since I think it's set late enough to peek at these variables. But even that looks to be tricky w/o some careful study.

So, enshrine the code here for now, and work on exporting it in a follow on commit.

imp updated this revision to Diff 44125.EditedJun 20 2018, 4:50 AM

I've updated things to be better. We set the baud rate in the environment, and then just before we boot, and we're a EFI console, we get it and some other things to try to piece together the right hw.uart.console value to set if we're booting serial and we are the EFI console.

kevans added inline comments.Jun 20 2018, 1:05 PM
stand/efi/loader/main.c
75 ↗(On Diff #44125)

This seems to have snuck in unused

imp added inline comments.Jun 20 2018, 2:18 PM
stand/efi/loader/main.c
75 ↗(On Diff #44125)

Yea, it's a merge error from a later patch.

imp marked 4 inline comments as done.Jun 20 2018, 4:37 PM
jhb added inline comments.Jun 20 2018, 5:56 PM
stand/efi/loader/main.c
548 ↗(On Diff #44125)

I think setting both means that serial is preferred which may be surprising for folks if a normal desktop box advertises both serial and video as the console as this will effectively turn off the video console post-boot.

(See shenanigans related to RB_SERIAL handling in stand/boot/i386/libi386/bootinfo.c where RB_SERIAL is turned off if "vidconsole" is first)

612 ↗(On Diff #44125)

Debug?

637 ↗(On Diff #44125)

Perhaps rather than RB_RESERVED1 have parse_uefi_con_out() return a boolean and accept howto by reference:

if (parse_uefi_con_out(&howto)) {
    setenv("console", "efi", 1);
} else {
    /* existing code to set "console" */
}
bootenv_set(howto);

This would move the merging into parse_uefi_con_out() by having it construct the temporary it already does and then do the merge stuff before 'return (true)' or the like. I think the code flow would be simpler and read better (and avoid abusing RB_RESERVED1).

654 ↗(On Diff #44125)

Unrelated?

imp added inline comments.Jun 20 2018, 7:41 PM
stand/efi/loader/main.c
548 ↗(On Diff #44125)

Correct. However, there's no way to know which is first at this point...

This does, however, make the kernel messages actually appear on both, which is what we want in this case.

The real solution, however, is to support a real /dev/constty that is the union of all the active ttys.

612 ↗(On Diff #44125)

yea...

637 ↗(On Diff #44125)

I'd dispute it's an abuse: it's supposed to be fore a boot loader defined purpose :)

But it wouldn't be a bad restructure.

654 ↗(On Diff #44125)

Yes and no. We never should have set this.

imp updated this revision to Diff 45259.Jul 13 2018, 10:05 PM

Review feedback and fix overrides

jhb added a comment.Jul 13 2018, 10:14 PM

I think this is ok as far as I can tell.

stand/efi/loader/main.c
582 ↗(On Diff #45259)

Mix of '#define<space>' vs '#define<tab>' (style(9) is to use tab)?

imp updated this revision to Diff 45260.Jul 14 2018, 12:26 AM

Style changes from john, collapse if statement one layer

imp marked 9 inline comments as done.Jul 14 2018, 12:28 AM

last cleanup... Will put through it's paces in a new couple places then commit.

stand/efi/loader/main.c
582 ↗(On Diff #45259)

Fixed.

kevans accepted this revision.Jul 14 2018, 12:35 AM
This revision is now accepted and ready to land.Jul 14 2018, 12:35 AM
This revision was automatically updated to reflect the committed changes.
imp marked an inline comment as done.