[RFC] lapic3: remove the union of user/kernel irq in struct vcpu

[RFC] lapic3: remove the union of user/kernel irq in struct vcpu

[He, Qing]

Avi,
	We have found a new bug about irqchip_in_kernel checking, as
addressed by the following patch:

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 88b0b89..69518f8 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -283,7 +283,8 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
 {
 	kvm_mmu_destroy(vcpu);
-	kvm_free_apic(vcpu->apic);
+	if (irqchip_in_kernel(vcpu->kvm))
+		kvm_free_apic(vcpu->apic);
 	free_page((unsigned long)vcpu->pio_data);
 	free_page((unsigned long)vcpu->run);
 }

	This is similar with what we try to fix several days ago. We
could certainly fix them as they appear, however, I'm beginning to
wonder, is the union of user/kernel irq really worthwhile? With this
union, we may save some space, but there is cost. We have to do run-time
checking which is time consuming, and there are potentially more bugs
which will eventually bite us someday. If we remove it, making
irq_summary and kernel apic not disturbing each other, the checking is
saved, and we don't have to worry about future bugs caused by
irq_summary, apic sharing any more.

	How do you think?

Thanks,
Qing

-------------------------------------------------------------------------
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: [RFC] lapic3: remove the union of user/kernel irq in struct vcpu

[Avi Kivity]

He, Qing wrote:
> Avi,
> 	We have found a new bug about irqchip_in_kernel checking, as
> addressed by the following patch:
>
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 88b0b89..69518f8 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -283,7 +283,8 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
>  void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
>  {
>  	kvm_mmu_destroy(vcpu);
> -	kvm_free_apic(vcpu->apic);
> +	if (irqchip_in_kernel(vcpu->kvm))
> +		kvm_free_apic(vcpu->apic);
>  	free_page((unsigned long)vcpu->pio_data);
>  	free_page((unsigned long)vcpu->run);
>  }
>
> 	This is similar with what we try to fix several days ago. We
> could certainly fix them as they appear, however, I'm beginning to
> wonder, is the union of user/kernel irq really worthwhile? With this
> union, we may save some space, but there is cost. We have to do run-time
> checking which is time consuming, and there are potentially more bugs
> which will eventually bite us someday. If we remove it, making
> irq_summary and kernel apic not disturbing each other, the checking is
> saved, and we don't have to worry about future bugs caused by
> irq_summary, apic sharing any more.
>
>   

I'm not opposed to the removal of the union (the space savings are 
hardly significant).  But will it really remove a source of bugs or just 
hide them deeper?  You'll need the tests most of the time anyway.

I'll apply a union-removing patch if you send one.


-------------------------------------------------------------------------
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: [RFC] lapic3: remove the union of user/kernel irq in struct vcpu

[He, Qing]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]



>-----Original Message-----
>From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>Sent: 2007年8月9日 22:28
>To: He, Qing
>Cc: Dong, Eddie; kvm-devel
>Subject: Re: [RFC] lapic3: remove the union of user/kernel irq in struct vcpu
>
>He, Qing wrote:
>> Avi,
>> 	We have found a new bug about irqchip_in_kernel checking, as
>> addressed by the following patch:
>>
>> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
>> index 88b0b89..69518f8 100644
>> --- a/drivers/kvm/kvm_main.c
>> +++ b/drivers/kvm/kvm_main.c
>> @@ -283,7 +283,8 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
>>  void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
>>  {
>>  	kvm_mmu_destroy(vcpu);
>> -	kvm_free_apic(vcpu->apic);
>> +	if (irqchip_in_kernel(vcpu->kvm))
>> +		kvm_free_apic(vcpu->apic);
>>  	free_page((unsigned long)vcpu->pio_data);
>>  	free_page((unsigned long)vcpu->run);
>>  }
>>
>> 	This is similar with what we try to fix several days ago. We
>> could certainly fix them as they appear, however, I'm beginning to
>> wonder, is the union of user/kernel irq really worthwhile? With this
>> union, we may save some space, but there is cost. We have to do run-time
>> checking which is time consuming, and there are potentially more bugs
>> which will eventually bite us someday. If we remove it, making
>> irq_summary and kernel apic not disturbing each other, the checking is
>> saved, and we don't have to worry about future bugs caused by
>> irq_summary, apic sharing any more.
>>
>>
>
>I'm not opposed to the removal of the union (the space savings are
>hardly significant).  But will it really remove a source of bugs or just
>hide them deeper?  You'll need the tests most of the time anyway.

Yes, some bugs may be hidden deeper, but, once this union is removed, they are simply not that significant. If there is sufficient checking about kernel/user irqchip at interrupt injection time, the `bugs' should have no impact whatsoever. At least, it won't crash the kernel...

>
>I'll apply a union-removing patch if you send one.


[-- Attachment #2: Type: text/plain, Size: 315 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel