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

[Jürgen Groß <jgross@xxxxxxxx>]

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