Refactor vcpu creation path.

PATCH In review Hollis Blanchard 2 participants 4 messages Oct 31, 2007 – Nov 1, 2007 KVM

lore

Share & tools

lore

Hollis Blanchard

author

2007-10-31 22:57 UTC #0 lore raw

PATCH 1/1 Refactor vcpu creation path.

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/

Hollis Blanchard

author

2007-10-31 22:57 UTC #1 lore raw

PATCH 1/1 RFC: Create kvm_x86_vcpu_init()

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/[email protected]>

drivers/kvm/kvm.h +3 -0

@@ -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);

drivers/kvm/kvm_main.c +2 ~~-33~~

@@ -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);

drivers/kvm/svm.c +3 -3

@@ -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);
 }

drivers/kvm/vmx.c +3 -3

@@ -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);

drivers/kvm/x86.c +54 -0

57 lines not on this page · raw message

Hollis Blanchard

author

2007-10-31 22:57 UTC #2 lore raw

PATCH 1/1 RFC: Create kvm_arch_vcpu_create()

In reply to 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/[email protected]>

drivers/kvm/kvm.h +3 -0

@@ -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);

drivers/kvm/kvm_main.c +4 ~~-20~~

@@ -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;
 }

drivers/kvm/x86.c +36 -0

@@ -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);
+}

Avi Kivity

2007-11-01 13:20 UTC #3 lore raw

In reply to Hollis Blanchard

Quoted 9 lines · 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.

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

Signature · 8 lines

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/