On 24/10/19 20:14, Sean Christopherson wrote: > On Thu, Oct 24, 2019 at 12:08:29PM +0200, Paolo Bonzini wrote: >> On 24/10/19 04:59, Junaid Shahid wrote: >>> AFAICT the kvm->users_count is already 0 before kvm_arch_destroy_vm() >>> is called from kvm_destroy_vm() in the normal case. >> >> Yes: >> >> if (refcount_dec_and_test(&kvm->users_count)) >> kvm_destroy_vm(kvm); >> >> where >> >> | int atomic_inc_and_test(atomic_t *v); >> | int atomic_dec_and_test(atomic_t *v); >> | >> | These two routines increment and decrement by 1, respectively, the >> | given atomic counter. They return a boolean indicating whether the >> | resulting counter value was zero or not. >> >>> So there really >>> shouldn't be any arch that does a kvm_put_kvm() inside >>> kvm_arch_destroy_vm(). I think it might be better to keep the >>> kvm_arch_destroy_vm() call after the refcount_set() to be consistent >>> with the normal path. >> >> I agree, so I am applying Jim's patch. > > Junaid also pointed out that x86 will dereference a NULL kvm->memslots[]. > >> If anything, we may want to WARN if the refcount is not 1 before the >> refcount_set. > > What about moving "refcount_set(&kvm->users_count, 1)" to right before the > VM is added to vm_list, i.e. after arch code and init'ing the mmu_notifier? > Along with a comment explaining the kvm_get_kvm() is illegal while the VM > is being created. > > That'd eliminate the atmoic_set() in the error path, which is confusing, > at least for me. It'd also obviate the need for an explicit WARN since > running with refcount debugging would immediately flag any arch that > tried to use kvm_get_kvm() during kvm_arch_create_vm(). > > Moving the refcount_set() could be done along with rearranging the memslots > and buses allocation/cleanup in a preparatory patch before adding the call > to kvm_arch_destroy_vm(). Sounds good. Paolo
