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 <kevin@koconnor.net>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 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();
+
     asm volatile(
         // Start thread
         "  pushl $1f\n"                 // store return pc
-- 
2.26.2

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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 <kevin@koconnor.net>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  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
> -- 
> 2.26.2
> 
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

Am 26.05.21 um 15:55 schrieb Kevin O'Connor:
>> 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.

My only concern with calling check_irqs() after run_thread() and
calling check_irqs() before switch_next() is that I can construct
the following call sequence in the main thread

run_thread(background_thread_1, NULL);
yield();
run_thread(background_thread_2, NULL);

where with the call to yield() and the following run_thread(),
the background_thread_1 runs twice without a check_irqs() call
in between. But I don't think there is such a code sequence in
SeaBIOS.

It will take a few days before I can send a new patch.

With best regards,
Volker

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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org