Series comparison

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 XXXXXXX..XXXXXXX 100644
--- a/src/stacks.c
+++ b/src/stacks.c
@@ -XXX,XX +XXX,XX @@ __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)
 {
@@ -XXX,XX +XXX,XX @@ 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

Hi,

I wrote a few patches for qemu [1] and patch 10/11 exposes a bug
in seabios.

Sometimes the PS/2 keyboard initialization fails and seabios
doesn't detect the keyboard. This patch fixes the PS/2 keyboard
initialization.

I wouldn't be surprised if my patch also fixes PS/2 keyboard
initialization problems on bare metal. A few bug reports look
very similar to what I see on my computer with qemu.

With best regards,
Volker

v2:
Kevin suggested to call check_irqs() in run_thread().

v3:
Add a new patch to fix the asm constraints in run_thread(). The
changes are required for the next patch.

Kevin suggested to call check_irqs() after the new thread ran in
run_thread().
Fixed an error in the commit message.

Call check_irqs() after switch_next() in yield().

v4:
Drop the patch to fix the asm constraints in run_thread().

Rename cur to edx in the asm constraints in run_thread() to
make it clear that edx is clobbered after the asm statement.

Volker Rümelin (2):
   stacks: call check_irqs() in run_thread()
   stacks: call check_irqs() after switch_next()

src/stacks.c | 13 +++++++++----
  1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.26.2

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

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 the interrupts were enabled once before yield()
returns in a background thread, the main thread must call
check_irqs() before or after 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.

Here is one of the call sequences leading to a PS/2 keyboard
initialization failure.

ps2_keyboard_setup()
  |
  ret = i8042_command(I8042_CMD_CTL_TEST, param);
  # 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.
          }

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 src/stacks.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/stacks.c b/src/stacks.c
index XXXXXXX..XXXXXXX 100644
--- a/src/stacks.c
+++ b/src/stacks.c
@@ -XXX,XX +XXX,XX @@ __end_thread(struct thread_info *old)
         dprintf(1, "All threads complete.\n");
 }
 
+void VISIBLE16 check_irqs(void);
+
 // Create a new thread and start executing 'func' in it.
 void
 run_thread(void (*func)(void*), void *data)
@@ -XXX,XX +XXX,XX @@ run_thread(void (*func)(void*), void *data)
     dprintf(DEBUG_thread, "/%08x\\ Start thread\n", (u32)thread);
     thread->stackpos = (void*)thread + THREADSTACKSIZE;
     struct thread_info *cur = getCurThread();
+    struct thread_info *edx = cur;
     hlist_add_after(&thread->node, &cur->node);
     asm volatile(
         // Start thread
@@ -XXX,XX +XXX,XX @@ run_thread(void (*func)(void*), void *data)
         "  popl %%ebp\n"                // restore %ebp
         "  retl\n"                      // restore pc
         "1:\n"
-        : "+a"(data), "+c"(func), "+b"(thread), "+d"(cur)
+        : "+a"(data), "+c"(func), "+b"(thread), "+d"(edx)
         : "m"(*(u8*)__end_thread), "m"(MainThread)
         : "esi", "edi", "cc", "memory");
+    if (cur == &MainThread)
+        // Permit irqs to fire
+        check_irqs();
     return;
 
 fail:
-- 
2.26.2

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

In function run_thread() the function check_irqs() gets called
after the thread switch for atomic handoff reasons. In yield()
it's the other way round.

If check_irqs() is called after run_thread() and check_irqs()
is called before switch_next() in yield(), it can happen in a
constructed case that a background thread runs twice without
a check_irqs() call in between. Call check_irqs() after
switch_next() in yield() to prevent this.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 src/stacks.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/stacks.c b/src/stacks.c
index XXXXXXX..XXXXXXX 100644
--- a/src/stacks.c
+++ b/src/stacks.c
@@ -XXX,XX +XXX,XX @@ yield(void)
         return;
     }
     struct thread_info *cur = getCurThread();
+    // Switch to the next thread
+    switch_next(cur);
     if (cur == &MainThread)
         // Permit irqs to fire
         check_irqs();
-
-    // Switch to the next thread
-    switch_next(cur);
 }
 
 void VISIBLE16
-- 
2.26.2

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