From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jason Wang <jasowang@redhat.com>
Subject: Re: [Qemu-devel] [question] e1000 interrupt storm happenedbecauseofits
 correspondingioapic->irr bit always set
Date: Fri, 29 Aug 2014 10:50:50 +0800
Message-ID: <53FFEA8A.4090507@redhat.com>
References: <201408231836387399956@sangfor.com>, <53FAA874.70703@redhat.com>,	<201408251517235889695@sangfor.com>, <53FAE5EB.8080809@redhat.com>,	<201408261728240882530@sangfor.com>, <53FD681D.7070602@redhat.com>,	<201408271731497879013@sangfor.com> <201408282055150251894@sangfor.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
To: Zhang Haoyu <zhanghy@sangfor.com>,
	qemu-devel <qemu-devel@nongnu.org>, kvm <kvm@vger.kernel.org>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:22491 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750892AbaH2CvB (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 28 Aug 2014 22:51:01 -0400
In-Reply-To: <201408282055150251894@sangfor.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 08/28/2014 08:55 PM, Zhang Haoyu wrote:
> Hi Jason,
> I tested below patch, it's okay, the e1000 interrupt storm disappeared.
> But I am going to make a bit change on it, could you help review it?
>
>> Currently, we call ioapic_service() immediately when we find the irq is still
>> active during eoi broadcast. But for real hardware, there's some dealy between
>> the EOI writing and irq delivery (system bus latency?). So we need to emulate
>> this behavior. Otherwise, for a guest who haven't register a proper irq handler
>> , it would stay in the interrupt routine as this irq would be re-injected
>> immediately after guest enables interrupt. This would lead guest can't move
>> forward and may miss the possibility to get proper irq handler registered (one
>> example is windows guest resuming from hibernation).
>>
>> As there's no way to differ the unhandled irq from new raised ones, this patch
>> solve this problems by scheduling a delayed work when the count of irq injected
>> during eoi broadcast exceeds a threshold value. After this patch, the guest can
>> move a little forward when there's no suitable irq handler in case it may
>> register one very soon and for guest who has a bad irq detection routine ( such
>> as note_interrupt() in linux ), this bad irq would be recognized soon as in the
>> past.
>>
>> Signed-off-by: Jason Wang <jasowang <at> redhat.com>
>> ---
>> virt/kvm/ioapic.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
>> virt/kvm/ioapic.h |    2 ++
>> 2 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index dcaf272..892253e 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> <at>  <at>  -221,6 +221,24  <at>  <at>  int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
>> 	return ret;
>> }
>>
>> +static void kvm_ioapic_eoi_inject_work(struct work_struct *work)
>> +{
>> +	int i, ret;
>> +	struct kvm_ioapic *ioapic = container_of(work, struct kvm_ioapic,
>> +						 eoi_inject.work);
>> +	spin_lock(&ioapic->lock);
>> +	for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>> +		union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i];
>> +
>> +		if (ent->fields.trig_mode != IOAPIC_LEVEL_TRIG)
>> +			continue;
>> +
>> +		if (ioapic->irr & (1 << i) && !ent->fields.remote_irr)
>> +			ret = ioapic_service(ioapic, i);
>> +	}
>> +	spin_unlock(&ioapic->lock);
>> +}
>> +
>> static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
>> 				     int trigger_mode)
>> {
>> <at>  <at>  -249,8 +267,29  <at>  <at>  static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
>>
>> 		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
>> 		ent->fields.remote_irr = 0;
>> -		if (!ent->fields.mask && (ioapic->irr & (1 << i)))
>> -			ioapic_service(ioapic, i);
>> +		if (!ent->fields.mask && (ioapic->irr & (1 << i))) {
>> +			++ioapic->irq_eoi;
> -+			++ioapic->irq_eoi;
> ++		    ++ioapic->irq_eoi[i];
>> +			if (ioapic->irq_eoi == 100) {
> -+			if (ioapic->irq_eoi == 100) {
> ++			if (ioapic->irq_eoi[i] == 100) {
>> +				/*
>> +				 * Real hardware does not deliver the irq so
>> +				 * immediately during eoi broadcast, so we need
>> +				 * to emulate this behavior. Otherwise, for
>> +				 * guests who has not registered handler of a
>> +				 * level irq, this irq would be injected
>> +				 * immediately after guest enables interrupt
>> +				 * (which happens usually at the end of the
>> +				 * common interrupt routine). This would lead
>> +				 * guest can't move forward and may miss the
>> +				 * possibility to get proper irq handler
>> +				 * registered. So we need to give some breath to
>> +				 * guest. TODO: 1 is too long?
>> +				 */
>> +				schedule_delayed_work(&ioapic->eoi_inject, 1);
>> +				ioapic->irq_eoi = 0;
> -+				ioapic->irq_eoi = 0;
> ++				ioapic->irq_eoi[i] = 0;
>> +			} else {
>> +				ioapic_service(ioapic, i);
>> +			}
>> +		}
> ++		else {
> ++			ioapic->irq_eoi[i] = 0;
> ++		}
>> 	}
>> }
> I think ioapic->irq_eoi is prone to reach to 100, because during a eoi broadcast, 
> it's possible that another interrupt's (not current eoi's corresponding interrupt) irr is set, so the ioapic->irq_eoi will grow continually,
> and not too long, ioapic->irq_eoi will reach to 100.
> I want to add "u32 irq_eoi[IOAPIC_NUM_PINS];" instead of "u32 irq_eoi;".
> Any ideas?
>
> Zhang Haoyu

I don't have objection to this per irq counter, but I don't see obvious
difference.

Btw, the patch has been rejected in the past since we're not sure
whether this behaviour is architectural (or ask Intel?). You need
convince the Gleb and other kvm maintainers for this idea first :).