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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>

Jan