[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [PATCH v3 10/43] arm64: add exception support

Hi Shijie,

On 16/04/18 07:31, Huang Shijie wrote:

This patch adds the exception support for arm64:
     .0) Add arm64/traps.h, and add new pt_regs{} for arm64.
     .1) Add save_registers/restore_registers which are based on FreeBSD code.


Please give a pointer to the code.


     .2) setup the vector table
     .3) remove the code for arm32.
This does not make sense in a patch that is adding arm64 support. The code removed look fairly independent, so can you move that in #2. 



Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
---
  arch/arm/arm64/arm64.S    | 147 ++++++++++++++++++++++++++++++++++++++++++++++
  arch/arm/arm64/traps.c    |  16 +++++
  include/arm/arm64/traps.h |  27 +++++++++
  include/arm/traps.h       |  21 +------
  4 files changed, 191 insertions(+), 20 deletions(-)
  create mode 100644 arch/arm/arm64/traps.c
  create mode 100644 include/arm/arm64/traps.h

diff --git a/arch/arm/arm64/arm64.S b/arch/arm/arm64/arm64.S
index b454cc6..9eb7ea0 100644
--- a/arch/arm/arm64/arm64.S
+++ b/arch/arm/arm64/arm64.S
@@ -1,6 +1,7 @@
  #include "asm.h"
  #include <arch_limits.h>
  #include <arm64/pagetable.h>
+#include <arm64/traps.h>
  #include <xen/xen.h>
/* This macro will use the x0/x1/x2/x16 */ 
@@ -101,6 +102,11 @@ ENTRY(_start)
      msr     ttbr0_el1, x0
      isb
+ /* Load the exception vectors */ 
+    ldr     x2, =vector_table
+    msr     vbar_el1, x2
+    isb
I don't think the isb is necessary here. You can rely on the one below (when setting up the SCTLR).
This would be fine because the vector table contains virtual address so it is unusable before turning on the MMU. 


+
      /* Turning on MMU */
      tlbi    vmalle1
      dsb     nsh
@@ -284,3 +290,144 @@ _setup_idmap_pgtable:
      adr     x0, idmap_l0_pgtable
      dsb     sy
      ret
+
+/* The save_registers/restore_registers are based on the code in FreeBSD */


Missing full stop.


+.macro save_registers el
All the code below is using hard tab, however the rest of the file is using soft tab. Please replace all hard tab with soft tab. 


+       mov     x18, sp
Something is wrong here. You seem to overwrite x18 without saving it. Did I miss anything? 


+
+       sub     sp, sp, #(PT_REG_SIZE)
+
+       stp     x28, x29, [sp, #(PT_REG_X + 28 * 8)]
I might be nice to make PT_REG_X a macro take the registers in parameters. This would ease the reading of the code. 


+       stp     x26, x27, [sp, #(PT_REG_X + 26 * 8)]
+       stp     x24, x25, [sp, #(PT_REG_X + 24 * 8)]
+       stp     x22, x23, [sp, #(PT_REG_X + 22 * 8)]
+       stp     x20, x21, [sp, #(PT_REG_X + 20 * 8)]
+       stp     x18, x19, [sp, #(PT_REG_X + 18 * 8)]
+       stp     x16, x17, [sp, #(PT_REG_X + 16 * 8)]
+       stp     x14, x15, [sp, #(PT_REG_X + 14 * 8)]
+       stp     x12, x13, [sp, #(PT_REG_X + 12 * 8)]
+       stp     x10, x11, [sp, #(PT_REG_X + 10 * 8)]
+       stp     x8,  x9,  [sp, #(PT_REG_X + 8  * 8)]
+       stp     x6,  x7,  [sp, #(PT_REG_X + 6  * 8)]
+       stp     x4,  x5,  [sp, #(PT_REG_X + 4  * 8)]
+       stp     x2,  x3,  [sp, #(PT_REG_X + 2  * 8)]
+       stp     x0,  x1,  [sp, #(PT_REG_X + 0  * 8)]
+
+       mrs     x10, elr_el1
+       mrs     x11, spsr_el1
+       mrs     x12, esr_el1
+.if \el == 0


AFAICT you will never go in EL0. So is there any reason to handle EL0?


+       mrs     x18, sp_el0
+.endif
Hmm I think you want the "mov x18, sp" but you also need to add the PT_REG_SIZE to get the correct sp. 


+       str     x10, [sp, #(PT_REG_ELR)]
+       stp     w11, w12, [sp, #(PT_REG_SPSR)]
+       stp     x18, x30, [sp, #(PT_REG_SP)]
+.endm
+
+.macro restore_registers el
Will you ever reach this macro with interrupt enabled? If so, don't you want to disable them. So you don't get interrupt in the middle of the restore? 


+       ldp     x18, x30, [sp, #(PT_REG_SP)]
+       ldp     x10, x11, [sp, #(PT_REG_ELR)]
+.if \el == 0
+       msr     sp_el0, x18
+.endif
+       msr     spsr_el1, x11
+       msr     elr_el1, x10
+
+       ldp     x0,  x1,  [sp, #(PT_REG_X + 0  * 8)]
+       ldp     x2,  x3,  [sp, #(PT_REG_X + 2  * 8)]
+       ldp     x4,  x5,  [sp, #(PT_REG_X + 4  * 8)]
+       ldp     x6,  x7,  [sp, #(PT_REG_X + 6  * 8)]
+       ldp     x8,  x9,  [sp, #(PT_REG_X + 8  * 8)]
+       ldp     x10, x11, [sp, #(PT_REG_X + 10 * 8)]
+       ldp     x12, x13, [sp, #(PT_REG_X + 12 * 8)]
+       ldp     x14, x15, [sp, #(PT_REG_X + 14 * 8)]
+       ldp     x16, x17, [sp, #(PT_REG_X + 16 * 8)]
+       ldp     x18, x19, [sp, #(PT_REG_X + 18 * 8)]
+       ldp     x20, x21, [sp, #(PT_REG_X + 20 * 8)]
+       ldp     x22, x23, [sp, #(PT_REG_X + 22 * 8)]
+       ldp     x24, x25, [sp, #(PT_REG_X + 24 * 8)]
+       ldp     x26, x27, [sp, #(PT_REG_X + 26 * 8)]
+       ldp     x28, x29, [sp, #(PT_REG_X + 28 * 8)]
+
+       mov     sp, x18
So x18 will not contain the sp here. But I think adding PT_REG_SIZE to sp should be enough here. 


+        eret
+.endm
+
+    .globl IRQ_handler
+IRQ_handler:
+    .long 0x0
I am not sure to understand the purpose of IRQ_handler. Can't you just directly call the handler? 


+
+    .align 6
+el1_sync:
+    save_registers 1
+    mov     x0, sp
+    mrs     x1, esr_el1;
+    mrs     x2, far_el1;
Do you expect the mini-OS to always run with interrupt disabled in EL1? If not, you may want to re-enable interrupt here. 


+    bl      do_sync
+    restore_registers 1
+
+    .align 6
+el1_irq:
+    save_registers 1
+    ldr     x0, IRQ_handler
+    blr     x0
+    restore_registers 1
+
+/* 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:                       \
+    save_registers  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);
+
+    /* 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 Exception level with SP_EL1 */
+    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 Exception level using AArch64 */
+    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 Exception level using AArch32 */
+    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       */
It looks like you don't implement EL0. I would try to simplify the EL0 handling in that case and also add a comment in the code. 


+END(vector_table)
diff --git a/arch/arm/arm64/traps.c b/arch/arm/arm64/traps.c
new file mode 100644
index 0000000..62dd2e6
--- /dev/null
+++ b/arch/arm/arm64/traps.c
@@ -0,0 +1,16 @@
+#include <mini-os/os.h>
+#include <mini-os/arm64/traps.h>
+#include <console.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/arm64/traps.h b/include/arm/arm64/traps.h
new file mode 100644
index 0000000..962f4a6
--- /dev/null
+++ b/include/arm/arm64/traps.h
@@ -0,0 +1,27 @@
+#ifndef _TRAPS_H_
+#define _TRAPS_H_
+
+#ifndef __ASSEMBLY__
+struct pt_regs {
+    uint64_t sp;
+    uint64_t pc;
+    uint64_t lr;  /* elr */
+    uint32_t pstate;
+    uint32_t esr;
+
+    /* From x0 ~ x29 */
+    uint64_t x[30];
+};
+
+#else
+
+#define PT_REG_SIZE   (272)
+
+#define PT_REG_SP     (0)
+#define PT_REG_ELR    (16)
+#define PT_REG_SPSR   (24)
+#define PT_REG_X      (32)
I honestly don't like hardcoding offset of the structure. This is a real call to mess up in the future (or even now). The lack of comments don't help either.
But I am pretty sure I asked it before. Can't they be generated automatically? 

Cheers,


+
+#endif
+
+#endif
diff --git a/include/arm/traps.h b/include/arm/traps.h
index 704df22..b076f41 100644
--- a/include/arm/traps.h
+++ b/include/arm/traps.h
@@ -1,20 +1 @@
-#ifndef _TRAPS_H_
-#define _TRAPS_H_
-
-struct pt_regs {
-    unsigned long r0;
-    unsigned long r1;
-    unsigned long r2;
-    unsigned long r3;
-    unsigned long r4;
-    unsigned long r5;
-    unsigned long r6;
-    unsigned long r7;
-    unsigned long r8;
-    unsigned long r9;
-    unsigned long r10;
-    unsigned long r11;
-    unsigned long r12;
-};
-
-#endif
+#include <mini-os/arm64/traps.h>



--
Julien Grall

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

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.