> -----Original Message----- > From: Akihiko Odaki <[email protected]> > Sent: Sunday, 23 April 2023 06:18 > Cc: Sriram Yagnaraman <[email protected]>; Jason Wang > <[email protected]>; Dmitry Fleytman <[email protected]>; > Michael S . Tsirkin <[email protected]>; Alex Bennée > <[email protected]>; Philippe Mathieu-Daudé <[email protected]>; > Thomas Huth <[email protected]>; Wainer dos Santos Moschetta > <[email protected]>; Beraldo Leal <[email protected]>; Cleber Rosa > <[email protected]>; Laurent Vivier <[email protected]>; Paolo Bonzini > <[email protected]>; [email protected]; Tomasz Dzieciol > <[email protected]>; Akihiko Odaki > <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]> 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;
