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

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

[Dmitry Fleytman]

Following patch fixes improper RX ring management E1000 code

Changes from version 2:
    Commit message beautification

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):
  e1000: drop check_rxov, always treat RX ring with RDH == RDT as empty

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

-- 
1.7.11.4

[Qemu-devel] [PATCH V3] e1000: drop check_rxov, always treat RX ring with RDH == 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 V3] e1000: drop check_rxov, always treat RX ring with RDH == RDT as empty

[Stefan Hajnoczi]

On Fri, Oct 19, 2012 at 07:56:55AM +0200, Dmitry Fleytman wrote:
> 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(-)

Applied to the net tree:
http://github.com/stefanha/qemu/commits/net

Thanks for your efforts in squashing this bug!  Glad it was possible to
drop check_rxov.

Stefan

Re: [Qemu-devel] [PATCH V3] e1000: drop check_rxov, always treat RX ring with RDH == RDT as empty

[Dmitry Fleytman]

Thanks Stefan,

It was my very first idea to drop check_rxov and solve the problem,
however for some reason I was sure that it required to emulate real HW behavior.

I'm glad we clarified this.

Regards,
Dmitry Fleytman

On Fri, Oct 19, 2012 at 9:52 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Oct 19, 2012 at 07:56:55AM +0200, Dmitry Fleytman wrote:
>> 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(-)
>
> Applied to the net tree:
> http://github.com/stefanha/qemu/commits/net
>
> Thanks for your efforts in squashing this bug!  Glad it was possible to
> drop check_rxov.
>
> Stefan



-- 
Dmitry Fleytman
Technology Expert and Consultant,

Daynix Computing Ltd.

Cell: +972-54-2819481

Skype: dmitry.fleytman