Re: [Minios-devel] [PATCH 07/40] arm64: add exception support

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Shijie,

On 03/11/17 03:11, Huang Shijie wrote:
> This patch adds the exception support for arm64:
>      .0) Add the new pt_regs{} for arm64.
>      .1) Add stack macros: trap_entry/trap_exit
>      .2) setup the vector table
>
> Change-Id: I61d86ed4516443bf8baf4ef0b13687f9cb2047fd
> Jira: ENTOS-247
> Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> ---
>   arch/arm/arm64/arm64.S | 154 +++++++++++++++++++++++++++++++++++++++++++++++++
>   arch/arm/arm64/traps.c |  15 +++++
>   include/arm/traps.h    |  14 +++++
>   3 files changed, 183 insertions(+)
>   create mode 100644 arch/arm/arm64/traps.c
>
> diff --git a/arch/arm/arm64/arm64.S b/arch/arm/arm64/arm64.S
> index 61c14a6..1321a1d 100644
> --- a/arch/arm/arm64/arm64.S
> +++ b/arch/arm/arm64/arm64.S
> @@ -105,6 +105,11 @@ ENTRY(_start)
>       msr     ttbr0_el1, x5
>       isb
>   
> +    /* Load the exception vectors */
> +    ldr     x2, =vector_table
> +    msr     vbar_el1, x2
> +    isb
> +
>       /* Turning on MMU */
>       tlbi    vmalle1
>       dsb     nsh
> @@ -305,3 +310,152 @@ ENTRY(_setup_idmap_pgtable)
>       dsb     sy
>       ret
>   ENDPROC(_setup_idmap_pgtable)
> +
> +/* please refer to struct pt_regs{} in traps.h */
> +#define FRAME_SIZE 272
Can you please define that in traps.h? This would at least help to 
prevent some divergence between pt_regs and FRAME_SIZE.
Question for the Mini-OS maintainers, do you have a way similar to 
asm-offsets.c in Mini-OS? This would avoid a lot of hardcoding value in 
the code.
> +
> +    .macro trap_entry, el
> +    sub   sp, sp, #FRAME_SIZE
> +    stp   x0, x1, [sp, #16 * 0]
> +    stp   x2, x3, [sp, #16 * 1]
> +    stp   x4, x5, [sp, #16 * 2]
> +    stp   x6, x7, [sp, #16 * 3]
> +    stp   x8, x9, [sp, #16 * 4]
> +    stp   x10, x11, [sp, #16 * 5]
> +    stp   x12, x13, [sp, #16 * 6]
> +    stp   x14, x15, [sp, #16 * 7]
> +    stp   x16, x17, [sp, #16 * 8]
> +    stp   x18, x19, [sp, #16 * 9]
> +    stp   x20, x21, [sp, #16 * 10]
> +    stp   x22, x23, [sp, #16 * 11]
> +    stp   x24, x25, [sp, #16 * 12]
> +    stp   x26, x27, [sp, #16 * 13]
> +    stp   x28, x29, [sp, #16 * 14]
Is there any way to simplify this code? Such as using writeback version?

> +
> +    .if     \el == 0
> +    mrs     x21, sp_el0
> +    .else
> +    add     x21, sp, #FRAME_SIZE
> +    .endif
This code was adapted from Linux, right? At least it looks like because 
of the way to save registers and the temporary register (which could be 
any between x0 and x29) used is strangely the same as Linux.
> +
> +    stp     x30, x21, [sp, #16 * 15]
> +
> +    mrs     x22, elr_el1
> +    mrs     x23, spsr_el1
> +    stp     x22, x23, [sp, #16 * 16]
> +    .endm
> +
> +    .macro trap_exit, el
> +    ldp   x22, x23, [sp, #16 * 16]
> +
> +    /* restore the elr and spsr */
> +    msr   elr_el1, x22
> +    msr   spsr_el1, x23
> +
> +    /* restore the lr(x30) and sp_el0 */
> +    ldp   x30, x21, [sp, #16 * 15]
> +
> +    .if     \el == 0
> +    msr     sp_el0, x21
> +    .endif
> +
> +    ldp   x0, x1, [sp, #16 * 0]
> +    ldp   x2, x3, [sp, #16 * 1]
> +    ldp   x4, x5, [sp, #16 * 2]
> +    ldp   x6, x7, [sp, #16 * 3]
> +    ldp   x8, x9, [sp, #16 * 4]
> +    ldp   x10, x11, [sp, #16 * 5]
> +    ldp   x12, x13, [sp, #16 * 6]
> +    ldp   x14, x15, [sp, #16 * 7]
> +    ldp   x16, x17, [sp, #16 * 8]
> +    ldp   x18, x19, [sp, #16 * 9]
> +    ldp   x20, x21, [sp, #16 * 10]
> +    ldp   x22, x23, [sp, #16 * 11]
> +    ldp   x24, x25, [sp, #16 * 12]
> +    ldp   x26, x27, [sp, #16 * 13]
> +    ldp   x28, x29, [sp, #16 * 14]
> +
> +    add     sp, sp, #FRAME_SIZE
> +    eret
> +    .endm
> +
> +    .globl IRQ_handler
> +IRQ_handler:
> +    .long 0x0
> +
> +    .align 6
> +el1_sync:
> +    trap_entry 1
> +    mov     x0, sp
> +    mrs     x1, esr_el1;
> +    mrs     x2, far_el1;
> +    bl      do_sync
> +    trap_exit 1
> +ENDPROC(el1_sync)
> +
> +    .align 6
> +el1_irq:
> +    trap_entry 1
> +    ldr     x0, IRQ_handler
> +    blr     x0
> +    trap_exit 1
> +ENDPROC(el1_irq)
> +
> +    /* Exception vector entry */
> +    .macro vector_entry label
> +    .align  7
> +    b       \label
> +    .endm
> +
> +    .align  11
> +ENTRY(vector_table)
> +    /* Current Exception level with SP_EL0 */
> +    vector_entry el1_sync_invalid         /* Synchronous EL1t       */
> +    vector_entry el1_irq_invalid          /* IRQ EL1t               */
> +    vector_entry el1_fiq_invalid          /* FIQ EL1t               */
> +    vector_entry el1_error_invalid        /* Error EL1t             */
> +
> +    /* Current Executable level with SP_EL1 */
Executable? Do you mean Exception?

> +    vector_entry el1_sync                 /* Synchronous EL1h       */
> +    vector_entry el1_irq                  /* IRQ EL1h               */
> +    vector_entry el1_fiq_invalid          /* FIQ EL1h               */
> +    vector_entry el1_error_invalid        /* Error EL1h             */
> +
> +    /* Lower Executable level using AArch64 */
Ditto.

> +    vector_entry el0_sync_invalid         /* Synchronous 64-bit EL0 */
> +    vector_entry el0_irq_invalid          /* IRQ 64-bit EL0         */
> +    vector_entry el0_fiq_invalid          /* FIQ 64-bit EL0         */
> +    vector_entry el0_error_invalid        /* Error 64-bit EL0       */
> +
> +    /* Lower Executable level using AArch32 */
Ditto.

> +    vector_entry el0_sync_invalid         /* Synchronous 32-bit EL0 */
> +    vector_entry el0_irq_invalid          /* IRQ 32-bit EL0         */
> +    vector_entry el0_fiq_invalid          /* FIQ 32-bit EL0         */
> +    vector_entry el0_error_invalid        /* Error 32-bit EL0       */
> +END(vector_table)
> +
> +/* Bad Abort numbers */
> +#define BAD_SYNC    0
> +#define BAD_IRQ     1
> +#define BAD_FIQ     2
> +#define BAD_ERROR   3
> +
> +#define el_invalid(name, reason, el)  \
> +    .align 6;                         \
> +name##_invalid:                       \
> +    trap_entry  el;                   \
> +    mov     x0, sp;                   \
> +    mov     x1, #(reason);            \
> +    mrs     x2, esr_el1;              \
> +    mrs     x3, far_el1;              \
> +    b       do_bad_mode;              \
> +ENDPROC(name##_invalid);              \
> +
> +el_invalid(el1_sync, BAD_SYNC, 1);
> +el_invalid(el0_sync, BAD_SYNC, 0);
> +el_invalid(el1_irq, BAD_IRQ, 1);
> +el_invalid(el0_irq, BAD_IRQ, 0);
> +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);
It would be more logic to define all the exception handler before 
creating the vector table.
> diff --git a/arch/arm/arm64/traps.c b/arch/arm/arm64/traps.c
> new file mode 100644
> index 0000000..200b0ab
> --- /dev/null
> +++ b/arch/arm/arm64/traps.c
> @@ -0,0 +1,15 @@
> +#include <mini-os/os.h>
> +#include <mini-os/traps.h>
> +
> +void do_bad_mode(struct pt_regs *regs, int reason,
> +                  unsigned long esr, unsigned long far)
> +{
> +    /* TO DO */
> +    do_exit();
> +}
> +
> +void do_sync(struct pt_regs *regs, unsigned long esr, unsigned long far)
> +{
> +    /* TO DO */
> +    do_exit();
> +}
> diff --git a/include/arm/traps.h b/include/arm/traps.h
> index 704df22..21de0e9 100644
> --- a/include/arm/traps.h
> +++ b/include/arm/traps.h
> @@ -1,6 +1,7 @@
>   #ifndef _TRAPS_H_
>   #define _TRAPS_H_
>   
> +#if defined(__arm__)
>   struct pt_regs {
>       unsigned long r0;
>       unsigned long r1;
> @@ -16,5 +17,18 @@ struct pt_regs {
>       unsigned long r11;
>       unsigned long r12;
>   };
> +#elif defined(__aarch64__)
> +struct pt_regs {
> +    /* From x0 ~ x29 */
> +    uint64_t x[30];
> +    union {
> +        uint64_t x30;
> +        uint64_t lr;
> +    };
> +    uint64_t sp;
> +    uint64_t pc;
> +    uint64_t pstate;
> +};
> +#endif
Please introduce {arm64,arm32}/traps.h

>   
>   #endif
Cheers,

--
Julien Grall

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