qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] e1000e: Fix ICR "Other" causes clear logic


From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH v2] e1000e: Fix ICR "Other" causes clear logic
Date: Tue, 23 May 2017 09:46:48 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0



On 2017年05月22日 19:26, Sameeh Jubran wrote:
This commit fixes a bug which causes the guest to hang. The bug was observed 
upon a
"receive overrun" (bit #6 of the ICR register) interrupt which could be 
triggered post
migration in a heavy traffic environment. Even though the "receive overrun" bit 
(#6)
is masked out by the IMS register (refer to the log below) the driver still 
receives
an interrupt as the "receive overrun" bit (#6) causes the "Other" - bit #24 of 
the
ICR register - bit to be set as documented below. The driver handles the 
interrupt
and clears the "Other" bit (#24) but doesn't clear the "receive overrun" bit 
(#6)
which leads to an infinite loop. Apparently the Windows driver expects that the
"receive overrun" bit and other ones - documented below - to be cleared when the
"Other" bit (#24) is cleared.

So to sum that up:
1. Bit #6 of the ICR register is set by heavy traffic
2. As a results of setting bit #6, bit #24 is set
3. The driver receives an interrupt for bit 24 (it doesn't receieve an 
interrupt for bit #6 as it is masked out by IMS)
4. The driver handles and clears the interrupt of bit #24
5. Bit #6 is still set.
6. 2 happens all over again

The Interrupt Cause Read - ICR register:

The ICR has the "Other" bit - bit #24 - that is set when one or more of the 
following
ICR register's bits are set:

LSC - bit #2, RXO - bit #6, MDAC - bit #9, SRPD - bit #16, ACK - bit #17, MNG - 
bit #18

This bug can occur with any of these bits depending on the driver's behaviour 
and
the way it configures the device. However, trying to reproduce it with any bit 
other
than RX0 is challenging and came to failure as the drivers don't implement most 
of
these bits, trying to reproduce it with LSC (Link Status Change - bit #2) bit 
didn't
succeed too as it seems that Windows handles this bit differently.

Log sample of the storm:

address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x1000000 (ICR: 
0x815000c2, IMS: 0x1a00004)
address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x0 (ICR: 0x815000c2, 
IMS: 0xa00004)
address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x0 (ICR: 0x815000c2, 
IMS: 0xa00004)
address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x0 (ICR: 0x815000c2, 
IMS: 0xa00004)
address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x0 (ICR: 0x815000c2, 
IMS: 0xa00004)
address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x0 (ICR: 0x815000c2, 
IMS: 0xa00004)
address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x0 (ICR: 0x815000c2, 
IMS: 0xa00004)
address@hidden:e1000e_irq_pending_interrupts ICR PENDING: 0x1000000 (ICR: 
0x815000c2, IMS: 0x1a00004)

* This bug behaviour wasn't observed with the Linux driver.

This commit solves:
https://bugzilla.redhat.com/show_bug.cgi?id=1447935
https://bugzilla.redhat.com/show_bug.cgi?id=1449490

Signed-off-by: Sameeh Jubran <address@hidden>
---
  hw/net/e1000e_core.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 28c5be1..8174b53 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2454,14 +2454,17 @@ e1000e_set_ics(E1000ECore *core, int index, uint32_t 
val)
  static void
  e1000e_set_icr(E1000ECore *core, int index, uint32_t val)
  {
+    uint32_t icr = 0;
      if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
          (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
          trace_e1000e_irq_icr_process_iame();
          e1000e_clear_ims_bits(core, core->mac[IAM]);
      }
- trace_e1000e_irq_icr_write(val, core->mac[ICR], core->mac[ICR] & ~val);
-    core->mac[ICR] &= ~val;
+    icr = core->mac[ICR] & ~val;
+    icr = (val & E1000_ICR_OTHER) ? (icr & ~E1000_ICR_OTHER_CAUSES) : icr;
+    trace_e1000e_irq_icr_write(val, core->mac[ICR], icr);
+    core->mac[ICR] = icr;
      e1000e_update_interrupt_state(core);
  }

Applied and add a comment in the code to explain.

Thanks



reply via email to

[Prev in Thread] Current Thread [Next in Thread]