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().
[1] https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg02152.html
Volker Rümelin (1): stacks: call check_irqs() in run_thread()
src/stacks.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
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
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 firecheck_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
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 firecheck_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
-
Kevin O'Connor -
Volker Rümelin