Re: [PATCH] Mini-OS: add some macros for asm statements

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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