src/stacks.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
The comment above the yield() function suggests that yield()
allows interrupts for a short time. Currently this is only true
if seabios was built without CONFIG_THREADS or if yield() is
called from the main thread. Change the code to always call
check_irqs() in yield().
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.
}
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
src/stacks.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/stacks.c b/src/stacks.c
index 2fe1bfb..70e8283 100644
--- a/src/stacks.c
+++ b/src/stacks.c
@@ -623,9 +623,25 @@ yield(void)
return;
}
struct thread_info *cur = getCurThread();
- if (cur == &MainThread)
- // Permit irqs to fire
+ struct thread_info *mt = &MainThread;
+
+ // Permit irqs to fire
+ if (cur == mt) {
check_irqs();
+ } else {
+ asm volatile(
+ " pushl %%ebp\n" // backup %ebp
+ " movl %%esp, %%esi\n"
+ " movl (%%eax), %%esp\n" // switch to MainThread stack
+ " pushl %%esi\n" // backup current thread stack pointer
+ " calll %1\n" // check_irqs();
+ " popl %%esi\n"
+ " movl %%esi, %%esp\n" // switch back to current thread stack
+ " popl %%ebp\n" // restore %ebp
+ : "+a"(mt)
+ : "m"(check_irqs)
+ : "ebx", "ecx", "edx", "esi", "edi", "cc", "memory");
+ }
// Switch to the next thread
switch_next(cur);
--
2.26.2
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
On Fri, May 14, 2021 at 08:03:20PM +0200, Volker Rümelin wrote: > The comment above the yield() function suggests that yield() > allows interrupts for a short time. Currently this is only true > if seabios was built without CONFIG_THREADS or if yield() is > called from the main thread. Change the code to always call > check_irqs() in yield(). I'm confused about the failure scenario you describe here, as yield() really should always allow irqs to run prior to returning. When called from a "background thread" it will not directly enable irqs, but it will always cycle through all "threads" before returning. Thus the "main thread" should always be reached and when it runs yield() it will permit irqs to run. Am I missing something? -Kevin _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
> On Fri, May 14, 2021 at 08:03:20PM +0200, Volker Rümelin wrote: >> The comment above the yield() function suggests that yield() >> allows interrupts for a short time. Currently this is only true >> if seabios was built without CONFIG_THREADS or if yield() is >> called from the main thread. Change the code to always call >> check_irqs() in yield(). > I'm confused about the failure scenario you describe here, as yield() > really should always allow irqs to run prior to returning. When > called from a "background thread" it will not directly enable irqs, > but it will always cycle through all "threads" before returning. Thus > the "main thread" should always be reached and when it runs yield() it > will permit irqs to run. > > Am I missing something? Hi Kevin, the main thread calls run_thread() to create a background thread, adds it behind main thread in the thread list and immediately executes the background thread until the background thread calls yield(). All background threads in the thread list run in sequence and call yield() until the last background thread yields to main thread. The main thread has not reached wait_threads(), which means it will create another background thread and run it. This will repeat until all background threads are created. You can see until now the main thread did not call yield() but the background threads ran several times. The main thread calls yield() for the first time in wait_threads(). With best regards, Volker > -Kevin _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
On Thu, May 20, 2021 at 08:53:34PM +0200, Volker Rümelin wrote:
> > On Fri, May 14, 2021 at 08:03:20PM +0200, Volker Rümelin wrote:
> > > The comment above the yield() function suggests that yield()
> > > allows interrupts for a short time. Currently this is only true
> > > if seabios was built without CONFIG_THREADS or if yield() is
> > > called from the main thread. Change the code to always call
> > > check_irqs() in yield().
> > I'm confused about the failure scenario you describe here, as yield()
> > really should always allow irqs to run prior to returning. When
> > called from a "background thread" it will not directly enable irqs,
> > but it will always cycle through all "threads" before returning. Thus
> > the "main thread" should always be reached and when it runs yield() it
> > will permit irqs to run.
> >
> > Am I missing something?
>
> Hi Kevin,
>
> the main thread calls run_thread() to create a background thread, adds it
> behind main thread in the thread list and immediately executes the
> background thread until the background thread calls yield(). All background
> threads in the thread list run in sequence and call yield() until the last
> background thread yields to main thread. The main thread has not reached
> wait_threads(), which means it will create another background thread and run
> it. This will repeat until all background threads are created. You can see
> until now the main thread did not call yield() but the background threads
> ran several times. The main thread calls yield() for the first time in
> wait_threads().
Thanks. It does seem the code is not doing what was intended. Could
we add the check to the bottom of run_thread() though? Something
like:
if (cur == &MainThread)
// Permit irqs to fire
check_irqs();
-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
> On Thu, May 20, 2021 at 08:53:34PM +0200, Volker Rümelin wrote: >>> On Fri, May 14, 2021 at 08:03:20PM +0200, Volker Rümelin wrote: >>>> The comment above the yield() function suggests that yield() >>>> allows interrupts for a short time. Currently this is only true >>>> if seabios was built without CONFIG_THREADS or if yield() is >>>> called from the main thread. Change the code to always call >>>> check_irqs() in yield(). >>> I'm confused about the failure scenario you describe here, as yield() >>> really should always allow irqs to run prior to returning. When >>> called from a "background thread" it will not directly enable irqs, >>> but it will always cycle through all "threads" before returning. Thus >>> the "main thread" should always be reached and when it runs yield() it >>> will permit irqs to run. >>> >>> Am I missing something? >> Hi Kevin, >> >> the main thread calls run_thread() to create a background thread, adds it >> behind main thread in the thread list and immediately executes the >> background thread until the background thread calls yield(). All background >> threads in the thread list run in sequence and call yield() until the last >> background thread yields to main thread. The main thread has not reached >> wait_threads(), which means it will create another background thread and run >> it. This will repeat until all background threads are created. You can see >> until now the main thread did not call yield() but the background threads >> ran several times. The main thread calls yield() for the first time in >> wait_threads(). > Thanks. It does seem the code is not doing what was intended. Could > we add the check to the bottom of run_thread() though? Something > like: > > if (cur == &MainThread) > // Permit irqs to fire > check_irqs(); I think check_irqs() has to be called at the top of run_thread() before the new background thread gets called. I'll test. I can reliably reproduce this problem. With best regards, Volker _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
>> On Thu, May 20, 2021 at 08:53:34PM +0200, Volker Rümelin wrote: >>>> On Fri, May 14, 2021 at 08:03:20PM +0200, Volker Rümelin wrote: >>>>> The comment above the yield() function suggests that yield() >>>>> allows interrupts for a short time. Currently this is only true >>>>> if seabios was built without CONFIG_THREADS or if yield() is >>>>> called from the main thread. Change the code to always call >>>>> check_irqs() in yield(). >>>> I'm confused about the failure scenario you describe here, as yield() >>>> really should always allow irqs to run prior to returning. When >>>> called from a "background thread" it will not directly enable irqs, >>>> but it will always cycle through all "threads" before returning. Thus >>>> the "main thread" should always be reached and when it runs yield() it >>>> will permit irqs to run. >>>> >>>> Am I missing something? >>> Hi Kevin, >>> >>> the main thread calls run_thread() to create a background thread, >>> adds it >>> behind main thread in the thread list and immediately executes the >>> background thread until the background thread calls yield(). All >>> background >>> threads in the thread list run in sequence and call yield() until >>> the last >>> background thread yields to main thread. The main thread has not >>> reached >>> wait_threads(), which means it will create another background thread >>> and run >>> it. This will repeat until all background threads are created. You >>> can see >>> until now the main thread did not call yield() but the background >>> threads >>> ran several times. The main thread calls yield() for the first time in >>> wait_threads(). >> Thanks. It does seem the code is not doing what was intended. Could >> we add the check to the bottom of run_thread() though? Something >> like: >> >> if (cur == &MainThread) >> // Permit irqs to fire >> check_irqs(); > > I think check_irqs() has to be called at the top of run_thread() > before the new background thread gets called. I'll test. I can > reliably reproduce this problem. > You're right. Your code also works at the bottom of run_thread(). But I had to change either the assembly code or the constraints of the asm statement, because the constraints don't match what the assembly code does. I still would prefer to call check_irqs() just before the switch to the new background thread. This is consistent with the code in yield(). yield() calls check_irqs() and then switches to the next thread. I'll send a new patch with your suggested changes. With best regards, Volker _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
© 2016 - 2024 Red Hat, Inc.