Re: [PATCH] e1000e: Added ICR clearing by corresponding IMS bit.

[Jason Wang]

On Wed, Oct 27, 2021 at 6:57 PM Andrew Melnichenko <andrew@daynix.com> wrote:
>
> Hi,
> Let's make things clear.
> At first, I've decided to fix the issue in the linux e1000e driver.
> (https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20200413/019497.html)
> Original driver developers suggest to fix the issue on qemu and assures that 
> driver works correctly on real hw.
> I've added fix according to the note 13.3.28 of the 8257X manual
> (https://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf)
> I've reference to 8257X spec which is an apparently a bit different to 
> 82574l-gbe-controller-datasheet.pdf

Yes, and 82475l is the referenced model when developing e1000e
emulation code I think.

Thanks

>
>
> On Thu, Oct 21, 2021 at 5:16 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> Hi Andrew:
>>
>> On Thu, Oct 21, 2021 at 6:27 AM Andrew Melnichenko <andrew@daynix.com> wrote:
>> >
>> > Hi,
>> > I've used this 
>> > manual(https://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf)
>> > It was provided by Intel when I've tried to research that bug.
>> > Although it's a bit newer manual - the article is 13.3.28.
>>
>> Note that it's not the model that e1000e tries to implement (82574L).
>> The device ID in qemu is 0x10D3 which is not listed in the above link
>> "4.7.7 Mandatory PCI Configuration Registers".
>>
>> Thanks
>>
>> >
>> >
>> > On Tue, Oct 19, 2021 at 10:56 AM Jason Wang <jasowang@redhat.com> wrote:
>> >>
>> >> On Thu, Oct 14, 2021 at 4:34 PM Andrew Melnichenko <andrew@daynix.com> 
>> >> wrote:
>> >> >
>> >> > Ping
>> >> >
>> >> > On Wed, Aug 18, 2021 at 9:10 PM Andrew Melnychenko <andrew@daynix.com> 
>> >> > wrote:
>> >> >>
>> >> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>> >> >>
>> >> >> The issue is in LSC clearing. So, after "link up"(during 
>> >> >> initialization),
>> >> >> the next LSC event is masked and can't be processed.
>> >> >> Technically, the event should be 'cleared' during ICR read.
>> >> >> On Windows guest, everything works well, mostly because of
>> >> >> different interrupt routines(ICR clears during register write).
>> >> >> So, added ICR clearing during reading, according to the note by
>> >> >> section 13.3.27 of the 8257X developers manual.
>> >> >>
>> >> >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>> >> >> ---
>> >> >>  hw/net/e1000e_core.c | 10 ++++++++++
>> >> >>  hw/net/trace-events  |  1 +
>> >> >>  2 files changed, 11 insertions(+)
>> >> >>
>> >> >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> >> >> index b75f2ab8fc..288897a975 100644
>> >> >> --- a/hw/net/e1000e_core.c
>> >> >> +++ b/hw/net/e1000e_core.c
>> >> >> @@ -2617,6 +2617,16 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>> >> >>          e1000e_clear_ims_bits(core, core->mac[IAM]);
>> >> >>      }
>> >> >>
>> >> >> +    /*
>> >> >> +     * PCIe* GbE Controllers Open Source Software Developer's Manual
>> >> >> +     * 13.3.27 Interrupt Cause Read Register
>> >>
>> >> Per link in the beginning of this file it should be 82574l I guess?
>> >>
>> >> If yes, I'm using revision 3.4 and it's 13.3.27 is not about ICR.
>> >>
>> >> What it said are:
>> >>
>> >> "
>> >> In MSI-X mode the bits in this register can be configured to
>> >> auto-clear when the MSI-X interrupt message is sent, in order to
>> >> minimize driver overhead, and when using MSI-X interrupt signaling. In
>> >> systems that do not support MSI-X, reading the ICR register clears
>> >> it's bits or writing 1b's clears the corresponding bits in this
>> >> register.
>> >> "
>> >>
>> >>
>> >> >> +     */
>> >> >> +    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>> >> >> +        (core->mac[ICR] & core->mac[IMS])) {
>> >> >> +        trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], 
>> >> >> core->mac[IMS]);
>> >> >> +        core->mac[ICR] = 0;
>> >> >> +    }
>> >>
>> >> Thanks
>> >>
>> >> >> +
>> >> >>      trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
>> >> >>      e1000e_update_interrupt_state(core);
>> >> >>      return ret;
>> >> >> diff --git a/hw/net/trace-events b/hw/net/trace-events
>> >> >> index c28b91ee1a..15fd09aa1c 100644
>> >> >> --- a/hw/net/trace-events
>> >> >> +++ b/hw/net/trace-events
>> >> >> @@ -225,6 +225,7 @@ 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"
>> >> >>  e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
>> >> >> +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) 
>> >> >> "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
>> >> >>  e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing 
>> >> >> IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
>> >> >>  e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR 
>> >> >> bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
>> >> >>  e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to 
>> >> >> IMC write 0x%x"
>> >> >> --
>> >> >> 2.31.1
>> >> >>
>> >>
>>