| Message ID | 20230602072516.42502-1-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | e1000e: Add ICR clearing by corresponding IMS bit | expand |
Dear Akihiko, Thank you for your patch. After looking at the diff, it looks like this is a QEMU patch, and not one for the Linux kernel. I leave my inline comments anyway. Am 02.06.23 um 09:25 schrieb Akihiko Odaki: It’d be nice if you started by summarizing the bug. > The datasheet does not say what happens when interrupt was asserted > (ICR.INT_ASSERT=1) and auto mask is *not* active. Personal nit: For better legibility, I’d separate paragraphs by an empty line, or – in this case – I wouldn’t wrap the line, just because a sentence ends. > However, section of 13.3.27 the PCIe* GbE Controllers Open Source > Software Developer’s Manual, which were written for older devices, > namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & > 82573E/82573V/82573L, does say: >> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is >> not 0b, but some ICR bit is set where the corresponding IMS bit is not >> set, then a read does not clear the ICR register. For example, if >> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register >> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read >> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b). > > Linux does no longer activate auto mask since commit > 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears > ICR even in such a case so we also should do so. … since commit 0a8047ac68e5 (e1000e: Fix msi-x interrupt automask) … > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441 > Signed-off-by: Andrew Melnychenko <andrew@daynix.com> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > Supersedes: <20201203133236.222207-1-andrew@daynix.com> > ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.") > > hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------ > hw/net/trace-events | 1 + > 2 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > index 9785ef279c..338bbbf4f4 100644 > --- a/hw/net/e1000e_core.c > +++ b/hw/net/e1000e_core.c > @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index) > e1000e_lower_interrupts(core, ICR, 0xffffffff); > } > > - if ((core->mac[ICR] & E1000_ICR_ASSERTED) && > - (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { > - trace_e1000e_irq_icr_clear_iame(); > - e1000e_lower_interrupts(core, ICR, 0xffffffff); > - trace_e1000e_irq_icr_process_iame(); > - e1000e_lower_interrupts(core, IMS, core->mac[IAM]); > + if (core->mac[ICR] & E1000_ICR_ASSERTED) { > + if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) { > + trace_e1000e_irq_icr_clear_iame(); > + e1000e_lower_interrupts(core, ICR, 0xffffffff); > + trace_e1000e_irq_icr_process_iame(); > + e1000e_lower_interrupts(core, IMS, core->mac[IAM]); > + } > + > + /* > + * The datasheet does not say what happens when interrupt was asserted I believe the network subsystem has a different commenting style than the rest of the Linux source code. > + * (ICR.INT_ASSERT=1) and auto mask is *not* active. > + * However, section of 13.3.27 the PCIe* GbE Controllers Open Source > + * Software Developer’s Manual, which were written for older devices, > + * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & > + * 82573E/82573V/82573L, does say: I’d add a blank line below. > + * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS > + * > is not 0b, but some ICR bit is set where the corresponding IMS bit > + * > is not set, then a read does not clear the ICR register. For > + * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the > + * > ICR register does not clear it. If IMS = 10101010b and > + * > ICR = 0101011b, then a read to the ICR register clears it entirely > + * > (ICR.INT_ASSERTED = 1b). > + * > + * Linux does no longer activate auto mask since commit > + * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware > + * clears ICR even in such a case so we also should do so. > + */ > + if (core->mac[ICR] & core->mac[IMS]) { > + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], > + core->mac[IMS]); > + e1000e_lower_interrupts(core, ICR, 0xffffffff); > + } > } > > return ret; > diff --git a/hw/net/trace-events b/hw/net/trace-events > index e97e9dc17b..9103488e17 100644 > --- a/hw/net/trace-events > +++ b/hw/net/trace-events > @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x" > e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int" > e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS" > e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME" > +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x" > e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X" > e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X" > e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x" Kind regards, Paul
[Removed bpoirier@suse.com from Cc: again.] Am 02.06.23 um 11:41 schrieb Paul Menzel: > Dear Akihiko, > > > Thank you for your patch. > > After looking at the diff, it looks like this is a QEMU patch, and not > one for the Linux kernel. I leave my inline comments anyway. > > > Am 02.06.23 um 09:25 schrieb Akihiko Odaki: > > It’d be nice if you started by summarizing the bug. > >> The datasheet does not say what happens when interrupt was asserted >> (ICR.INT_ASSERT=1) and auto mask is *not* active. > > Personal nit: For better legibility, I’d separate paragraphs by an empty > line, or – in this case – I wouldn’t wrap the line, just because a > sentence ends. > >> However, section of 13.3.27 the PCIe* GbE Controllers Open Source >> Software Developer’s Manual, which were written for older devices, >> namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & >> 82573E/82573V/82573L, does say: >>> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is >>> not 0b, but some ICR bit is set where the corresponding IMS bit is not >>> set, then a read does not clear the ICR register. For example, if >>> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register >>> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read >>> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b). >> >> Linux does no longer activate auto mask since commit >> 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears >> ICR even in such a case so we also should do so. > > … since commit 0a8047ac68e5 (e1000e: Fix msi-x interrupt automask) … > >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441 >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> Supersedes: <20201203133236.222207-1-andrew@daynix.com> >> ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.") >> >> hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------ >> hw/net/trace-events | 1 + >> 2 files changed, 33 insertions(+), 6 deletions(-) >> >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >> index 9785ef279c..338bbbf4f4 100644 >> --- a/hw/net/e1000e_core.c >> +++ b/hw/net/e1000e_core.c >> @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index) >> e1000e_lower_interrupts(core, ICR, 0xffffffff); >> } >> - if ((core->mac[ICR] & E1000_ICR_ASSERTED) && >> - (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >> - trace_e1000e_irq_icr_clear_iame(); >> - e1000e_lower_interrupts(core, ICR, 0xffffffff); >> - trace_e1000e_irq_icr_process_iame(); >> - e1000e_lower_interrupts(core, IMS, core->mac[IAM]); >> + if (core->mac[ICR] & E1000_ICR_ASSERTED) { >> + if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) { >> + trace_e1000e_irq_icr_clear_iame(); >> + e1000e_lower_interrupts(core, ICR, 0xffffffff); >> + trace_e1000e_irq_icr_process_iame(); >> + e1000e_lower_interrupts(core, IMS, core->mac[IAM]); >> + } >> + >> + /* >> + * The datasheet does not say what happens when interrupt was asserted > > I believe the network subsystem has a different commenting style than > the rest of the Linux source code. > >> + * (ICR.INT_ASSERT=1) and auto mask is *not* active. >> + * However, section of 13.3.27 the PCIe* GbE Controllers Open Source >> + * Software Developer’s Manual, which were written for older devices, >> + * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & >> + * 82573E/82573V/82573L, does say: > > I’d add a blank line below. > >> + * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS >> + * > is not 0b, but some ICR bit is set where the corresponding IMS bit >> + * > is not set, then a read does not clear the ICR register. For >> + * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the >> + * > ICR register does not clear it. If IMS = 10101010b and >> + * > ICR = 0101011b, then a read to the ICR register clears it entirely >> + * > (ICR.INT_ASSERTED = 1b). >> + * >> + * Linux does no longer activate auto mask since commit >> + * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware >> + * clears ICR even in such a case so we also should do so. >> + */ >> + if (core->mac[ICR] & core->mac[IMS]) { >> + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], >> + core->mac[IMS]); >> + e1000e_lower_interrupts(core, ICR, 0xffffffff); >> + } >> } >> return ret; >> diff --git a/hw/net/trace-events b/hw/net/trace-events >> index e97e9dc17b..9103488e17 100644 >> --- a/hw/net/trace-events >> +++ b/hw/net/trace-events >> @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x" >> e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read >> due to non MSI-X int" >> e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS" >> e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME" >> +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x" >> e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X" >> e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X" >> e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x" > > > Kind regards, > > Paul
On 2023/06/02 16:25, Akihiko Odaki wrote: > The datasheet does not say what happens when interrupt was asserted > (ICR.INT_ASSERT=1) and auto mask is *not* active. > However, section of 13.3.27 the PCIe* GbE Controllers Open Source > Software Developer’s Manual, which were written for older devices, > namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & > 82573E/82573V/82573L, does say: >> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is >> not 0b, but some ICR bit is set where the corresponding IMS bit is not >> set, then a read does not clear the ICR register. For example, if >> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register >> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read >> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b). > > Linux does no longer activate auto mask since commit > 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears > ICR even in such a case so we also should do so. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441 > Signed-off-by: Andrew Melnychenko <andrew@daynix.com> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > Supersedes: <20201203133236.222207-1-andrew@daynix.com> > ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.") > > hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------ > hw/net/trace-events | 1 + > 2 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > index 9785ef279c..338bbbf4f4 100644 > --- a/hw/net/e1000e_core.c > +++ b/hw/net/e1000e_core.c > @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index) > e1000e_lower_interrupts(core, ICR, 0xffffffff); > } > > - if ((core->mac[ICR] & E1000_ICR_ASSERTED) && > - (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { > - trace_e1000e_irq_icr_clear_iame(); > - e1000e_lower_interrupts(core, ICR, 0xffffffff); > - trace_e1000e_irq_icr_process_iame(); > - e1000e_lower_interrupts(core, IMS, core->mac[IAM]); > + if (core->mac[ICR] & E1000_ICR_ASSERTED) { > + if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) { > + trace_e1000e_irq_icr_clear_iame(); > + e1000e_lower_interrupts(core, ICR, 0xffffffff); > + trace_e1000e_irq_icr_process_iame(); > + e1000e_lower_interrupts(core, IMS, core->mac[IAM]); > + } > + > + /* > + * The datasheet does not say what happens when interrupt was asserted > + * (ICR.INT_ASSERT=1) and auto mask is *not* active. > + * However, section of 13.3.27 the PCIe* GbE Controllers Open Source > + * Software Developer’s Manual, which were written for older devices, > + * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & > + * 82573E/82573V/82573L, does say: > + * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS > + * > is not 0b, but some ICR bit is set where the corresponding IMS bit > + * > is not set, then a read does not clear the ICR register. For > + * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the > + * > ICR register does not clear it. If IMS = 10101010b and > + * > ICR = 0101011b, then a read to the ICR register clears it entirely > + * > (ICR.INT_ASSERTED = 1b). > + * > + * Linux does no longer activate auto mask since commit > + * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware > + * clears ICR even in such a case so we also should do so. > + */ > + if (core->mac[ICR] & core->mac[IMS]) { > + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], > + core->mac[IMS]); > + e1000e_lower_interrupts(core, ICR, 0xffffffff); > + } > } > > return ret; > diff --git a/hw/net/trace-events b/hw/net/trace-events > index e97e9dc17b..9103488e17 100644 > --- a/hw/net/trace-events > +++ b/hw/net/trace-events > @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x" > e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int" > e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS" > e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME" > +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x" > e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X" > e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X" > e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x" Hi Jason, Can you have a look at this patch and "[PATCH] igb: Remove obsolete workaround for Windows": https://patchew.org/QEMU/20230529023704.9387-1-akihiko.odaki@daynix.com/ Regards, Akihiko Odaki
On Thu, Jun 29, 2023 at 4:53 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2023/06/02 16:25, Akihiko Odaki wrote: > > The datasheet does not say what happens when interrupt was asserted > > (ICR.INT_ASSERT=1) and auto mask is *not* active. > > However, section of 13.3.27 the PCIe* GbE Controllers Open Source > > Software Developer’s Manual, which were written for older devices, > > namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & > > 82573E/82573V/82573L, does say: > >> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is > >> not 0b, but some ICR bit is set where the corresponding IMS bit is not > >> set, then a read does not clear the ICR register. For example, if > >> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register > >> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read > >> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b). > > > > Linux does no longer activate auto mask since commit > > 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears > > ICR even in such a case so we also should do so. > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441 > > Signed-off-by: Andrew Melnychenko <andrew@daynix.com> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > --- > > Supersedes: <20201203133236.222207-1-andrew@daynix.com> > > ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.") > > > > hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------ > > hw/net/trace-events | 1 + > > 2 files changed, 33 insertions(+), 6 deletions(-) > > > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > > index 9785ef279c..338bbbf4f4 100644 > > --- a/hw/net/e1000e_core.c > > +++ b/hw/net/e1000e_core.c > > @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index) > > e1000e_lower_interrupts(core, ICR, 0xffffffff); > > } > > > > - if ((core->mac[ICR] & E1000_ICR_ASSERTED) && > > - (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { > > - trace_e1000e_irq_icr_clear_iame(); > > - e1000e_lower_interrupts(core, ICR, 0xffffffff); > > - trace_e1000e_irq_icr_process_iame(); > > - e1000e_lower_interrupts(core, IMS, core->mac[IAM]); > > + if (core->mac[ICR] & E1000_ICR_ASSERTED) { > > + if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) { > > + trace_e1000e_irq_icr_clear_iame(); > > + e1000e_lower_interrupts(core, ICR, 0xffffffff); > > + trace_e1000e_irq_icr_process_iame(); > > + e1000e_lower_interrupts(core, IMS, core->mac[IAM]); > > + } > > + > > + /* > > + * The datasheet does not say what happens when interrupt was asserted > > + * (ICR.INT_ASSERT=1) and auto mask is *not* active. > > + * However, section of 13.3.27 the PCIe* GbE Controllers Open Source > > + * Software Developer’s Manual, which were written for older devices, > > + * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & > > + * 82573E/82573V/82573L, does say: > > + * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS > > + * > is not 0b, but some ICR bit is set where the corresponding IMS bit > > + * > is not set, then a read does not clear the ICR register. For > > + * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the > > + * > ICR register does not clear it. If IMS = 10101010b and > > + * > ICR = 0101011b, then a read to the ICR register clears it entirely > > + * > (ICR.INT_ASSERTED = 1b). > > + * > > + * Linux does no longer activate auto mask since commit > > + * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware > > + * clears ICR even in such a case so we also should do so. > > + */ > > + if (core->mac[ICR] & core->mac[IMS]) { > > + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], > > + core->mac[IMS]); > > + e1000e_lower_interrupts(core, ICR, 0xffffffff); > > + } > > } > > > > return ret; > > diff --git a/hw/net/trace-events b/hw/net/trace-events > > index e97e9dc17b..9103488e17 100644 > > --- a/hw/net/trace-events > > +++ b/hw/net/trace-events > > @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x" > > e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int" > > e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS" > > e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME" > > +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x" > > e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X" > > e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X" > > e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x" > > Hi Jason, > > Can you have a look at this patch and > "[PATCH] igb: Remove obsolete workaround for Windows": > https://patchew.org/QEMU/20230529023704.9387-1-akihiko.odaki@daynix.com/ > > Regards, > Akihiko Odaki I've queued both of the patches. Thanks >
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index 9785ef279c..338bbbf4f4 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index) e1000e_lower_interrupts(core, ICR, 0xffffffff); } - if ((core->mac[ICR] & E1000_ICR_ASSERTED) && - (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { - trace_e1000e_irq_icr_clear_iame(); - e1000e_lower_interrupts(core, ICR, 0xffffffff); - trace_e1000e_irq_icr_process_iame(); - e1000e_lower_interrupts(core, IMS, core->mac[IAM]); + if (core->mac[ICR] & E1000_ICR_ASSERTED) { + if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) { + trace_e1000e_irq_icr_clear_iame(); + e1000e_lower_interrupts(core, ICR, 0xffffffff); + trace_e1000e_irq_icr_process_iame(); + e1000e_lower_interrupts(core, IMS, core->mac[IAM]); + } + + /* + * The datasheet does not say what happens when interrupt was asserted + * (ICR.INT_ASSERT=1) and auto mask is *not* active. + * However, section of 13.3.27 the PCIe* GbE Controllers Open Source + * Software Developer’s Manual, which were written for older devices, + * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & + * 82573E/82573V/82573L, does say: + * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS + * > is not 0b, but some ICR bit is set where the corresponding IMS bit + * > is not set, then a read does not clear the ICR register. For + * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the + * > ICR register does not clear it. If IMS = 10101010b and + * > ICR = 0101011b, then a read to the ICR register clears it entirely + * > (ICR.INT_ASSERTED = 1b). + * + * Linux does no longer activate auto mask since commit + * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware + * clears ICR even in such a case so we also should do so. + */ + if (core->mac[ICR] & core->mac[IMS]) { + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], + core->mac[IMS]); + e1000e_lower_interrupts(core, ICR, 0xffffffff); + } } return ret; diff --git a/hw/net/trace-events b/hw/net/trace-events index e97e9dc17b..9103488e17 100644 --- a/hw/net/trace-events +++ b/hw/net/trace-events @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x" e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int" e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS" e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME" +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x" e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X" e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X" e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"