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

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

[andrew]

From: Andrew Melnychenko <andrew@daynix.com>

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
Added ICR clearing if there is IMS bit - 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 | 9 +++++++++
 hw/net/trace-events  | 1 +
 2 files changed, 10 insertions(+)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index d5676871fa..302e99ff46 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2624,6 +2624,15 @@ 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
+     */
+    if (core->mac[ICR] & core->mac[IMS]) {
+        trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], core->mac[IMS]);
+        core->mac[ICR] = 0;
+    }
+
     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 e18f883cfd..46e40fcfa9 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -237,6 +237,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.24.1

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

[Jason Wang]

On 2020/5/7 上午5:26, andrew@daynix.com wrote:
> From: Andrew Melnychenko <andrew@daynix.com>
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> Added ICR clearing if there is IMS bit - 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 | 9 +++++++++
>   hw/net/trace-events  | 1 +
>   2 files changed, 10 insertions(+)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index d5676871fa..302e99ff46 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2624,6 +2624,15 @@ 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
> +     */


Hi Andrew:

Which version of the manual did you use? I try to use the one mentioned 
in e1000e.c which is 
http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf.

But I couldn't find chapter 13.3.27.

Thanks


> +    if (core->mac[ICR] & core->mac[IMS]) {
> +        trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], core->mac[IMS]);
> +        core->mac[ICR] = 0;
> +    }
> +
>       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 e18f883cfd..46e40fcfa9 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -237,6 +237,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"

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

[Andrew Melnichenko]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

Yo, I've used OpenSDM_8257x-18.pdf specification.
This document was recommended by Intel guys(Also, they referenced to that
note).
I've made a fast fix and it works. Before that I had a fix for Linux e1000e
driver.
Overall, the issue was in pending interrupts that can't be cleared by
reading ICR in Linux(Windows driver clears by writing to ICR).

You can download spec for example from:
http://iweb.dl.sourceforge.net/project/e1000/8257x%20Developer%20Manual/Revision%201.8/OpenSDM_8257x-18.pdf

On Fri, May 8, 2020 at 5:21 AM Jason Wang <jasowang@redhat.com> wrote:

>
> On 2020/5/7 上午5:26, andrew@daynix.com wrote:
> > From: Andrew Melnychenko <andrew@daynix.com>
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> > Added ICR clearing if there is IMS bit - 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 | 9 +++++++++
> >   hw/net/trace-events  | 1 +
> >   2 files changed, 10 insertions(+)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index d5676871fa..302e99ff46 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -2624,6 +2624,15 @@ 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
> > +     */
>
>
> Hi Andrew:
>
> Which version of the manual did you use? I try to use the one mentioned
> in e1000e.c which is
>
> http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
> .
>
> But I couldn't find chapter 13.3.27.
>
> Thanks
>
>
> > +    if (core->mac[ICR] & core->mac[IMS]) {
> > +        trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
> core->mac[IMS]);
> > +        core->mac[ICR] = 0;
> > +    }
> > +
> >       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 e18f883cfd..46e40fcfa9 100644
> > --- a/hw/net/trace-events
> > +++ b/hw/net/trace-events
> > @@ -237,6 +237,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"
>
>

[-- Attachment #2: Type: text/html, Size: 4448 bytes --]

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

[Jason Wang]

On 2020/5/9 上午2:13, Andrew Melnichenko wrote:
> Yo, I've used OpenSDM_8257x-18.pdf specification.
> This document was recommended by Intel guys(Also, they referenced to 
> that note).
> I've made a fast fix and it works. Before that I had a fix for Linux 
> e1000e driver.
> Overall, the issue was in pending interrupts that can't be cleared by 
> reading ICR in Linux(Windows driver clears by writing to ICR).
>
> You can download spec for example from:
> http://iweb.dl.sourceforge.net/project/e1000/8257x%20Developer%20Manual/Revision%201.8/OpenSDM_8257x-18.pdf


Interesting, this spec doesn't include 82574l which is what e1000e 
claims to emulate:

     c->vendor_id = PCI_VENDOR_ID_INTEL;
     c->device_id = E1000_DEV_ID_82574L;

Looking at 82574l spec (using the link mentioned in e1000e_core.c), it 
said (7.4.3):

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.

So the auto clear is under the control of EIAC (MSIX) or unconditionally 
(non MSI-X).

But what has been implemented in e1000e_mac_icr_read() is something 
similar to the behavior of non 82574l card.

So I think we should implement the 82574l behavior?

Thanks


>
> On Fri, May 8, 2020 at 5:21 AM Jason Wang <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/5/7 上午5:26, andrew@daynix.com <mailto:andrew@daynix.com>
>     wrote:
>     > From: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     >
>     > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>     > Added ICR clearing if there is IMS bit - according to the note by
>     > section 13.3.27 of the 8257X developers manual.
>     >
>     > Signed-off-by: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     > ---
>     >   hw/net/e1000e_core.c | 9 +++++++++
>     >   hw/net/trace-events  | 1 +
>     >   2 files changed, 10 insertions(+)
>     >
>     > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>     > index d5676871fa..302e99ff46 100644
>     > --- a/hw/net/e1000e_core.c
>     > +++ b/hw/net/e1000e_core.c
>     > @@ -2624,6 +2624,15 @@ 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
>     > +     */
>
>
>     Hi Andrew:
>
>     Which version of the manual did you use? I try to use the one
>     mentioned
>     in e1000e.c which is
>     http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf.
>
>     But I couldn't find chapter 13.3.27.
>
>     Thanks
>
>
>     > +    if (core->mac[ICR] & core->mac[IMS]) {
>     > + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
>     core->mac[IMS]);
>     > +        core->mac[ICR] = 0;
>     > +    }
>     > +
>     >       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 e18f883cfd..46e40fcfa9 100644
>     > --- a/hw/net/trace-events
>     > +++ b/hw/net/trace-events
>     > @@ -237,6 +237,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"
>

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

[Andrew Melnichenko]

[-- Attachment #1: Type: text/plain, Size: 5628 bytes --]

Yo,

> So I think we should implement the 82574l behavior?

 Well, as I understand it - its already implemented. I've added ICR
clearance if ICR & IMS(also need to add ICR_ASSERTED check, my bad, I'll
prepare new patch).

At first, I had hacks to clear 'msi_causes_pending' at
'e1000e_core_set_link_status()' before link down. It works but it's not a
solution.
Also, on Windows the bug doesn't reproduce. I've traced Windows and Linux -
the difference that Windows driver clears pending by writing to ICR, where
Linux tries to clear by reading it.
I had another possible fix - for Linux driver(writing to ICR at interrupt
routine).
I've asked intel guys, does Linux driver works with a device(I don't have
real one). Thay said that it works and suggested to check 8257x spec. I'll
forward the message to you.

On Sat, May 9, 2020 at 9:02 AM Jason Wang <jasowang@redhat.com> wrote:

>
> On 2020/5/9 上午2:13, Andrew Melnichenko wrote:
> > Yo, I've used OpenSDM_8257x-18.pdf specification.
> > This document was recommended by Intel guys(Also, they referenced to
> > that note).
> > I've made a fast fix and it works. Before that I had a fix for Linux
> > e1000e driver.
> > Overall, the issue was in pending interrupts that can't be cleared by
> > reading ICR in Linux(Windows driver clears by writing to ICR).
> >
> > You can download spec for example from:
> >
> http://iweb.dl.sourceforge.net/project/e1000/8257x%20Developer%20Manual/Revision%201.8/OpenSDM_8257x-18.pdf
>
>
> Interesting, this spec doesn't include 82574l which is what e1000e
> claims to emulate:
>
>      c->vendor_id = PCI_VENDOR_ID_INTEL;
>      c->device_id = E1000_DEV_ID_82574L;
>
> Looking at 82574l spec (using the link mentioned in e1000e_core.c), it
> said (7.4.3):
>
> 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.
>
> So the auto clear is under the control of EIAC (MSIX) or unconditionally
> (non MSI-X).
>
> But what has been implemented in e1000e_mac_icr_read() is something
> similar to the behavior of non 82574l card.
>
> So I think we should implement the 82574l behavior?
>
> Thanks
>
>
> >
> > On Fri, May 8, 2020 at 5:21 AM Jason Wang <jasowang@redhat.com
> > <mailto:jasowang@redhat.com>> wrote:
> >
> >
> >     On 2020/5/7 上午5:26, andrew@daynix.com <mailto:andrew@daynix.com>
> >     wrote:
> >     > From: Andrew Melnychenko <andrew@daynix.com
> >     <mailto:andrew@daynix.com>>
> >     >
> >     > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> >     > Added ICR clearing if there is IMS bit - according to the note by
> >     > section 13.3.27 of the 8257X developers manual.
> >     >
> >     > Signed-off-by: Andrew Melnychenko <andrew@daynix.com
> >     <mailto:andrew@daynix.com>>
> >     > ---
> >     >   hw/net/e1000e_core.c | 9 +++++++++
> >     >   hw/net/trace-events  | 1 +
> >     >   2 files changed, 10 insertions(+)
> >     >
> >     > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> >     > index d5676871fa..302e99ff46 100644
> >     > --- a/hw/net/e1000e_core.c
> >     > +++ b/hw/net/e1000e_core.c
> >     > @@ -2624,6 +2624,15 @@ 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
> >     > +     */
> >
> >
> >     Hi Andrew:
> >
> >     Which version of the manual did you use? I try to use the one
> >     mentioned
> >     in e1000e.c which is
> >
> http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
> .
> >
> >     But I couldn't find chapter 13.3.27.
> >
> >     Thanks
> >
> >
> >     > +    if (core->mac[ICR] & core->mac[IMS]) {
> >     > + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
> >     core->mac[IMS]);
> >     > +        core->mac[ICR] = 0;
> >     > +    }
> >     > +
> >     >       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 e18f883cfd..46e40fcfa9 100644
> >     > --- a/hw/net/trace-events
> >     > +++ b/hw/net/trace-events
> >     > @@ -237,6 +237,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"
> >
>
>

[-- Attachment #2: Type: text/html, Size: 8083 bytes --]

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

[Jason Wang]

On 2020/5/11 下午6:08, Andrew Melnichenko wrote:
> Yo,
>
>     So I think we should implement the 82574l behavior?
>
>  Well, as I understand it - its already implemented. I've added ICR 
> clearance if ICR & IMS(also need to add ICR_ASSERTED check, my bad, 
> I'll prepare new patch).


Yes, but it behave more like e.g 82573 not what we claim to emulate like 
82574l.


>
> At first, I had hacks to clear 'msi_causes_pending' at 
> 'e1000e_core_set_link_status()' before link down. It works but it's 
> not a solution.
> Also, on Windows the bug doesn't reproduce. I've traced Windows and 
> Linux - the difference that Windows driver clears pending by writing 
> to ICR, where Linux tries to clear by reading it.
> I had another possible fix - for Linux driver(writing to ICR at 
> interrupt routine).
> I've asked intel guys, does Linux driver works with a device(I don't 
> have real one). Thay said that it works and suggested to check 8257x 
> spec. I'll forward the message to you.


Ok.

Thanks


>
> On Sat, May 9, 2020 at 9:02 AM Jason Wang <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/5/9 上午2:13, Andrew Melnichenko wrote:
>     > Yo, I've used OpenSDM_8257x-18.pdf specification.
>     > This document was recommended by Intel guys(Also, they
>     referenced to
>     > that note).
>     > I've made a fast fix and it works. Before that I had a fix for
>     Linux
>     > e1000e driver.
>     > Overall, the issue was in pending interrupts that can't be
>     cleared by
>     > reading ICR in Linux(Windows driver clears by writing to ICR).
>     >
>     > You can download spec for example from:
>     >
>     http://iweb.dl.sourceforge.net/project/e1000/8257x%20Developer%20Manual/Revision%201.8/OpenSDM_8257x-18.pdf
>
>
>     Interesting, this spec doesn't include 82574l which is what e1000e
>     claims to emulate:
>
>          c->vendor_id = PCI_VENDOR_ID_INTEL;
>          c->device_id = E1000_DEV_ID_82574L;
>
>     Looking at 82574l spec (using the link mentioned in
>     e1000e_core.c), it
>     said (7.4.3):
>
>     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.
>
>     So the auto clear is under the control of EIAC (MSIX) or
>     unconditionally
>     (non MSI-X).
>
>     But what has been implemented in e1000e_mac_icr_read() is something
>     similar to the behavior of non 82574l card.
>
>     So I think we should implement the 82574l behavior?
>
>     Thanks
>
>
>     >
>     > On Fri, May 8, 2020 at 5:21 AM Jason Wang <jasowang@redhat.com
>     <mailto:jasowang@redhat.com>
>     > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>> wrote:
>     >
>     >
>     >     On 2020/5/7 上午5:26, andrew@daynix.com
>     <mailto:andrew@daynix.com> <mailto:andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     >     wrote:
>     >     > From: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>
>     >     <mailto:andrew@daynix.com <mailto:andrew@daynix.com>>>
>     >     >
>     >     > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>     >     > Added ICR clearing if there is IMS bit - according to the
>     note by
>     >     > section 13.3.27 of the 8257X developers manual.
>     >     >
>     >     > Signed-off-by: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>
>     >     <mailto:andrew@daynix.com <mailto:andrew@daynix.com>>>
>     >     > ---
>     >     >   hw/net/e1000e_core.c | 9 +++++++++
>     >     >   hw/net/trace-events  | 1 +
>     >     >   2 files changed, 10 insertions(+)
>     >     >
>     >     > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>     >     > index d5676871fa..302e99ff46 100644
>     >     > --- a/hw/net/e1000e_core.c
>     >     > +++ b/hw/net/e1000e_core.c
>     >     > @@ -2624,6 +2624,15 @@ 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
>     >     > +     */
>     >
>     >
>     >     Hi Andrew:
>     >
>     >     Which version of the manual did you use? I try to use the one
>     >     mentioned
>     >     in e1000e.c which is
>     >
>     http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf.
>     >
>     >     But I couldn't find chapter 13.3.27.
>     >
>     >     Thanks
>     >
>     >
>     >     > +    if (core->mac[ICR] & core->mac[IMS]) {
>     >     > + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
>     >     core->mac[IMS]);
>     >     > +        core->mac[ICR] = 0;
>     >     > +    }
>     >     > +
>     >     >  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 e18f883cfd..46e40fcfa9 100644
>     >     > --- a/hw/net/trace-events
>     >     > +++ b/hw/net/trace-events
>     >     > @@ -237,6 +237,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"
>     >
>

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

[Alexander Duyck]

Okay. That sounds reasonable. You should repost this with your more
thorough explanation of the problem and how this solves it in the
patch description.

Thanks.

- Alex

On Mon, Dec 14, 2020 at 3:09 AM Andrew Melnichenko <andrew@daynix.com> wrote:
>
> Hi,
> 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, I've added ICR clearing during read, according to the note by
> section 13.3.27 of the 8257X developers manual.
>
> On Thu, Dec 3, 2020 at 7:57 PM Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>
>> On Thu, Dec 3, 2020 at 5:00 AM Andrew Melnychenko <andrew@daynix.com> wrote:
>> >
>> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>>
>> So the bugzilla seems to be reporting that the NIC operstate is being
>> misreported when qemu has configured the link down. Based on the
>> description it isn't clear to me how this patch addresses that. Some
>> documentation on how you reproduced the issue, and what was seen
>> before and after this patch would be useful.
>>
>> > Added ICR clearing if there is IMS bit - according to the note by
>>
>> Should probably be "Add" instead of "Added". Same for the title of the patch.
>>
>> > 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 095c01ebc6..9705f5c52e 100644
>> > --- a/hw/net/e1000e_core.c
>> > +++ b/hw/net/e1000e_core.c
>> > @@ -2624,6 +2624,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
>> > +     */
>> > +    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;
>> > +    }
>> > +
>> >      trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
>> >      e1000e_update_interrupt_state(core);
>> >      return ret;
>>
>> Changes like this have historically been problematic. I am curious
>> what testing had been done on this and with what drivers? Keep in mind
>> that we have to support several flavors of BSD, Windows, and Linux
>> with this.
>>
>> > diff --git a/hw/net/trace-events b/hw/net/trace-events
>> > index 5db45456d9..2c3521a19c 100644
>> > --- a/hw/net/trace-events
>> > +++ b/hw/net/trace-events
>> > @@ -237,6 +237,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.29.2
>> >

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

[Andrew Melnichenko]

[-- Attachment #1: Type: text/plain, Size: 3420 bytes --]

Hi,
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, I've added ICR clearing during read, according to the note by
section 13.3.27 of the 8257X developers manual.

On Thu, Dec 3, 2020 at 7:57 PM Alexander Duyck <alexander.duyck@gmail.com>
wrote:

> On Thu, Dec 3, 2020 at 5:00 AM Andrew Melnychenko <andrew@daynix.com>
> wrote:
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>
> So the bugzilla seems to be reporting that the NIC operstate is being
> misreported when qemu has configured the link down. Based on the
> description it isn't clear to me how this patch addresses that. Some
> documentation on how you reproduced the issue, and what was seen
> before and after this patch would be useful.
>
> > Added ICR clearing if there is IMS bit - according to the note by
>
> Should probably be "Add" instead of "Added". Same for the title of the
> patch.
>
> > 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 095c01ebc6..9705f5c52e 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -2624,6 +2624,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
> > +     */
> > +    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;
> > +    }
> > +
> >      trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
> >      e1000e_update_interrupt_state(core);
> >      return ret;
>
> Changes like this have historically been problematic. I am curious
> what testing had been done on this and with what drivers? Keep in mind
> that we have to support several flavors of BSD, Windows, and Linux
> with this.
>
> > diff --git a/hw/net/trace-events b/hw/net/trace-events
> > index 5db45456d9..2c3521a19c 100644
> > --- a/hw/net/trace-events
> > +++ b/hw/net/trace-events
> > @@ -237,6 +237,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.29.2
> >
>

[-- Attachment #2: Type: text/html, Size: 4543 bytes --]

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

[Alexander Duyck]

On Thu, Dec 3, 2020 at 5:00 AM Andrew Melnychenko <andrew@daynix.com> wrote:
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441

So the bugzilla seems to be reporting that the NIC operstate is being
misreported when qemu has configured the link down. Based on the
description it isn't clear to me how this patch addresses that. Some
documentation on how you reproduced the issue, and what was seen
before and after this patch would be useful.

> Added ICR clearing if there is IMS bit - according to the note by

Should probably be "Add" instead of "Added". Same for the title of the patch.

> 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 095c01ebc6..9705f5c52e 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2624,6 +2624,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
> +     */
> +    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;
> +    }
> +
>      trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
>      e1000e_update_interrupt_state(core);
>      return ret;

Changes like this have historically been problematic. I am curious
what testing had been done on this and with what drivers? Keep in mind
that we have to support several flavors of BSD, Windows, and Linux
with this.

> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index 5db45456d9..2c3521a19c 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -237,6 +237,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.29.2
>

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

[Andrew Melnychenko]

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
Added ICR clearing if there is IMS bit - 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 095c01ebc6..9705f5c52e 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2624,6 +2624,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
+     */
+    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;
+    }
+
     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 5db45456d9..2c3521a19c 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -237,6 +237,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.29.2