From e34599d827b3d0515a17f02cfcc48ba6d957997e Mon Sep 17 00:00:00 2001 From: Ivan Velickovic Date: Tue, 13 Aug 2024 11:26:23 +1000 Subject: [PATCH] Cleanup vGIC fault handling code * Make virq_controller_init for the vGIC register the vGIC region with the appropriate callbacks using the library's API instead of hard-coding it in the fault handling code. * Don't use `BOARD_` defines, instead just use seL4 based defines since we eventually want the library to be Microkit agnostic. Still not satisfied with the vGIC/fault handling code, there's still lots of improvements to be made. Signed-off-by: Ivan Velickovic --- build.zig | 2 - examples/simple/build.zig | 1 - examples/zig/build.zig | 2 +- include/libvmm/arch/aarch64/vgic/vdist.h | 22 ++--------- include/libvmm/arch/aarch64/vgic/vgic.h | 13 +++--- src/arch/aarch64/fault.c | 50 +++++++++--------------- src/arch/aarch64/vgic/vgic.c | 7 +--- src/arch/aarch64/vgic/vgic_v3.c | 14 ++----- src/arch/aarch64/virq.c | 20 +++++++++- 9 files changed, 51 insertions(+), 80 deletions(-) diff --git a/build.zig b/build.zig index 4ad83f380..db08f83e6 100644 --- a/build.zig +++ b/build.zig @@ -33,7 +33,6 @@ pub fn build(b: *std.Build) void { const target = b.standardTargetOptions(.{}); const libmicrokit_include_opt = b.option([]const u8, "libmicrokit_include", "Path to the libmicrokit include directory") orelse null; - const microkit_board_opt = b.option([]const u8, "microkit_board", "Name of Microkit board") orelse null; // Default to vGIC version 2 const arm_vgic_version = b.option(usize, "arm_vgic_version", "ARM vGIC version to emulate") orelse null; @@ -69,7 +68,6 @@ pub fn build(b: *std.Build) void { "-Wno-unused-function", "-mstrict-align", "-fno-sanitize=undefined", // @ivanv: ideally we wouldn't have to turn off UBSAN - b.fmt("-DBOARD_{s}", .{ microkit_board_opt.? }) // @ivanv: shouldn't be needed as the library should not depend on the board } }); diff --git a/examples/simple/build.zig b/examples/simple/build.zig index 0f5721e86..b1746fd35 100644 --- a/examples/simple/build.zig +++ b/examples/simple/build.zig @@ -103,7 +103,6 @@ pub fn build(b: *std.Build) void { .optimize = optimize, .libmicrokit_include = @as([]const u8, libmicrokit_include), .arm_vgic_version = arm_vgic_version, - .microkit_board = @as([]const u8, microkit_board), }); const libvmm = libvmm_dep.artifact("vmm"); diff --git a/examples/zig/build.zig b/examples/zig/build.zig index 739723709..1f49227e3 100644 --- a/examples/zig/build.zig +++ b/examples/zig/build.zig @@ -54,8 +54,8 @@ pub fn build(b: *std.Build) void { .optimize = optimize, .libmicrokit_include = @as([]const u8, libmicrokit_include), // Because we only support QEMU virt AArch64, vGIC version is 2. + // Because we only support QEMU virt AArch64, vGIC version is always 2. .arm_vgic_version = @as(usize, 2), - .microkit_board = @as([]const u8, microkit_board), }); const libvmm = libvmm_dep.artifact("vmm"); diff --git a/include/libvmm/arch/aarch64/vgic/vdist.h b/include/libvmm/arch/aarch64/vgic/vdist.h index 0d4823bc2..768f39b16 100644 --- a/include/libvmm/arch/aarch64/vgic/vdist.h +++ b/include/libvmm/arch/aarch64/vgic/vdist.h @@ -248,7 +248,6 @@ static void vgic_dist_clr_pending_irq(struct gic_dist_map *dist, size_t vcpu_id, static bool vgic_dist_reg_read(size_t vcpu_id, vgic_t *vgic, uint64_t offset, uint64_t fsr, seL4_UserContext *regs) { - bool success = false; struct gic_dist_map *gic_dist = vgic_get_dist(vgic->registers); uint32_t reg = 0; int reg_offset = 0; @@ -400,23 +399,13 @@ static bool vgic_dist_reg_read(size_t vcpu_id, vgic_t *vgic, uint64_t offset, ui #endif default: LOG_VMM_ERR("Unknown register offset 0x%x", offset); - // err = ignore_fault(fault); - success = fault_advance_vcpu(vcpu_id, regs); - assert(success); - goto fault_return; + /* Pretend that everything is fine */ + return true; } uint32_t mask = fault_get_data_mask(GIC_DIST_PADDR + offset, fsr); - // fault_set_data(fault, reg & mask); - // @ivanv: interesting, when we don't call fault_Set_data in the CAmkES VMM, everything works fine?... - success = fault_advance(vcpu_id, regs, GIC_DIST_PADDR + offset, fsr, reg & mask); - assert(success); + fault_emulate_write(regs, GIC_DIST_PADDR + offset, fsr, reg & mask); -fault_return: - if (!success) { - printf("reg read success is false\n"); - } - // @ivanv: revisit, also make fault_return consistint. it's called something else in vgic_dist_reg_write - return success; + return true; } static inline void emulate_reg_write_access(seL4_UserContext *regs, uint64_t addr, uint64_t fsr, uint32_t *reg) @@ -620,9 +609,6 @@ static bool vgic_dist_reg_write(size_t vcpu_id, vgic_t *vgic, uint64_t offset, u return false; } - success = fault_advance_vcpu(vcpu_id, regs); - assert(success); - return success; } diff --git a/include/libvmm/arch/aarch64/vgic/vgic.h b/include/libvmm/arch/aarch64/vgic/vgic.h index dc7030959..dab3759fb 100644 --- a/include/libvmm/arch/aarch64/vgic/vgic.h +++ b/include/libvmm/arch/aarch64/vgic/vgic.h @@ -10,16 +10,13 @@ #include #include -// @ivanv: this should all come from the DTS! -// @ivanv: either this should all be compile time or all runtime -// as in initialising the vgic should depend on the runtime values -#if defined(BOARD_qemu_virt_aarch64) +#if defined(CONFIG_PLAT_QEMU_ARM_VIRT) #define GIC_V2 #define GIC_DIST_PADDR 0x8000000 -#elif defined(BOARD_odroidc4) +#elif defined(CONFIG_PLAT_ODROIDC4) #define GIC_V2 #define GIC_DIST_PADDR 0xffc01000 -#elif defined(BOARD_maaxboard) +#elif defined(CONFIG_PLAT_MAAXBOARD) #define GIC_V3 #define GIC_DIST_PADDR 0x38800000 #define GIC_REDIST_PADDR 0x38880000 @@ -54,7 +51,7 @@ void vgic_init(); bool fault_handle_vgic_maintenance(size_t vcpu_id); -bool handle_vgic_dist_fault(size_t vcpu_id, uint64_t fault_addr, uint64_t fsr, seL4_UserContext *regs); -bool handle_vgic_redist_fault(size_t vcpu_id, uint64_t fault_addr, uint64_t fsr, seL4_UserContext *regs); +bool handle_vgic_dist_fault(size_t vcpu_id, size_t offset, size_t fsr, seL4_UserContext *regs, void *data); +bool handle_vgic_redist_fault(size_t vcpu_id, size_t offset, size_t fsr, seL4_UserContext *regs, void *data); bool vgic_register_irq(size_t vcpu_id, int virq_num, virq_ack_fn_t ack_fn, void *ack_data); bool vgic_inject_irq(size_t vcpu_id, int irq); diff --git a/src/arch/aarch64/fault.c b/src/arch/aarch64/fault.c index 396778b07..fa1d44f65 100644 --- a/src/arch/aarch64/fault.c +++ b/src/arch/aarch64/fault.c @@ -360,38 +360,26 @@ bool fault_handle_vm_exception(size_t vcpu_id) int err = seL4_TCB_ReadRegisters(BASE_VM_TCB_CAP + vcpu_id, false, 0, SEL4_USER_CONTEXT_SIZE, ®s); assert(err == seL4_NoError); - switch (addr) { - case GIC_DIST_PADDR...GIC_DIST_PADDR + GIC_DIST_SIZE: - return handle_vgic_dist_fault(vcpu_id, addr, fsr, ®s); -#if defined(GIC_V3) - /* Need to handle redistributor faults for GICv3 platforms. */ - case GIC_REDIST_PADDR...GIC_REDIST_PADDR + GIC_REDIST_SIZE: - return handle_vgic_redist_fault(vcpu_id, addr, fsr, ®s); -#endif - default: { - bool success = fault_handle_registered_vm_exceptions(vcpu_id, addr, fsr, ®s); - if (!success) { - /* - * We could not find a registered handler for the address, meaning that the fault - * is genuinely unexpected. Surprise! - * Now we print out as much information relating to the fault as we can, hopefully - * the programmer can figure out what went wrong. - */ - size_t ip = microkit_mr_get(seL4_VMFault_IP); - size_t is_prefetch = seL4_GetMR(seL4_VMFault_PrefetchFault); - bool is_write = fault_is_write(fsr); - LOG_VMM_ERR("unexpected memory fault on address: 0x%lx, FSR: 0x%lx, IP: 0x%lx, is_prefetch: %s, is_write: %s\n", - addr, fsr, ip, is_prefetch ? "true" : "false", is_write ? "true" : "false"); - tcb_print_regs(vcpu_id); - vcpu_print_regs(vcpu_id); - } else { - /* @ivanv, is it correct to unconditionally advance the CPU here? */ - fault_advance_vcpu(vcpu_id, ®s); - } - - return success; - } + bool success = fault_handle_registered_vm_exceptions(vcpu_id, addr, fsr, ®s); + if (!success) { + /* + * We could not find a registered handler for the address, meaning that the fault + * is genuinely unexpected. Surprise! + * Now we print out as much information relating to the fault as we can, hopefully + * the programmer can figure out what went wrong. + */ + size_t ip = microkit_mr_get(seL4_VMFault_IP); + size_t is_prefetch = seL4_GetMR(seL4_VMFault_PrefetchFault); + bool is_write = fault_is_write(fsr); + LOG_VMM_ERR("unexpected memory fault on address: 0x%lx, FSR: 0x%lx, IP: 0x%lx, is_prefetch: %s, is_write: %s\n", + addr, fsr, ip, is_prefetch ? "true" : "false", is_write ? "true" : "false"); + tcb_print_regs(vcpu_id); + vcpu_print_regs(vcpu_id); + } else { + fault_advance_vcpu(vcpu_id, ®s); } + + return success; } bool fault_handle(size_t vcpu_id, microkit_msginfo msginfo) { diff --git a/src/arch/aarch64/vgic/vgic.c b/src/arch/aarch64/vgic/vgic.c index 578598027..fb160566c 100644 --- a/src/arch/aarch64/vgic/vgic.c +++ b/src/arch/aarch64/vgic/vgic.c @@ -97,13 +97,8 @@ bool vgic_inject_irq(size_t vcpu_id, int irq) } // @ivanv: revisit this whole function -bool handle_vgic_dist_fault(size_t vcpu_id, uint64_t fault_addr, uint64_t fsr, seL4_UserContext *regs) +bool handle_vgic_dist_fault(size_t vcpu_id, size_t offset, size_t fsr, seL4_UserContext *regs, void *data) { - /* Make sure that the fault address actually lies within the GIC distributor region. */ - assert(fault_addr >= GIC_DIST_PADDR); - assert(fault_addr - GIC_DIST_PADDR < GIC_DIST_SIZE); - - uint64_t offset = fault_addr - GIC_DIST_PADDR; bool success = false; if (fault_is_read(fsr)) { // printf("VGIC|INFO: Read dist\n"); diff --git a/src/arch/aarch64/vgic/vgic_v3.c b/src/arch/aarch64/vgic/vgic_v3.c index 87507c22d..59e6f9d87 100644 --- a/src/arch/aarch64/vgic/vgic_v3.c +++ b/src/arch/aarch64/vgic/vgic_v3.c @@ -90,9 +90,8 @@ static bool handle_vgic_redist_read_fault(size_t vcpu_id, vgic_t *vgic, uint64_t uintptr_t fault_addr = GIC_REDIST_PADDR + offset; uint32_t mask = fault_get_data_mask(fault_addr, fsr); + fault_emulate_write(regs, fault_addr, fsr, reg & mask); // @ivanv: todo error handling - bool success = fault_advance(vcpu_id, regs, fault_addr, fsr, reg & mask); - assert(success); return true; } @@ -145,17 +144,10 @@ static bool handle_vgic_redist_write_fault(size_t vcpu_id, vgic_t *vgic, uint64_ LOG_VMM_ERR("Unknown register offset 0x%x, value: 0x%x\n", offset, fault_get_data(regs, fsr)); } - bool success = fault_advance_vcpu(vcpu_id, regs); - assert(success); - - return success; + return true; } -bool handle_vgic_redist_fault(size_t vcpu_id, uint64_t fault_addr, uint64_t fsr, seL4_UserContext *regs) { - assert(fault_addr >= GIC_REDIST_PADDR); - uint64_t offset = fault_addr - GIC_REDIST_PADDR; - assert(offset < GIC_REDIST_SIZE); - +bool handle_vgic_redist_fault(size_t vcpu_id, size_t offset, size_t fsr, seL4_UserContext *regs, void *data) { if (fault_is_read(fsr)) { return handle_vgic_redist_read_fault(vcpu_id, &vgic, offset, fsr, regs); } else { diff --git a/src/arch/aarch64/virq.c b/src/arch/aarch64/virq.c index 8f7459a03..96bccc1cb 100644 --- a/src/arch/aarch64/virq.c +++ b/src/arch/aarch64/virq.c @@ -7,6 +7,7 @@ #include #include #include +#include #include /* Maps Microkit channel numbers with registered vIRQ */ @@ -24,8 +25,9 @@ static void vppi_event_ack(size_t vcpu_id, int irq, void *cookie) static void sgi_ack(size_t vcpu_id, int irq, void *cookie) {} bool virq_controller_init(size_t boot_vcpu_id) { + bool success; + vgic_init(); - // @ivanv: todo, do this dynamically instead of compile time? #if defined(GIC_V2) LOG_VMM("initialised virtual GICv2 driver\n"); #elif defined(GIC_V3) @@ -34,7 +36,21 @@ bool virq_controller_init(size_t boot_vcpu_id) { #error "Unsupported GIC version" #endif - bool success = vgic_register_irq(boot_vcpu_id, PPI_VTIMER_IRQ, &vppi_event_ack, NULL); + /* Register the fault handler */ + success = fault_register_vm_exception_handler(GIC_DIST_PADDR, GIC_DIST_SIZE, handle_vgic_dist_fault, NULL); + if (!success) { + LOG_VMM_ERR("Failed to register fault handler for GIC distributor region\n"); + return false; + } +#if defined(GIC_V3) + success = fault_register_vm_exception_handler(GIC_REDIST_PADDR, GIC_REDIST_SIZE, handle_vgic_redist_fault, NULL); + if (!success) { + LOG_VMM_ERR("Failed to register fault handler for GIC redistributor region\n"); + return false; + } +#endif + + success = vgic_register_irq(boot_vcpu_id, PPI_VTIMER_IRQ, &vppi_event_ack, NULL); if (!success) { LOG_VMM_ERR("Failed to register vCPU virtual timer IRQ: 0x%lx\n", PPI_VTIMER_IRQ); return false;