diff mbox series

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

Message ID 20201203133236.222207-1-andrew@daynix.com (mailing list archive)
State New, archived
Headers show
Series [v2] e1000e: Added ICR clearing by corresponding IMS bit. | expand

Commit Message

Andrew Melnychenko Dec. 3, 2020, 1:32 p.m. UTC
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(+)

Comments

Alexander Duyck Dec. 3, 2020, 5:57 p.m. UTC | #1
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
>
Andrew Melnychenko Dec. 14, 2020, 11:42 a.m. UTC | #2
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
> >
>
Alexander Duyck Dec. 14, 2020, 4:31 p.m. UTC | #3
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
>> >
diff mbox series

Patch

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"