Re: [Xen-devel] [PATCH 02/14] xen: arm: tweak arm64 stack frame layout

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-04-08 at 14:31 +0100, Stefano Stabellini wrote:
> On Tue, 12 Mar 2013, Ian Campbell wrote:
> > From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > Correct definition of UREGS_kernel_sizeof and use it.
> > 
> > Correct adjustment of stack on entry and exit.
> > 
> > Add 64-bit versions of the build time checks for stack pointer alignment
> > correctness when pushing the stack frames.
> > 
> > Lastly, correct the padding in the stack frames to properly align the inner 
> > and
> > outer frames and also avoid an unnecessary 64bit padding field.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/arm64/asm-offsets.c      |    2 +-
> >  xen/arch/arm/arm64/entry.S            |    9 +++++----
> >  xen/arch/arm/domain.c                 |    2 ++
> >  xen/arch/arm/traps.c                  |    7 +++++++
> >  xen/include/asm-arm/arm64/processor.h |    7 +++----
> >  5 files changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/arch/arm/arm64/asm-offsets.c 
> > b/xen/arch/arm/arm64/asm-offsets.c
> > index 7949e3e..7544082 100644
> > --- a/xen/arch/arm/arm64/asm-offsets.c
> > +++ b/xen/arch/arm/arm64/asm-offsets.c
> > @@ -39,7 +39,7 @@ void __dummy__(void)
> >     OFFSET(UREGS_SP_el1, struct cpu_user_regs, sp_el1);
> >     OFFSET(UREGS_ELR_el1, struct cpu_user_regs, elr_el1);
> >  
> > -   OFFSET(UREGS_kernel_sizeof, struct cpu_user_regs, cpsr);
> > +   OFFSET(UREGS_kernel_sizeof, struct cpu_user_regs, spsr_el1);
> >     DEFINE(UREGS_user_sizeof, sizeof(struct cpu_user_regs));
> >     BLANK();
> >  
> > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > index 9d38088..a5fd608 100644
> > --- a/xen/arch/arm/arm64/entry.S
> > +++ b/xen/arch/arm/arm64/entry.S
> > @@ -34,7 +34,7 @@ lr      .req    x30             // link register
> >          mrs     x22, SP_el0
> >          str     x22, [x21]
> >  
> > -        add     x21, sp, #UREGS_ELR_el1
> > +        add     x21, sp, #UREGS_SP_el1
> >          mrs     x22, SP_el1
> >          mrs     x23, ELR_el1
> >          stp     x22, x23, [x21]
> 
> Honestly I think it would be much cleaner and easier to read if we saved
> sp_el1 and elr_el1 separately.

I think stp is supposed to be faster, but I'm running on a model so I'm
not sure. Also there are weird quadword alignment constraints on the
stack and stp is atomic from that PoV.

> 
> 
> > @@ -59,7 +59,7 @@ lr      .req    x30             // link register
> >   * Save state on entry to hypervisor
> >   */
> >          .macro  entry, hyp, compat
> > -        sub     sp, sp, #(UREGS_SPSR_el1 - UREGS_SP)
> > +        sub     sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
> >          push    x28, x29
> >          push    x26, x27
> >          push    x24, x25
> > @@ -78,7 +78,7 @@ lr      .req    x30             // link register
> >  
> >          .if \hyp == 1        /* Hypervisor mode */
> >  
> > -        add     x21, sp, #(UREGS_X0 - UREGS_SP)
> > +        add     x21, sp, #UREGS_kernel_sizeof
> >  
> >          .else                /* Guest mode */
> >  
> > @@ -213,7 +213,8 @@ ENTRY(return_to_hypervisor)
> >          pop     x26, x27
> >          pop     x28, x29
> >  
> > -        ldr     lr, [sp], #(UREGS_SPSR_el1 - UREGS_SP)
> > +        ldr     lr, [sp], #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR 
> > */
> > +
> >          eret
> >  
> >  /*
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index f369871..08bb132 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -419,6 +419,8 @@ int vcpu_initialise(struct vcpu *v)
> >  {
> >      int rc = 0;
> >  
> > +    BUILD_BUG_ON( sizeof(struct cpu_info) > STACK_SIZE );
> > +
> >      v->arch.stack = alloc_xenheap_pages(STACK_ORDER, 
> > MEMF_node(vcpu_to_node(v)));
> >      if ( v->arch.stack == NULL )
> >          return -ENOMEM;
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index a98a45e..6b19bc5 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -43,9 +43,16 @@
> >   * entry.S) and struct cpu_info (which lives at the bottom of a Xen
> >   * stack) must be doubleword-aligned in size.  */
> >  static inline void check_stack_alignment_constraints(void) {
> > +#ifdef CONFIG_ARM_64
> > +    BUILD_BUG_ON((sizeof (struct cpu_user_regs)) & 0xf);
> > +    BUILD_BUG_ON((offsetof(struct cpu_user_regs, spsr_el1)) & 0xf);
> > +    BUILD_BUG_ON((offsetof(struct cpu_user_regs, lr)) & 0xf);
> > +    BUILD_BUG_ON((sizeof (struct cpu_info)) & 0xf);
> 
> Why both spsr_el1 and lr?
> From the comment on top of check_stack_alignment_constraints I would
> think that only spsr_el1 is needed?

You are right spsr_el1 is needed to check the alignments of the
inner/outer frames.

The lr check is because I had a few problems getting the inner code
pushing working correctly, IIRC it relates to the use of stp (since you
need to make sure the pair are correctly aligned)

> 
> 
> > +#else
> >      BUILD_BUG_ON((sizeof (struct cpu_user_regs)) & 0x7);
> >      BUILD_BUG_ON((offsetof(struct cpu_user_regs, sp_usr)) & 0x7);
> >      BUILD_BUG_ON((sizeof (struct cpu_info)) & 0x7);
> > +#endif
> >  }
> >  
> >  static int debug_stack_lines = 20;
> > diff --git a/xen/include/asm-arm/arm64/processor.h 
> > b/xen/include/asm-arm/arm64/processor.h
> > index b4602fa..bf436c8 100644
> > --- a/xen/include/asm-arm/arm64/processor.h
> > +++ b/xen/include/asm-arm/arm64/processor.h
> > @@ -51,6 +51,7 @@ struct cpu_user_regs
> >      __DECL_REG(x27,          r11_fiq);
> >      __DECL_REG(x28,          r12_fiq);
> >      __DECL_REG(/* x29 */ fp, /* r13_fiq */ sp_fiq);
> > +
> >      __DECL_REG(/* x30 */ lr, /* r14_fiq */ lr_fiq);
> >  
> >      register_t sp; /* Valid for hypervisor frames */
> > @@ -59,7 +60,7 @@ struct cpu_user_regs
> >      __DECL_REG(pc,           pc32);             /* ELR_EL2 */
> >      uint32_t cpsr;                              /* SPSR_EL2 */
> >  
> > -    uint64_t pad0;
> > +    uint32_t pad0; /* Align end of kernel frame. */
> 
> That would be because cpsr is a uint32_t, right?

Right, I think.
> 
> 
> >      /* Outer guest frame only from here on... */
> >  
> > @@ -68,7 +69,7 @@ struct cpu_user_regs
> >          uint32_t spsr_svc;       /* AArch32 */
> >      };
> >  
> > -    uint32_t pad1; /* Align */
> > +    uint32_t pad1; /* Doubleword-align the user half of the frame */
> 
> This one is because of the preceding union of two uint32_t

Yup.

> 
> >      /* AArch32 guests only */
> >      uint32_t spsr_fiq, spsr_irq, spsr_und, spsr_abt;
> > @@ -76,8 +77,6 @@ struct cpu_user_regs
> >      /* AArch64 guests only */
> >      uint64_t sp_el0;
> >      uint64_t sp_el1, elr_el1;
> > -
> > -    uint64_t pad2; /* Doubleword-align the user half of the frame */
> >  };
> 
> of course padding with uint64_t is useless on arm (32 and 64)

Right.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel