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

[Akihiko Odaki]

On 2023/04/24 18:29, Sriram Yagnaraman wrote:
> > -----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?

That is handled in igb_commit_icr(), which is renamed to igb_nsicr() in 
patch "igb: Notify only new interrupts".

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