|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 02/14] xen: arm: tweak arm64 stack frame layout
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.
> @@ -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?
> +#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?
> /* 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
> /* 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)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |