[PATCH] hw/net: e1000e: Clear ICR on read when using non MSI-X interrupts

[PATCH] hw/net: e1000e: Clear ICR on read when using non MSI-X interrupts

[Nick Hudson]

In section 7.4.3 of the 82574 datasheet it states that

    "In systems that do not support MSI-X, reading the ICR
     register clears it's bits..."

Some OSes rely on this.

Signed-off-by: Nick Hudson <skrll@netbsd.org>
---
 hw/net/e1000e_core.c | 5 +++++
 hw/net/trace-events  | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 8ae6fb7e14..2c51089a82 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2607,6 +2607,11 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
         core->mac[ICR] = 0;
     }
 
+    if (!msix_enabled(core->owner)) {
+        trace_e1000e_irq_icr_clear_nonmsix_icr_read();
+        core->mac[ICR] = 0;
+    }
+
     if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
         (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
         trace_e1000e_irq_icr_clear_iame();
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 643338f610..084086ec44 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -221,6 +221,7 @@ e1000e_irq_write_ics(uint32_t val) "Adding ICR bits 0x%x"
 e1000e_irq_icr_process_iame(void) "Clearing IMS bits due to IAME"
 e1000e_irq_read_ics(uint32_t ics) "Current ICS: 0x%x"
 e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x"
+e1000e_irq_icr_clear_nonmisx_icr_read(void) "Clearing ICR on read due to non MSI-X int"
 e1000e_irq_icr_read_entry(uint32_t icr) "Starting ICR read. Current ICR: 0x%x"
 e1000e_irq_icr_read_exit(uint32_t icr) "Ending ICR read. Current ICR: 0x%x"
 e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
-- 
2.25.1

Re: [PATCH] hw/net: e1000e: Clear ICR on read when using non MSI-X interrupts

[Jason Wang]

On Thu, Feb 10, 2022 at 4:10 PM Nick Hudson <skrll@netbsd.org> wrote:
>
> In section 7.4.3 of the 82574 datasheet it states that
>
>     "In systems that do not support MSI-X, reading the ICR
>      register clears it's bits..."
>
> Some OSes rely on this.
>
> Signed-off-by: Nick Hudson <skrll@netbsd.org>
> ---
>  hw/net/e1000e_core.c | 5 +++++
>  hw/net/trace-events  | 1 +
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 8ae6fb7e14..2c51089a82 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2607,6 +2607,11 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>          core->mac[ICR] = 0;
>      }
>
> +    if (!msix_enabled(core->owner)) {
> +        trace_e1000e_irq_icr_clear_nonmsix_icr_read();
> +        core->mac[ICR] = 0;
> +    }
> +
>      if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>          (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
>          trace_e1000e_irq_icr_clear_iame();
> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index 643338f610..084086ec44 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -221,6 +221,7 @@ e1000e_irq_write_ics(uint32_t val) "Adding ICR bits 0x%x"
>  e1000e_irq_icr_process_iame(void) "Clearing IMS bits due to IAME"
>  e1000e_irq_read_ics(uint32_t ics) "Current ICS: 0x%x"
>  e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x"
> +e1000e_irq_icr_clear_nonmisx_icr_read(void) "Clearing ICR on read due to non MSI-X int"

s/mis/msi/

FAILED: libcommon.fa.p/hw_net_e1000e_core.c.o
cc -m64 -mcx16 -Ilibcommon.fa.p -I../common-user/host/x86_64
-I../capstone/include/capstone -I../dtc/libfdt -I../slirp
-I../slirp/src -I/usr/include/pixman-1 -I/usr/include/libpng16
-I/usr/include/libmount -I/usr/include/blkid -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include
-I/usr/include/gio-unix-2.0 -fdiagnostics-color=auto -Wall
-Winvalid-pch -Werror -std=gnu11 -g -isystem
/home/devel/git/qemu/linux-headers -isystem linux-headers -iquote .
-iquote /home/devel/git/qemu -iquote /home/devel/git/qemu/include
-iquote /home/devel/git/qemu/disas/libvixl -iquote
/home/devel/git/qemu/tcg/i386 -pthread -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
-Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes
-fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
-Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi
-fstack-protector-strong -fPIE -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600
-DNCURSES_WIDECHAR=1 -MD -MQ libcommon.fa.p/hw_net_e1000e_core.c.o -MF
libcommon.fa.p/hw_net_e1000e_core.c.o.d -o
libcommon.fa.p/hw_net_e1000e_core.c.o -c ../hw/net/e1000e_core.c
../hw/net/e1000e_core.c: In function ‘e1000e_mac_icr_read’:
../hw/net/e1000e_core.c:2611:9: error: implicit declaration of
function ‘trace_e1000e_irq_icr_clear_nonmsix_icr_read’; did you mean
‘trace_e1000e_irq_icr_clear_nonmisx_icr_read’?
[-Werror=implicit-function-declaration]
 2611 |         trace_e1000e_irq_icr_clear_nonmsix_icr_read();
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |         trace_e1000e_irq_icr_clear_nonmisx_icr_read
../hw/net/e1000e_core.c:2611:9: error: nested extern declaration of
‘trace_e1000e_irq_icr_clear_nonmsix_icr_read’ [-Werror=nested-externs]

I wonder how the patch is tested.

Thanks

>  e1000e_irq_icr_read_entry(uint32_t icr) "Starting ICR read. Current ICR: 0x%x"
>  e1000e_irq_icr_read_exit(uint32_t icr) "Ending ICR read. Current ICR: 0x%x"
>  e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
> --
> 2.25.1
>