From bd84d4c217b7a49c4b5ff83864aaa3dcbaba21d4 Mon Sep 17 00:00:00 2001 From: Diogo Costa Date: Mon, 9 Oct 2023 09:47:47 +0100 Subject: [PATCH 1/3] fix(irq): add spinlock to prevent interrupt reservation race condition Signed-off-by: Diogo Costa --- src/core/interrupts.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/core/interrupts.c b/src/core/interrupts.c index 76b625fee..2b5fd2809 100644 --- a/src/core/interrupts.c +++ b/src/core/interrupts.c @@ -12,6 +12,7 @@ BITMAP_ALLOC(hyp_interrupt_bitmap, MAX_INTERRUPTS); BITMAP_ALLOC(global_interrupt_bitmap, MAX_INTERRUPTS); +spinlock_t irq_reserve_lock = SPINLOCK_INITVAL; irq_handler_t interrupt_handlers[MAX_INTERRUPTS]; @@ -83,6 +84,7 @@ enum irq_res interrupts_handle(irqid_t int_id) void interrupts_vm_assign(struct vm *vm, irqid_t id) { + spin_lock(&irq_reserve_lock); if (interrupts_arch_conflict(global_interrupt_bitmap, id)) { ERROR("Interrupts conflict, id = %d\n", id); } @@ -91,13 +93,16 @@ void interrupts_vm_assign(struct vm *vm, irqid_t id) bitmap_set(vm->interrupt_bitmap, id); bitmap_set(global_interrupt_bitmap, id); + spin_unlock(&irq_reserve_lock); } void interrupts_reserve(irqid_t int_id, irq_handler_t handler) { + spin_lock(&irq_reserve_lock); if ((int_id < MAX_INTERRUPTS) && !interrupt_assigned(int_id)) { interrupt_handlers[int_id] = handler; bitmap_set(hyp_interrupt_bitmap, int_id); bitmap_set(global_interrupt_bitmap, int_id); } + spin_unlock(&irq_reserve_lock); } From 6dfb8fbde6f164084387f40f3f88ee9648a55e2e Mon Sep 17 00:00:00 2001 From: Diogo Costa Date: Fri, 6 Oct 2023 18:15:48 +0100 Subject: [PATCH 2/3] fix(irq): include interrupt reservation and assignment check Signed-off-by: Diogo Costa --- src/core/inc/interrupts.h | 4 ++-- src/core/interrupts.c | 26 +++++++++++++++++--------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/core/inc/interrupts.h b/src/core/inc/interrupts.h index 44b105844..c6fddfa8e 100644 --- a/src/core/inc/interrupts.h +++ b/src/core/inc/interrupts.h @@ -16,7 +16,7 @@ struct vm; typedef void (*irq_handler_t)(irqid_t int_id); void interrupts_init(); -void interrupts_reserve(irqid_t int_id, irq_handler_t handler); +bool interrupts_reserve(irqid_t int_id, irq_handler_t handler); void interrupts_cpu_sendipi(cpuid_t target_cpu, irqid_t ipi_id); void interrupts_cpu_enable(irqid_t int_id, bool en); @@ -27,7 +27,7 @@ void interrupts_clear(irqid_t int_id); enum irq_res { HANDLED_BY_HYP, FORWARD_TO_VM }; enum irq_res interrupts_handle(irqid_t int_id); -void interrupts_vm_assign(struct vm *vm, irqid_t id); +bool interrupts_vm_assign(struct vm *vm, irqid_t id); /* Must be implemented by architecture */ diff --git a/src/core/interrupts.c b/src/core/interrupts.c index 2b5fd2809..a6e901869 100644 --- a/src/core/interrupts.c +++ b/src/core/interrupts.c @@ -82,27 +82,35 @@ enum irq_res interrupts_handle(irqid_t int_id) } } -void interrupts_vm_assign(struct vm *vm, irqid_t id) +bool interrupts_vm_assign(struct vm *vm, irqid_t id) { - spin_lock(&irq_reserve_lock); - if (interrupts_arch_conflict(global_interrupt_bitmap, id)) { - ERROR("Interrupts conflict, id = %d\n", id); - } + bool ret = false; - interrupts_arch_vm_assign(vm, id); + spin_lock(&irq_reserve_lock); + if (!interrupts_arch_conflict(global_interrupt_bitmap, id)) { + ret = true; + interrupts_arch_vm_assign(vm, id); - bitmap_set(vm->interrupt_bitmap, id); - bitmap_set(global_interrupt_bitmap, id); + bitmap_set(vm->interrupt_bitmap, id); + bitmap_set(global_interrupt_bitmap, id); + } spin_unlock(&irq_reserve_lock); + + return ret; } -void interrupts_reserve(irqid_t int_id, irq_handler_t handler) +bool interrupts_reserve(irqid_t int_id, irq_handler_t handler) { + bool ret = false; + spin_lock(&irq_reserve_lock); if ((int_id < MAX_INTERRUPTS) && !interrupt_assigned(int_id)) { + ret = true; interrupt_handlers[int_id] = handler; bitmap_set(hyp_interrupt_bitmap, int_id); bitmap_set(global_interrupt_bitmap, int_id); } spin_unlock(&irq_reserve_lock); + + return ret; } From ae4badd4fef01685e6c40b0e7c4cbf13f7506630 Mon Sep 17 00:00:00 2001 From: Diogo Costa Date: Thu, 12 Oct 2023 09:21:12 +0100 Subject: [PATCH 3/3] feat(verbose): improve error handling for interrupt reservation Signed-off-by: Diogo Costa --- src/arch/armv8/gic.c | 6 ++++-- src/arch/riscv/iommu.c | 5 ++++- src/arch/riscv/sbi.c | 4 +++- src/core/interrupts.c | 4 +++- src/core/vm.c | 4 +++- 5 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/arch/armv8/gic.c b/src/arch/armv8/gic.c index e95075dbd..f2027a6af 100644 --- a/src/arch/armv8/gic.c +++ b/src/arch/armv8/gic.c @@ -68,8 +68,10 @@ void gicd_init() /* No need to setup gicd->NSACR as all interrupts are setup to group 1 */ - interrupts_reserve(platform.arch.gic.maintenance_id, - gic_maintenance_handler); + if(!interrupts_reserve(platform.arch.gic.maintenance_id, + gic_maintenance_handler)) { + ERROR("Failed to reserve GIC maintenance interrupt"); + } } void gic_map_mmio(); diff --git a/src/arch/riscv/iommu.c b/src/arch/riscv/iommu.c index d901ad344..494fa9c9c 100644 --- a/src/arch/riscv/iommu.c +++ b/src/arch/riscv/iommu.c @@ -334,7 +334,10 @@ void rv_iommu_init(void) rv_iommu.hw.reg_ptr->fqh = 0; // Allocate IRQ for FQ - interrupts_reserve(platform.arch.iommu.fq_irq_id, rv_iommu_fq_irq_handler); + if(!interrupts_reserve(platform.arch.iommu.fq_irq_id, rv_iommu_fq_irq_handler)) { + ERROR("Failed to reserve IOMMU FQ interrupt"); + } + interrupts_cpu_enable(platform.arch.iommu.fq_irq_id, true); // Enable FQ (fqcsr) diff --git a/src/arch/riscv/sbi.c b/src/arch/riscv/sbi.c index 101b9bdae..96522cae0 100644 --- a/src/arch/riscv/sbi.c +++ b/src/arch/riscv/sbi.c @@ -476,5 +476,7 @@ void sbi_init() } } - interrupts_reserve(TIMR_INT_ID, sbi_timer_irq_handler); + if(!interrupts_reserve(TIMR_INT_ID, sbi_timer_irq_handler)) { + ERROR("Failed to reserve SBI TIMR_INT_ID interrupt"); + } } diff --git a/src/core/interrupts.c b/src/core/interrupts.c index a6e901869..96f381126 100644 --- a/src/core/interrupts.c +++ b/src/core/interrupts.c @@ -41,7 +41,9 @@ inline void interrupts_init() interrupts_arch_init(); if (cpu()->id == CPU_MASTER) { - interrupts_reserve(IPI_CPU_MSG, cpu_msg_handler); + if(!interrupts_reserve(IPI_CPU_MSG, cpu_msg_handler)) { + ERROR("Failed to reserve IPI_CPU_MSG interrupt"); + } } interrupts_cpu_enable(IPI_CPU_MSG, true); diff --git a/src/core/vm.c b/src/core/vm.c index d5d105035..a4c853bab 100644 --- a/src/core/vm.c +++ b/src/core/vm.c @@ -207,7 +207,9 @@ static void vm_init_dev(struct vm* vm, const struct vm_config* config) } for (size_t j = 0; j < dev->interrupt_num; j++) { - interrupts_vm_assign(vm, dev->interrupts[j]); + if(!interrupts_vm_assign(vm, dev->interrupts[j])) { + ERROR("Failed to assign interrupt id %d", dev->interrupts[j]); + } } }