[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 28/04/2018 06:40, Huang Shijie wrote:

On Wed, Apr 18, 2018 at 06:48:24PM +0100, Julien Grall wrote:

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.

okay.



     .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.

okay.




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.


okay, I will think about it.

+
      /* 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.

I will change it to soft tab.



+       mov     x18, sp


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

I copied from the freebsd code. The original code uses x18 here.
Well, freebsd is using x18 because they effectively saved it a bit before to help with DTRACE support. So you need to adapt the code for Mini-OS and rework what does not apply for us. 

[...]




+
+       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.

okay, no problem.




+       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?

there is invalid case for EL0, such as el0_irq.
So keep it here for them.
I know the case is invalid but you should never reach them as you never return in EL0. My concern here is you seem to loosely implement EL0 support and no way to test whether it is going to work. So what's the purpose of keeping code that's going to rotten? 






+       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.

The sp is correct here.
We have sub the PT_REG_SIZE at the beginning of the @save_registers.
Well, the SP you want to store is the stack pointer before the context was saved. This is the more meaningful one when you want to print the context what was running before the exception.
But as you will need to move the instruction mov x18, sp here you will end to up to store the wrong SP here. 




+       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?

The interrupt is disabled here.


Then document it please.







+       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.

the x18 stores the previous sp maybe is different with current sp,
  So add PT_REG_SIZE to sp is wrong.


See above.




+        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?

We have already have the el1_irq to handle the IRQ.
and we need to save the context before call IRQ_handler.
You didn't understand why question. I didn't ask whether you can replace el1_irq by IRQ_handler, but why you need a pointer to the handler here rather than calling gic_handler in el1_irq directly. 

[...]


+#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?

I do not know how to generated it automatically :(
I have already suggested way in the previous e-mails. I am sure you can go through them again and look at what was discussed. 

Cheers,

--
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®.