[PATCH es] bootmenu: Gerd Hoffman version - feedback

[Mike Banon]

Dear Gerd Hoffman,

If my advanced_bootmenu new patch ( [*] link below ) is not
upstream-able ( for what reasons? ), I will be happy if your
alternative solution gets merged - at least because it is better than
what's currently in SeaBIOS master. However, I just tested your
patches and noticed some issues which did not exist at my original
version [*] - https://mail.coreboot.org/hyperkitty/list/[e-mail]/thread/CKWLNTZ...
. I hope that you could review my feedback and address at least some
of them :

1) It is not possible to choose the boot entry by a single key press
of a fullsize keyboard. We have so many keyboard keys, why are we
limited to 1234567890 ? Seems inefficient. The alternative way, which
I followed, is to continue with qwertyuiop etc. - to utilize 4 rows of
keyboard keys - and make a single key press selection possible.

2) If there are >10 menu entries and you accidentally press '0'
instead of '1', there is no way to backspace/delete this wrong
character. Although, if there is an ability to remove the wrong
characters, then a confirmation of a correct input (e.g. by pressing
the Enter key) is also needed - to confirm that your last character
wasn't wrong. I implemented this mini console interface in my version
- but only for a numpad; for keyboard it wasn't needed because it is a
single key boot there.

3) If there are >20 menu entries, the first menu entries may not fit
on a screen. (That's why I had to add 2 pages support.) Perhaps you
think it's unlikely that anyone would need >20 menu entries: however,
if you are a floppy-based OS developer who is debugging something that
could be reproduced only at bare metal (so no QEMU) - you may want to
add a lot of different versions of your floppy to your coreboot build
- to reduce the amount of BIOS chip reflashes and just for your
convenience. I stopped at 35 max simply because ran out of keyboard
letter keys (could have gone double-letter but wanted to preserve a
single key boot).

4) Although a few USB numpads do have an Escape key (e.g. Motospeed
K23), the majority of them do not - so it is impossible for them to
open a boot menu by pressing Escape - simply because no such key.
(That's why I added "/" character to the menukeys - every USB numpad
has this key.) Also, in your patch version it's not possible to access
TPM from a numpad because no t or m keys on it - so I had to add '-'
character to the menukeys.

To summarize, perhaps the only advantage of your current patch version
is that it is 3 times smaller, so 3 times more likely to be merged.
However it doesn't necessarily mean that my original patch is
inefficient or bloated: I optimized it as best as I could, and quite
sure that if you'd have tried to implement the features mentioned
above - as well as some others I may have forgot to mention - then
your patch linecount would've jumped too.

P.S. If there are any issues with my patch, which have persuaded you
to create your own version (aside from the size issue) - I am open to
suggestions and could try to address them when I will have enough free
time.

Best regards,
Mike Banon

[Gerd Hoffmann]

On Thu, Apr 11, 2019 at 05:11:29PM +0300, Mike Banon wrote:
> Dear Gerd Hoffman,
>
> If my advanced_bootmenu new patch ( [*] link below ) is not
> upstream-able ( for what reasons? ),

Too many changes in a single patch.  Needs splitting up into one patch
per change.
> what's currently in SeaBIOS master. However, I just tested your
> patches and noticed some issues which did not exist at my original
> version [*] - https://mail.coreboot.org/hyperkitty/list/[e-mail]/thread/CKWLNTZ...
> . I hope that you could review my feedback and address at least some
> of them :

Well, the patch solves *one* problem.
> It is not possible to choose the boot entry by a single key press
>
> of a fullsize keyboard. We have so many keyboard keys, why are we
> limited to 1234567890 ? Seems inefficient. The alternative way, which
> I followed, is to continue with qwertyuiop etc. - to utilize 4 rows of
> keyboard keys - and make a single key press selection possible.

Fixed in v2 -- back to single key mode, using letter keys for entry #10
and beyond.
> If there are >20 menu entries, the first menu entries may not fit
>
> on a screen. (That's why I had to add 2 pages support.) Perhaps you
> think it's unlikely that anyone would need >20 menu entries: however,

Even 10 entries not being enough comes up rarely enough.  Now you have
24 (visible) entries with standard vga text mode (80x25).  If that still
isn't enough I guess I'd try to use 80x50 text mode instead of
implementing multiple pages.

In any case this should go to a separate patch.
> if you are a floppy-based OS developer who is debugging something that
> could be reproduced only at bare metal (so no QEMU) - you may want to
> add a lot of different versions of your floppy to your coreboot build
> to reduce the amount of BIOS chip reflashes and just for your
>
> convenience.

Well, the syslinux bootloader comes with a memdisk utility, you can boot
from floppy images stored on your hard drive that way.  I would prefer
this over reflashing the BIOS all day long.
> Although a few USB numpads do have an Escape key (e.g. Motospeed
>
> K23), the majority of them do not - so it is impossible for them to
> open a boot menu by pressing Escape - simply because no such key.
> (That's why I added "/" character to the menukeys - every USB numpad
> has this key.)

Separate patch too.
> Also, in your patch version it's not possible to access
> TPM from a numpad because no t or m keys on it - so I had to add '-'
> character to the menukeys.

That is pointless unless you fix the tpm menu to support the numpad too.
And, of course, make that a separate patch.

cheers,
  Gerd

[Mike Banon]

On Thu, Apr 11, 2019 at 11:34 PM Gerd Hoffmann  wrote:
> Too many changes in a single patch.  Needs splitting up into
> one patch per change.

Good idea, thank you. I could try to split the numpad support/console
interface into a second patch. Although I provided a coreboot.rom QEMU
image with lots of floppies, which could have been used for a review
of this patch to see it in action :
> Here is a coreboot image for QEMU with these two patches applied and my

collection of wonderful and useful floppies added to popular the entries list:
https://github.com/mikebdp2/floparchive/blob/master/coreboot.rom?raw=true
Descriptions of the most prominent floppies could be found here:
http://dangerousprototypes.com/docs/Lenovo_G505S_hacking#Useful_floppies
> Run this coreboot.rom by executing this QEMU command (some floppies are 64bit)

qemu-system-x86_64 -L . -m 768 -localtime -vga vmware -net nic,model=rtl8139 \
-net user -soundhw ac97 -bios ./coreboot.rom -boot menu=on -serial stdio

On Thu, Apr 11, 2019 at 11:34 PM Gerd Hoffmann  wrote:
> Fixed in v2 -- back to single key mode, using letter keys for entry #10
> and beyond.

I see that you decided to continue it with "abcdef" instead of
suggested "qwerty". Interesting, what approach would be more
convenient for the end user... Also, what do you think about moving a
tpm menu key from "t" letter (which is weirdly situated in the middle
of keyboard) to "m" letter, which is at the end of letter rows? also t
= trusted - is an adjective, while m = module is a noun, so seems to
be more appropriate for the naming.

On Thu, Apr 11, 2019 at 11:34 PM Gerd Hoffmann  wrote:
> Even 10 entries not being enough comes up rarely enough.
> Now you have 24 (visible) entries with standard vga text mode (80x25).

Just 18-19 considering there could be a TPM and still want to show the
top instructions.

On Thu, Apr 11, 2019 at 11:34 PM Gerd Hoffmann  wrote:
> Well, the syslinux bootloader comes with a memdisk utility, you can boot
> from floppy images stored on your hard drive that way.  I would prefer
> this over reflashing the BIOS all day long.

There are floppy OS problems which couldn't be reproduced or debugged
this way. For example: Visopsys (interesting floppy OS with GUI for
disk partitioning) loads fine from a physical media, but currently
can't load as a virtual floppy from coreboot/SeaBIOS because of the
problems with ramdisk support. And there was a similar problem with
another interesting floppy OS - Fiwix - but we successfully debugged
and fixed it together with author.

On Thu, Apr 11, 2019 at 11:34 PM Gerd Hoffmann  wrote:
> > Also, in your patch version it's not possible to access
> > TPM from a numpad because no t or m keys on it - so I had to add '-'
> > character to the menukeys.
>
> That is pointless unless you fix the tpm menu to support the numpad too.

Sadly I don't have any devices with TPM so it is hard for me to debug
this part. The best thing I could do is to hack into SeaBIOS tpm code
to make it show the tpm menu entries stuff which does not exist, but
at the moment I don't even know how it looks like...

Best regards,
Mike Banon

[Gerd Hoffmann]

Hi,
> I see that you decided to continue it with "abcdef" instead of
> suggested "qwerty". Interesting, what approach would be more
> convenient for the end user...

I find lexical ordering more intuitive.  The char array allows easy
reordering though.

Also note that not all keyboards layouts have querty ordering.  Mine
(german) has quertz, and dvorak looks completely different.

On the other hand seabios doesn't support layouts other than us english
anyway ...
> Also, what do you think about moving a
> tpm menu key from "t" letter (which is weirdly situated in the middle
> of keyboard) to "m" letter, which is at the end of letter rows? also t
> = trusted - is an adjective, while m = module is a noun, so seems to
> be more appropriate for the naming.

But "module" is a rather generic word, I don't think this is useful.

cheers,
  Gerd

[Mike Banon]

Good day,
> Also note that not all keyboards layouts have querty ordering.  Mine
> (german) has quertz, and dvorak looks completely different.

Thank you, I forgot about this. Hope their scancodes are the same...

By the way: if we are extending the boot menu, at ./src/config.h we
need to increase the "#define BUILD_MAX_EXTDRIVE 16" to a higher
number, otherwise you will run into "add_drive" error messages while
having a lot of entries. I increased to "#define BUILD_MAX_EXTDRIVE
36".

Cheers,
Mike

On Fri, Apr 12, 2019 at 4:54 PM Gerd Hoffmann  wrote:
> Hi,
> > I see that you decided to continue it with "abcdef" instead of
> > suggested "qwerty". Interesting, what approach would be more
> > convenient for the end user...
>
> I find lexical ordering more intuitive.  The char array allows easy
> reordering though.
>
> Also note that not all keyboards layouts have querty ordering.  Mine
> (german) has quertz, and dvorak looks completely different.
>
> On the other hand seabios doesn't support layouts other than us english
> anyway ...
> > Also, what do you think about moving a
> > tpm menu key from "t" letter (which is weirdly situated in the middle
> > of keyboard) to "m" letter, which is at the end of letter rows? also t
> > = trusted - is an adjective, while m = module is a noun, so seems to
> > be more appropriate for the naming.
>
> But "module" is a rather generic word, I don't think this is useful.
>
> cheers,
>   Gerd