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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Mon, Apr 30, 2018 at 09:05:23AM +0100, Julien Grall wrote:
Hi Julien,
> 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.
okay, I will do some rework about this code.

> 
> [...]
> 
> >>
> >>>+
> >>>+  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?
okay, I can remove the code for 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.
> >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.
Do you mean I should store the SP like this:

       --------------------------------------
        sub     sp, sp, #(PT_REG_SIZE)

        mov     x18, sp

        stp     ....
        stp     ....
        stp     ....
       --------------------------------------
  
In this way, we store the correct SP value.


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

Yes, I think we can call gic_handler directly in el1_irq.

I admit that I did not do the code optimization for it.

> 
> [...]
> 
> >>>+#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.
sorry, I searched my old email, but I cannot find it.
could you please tell me again?

Thanks
Huang Shijie

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