kvm_free_lapic() to pair with kvm_create_lapic()

PATCH Applied Rusty Russell 2 participants 4 messages Oct 8, 2007 2 messages not in the archive KVM

Share & tools

Rusty Russell

author

2007-10-08 00:48 UTC #0 lore raw

PATCH 1/3 kvm_free_lapic() to pair with kvm_create_lapic()

Instead of the asymetry of kvm_free_apic, implement kvm_free_lapic(). And guess what? I found a minor bug: we don't need to hrtimer_cancel() from kvm_main.c, because we do that in kvm_free_apic().

Also:
1) kvm_vcpu_uninit should be the reverse order from kvm_vcpu_init.
2) Don't set apic->regs_page to zero before freeing apic.

Signed-off-by Rusty Russell <rusty-8n+1lVoiYb80n/[email protected]>

drivers/kvm/irq.h +1 -1

index 11fc014..508280e 100644
@@ -139,7 +139,7 @@ int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
-void kvm_free_apic(struct kvm_lapic *apic);
+void kvm_free_lapic(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
 void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);

drivers/kvm/kvm_main.c +1 -3

index 2c3986f..f82b5ad 100644
@@ -288,10 +288,8 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
 
 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
 {
+	kvm_free_lapic(vcpu);
 	kvm_mmu_destroy(vcpu);
-	if (vcpu->apic)
-		hrtimer_cancel(&vcpu->apic->timer.dev);
-	kvm_free_apic(vcpu->apic);
 	free_page((unsigned long)vcpu->pio_data);
 	free_page((unsigned long)vcpu->run);
 }

drivers/kvm/lapic.c +9 ~~-10~~

index 6e0f7e5..27e6249 100644
@@ -739,19 +739,17 @@ static int apic_mmio_range(struct kvm_io_device *this, gpa_t addr)
 	return ret;
 }
 
-void kvm_free_apic(struct kvm_lapic *apic)
+void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
-	if (!apic)
+	if (!vcpu->apic)
 		return;
 
-	hrtimer_cancel(&apic->timer.dev);
+	hrtimer_cancel(&vcpu->apic->timer.dev);
 
-	if (apic->regs_page) {
-		__free_page(apic->regs_page);
-		apic->regs_page = 0;
-	}
+	if (vcpu->apic->regs_page)
+		__free_page(vcpu->apic->regs_page);
 
-	kfree(apic);
+	kfree(vcpu->apic);
 }
 
 /*

@@ -939,7 +937,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 	if (apic->regs_page == NULL) {
 		printk(KERN_ERR "malloc apic regs error for vcpu %x\n",
 		       vcpu->vcpu_id);
-		goto nomem;
+		goto nomem_free_apic;
 	}
 	apic->regs = page_address(apic->regs_page);
 	memset(apic->regs, 0, PAGE_SIZE);

@@ -957,8 +955,9 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 	apic->dev.private = apic;
 
 	return 0;
+nomem_free_apic:
+	kfree(apic);
 nomem:
-	kvm_free_apic(apic);
 	return -ENOMEM;
 }
 EXPORT_SYMBOL_GPL(kvm_create_lapic);

-------------------------------------------------------------------------
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/

A message that is not in the archive (sent off-list or not yet received): 200710081048.30985.rusty-8n+1lVoiYb80n/[email protected]

Rusty Russell

author

2007-10-08 00:50 UTC #2 lore raw

PATCH 2/3 Hoist kvm_create_lapic() into kvm_vcpu_init()

Move kvm_create_lapic() into kvm_vcpu_init(), rather than having svm and vmx do it. And make it return the error rather than a fairly random -ENOMEM.

This also solves the problem that neither svm.c nor vmx.c actually handles the error path properly.

Signed-off-by Rusty Russell <rusty-8n+1lVoiYb80n/[email protected]>

drivers/kvm/kvm_main.c +9 -1

index f82b5ad..47f6015 100644
@@ -275,14 +275,22 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	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 -ENOMEM;
+	return r;
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_init);

drivers/kvm/svm.c +0 -6

index a533088..e713f73 100644
@@ -580,12 +580,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 	if (err)
 		goto free_svm;
 
-	if (irqchip_in_kernel(kvm)) {
-		err = kvm_create_lapic(&svm->vcpu);
-		if (err < 0)
-			goto free_svm;
-	}
-
 	page = alloc_page(GFP_KERNEL);
 	if (!page) {
 		err = -ENOMEM;

drivers/kvm/vmx.c +0 -6

index 8606a5a..820f687 100644
@@ -2426,12 +2426,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 	if (err)
 		goto free_vcpu;
 
-	if (irqchip_in_kernel(kvm)) {
-		err = kvm_create_lapic(&vmx->vcpu);
-		if (err < 0)
-			goto free_vcpu;
-	}
-
 	vmx->guest_msrs = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!vmx->guest_msrs) {
 		err = -ENOMEM;

A message that is not in the archive (sent off-list or not yet received): 200710081050.48794.rusty-8n+1lVoiYb80n/[email protected]

Rusty Russell

author

2007-10-08 00:55 UTC #4 lore raw

PATCH 3/3 Remove gratuitous casts from lapic.c

Since vcpu->apic is of the correct type, there's not need to cast. Perhaps this will be required in future?

Signed-off-by Rusty Russell <rusty-8n+1lVoiYb80n/[email protected]>

drivers/kvm/lapic.c +5 -5

index 27e6249..68aa9e4 100644
@@ -172,7 +172,7 @@ static inline int apic_find_highest_irr(struct kvm_lapic *apic)
 
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
+	struct kvm_lapic *apic = vcpu->apic;
 	int highest_irr;
 
 	if (!apic)

@@ -760,7 +760,7 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
 
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8)
 {
-	struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
+	struct kvm_lapic *apic = vcpu->apic;
 
 	if (!apic)
 		return;

@@ -769,7 +769,7 @@ void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8)
 
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
+	struct kvm_lapic *apic = vcpu->apic;
 	u64 tpr;
 
 	if (!apic)

@@ -782,7 +782,7 @@ EXPORT_SYMBOL_GPL(kvm_lapic_get_cr8);
 
 void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 {
-	struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
+	struct kvm_lapic *apic = vcpu->apic;
 
 	if (!apic) {
 		value |= MSR_IA32_APICBASE_BSP;

@@ -859,7 +859,7 @@ EXPORT_SYMBOL_GPL(kvm_lapic_reset);
 
 int kvm_lapic_enabled(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
+	struct kvm_lapic *apic = vcpu->apic;
 	int ret = 0;
 
 	if (!apic)

Avi Kivity

Applied

2007-10-08 08:08 UTC #5 lore raw

In reply to a message not in the archive

Quoted 9 lines · Rusty Russell

Instead of the asymetry of kvm_free_apic, implement kvm_free_lapic(). And guess what? I found a minor bug: we don't need to hrtimer_cancel() from kvm_main.c, because we do that in kvm_free_apic().
Also:
1) kvm_vcpu_uninit should be the reverse order from kvm_vcpu_init.
2) Don't set apic->regs_page to zero before freeing apic.

Applied all three, thanks.

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/