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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Wed, Nov 08, 2017 at 11:00:19AM +0000, Julien Grall wrote:
> 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.
okay.

Thanks
Huang Shijie

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