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().
[1]https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg02152.html
Volker Rümelin (3): stacks: fix asm constraints in run_thread() stacks: call check_irqs() in run_thread() stacks: call check_irqs() after switch_next()
src/stacks.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
The variables data, func, thread and cur are input operands for the asm statement in run_thread(). The assembly code clobbers these inputs.
From the gcc documentation chapter Extended-Asm, Input Operands: "It is not possible to use clobbers to inform the compiler that the values in these inputs are changing. One common work-around is to tie the changing input variable to an output variable that never gets used."
Add unused output variables and fix the asm constraints in run_thread().
Signed-off-by: Volker Rümelin vr_qemu@t-online.de --- src/stacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/stacks.c b/src/stacks.c index 2fe1bfb..ef0aba1 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -565,6 +565,7 @@ run_thread(void (*func)(void*), void *data) thread->stackpos = (void*)thread + THREADSTACKSIZE; struct thread_info *cur = getCurThread(); hlist_add_after(&thread->node, &cur->node); + u32 unused_a, unused_b, unused_c, unused_d; asm volatile( // Start thread " pushl $1f\n" // store return pc @@ -576,14 +577,15 @@ run_thread(void (*func)(void*), void *data) // End thread " movl %%ebx, %%eax\n" // %eax = thread " movl 4(%%ebx), %%ebx\n" // %ebx = thread->node.next - " movl (%5), %%esp\n" // %esp = MainThread.stackpos - " calll %4\n" // call __end_thread(thread) + " movl (%9), %%esp\n" // %esp = MainThread.stackpos + " calll %8\n" // call __end_thread(thread) " movl -4(%%ebx), %%esp\n" // %esp = next->stackpos " popl %%ebp\n" // restore %ebp " retl\n" // restore pc "1:\n" - : "+a"(data), "+c"(func), "+b"(thread), "+d"(cur) - : "m"(*(u8*)__end_thread), "m"(MainThread) + : "=a"(unused_a), "=c"(unused_c), "=b"(unused_b), "=d"(unused_d) + : "0"(data), "1"(func), "2"(thread), "3"(cur), + "m"(*(u8*)__end_thread), "m"(MainThread) : "esi", "edi", "cc", "memory"); return;
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 | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/stacks.c b/src/stacks.c index ef0aba1..58c0317 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) @@ -587,6 +589,9 @@ run_thread(void (*func)(void*), void *data) : "0"(data), "1"(func), "2"(thread), "3"(cur), "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 58c0317..e090919 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -630,12 +630,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 Tue, Jun 01, 2021 at 09:06:54PM +0200, Volker Rümelin wrote:
The variables data, func, thread and cur are input operands for the asm statement in run_thread(). The assembly code clobbers these inputs.
From the gcc documentation chapter Extended-Asm, Input Operands: "It is not possible to use clobbers to inform the compiler that the values in these inputs are changing. One common work-around is to tie the changing input variable to an output variable that never gets used."
Thanks. However, this change doesn't look correct to me. The variables data, func, thread and cur were all *output* operands. They were output operands that use "+" to indicate that they also have inputs associated with them. That is, unless I'm missing something, the asm already followed the gcc documentation (use an output operand and don't use the results of it).
Did you observe a problem during runtime with the original code?
Add unused output variables and fix the asm constraints in run_thread().
Signed-off-by: Volker Rümelin vr_qemu@t-online.de
src/stacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/stacks.c b/src/stacks.c index 2fe1bfb..ef0aba1 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -565,6 +565,7 @@ run_thread(void (*func)(void*), void *data) thread->stackpos = (void*)thread + THREADSTACKSIZE; struct thread_info *cur = getCurThread(); hlist_add_after(&thread->node, &cur->node);
- u32 unused_a, unused_b, unused_c, unused_d; asm volatile( // Start thread " pushl $1f\n" // store return pc
@@ -576,14 +577,15 @@ run_thread(void (*func)(void*), void *data) // End thread " movl %%ebx, %%eax\n" // %eax = thread " movl 4(%%ebx), %%ebx\n" // %ebx = thread->node.next
" movl (%5), %%esp\n" // %esp = MainThread.stackpos" calll %4\n" // call __end_thread(thread)
" movl (%9), %%esp\n" // %esp = MainThread.stackpos" calll %8\n" // call __end_thread(thread) " movl -4(%%ebx), %%esp\n" // %esp = next->stackpos " popl %%ebp\n" // restore %ebp " retl\n" // restore pc "1:\n"
: "+a"(data), "+c"(func), "+b"(thread), "+d"(cur): "m"(*(u8*)__end_thread), "m"(MainThread)
: "=a"(unused_a), "=c"(unused_c), "=b"(unused_b), "=d"(unused_d): "0"(data), "1"(func), "2"(thread), "3"(cur),"m"(*(u8*)__end_thread), "m"(MainThread)
The original code made sure data was in eax, func in ecx, thread in ebx, and cur in edx. The altered code no longer enforces this association and I think that could introduce a bug.
Separately, the rest of the patch series looks good to me.
-Kevin
On Tue, Jun 01, 2021 at 09:06:54PM +0200, Volker Rümelin wrote:
The variables data, func, thread and cur are input operands for the asm statement in run_thread(). The assembly code clobbers these inputs.
From the gcc documentation chapter Extended-Asm, Input Operands:
"It is not possible to use clobbers to inform the compiler that the values in these inputs are changing. One common work-around is to tie the changing input variable to an output variable that never gets used."
Thanks. However, this change doesn't look correct to me. The variables data, func, thread and cur were all *output* operands. They were output operands that use "+" to indicate that they also have inputs associated with them. That is, unless I'm missing something, the asm already followed the gcc documentation (use an output operand and don't use the results of it).
Hi Kevin,
the "+" output constraint indicates that the assembly code uses the variable as input and updates the same variable.
The gcc manual does not say the compiler will not use the output operand result. It actually uses the updated variable.
Your code works because you never use the output variables.
Did you observe a problem during runtime with the original code?
My next patch does not work, because the compiler assumes edx is the updated variable cur at the end of the assembly code, but edx is actually clobbered. Just use a dprintf() to print cur before and after the asm statement to see I'm right. objdump -dS src/stacks.o will show the same.
Add unused output variables and fix the asm constraints in run_thread().
Signed-off-by: Volker Rümelinvr_qemu@t-online.de
src/stacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/stacks.c b/src/stacks.c index 2fe1bfb..ef0aba1 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -565,6 +565,7 @@ run_thread(void (*func)(void*), void *data) thread->stackpos = (void*)thread + THREADSTACKSIZE; struct thread_info *cur = getCurThread(); hlist_add_after(&thread->node, &cur->node);
- u32 unused_a, unused_b, unused_c, unused_d; asm volatile( // Start thread " pushl $1f\n" // store return pc
@@ -576,14 +577,15 @@ run_thread(void (*func)(void*), void *data) // End thread " movl %%ebx, %%eax\n" // %eax = thread " movl 4(%%ebx), %%ebx\n" // %ebx = thread->node.next
" movl (%5), %%esp\n" // %esp = MainThread.stackpos" calll %4\n" // call __end_thread(thread)
" movl (%9), %%esp\n" // %esp = MainThread.stackpos" calll %8\n" // call __end_thread(thread) " movl -4(%%ebx), %%esp\n" // %esp = next->stackpos " popl %%ebp\n" // restore %ebp " retl\n" // restore pc "1:\n"
: "+a"(data), "+c"(func), "+b"(thread), "+d"(cur): "m"(*(u8*)__end_thread), "m"(MainThread)
: "=a"(unused_a), "=c"(unused_c), "=b"(unused_b), "=d"(unused_d): "0"(data), "1"(func), "2"(thread), "3"(cur),"m"(*(u8*)__end_thread), "m"(MainThread)The original code made sure data was in eax, func in ecx, thread in ebx, and cur in edx. The altered code no longer enforces this association and I think that could introduce a bug.
The digit input constraints tie the variables to the correct register just like the old code. data, func, thread and cur are still in eax, ecx, ebx and edx at the beginning of the assembly code.
With best regards, Volker
Separately, the rest of the patch series looks good to me.
-Kevin
On Wed, Jun 02, 2021 at 09:25:54PM +0200, Volker Rümelin wrote:
On Tue, Jun 01, 2021 at 09:06:54PM +0200, Volker Rümelin wrote:
The variables data, func, thread and cur are input operands for the asm statement in run_thread(). The assembly code clobbers these inputs.
From the gcc documentation chapter Extended-Asm, Input Operands:
"It is not possible to use clobbers to inform the compiler that the values in these inputs are changing. One common work-around is to tie the changing input variable to an output variable that never gets used."
Thanks. However, this change doesn't look correct to me. The variables data, func, thread and cur were all *output* operands. They were output operands that use "+" to indicate that they also have inputs associated with them. That is, unless I'm missing something, the asm already followed the gcc documentation (use an output operand and don't use the results of it).
Hi Kevin,
the "+" output constraint indicates that the assembly code uses the variable as input and updates the same variable.
The gcc manual does not say the compiler will not use the output operand result. It actually uses the updated variable.
Your code works because you never use the output variables.
Did you observe a problem during runtime with the original code?
My next patch does not work, because the compiler assumes edx is the updated variable cur at the end of the assembly code, but edx is actually clobbered. Just use a dprintf() to print cur before and after the asm statement to see I'm right. objdump -dS src/stacks.o will show the same.
Okay, I think I understand the issue. The asm constrains aren't wrong, but "cur" is clobbered by the asm. I think it's probably simpler to use this in the new code:
if (getCurThread() == &MainThread) check_irqs();
And do something similar in yield(). That is, not save/restore getCurThread() during stack switches. If it helps, we can also rename "cur" to "edx" to make it more clear that the C variable is clobbered.
Add unused output variables and fix the asm constraints in run_thread().
Signed-off-by: Volker Rümelinvr_qemu@t-online.de
src/stacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/stacks.c b/src/stacks.c index 2fe1bfb..ef0aba1 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -565,6 +565,7 @@ run_thread(void (*func)(void*), void *data) thread->stackpos = (void*)thread + THREADSTACKSIZE; struct thread_info *cur = getCurThread(); hlist_add_after(&thread->node, &cur->node);
- u32 unused_a, unused_b, unused_c, unused_d; asm volatile( // Start thread " pushl $1f\n" // store return pc
@@ -576,14 +577,15 @@ run_thread(void (*func)(void*), void *data) // End thread " movl %%ebx, %%eax\n" // %eax = thread " movl 4(%%ebx), %%ebx\n" // %ebx = thread->node.next
" movl (%5), %%esp\n" // %esp = MainThread.stackpos" calll %4\n" // call __end_thread(thread)
" movl (%9), %%esp\n" // %esp = MainThread.stackpos" calll %8\n" // call __end_thread(thread) " movl -4(%%ebx), %%esp\n" // %esp = next->stackpos " popl %%ebp\n" // restore %ebp " retl\n" // restore pc "1:\n"
: "+a"(data), "+c"(func), "+b"(thread), "+d"(cur): "m"(*(u8*)__end_thread), "m"(MainThread)
: "=a"(unused_a), "=c"(unused_c), "=b"(unused_b), "=d"(unused_d): "0"(data), "1"(func), "2"(thread), "3"(cur),"m"(*(u8*)__end_thread), "m"(MainThread)The original code made sure data was in eax, func in ecx, thread in ebx, and cur in edx. The altered code no longer enforces this association and I think that could introduce a bug.
The digit input constraints tie the variables to the correct register just like the old code. data, func, thread and cur are still in eax, ecx, ebx and edx at the beginning of the assembly code.
Sorry, you are correct, I missed the association. That said, I don't think we need to tell gcc to save/restore the original values during the stack switch as they can be easily recalculated.
Cheers, -Kevin
On Wed, Jun 02, 2021 at 09:25:54PM +0200, Volker Rümelin wrote:
On Tue, Jun 01, 2021 at 09:06:54PM +0200, Volker Rümelin wrote:
The variables data, func, thread and cur are input operands for the asm statement in run_thread(). The assembly code clobbers these inputs.
From the gcc documentation chapter Extended-Asm, Input Operands:
"It is not possible to use clobbers to inform the compiler that the values in these inputs are changing. One common work-around is to tie the changing input variable to an output variable that never gets used."
Thanks. However, this change doesn't look correct to me. The variables data, func, thread and cur were all *output* operands. They were output operands that use "+" to indicate that they also have inputs associated with them. That is, unless I'm missing something, the asm already followed the gcc documentation (use an output operand and don't use the results of it).
Hi Kevin,
the "+" output constraint indicates that the assembly code uses the variable as input and updates the same variable.
The gcc manual does not say the compiler will not use the output operand result. It actually uses the updated variable.
Your code works because you never use the output variables.
Did you observe a problem during runtime with the original code?
My next patch does not work, because the compiler assumes edx is the updated variable cur at the end of the assembly code, but edx is actually clobbered. Just use a dprintf() to print cur before and after the asm statement to see I'm right. objdump -dS src/stacks.o will show the same.
Okay, I think I understand the issue. The asm constrains aren't wrong, but "cur" is clobbered by the asm. I think it's probably simpler to use this in the new code:
if (getCurThread() == &MainThread) check_irqs();And do something similar in yield(). That is, not save/restore getCurThread() during stack switches. If it helps, we can also rename "cur" to "edx" to make it more clear that the C variable is clobbered.
The compiler on my computer decides it's better to save/restore getCurThread() during stack switches. It optimizes the second getCurThread() away. My compiler also applies that optimization to yield().
I will send version 4 patch series where I drop patch 1/3, rename cur to edx in patch 2/2 and keep patch 3/3.
With best regards, Volker
Add unused output variables and fix the asm constraints in run_thread().
Signed-off-by: Volker Rümelinvr_qemu@t-online.de
src/stacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/stacks.c b/src/stacks.c index 2fe1bfb..ef0aba1 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -565,6 +565,7 @@ run_thread(void (*func)(void*), void *data) thread->stackpos = (void*)thread + THREADSTACKSIZE; struct thread_info *cur = getCurThread(); hlist_add_after(&thread->node, &cur->node);
- u32 unused_a, unused_b, unused_c, unused_d; asm volatile( // Start thread " pushl $1f\n" // store return pc
@@ -576,14 +577,15 @@ run_thread(void (*func)(void*), void *data) // End thread " movl %%ebx, %%eax\n" // %eax = thread " movl 4(%%ebx), %%ebx\n" // %ebx = thread->node.next
" movl (%5), %%esp\n" // %esp = MainThread.stackpos" calll %4\n" // call __end_thread(thread)
" movl (%9), %%esp\n" // %esp = MainThread.stackpos" calll %8\n" // call __end_thread(thread) " movl -4(%%ebx), %%esp\n" // %esp = next->stackpos " popl %%ebp\n" // restore %ebp " retl\n" // restore pc "1:\n"
: "+a"(data), "+c"(func), "+b"(thread), "+d"(cur): "m"(*(u8*)__end_thread), "m"(MainThread)
: "=a"(unused_a), "=c"(unused_c), "=b"(unused_b), "=d"(unused_d): "0"(data), "1"(func), "2"(thread), "3"(cur),"m"(*(u8*)__end_thread), "m"(MainThread)The original code made sure data was in eax, func in ecx, thread in ebx, and cur in edx. The altered code no longer enforces this association and I think that could introduce a bug.
The digit input constraints tie the variables to the correct register just like the old code. data, func, thread and cur are still in eax, ecx, ebx and edx at the beginning of the assembly code.
Sorry, you are correct, I missed the association. That said, I don't think we need to tell gcc to save/restore the original values during the stack switch as they can be easily recalculated.
-
Kevin O'Connor -
Volker Rümelin