e1000 interrupt storm happened because of its corresponding ioapic->irr bit always set

question Zhang Haoyu 4 participants 18 messages Aug 23, 2014 – Sep 4, 2014 KVM

Share & tools

Zhang Haoyu

author

2014-08-23 10:36 UTC #0 lore raw

Hi, all

I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().

Any ideas?

Thanks,
Zhang Haoyu

Jason Wang

2014-08-25 03:07 UTC #1 lore raw

Quoted 6 lines · Zhang Haoyu

Hi, all
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Any ideas?

We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).

That was probably caused by something wrong in e1000 emulation which causes interrupt to be injected into windows guest before its interrupt handler is registered. And Windows guest does not have a mechanism to detect and disable irq in such condition.

e1000 emulation is far from stable and complete (e.g run e1000 ethtool selftest in linux guest may see lots of errors). It's complicate and subtle (even has undocumented registers and behaviour). You should better consider to use virtio which are more stable and fast in a kvm guest (unless some intel guys are involved to improve e1000 emulation).

Thanks

Quoted 5 lines

Thanks,
Zhang Haoyu

Zhang Haoyu

author

2014-08-25 07:19 UTC #2 lore raw

Re: [question] e1000 interrupt storm happened becauseof its corresponding ioapic->irr bit always set

Quoted 11 lines

Quoted 6 lines
Hi, all
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).

Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007
http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351
which one tries to fix this problem, or all of them?

Quoted 5 lines

That was probably caused by something wrong in e1000 emulation which causes interrupt to be injected into windows guest before its interrupt handler is registered. And Windows guest does not have a mechanism to detect and disable irq in such condition.

Sorry, I don't understand,
I think one interrupt should not been enabled before its handler is successfully registered, is it possible that e1000 emulation inject the interrupt before the interrupt is succesfully enabled?

Thanks,
Zhang Haoyu

Quoted 12 lines

e1000 emulation is far from stable and complete (e.g run e1000 ethtool selftest in linux guest may see lots of errors). It's complicate and subtle (even has undocumented registers and behaviour). You should better consider to use virtio which are more stable and fast in a kvm guest (unless some intel guys are involved to improve e1000 emulation).
Thanks
Quoted 5 lines
Thanks,
Zhang Haoyu

Jason Wang

2014-08-25 07:29 UTC #3 lore raw

Re: [question] e1000 interrupt storm happened becauseof its corresponding ioapic->irr bit always set

Quoted 5 lines · Zhang Haoyu

Quoted 5 lines
Quoted 5 lines
Hi, all
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).
Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007

This series is the first try to fix the guest hang during guest hibernation or driver enable/disable.

Quoted 2 lines

http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351

Those are follow-up that tries to fix the bugs introduced by the autoneg hack.

Quoted 1 line

which one tries to fix this problem, or all of them?

As you can see, those kinds of hacking may not as good as we expect since we don't know exactly how e1000 works. Only the register function description from Intel's manual may not be sufficient. And you can search e1000 in the archives and you can find some behaviour of e1000 registers were not fictionalized like what spec said. It was really suggested to use virtio-net instead of e1000 in guest.

Quoted 28 lines

Quoted 6 lines
That was probably caused by something wrong in e1000 emulation which causes interrupt to be injected into windows guest before its interrupt handler is registered. And Windows guest does not have a mechanism to detect and disable irq in such condition.
Sorry, I don't understand,
I think one interrupt should not been enabled before its handler is successfully registered, is it possible that e1000 emulation inject the interrupt before the interrupt is succesfully enabled?
Thanks,
Zhang Haoyu
Quoted 11 lines
e1000 emulation is far from stable and complete (e.g run e1000 ethtool selftest in linux guest may see lots of errors). It's complicate and subtle (even has undocumented registers and behaviour). You should better consider to use virtio which are more stable and fast in a kvm guest (unless some intel guys are involved to improve e1000 emulation).
Thanks
Quoted 4 lines
Thanks,
Zhang Haoyu
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Zhang Haoyu

author

2014-08-25 08:29 UTC #4 lore raw

Re: [question] e1000 interrupt storm happened becauseof its correspondingioapic->irr bit always set

Quoted 21 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Hi, all
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).
Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007
This series is the first try to fix the guest hang during guest hibernation or driver enable/disable.
Quoted 2 lines
http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351
Those are follow-up that tries to fix the bugs introduced by the autoneg hack.

Quoted 8 lines

Quoted 1 line
which one tries to fix this problem, or all of them?
As you can see, those kinds of hacking may not as good as we expect since we don't know exactly how e1000 works. Only the register function description from Intel's manual may not be sufficient. And you can search e1000 in the archives and you can find some behaviour of e1000 registers were not fictionalized like what spec said. It was really suggested to use virtio-net instead of e1000 in guest.

We support both, virtio-net is the recommended option,
with regard to some guest (e.g., windows server 2000), virtio-net is not supported, e1000 is the last option.

Quoted 22 lines

Quoted 22 lines
Quoted 6 lines
That was probably caused by something wrong in e1000 emulation which causes interrupt to be injected into windows guest before its interrupt handler is registered. And Windows guest does not have a mechanism to detect and disable irq in such condition.
Sorry, I don't understand,
I think one interrupt should not been enabled before its handler is successfully registered, is it possible that e1000 emulation inject the interrupt before the interrupt is succesfully enabled?
Thanks,
Zhang Haoyu
Quoted 9 lines
e1000 emulation is far from stable and complete (e.g run e1000 ethtool selftest in linux guest may see lots of errors). It's complicate and subtle (even has undocumented registers and behaviour). You should better consider to use virtio which are more stable and fast in a kvm guest (unless some intel guys are involved to improve e1000 emulation).
Thanks
Quoted 2 lines
Thanks,
Zhang Haoyu

Zhang Haoyu

author

2014-08-26 09:30 UTC #5 lore raw

Re: [question] e1000 interrupt storm happened becauseof its correspondingioapic->irr bit always set

In reply to Jason Wang

Quoted 21 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Hi, all
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).
Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007
This series is the first try to fix the guest hang during guest hibernation or driver enable/disable.
Quoted 2 lines
http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351
Those are follow-up that tries to fix the bugs introduced by the autoneg hack.

Quoted 9 lines

Quoted 1 line
which one tries to fix this problem, or all of them?
As you can see, those kinds of hacking may not as good as we expect since we don't know exactly how e1000 works. Only the register function description from Intel's manual may not be sufficient. And you can search e1000 in the archives and you can find some behaviour of e1000 registers were not fictionalized like what spec said. It was really suggested to use virtio-net instead of e1000 in guest.
Quoted 1 line

Will the "[PATCH] kvm: ioapic: conditionally delay irq delivery during eoi broadcast" add delay to virtual interrupt injection sometimes, then some time delay sensitive applications will be impacted?

Thanks,
Zhang Haoyu

Jason Wang

2014-08-27 05:09 UTC #6 lore raw

Re: [question] e1000 interrupt storm happened becauseof its correspondingioapic->irr bit always set

Quoted 20 lines · Zhang Haoyu

Quoted 20 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Hi, all
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).
Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007
This series is the first try to fix the guest hang during guest hibernation or driver enable/disable.
Quoted 2 lines
http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351
Those are follow-up that tries to fix the bugs introduced by the autoneg hack.
Quoted 1 line
which one tries to fix this problem, or all of them?

Quoted 8 lines

Quoted 6 lines
As you can see, those kinds of hacking may not as good as we expect since we don't know exactly how e1000 works. Only the register function description from Intel's manual may not be sufficient. And you can search e1000 in the archives and you can find some behaviour of e1000 registers were not fictionalized like what spec said. It was really suggested to use virtio-net instead of e1000 in guest.
Will the "[PATCH] kvm: ioapic: conditionally delay irq delivery during eoi broadcast" add delay to virtual interrupt injection sometimes, then some time delay sensitive applications will be impacted?

I don't test it too much but it only give a minor delay of 1% irq in the hope of guest irq handler will be registered shortly. But I suspect it's the bug of e1000 who inject the irq in the wrong time. Under what cases did you meet this issue?

Quoted 5 lines

Thanks,
Zhang Haoyu

Zhang Haoyu

author

2014-08-27 09:34 UTC #7 lore raw

Re: [Qemu-devel] [question] e1000 interrupt storm happened becauseofits correspondingioapic->irr bit always set

Quoted 26 lines

Quoted 26 lines
Quoted 26 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Hi, all
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).
Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007
This series is the first try to fix the guest hang during guest hibernation or driver enable/disable.
Quoted 2 lines
http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351
Those are follow-up that tries to fix the bugs introduced by the autoneg hack.
Quoted 1 line
which one tries to fix this problem, or all of them?
As you can see, those kinds of hacking may not as good as we expect since we don't know exactly how e1000 works. Only the register function description from Intel's manual may not be sufficient. And you can search e1000 in the archives and you can find some behaviour of e1000 registers were not fictionalized like what spec said. It was really suggested to use virtio-net instead of e1000 in guest.

Quoted 7 lines

Quoted 2 lines
Will the "[PATCH] kvm: ioapic: conditionally delay irq delivery during eoi broadcast" add delay to virtual interrupt injection sometimes, then some time delay sensitive applications will be impacted?
I don't test it too much but it only give a minor delay of 1% irq in the hope of guest irq handler will be registered shortly. But I suspect it's the bug of e1000 who inject the irq in the wrong time. Under what cases did you meet this issue?

Some scenarios, not constant and 100% reproducity,
e.g., reboot vm, ifdown e1000 nic, install kaspersky(network configuration is performed during installing stage), .etc.

Thanks,
Zhang Haoyu

Quoted 3 lines

Quoted 3 lines
Thanks,
Zhang Haoyu

Jason Wang

2014-08-28 07:12 UTC #8 lore raw

Re: [Qemu-devel] [question] e1000 interrupt storm happened becauseofits correspondingioapic->irr bit always set

Quoted 29 lines · Zhang Haoyu

Quoted 29 lines
Quoted 29 lines
Quoted 28 lines
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
Hi, all
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm,
because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).
Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007
This series is the first try to fix the guest hang during guest hibernation or driver enable/disable.
Quoted 2 lines
Quoted 2 lines
http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351
Those are follow-up that tries to fix the bugs introduced by the autoneg hack.
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
which one tries to fix this problem, or all of them?
As you can see, those kinds of hacking may not as good as we expect since we don't know exactly how e1000 works. Only the register function description from Intel's manual may not be sufficient. And you can search e1000 in the archives and you can find some behaviour of e1000 registers were not fictionalized like what spec said. It was really suggested to use virtio-net instead of e1000 in guest.
Will the "[PATCH] kvm: ioapic: conditionally delay irq delivery during eoi broadcast" add delay to virtual interrupt injection sometimes, then some time delay sensitive applications will be impacted?

Quoted 6 lines

Quoted 4 lines
Quoted 4 lines
I don't test it too much but it only give a minor delay of 1% irq in the hope of guest irq handler will be registered shortly. But I suspect it's the bug of e1000 who inject the irq in the wrong time. Under what cases did you meet this issue?
Some scenarios, not constant and 100% reproducity,
e.g., reboot vm, ifdown e1000 nic, install kaspersky(network configuration is performed during installing stage), .etc.

If you want to start the debugging, I suggest to enable e1000 debug and then analysis the log before the interrupt storm. This may help to locate the issue.

Thanks

Zhang Haoyu

author

2014-08-28 12:57 UTC #9 lore raw

Re: [Qemu-devel] [question] e1000 interrupt storm happenedbecauseofits correspondingioapic->irr bit always set

In reply to Zhang Haoyu

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?

Quoted 56 lines

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(-)
virt/kvm/ioapic.c +0 -0
index dcaf272..892253e 100644
<at> <at> -221,6 +221,24 <at> <at> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
	return ret;
}
virt/kvm/ioapic.c +18 -0
+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;

virt/kvm/ioapic.c

Quoted 4 lines

virt/kvm/ioapic.c +2 -2

-		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];

virt/kvm/ioapic.c

Quoted 1 line

virt/kvm/ioapic.c +1 -0
+			if (ioapic->irq_eoi == 100) {

-+ if (ioapic->irq_eoi == 100) {
++ if (ioapic->irq_eoi[i] == 100) {

Quoted 6 lines

virt/kvm/ioapic.c +6 -0

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

virt/kvm/ioapic.c

Quoted 10 lines

virt/kvm/ioapic.c +10 -0

+				 * 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;

virt/kvm/ioapic.c

Quoted 4 lines

virt/kvm/ioapic.c +4 -0

+			} else {
+				ioapic_service(ioapic, i);
+			}
+		}

++ else {
++ ioapic->irq_eoi[i] = 0;
++ }

Quoted 2 lines

}
}

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

virt/kvm/ioapic.c

Quoted 13 lines

<at> <at> -375,12 +414,14 <at> <at> void kvm_ioapic_reset(struct kvm_ioapic *ioapic)
{
	int i;
virt/kvm/ioapic.c +1 -0
+	cancel_delayed_work_sync(&ioapic->eoi_inject);
	for (i = 0; i < IOAPIC_NUM_PINS; i++)
		ioapic->redirtbl[i].fields.mask = 1;
	ioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
	ioapic->ioregsel = 0;
	ioapic->irr = 0;
	ioapic->id = 0;
virt/kvm/ioapic.c +1 -0
+	ioapic->irq_eoi = 0;

-+ ioapic->irq_eoi = 0;
++ memset(ioapic->irq_eoi, 0x00, IOAPIC_NUM_PINS);

Quoted 27 lines

	update_handled_vectors(ioapic);
}

<at> <at> -398,6 +439,7 <at> <at> int kvm_ioapic_init(struct kvm *kvm)

	if (!ioapic)
		return -ENOMEM;
	spin_lock_init(&ioapic->lock);

virt/kvm/ioapic.c +1 -0

+	INIT_DELAYED_WORK(&ioapic->eoi_inject, kvm_ioapic_eoi_inject_work);

	kvm->arch.vioapic = ioapic;
	kvm_ioapic_reset(ioapic);
	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
<at>  <at>  -418,6 +460,7  <at>  <at>  void kvm_ioapic_destroy(struct kvm *kvm)
{
	struct kvm_ioapic *ioapic = kvm->arch.vioapic;

virt/kvm/ioapic.c +1 -0

+	cancel_delayed_work_sync(&ioapic->eoi_inject);

	if (ioapic) {
		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
		kvm->arch.vioapic = NULL;

virt/kvm/ioapic.h +0 -0

index 0b190c3..8938e66 100644

<at>  <at>  -47,6 +47,8  <at>  <at>  struct kvm_ioapic {
	void (*ack_notifier)(void *opaque, int irq);
	spinlock_t lock;
	DECLARE_BITMAP(handled_vectors, 256);

virt/kvm/ioapic.h

Quoted 2 lines

virt/kvm/ioapic.h +2 -0

+	struct delayed_work eoi_inject;
+	u32 irq_eoi;

-+ u32 irq_eoi;
++ u32 irq_eoi[IOAPIC_NUM_PINS];

Quoted 3 lines

};
#ifdef DEBUG

Jason Wang

2014-08-29 02:51 UTC #10 lore raw

Re: [Qemu-devel] [question] e1000 interrupt storm happenedbecauseofits correspondingioapic->irr bit always set

Quoted 96 lines · Zhang Haoyu

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?
Quoted 56 lines
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(-)
virt/kvm/ioapic.c +0 -0
index dcaf272..892253e 100644
<at> <at> -221,6 +221,24 <at> <at> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
	return ret;
}
virt/kvm/ioapic.c +18 -0
+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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +2 -2
-		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];
virt/kvm/ioapic.c
Quoted 1 line
virt/kvm/ioapic.c +1 -0
+			if (ioapic->irq_eoi == 100) {
-+ if (ioapic->irq_eoi == 100) {
++ if (ioapic->irq_eoi[i] == 100) {
Quoted 6 lines
virt/kvm/ioapic.c +6 -0
+				/*
+				 * 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
virt/kvm/ioapic.c
Quoted 10 lines
virt/kvm/ioapic.c +10 -0
+				 * 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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +4 -0
+			} else {
+				ioapic_service(ioapic, i);
+			}
+		}
++ else {
++ ioapic->irq_eoi[i] = 0;
++ }
Quoted 2 lines
	}
}

Quoted 7 lines

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 :).

Zhang Haoyu

author

2014-08-29 03:16 UTC #11 lore raw

Re: [Qemu-devel] [question] e1000 interrupt storm happenedbecauseofitscorrespondingioapic->irr bit always set

In reply to Zhang Haoyu

Hi, Yang, Gleb, Michael,
Could you help review below patch please?

Thanks,
Zhang Haoyu

Quoted 110 lines

Quoted 96 lines
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?
Quoted 56 lines
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(-)
virt/kvm/ioapic.c +0 -0
index dcaf272..892253e 100644
<at> <at> -221,6 +221,24 <at> <at> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
	return ret;
}
virt/kvm/ioapic.c +18 -0
+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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +2 -2
-		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];
virt/kvm/ioapic.c
Quoted 1 line
virt/kvm/ioapic.c +1 -0
+			if (ioapic->irq_eoi == 100) {
-+ if (ioapic->irq_eoi == 100) {
++ if (ioapic->irq_eoi[i] == 100) {
Quoted 6 lines
virt/kvm/ioapic.c +6 -0
+				/*
+				 * 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
virt/kvm/ioapic.c
Quoted 10 lines
virt/kvm/ioapic.c +10 -0
+				 * 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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +4 -0
+			} else {
+				ioapic_service(ioapic, i);
+			}
+		}
++ else {
++ ioapic->irq_eoi[i] = 0;
++ }
Quoted 2 lines
	}
}
Quoted 7 lines
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 :).

Zhang, Yang Z

2014-08-29 04:07 UTC #12 lore raw

Re: [question] e1000 interrupt storm happenedbecauseofitscorrespondingioapic->irr bit always set

Zhang Haoyu wrote on 2014-08-29:

Quoted 2 lines

Hi, Yang, Gleb, Michael,
Could you help review below patch please?

I don't quite understand the background. Why ioacpi->irr is setting before EOI? It should be driver's responsibility to clear the interrupt before issuing EOI.

Quoted 103 lines

Thanks,
Zhang Haoyu
Quoted 99 lines
Quoted 84 lines
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?
Quoted 44 lines
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(-)
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +0 -0
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; }
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +2 -0
+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);
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +1 -0
+	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;
virt/kvm/ioapic.c index
Quoted 4 lines
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +2 -2
-		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];
virt/kvm/ioapic.c index
Quoted 1 line
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +1 -0
+			if (ioapic->irq_eoi == 100) {
-+ if (ioapic->irq_eoi == 100) {
++ if (ioapic->irq_eoi[i] == 100) {
Quoted 6 lines
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +6 -0
+				/*
+				 * 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
virt/kvm/ioapic.c index
Quoted 10 lines
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +10 -0
+				 * 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;
virt/kvm/ioapic.c index
Quoted 4 lines
virt/kvm/ioapic.c index renamed from virt/kvm/ioapic.c +4 -0
+			} else {
+				ioapic_service(ioapic, i);
+			}
+		}
++ else {
++ ioapic->irq_eoi[i] = 0;
++ }
Quoted 2 lines
	}
}
Quoted 8 lines
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 :).

Best regards,
Yang

Jason Wang

2014-08-29 04:28 UTC #13 lore raw

Re: [Qemu-devel] [question] e1000 interrupt storm happenedbecauseofitscorrespondingioapic->irr bit always set

Quoted 5 lines · Yang Z

Zhang Haoyu wrote on 2014-08-29:
Quoted 2 lines
Quoted 2 lines
Hi, Yang, Gleb, Michael,
Could you help review below patch please?
I don't quite understand the background. Why ioacpi->irr is setting before EOI? It should be driver's responsibility to clear the interrupt before issuing EOI.

This may happen when a interrupt was injected to guest when its irq handler (driver) was not registered. So irr was still set even during EOI broadcast, and then this irq will be injected to guest immediately. This may cause a dead loop for guest who does not have the ability to detect and disable interrupt storm.

This may be a bug of device model, but we want to know in real cpu, is there a small time gap between the finish of EOI broadcast and the interrupt raised by EOI? If yes, looks like KVM should emulate this behaviour?

Thanks

Michael S. Tsirkin

2014-09-02 15:44 UTC #14 lore raw

Re: [Qemu-devel] [question] e1000 interrupt storm happenedbecauseofits correspondingioapic->irr bit always set

In reply to Zhang Haoyu

Quoted 96 lines · Zhang Haoyu

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?
Quoted 56 lines
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(-)
virt/kvm/ioapic.c +0 -0
index dcaf272..892253e 100644
<at> <at> -221,6 +221,24 <at> <at> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
	return ret;
}
virt/kvm/ioapic.c +18 -0
+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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +2 -2
-		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];
virt/kvm/ioapic.c
Quoted 1 line
virt/kvm/ioapic.c +1 -0
+			if (ioapic->irq_eoi == 100) {
-+ if (ioapic->irq_eoi == 100) {
++ if (ioapic->irq_eoi[i] == 100) {
Quoted 6 lines
virt/kvm/ioapic.c +6 -0
+				/*
+				 * 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
virt/kvm/ioapic.c
Quoted 10 lines
virt/kvm/ioapic.c +10 -0
+				 * 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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +4 -0
+			} else {
+				ioapic_service(ioapic, i);
+			}
+		}
++ else {
++ ioapic->irq_eoi[i] = 0;
++ }
Quoted 2 lines
	}
}

Quoted 7 lines

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'm a bit concerned how this will affect realtime guests. Worth adding a flag to enable this, so that e.g. virtio is not affected?

Quoted 50 lines

virt/kvm/ioapic.c
Quoted 13 lines
<at> <at> -375,12 +414,14 <at> <at> void kvm_ioapic_reset(struct kvm_ioapic *ioapic)
{
	int i;
virt/kvm/ioapic.c +1 -0
+	cancel_delayed_work_sync(&ioapic->eoi_inject);
	for (i = 0; i < IOAPIC_NUM_PINS; i++)
		ioapic->redirtbl[i].fields.mask = 1;
	ioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
	ioapic->ioregsel = 0;
	ioapic->irr = 0;
	ioapic->id = 0;
virt/kvm/ioapic.c +1 -0
+	ioapic->irq_eoi = 0;
-+ ioapic->irq_eoi = 0;
++ memset(ioapic->irq_eoi, 0x00, IOAPIC_NUM_PINS);
Quoted 27 lines
	update_handled_vectors(ioapic);
}
<at> <at> -398,6 +439,7 <at> <at> int kvm_ioapic_init(struct kvm *kvm)
	if (!ioapic)
		return -ENOMEM;
	spin_lock_init(&ioapic->lock);
virt/kvm/ioapic.c +1 -0
+	INIT_DELAYED_WORK(&ioapic->eoi_inject, kvm_ioapic_eoi_inject_work);
	kvm->arch.vioapic = ioapic;
	kvm_ioapic_reset(ioapic);
	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
<at>  <at>  -418,6 +460,7  <at>  <at>  void kvm_ioapic_destroy(struct kvm *kvm)
{
	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
virt/kvm/ioapic.c +1 -0
+	cancel_delayed_work_sync(&ioapic->eoi_inject);
	if (ioapic) {
		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
		kvm->arch.vioapic = NULL;
virt/kvm/ioapic.h +0 -0
index 0b190c3..8938e66 100644
<at>  <at>  -47,6 +47,8  <at>  <at>  struct kvm_ioapic {
	void (*ack_notifier)(void *opaque, int irq);
	spinlock_t lock;
	DECLARE_BITMAP(handled_vectors, 256);
virt/kvm/ioapic.h
Quoted 2 lines
virt/kvm/ioapic.h +2 -0
+	struct delayed_work eoi_inject;
+	u32 irq_eoi;
-+ u32 irq_eoi;
++ u32 irq_eoi[IOAPIC_NUM_PINS];
Quoted 3 lines
};
#ifdef DEBUG

Zhang Haoyu

author

2014-09-04 04:27 UTC #15 lore raw

Re: [Qemu-devel] [question] e1000 interrupt stormhappenedbecauseofits correspondingioapic->irr bit always set

Quoted 102 lines

Quoted 102 lines
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?
Quoted 56 lines
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(-)
virt/kvm/ioapic.c +0 -0
index dcaf272..892253e 100644
<at> <at> -221,6 +221,24 <at> <at> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
	return ret;
}
virt/kvm/ioapic.c +18 -0
+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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +2 -2
-		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];
virt/kvm/ioapic.c
Quoted 1 line
virt/kvm/ioapic.c +1 -0
+			if (ioapic->irq_eoi == 100) {
-+ if (ioapic->irq_eoi == 100) {
++ if (ioapic->irq_eoi[i] == 100) {
Quoted 6 lines
virt/kvm/ioapic.c +6 -0
+				/*
+				 * 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
virt/kvm/ioapic.c
Quoted 10 lines
virt/kvm/ioapic.c +10 -0
+				 * 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;
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +4 -0
+			} else {
+				ioapic_service(ioapic, i);
+			}
+		}
++ else {
++ ioapic->irq_eoi[i] = 0;
++ }
Quoted 2 lines
	}
}
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?

Quoted 6 lines

Quoted 1 line
Zhang Haoyu
I'm a bit concerned how this will affect realtime guests. Worth adding a flag to enable this, so that e.g. virtio is not affected?

Your concern is reasonable.
If applying Jason's original patch, sometimes the virtio's interrupt delay is more than 4ms(my host's HZ=250),
but very rarely happened.
And with my above change on it(per irq counter instead of total irq counter), the delayed virtio interrupt is more rarely happened, then I use 1000 instead of 100 on "if (ioapic->irq_eoi[i] == 1000)", I made a test for 10min, the delayed virtio interrupt has not happened.

Thanks,
Zhang Haoyu

Quoted 51 lines

Quoted 51 lines
Quoted 13 lines
<at> <at> -375,12 +414,14 <at> <at> void kvm_ioapic_reset(struct kvm_ioapic *ioapic)
{
	int i;
virt/kvm/ioapic.c +1 -0
+	cancel_delayed_work_sync(&ioapic->eoi_inject);
	for (i = 0; i < IOAPIC_NUM_PINS; i++)
		ioapic->redirtbl[i].fields.mask = 1;
	ioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
	ioapic->ioregsel = 0;
	ioapic->irr = 0;
	ioapic->id = 0;
virt/kvm/ioapic.c
Quoted 1 line
virt/kvm/ioapic.c +1 -0
+	ioapic->irq_eoi = 0;
-+ ioapic->irq_eoi = 0;
++ memset(ioapic->irq_eoi, 0x00, IOAPIC_NUM_PINS);
Quoted 27 lines
	update_handled_vectors(ioapic);
}
<at> <at> -398,6 +439,7 <at> <at> int kvm_ioapic_init(struct kvm *kvm)
	if (!ioapic)
		return -ENOMEM;
	spin_lock_init(&ioapic->lock);
virt/kvm/ioapic.c +1 -0
+	INIT_DELAYED_WORK(&ioapic->eoi_inject, kvm_ioapic_eoi_inject_work);
	kvm->arch.vioapic = ioapic;
	kvm_ioapic_reset(ioapic);
	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
<at>  <at>  -418,6 +460,7  <at>  <at>  void kvm_ioapic_destroy(struct kvm *kvm)
{
	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
virt/kvm/ioapic.c +1 -0
+	cancel_delayed_work_sync(&ioapic->eoi_inject);
	if (ioapic) {
		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
		kvm->arch.vioapic = NULL;
virt/kvm/ioapic.h +0 -0
index 0b190c3..8938e66 100644
<at>  <at>  -47,6 +47,8  <at>  <at>  struct kvm_ioapic {
	void (*ack_notifier)(void *opaque, int irq);
	spinlock_t lock;
	DECLARE_BITMAP(handled_vectors, 256);
virt/kvm/ioapic.h
Quoted 2 lines
virt/kvm/ioapic.h +2 -0
+	struct delayed_work eoi_inject;
+	u32 irq_eoi;
-+ u32 irq_eoi;
++ u32 irq_eoi[IOAPIC_NUM_PINS];
Quoted 3 lines
};
#ifdef DEBUG

Jason Wang

2014-09-04 04:57 UTC #16 lore raw

Re: [Qemu-devel] [question] e1000 interrupt stormhappenedbecauseofits correspondingioapic->irr bit always set

Quoted 108 lines · Zhang Haoyu

Quoted 102 lines
Quoted 102 lines
Hi Jason,
Quoted 101 lines
Quoted 101 lines
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?
Quoted 54 lines
Quoted 54 lines
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(-)
virt/kvm/ioapic.c +0 -0
index dcaf272..892253e 100644
<at> <at> -221,6 +221,24 <at> <at> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
	return ret;
}
virt/kvm/ioapic.c +18 -0
+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,
Quoted 6 lines
virt/kvm/ioapic.c
Quoted 6 lines
		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
		ent->fields.remote_irr = 0;
virt/kvm/ioapic.c +2 -2
-		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];
Quoted 1 line
virt/kvm/ioapic.c
Quoted 1 line
virt/kvm/ioapic.c +1 -0
+			if (ioapic->irq_eoi == 100) {
-+ if (ioapic->irq_eoi == 100) {
++ if (ioapic->irq_eoi[i] == 100) {
Quoted 8 lines
Quoted 8 lines
virt/kvm/ioapic.c +8 -0
+				/*
+				 * 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
Quoted 8 lines
virt/kvm/ioapic.c
Quoted 8 lines
virt/kvm/ioapic.c +8 -0
+				 * 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;
Quoted 4 lines
virt/kvm/ioapic.c
Quoted 4 lines
virt/kvm/ioapic.c +4 -0
+			} else {
+				ioapic_service(ioapic, i);
+			}
+		}
++ else {
++ ioapic->irq_eoi[i] = 0;
++ }
Quoted 2 lines
Quoted 2 lines
	}
}
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?
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Zhang Haoyu
I'm a bit concerned how this will affect realtime guests. Worth adding a flag to enable this, so that e.g. virtio is not affected?

Quoted 9 lines

Your concern is reasonable.
If applying Jason's original patch, sometimes the virtio's interrupt delay is more than 4ms(my host's HZ=250),
but very rarely happened.
And with my above change on it(per irq counter instead of total irq counter), the delayed virtio interrupt is more rarely happened, then I use 1000 instead of 100 on "if (ioapic->irq_eoi[i] == 1000)", I made a test for 10min, the delayed virtio interrupt has not happened.
Thanks,
Zhang Haoyu

I agree 100 is too aggressive here. Probably you may use a number even much higher than 1000.

One more thing, may worth to add a tracepoint also if we really want this.

Jason Wang

2014-08-25 07:33 UTC #17 lore raw

Re: [question] e1000 interrupt storm happened becauseof its corresponding ioapic->irr bit always set

In reply to Zhang Haoyu

Quoted 18 lines · Zhang Haoyu

Quoted 11 lines
Quoted 11 lines
Hi, all
Quoted 5 lines
Quoted 5 lines
I use a qemu-1.4.1/qemu-2.0.0 to run win7 guest, and encounter e1000 NIC interrupt storm, because "if (!ent->fields.mask && (ioapic->irr & (1 << i)))" is always true in __kvm_ioapic_update_eoi().
Any ideas?
We meet this several times: search the autoneg patches for an example of workaround for this in qemu, and patch kvm: ioapic: conditionally delay irq delivery during eoi broadcast for an workaround in kvm (rejected).
Thanks, Jason,
I searched "e1000 autoneg" in gmane.comp.emulators.qemu, and found below patches,
http://thread.gmane.org/gmane.comp.emulators.qemu/143001/focus=143007
http://thread.gmane.org/gmane.comp.emulators.qemu/284105/focus=284765
http://thread.gmane.org/gmane.comp.emulators.qemu/186159/focus=187351
which one tries to fix this problem, or all of them?

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
That was probably caused by something wrong in e1000 emulation which causes interrupt to be injected into windows guest before its interrupt handler is registered. And Windows guest does not have a mechanism to detect and disable irq in such condition.
Sorry, I don't understand,
I think one interrupt should not been enabled before its handler is successfully registered, is it possible that e1000 emulation inject the interrupt before the interrupt is succesfully enabled?

There's no way for qemu to know whether or not the irq handler was registered in guest. So if qemu behaves differently with a physical card, it may lead the interrupt to be injected into guest too early. You can search redhat bugzilla for lots of related bugs, some even with in-depth analysis.

Thanks

Quoted 4 lines

Thanks,
Zhang Haoyu