arch/x86/sched.c | 32 +++++++++----------------------- include/x86/os.h | 19 +++++++++++-------- 2 files changed, 20 insertions(+), 31 deletions(-)
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(-)
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" )
--
2.35.3On 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
© 2016 - 2026 Red Hat, Inc.