|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [PATCH v3 10/43] arm64: add exception support
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.
It is really not proper to overwrite x18.
I will change it later.
>
> >+
> >+ 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.
>
> >+ 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.
>
> >+ 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.
>
> >+ 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.
>
> >+ 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.
>
> >+
> >+ .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.
okay, I will check it.
>
> >+ 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.
okay,
>
> >+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?
I do not know how to generated it automatically :(
thanks
Huang Shijie
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |