MMIO-bus unregistration may synchronize SRCU and must run outside config_lock. Conversely, clearing the redistributor assignment needs config_lock, and teardown must do so before freeing the redistributor regions.
Introduce an already-locked unassignment primitive that only clears the cached region and base address. It deliberately does not adjust free_index: failure rollback resets all region counters, while VM teardown frees the regions. Keep MMIO-bus unregistration separate. Unregister devices before taking config_lock in VM teardown, then unassign the vCPUs before freeing their regions. Move redistributor cleanup out of __kvm_vgic_vcpu_destroy() and into its outer wrapper. This preserves failed-vCPU cleanup before its memory can be freed, without the special conditional in the common destructor. The region destructor no longer needs to scan the vCPUs. Suggested-by: Marc Zyngier <[email protected]> Assisted-by: Codex:gpt-5.6-sol Signed-off-by: Karl Mehltretter <[email protected]> --- arch/arm64/kvm/vgic/vgic-init.c | 46 +++++++++++------------------- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 16 ++--------- arch/arm64/kvm/vgic/vgic.h | 1 + 3 files changed, 21 insertions(+), 42 deletions(-) diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c index 0a3df6d3a691..a0d72b540331 100644 --- a/arch/arm64/kvm/vgic/vgic-init.c +++ b/arch/arm64/kvm/vgic/vgic-init.c @@ -523,29 +523,6 @@ static void __kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu) INIT_LIST_HEAD(&vgic_cpu->ap_list_head); kfree(vgic_cpu->private_irqs); vgic_cpu->private_irqs = NULL; - - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { - /* - * If this vCPU is being destroyed because of a failed creation - * then unregister the redistributor to avoid leaving behind a - * dangling pointer to the vCPU struct. - * - * vCPUs that have been successfully created (i.e. added to - * kvm->vcpu_array) get unregistered in kvm_vgic_destroy(), as - * this function gets called while holding kvm->arch.config_lock - * in the VM teardown path and would otherwise introduce a lock - * inversion w.r.t. kvm->srcu. - * - * vCPUs that failed creation are torn down outside of the - * kvm->arch.config_lock and do not get unregistered in - * kvm_vgic_destroy(), meaning it is both safe and necessary to - * do so here. - */ - if (kvm_get_vcpu_by_id(vcpu->kvm, vcpu->vcpu_id) != vcpu) - vgic_unregister_redist_iodev(vcpu); - - vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF; - } } void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu) @@ -553,7 +530,16 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu) struct kvm *kvm = vcpu->kvm; mutex_lock(&kvm->slots_lock); + if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) + vgic_unregister_redist_iodev(vcpu); + __kvm_vgic_vcpu_destroy(vcpu); + + if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { + mutex_lock(&kvm->arch.config_lock); + __vgic_unassign_redist_iodev(vcpu); + mutex_unlock(&kvm->arch.config_lock); + } mutex_unlock(&kvm->slots_lock); } @@ -563,21 +549,23 @@ void kvm_vgic_destroy(struct kvm *kvm) unsigned long i; mutex_lock(&kvm->slots_lock); + if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) + kvm_for_each_vcpu(i, vcpu, kvm) + vgic_unregister_redist_iodev(vcpu); + mutex_lock(&kvm->arch.config_lock); vgic_debug_destroy(kvm); - kvm_for_each_vcpu(i, vcpu, kvm) + kvm_for_each_vcpu(i, vcpu, kvm) { __kvm_vgic_vcpu_destroy(vcpu); + if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) + __vgic_unassign_redist_iodev(vcpu); + } kvm_vgic_dist_destroy(kvm); mutex_unlock(&kvm->arch.config_lock); - - if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) - kvm_for_each_vcpu(i, vcpu, kvm) - vgic_unregister_redist_iodev(vcpu); - mutex_unlock(&kvm->slots_lock); } diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c index 6c009deb11d4..dc860178105d 100644 --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c @@ -775,8 +775,7 @@ static void vgic_undo_redist_assignment(struct kvm_vcpu *vcpu) guard(mutex)(&vcpu->kvm->arch.config_lock); vgic_cpu->rdreg->free_index--; - vgic_cpu->rdreg = NULL; - vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF; + __vgic_unassign_redist_iodev(vcpu); } /** @@ -855,7 +854,7 @@ void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu) kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &rd_dev->dev); } -static void vgic_reset_redist_iodev(struct kvm_vcpu *vcpu) +void __vgic_unassign_redist_iodev(struct kvm_vcpu *vcpu) { struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; @@ -881,7 +880,7 @@ static void vgic_v3_rollback_redist_region(struct kvm *kvm, u32 index) guard(mutex)(&kvm->arch.config_lock); kvm_for_each_vcpu(c, vcpu, kvm) - vgic_reset_redist_iodev(vcpu); + __vgic_unassign_redist_iodev(vcpu); list_for_each_entry(iter, &kvm->arch.vgic.rd_regions, list) iter->free_index = 0; @@ -991,17 +990,8 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index, void vgic_v3_free_redist_region(struct kvm *kvm, struct vgic_redist_region *rdreg) { - struct kvm_vcpu *vcpu; - unsigned long c; - lockdep_assert_held(&kvm->arch.config_lock); - /* Garbage collect the region */ - kvm_for_each_vcpu(c, vcpu, kvm) { - if (vcpu->arch.vgic_cpu.rdreg == rdreg) - vcpu->arch.vgic_cpu.rdreg = NULL; - } - list_del(&rdreg->list); kfree(rdreg); } diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h index b71d486ae514..1a2e40004a47 100644 --- a/arch/arm64/kvm/vgic/vgic.h +++ b/arch/arm64/kvm/vgic/vgic.h @@ -350,6 +350,7 @@ int vgic_v3_save_pending_tables(struct kvm *kvm); int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count); int vgic_register_redist_iodev(struct kvm_vcpu *vcpu); void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu); +void __vgic_unassign_redist_iodev(struct kvm_vcpu *vcpu); bool vgic_v3_check_base(struct kvm *kvm); void vgic_v3_load(struct kvm_vcpu *vcpu); -- 2.39.5 (Apple Git-154)
