| Message ID | 20240416071139.11083-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Mini-OS: add some macros for asm statements | expand |
On 16/04/2024 8:11 am, Juergen Gross wrote: > diff --git a/arch/x86/sched.c b/arch/x86/sched.c > index dabe6fd6..460dea2e 100644 > --- a/arch/x86/sched.c > +++ b/arch/x86/sched.c > @@ -119,20 +113,12 @@ struct thread* arch_create_thread(char *name, void (*function)(void *), > > void run_idle_thread(void) > { > - /* Switch stacks and run the thread */ > -#if defined(__i386__) > - __asm__ __volatile__("mov %0,%%esp\n\t" > - "push %1\n\t" > - "ret" > + /* Switch stacks and run the thread */ > + __asm__ __volatile__("mov %0,"ASM_SP"\n\t" > + "push %1\n\t" > + "ret" > :"=m" (idle_thread->sp) > - :"m" (idle_thread->ip)); > -#elif defined(__x86_64__) > - __asm__ __volatile__("mov %0,%%rsp\n\t" > - "push %1\n\t" > - "ret" > - :"=m" (idle_thread->sp) > - :"m" (idle_thread->ip)); > -#endif > + :"m" (idle_thread->ip)); I know you're only transforming the existing logic, but this is dodgy in several ways. First, PUSH/RET is more commonly spelt JMP. Second, sp is an input parameter not an output, so I'm pretty sure this only works by luck. 00000000000001ee <run_idle_thread>: 1ee: 55 push %rbp 1ef: 48 89 e5 mov %rsp,%rbp 1f2: 48 8b 05 00 00 00 00 mov 0x0(%rip),%rax # 1f9 <run_idle_thread+0xb> 1f9: 48 8b 15 00 00 00 00 mov 0x0(%rip),%rdx # 200 <run_idle_thread+0x12> 200: 48 8b 60 10 mov 0x10(%rax),%rsp 204: ff 72 18 pushq 0x18(%rdx) 207: c3 retq 208: 90 nop 209: 5d pop %rbp 20a: c3 retq This only works because the address constructed for sp's "output" is happens to be on a single-parameter instruction where's its good as an input too. Anyway, this is a much better way of writing it: asm volatile ("mov %[sp], %%"ASM_SP"\n\t" "jmp *%[ip]\n\t" : : [sp] "m" (idle_thread->sp), [ip] "m" (idle_thread->ip)); and also highlights that run_idle_thread() should be marked noreturn. > } > > unsigned long __local_irq_save(void) > diff --git a/include/x86/os.h b/include/x86/os.h > index ee34d784..485d90b8 100644 > --- a/include/x86/os.h > +++ b/include/x86/os.h > @@ -77,6 +77,17 @@ int arch_suspend(void); > void arch_post_suspend(int canceled); > void arch_fini(void); > > +#if defined(__i386__) > +#define __SZ "l" > +#define __REG "e" > +#else > +#define __SZ "q" > +#define __REG "r" > +#endif > + > +#define ASM_SP "%%"__REG"sp" The %% should be at the usage sites, not here. That way, you can use ASM_SP in e.g. file-scope asm where it's spelt with a single %. > +#define ASM_MOV "mov"__SZ There's no need for ASM_MOV. Regular plain mov (no suffix) will work in all the cases you're trying to use it, and makes the asm easier to read. Notice how run_idle_thread() is already like this. ~Andrew
diff --git a/arch/x86/sched.c b/arch/x86/sched.c index dabe6fd6..460dea2e 100644 --- a/arch/x86/sched.c +++ b/arch/x86/sched.c @@ -60,16 +60,10 @@ void dump_stack(struct thread *thread) unsigned long *bottom = (unsigned long *)(thread->stack + STACK_SIZE); unsigned long *pointer = (unsigned long *)thread->sp; int count; - if(thread == current) - { -#ifdef __i386__ - asm("movl %%esp,%0" - : "=r"(pointer)); -#else - asm("movq %%rsp,%0" - : "=r"(pointer)); -#endif - } + + if ( thread == current ) + asm(ASM_MOV" "ASM_SP",%0" : "=r"(pointer)); + printk("The stack for \"%s\"\n", thread->name); for(count = 0; count < 25 && pointer < bottom; count ++) { @@ -119,20 +113,12 @@ struct thread* arch_create_thread(char *name, void (*function)(void *), void run_idle_thread(void) { - /* Switch stacks and run the thread */ -#if defined(__i386__) - __asm__ __volatile__("mov %0,%%esp\n\t" - "push %1\n\t" - "ret" + /* Switch stacks and run the thread */ + __asm__ __volatile__("mov %0,"ASM_SP"\n\t" + "push %1\n\t" + "ret" :"=m" (idle_thread->sp) - :"m" (idle_thread->ip)); -#elif defined(__x86_64__) - __asm__ __volatile__("mov %0,%%rsp\n\t" - "push %1\n\t" - "ret" - :"=m" (idle_thread->sp) - :"m" (idle_thread->ip)); -#endif + :"m" (idle_thread->ip)); } unsigned long __local_irq_save(void) diff --git a/include/x86/os.h b/include/x86/os.h index ee34d784..485d90b8 100644 --- a/include/x86/os.h +++ b/include/x86/os.h @@ -77,6 +77,17 @@ int arch_suspend(void); void arch_post_suspend(int canceled); void arch_fini(void); +#if defined(__i386__) +#define __SZ "l" +#define __REG "e" +#else +#define __SZ "q" +#define __REG "r" +#endif + +#define ASM_SP "%%"__REG"sp" +#define ASM_MOV "mov"__SZ + #ifdef CONFIG_PARAVIRT /* @@ -141,14 +152,6 @@ do { \ #else -#if defined(__i386__) -#define __SZ "l" -#define __REG "e" -#else -#define __SZ "q" -#define __REG "r" -#endif - #define __cli() asm volatile ( "cli" : : : "memory" ) #define __sti() asm volatile ( "sti" : : : "memory" )
Instead of having #ifdefs sprinkled around in x86 code, add some macros defining constants for asm statements to address differences between 32- and 64-bit mode. Modify existing code to use those macros. Signed-off-by: Juergen Gross <jgross@suse.com> --- arch/x86/sched.c | 32 +++++++++----------------------- include/x86/os.h | 19 +++++++++++-------- 2 files changed, 20 insertions(+), 31 deletions(-)