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 <[email protected]>

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
         }
     });
 

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");
 

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");
 

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;
 }
 

include/libvmm/arch/aarch64/vgic/vgic.h

@@ -10,16 +10,13 @@
 #include <stdint.h>
 #include <libvmm/virq.h>
 
-// @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);

src/arch/aarch64/fault.c

@@ -360,38 +360,27 @@ 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, &regs);
     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, &regs);
-#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, &regs);
-#endif
-        default: {
-            bool success = fault_handle_registered_vm_exceptions(vcpu_id, addr, fsr, &regs);
-            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, &regs);
-            }
-
-            return success;
-        }
+    bool success = fault_handle_registered_vm_exceptions(vcpu_id, addr, fsr, &regs);
+    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, &regs);
     }
+
+    return success;
 }
 
 bool fault_handle(size_t vcpu_id, microkit_msginfo msginfo) {

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");

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 {

src/arch/aarch64/virq.c

@@ -7,6 +7,7 @@
 #include <microkit.h>
 #include <libvmm/virq.h>
 #include <libvmm/util/util.h>
+#include <libvmm/arch/aarch64/fault.h>
 #include <libvmm/arch/aarch64/vgic/vgic.h>
 
 /* 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;