[SeaBIOS] Re: [PATCH v3 1/3] stacks: fix asm constraints in run_thread()
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ümelin > > > --- > > > 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 ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH v3 1/3] stacks: fix asm constraints in run_thread()
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ümelin --- 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 ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH v3 1/3] stacks: fix asm constraints in run_thread()
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 > --- > 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 ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH v3] Increate BUILD_MIN_BIOSTABLE for large roms
On Mon, May 31, 2021 at 07:55:28AM +0200, Gerd Hoffmann wrote: > BUILD_MIN_BIOSTABLE reserves space in the f-segment. Some data > structures -- for example disk drives known to seabios -- must be > stored there, so the space available here limits the number of > devices seabios is able to manage. > > This patch sets BUILD_MIN_BIOSTABLE to 8k for bios images being 256k or > larger in size. 32bit code is moved off in that case, so we have more > room in the f-segment then. Thanks. Looks good to me. -Kevin ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
