Re: [Minios-devel] [UNIKRAFT PATCHv3 15/25] plat/kvm: Force align the stack pointer for Arm64 EL1 exceptions

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Wei Chen,

Please find the comments inline:

Thanks & Regards
Sharan

On 12/13/18 10:15 AM, Wei Chen wrote:
> If we enable the SCTLR_ELx.SA, Arm64 stack pointer must be aligned to
> 16-byte before being used as base address. But some valid EL1 exceptions
> don't guarantee that SP_EL1 was aligned when entering the exceptions.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   plat/kvm/arm/exceptions.S | 65 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 54 insertions(+), 11 deletions(-)
>
> diff --git a/plat/kvm/arm/exceptions.S b/plat/kvm/arm/exceptions.S
> index e4a5c74..3fafb40 100644
> --- a/plat/kvm/arm/exceptions.S
> +++ b/plat/kvm/arm/exceptions.S
> @@ -29,9 +29,51 @@
>   #include <uk/arch/lcpu.h>
>   #include <uk/asm.h>
>   
> +.macro EXCHANGE_SP_WITH_X0
> +       add sp, sp, x0  // new_sp = sp + x0
> +       sub x0, sp, x0  // new_x0 = new_sp - x0 = sp + x0 - x0 = sp
> +       sub sp, sp, x0  // new_sp = new_sp - new_x0 = sp + x0 - sp = x0
> +.endm
> +
> +.macro ALIGN_STACK
> +       // First, exchange the SP with x0
> +       EXCHANGE_SP_WITH_X0
> +
> +       // Check whether the stack is alignment
> +       tst x0, #0xf
> +       // If yes, save and go out. If not, align the stack
> +       b.eq 0f
> +
> +       // Start to align the stack.
> +
> +       // We will use the x1 as temporary, save x1 to stack temporary
> +       str x1, [x0]
> +
> +       // Align down sp to 16-byte, save old sp to aligned_sp[__SP_OFFSET]
> +       bic x1, x0, #0xf
> +       str x0, [x1, #__SP_OFFSET]
> +
> +       // Restore x1 before x0 is overridden
> +       ldr x1, [x0]
> +
> +       // Save aligned_sp to x0
> +       bic x0, x0, #0xf
> +       b 1f
> +0:
> +       str x0, [x0, #__SP_OFFSET]
> +1:
> +       // Change back the SP from x0
> +       EXCHANGE_SP_WITH_X0
> +.endm
> +
>   .macro ENTER_TRAP, el
>         sub sp, sp, #__TRAP_STACK_SIZE
>   
> +.if	\el != 0
> +       /* Force align the stack, and save SP to __SP_OFFSET */
> +       ALIGN_STACK
> +.endif
> +
>         /* Save general purpose registers */
>         stp x0, x1, [sp, #16 * 0]
>         stp x2, x3, [sp, #16 * 1]
> @@ -58,25 +100,17 @@
>         mrs x23, esr_el1
>         stp x22, x23, [sp, #16 * 16]
>   
> -	/* Save stack pointer for lower level exception */
>   .if \el == 0
> +       /* Save stack pointer for lower level exception */
>         mrs x18, sp_el0
> -.else
> -       add x18, sp, #__TRAP_STACK_SIZE
> +       str x18, [sp, #__SP_EL0_OFFSET]
>   .endif
> -       str x18, [sp, #16 * 17]
> +
>   .endm
>   
>   .macro LEAVE_TRAP, el
>         /* Mask IRQ to make sure restore would not be interrupted by IRQ */
>         msr daifset, #2
> -
> -       /* Restore stack pointer for lower level exception */
> -       ldr x18, [sp, #16 * 17]
> -.if \el == 0
> -       msr sp_el0, x18
> -.endif
> -
>         /* Restore pstate and exception status register */
>         ldp x22, x23, [sp, #16 * 16]
>         msr spsr_el1, x22
> @@ -103,6 +137,15 @@
>         ldp x2, x3, [sp, #16 * 1]
>         ldp x0, x1, [sp, #16 * 0]
>   
> +.if \el == 0
> +       /* Restore stack pointer for exception from EL0 */
> +       ldr x18, [sp, #__SP_EL0_OFFSET]
> +       msr sp_el0, x18
> +.else
> +       /* Restore stack pointer for exception from EL1 */
> +       ldr x18, [sp, #__SP_OFFSET]
> +       mov sp, x18
> +.endif
We are restoring the x18 register and overwriting it sp_el0 or sp.
Is this the expected behavior? Probably we can restore the x18, x19 
after this operation.
>         add sp, sp, #__TRAP_STACK_SIZE
>   
>   	eret
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel