Re: [Minios-devel] [UNIKRAFT PATCHv4 27/43] plat/kvm: Add exception table for Arm64

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxxxxx>
> Sent: 2018年7月12日 20:13
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 27/43] plat/kvm: Add exception
> table for Arm64
> 
> Hi Wei,
> 
> On 06/07/18 10:03, Wei Chen wrote:
> > On Arm64, we need SYNC exception handler to handle some exceptions
> > like access NULL pointer, and we need IRQ exception handler to handle
> > IRQs like timer IRQ. Both these types of exceptions would be handled
> > in EL1. Except these two types of exceptions, other exceptions would
> > treated as invalid exceptions.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > ---
> >   plat/kvm/arm/entry64.S    |   4 +
> >   plat/kvm/arm/exceptions.S | 209 ++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 213 insertions(+)
> >   create mode 100644 plat/kvm/arm/exceptions.S
> >
> > diff --git a/plat/kvm/arm/entry64.S b/plat/kvm/arm/entry64.S
> > index 8b470c1..c031b79 100644
> > --- a/plat/kvm/arm/entry64.S
> > +++ b/plat/kvm/arm/entry64.S
> > @@ -39,6 +39,10 @@ ENTRY(_libkvmplat_entry)
> >     orr x0, x0, #CPACR_FPEN_TRAP_NONE
> >     msr cpacr_el1, x0
> >
> > +   /* Setup excetpion vector table address before enable MMU */
> > +   ldr x29, =vector_table
> > +   msr VBAR_EL1, x29
> > +
> >
> >     /* Load dtb address to x0 as a parameter */
> >     ldr x0, =_dtb
> > diff --git a/plat/kvm/arm/exceptions.S b/plat/kvm/arm/exceptions.S
> > new file mode 100644
> > index 0000000..3e2edc6
> > --- /dev/null
> > +++ b/plat/kvm/arm/exceptions.S
> > @@ -0,0 +1,209 @@
> > +/* SPDX-License-Identifier: ISC */
> 
> Same remark as before for SPDX.
> 
> > +/*-
> 
> s/-//
> 
> > + *
> > + * Copyright (c) 2014 Andrew Turner, All rights reserved.
> > + * Copyright (c) 2018 Arm Ltd., All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> PURPOSE
> > + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> CONSEQUENTIAL
> > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> STRICT
> > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
> WAY
> > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > + * SUCH DAMAGE.
> > + *
> > + */
> > +#include <uk/arch/limits.h>
> > +#include <arm/cpu_defs.h>
> > +
> > +/*
> > + * Stack size to save general purpose registers and essential system
> > + * registers. 8 * (30 + elr_el1 + spsr_el1 + esr_el1) = 264.
> > + * From exceptions come from EL0, we have to save sp_el0. So the
> > + * TRAP_STACK_SIZE should be 264 + 8 = 272
> > + */
> > +#define TRAP_STACK_SIZE 272
> 
> That's sound really fragile. There are no way to relate that value with
> the structure itself. This means, it will be really hard to keep the
> change in sync.
> 
> The best solution is to find the size automatically. The other solution
> would be to have this define very close to the structure with a big fat
> warning on top saying "TRAP_STACK_SIZE needs to be changed".
> 

Did you men using a offset.s to find the size automatically? If yes, prefer
The second one. I would like to define a structure similar to pt_regs.

> > +
> > +/*
> > + * IRQ_handler can be updated by interrupt chip (GIC) driver.
> > + * Before that, reset IRQ_handler address to 0.
> > + */
> > +.globl IRQ_handler
> > +IRQ_handler:
> > +   .long 0x0
> 
> This is yet another ugly bits of Mini-OS Arm. As I commented on the
> Arm64 Mini-OS series, what is the point of this? You should never
> receive interrupt before the GIC has been setup as you should have
> interrupt disabled until then.
> 
> Furthermore, AFAIU, you are planning to support only on GIC for a given
> binary. So there are no need for this.
> 

Ok, I admit that before GIC enabled, we should not be here.
I have another question, can I trigger a CPU virtual interrupt to be here?

> > +
> > +.macro ENTER_TRAP, el
> > +   sub  sp, sp, #TRAP_STACK_SIZE
> > +
> > +   /* Save general purpose registers */
> > +   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]
> > +
> > +   /* Save LR and exception PC */
> > +   mrs x21, elr_el1
> > +   stp x30, x21, [sp, #16 * 15]
> > +
> > +   /* Save pstate and exception status register */
> > +   mrs x22, spsr_el1
> > +   mrs x23, esr_el1
> > +   stp x22, x23, [sp, #16 * 16]
> > +
> > +   /* Save stack poniter for lower level exception */
> 
> s/poniter/pointer/
> 

Ok.

> > +.if \el == 0
> > +   mrs x18, sp_el0
> > +.else
> > +   add x18, sp, #TRAP_STACK_SIZE
> > +.endif
> > +   str x18, [sp, #16 * 17]
> > +.endm
> > +
> > +.macro LEAVE_TRAP, el
> 
> I know that you don't support interrupt yet. But 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?
> 

No, I didn't. I just test sync exception entry with this exception table.
Other exceptions will be tested after GIC enabled.

> > +   /* Restore stack poniter for lower level exception */
> 
> s/poniter/pointer/
> 
> > +   ldr x18, [sp, #16 * 17]
> > +.if \el == 0
> > +   msr sp_el0, x18
> > +.endif
> > +
> > +   /* Restore pstate and exception status register */
> > +   ldp x22, x23, [sp, #16 * 16]
> > +   msr spsr_el1, x22
> > +   msr esr_el1, x23
> > +
> > +   /* Restore LR and exception PC */
> > +   ldp x30, x21, [sp, #16 * 15]
> > +   msr elr_el1, x21
> > +
> > +   /* Restore general purpose registers */
> > +   ldp x28, x29, [sp, #16 * 14]
> > +   ldp x26, x27, [sp, #16 * 13]
> > +   ldp x24, x25, [sp, #16 * 12]
> > +   ldp x22, x23, [sp, #16 * 11]
> > +   ldp x20, x21, [sp, #16 * 10]
> > +   ldp x18, x19, [sp, #16 * 9]
> > +   ldp x16, x17, [sp, #16 * 8]
> > +   ldp x14, x15, [sp, #16 * 7]
> > +   ldp x12, x13, [sp, #16 * 6]
> > +   ldp x10, x11, [sp, #16 * 5]
> > +   ldp x8, x9, [sp, #16 * 4]
> > +   ldp x6, x7, [sp, #16 * 3]
> > +   ldp x4, x5, [sp, #16 * 2]
> > +   ldp x2, x3, [sp, #16 * 1]
> > +   ldp x0, x1, [sp, #16 * 0]
> > +
> > +   eret
> > +.endm
> > +
> > +/*
> > + * Most aarch64 SoC is using 64-byte cache line. Align the
> > + * exception handlers to 64-byte will benefit the cache hit
> > + * rate of handlers.
> > + */
> > +.align 6
> > +el1_sync:
> 
> Is there any concern about Meltdown/Spectre for Unikraft? I guess not,
> but wanted to double check.
> 

No, in current stage, I think we don't think so much. And I don't want
to involve such cases in the very first enablement series.

> > +   ENTER_TRAP 1
> > +   mov x0, sp
> > +   mrs x1, far_el1
> > +   bl trap_handler
> > +   LEAVE_TRAP 1
> > +
> > +.align 6
> > +el1_irq:
> > +   ENTER_TRAP 1
> > +   ldr x0, IRQ_handler
> > +   blr x0
> > +   LEAVE_TRAP 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:                            \
> > +   ENTER_TRAP  el;                 \
> > +   mov x0, sp;                     \
> > +   mov x1, el;                     \
> > +   mov x2, #(reason);              \
> > +   mrs x3, far_el1;                \
> > +   b   invalid_trap_handler;       \
> > +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);
> > +
> > +
> > +/*
> > + * Macro for Exception vectors.
> > + */
> > +.macro vector_entry label
> > +.align  7
> > +   b \label
> > +.endm
> > +
> > +/*
> > + * Exception vectors.
> > + *
> > + * AArch64 unikernel runs in EL1 mode using the SP1 stack. The vectors
> > + * don't have a fixed address, only alignment (2^11) requirements.
> > + */
> > +.pushsection ".exception.text", "ax"
> 
> Hmmm, I don't see this section in the linker script. But is it necessary
> to have them in a separate section?
> 

Currently, this is unnecessary.

> > +.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       */
> > +END(vector_table)
> >
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel