[Qemu-devel] [PATCH V2] E1000 RX ring management fix

[Qemu-devel] [PATCH V2] E1000 RX ring management fix

[Dmitry Fleytman]

Following patch fixes improper RX ring management E1000 code

Changes from version 1:
    1st patch changed so it drops check_rxov field because it is redundant and leads to race conditions
    See commit description for details

    2nd patch (live migration) dropped because corresponding field got deleted

Also I've made short experiment with an Intel adapter controlled by e1000e driver.
Indeed I saw no RX indication attempt when RX ring's RDH and RDT are equal.

Dmitry Fleytman (1):
  Drop check_rxov, always treat RX ring with RHD == RDT as empty

 hw/e1000.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH V2] Drop check_rxov, always treat RX ring with RHD == RDT as empty

[Dmitry Fleytman]

Real HW always treats RX ring with RDH == RDT as empty.
Emulation is supposed to behave the same.

Reported-by: Chris Webb <chris.webb@elastichosts.com>
Reported-by: Richard Davies <richard.davies@elastichosts.com>
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/e1000.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 63fee10..ab39d47 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -92,7 +92,6 @@ typedef struct E1000State_st {
 
     uint32_t rxbuf_size;
     uint32_t rxbuf_min_shift;
-    int check_rxov;
     struct e1000_tx {
         unsigned char header[256];
         unsigned char vlan_header[4];
@@ -741,11 +740,11 @@ static bool e1000_has_rxbufs(E1000State *s, size_t total_size)
     int bufs;
     /* Fast-path short packets */
     if (total_size <= s->rxbuf_size) {
-        return s->mac_reg[RDH] != s->mac_reg[RDT] || !s->check_rxov;
+        return s->mac_reg[RDH] != s->mac_reg[RDT];
     }
     if (s->mac_reg[RDH] < s->mac_reg[RDT]) {
         bufs = s->mac_reg[RDT] - s->mac_reg[RDH];
-    } else if (s->mac_reg[RDH] > s->mac_reg[RDT] || !s->check_rxov) {
+    } else if (s->mac_reg[RDH] > s->mac_reg[RDT]) {
         bufs = s->mac_reg[RDLEN] /  sizeof(struct e1000_rx_desc) +
             s->mac_reg[RDT] - s->mac_reg[RDH];
     } else {
@@ -848,7 +847,6 @@ e1000_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 
         if (++s->mac_reg[RDH] * sizeof(desc) >= s->mac_reg[RDLEN])
             s->mac_reg[RDH] = 0;
-        s->check_rxov = 1;
         /* see comment in start_xmit; same here */
         if (s->mac_reg[RDH] == rdh_start) {
             DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n",
@@ -925,7 +923,6 @@ mac_writereg(E1000State *s, int index, uint32_t val)
 static void
 set_rdt(E1000State *s, int index, uint32_t val)
 {
-    s->check_rxov = 0;
     s->mac_reg[index] = val & 0xffff;
     if (e1000_has_rxbufs(s, 1)) {
         qemu_flush_queued_packets(&s->nic->nc);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH V2] Drop check_rxov, always treat RX ring with RHD == RDT as empty

[Peter Maydell]

On 18 October 2012 19:59, Dmitry Fleytman <dmitry@daynix.com> wrote:
> Real HW always treats RX ring with RDH == RDT as empty.
> Emulation is supposed to behave the same.

If you need to do a v3 of this patch for some reason, it would
be nice to amend the summary line so it started "e1000: " so
people scanning git logs know which device is affected.

Also your commit message body says "RDH" but the subject
says "RHD"...

thanks
-- PMM

Re: [Qemu-devel] [PATCH V2] Drop check_rxov, always treat RX ring with RHD == RDT as empty

[Dmitry Fleytman]

Thanks, Peter

I'll resend the patch.

Sent from my iPad

On Oct 18, 2012, at 9:20 PM, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 18 October 2012 19:59, Dmitry Fleytman <dmitry@daynix.com> wrote:
>> Real HW always treats RX ring with RDH == RDT as empty.
>> Emulation is supposed to behave the same.
> 
> If you need to do a v3 of this patch for some reason, it would
> be nice to amend the summary line so it started "e1000: " so
> people scanning git logs know which device is affected.
> 
> Also your commit message body says "RDH" but the subject
> says "RHD"...
> 
> thanks
> -- PMM