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

[Andrew Melnichenko]

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

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