| Message ID | 20240719155701.18856-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Mini-OS: add some macros for asm statements | expand |
On 19.07.2024 17:57, Juergen Gross wrote: > --- 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("mov %%"ASM_SP",%0" : "=r"(pointer)); As you switch the if() to Xen style, why not also the asm()? Irrespective of which precise style is meant to be used, the last closing double quote likely also wants to be followed by a blank? > @@ -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" > - :"=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 > + /* Switch stacks and run the thread */ > + asm volatile ("mov %[sp], %%"ASM_SP"\n\t" > + "jmp *%[ip]\n\t" > + : > + : [sp] "m" (idle_thread->sp), > + [ip] "m" (idle_thread->ip)); > } Here instead you look to be switching to Linux style. Was that intended? As an aside, I think the construct is slightly problematic: In principle the compiler could make a copy of idle_thread->ip on the stack. (It won't normally, for code efficiency reasons.) That would break with the earlier change of the stack pointer. Using an "r" constraint would perhaps be better there. Yet if so wanted, that would certainly be a separate change. With the adjustments (or respective clarifications as to style intentions), which I'd be fine making while committing so long as you agree: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 22.07.24 09:15, Jan Beulich wrote: > On 19.07.2024 17:57, Juergen Gross wrote: >> --- 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("mov %%"ASM_SP",%0" : "=r"(pointer)); > > As you switch the if() to Xen style, why not also the asm()? Irrespective of > which precise style is meant to be used, the last closing double quote likely > also wants to be followed by a blank? Yes, indeed. > >> @@ -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" >> - :"=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 >> + /* Switch stacks and run the thread */ >> + asm volatile ("mov %[sp], %%"ASM_SP"\n\t" >> + "jmp *%[ip]\n\t" >> + : >> + : [sp] "m" (idle_thread->sp), >> + [ip] "m" (idle_thread->ip)); >> } > > Here instead you look to be switching to Linux style. Was that intended? No, not really. I took Andrew's suggestion verbatim. > As an aside, I think the construct is slightly problematic: In principle > the compiler could make a copy of idle_thread->ip on the stack. (It > won't normally, for code efficiency reasons.) That would break with the > earlier change of the stack pointer. Using an "r" constraint would > perhaps be better there. Yet if so wanted, that would certainly be a > separate change. > > With the adjustments (or respective clarifications as to style intentions), > which I'd be fine making while committing so long as you agree: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. I agree with the suggested changes (to spell it out explicitly: I meant to use Xen coding style, as this is the Mini-OS style, too). Juergen
diff --git a/arch/x86/sched.c b/arch/x86/sched.c index dabe6fd6..42805f9f 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("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" - :"=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 + /* Switch stacks and run the thread */ + asm volatile ("mov %[sp], %%"ASM_SP"\n\t" + "jmp *%[ip]\n\t" + : + : [sp] "m" (idle_thread->sp), + [ip] "m" (idle_thread->ip)); } unsigned long __local_irq_save(void) diff --git a/include/x86/os.h b/include/x86/os.h index ee34d784..0095be13 100644 --- a/include/x86/os.h +++ b/include/x86/os.h @@ -61,6 +61,16 @@ #define TRAP_deferred_nmi 31 #define TRAP_xen_callback 32 +#if defined(__i386__) +#define __SZ "l" +#define __REG "e" +#else +#define __SZ "q" +#define __REG "r" +#endif + +#define ASM_SP __REG"sp" + /* Everything below this point is not included by assembler (.S) files. */ #ifndef __ASSEMBLY__ @@ -141,14 +151,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. While at it convert the assembler parts of run_idle_thread() to a more sane variant. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - addressed comments by Andrew Cooper --- arch/x86/sched.c | 34 ++++++++++------------------------ include/x86/os.h | 18 ++++++++++-------- 2 files changed, 20 insertions(+), 32 deletions(-)