| Message ID | 20211105192101.3862492-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: Turn the vcpu array into an xarray | expand |
On Fri, Nov 05, 2021, Marc Zyngier wrote: > All architectures have similar loops iterating over the vcpus, > freeing one vcpu at a time, and eventually wiping the reference > off the vcpus array. They are also inconsistently taking > the kvm->lock mutex when wiping the references from the array. ... > +void kvm_destroy_vcpus(struct kvm *kvm) > +{ > + unsigned int i; > + struct kvm_vcpu *vcpu; > + > + kvm_for_each_vcpu(i, vcpu, kvm) > + kvm_vcpu_destroy(vcpu); > + > + mutex_lock(&kvm->lock); But why is kvm->lock taken here? Unless I'm overlooking an arch, everyone calls this from kvm_arch_destroy_vm(), in which case this is the only remaining reference to @kvm. And if there's some magic path for which that's not true, I don't see how it can possibly be safe to call kvm_vcpu_destroy() without holding kvm->lock, or how this would guarantee that all vCPUs have actually been destroyed before nullifying the array. > + for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) > + kvm->vcpus[i] = NULL; > + > + atomic_set(&kvm->online_vcpus, 0); > + mutex_unlock(&kvm->lock); > +} > +EXPORT_SYMBOL_GPL(kvm_destroy_vcpus);
On Fri, 05 Nov 2021 20:12:12 +0000, Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Nov 05, 2021, Marc Zyngier wrote: > > All architectures have similar loops iterating over the vcpus, > > freeing one vcpu at a time, and eventually wiping the reference > > off the vcpus array. They are also inconsistently taking > > the kvm->lock mutex when wiping the references from the array. > > ... > > > +void kvm_destroy_vcpus(struct kvm *kvm) > > +{ > > + unsigned int i; > > + struct kvm_vcpu *vcpu; > > + > > + kvm_for_each_vcpu(i, vcpu, kvm) > > + kvm_vcpu_destroy(vcpu); > > + > > + mutex_lock(&kvm->lock); > > But why is kvm->lock taken here? Unless I'm overlooking an arch, > everyone calls this from kvm_arch_destroy_vm(), in which case this > is the only remaining reference to @kvm. And if there's some magic > path for which that's not true, I don't see how it can possibly be > safe to call kvm_vcpu_destroy() without holding kvm->lock, or how > this would guarantee that all vCPUs have actually been destroyed > before nullifying the array. I asked myself the same question two years ago, and couldn't really understand the requirement. However, x86 does just that, so I preserved the behaviour. If you too believe that this is just wrong, I'm happy to drop the locking altogether. If that breaks someone's flow, they'll shout soon enough. Thanks, M.
On Fri, 5 Nov 2021 19:20:57 +0000 Marc Zyngier <maz@kernel.org> wrote: > All architectures have similar loops iterating over the vcpus, > freeing one vcpu at a time, and eventually wiping the reference > off the vcpus array. They are also inconsistently taking > the kvm->lock mutex when wiping the references from the array. > > Make this code common, which will simplify further changes. > > Signed-off-by: Marc Zyngier <maz@kernel.org> no objections Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > arch/arm64/kvm/arm.c | 10 +--------- > arch/mips/kvm/mips.c | 21 +-------------------- > arch/powerpc/kvm/powerpc.c | 10 +--------- > arch/riscv/kvm/vm.c | 10 +--------- > arch/s390/kvm/kvm-s390.c | 18 +----------------- > arch/x86/kvm/x86.c | 9 +-------- > include/linux/kvm_host.h | 2 +- > virt/kvm/kvm_main.c | 20 ++++++++++++++++++-- > 8 files changed, 25 insertions(+), 75 deletions(-) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index f5490afe1ebf..75bb7215da03 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -175,19 +175,11 @@ vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > */ > void kvm_arch_destroy_vm(struct kvm *kvm) > { > - int i; > - > bitmap_free(kvm->arch.pmu_filter); > > kvm_vgic_destroy(kvm); > > - for (i = 0; i < KVM_MAX_VCPUS; ++i) { > - if (kvm->vcpus[i]) { > - kvm_vcpu_destroy(kvm->vcpus[i]); > - kvm->vcpus[i] = NULL; > - } > - } > - atomic_set(&kvm->online_vcpus, 0); > + kvm_destroy_vcpus(kvm); > } > > int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > index 562aa878b266..ceacca74f808 100644 > --- a/arch/mips/kvm/mips.c > +++ b/arch/mips/kvm/mips.c > @@ -171,25 +171,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > return 0; > } > > -void kvm_mips_free_vcpus(struct kvm *kvm) > -{ > - unsigned int i; > - struct kvm_vcpu *vcpu; > - > - kvm_for_each_vcpu(i, vcpu, kvm) { > - kvm_vcpu_destroy(vcpu); > - } > - > - mutex_lock(&kvm->lock); > - > - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) > - kvm->vcpus[i] = NULL; > - > - atomic_set(&kvm->online_vcpus, 0); > - > - mutex_unlock(&kvm->lock); > -} > - > static void kvm_mips_free_gpa_pt(struct kvm *kvm) > { > /* It should always be safe to remove after flushing the whole range */ > @@ -199,7 +180,7 @@ static void kvm_mips_free_gpa_pt(struct kvm *kvm) > > void kvm_arch_destroy_vm(struct kvm *kvm) > { > - kvm_mips_free_vcpus(kvm); > + kvm_destroy_vcpus(kvm); > kvm_mips_free_gpa_pt(kvm); > } > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index 35e9cccdeef9..492e4a4121cb 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -463,9 +463,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > void kvm_arch_destroy_vm(struct kvm *kvm) > { > - unsigned int i; > - struct kvm_vcpu *vcpu; > - > #ifdef CONFIG_KVM_XICS > /* > * We call kick_all_cpus_sync() to ensure that all > @@ -476,14 +473,9 @@ void kvm_arch_destroy_vm(struct kvm *kvm) > kick_all_cpus_sync(); > #endif > > - kvm_for_each_vcpu(i, vcpu, kvm) > - kvm_vcpu_destroy(vcpu); > + kvm_destroy_vcpus(kvm); > > mutex_lock(&kvm->lock); > - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) > - kvm->vcpus[i] = NULL; > - > - atomic_set(&kvm->online_vcpus, 0); > > kvmppc_core_destroy_vm(kvm); > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c > index 26399df15b63..6af6cde295eb 100644 > --- a/arch/riscv/kvm/vm.c > +++ b/arch/riscv/kvm/vm.c > @@ -46,15 +46,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > void kvm_arch_destroy_vm(struct kvm *kvm) > { > - int i; > - > - for (i = 0; i < KVM_MAX_VCPUS; ++i) { > - if (kvm->vcpus[i]) { > - kvm_vcpu_destroy(kvm->vcpus[i]); > - kvm->vcpus[i] = NULL; > - } > - } > - atomic_set(&kvm->online_vcpus, 0); > + kvm_destroy_vcpus(kvm); > } > > int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index c6257f625929..7af53b8788fa 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2819,27 +2819,11 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu) > free_page((unsigned long)(vcpu->arch.sie_block)); > } > > -static void kvm_free_vcpus(struct kvm *kvm) > -{ > - unsigned int i; > - struct kvm_vcpu *vcpu; > - > - kvm_for_each_vcpu(i, vcpu, kvm) > - kvm_vcpu_destroy(vcpu); > - > - mutex_lock(&kvm->lock); > - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) > - kvm->vcpus[i] = NULL; > - > - atomic_set(&kvm->online_vcpus, 0); > - mutex_unlock(&kvm->lock); > -} > - > void kvm_arch_destroy_vm(struct kvm *kvm) > { > u16 rc, rrc; > > - kvm_free_vcpus(kvm); > + kvm_destroy_vcpus(kvm); > sca_dispose(kvm); > kvm_s390_gisa_destroy(kvm); > /* > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index c1c4e2b05a63..498a43126615 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -11302,15 +11302,8 @@ static void kvm_free_vcpus(struct kvm *kvm) > kvm_clear_async_pf_completion_queue(vcpu); > kvm_unload_vcpu_mmu(vcpu); > } > - kvm_for_each_vcpu(i, vcpu, kvm) > - kvm_vcpu_destroy(vcpu); > - > - mutex_lock(&kvm->lock); > - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) > - kvm->vcpus[i] = NULL; > > - atomic_set(&kvm->online_vcpus, 0); > - mutex_unlock(&kvm->lock); > + kvm_destroy_vcpus(kvm); > } > > void kvm_arch_sync_events(struct kvm *kvm) > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 60a35d9fe259..36967291b8c6 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -725,7 +725,7 @@ static inline struct kvm_vcpu *kvm_get_vcpu_by_id(struct kvm *kvm, int id) > if (WARN_ON_ONCE(!memslot->npages)) { \ > } else > > -void kvm_vcpu_destroy(struct kvm_vcpu *vcpu); > +void kvm_destroy_vcpus(struct kvm *kvm); > > void vcpu_load(struct kvm_vcpu *vcpu); > void vcpu_put(struct kvm_vcpu *vcpu); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 3f6d450355f0..d83553eeea21 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -435,7 +435,7 @@ static void kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id) > vcpu->last_used_slot = 0; > } > > -void kvm_vcpu_destroy(struct kvm_vcpu *vcpu) > +static void kvm_vcpu_destroy(struct kvm_vcpu *vcpu) > { > kvm_dirty_ring_free(&vcpu->dirty_ring); > kvm_arch_vcpu_destroy(vcpu); > @@ -450,7 +450,23 @@ void kvm_vcpu_destroy(struct kvm_vcpu *vcpu) > free_page((unsigned long)vcpu->run); > kmem_cache_free(kvm_vcpu_cache, vcpu); > } > -EXPORT_SYMBOL_GPL(kvm_vcpu_destroy); > + > +void kvm_destroy_vcpus(struct kvm *kvm) > +{ > + unsigned int i; > + struct kvm_vcpu *vcpu; > + > + kvm_for_each_vcpu(i, vcpu, kvm) > + kvm_vcpu_destroy(vcpu); > + > + mutex_lock(&kvm->lock); > + for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) > + kvm->vcpus[i] = NULL; > + > + atomic_set(&kvm->online_vcpus, 0); > + mutex_unlock(&kvm->lock); > +} > +EXPORT_SYMBOL_GPL(kvm_destroy_vcpus); > > #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER) > static inline struct kvm *mmu_notifier_to_kvm(struct mmu_notifier *mn)
On 11/6/21 12:17, Marc Zyngier wrote: > > If you too believe that this is just wrong, I'm happy to drop the > locking altogether. If that breaks someone's flow, they'll shout soon > enough. Yes, it's not necessary. It was added in 2009 (commit 988a2cae6a3c, "KVM: Use macro to iterate over vcpus.") and it was unnecessary back then too. Paolo
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index f5490afe1ebf..75bb7215da03 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -175,19 +175,11 @@ vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) */ void kvm_arch_destroy_vm(struct kvm *kvm) { - int i; - bitmap_free(kvm->arch.pmu_filter); kvm_vgic_destroy(kvm); - for (i = 0; i < KVM_MAX_VCPUS; ++i) { - if (kvm->vcpus[i]) { - kvm_vcpu_destroy(kvm->vcpus[i]); - kvm->vcpus[i] = NULL; - } - } - atomic_set(&kvm->online_vcpus, 0); + kvm_destroy_vcpus(kvm); } int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c index 562aa878b266..ceacca74f808 100644 --- a/arch/mips/kvm/mips.c +++ b/arch/mips/kvm/mips.c @@ -171,25 +171,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) return 0; } -void kvm_mips_free_vcpus(struct kvm *kvm) -{ - unsigned int i; - struct kvm_vcpu *vcpu; - - kvm_for_each_vcpu(i, vcpu, kvm) { - kvm_vcpu_destroy(vcpu); - } - - mutex_lock(&kvm->lock); - - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) - kvm->vcpus[i] = NULL; - - atomic_set(&kvm->online_vcpus, 0); - - mutex_unlock(&kvm->lock); -} - static void kvm_mips_free_gpa_pt(struct kvm *kvm) { /* It should always be safe to remove after flushing the whole range */ @@ -199,7 +180,7 @@ static void kvm_mips_free_gpa_pt(struct kvm *kvm) void kvm_arch_destroy_vm(struct kvm *kvm) { - kvm_mips_free_vcpus(kvm); + kvm_destroy_vcpus(kvm); kvm_mips_free_gpa_pt(kvm); } diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 35e9cccdeef9..492e4a4121cb 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -463,9 +463,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) void kvm_arch_destroy_vm(struct kvm *kvm) { - unsigned int i; - struct kvm_vcpu *vcpu; - #ifdef CONFIG_KVM_XICS /* * We call kick_all_cpus_sync() to ensure that all @@ -476,14 +473,9 @@ void kvm_arch_destroy_vm(struct kvm *kvm) kick_all_cpus_sync(); #endif - kvm_for_each_vcpu(i, vcpu, kvm) - kvm_vcpu_destroy(vcpu); + kvm_destroy_vcpus(kvm); mutex_lock(&kvm->lock); - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) - kvm->vcpus[i] = NULL; - - atomic_set(&kvm->online_vcpus, 0); kvmppc_core_destroy_vm(kvm); diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c index 26399df15b63..6af6cde295eb 100644 --- a/arch/riscv/kvm/vm.c +++ b/arch/riscv/kvm/vm.c @@ -46,15 +46,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) void kvm_arch_destroy_vm(struct kvm *kvm) { - int i; - - for (i = 0; i < KVM_MAX_VCPUS; ++i) { - if (kvm->vcpus[i]) { - kvm_vcpu_destroy(kvm->vcpus[i]); - kvm->vcpus[i] = NULL; - } - } - atomic_set(&kvm->online_vcpus, 0); + kvm_destroy_vcpus(kvm); } int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index c6257f625929..7af53b8788fa 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2819,27 +2819,11 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu) free_page((unsigned long)(vcpu->arch.sie_block)); } -static void kvm_free_vcpus(struct kvm *kvm) -{ - unsigned int i; - struct kvm_vcpu *vcpu; - - kvm_for_each_vcpu(i, vcpu, kvm) - kvm_vcpu_destroy(vcpu); - - mutex_lock(&kvm->lock); - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) - kvm->vcpus[i] = NULL; - - atomic_set(&kvm->online_vcpus, 0); - mutex_unlock(&kvm->lock); -} - void kvm_arch_destroy_vm(struct kvm *kvm) { u16 rc, rrc; - kvm_free_vcpus(kvm); + kvm_destroy_vcpus(kvm); sca_dispose(kvm); kvm_s390_gisa_destroy(kvm); /* diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c1c4e2b05a63..498a43126615 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -11302,15 +11302,8 @@ static void kvm_free_vcpus(struct kvm *kvm) kvm_clear_async_pf_completion_queue(vcpu); kvm_unload_vcpu_mmu(vcpu); } - kvm_for_each_vcpu(i, vcpu, kvm) - kvm_vcpu_destroy(vcpu); - - mutex_lock(&kvm->lock); - for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) - kvm->vcpus[i] = NULL; - atomic_set(&kvm->online_vcpus, 0); - mutex_unlock(&kvm->lock); + kvm_destroy_vcpus(kvm); } void kvm_arch_sync_events(struct kvm *kvm) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 60a35d9fe259..36967291b8c6 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -725,7 +725,7 @@ static inline struct kvm_vcpu *kvm_get_vcpu_by_id(struct kvm *kvm, int id) if (WARN_ON_ONCE(!memslot->npages)) { \ } else -void kvm_vcpu_destroy(struct kvm_vcpu *vcpu); +void kvm_destroy_vcpus(struct kvm *kvm); void vcpu_load(struct kvm_vcpu *vcpu); void vcpu_put(struct kvm_vcpu *vcpu); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3f6d450355f0..d83553eeea21 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -435,7 +435,7 @@ static void kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id) vcpu->last_used_slot = 0; } -void kvm_vcpu_destroy(struct kvm_vcpu *vcpu) +static void kvm_vcpu_destroy(struct kvm_vcpu *vcpu) { kvm_dirty_ring_free(&vcpu->dirty_ring); kvm_arch_vcpu_destroy(vcpu); @@ -450,7 +450,23 @@ void kvm_vcpu_destroy(struct kvm_vcpu *vcpu) free_page((unsigned long)vcpu->run); kmem_cache_free(kvm_vcpu_cache, vcpu); } -EXPORT_SYMBOL_GPL(kvm_vcpu_destroy); + +void kvm_destroy_vcpus(struct kvm *kvm) +{ + unsigned int i; + struct kvm_vcpu *vcpu; + + kvm_for_each_vcpu(i, vcpu, kvm) + kvm_vcpu_destroy(vcpu); + + mutex_lock(&kvm->lock); + for (i = 0; i < atomic_read(&kvm->online_vcpus); i++) + kvm->vcpus[i] = NULL; + + atomic_set(&kvm->online_vcpus, 0); + mutex_unlock(&kvm->lock); +} +EXPORT_SYMBOL_GPL(kvm_destroy_vcpus); #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER) static inline struct kvm *mmu_notifier_to_kvm(struct mmu_notifier *mn)
All architectures have similar loops iterating over the vcpus, freeing one vcpu at a time, and eventually wiping the reference off the vcpus array. They are also inconsistently taking the kvm->lock mutex when wiping the references from the array. Make this code common, which will simplify further changes. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/arm.c | 10 +--------- arch/mips/kvm/mips.c | 21 +-------------------- arch/powerpc/kvm/powerpc.c | 10 +--------- arch/riscv/kvm/vm.c | 10 +--------- arch/s390/kvm/kvm-s390.c | 18 +----------------- arch/x86/kvm/x86.c | 9 +-------- include/linux/kvm_host.h | 2 +- virt/kvm/kvm_main.c | 20 ++++++++++++++++++-- 8 files changed, 25 insertions(+), 75 deletions(-)