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

[Akihiko Odaki]

On 2023/04/24 20:23, Sriram Yagnaraman wrote:
> > -----Original Message-----
> > From: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Sent: Monday, 24 April 2023 12:52
> > To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> > Cc: 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>
> > Subject: Re: [PATCH v3 06/47] igb: Clear IMS bits when committing ICR access
> >
> > 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/tr
> > > > ee/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".
>
> Mm, I must be missing something, but I still don't see the ICR bits being 
> cleared igb_commit_icr/igb_nsicr().
> For e.g. e1000e_mac_icr_read does this explicitly:
>      if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>          (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
>          trace_e1000e_irq_icr_clear_iame();
>          core->mac[ICR] = 0;
>          trace_e1000e_irq_icr_process_iame();
>          e1000e_clear_ims_bits(core, core->mac[IAM]);
>      }

Now I get it. This is indeed missing and needs to be handled, just as 
you suggested.

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