Re: [Minios-devel] [PATCH 20/40] arm64: implement the __arch_switch_threads

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 08/11/17 09:26, Huang Shijie wrote:
> On Mon, Nov 06, 2017 at 02:25:55PM +0000, Julien Grall wrote:
> > Hi Shijie,
> >
> > On 03/11/17 03:11, Huang Shijie wrote:
> > > The __arch_switch_threads is used for switching the threads.
> > > This patch implements it, and defines the stack layout.
> > >
> > > Change-Id: I195df8a34ad4738c3e3e925e6d7ed1c46eece0c8
> > > Jira: ENTOS-247
> > > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > > ---
> > >    arch/arm/arm64/arm64.S | 59 
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >    include/arm/arm64/os.h |  3 +++
> > >    2 files changed, 62 insertions(+)
> > >
> > > diff --git a/arch/arm/arm64/arm64.S b/arch/arm/arm64/arm64.S
> > > index e16ef95..6dab875 100644
> > > --- a/arch/arm/arm64/arm64.S
> > > +++ b/arch/arm/arm64/arm64.S
> > > @@ -1,6 +1,7 @@
> > >    #include "asm.h"
> > >    #include <arm64/pagetable.h>
> > >    #include <xen/xen.h>
> > > +#include <arm64/os.h>
> > Can we try to have the header in alphabetical order?
> okay..
> > >    /* This macro will use the x0/x1/x2/x16 */
> > >    #define PRINT(_s)                              \
> > > @@ -461,3 +462,61 @@ el_invalid(el1_fiq, BAD_FIQ, 1);
> > >    el_invalid(el0_fiq, BAD_FIQ, 0);
> > >    el_invalid(el1_error, BAD_ERROR, 1);
> > >    el_invalid(el0_error, BAD_ERROR, 0);
> > > +
> > > +/*
> > > + * => x0 = &prev->sp
> > > + *    x1 = &next->sp
> > > + * <= switch to the next thread
> > > + *
> > > + *  The stack layout shows below:
> > > + *
> > > + *                    ---------------
> > > + *                   |      lr       |
> > > + *                   |---------------|
> > > + *                   |  User data    |
> > > + *                   |---------------|
> > > + *                   |      sp       |    -----------------
> > > + *                   |---------------|          |
> > > + *                   |      x29      |          |
> > > + *                   |---------------|          v
> > > + *                   |      x28      |
> > > + *                   |---------------|  CALLEE_SAVED_REGISTERS
> > > + *                   |     .....     |
> > > + *                   |---------------|          ^
> > > + *                   |      x20      |          |
> > > + *                   |---------------|          |
> > > + *    thread->sp --> |      x19      |    -----------------
> > > + *                   |---------------|
> > > + */
> > > +ENTRY(__arch_switch_threads)
> > > +    /* Store the callee-saved registers and lr(x30) */
> > > +    mov   x4, sp
> > > +
> > > +    ldr   x2, [x0]
> > > +    stp   x19, x20, [x2, #16 * 0]
> > > +    stp   x21, x22, [x2, #16 * 1]
> > > +    stp   x23, x24, [x2, #16 * 2]
> > > +    stp   x25, x26, [x2, #16 * 3]
> > > +    stp   x27, x28, [x2, #16 * 4]
> > > +    stp   x29, x4,  [x2, #16 * 5]
> > See my comments in patch #3 regarding [xN, ...].
> >
> > > +
> > > +    add   x2, x2, #(CALLEE_SAVED_REGISTERS * 8)
> > > +    add   x2, x2, #8             /* skip the user data */
> > > +    str   x30, [x2]
> > > +
> > > +    /* Restore the callee-saved registers and lr(x30) */
> > > +    ldr   x2, [x1]
> > > +    ldp   x19, x20, [x2, #16 * 0]
> > > +    ldp   x21, x22, [x2, #16 * 1]
> > > +    ldp   x23, x24, [x2, #16 * 2]
> > > +    ldp   x25, x26, [x2, #16 * 3]
> > > +    ldp   x27, x28, [x2, #16 * 4]
> > > +    ldp   x29, x4,  [x2, #16 * 5]
> > > +
> > > +    add   x2, x2, #(CALLEE_SAVED_REGISTERS * 8)
> > > +    add   x2, x2, #8             /* skip the user data */
> > > +    ldr   x30, [x2]
> > > +
> > > +    mov   sp,  x4
> > > +    ret
> > > +ENDPROC(__arch_switch_threads)
> > > diff --git a/include/arm/arm64/os.h b/include/arm/arm64/os.h
> > > index 9c36a2c..1ffd99b 100644
> > > --- a/include/arm/arm64/os.h
> > > +++ b/include/arm/arm64/os.h
> > > @@ -1,6 +1,7 @@
> > >    #ifndef _ARM64_OS_H_
> > >    #define _ARM64_OS_H_
> > > +#ifndef __ASSEMBLY__
> > This belongs to the patch introducing os.h
> It looks strange we add the __ASSEMBLY__ in that patch.
> That patch does not use the assembly language.
I am not convinced but whatever. We have bigger fish to fry...

However, any modification should be justified in the commit message.
In that case this looked completely random until now.

So make sure to update the commit message if you don't want to move
that.

Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel