[Qemu-devel] [PATCH 0/7] e1000e: A few fixes for RX data path

[Qemu-devel] [PATCH 0/7] e1000e: A few fixes for RX data path

[Dmitry Fleytman]

This series contains a few RX logic fixes for e1000e.

These fixes improve compliance to HW device specification
and fix occasional interruptions of RX traffic with
the latest linux driver.

Dmitry Fleytman (7):
  e1000e: Flush all receive queues on receive enable
  e1000e: Flush receive queue on link up
  e1000e: Fix CTRL_EXT.EIAME behavior
  e1000e: Fix PBACLR implementation
  e1000e: Fix OTHER interupts processing for MSI-X
  e1000e: Fix spurious RX TCP ACK interrupts
  e1000e: Fix EIAC register implementation

 hw/net/e1000e.c      |  2 +-
 hw/net/e1000e_core.c | 30 +++++++++++++++++++-----------
 hw/net/e1000e_core.h |  3 +++
 hw/net/trace-events  |  2 +-
 4 files changed, 24 insertions(+), 13 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/7] e1000e: Flush all receive queues on receive enable

[Dmitry Fleytman]

Before this patch first netdev queue only was flushed.

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/e1000e.c      | 2 +-
 hw/net/e1000e_core.c | 2 +-
 hw/net/e1000e_core.h | 3 +++
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index bad43f4..4994e1c 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -400,7 +400,7 @@ static void e1000e_write_config(PCIDevice *pci_dev, uint32_t address,
 
     if (range_covers_byte(address, len, PCI_COMMAND) &&
         (pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
-        qemu_flush_queued_packets(qemu_get_queue(s->nic));
+        e1000e_start_recv(&s->core);
     }
 }
 
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index badb1fe..ade4d25 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -953,7 +953,7 @@ e1000e_has_rxbufs(E1000ECore *core, const E1000E_RingInfo *r,
                          core->rx_desc_buf_size;
 }
 
-static inline void
+void
 e1000e_start_recv(E1000ECore *core)
 {
     int i;
diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h
index 5f413a9..1ff6978 100644
--- a/hw/net/e1000e_core.h
+++ b/hw/net/e1000e_core.h
@@ -144,3 +144,6 @@ e1000e_receive(E1000ECore *core, const uint8_t *buf, size_t size);
 
 ssize_t
 e1000e_receive_iov(E1000ECore *core, const struct iovec *iov, int iovcnt);
+
+void
+e1000e_start_recv(E1000ECore *core);
-- 
2.7.4

[Qemu-devel] [PATCH 2/7] e1000e: Flush receive queues on link up

[Dmitry Fleytman]

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/e1000e_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index ade4d25..a9603e0 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -1807,6 +1807,7 @@ e1000e_core_set_link_status(E1000ECore *core)
                                    core->autoneg_timer);
         } else {
             e1000x_update_regs_on_link_up(core->mac, core->phy[0]);
+            e1000e_start_recv(core);
         }
     }
 
@@ -2187,6 +2188,8 @@ e1000e_autoneg_timer(void *opaque)
     E1000ECore *core = opaque;
     if (!qemu_get_queue(core->owner_nic)->link_down) {
         e1000x_update_regs_on_autoneg_done(core->mac, core->phy[0]);
+        e1000e_start_recv(core);
+
         e1000e_update_flowctl_status(core);
         /* signal link status change to the guest */
         e1000e_set_interrupt_cause(core, E1000_ICR_LSC);
-- 
2.7.4

[Qemu-devel] [PATCH 3/7] e1000e: Fix CTRL_EXT.EIAME behavior

[Dmitry Fleytman]

CTRL_EXT.EIAME bit controls clearing of IAM bits,
but current code clears IMS bits instead.

See spec. 10.2.2.5 Extended Device Control Register.

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/e1000e_core.c | 4 ++--
 hw/net/trace-events  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index a9603e0..22765cb 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2008,8 +2008,8 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg)
     }
 
     if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_EIAME) {
-        trace_e1000e_irq_ims_clear_eiame(core->mac[IAM], cause);
-        e1000e_clear_ims_bits(core, core->mac[IAM] & cause);
+        trace_e1000e_irq_iam_clear_eiame(core->mac[IAM], cause);
+        core->mac[IAM] &= ~cause;
     }
 
     trace_e1000e_irq_icr_clear_eiac(core->mac[ICR], core->mac[EIAC]);
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 8d38d77..aa83a7c 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -223,7 +223,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_ims_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 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"
 e1000e_irq_fire_delayed_interrupts(void) "Firing delayed interrupts"
-- 
2.7.4

[Qemu-devel] [PATCH 4/7] e1000e: Fix PBACLR implementation

[Dmitry Fleytman]

This patch fixes incorrect check for
interrypt type being used.

PBSCLR register is valid for MSI-X only.

See spec. 10.2.3.13 MSI—X PBA Clear

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/e1000e_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 22765cb..c38ed10 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val)
 
     core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK;
 
-    if (msix_enabled(core->owner)) {
+    if (!msix_enabled(core->owner)) {
         return;
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH 5/7] e1000e: Fix OTHER interrupts processing for MSI-X

[Dmitry Fleytman]

Interrupt mask for legacy OTHER causes should
not apply to MSI-X OTHER cause.

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/e1000e_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index c38ed10..e70e669 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2131,7 +2131,7 @@ e1000e_update_interrupt_state(E1000ECore *core)
 
     /* Set ICR[OTHER] for MSI-X */
     if (is_msix) {
-        if (core->mac[ICR] & core->mac[IMS] & E1000_ICR_OTHER_CAUSES) {
+        if (core->mac[ICR] & E1000_ICR_OTHER_CAUSES) {
             core->mac[ICR] |= E1000_ICR_OTHER;
             trace_e1000e_irq_add_msi_other(core->mac[ICR]);
         }
-- 
2.7.4

[Qemu-devel] [PATCH 6/7] e1000e: Fix spurious RX TCP ACK interrupts

[Dmitry Fleytman]

Do not raise ACK interrupts when
RFCTL.ACKDIS bit is set (see spec. 10.2.5.16).

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/e1000e_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index e70e669..f4ec79f 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -1710,7 +1710,8 @@ e1000e_receive_iov(E1000ECore *core, const struct iovec *iov, int iovcnt)
         }
 
         /* Perform ACK receive detection */
-        if (e1000e_is_tcp_ack(core, core->rx_pkt)) {
+        if  (!(core->mac[RFCTL] & E1000_RFCTL_ACK_DIS) &&
+             (e1000e_is_tcp_ack(core, core->rx_pkt))) {
             n |= E1000_ICS_ACK;
         }
 
-- 
2.7.4

[Qemu-devel] [PATCH 7/7] e1000e: Fix EIAC register implementation

[Dmitry Fleytman]

This patch fixes 2 issues:

1. Bits set in EIAC register should be cleared
   from IMS when EIAM is not used.
2. Only bit that corresonds to the interrupt being
   raised should be cleared.

See spec. 10.2.4.7 Interrupt Auto Clear

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/e1000e_core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index f4ec79f..3c38980 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2015,13 +2015,17 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg)
 
     trace_e1000e_irq_icr_clear_eiac(core->mac[ICR], core->mac[EIAC]);
 
-    if (core->mac[EIAC] & E1000_ICR_OTHER) {
-        effective_eiac = (core->mac[EIAC] & E1000_EIAC_MASK) |
-                         E1000_ICR_OTHER_CAUSES;
-    } else {
-        effective_eiac = core->mac[EIAC] & E1000_EIAC_MASK;
+    effective_eiac = core->mac[EIAC] & cause;
+
+    if (effective_eiac == E1000_ICR_OTHER) {
+        effective_eiac |= E1000_ICR_OTHER_CAUSES;
     }
+
     core->mac[ICR] &= ~effective_eiac;
+
+    if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
+        core->mac[IMS] &= ~effective_eiac;
+    }
 }
 
 static void
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/7] e1000e: A few fixes for RX data path

[Dmitry Fleytman]

Hi qemu-devel,

Ping.

~Dmitry

> On 15 Sep 2016, at 09:14 AM, Dmitry Fleytman <dmitry@daynix.com> wrote:
> 
> This series contains a few RX logic fixes for e1000e.
> 
> These fixes improve compliance to HW device specification
> and fix occasional interruptions of RX traffic with
> the latest linux driver.
> 
> Dmitry Fleytman (7):
>  e1000e: Flush all receive queues on receive enable
>  e1000e: Flush receive queue on link up
>  e1000e: Fix CTRL_EXT.EIAME behavior
>  e1000e: Fix PBACLR implementation
>  e1000e: Fix OTHER interupts processing for MSI-X
>  e1000e: Fix spurious RX TCP ACK interrupts
>  e1000e: Fix EIAC register implementation
> 
> hw/net/e1000e.c      |  2 +-
> hw/net/e1000e_core.c | 30 +++++++++++++++++++-----------
> hw/net/e1000e_core.h |  3 +++
> hw/net/trace-events  |  2 +-
> 4 files changed, 24 insertions(+), 13 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH 4/7] e1000e: Fix PBACLR implementation

[Jason Wang]

On 2016年09月15日 14:14, Dmitry Fleytman wrote:
> This patch fixes incorrect check for
> interrypt type being used.
>
> PBSCLR register is valid for MSI-X only.
>
> See spec. 10.2.3.13 MSI—X PBA Clear
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> ---
>   hw/net/e1000e_core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 22765cb..c38ed10 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val)
>   
>       core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK;
>   
> -    if (msix_enabled(core->owner)) {
> +    if (!msix_enabled(core->owner)) {
>           return;
>       }
>   

Spec also said "writing 0b has no effect". So we'd better implement this 
behavior too?

Re: [Qemu-devel] [PATCH 4/7] e1000e: Fix PBACLR implementation

[Dmitry Fleytman]

> On 22 Sep 2016, at 09:40 AM, Jason Wang <jasowang@redhat.com> wrote:
> 
> 
> 
> On 2016年09月15日 14:14, Dmitry Fleytman wrote:
>> This patch fixes incorrect check for
>> interrypt type being used.
>> 
>> PBSCLR register is valid for MSI-X only.
>> 
>> See spec. 10.2.3.13 MSI—X PBA Clear
>> 
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> ---
>>  hw/net/e1000e_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> index 22765cb..c38ed10 100644
>> --- a/hw/net/e1000e_core.c
>> +++ b/hw/net/e1000e_core.c
>> @@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val)
>>        core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK;
>>  -    if (msix_enabled(core->owner)) {
>> +    if (!msix_enabled(core->owner)) {
>>          return;
>>      }
>>  
> 
> Spec also said "writing 0b has no effect". So we'd better implement this behavior too?


Hi Jason,

Not sure I understand you correctly.

With current implementation, writing 0b does nothing
except that it changes value of PBACLR being read.

I just verified that physical device behaves exactly like this.

Is this what you meant?

~Dmitry

Re: [Qemu-devel] [PATCH 4/7] e1000e: Fix PBACLR implementation

[Jason Wang]

On 2016年09月22日 17:01, Dmitry Fleytman wrote:
>> On 22 Sep 2016, at 09:40 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>>
>> On 2016年09月15日 14:14, Dmitry Fleytman wrote:
>>> This patch fixes incorrect check for
>>> interrypt type being used.
>>>
>>> PBSCLR register is valid for MSI-X only.
>>>
>>> See spec. 10.2.3.13 MSI—X PBA Clear
>>>
>>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>>> ---
>>>   hw/net/e1000e_core.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>>> index 22765cb..c38ed10 100644
>>> --- a/hw/net/e1000e_core.c
>>> +++ b/hw/net/e1000e_core.c
>>> @@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val)
>>>         core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK;
>>>   -    if (msix_enabled(core->owner)) {
>>> +    if (!msix_enabled(core->owner)) {
>>>           return;
>>>       }
>>>   
>> Spec also said "writing 0b has no effect". So we'd better implement this behavior too?
>
> Hi Jason,
>
> Not sure I understand you correctly.
>
> With current implementation, writing 0b does nothing
> except that it changes value of PBACLR being read.
>
> I just verified that physical device behaves exactly like this.
>
> Is this what you meant?
>
> ~Dmitry
>

Yes, then it looks fine to me.

Thanks

Re: [Qemu-devel] [PATCH 0/7] e1000e: A few fixes for RX data path

[Jason Wang]

On 2016年09月15日 14:14, Dmitry Fleytman wrote:
> This series contains a few RX logic fixes for e1000e.
>
> These fixes improve compliance to HW device specification
> and fix occasional interruptions of RX traffic with
> the latest linux driver.
>
> Dmitry Fleytman (7):
>    e1000e: Flush all receive queues on receive enable
>    e1000e: Flush receive queue on link up
>    e1000e: Fix CTRL_EXT.EIAME behavior
>    e1000e: Fix PBACLR implementation
>    e1000e: Fix OTHER interupts processing for MSI-X
>    e1000e: Fix spurious RX TCP ACK interrupts
>    e1000e: Fix EIAC register implementation
>
>   hw/net/e1000e.c      |  2 +-
>   hw/net/e1000e_core.c | 30 +++++++++++++++++++-----------
>   hw/net/e1000e_core.h |  3 +++
>   hw/net/trace-events  |  2 +-
>   4 files changed, 24 insertions(+), 13 deletions(-)
>

Applied, thanks.