On Thu, Oct 24, 2019 at 04:03:26PM -0700, Jim Mattson wrote:
> This reorganization will allow us to call kvm_arch_destroy_vm in the
> event that kvm_create_vm fails after calling kvm_arch_init_vm.
>
> Suggested-by: Junaid Shahid <junaids@xxxxxxxxxx>
> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Reviewed-by: Junaid Shahid <junaids@xxxxxxxxxx>
> ---
> virt/kvm/kvm_main.c | 40 +++++++++++++++++++++-------------------
> 1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 525e0dbc623f9..77819597d7e8e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -627,8 +627,9 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
>
> static struct kvm *kvm_create_vm(unsigned long type)
> {
> - int r, i;
> struct kvm *kvm = kvm_arch_alloc_vm();
> + int r = -ENOMEM;
> + int i;
>
> if (!kvm)
> return ERR_PTR(-ENOMEM);
> @@ -642,6 +643,25 @@ static struct kvm *kvm_create_vm(unsigned long type)
> mutex_init(&kvm->slots_lock);
> INIT_LIST_HEAD(&kvm->devices);
>
> + BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
> +
> + for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
> + struct kvm_memslots *slots = kvm_alloc_memslots();
> +
> + if (!slots)
> + goto out_err_no_disable;
> + /* Generations must be different for each address space. */
> + slots->generation = i;
> + rcu_assign_pointer(kvm->memslots[i], slots);
> + }
> +
> + for (i = 0; i < KVM_NR_BUSES; i++) {
> + rcu_assign_pointer(kvm->buses[i],
> + kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT));
> + if (!kvm->buses[i])
> + goto out_err_no_disable;
Personally I'd prefer to add labels for each stage of destruction instead
of abusing the NULL handling of kfree() and kvm_free_memslots(), especially
since not doing so forces the next patch to update these gotos.
Inverting the labels to describe what's being done instead of what's not
being done might help with the readability and naming.
> + }
> +
> r = kvm_arch_init_vm(kvm, type);
> if (r)
> goto out_err_no_disable;
> @@ -654,28 +674,10 @@ static struct kvm *kvm_create_vm(unsigned long type)
> INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list);
> #endif
>
> - BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
> -
> - r = -ENOMEM;
> - for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
> - struct kvm_memslots *slots = kvm_alloc_memslots();
> - if (!slots)
> - goto out_err_no_srcu;
> - /* Generations must be different for each address space. */
> - slots->generation = i;
> - rcu_assign_pointer(kvm->memslots[i], slots);
> - }
> -
> if (init_srcu_struct(&kvm->srcu))
> goto out_err_no_srcu;
> if (init_srcu_struct(&kvm->irq_srcu))
> goto out_err_no_irq_srcu;
> - for (i = 0; i < KVM_NR_BUSES; i++) {
> - rcu_assign_pointer(kvm->buses[i],
> - kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT));
> - if (!kvm->buses[i])
> - goto out_err;
> - }
>
> r = kvm_init_mmu_notifier(kvm);
> if (r)
> --
> 2.24.0.rc0.303.g954a862665-goog
>
