add beastie support for loader.efi
ClosedPublic

Authored by tsoome on Jan 5 2016, 9:53 AM.

Details

Summary

This is "backport" of the work I did at illumos side and is essentially based on vidconsole implementation.

few notes:
the getchar() needs to translate special keys, I did left in the switch (in illumos code I was also translating arrow keys etc) even as it currently has only entry for esc key.

"box" bios codes are translated for putchar. the issue is, in EFI the character codes are multibyte and as ficl does not support multibyte chars, this translation is currently most reasonable. There is an catch tho - as in bios the box chars are upper half of the table and therefore are having "negative" values, this will bring in the signed versus unsigned char issue for putchar(). In this diff I did add cast to unsigned char for amd64 variant of ficl. This allows to use meaningful hex codes for bios box chars in putchar.

And final note is about HO() call added for mode command - to make mode switches more "clean".

Test Plan

UI is functional, ESC sequences do work.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
tsoome retitled this revision from to add beastie support for loader.efi.Jan 5 2016, 9:53 AM
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome set the repository for this revision to rS FreeBSD src repository.
emaste added a subscriber: emaste.Jan 5 2016, 6:26 PM

Thank you for this work! I'll review and test as soon as I can.

emaste added inline comments.Jan 5 2016, 6:33 PM
sys/boot/efi/libefi/Makefile
24

Is there a reason an end user wouldn't want TERM_EMU supported (at build time)?

sys/boot/efi/libefi/efi_console.c
2–3

Perhaps this should carry an additional copyright

72–74

Minor nit, different comment style here compared to other functions

133

Should have an XXX marker for now (and be fixed in the future)

tsoome marked an inline comment as done.Jan 5 2016, 6:45 PM

I have been wondering the same really, but I left it in to have same semantics with bios version (vidconsole). I think it should get removed eventually, especially as without terminal support, the beastie will be useless anyhow. I dont know about freebsd traditions about such updates - I personally would rather just see followup issue created to remove TERM_EMU:)

imp added a comment.Jan 5 2016, 6:55 PM
In D4797#101848, @tsoome_me.com wrote:

I have been wondering the same really, but I left it in to have same semantics with bios version (vidconsole). I think it should get removed eventually, especially as without terminal support, the beastie will be useless anyhow. I dont know about freebsd traditions about such updates - I personally would rather just see followup issue created to remove TERM_EMU:)

I know some places have used it to reduce the footprint of the resulting binary, at least for vidconsole. It makes enough of a difference that it is useful there. However, the target market for UEFI I'm less sure of. The only place I think it might matter is for the embedded i386 kit that boots with 32-bit EFI, but those should be beefy enough that I doubt this would matter.

There's a lot of people that don't care about beastie support and compile it out / disable it because it is useless bloat for a boot loader, even if the space savings isn't a consideration.

tsoome marked an inline comment as done.Jan 5 2016, 6:55 PM

I'm not sure how justified it would be in sense that this update is really just duplication of vidconsole. Perhaps later when more proper terminal io support will be added, then it would be more appropriate. In illumos version I actually have cli editor with history as well, but I want to keep this update small and self contained, also the linenoise (used for cli editor), is a bit too "dumb" to support serial console properly and that option would need more investigation.

will added a subscriber: will.Jan 5 2016, 6:57 PM
emaste added a comment.Jan 5 2016, 7:01 PM

The only place I think it might matter is for the embedded i386 kit that boots with 32-bit EFI, but those should be beefy enough that I doubt this would matter.

There's a lot of people that don't care about beastie support and compile it out / disable it because it is useless bloat for a boot loader, even if the space savings isn't a consideration.

Yeah, the extra couple of kbytes is not going to matter for any system with UEFI firmware. I also turn the beastie support off on my own systems, but don't see a need for a build time option here to save the bit of extra space.

I'd have a slight worry that the !TERM_EMU case will bitrot, but in any case it already exists so I'd just commit it as is.

tsoome updated this revision to Diff 11954.Jan 5 2016, 7:01 PM

updated comments as requested.

tsoome marked an inline comment as done.Jan 5 2016, 7:02 PM

fixed

tsoome marked an inline comment as done.Jan 5 2016, 7:02 PM

fixed

tsoome added a comment.Jan 5 2016, 8:16 PM
In D4797#101857, @imp wrote:
In D4797#101848, @tsoome_me.com wrote:

I have been wondering the same really, but I left it in to have same semantics with bios version (vidconsole). I think it should get removed eventually, especially as without terminal support, the beastie will be useless anyhow. I dont know about freebsd traditions about such updates - I personally would rather just see followup issue created to remove TERM_EMU:)

I know some places have used it to reduce the footprint of the resulting binary, at least for vidconsole. It makes enough of a difference that it is useful there. However, the target market for UEFI I'm less sure of. The only place I think it might matter is for the embedded i386 kit that boots with 32-bit EFI, but those should be beefy enough that I doubt this would matter.

There's a lot of people that don't care about beastie support and compile it out / disable it because it is useless bloat for a boot loader, even if the space savings isn't a consideration.

of course it is not hard to remove it now, but having it around for now also adds some possible value (even if slim). I personally like the view where options for different platforms are similar. But if people feel it is really unnecessary, I don't really mind to nuke it.

adrian accepted this revision.Jan 5 2016, 9:57 PM
adrian added a reviewer: adrian.
adrian added a subscriber: adrian.

looks good. I'll refactor this stuff after it's in -head.

This revision is now accepted and ready to land.Jan 5 2016, 9:57 PM
emaste accepted this revision.Jan 6 2016, 3:17 PM
emaste added a reviewer: emaste.

looks good. I'll refactor this stuff after it's in -head.

Yeah, I have a few tweaks I might make before committing later today.

This revision was automatically updated to reflect the committed changes.
tsoome added a comment.Jan 6 2016, 4:04 PM

hi!

apparently you have missed beastie.4th diff - the beastie-start will not run if console is efi;)

rgds,
toomas

emaste added a comment.Jan 6 2016, 4:06 PM

apparently you have missed beastie.4th diff - the beastie-start will not run if console is efi;)

Yeah, I got it in the next commit (rS293234) along with an update to the man pages.

tsoome added a comment.Jan 6 2016, 4:08 PM

yes, I just did notice it:)

thanks,
toomas