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(-)
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.
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_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 2fe1bfb..df32325 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -549,6 +549,8 @@ __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) @@ -564,6 +566,7 @@ 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 @@ -582,9 +585,12 @@ 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:
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 df32325..4c8e5df 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -629,12 +629,11 @@ 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
On Fri, Jun 04, 2021 at 07:59:27PM +0200, Volker Rümelin wrote:
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.
Thanks, the series looks good to me.
-Kevin
On Fri, Jun 04, 2021 at 07:59:27PM +0200, Volker Rümelin wrote:
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.
Thanks, I committed this series.
-Kevin
-
Kevin O'Connor -
Volker Rümelin