RE: [PATCH v3 06/47] igb: Clear IMS bits when committing ICR access

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Sunday, 23 April 2023 06:18
> Cc: Sriram Yagnaraman <sriram.yagnaraman@est.tech>; Jason Wang
> <jasowang@redhat.com>; Dmitry Fleytman <dmitry.fleytman@gmail.com>;
> Michael S . Tsirkin <mst@redhat.com>; Alex Bennée
> <alex.bennee@linaro.org>; Philippe Mathieu-Daudé <philmd@linaro.org>;
> Thomas Huth <thuth@redhat.com>; Wainer dos Santos Moschetta
> <wainersm@redhat.com>; Beraldo Leal <bleal@redhat.com>; Cleber Rosa
> <crosa@redhat.com>; Laurent Vivier <lvivier@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>; qemu-devel@nongnu.org; Tomasz Dzieciol
> <t.dzieciol@partner.samsung.com>; Akihiko Odaki
> <akihiko.odaki@daynix.com>
> Subject: [PATCH v3 06/47] igb: Clear IMS bits when committing ICR access
> 
> The datasheet says contradicting statements regarding ICR accesses so it is 
> not
> reliable to determine the behavior of ICR accesses. However, e1000e does
> clear IMS bits when reading ICR accesses and Linux also expects ICR accesses
> will clear IMS bits according to:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/
> net/ethernet/intel/igb/igb_main.c?h=v6.2#n8048
> 
> Fixes: 3a977deebe ("Intrdocue igb device emulation")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/net/igb_core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> 96a118b6c1..eaca5bd2b6 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -2452,16 +2452,16 @@ igb_set_ims(IGBCore *core, int index, uint32_t
> val)  static void igb_commit_icr(IGBCore *core)  {
>      /*
> -     * If GPIE.NSICR = 0, then the copy of IAM to IMS will occur only if at
> +     * If GPIE.NSICR = 0, then the clear of IMS will occur only if at
>       * least one bit is set in the IMS and there is a true interrupt as
>       * reflected in ICR.INTA.
>       */
>      if ((core->mac[GPIE] & E1000_GPIE_NSICR) ||
>          (core->mac[IMS] && (core->mac[ICR] & E1000_ICR_INT_ASSERTED))) {
> -        igb_set_ims(core, IMS, core->mac[IAM]);
> -    } else {
> -        igb_update_interrupt_state(core);
> +        igb_clear_ims_bits(core, core->mac[IAM]);
>      }
> +
> +    igb_update_interrupt_state(core);
>  }
> 
>  static void igb_set_icr(IGBCore *core, int index, uint32_t val)
> --
> 2.40.0

Reviewed-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>

An additional question, should ICR be cleared if an actual interrupt was 
asserted?
(According to 7.3.2.11 GPIE: Non Selective Interrupt clear on read: When set, 
every read of ICR clears it. When this bit is cleared, an ICR read causes it to 
be cleared only if an actual interrupt was asserted or IMS = 0b.)
Something like this?

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index eaca5bd2b6..aaaf80e4a3 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -2529,6 +2529,9 @@ igb_mac_icr_read(IGBCore *core, int index)
     } else if (core->mac[IMS] == 0) {
         trace_e1000e_irq_icr_clear_zero_ims();
         core->mac[ICR] = 0;
+    } else if (core->mac[ICR] & E1000_ICR_INT_ASSERTED) {
+        e1000e_irq_icr_clear_iame();
+        core->mac[ICR] = 0;
     } else if (!msix_enabled(core->owner)) {
         trace_e1000e_irq_icr_clear_nonmsix_icr_read();
         core->mac[ICR] = 0;