Menu time-out missing when GRUB is loaded quickly and `at_keyboard`

Menu time-out missing when GRUB is loaded quickly and `at_keyboard`

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

Dear GRUB folks,


When the module `at_keyboard` is directly into the GRUB image 
(`--modules`), and GRUB is loaded really quickly, then the timer, which, 
after counting down to 0 (`GRUB_TIMEOUT`), starts the selected entry, is 
not shown.

I noticed this issue on the ASRock E350M1 with coreboot and a small GRUB 
payload, and a PS/2 keyboard connected. Due to the missing time-out, I 
manually have to confirm the selected entry. By chance, the keyboard 
wasn’t connected setting up the system somewhere else, and that made it 
work as expected. So, it looks like, it’s related to `at_keyboard`, and 
some race, because the bigger default GRUB payload also does not show 
the problem.

Luckily, it’s easily reproducible with GRUB’s standard 
`default_payload.elf` and QEMU.

Please find the instructions below to reproduce the issue.

     $ git clone https://review.coreboot.org/coreboot
     $ cd coreboot
     $ # save attached grub.cfg in the directory
     $ util/scripts/config -e PAYLOAD_GRUB2
     grep: .config: Datei oder Verzeichnis nicht gefunden
     $ util/scripts/config -e GRUB2_INCLUDE_RUNTIME_CONFIG_FILE
     $ util/scripts/config -e GRUB2_MASTER
     $ util/scripts/config -e CONFIG_COREBOOT_ROMSIZE_KB_2048 # default 
of 512 KB too small for GRUB payload
     $ util/scripts/config -e ANY_TOOLCHAIN
     $ # or: make crossgcc-i386 CPUS=`nproc`
     $ make olddefconfig
     $ make -j`nproc`
     $ qemu-system-x86_64 --version
      QEMU emulator version 3.1.0 (Debian 1:3.1+dfsg-2+b1)
      Copyright (c) 2003-2018 Fabrice Bellard and the QEMU Project 
developers
     $ qemu-system-x86_64 -M pc -bios build/coreboot.rom -serial stdio

*No* time-out is shown. Telling QEMU to emulate a USB keyboard, 
indirectly disabling the PS/2 keyboard, the time-out *is* shown.

     $ qemu-system-x86_64 -M pc -bios build/coreboot.rom -serial stdio \
     -usb -device usb-kbd

Not including `at_keyboard` directly in GRUB’s “core image”, modules 
loaded automatically, the time-out is also shown.

`set debug=at_keyb` did not show anything interesting.

Can you reproduce that, and see what the problem is?


Kind regards,

Paul


PS: Please find the instructions to build GRUB as a coreboot payload 
done by coreboot’s Kconfig system automatically, and how put it into the 
coreboot filesystem CBFS.

```
$ git clone https://git.savannah.gnu.org/git/grub.git/
$ cd grub
$ git describe --tags --dirty
grub-2.02-238-ga791dc0e3
$ ./autogen.sh
$ ./configure --with-platform=coreboot
$ make default_payload.elf # this includes `at_keyboard`
$ cp default_payload.elf my/coreboot/folder/
$ cd my/coreboot/folder/
$ build/cbfstool build/coreboot.rom print
$ build/cbfstool build/coreboot.rom remove -n fallback/payload
$ build/cbfstool build/coreboot.rom add-payload -f payload.elf -n 
fallback/payload -c lzma
```


Kind regards,

Paul


[-- Attachment #2: grub.cfg --]
[-- Type: text/plain, Size: 58 bytes --]

set timeout=5
menuentry 'Power off machine' {
     halt
}

[PATCH] normal/menu: Do not treat error values as key presses

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

Date: Sat, 4 Feb 2019 22:39:37 +0100

Some terminals, like `grub-core/term/at_keyboard.c`, return `-1` in case
they are not ready yet.

      if (! KEYBOARD_ISREADY (grub_inb (KEYBOARD_REG_STATUS)))
        return -1;

Currently, that is treated as a key press, and the menu time-out is
cancelled/cleared. This is unwanted, as the boot is stopped and the user
manually has to select a menu entry. Therefore, adapt the condition to
require the key value also to be greater than 0.

`GRUB_TERM_NO_KEY` is defined as 0, so the condition could be collapsed
to greater or equal than (≥) 0, but the compiler will probably do that
for us anyway, so keep the cases separate for clarity.

This is tested with coreboot, the GRUB default payload, and the
configuration file `grub.cfg` below.

For GRUB:

    $ ./autogen.sh
    $ ./configure --with-platform=coreboot
    $ make -j`nproc`
    $ make default_payload.elf

For coreboot:

    $ more grub.cfg
    serial --unit 0 --speed 115200
    set timeout=5

    menuentry 'halt' {
        halt
    }
    $ build/cbfstool build/coreboot.rom add-payload \
        -f /dev/shm/grub/default_payload.elf -n fallback/payload -c lzma
    $ build/cbfstool build/coreboot.rom add -f grub.cfg -n etc/grub.cfg -t raw
    $ qemu-system-x86_64 --version
    QEMU emulator version 3.1.0 (Debian 1:3.1+dfsg-2+b1)
    Copyright (c) 2003-2018 Fabrice Bellard and the QEMU Project developers
    $ qemu-system-x86_64 -M pc -bios build/coreboot.rom -serial stdio -nic none

Currently, the time-out is cancelled/cleared. With the commit, it is not.

With a small GRUB payload, this the problem is also reproducible on the
ASRock E350M1.

Link: http://lists.gnu.org/archive/html/grub-devel/2019-01/msg00037.html
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 grub-core/normal/menu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index e7a83c2d6..68159e771 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -698,7 +698,8 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
 
       c = grub_getkey_noblock ();
 
-      if (c != GRUB_TERM_NO_KEY)
+      /* Negative values are returned on error */
+      if ((c != GRUB_TERM_NO_KEY) && (c > 0))
 	{
 	  if (timeout >= 0)
 	    {
-- 
2.20.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH] normal/menu: Do not treat error values as key presses

[Daniel Kiper]

On Tue, Feb 05, 2019 at 05:29:13PM +0100, Paul Menzel wrote:
> Date: Sat, 4 Feb 2019 22:39:37 +0100
>
> Some terminals, like `grub-core/term/at_keyboard.c`, return `-1` in case
> they are not ready yet.
>
>       if (! KEYBOARD_ISREADY (grub_inb (KEYBOARD_REG_STATUS)))
>         return -1;
>
> Currently, that is treated as a key press, and the menu time-out is
> cancelled/cleared. This is unwanted, as the boot is stopped and the user
> manually has to select a menu entry. Therefore, adapt the condition to
> require the key value also to be greater than 0.
>
> `GRUB_TERM_NO_KEY` is defined as 0, so the condition could be collapsed
> to greater or equal than (≥) 0, but the compiler will probably do that
> for us anyway, so keep the cases separate for clarity.
>
> This is tested with coreboot, the GRUB default payload, and the
> configuration file `grub.cfg` below.
>
> For GRUB:
>
>     $ ./autogen.sh
>     $ ./configure --with-platform=coreboot
>     $ make -j`nproc`
>     $ make default_payload.elf
>
> For coreboot:
>
>     $ more grub.cfg
>     serial --unit 0 --speed 115200
>     set timeout=5
>
>     menuentry 'halt' {
>         halt
>     }
>     $ build/cbfstool build/coreboot.rom add-payload \
>         -f /dev/shm/grub/default_payload.elf -n fallback/payload -c lzma
>     $ build/cbfstool build/coreboot.rom add -f grub.cfg -n etc/grub.cfg -t raw
>     $ qemu-system-x86_64 --version
>     QEMU emulator version 3.1.0 (Debian 1:3.1+dfsg-2+b1)
>     Copyright (c) 2003-2018 Fabrice Bellard and the QEMU Project developers
>     $ qemu-system-x86_64 -M pc -bios build/coreboot.rom -serial stdio -nic none
>
> Currently, the time-out is cancelled/cleared. With the commit, it is not.
>
> With a small GRUB payload, this the problem is also reproducible on the
> ASRock E350M1.
>
> Link: http://lists.gnu.org/archive/html/grub-devel/2019-01/msg00037.html
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel