|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCHv4 27/43] plat/kvm: Add exception table for Arm64
On 13/07/18 10:54, Wei Chen wrote: Hi Julien, Hi Wei, -----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: My preference is finding the size automatically. I think this should be in a TODO list for future improvement. Anyway, I am not sure to understand what you mean by defining a structure similar to pt_regs. Per my understanding, you are re-using the same layout. What I suggested is you define the TRAP_STACK_SIZE in the same header where you define pt_regs. This will at least limit the potential error to have them unsync. + +/* + * IRQ_handler can be updated by interrupt chip (GIC) driver. + * Before that, reset IRQ_handler address to 0. + */ +.globl IRQ_handler +IRQ_handler: + .long 0x0This 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? The IRQ exception handler will never get executed when the interrupts are masked. So as long as you are masked, you are "interrupt-safe". + +.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, elI 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. You might want to add a comment with a TODO. So this is not forgotten. + /* 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. You might want to add a comment with a TODO. So this is not forgotten. + 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. Can you drop it then? +.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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |