Re: [kvm-commits] KVM: Fix ioapic level-triggered interrupt redelivery

Re: [kvm-commits] KVM: Fix ioapic level-triggered interrupt redelivery

[Dong, Eddie]

kvm-commits-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org wrote:
> repository: /home/avi/kvm/linux-2.6
> branch: (no branch)
> commit 5e7a195fc4b1c0df577658e01a25b91d49bc68ee
> Author: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> Date:   Tue Sep 18 14:19:00 2007 +0200
> 
>    KVM: Fix ioapic level-triggered interrupt redelivery
> 
>    The ioapic failed to set the irr bit if a previous
> interrupt was already
>    being serviced.  This caused interrupt loss fairly soon, leading
>    to loss of level-triggered devices like pic networking.
> 
>    This patch fixes the problem by always setting irr when an
> irq is asserted.
> 
>    Signed-off-by: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> 
> diff --git a/drivers/kvm/ioapic.c b/drivers/kvm/ioapic.c
> index 3ee13c3..b8c7da4 100644
> --- a/drivers/kvm/ioapic.c
> +++ b/drivers/kvm/ioapic.c
> @@ -243,17 +243,10 @@ void kvm_ioapic_set_irq(struct
> kvm_ioapic *ioapic, int irq, int level)
> 		entry = ioapic->redirtbl[irq];
> 		if (!level)
> 			ioapic->irr &= ~mask;
> -		if (entry.fields.trig_mode) {	/* level triggered */
> -			if (level && !entry.fields.remote_irr) {
> -				ioapic->irr |= mask;
> -				ioapic_service(ioapic, irq);
> -			}
> -		} else if (level && !(ioapic->irr & mask)) {
> -			/*
> -			 * edge triggered
> -			 */
> +		else {
> 			ioapic->irr |= mask;
> -			ioapic_service(ioapic, irq);
> +			if (!entry.fields.trig_mode ||
> !entry.fields.remote_irr)
> +				ioapic_service(ioapic, irq);

Good fix for ioapic->irr to indicate the line state. But
I think this one will cause a redunadant edge trigger IRQ. A device 
model may repeat call SET_IRQ_LINE state even w/o state change
 (thus no IRQ), with this logic, a redunadnat IRQ will happen.

if (!entry.fields.trig_mode ||           =====> 
	if ((!entry.fields.trig_mode && (old_irr & mask ==0)) ||


Also architectually level = 0 may also mean an IRQ to IOAPIC
 if the polarity is negative though today we may not see this. 
But this change will expose the risk, and the propose of pass-through
 hardware device will change the polarity.



> 		}
> 	}
> }
> @@ -285,18 +278,8 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int
> 	vector) ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
> 
> 	ent->fields.remote_irr = 0;
> -	/*
> -	 * TODO:
> -	 * qemu ioapic doesn't re-deliver level
> -	 * triggered irq at the time of APIC EOI.
> -	 * Adding back following code sme time causes RHEL5U
> -	 * guest boot no progress at ethernet bringup, so
> -	 * leave it same with qemu for now and revisit later.
> -	 */
> -/*
> 	if (!ent->fields.mask && (ioapic->irr & (1 << gsi)))
> 		ioapic_deliver(ioapic, gsi);
> -*/

The IRQ line should be resampled per spec, but I met problem 
month ago. Now I am wondering it may be caused by incorrect
 ioapic->irr. Your above patch fixed this:-)


thx, eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-commits] KVM: Fix ioapic level-triggered interrupt redelivery

[Avi Kivity]

Dong, Eddie wrote:
>> diff --git a/drivers/kvm/ioapic.c b/drivers/kvm/ioapic.c
>> index 3ee13c3..b8c7da4 100644
>> --- a/drivers/kvm/ioapic.c
>> +++ b/drivers/kvm/ioapic.c
>> @@ -243,17 +243,10 @@ void kvm_ioapic_set_irq(struct
>> kvm_ioapic *ioapic, int irq, int level)
>> 		entry = ioapic->redirtbl[irq];
>> 		if (!level)
>> 			ioapic->irr &= ~mask;
>> -		if (entry.fields.trig_mode) {	/* level triggered */
>> -			if (level && !entry.fields.remote_irr) {
>> -				ioapic->irr |= mask;
>> -				ioapic_service(ioapic, irq);
>> -			}
>> -		} else if (level && !(ioapic->irr & mask)) {
>> -			/*
>> -			 * edge triggered
>> -			 */
>> +		else {
>> 			ioapic->irr |= mask;
>> -			ioapic_service(ioapic, irq);
>> +			if (!entry.fields.trig_mode ||
>> !entry.fields.remote_irr)
>> +				ioapic_service(ioapic, irq);
>>     
>
> Good fix for ioapic->irr to indicate the line state. But
> I think this one will cause a redunadant edge trigger IRQ. A device 
> model may repeat call SET_IRQ_LINE state even w/o state change
>  (thus no IRQ), with this logic, a redunadnat IRQ will happen.
>
> if (!entry.fields.trig_mode ||           =====> 
> 	if ((!entry.fields.trig_mode && (old_irr & mask ==0)) ||
>
>   

Thanks, I'll do that.

> Also architectually level = 0 may also mean an IRQ to IOAPIC
>  if the polarity is negative though today we may not see this. 
> But this change will expose the risk, and the propose of pass-through
>  hardware device will change the polarity.
>   

Sure, if you run Xen + pci passthrough with the polarity reversal on kvm :)

We do want a correct polarity implementation -- I'll do that later on.  
I certainly won't say no to patches...

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-commits] KVM: Fix ioapic level-triggered interrupt redelivery

[Avi Kivity]

Avi Kivity wrote:
>
>> Also architectually level = 0 may also mean an IRQ to IOAPIC
>>  if the polarity is negative though today we may not see this. But 
>> this change will expose the risk, and the propose of pass-through
>>  hardware device will change the polarity.
>>   
>
> Sure, if you run Xen + pci passthrough with the polarity reversal on 
> kvm :)
>
> We do want a correct polarity implementation -- I'll do that later 
> on.  I certainly won't say no to patches...
>

Can't we just do


    level ^= polarity;

at the beginning of the function?  Then reversed polarity interrupts use 
the same code as normal interrupts.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-commits] KVM: Fix ioapic level-triggered interrupt redelivery

[Dong, Eddie]

Avi Kivity wrote:
> Avi Kivity wrote:
>> 
>>> Also architectually level = 0 may also mean an IRQ to IOAPIC
>>>  if the polarity is negative though today we may not see this. But
>>> this change will expose the risk, and the propose of pass-through
>>>  hardware device will change the polarity.
>>> 
>> 
>> Sure, if you run Xen + pci passthrough with the polarity reversal on
>> kvm :) 
>> 
>> We do want a correct polarity implementation -- I'll do that later
>> on.  I certainly won't say no to patches...
>> 
> 
> Can't we just do
> 
> 
>    level ^= polarity;
> 
> at the beginning of the function?  Then reversed polarity interrupts
> use the same code as normal interrupts.
> 
Yes, let ioapic->irr indicate the interrupt line state since we need it
for both level and edge. Then use above logic for the next interrupt
generation logic :-)
thanks, Eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-commits] KVM: Fix ioapic level-triggered interrupt redelivery

[Avi Kivity]

Dong, Eddie wrote:
> Avi Kivity wrote:
>   
>> Avi Kivity wrote:
>>     
>>>> Also architectually level = 0 may also mean an IRQ to IOAPIC
>>>>  if the polarity is negative though today we may not see this. But
>>>> this change will expose the risk, and the propose of pass-through
>>>>  hardware device will change the polarity.
>>>>
>>>>         
>>> Sure, if you run Xen + pci passthrough with the polarity reversal on
>>> kvm :) 
>>>
>>> We do want a correct polarity implementation -- I'll do that later
>>> on.  I certainly won't say no to patches...
>>>
>>>       
>> Can't we just do
>>
>>
>>    level ^= polarity;
>>
>> at the beginning of the function?  Then reversed polarity interrupts
>> use the same code as normal interrupts.
>>
>>     
> Yes, let ioapic->irr indicate the interrupt line state since we need it
> for both level and edge. Then use above logic for the next interrupt
> generation logic :-)
>   

So committed.

Thanks.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/