[PATCH 0 of 2] Refactor vcpu creation path.

[PATCH 0 of 2] Refactor vcpu creation path.

[Hollis Blanchard]

These patches are not simple code movement, and I haven't yet tested them so
I'm just putting them out for comment.

Basically we're expanding the VCPU creation path to add an "arch" layer between
main.c and svm/vmx.c. Since there is one call in each direction (main.c ->
svm/vmx.c, svm/vmx.c -> main.c) we end up with two new architecture functions
in the middle.

5 files changed, 108 insertions(+), 59 deletions(-)
drivers/kvm/kvm.h      |    6 +++
drivers/kvm/kvm_main.c |   59 +++----------------------------
drivers/kvm/svm.c      |    6 +--
drivers/kvm/vmx.c      |    6 +--
drivers/kvm/x86.c      |   90 ++++++++++++++++++++++++++++++++++++++++++++++++

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[PATCH 1 of 2] RFC: Create kvm_x86_vcpu_init()

[Hollis Blanchard]

5 files changed, 65 insertions(+), 39 deletions(-)
drivers/kvm/kvm.h      |    3 ++
drivers/kvm/kvm_main.c |   35 +------------------------------
drivers/kvm/svm.c      |    6 ++---
drivers/kvm/vmx.c      |    6 ++---
drivers/kvm/x86.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++


Create kvm_x86_vcpu_init(), moving x86 code out of kvm_main.c.

Old code flow:
	kvm_vm_ioctl_create_vcpu()
	kvm_x86_ops->vcpu_create()
	kvm_vcpu_init()

New code flow:
	kvm_vm_ioctl_create_vcpu()
	kvm_x86_ops->vcpu_create()
	kvm_x86_vcpu_init()
	kvm_vcpu_init()

Only build-tested! Comments?

Signed-off-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -490,6 +490,9 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu,
 int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id);
 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
 
+int kvm_x86_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id);
+void kvm_x86_vcpu_uninit(struct kvm_vcpu *vcpu);
+
 void vcpu_load(struct kvm_vcpu *vcpu);
 void vcpu_put(struct kvm_vcpu *vcpu);
 
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -127,13 +127,8 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu,
 
 	mutex_init(&vcpu->mutex);
 	vcpu->cpu = -1;
-	vcpu->mmu.root_hpa = INVALID_PAGE;
 	vcpu->kvm = kvm;
 	vcpu->vcpu_id = id;
-	if (!irqchip_in_kernel(kvm) || id == 0)
-		vcpu->mp_state = VCPU_MP_STATE_RUNNABLE;
-	else
-		vcpu->mp_state = VCPU_MP_STATE_UNINITIALIZED;
 	init_waitqueue_head(&vcpu->wq);
 
 	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
@@ -143,31 +138,8 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu,
 	}
 	vcpu->run = page_address(page);
 
-	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
-	if (!page) {
-		r = -ENOMEM;
-		goto fail_free_run;
-	}
-	vcpu->pio_data = page_address(page);
-
-	r = kvm_mmu_create(vcpu);
-	if (r < 0)
-		goto fail_free_pio_data;
-
-	if (irqchip_in_kernel(kvm)) {
-		r = kvm_create_lapic(vcpu);
-		if (r < 0)
-			goto fail_mmu_destroy;
-	}
-
-	return 0;
-
-fail_mmu_destroy:
-	kvm_mmu_destroy(vcpu);
-fail_free_pio_data:
-	free_page((unsigned long)vcpu->pio_data);
-fail_free_run:
-	free_page((unsigned long)vcpu->run);
+	return 0;
+
 fail:
 	return r;
 }
@@ -175,9 +147,6 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
 
 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
 {
-	kvm_free_lapic(vcpu);
-	kvm_mmu_destroy(vcpu);
-	free_page((unsigned long)vcpu->pio_data);
 	free_page((unsigned long)vcpu->run);
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_uninit);
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -578,7 +578,7 @@ static struct kvm_vcpu *svm_create_vcpu(
 		goto out;
 	}
 
-	err = kvm_vcpu_init(&svm->vcpu, kvm, id);
+	err = kvm_x86_vcpu_init(&svm->vcpu, kvm, id);
 	if (err)
 		goto free_svm;
 
@@ -604,7 +604,7 @@ static struct kvm_vcpu *svm_create_vcpu(
 	return &svm->vcpu;
 
 uninit:
-	kvm_vcpu_uninit(&svm->vcpu);
+	kvm_x86_vcpu_uninit(&svm->vcpu);
 free_svm:
 	kmem_cache_free(kvm_vcpu_cache, svm);
 out:
@@ -616,7 +616,7 @@ static void svm_free_vcpu(struct kvm_vcp
 	struct vcpu_svm *svm = to_svm(vcpu);
 
 	__free_page(pfn_to_page(svm->vmcb_pa >> PAGE_SHIFT));
-	kvm_vcpu_uninit(vcpu);
+	kvm_x86_vcpu_uninit(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, svm);
 }
 
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -2496,7 +2496,7 @@ static void vmx_free_vcpu(struct kvm_vcp
 	vmx_free_vmcs(vcpu);
 	kfree(vmx->host_msrs);
 	kfree(vmx->guest_msrs);
-	kvm_vcpu_uninit(vcpu);
+	kvm_x86_vcpu_uninit(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, vmx);
 }
 
@@ -2509,7 +2509,7 @@ static struct kvm_vcpu *vmx_create_vcpu(
 	if (!vmx)
 		return ERR_PTR(-ENOMEM);
 
-	err = kvm_vcpu_init(&vmx->vcpu, kvm, id);
+	err = kvm_x86_vcpu_init(&vmx->vcpu, kvm, id);
 	if (err)
 		goto free_vcpu;
 
@@ -2546,7 +2546,7 @@ free_guest_msrs:
 free_guest_msrs:
 	kfree(vmx->guest_msrs);
 uninit_vcpu:
-	kvm_vcpu_uninit(&vmx->vcpu);
+	kvm_x86_vcpu_uninit(&vmx->vcpu);
 free_vcpu:
 	kmem_cache_free(kvm_vcpu_cache, vmx);
 	return ERR_PTR(err);
diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
--- a/drivers/kvm/x86.c
+++ b/drivers/kvm/x86.c
@@ -2323,3 +2323,57 @@ void kvm_put_guest_fpu(struct kvm_vcpu *
 	fx_restore(&vcpu->host_fx_image);
 }
 EXPORT_SYMBOL_GPL(kvm_put_guest_fpu);
+
+int kvm_x86_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
+{
+	struct page *page;
+	int r;
+
+	r = kvm_vcpu_init(vcpu, kvm, id);
+	if (r < 0)
+		goto fail;
+
+	vcpu->mmu.root_hpa = INVALID_PAGE;
+
+	if (!irqchip_in_kernel(kvm) || id == 0)
+		vcpu->mp_state = VCPU_MP_STATE_RUNNABLE;
+	else
+		vcpu->mp_state = VCPU_MP_STATE_UNINITIALIZED;
+
+	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+	if (!page) {
+		r = -ENOMEM;
+		goto fail_free_run;
+	}
+	vcpu->pio_data = page_address(page);
+
+	r = kvm_mmu_create(vcpu);
+	if (r < 0)
+		goto fail_free_pio_data;
+
+	if (irqchip_in_kernel(kvm)) {
+		r = kvm_create_lapic(vcpu);
+		if (r < 0)
+			goto fail_mmu_destroy;
+	}
+
+	return 0;
+
+fail_mmu_destroy:
+	kvm_mmu_destroy(vcpu);
+fail_free_pio_data:
+	free_page((unsigned long)vcpu->pio_data);
+fail_free_run:
+	free_page((unsigned long)vcpu->run);
+fail:
+	return r;
+}
+EXPORT_SYMBOL_GPL(kvm_x86_vcpu_init);
+
+void kvm_x86_vcpu_uninit(struct kvm_vcpu *vcpu)
+{
+	kvm_free_lapic(vcpu);
+	kvm_mmu_destroy(vcpu);
+	free_page((unsigned long)vcpu->pio_data);
+}
+EXPORT_SYMBOL_GPL(kvm_x86_vcpu_uninit);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[PATCH 2 of 2] RFC: Create kvm_arch_vcpu_create()

[Hollis Blanchard]

3 files changed, 43 insertions(+), 20 deletions(-)
drivers/kvm/kvm.h      |    3 +++
drivers/kvm/kvm_main.c |   24 ++++--------------------
drivers/kvm/x86.c      |   36 ++++++++++++++++++++++++++++++++++++


Create kvm_arch_vcpu_create(), moving more x86 code out of kvm_main.c.

Old code flow (from patch 1):
	kvm_vm_ioctl_create_vcpu()
	kvm_x86_ops->vcpu_create()
	kvm_x86_vcpu_init()
	kvm_vcpu_init()

New code flow:
	kvm_vm_ioctl_create_vcpu()
	kvm_arch_vcpu_create()
	kvm_x86_ops->vcpu_create()
	kvm_x86_vcpu_init()
	kvm_vcpu_init()

Only build-tested, and the error-handling paths are a little tricky! Comments?

Signed-off-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -490,6 +490,9 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu,
 int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id);
 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
 
+struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, int n);
+void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu);
+
 int kvm_x86_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id);
 void kvm_x86_vcpu_uninit(struct kvm_vcpu *vcpu);
 
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -852,28 +852,17 @@ static int kvm_vm_ioctl_create_vcpu(stru
 	if (!valid_vcpu(n))
 		return -EINVAL;
 
-	vcpu = kvm_x86_ops->vcpu_create(kvm, n);
+	vcpu = kvm_arch_vcpu_create(kvm, n);
 	if (IS_ERR(vcpu))
 		return PTR_ERR(vcpu);
 
 	preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops);
-
-	/* We do fxsave: this must be aligned. */
-	BUG_ON((unsigned long)&vcpu->host_fx_image & 0xF);
-
-	vcpu_load(vcpu);
-	r = kvm_x86_ops->vcpu_reset(vcpu);
-	if (r == 0)
-		r = kvm_mmu_setup(vcpu);
-	vcpu_put(vcpu);
-	if (r < 0)
-		goto free_vcpu;
 
 	mutex_lock(&kvm->lock);
 	if (kvm->vcpus[n]) {
 		r = -EEXIST;
 		mutex_unlock(&kvm->lock);
-		goto mmu_unload;
+		goto destroy_vcpu;
 	}
 	kvm->vcpus[n] = vcpu;
 	mutex_unlock(&kvm->lock);
@@ -889,13 +878,8 @@ unlink:
 	kvm->vcpus[n] = NULL;
 	mutex_unlock(&kvm->lock);
 
-mmu_unload:
-	vcpu_load(vcpu);
-	kvm_mmu_unload(vcpu);
-	vcpu_put(vcpu);
-
-free_vcpu:
-	kvm_x86_ops->vcpu_free(vcpu);
+destroy_vcpu:
+	kvm_arch_vcpu_destroy(vcpu);
 	return r;
 }
 
diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
--- a/drivers/kvm/x86.c
+++ b/drivers/kvm/x86.c
@@ -2377,3 +2377,39 @@ void kvm_x86_vcpu_uninit(struct kvm_vcpu
 	free_page((unsigned long)vcpu->pio_data);
 }
 EXPORT_SYMBOL_GPL(kvm_x86_vcpu_uninit);
+
+struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, int n)
+{
+	struct kvm_vcpu *vcpu;
+	int r;
+
+	vcpu = kvm_x86_ops->vcpu_create(kvm, n);
+	if (IS_ERR(vcpu))
+		return vcpu;
+
+	/* We do fxsave: this must be aligned. */
+	BUG_ON((unsigned long)&vcpu->host_fx_image & 0xF);
+
+	vcpu_load(vcpu);
+	r = kvm_x86_ops->vcpu_reset(vcpu);
+	if (r == 0)
+		r = kvm_mmu_setup(vcpu);
+	vcpu_put(vcpu);
+	if (r < 0)
+		goto free_vcpu;
+
+	return vcpu;
+
+free_vcpu:
+	kvm_x86_ops->vcpu_free(vcpu);
+	return ERR_PTR(r);
+}
+
+void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
+{
+	vcpu_load(vcpu);
+	kvm_mmu_unload(vcpu);
+	vcpu_put(vcpu);
+
+	kvm_x86_ops->vcpu_free(vcpu);
+}

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH 0 of 2] Refactor vcpu creation path.

[Avi Kivity]

Hollis Blanchard wrote:
> These patches are not simple code movement, and I haven't yet tested them so
> I'm just putting them out for comment.
>
> Basically we're expanding the VCPU creation path to add an "arch" layer between
> main.c and svm/vmx.c. Since there is one call in each direction (main.c ->
> svm/vmx.c, svm/vmx.c -> main.c) we end up with two new architecture functions
> in the middle.
>
>   

The approach looks good.  Will apply once they've been run tested and 
the failure paths reviewed (only gave them a cursory look now).

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/