Re: [PATCH] KVM: Fix RTC interrupt coalescing tracking

[Jan Kiszka <jan.kiszka@web.de>]

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

On 2013-06-26 08:15, Gleb Natapov wrote:
> On Wed, Jun 26, 2013 at 07:49:37AM +0200, Jan Kiszka wrote:
>> On 2013-06-24 14:19, Gleb Natapov wrote:
>>> This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
>>> the commit kvm_apic_set_irq() no longer returns accurate information
>>> about interrupt injection status if injection is done into disabled
>>> APIC. RTC interrupt coalescing tracking relies on the information to be
>>> accurate and cannot recover if it is not.
>>>
>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 9d75193..9f4bea8 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
>>>  	return highest_irr;
>>>  }
>>>  
>>> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> -			      int vector, int level, int trig_mode,
>>> -			      unsigned long *dest_map);
>>> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> +			     int vector, int level, int trig_mode,
>>> +			     unsigned long *dest_map);
>>
>> I still think __apic_accept_irq should unconditionally inject, and the
>> test for acpi_enabled belongs into kvm_apic_set_irq. Why should
>> __apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off?
>> See below for another reason to refactor this part of the interface.
>>
> 10.4.7.2 Local APIC State After It Has Been Software Disabled
> 
> The local APIC will respond normally to INIT, NMI, SMI, and SIPI
> messages.

OK, I see.

> 
>>>  
>>> -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
>>> -		      unsigned long *dest_map)
>>> +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
>>> +		unsigned long *dest_map)
>>>  {
>>>  	struct kvm_lapic *apic = vcpu->arch.apic;
>>>  
>>> -	__apic_accept_irq(apic, irq->delivery_mode, irq->vector,
>>> -			  irq->level, irq->trig_mode, dest_map);
>>> +	return __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
>>> +			irq->level, irq->trig_mode, dest_map);
>>>  }
>>>  
>>>  static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
>>> @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>>>  	*r = -1;
>>>  
>>>  	if (irq->shorthand == APIC_DEST_SELF) {
>>> -		kvm_apic_set_irq(src->vcpu, irq, dest_map);
>>> -		*r = 1;
>>> +		*r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
>>>  		return true;
>>>  	}
>>>  
>>> @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>>>  			continue;
>>>  		if (*r < 0)
>>>  			*r = 0;
>>> -		kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
>>> -		*r += 1;
>>> +		*r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
>>>  	}
>>>  
>>>  	ret = true;
>>> @@ -664,11 +662,15 @@ out:
>>>  	return ret;
>>>  }
>>>  
>>> -/* Set an IRQ pending in the lapic. */
>>> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> -			      int vector, int level, int trig_mode,
>>> -			      unsigned long *dest_map)
>>> +/*
>>> + * Add a pending IRQ into lapic.
>>> + * Return 1 if successfully added and 0 if discarded.
>>> + */
>>> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> +			     int vector, int level, int trig_mode,
>>> +			     unsigned long *dest_map)
>>>  {
>>> +	int result = 0;
>>>  	struct kvm_vcpu *vcpu = apic->vcpu;
>>>  
>>>  	switch (delivery_mode) {
>>> @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>>  		if (dest_map)
>>>  			__set_bit(vcpu->vcpu_id, dest_map);
>>>  
>>> -		if (kvm_x86_ops->deliver_posted_interrupt)
>>> +		if (kvm_x86_ops->deliver_posted_interrupt) {
>>> +			result = 1;
>>>  			kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
>>> -		else {
>>> -			if (apic_test_and_set_irr(vector, apic)) {
>>> +		} else {
>>> +			result = !apic_test_and_set_irr(vector, apic);
>>
>> This part of the revert makes no sense. If deliver_posted_interrupt is
>> on, we don't have this feedback anymore, thus we decided to remove it, no?
>>
> Agree, but I wanted to do clear revert and fix on top.

Fine with me, let's write a separate fix.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]