On Fri, 05 Nov 2021 20:12:12 +0000,
Sean Christopherson [off-list ref] wrote:

On Fri, Nov 05, 2021, Marc Zyngier wrote:

quoted

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.

...

quoted

+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.

-- 
Without deviation from the norm, progress is not possible.