[SeaBIOS] Re: [PATCH] stacks: call check_irqs() in run_thread()

[Kevin O'Connor]

On Sun, May 23, 2021 at 09:07:46AM +0200, Volker Rümelin wrote:
> The comment above the yield() function suggests that yield()
> allows interrupts for a short time. But yield() only briefly
> enables interrupts if SeaBIOS was built without CONFIG_THREADS
> or if yield() is called from the main thread. In order to
> guarantee that interrupts were enabled once before yield()
> returns in a background thread, the main thread must call
> check_irqs() before every thread switch. The function
> run_thread() also switches threads, but the call to check_irqs()
> was forgotten. Add the missing check_irqs() call.
>
> This fixes PS/2 keyboard initialization failures.
>
> The code in src/hw/ps2port.c relies on yield() to briefly enable
> interrupts. There is a comment above the yield() function in
> __ps2_command(), where the author left a remark why the call to
> yield() is actually needed.
>
> Here is one of the call sequences leading to a PS/2 keyboard
> initialization failure.
>
> ps2_keyboard_setup()
>   |
>   ret = i8042_command(I8042_CMD_KBD_DISABLE, NULL);
>   # This command will register an interrupt if the PS/2 device
>   # controller raises interrupts for replies to a controller
>   # command.
>   |
>   ret = ps2_kbd_command(ATKBD_CMD_RESET_BAT, param);
>     |
>     ps2_command(0, command, param);
>       |
>       ret = __ps2_command(aux, command, param);
>         |
>         // Flush any interrupts already pending.
>         yield();
>         # yield() doesn't flush interrupts if the main thread
>         # hasn't reached wait_threads().
>         |
>         ret = ps2_sendbyte(aux, command, 1000);
>         # Reset the PS/2 keyboard controller and wait for
>         # PS2_RET_ACK.
>         |
>         ret = ps2_recvbyte(aux, 0, 4000);
>           |
>           for (;;) {
>             |
>             status = inb(PORT_PS2_STATUS);
>             # I8042_STR_OBF isn't set because the keyboard self
>             # test reply is still on wire.
>             |
>             yield();
>             # After a few yield()s the keyboard interrupt fires
>             # and clears the I8042_STR_OBF status bit. If the
>             # keyboard self test reply arrives before the
>             # interrupt fires the keyboard reply is lost and
>             # ps2_recvbyte() returns after the timeout.
>           }
>
> Suggested-by: Kevin O'Connor 
> Signed-off-by: Volker Rümelin
>
> src/stacks.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/stacks.c b/src/stacks.c
> index 2fe1bfb..641b13f 100644
> --- a/src/stacks.c
> +++ b/src/stacks.c
> @@ -549,7 +549,10 @@ __end_thread(struct thread_info *old)
>          dprintf(1, "All threads complete.\n");
>  }
>
> -// Create a new thread and start executing 'func' in it.
> +void VISIBLE16 check_irqs(void);
>
> +// Create a new thread. Briefly permit irqs to occur and start
> +// executing 'func' in the new thread.
>  void
>  run_thread(void (*func)(void*), void *data)
>  {
> @@ -565,6 +568,10 @@ run_thread(void (*func)(void*), void *data)
>      thread->stackpos = (void*)thread + THREADSTACKSIZE;
>      struct thread_info *cur = getCurThread();
>      hlist_add_after(&thread->node, &cur->node);
> if (cur == &MainThread)
>    // Permit irqs to fire
>    check_irqs();

Thanks.  I'm concerned about running check_irqs() before starting the
thread, because I'm not sure if there are code locations that are
expecting an atomic handoff between calling thread and called thread.
The current "threading" scheme uses cooperative multi-tasking and
calling before the thread would effectively add a yield() in new
locations.  (In contrast, I think we can be confident that adding a
yield() after the run_thread() is okay because all other "threads" are
already assured to have run by that point.)  If there is a concern
with parity wrt yield(), we should be able to move the check_irqs()
call in yield() to after the code returns to the main thread.

Cheers,
-Kevin
> asm volatile(
>        // Start thread
>        "  pushl $1f\n"                 // store return pc