[Qemu-devel] [PATCH arm-devs v1 00/13] Cadence GEM Bugfixes and missing features

[Qemu-devel] [PATCH arm-devs v1 00/13] Cadence GEM Bugfixes and missing features

[Peter Crosthwaite]

Hi Peter,

This series updated cadence GEM (ethernet in Zynq) with a number of
bugfixes and enhancements. Most of this has come out of some stress
testing we have done internally.

I have also testing briefly using mainline Linux GEM driver.

root@zynq:~# wget
http://people.debian.org/~aurel32/qemu/arm/debian_lenny_arm_staandard.qcow2
Connecting to proxy.xilinx.com:8080 (149.199.33.100:8080)
debian_lenny_arm_sta  18% |*****                          | 29898k
0:00:21 ETArandom: nonblocking pool is initialized
debian_lenny_arm_sta 100% |*******************************|   157M
0:00:00 ETA
root@zynq:~# md5sum debian_lenny_arm_standard.qcow2
7be70fc105525803fae5e095f5b4ecb5  debian_lenny_arm_standard.qcow2

Regards,
Peter


Edgar E. Iglesias (1):
  net/cadence_gem: Update DMA rx descriptors as we process them

Peter Crosthwaite (12):
  net/cadence_gem: Implement mac level loopback mode
  net/cadence_gem: Don't assert against 0 buffer address
  net/cadence_gem: simplify rx buf descriptor walking
  net/cadence_gem: Prefetch rx descriptors ASAP
  net/cadence_gem: Implement RX descriptor match mode flags
  net/cadence_gem: Implement SAR match bit in rx desc
  net/cadence_gem: Implement SAR (de)activation
  net/cadence_gem: Fix rx multi-fragment packets
  net/cadence_gem: Fix small packet FCS stripping
  net/cadence_gem: Fix register w1c logic
  net/cadence_gem: Improve can_receive debug printfery
  net/cadence_gem: Dont rx packets when no rx buffer available

 hw/net/cadence_gem.c | 264 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 170 insertions(+), 94 deletions(-)

-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 01/13] net/cadence_gem: Implement mac level loopback mode

[Peter Crosthwaite]

Cadence GEM has a MAC level loopback mode. Implement. Use the same basic
operation as the already implemented PHY loopback.

Reported-by: Deepika Dhamija <deepika@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 4a355bb..a31801d 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -893,7 +893,7 @@ static void gem_transmit(GemState *s)
             gem_transmit_updatestats(s, tx_packet, total_bytes);
 
             /* Send the packet somewhere */
-            if (s->phy_loop) {
+            if (s->phy_loop || (s->regs[GEM_NWCTRL] & GEM_NWCTRL_LOCALLOOP)) {
                 gem_receive(qemu_get_queue(s->nic), tx_packet, total_bytes);
             } else {
                 qemu_send_packet(qemu_get_queue(s->nic), tx_packet,
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 02/13] net/cadence_gem: Update DMA rx descriptors as we process them

[Peter Crosthwaite]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

We were updating the ownership bit of all descriptors if packets
get split and written through several descriptors.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index a31801d..b84ee60 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -592,6 +592,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     unsigned   rxbuf_offset;
     uint8_t    rxbuf[2048];
     uint8_t   *rxbuf_ptr;
+    bool first_desc = true;
 
     s = qemu_get_nic_opaque(nc);
 
@@ -701,6 +702,21 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
                                   rxbuf_ptr, MIN(bytes_to_copy, rxbufsize));
         bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
         rxbuf_ptr += MIN(bytes_to_copy, rxbufsize);
+
+        /* Update the descriptor.  */
+        if (first_desc) {
+            rx_desc_set_sof(desc);
+            first_desc = false;
+        }
+        if (bytes_to_copy == 0) {
+            rx_desc_set_eof(desc);
+            rx_desc_set_length(desc, size);
+        }
+        rx_desc_set_ownership(desc);
+        /* Descriptor write-back.  */
+        cpu_physical_memory_write(packet_desc_addr,
+                                  (uint8_t *)&desc[0], sizeof(desc));
+
         if (bytes_to_copy == 0) {
             break;
         }
@@ -716,12 +732,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     DB_PRINT("set length: %ld, EOF on descriptor 0x%x\n", size,
             (unsigned)packet_desc_addr);
 
-    /* Update last descriptor with EOF and total length */
-    rx_desc_set_eof(desc);
-    rx_desc_set_length(desc, size);
-    cpu_physical_memory_write(packet_desc_addr,
-                              (uint8_t *)&desc[0], sizeof(desc));
-
     /* Advance RX packet descriptor Q */
     last_desc_addr = packet_desc_addr;
     packet_desc_addr = s->rx_desc_addr;
@@ -734,20 +744,9 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         s->rx_desc_addr += 8;
     }
 
-    DB_PRINT("set SOF, OWN on descriptor 0x%08x\n", (unsigned)packet_desc_addr);
-
     /* Count it */
     gem_receive_updatestats(s, buf, size);
 
-    /* Update first descriptor (which could also be the last) */
-    /* read descriptor */
-    cpu_physical_memory_read(packet_desc_addr,
-                             (uint8_t *)&desc[0], sizeof(desc));
-    rx_desc_set_sof(desc);
-    rx_desc_set_ownership(desc);
-    cpu_physical_memory_write(packet_desc_addr,
-                              (uint8_t *)&desc[0], sizeof(desc));
-
     s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_FRMRCVD;
     s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
 
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 03/13] net/cadence_gem: Don't assert against 0 buffer address

[Peter Crosthwaite]

This has no real hardware analog. Leave the error message in is as
it is almost certainly a guest error, but fallthrough to the expected
behaviour.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index b84ee60..3f30caf 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -694,7 +694,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         if (rx_desc_get_buffer(desc) == 0) {
             DB_PRINT("Invalid RX buffer (NULL) for descriptor 0x%x\n",
                      (unsigned)packet_desc_addr);
-            break;
         }
 
         /* Copy packet data to emulated DMA buffer */
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 04/13] net/cadence_gem: simplify rx buf descriptor walking

[Peter Crosthwaite]

There was a replication of the rx descriptor address walking logic.
Reorder the flow control to remove. This refactoring also obsoletes
the local variables packet_desc_addr and last_desc_addr.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 39 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 3f30caf..73ac0d8 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -586,7 +586,6 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
 static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
     unsigned    desc[2];
-    hwaddr packet_desc_addr, last_desc_addr;
     GemState *s;
     unsigned   rxbufsize, bytes_to_copy;
     unsigned   rxbuf_offset;
@@ -667,17 +666,16 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 
     DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
 
-    packet_desc_addr = s->rx_desc_addr;
-    while (1) {
-        DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);
+    while (bytes_to_copy) {
+        DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
         /* read current descriptor */
-        cpu_physical_memory_read(packet_desc_addr,
+        cpu_physical_memory_read(s->rx_desc_addr,
                                  (uint8_t *)&desc[0], sizeof(desc));
 
         /* Descriptor owned by software ? */
         if (rx_desc_get_ownership(desc) == 1) {
             DB_PRINT("descriptor 0x%x owned by sw.\n",
-                     (unsigned)packet_desc_addr);
+                     (unsigned)s->rx_desc_addr);
             s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
             s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
             /* Handle interrupt consequences */
@@ -693,7 +691,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
          */
         if (rx_desc_get_buffer(desc) == 0) {
             DB_PRINT("Invalid RX buffer (NULL) for descriptor 0x%x\n",
-                     (unsigned)packet_desc_addr);
+                     (unsigned)s->rx_desc_addr);
         }
 
         /* Copy packet data to emulated DMA buffer */
@@ -713,36 +711,19 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         }
         rx_desc_set_ownership(desc);
         /* Descriptor write-back.  */
-        cpu_physical_memory_write(packet_desc_addr,
+        cpu_physical_memory_write(s->rx_desc_addr,
                                   (uint8_t *)&desc[0], sizeof(desc));
 
-        if (bytes_to_copy == 0) {
-            break;
-        }
-
         /* Next descriptor */
         if (rx_desc_get_wrap(desc)) {
-            packet_desc_addr = s->regs[GEM_RXQBASE];
+            DB_PRINT("wrapping RX descriptor list\n");
+            s->rx_desc_addr = s->regs[GEM_RXQBASE];
         } else {
-            packet_desc_addr += 8;
+            DB_PRINT("incrementing RX descriptor list\n");
+            s->rx_desc_addr += 8;
         }
     }
 
-    DB_PRINT("set length: %ld, EOF on descriptor 0x%x\n", size,
-            (unsigned)packet_desc_addr);
-
-    /* Advance RX packet descriptor Q */
-    last_desc_addr = packet_desc_addr;
-    packet_desc_addr = s->rx_desc_addr;
-    s->rx_desc_addr = last_desc_addr;
-    if (rx_desc_get_wrap(desc)) {
-        s->rx_desc_addr = s->regs[GEM_RXQBASE];
-        DB_PRINT("wrapping RX descriptor list\n");
-    } else {
-        DB_PRINT("incrementing RX descriptor list\n");
-        s->rx_desc_addr += 8;
-    }
-
     /* Count it */
     gem_receive_updatestats(s, buf, size);
 
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 05/13] net/cadence_gem: Prefetch rx descriptors ASAP

[Peter Crosthwaite]

The real hardware prefetches rx buffer descriptors ASAP and
potentially throws relevant interrupts following the fetch
even in the absence of a recieved packet.

Reported-by: Deepika Dhamija <deepika@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 64 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 73ac0d8..e5a6d87 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -346,6 +346,8 @@ typedef struct GemState {
     uint32_t rx_desc_addr;
     uint32_t tx_desc_addr;
 
+    unsigned rx_desc[2];
+
 } GemState;
 
 /* The broadcast MAC address: 0xFFFFFFFFFFFF */
@@ -579,13 +581,30 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
     return GEM_RX_REJECT;
 }
 
+static void gem_get_rx_desc(GemState *s)
+{
+        DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
+        /* read current descriptor */
+        cpu_physical_memory_read(s->rx_desc_addr,
+                                 (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
+
+        /* Descriptor owned by software ? */
+        if (rx_desc_get_ownership(s->rx_desc) == 1) {
+            DB_PRINT("descriptor 0x%x owned by sw.\n",
+                     (unsigned)s->rx_desc_addr);
+            s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
+            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
+            /* Handle interrupt consequences */
+            gem_update_int_status(s);
+        }
+}
+
 /*
  * gem_receive:
  * Fit a packet handed to us by QEMU into the receive descriptor ring.
  */
 static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
-    unsigned    desc[2];
     GemState *s;
     unsigned   rxbufsize, bytes_to_copy;
     unsigned   rxbuf_offset;
@@ -595,11 +614,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 
     s = qemu_get_nic_opaque(nc);
 
-    /* Do nothing if receive is not enabled. */
-    if (!gem_can_receive(nc)) {
-        return -1;
-    }
-
     /* Is this destination MAC address "for us" ? */
     if (gem_mac_address_filter(s, buf) == GEM_RX_REJECT) {
         return -1;
@@ -667,61 +681,52 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
 
     while (bytes_to_copy) {
-        DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
-        /* read current descriptor */
-        cpu_physical_memory_read(s->rx_desc_addr,
-                                 (uint8_t *)&desc[0], sizeof(desc));
-
-        /* Descriptor owned by software ? */
-        if (rx_desc_get_ownership(desc) == 1) {
-            DB_PRINT("descriptor 0x%x owned by sw.\n",
-                     (unsigned)s->rx_desc_addr);
-            s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
-            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
-            /* Handle interrupt consequences */
-            gem_update_int_status(s);
+        /* Do nothing if receive is not enabled. */
+        if (!gem_can_receive(nc)) {
+            assert(!first_desc);
             return -1;
         }
 
         DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize),
-                rx_desc_get_buffer(desc));
+                rx_desc_get_buffer(s->rx_desc));
 
         /*
          * Let's have QEMU lend a helping hand.
          */
-        if (rx_desc_get_buffer(desc) == 0) {
+        if (rx_desc_get_buffer(s->rx_desc) == 0) {
             DB_PRINT("Invalid RX buffer (NULL) for descriptor 0x%x\n",
                      (unsigned)s->rx_desc_addr);
         }
 
         /* Copy packet data to emulated DMA buffer */
-        cpu_physical_memory_write(rx_desc_get_buffer(desc) + rxbuf_offset,
+        cpu_physical_memory_write(rx_desc_get_buffer(s->rx_desc) + rxbuf_offset,
                                   rxbuf_ptr, MIN(bytes_to_copy, rxbufsize));
         bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
         rxbuf_ptr += MIN(bytes_to_copy, rxbufsize);
 
         /* Update the descriptor.  */
         if (first_desc) {
-            rx_desc_set_sof(desc);
+            rx_desc_set_sof(s->rx_desc);
             first_desc = false;
         }
         if (bytes_to_copy == 0) {
-            rx_desc_set_eof(desc);
-            rx_desc_set_length(desc, size);
+            rx_desc_set_eof(s->rx_desc);
+            rx_desc_set_length(s->rx_desc, size);
         }
-        rx_desc_set_ownership(desc);
+        rx_desc_set_ownership(s->rx_desc);
         /* Descriptor write-back.  */
         cpu_physical_memory_write(s->rx_desc_addr,
-                                  (uint8_t *)&desc[0], sizeof(desc));
+                                  (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
 
         /* Next descriptor */
-        if (rx_desc_get_wrap(desc)) {
+        if (rx_desc_get_wrap(s->rx_desc)) {
             DB_PRINT("wrapping RX descriptor list\n");
             s->rx_desc_addr = s->regs[GEM_RXQBASE];
         } else {
             DB_PRINT("incrementing RX descriptor list\n");
             s->rx_desc_addr += 8;
         }
+        gem_get_rx_desc(s);
     }
 
     /* Count it */
@@ -1061,6 +1066,9 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
     /* Handle register write side effects */
     switch (offset) {
     case GEM_NWCTRL:
+        if (val & GEM_NWCTRL_RXENA) {
+            gem_get_rx_desc(s);
+        }
         if (val & GEM_NWCTRL_TXSTART) {
             gem_transmit(s);
         }
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 06/13] net/cadence_gem: Implement RX descriptor match mode flags

[Peter Crosthwaite]

The various Rx packet address matching mode flags were not being set in
the rx descriptor. Implement.

Reported-by: Deepika Dhamija <deepika@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 80 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 15 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index e5a6d87..4fbfb8d 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -222,8 +222,13 @@
 #define PHY_REG_INT_ST_ENERGY   0x0010
 
 /***********************************************************************/
-#define GEM_RX_REJECT  1
-#define GEM_RX_ACCEPT  0
+#define GEM_RX_REJECT                   (-1)
+#define GEM_RX_PROM_ACCEPT              (-2)
+#define GEM_RX_BROADCAST_ACCEPT         (-3)
+#define GEM_RX_MULTICAST_HASH_ACCEPT    (-4)
+#define GEM_RX_UNICAST_HASH_ACCEPT      (-5)
+
+#define GEM_RX_SAR_ACCEPT               0
 
 /***********************************************************************/
 
@@ -236,6 +241,12 @@
 #define DESC_0_RX_WRAP 0x00000002
 #define DESC_0_RX_OWNERSHIP 0x00000001
 
+#define R_DESC_1_RX_SAR_SHIFT           25
+#define R_DESC_1_RX_SAR_LENGTH          2
+#define R_DESC_1_RX_UNICAST_HASH        (1 << 29)
+#define R_DESC_1_RX_MULTICAST_HASH      (1 << 30)
+#define R_DESC_1_RX_BROADCAST           (1 << 31)
+
 #define DESC_1_RX_SOF 0x00004000
 #define DESC_1_RX_EOF 0x00008000
 
@@ -315,6 +326,27 @@ static inline void rx_desc_set_length(unsigned *desc, unsigned len)
     desc[1] |= len;
 }
 
+static inline void rx_desc_set_broadcast(unsigned *desc)
+{
+    desc[1] |= R_DESC_1_RX_BROADCAST;
+}
+
+static inline void rx_desc_set_unicast_hash(unsigned *desc)
+{
+    desc[1] |= R_DESC_1_RX_UNICAST_HASH;
+}
+
+static inline void rx_desc_set_multicast_hash(unsigned *desc)
+{
+    desc[1] |= R_DESC_1_RX_MULTICAST_HASH;
+}
+
+static inline void rx_desc_set_sar(unsigned *desc, int sar_idx)
+{
+    desc[1] = deposit32(desc[1], R_DESC_1_RX_SAR_SHIFT, R_DESC_1_RX_SAR_LENGTH,
+                        sar_idx);
+}
+
 #define TYPE_CADENCE_GEM "cadence_gem"
 #define GEM(obj) OBJECT_CHECK(GemState, (obj), TYPE_CADENCE_GEM)
 
@@ -527,9 +559,6 @@ static unsigned calc_mac_hash(const uint8_t *mac)
 /*
  * gem_mac_address_filter:
  * Accept or reject this destination address?
- * Returns:
- * GEM_RX_REJECT: reject
- * GEM_RX_ACCEPT: accept
  */
 static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
 {
@@ -538,7 +567,7 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
 
     /* Promiscuous mode? */
     if (s->regs[GEM_NWCFG] & GEM_NWCFG_PROMISC) {
-        return GEM_RX_ACCEPT;
+        return GEM_RX_PROM_ACCEPT;
     }
 
     if (!memcmp(packet, broadcast_addr, 6)) {
@@ -546,7 +575,7 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
         if (s->regs[GEM_NWCFG] & GEM_NWCFG_BCAST_REJ) {
             return GEM_RX_REJECT;
         }
-        return GEM_RX_ACCEPT;
+        return GEM_RX_BROADCAST_ACCEPT;
     }
 
     /* Accept packets -w- hash match? */
@@ -557,24 +586,24 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
         hash_index = calc_mac_hash(packet);
         if (hash_index < 32) {
             if (s->regs[GEM_HASHLO] & (1<<hash_index)) {
-                return GEM_RX_ACCEPT;
+                return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
+                                           GEM_RX_UNICAST_HASH_ACCEPT;
             }
         } else {
             hash_index -= 32;
             if (s->regs[GEM_HASHHI] & (1<<hash_index)) {
-                return GEM_RX_ACCEPT;
+                return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
+                                           GEM_RX_UNICAST_HASH_ACCEPT;
             }
         }
     }
 
     /* Check all 4 specific addresses */
     gem_spaddr = (uint8_t *)&(s->regs[GEM_SPADDR1LO]);
-    for (i = 0; i < 4; i++) {
-        if (!memcmp(packet, gem_spaddr, 6)) {
-            return GEM_RX_ACCEPT;
+    for (i = 3; i >= 0; i--) {
+        if (!memcmp(packet, gem_spaddr + 8 * i, 6)) {
+            return GEM_RX_SAR_ACCEPT + i;
         }
-
-        gem_spaddr += 8;
     }
 
     /* No address match; reject the packet */
@@ -611,11 +640,13 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     uint8_t    rxbuf[2048];
     uint8_t   *rxbuf_ptr;
     bool first_desc = true;
+    int maf;
 
     s = qemu_get_nic_opaque(nc);
 
     /* Is this destination MAC address "for us" ? */
-    if (gem_mac_address_filter(s, buf) == GEM_RX_REJECT) {
+    maf = gem_mac_address_filter(s, buf);
+    if (maf == GEM_RX_REJECT) {
         return -1;
     }
 
@@ -714,6 +745,25 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
             rx_desc_set_length(s->rx_desc, size);
         }
         rx_desc_set_ownership(s->rx_desc);
+
+        switch (maf) {
+        case GEM_RX_PROM_ACCEPT:
+            break;
+        case GEM_RX_BROADCAST_ACCEPT:
+            rx_desc_set_broadcast(s->rx_desc);
+            break;
+        case GEM_RX_UNICAST_HASH_ACCEPT:
+            rx_desc_set_unicast_hash(s->rx_desc);
+            break;
+        case GEM_RX_MULTICAST_HASH_ACCEPT:
+            rx_desc_set_multicast_hash(s->rx_desc);
+            break;
+        case GEM_RX_REJECT:
+            abort();
+        default: /* SAR */
+            rx_desc_set_sar(s->rx_desc, maf);
+        }
+
         /* Descriptor write-back.  */
         cpu_physical_memory_write(s->rx_desc_addr,
                                   (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 07/13] net/cadence_gem: Implement SAR match bit in rx desc

[Peter Crosthwaite]

Bit 27 of the RX buffer desc word 1 should be set when the packet was
accepted due to specific address register match. Implement.

This feature is absent from the Xilinx documentation (UG585) but the
behaviour is tested as accurate on real hardware.

Reported-by: Deepika Dhamija <deepika@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 4fbfb8d..6f11d6a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -243,6 +243,7 @@
 
 #define R_DESC_1_RX_SAR_SHIFT           25
 #define R_DESC_1_RX_SAR_LENGTH          2
+#define R_DESC_1_RX_SAR_MATCH           (1 << 27)
 #define R_DESC_1_RX_UNICAST_HASH        (1 << 29)
 #define R_DESC_1_RX_MULTICAST_HASH      (1 << 30)
 #define R_DESC_1_RX_BROADCAST           (1 << 31)
@@ -345,6 +346,7 @@ static inline void rx_desc_set_sar(unsigned *desc, int sar_idx)
 {
     desc[1] = deposit32(desc[1], R_DESC_1_RX_SAR_SHIFT, R_DESC_1_RX_SAR_LENGTH,
                         sar_idx);
+    desc[1] |= R_DESC_1_RX_SAR_MATCH;
 }
 
 #define TYPE_CADENCE_GEM "cadence_gem"
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 08/13] net/cadence_gem: Implement SAR (de)activation

[Peter Crosthwaite]

The Specific address registers can be enabled or disabled by software.
QEMU was assuming they where always enabled. Implement the
disable/enable feature. SARs are disabled by writing to the lower half
register. They are re-enabled by then writing the upper half.

Reported-by: Deepika Dhamija <deepika@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 6f11d6a..c6eb9ab 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -382,6 +382,7 @@ typedef struct GemState {
 
     unsigned rx_desc[2];
 
+    bool sar_active[4];
 } GemState;
 
 /* The broadcast MAC address: 0xFFFFFFFFFFFF */
@@ -603,7 +604,7 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
     /* Check all 4 specific addresses */
     gem_spaddr = (uint8_t *)&(s->regs[GEM_SPADDR1LO]);
     for (i = 3; i >= 0; i--) {
-        if (!memcmp(packet, gem_spaddr + 8 * i, 6)) {
+        if (s->sar_active[i] && !memcmp(packet, gem_spaddr + 8 * i, 6)) {
             return GEM_RX_SAR_ACCEPT + i;
         }
     }
@@ -985,6 +986,7 @@ static void gem_phy_reset(GemState *s)
 
 static void gem_reset(DeviceState *d)
 {
+    int i;
     GemState *s = GEM(d);
 
     DB_PRINT("\n");
@@ -1004,6 +1006,10 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_DESCONF5] = 0x002f2145;
     s->regs[GEM_DESCONF6] = 0x00000200;
 
+    for (i = 0; i < 4; ++i) {
+        s->sar_active[i] = false;
+    }
+
     gem_phy_reset(s);
 
     gem_update_int_status(s);
@@ -1153,6 +1159,18 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
         s->regs[GEM_IMR] |= val;
         gem_update_int_status(s);
         break;
+    case GEM_SPADDR1LO:
+    case GEM_SPADDR2LO:
+    case GEM_SPADDR3LO:
+    case GEM_SPADDR4LO:
+        s->sar_active[(offset - GEM_SPADDR1LO) / 2] = false;
+        break;
+    case GEM_SPADDR1HI:
+    case GEM_SPADDR2HI:
+    case GEM_SPADDR3HI:
+    case GEM_SPADDR4HI:
+        s->sar_active[(offset - GEM_SPADDR1HI) / 2] = true;
+        break;
     case GEM_PHYMNTNC:
         if (val & GEM_PHYMNTNC_OP_W) {
             uint32_t phy_addr, reg_num;
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 09/13] net/cadence_gem: Fix rx multi-fragment packets

[Peter Crosthwaite]

Bytes_to_copy was being updated before its final use where it
advances the rx buffer pointer. This was causing total mayhem,
where packet data for any subsequent fragments was being fetched
from the wrong place.

Reported-by: Deepika Dhamija <deepika@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index c6eb9ab..eb0fa95 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -735,8 +735,8 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         /* Copy packet data to emulated DMA buffer */
         cpu_physical_memory_write(rx_desc_get_buffer(s->rx_desc) + rxbuf_offset,
                                   rxbuf_ptr, MIN(bytes_to_copy, rxbufsize));
-        bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
         rxbuf_ptr += MIN(bytes_to_copy, rxbufsize);
+        bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
 
         /* Update the descriptor.  */
         if (first_desc) {
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 10/13] net/cadence_gem: Fix small packet FCS stripping

[Peter Crosthwaite]

The minimum packet size is 64, however this is before FCS stripping
occurs. So when FCS stripping the minimum packet size is 60. Fix.

Reported-by: Deepika Dhamija <deepika@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index eb0fa95..babd39d 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -674,6 +674,14 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     rxbuf_offset = (s->regs[GEM_NWCFG] & GEM_NWCFG_BUFF_OFST_M) >>
                    GEM_NWCFG_BUFF_OFST_S;
 
+    /* Pad to minimum length. Assume FCS field is stripped, logic
+     * below will increment it to the real minimum of 64 when
+     * not FCS stripping
+     */
+    if (size < 60) {
+        size = 60;
+    }
+
     /* The configure size of each receive buffer.  Determines how many
      * buffers needed to hold this packet.
      */
@@ -707,11 +715,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         size += 4;
     }
 
-    /* Pad to minimum length */
-    if (size < 64) {
-        size = 64;
-    }
-
     DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
 
     while (bytes_to_copy) {
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 11/13] net/cadence_gem: Fix register w1c logic

[Peter Crosthwaite]

This write-1-clear logic was incorrect. It was always clearing w1c
bits regardless of whether thie written value was 1 or not. i.e. it
was implementing a write-anything-to-clear strategy.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index babd39d..7f79925 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1114,15 +1114,14 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
 
     /* Squash bits which are read only in write value */
     val &= ~(s->regs_ro[offset]);
-    /* Preserve (only) bits which are read only in register */
-    readonly = s->regs[offset];
-    readonly &= s->regs_ro[offset];
-
-    /* Squash bits which are write 1 to clear */
-    val &= ~(s->regs_w1c[offset] & val);
+    /* Preserve (only) bits which are read only and wtc in register */
+    readonly = s->regs[offset] & (s->regs_ro[offset] | s->regs_w1c[offset]);
 
     /* Copy register write to backing store */
-    s->regs[offset] = val | readonly;
+    s->regs[offset] = (val & ~s->regs_w1c[offset]) | readonly;
+
+    /* do w1c */
+    s->regs[offset] &= ~(s->regs_w1c[offset] & val);
 
     /* Handle register write side effects */
     switch (offset) {
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 12/13] net/cadence_gem: Improve can_receive debug printfery

[Peter Crosthwaite]

Currently this just floods indicating that can_receive has been called
by the net framework. Instead, save the result of the most recent
can_recieve callback as state and only print a message if the result
changes (indicating some sort of actual state change in GEM). Make said
debug message more meaningful as well.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 7f79925..8c2d8fc 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -380,6 +380,8 @@ typedef struct GemState {
     uint32_t rx_desc_addr;
     uint32_t tx_desc_addr;
 
+    uint8_t can_rx_state; /* Debug only */
+
     unsigned rx_desc[2];
 
     bool sar_active[4];
@@ -452,13 +454,19 @@ static int gem_can_receive(NetClientState *nc)
 
     s = qemu_get_nic_opaque(nc);
 
-    DB_PRINT("\n");
-
     /* Do nothing if receive is not enabled. */
     if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_RXENA)) {
+        if (s->can_rx_state != 1) {
+            s->can_rx_state = 1;
+            DB_PRINT("cant receive - noenable\n");
+        }
         return 0;
     }
 
+    if (s->can_rx_state != 0) {
+        s->can_rx_state = 0;
+        DB_PRINT("can receive 0x%x\n", s->rx_desc_addr);
+    }
     return 1;
 }
 
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 13/13] net/cadence_gem: Dont rx packets when no rx buffer available

[Peter Crosthwaite]

Return false from can_recieve() when no valid buffer descriptor is
available. Ensures against mass packet droppage in some applications.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 8c2d8fc..a0499ac 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -463,6 +463,15 @@ static int gem_can_receive(NetClientState *nc)
         return 0;
     }
 
+    if (rx_desc_get_ownership(s->rx_desc) == 1) {
+        if (s->can_rx_state != 2) {
+            s->can_rx_state = 2;
+            DB_PRINT("cant receive - busy buffer descriptor 0x%x\n",
+                     s->rx_desc_addr);
+        }
+        return 0;
+    }
+
     if (s->can_rx_state != 0) {
         s->can_rx_state = 0;
         DB_PRINT("can receive 0x%x\n", s->rx_desc_addr);
@@ -1144,7 +1153,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
             /* Reset to start of Q when transmit disabled. */
             s->tx_desc_addr = s->regs[GEM_TXQBASE];
         }
-        if (val & GEM_NWCTRL_RXENA) {
+        if (gem_can_receive(qemu_get_queue(s->nic))) {
             qemu_flush_queued_packets(qemu_get_queue(s->nic));
         }
         break;
-- 
1.8.4.4

Re: [Qemu-devel] [PATCH arm-devs v1 03/13] net/cadence_gem: Don't assert against 0 buffer address

[Peter Maydell]

On 2 December 2013 07:10, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This has no real hardware analog. Leave the error message in is as
> it is almost certainly a guest error, but fallthrough to the expected
> behaviour.

Maybe we should qemu_log_mask(LOG_GUEST_ERROR, ...)  then?

> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index b84ee60..3f30caf 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -694,7 +694,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          if (rx_desc_get_buffer(desc) == 0) {
>              DB_PRINT("Invalid RX buffer (NULL) for descriptor 0x%x\n",
>                       (unsigned)packet_desc_addr);
> -            break;
>          }
>
>          /* Copy packet data to emulated DMA buffer */
> --
> 1.8.4.4
>

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 01/13] net/cadence_gem: Implement mac level loopback mode

[Peter Maydell]

On 2 December 2013 07:09, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Cadence GEM has a MAC level loopback mode. Implement. Use the same basic
> operation as the already implemented PHY loopback.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 02/13] net/cadence_gem: Update DMA rx descriptors as we process them

[Peter Maydell]

On 2 December 2013 07:09, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> We were updating the ownership bit of all descriptors if packets
> get split and written through several descriptors.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 04/13] net/cadence_gem: simplify rx buf descriptor walking

[Peter Maydell]

On 2 December 2013 07:11, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> There was a replication of the rx descriptor address walking logic.
> Reorder the flow control to remove. This refactoring also obsoletes
> the local variables packet_desc_addr and last_desc_addr.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 39 ++++++++++-----------------------------
>  1 file changed, 10 insertions(+), 29 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3f30caf..73ac0d8 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -586,7 +586,6 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
>  static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>  {
>      unsigned    desc[2];
> -    hwaddr packet_desc_addr, last_desc_addr;
>      GemState *s;
>      unsigned   rxbufsize, bytes_to_copy;
>      unsigned   rxbuf_offset;
> @@ -667,17 +666,16 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>
>      DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
>
> -    packet_desc_addr = s->rx_desc_addr;
> -    while (1) {
> -        DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);
> +    while (bytes_to_copy) {
> +        DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);

Can we get here with bytes_to_copy == 0 ?  The logic sets size to a minimum
of 64 bytes but we've already set up bytes_to_copy before that.

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 05/13] net/cadence_gem: Prefetch rx descriptors ASAP

[Peter Maydell]

On 2 December 2013 07:11, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> The real hardware prefetches rx buffer descriptors ASAP and
> potentially throws relevant interrupts following the fetch
> even in the absence of a recieved packet.
>
> Reported-by: Deepika Dhamija <deepika@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 64 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 36 insertions(+), 28 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 73ac0d8..e5a6d87 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -346,6 +346,8 @@ typedef struct GemState {
>      uint32_t rx_desc_addr;
>      uint32_t tx_desc_addr;
>
> +    unsigned rx_desc[2];
> +
>  } GemState;
>
>  /* The broadcast MAC address: 0xFFFFFFFFFFFF */
> @@ -579,13 +581,30 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
>      return GEM_RX_REJECT;
>  }
>
> +static void gem_get_rx_desc(GemState *s)
> +{
> +        DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
> +        /* read current descriptor */
> +        cpu_physical_memory_read(s->rx_desc_addr,
> +                                 (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
> +
> +        /* Descriptor owned by software ? */
> +        if (rx_desc_get_ownership(s->rx_desc) == 1) {
> +            DB_PRINT("descriptor 0x%x owned by sw.\n",
> +                     (unsigned)s->rx_desc_addr);
> +            s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
> +            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> +            /* Handle interrupt consequences */
> +            gem_update_int_status(s);
> +        }
> +}
> +

Looks OK codewise but your indent here is wrong...

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 06/13] net/cadence_gem: Implement RX descriptor match mode flags

[Peter Maydell]

On 2 December 2013 07:12, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> The various Rx packet address matching mode flags were not being set in
> the rx descriptor. Implement.

> -#define GEM_RX_REJECT  1
> -#define GEM_RX_ACCEPT  0
> +#define GEM_RX_REJECT                   (-1)
> +#define GEM_RX_PROM_ACCEPT              (-2)
> +#define GEM_RX_BROADCAST_ACCEPT         (-3)
> +#define GEM_RX_MULTICAST_HASH_ACCEPT    (-4)
> +#define GEM_RX_UNICAST_HASH_ACCEPT      (-5)
> +
> +#define GEM_RX_SAR_ACCEPT               0

> @@ -527,9 +559,6 @@ static unsigned calc_mac_hash(const uint8_t *mac)
>  /*
>   * gem_mac_address_filter:
>   * Accept or reject this destination address?
> - * Returns:
> - * GEM_RX_REJECT: reject
> - * GEM_RX_ACCEPT: accept
>   */

It would be nice to update this comment about the return value rather
than just deleting it. I think the new semantics are
"Returns a GEM_RX_ constant, or a valid SAR", right
(though I may have mangled the SAR terminology, feel free to
correct/expand).

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 07/13] net/cadence_gem: Implement SAR match bit in rx desc

[Peter Maydell]

On 2 December 2013 07:12, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Bit 27 of the RX buffer desc word 1 should be set when the packet was
> accepted due to specific address register match. Implement.
>
> This feature is absent from the Xilinx documentation (UG585) but the
> behaviour is tested as accurate on real hardware.
>
> Reported-by: Deepika Dhamija <deepika@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 08/13] net/cadence_gem: Implement SAR (de)activation

[Peter Maydell]

On 2 December 2013 07:13, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> The Specific address registers can be enabled or disabled by software.
> QEMU was assuming they where always enabled. Implement the

"were"

> disable/enable feature. SARs are disabled by writing to the lower half
> register. They are re-enabled by then writing the upper half.
>
> Reported-by: Deepika Dhamija <deepika@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 6f11d6a..c6eb9ab 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -382,6 +382,7 @@ typedef struct GemState {
>
>      unsigned rx_desc[2];
>
> +    bool sar_active[4];

This state needs to be added to the VMState struct as well.

>  } GemState;
>
>  /* The broadcast MAC address: 0xFFFFFFFFFFFF */
> @@ -603,7 +604,7 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
>      /* Check all 4 specific addresses */
>      gem_spaddr = (uint8_t *)&(s->regs[GEM_SPADDR1LO]);
>      for (i = 3; i >= 0; i--) {
> -        if (!memcmp(packet, gem_spaddr + 8 * i, 6)) {
> +        if (s->sar_active[i] && !memcmp(packet, gem_spaddr + 8 * i, 6)) {
>              return GEM_RX_SAR_ACCEPT + i;
>          }
>      }
> @@ -985,6 +986,7 @@ static void gem_phy_reset(GemState *s)
>
>  static void gem_reset(DeviceState *d)
>  {
> +    int i;
>      GemState *s = GEM(d);
>
>      DB_PRINT("\n");
> @@ -1004,6 +1006,10 @@ static void gem_reset(DeviceState *d)
>      s->regs[GEM_DESCONF5] = 0x002f2145;
>      s->regs[GEM_DESCONF6] = 0x00000200;
>
> +    for (i = 0; i < 4; ++i) {

"i++" is more idiomatic for C.

Otherwise looks good.

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 09/13] net/cadence_gem: Fix rx multi-fragment packets

[Peter Maydell]

On 2 December 2013 07:14, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Bytes_to_copy was being updated before its final use where it
> advances the rx buffer pointer. This was causing total mayhem,
> where packet data for any subsequent fragments was being fetched
> from the wrong place.
>
> Reported-by: Deepika Dhamija <deepika@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 10/13] net/cadence_gem: Fix small packet FCS stripping

[Peter Maydell]

On 2 December 2013 07:14, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> The minimum packet size is 64, however this is before FCS stripping
> occurs. So when FCS stripping the minimum packet size is 60. Fix.
>
> Reported-by: Deepika Dhamija <deepika@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index eb0fa95..babd39d 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -674,6 +674,14 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      rxbuf_offset = (s->regs[GEM_NWCFG] & GEM_NWCFG_BUFF_OFST_M) >>
>                     GEM_NWCFG_BUFF_OFST_S;
>
> +    /* Pad to minimum length. Assume FCS field is stripped, logic
> +     * below will increment it to the real minimum of 64 when
> +     * not FCS stripping
> +     */
> +    if (size < 60) {
> +        size = 60;
> +    }
> +
>      /* The configure size of each receive buffer.  Determines how many
>       * buffers needed to hold this packet.
>       */
> @@ -707,11 +715,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          size += 4;
>      }
>
> -    /* Pad to minimum length */
> -    if (size < 64) {
> -        size = 64;
> -    }
> -

This change moves the padding of size from below the point where
we initialize bytes_to_copy to above it, so now bytes_to_copy will
get the padded value rather than the unpadded value. If this is deliberate
it should probably be spelled out somewhere. (See also comments on
earlier patch.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 11/13] net/cadence_gem: Fix register w1c logic

[Peter Maydell]

On 2 December 2013 07:15, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This write-1-clear logic was incorrect. It was always clearing w1c
> bits regardless of whether thie written value was 1 or not. i.e. it
> was implementing a write-anything-to-clear strategy.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Looks correct now, though it is not the most translucent piece
of logic I've ever read :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 12/13] net/cadence_gem: Improve can_receive debug printfery

[Peter Maydell]

On 2 December 2013 07:15, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Currently this just floods indicating that can_receive has been called
> by the net framework. Instead, save the result of the most recent
> can_recieve callback as state and only print a message if the result
> changes (indicating some sort of actual state change in GEM). Make said
> debug message more meaningful as well.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 7f79925..8c2d8fc 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -380,6 +380,8 @@ typedef struct GemState {
>      uint32_t rx_desc_addr;
>      uint32_t tx_desc_addr;
>
> +    uint8_t can_rx_state; /* Debug only */
> +
>      unsigned rx_desc[2];
>
>      bool sar_active[4];
> @@ -452,13 +454,19 @@ static int gem_can_receive(NetClientState *nc)
>
>      s = qemu_get_nic_opaque(nc);
>
> -    DB_PRINT("\n");
> -
>      /* Do nothing if receive is not enabled. */
>      if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_RXENA)) {
> +        if (s->can_rx_state != 1) {
> +            s->can_rx_state = 1;
> +            DB_PRINT("cant receive - noenable\n");

"can't".

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 13/13] net/cadence_gem: Dont rx packets when no rx buffer available

[Peter Maydell]

On 2 December 2013 07:16, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:

Missing apostrophe in Subject.

> Return false from can_recieve() when no valid buffer descriptor is

"can_receive"

> available. Ensures against mass packet droppage in some applications.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 8c2d8fc..a0499ac 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -463,6 +463,15 @@ static int gem_can_receive(NetClientState *nc)
>          return 0;
>      }
>
> +    if (rx_desc_get_ownership(s->rx_desc) == 1) {
> +        if (s->can_rx_state != 2) {
> +            s->can_rx_state = 2;
> +            DB_PRINT("cant receive - busy buffer descriptor 0x%x\n",

"can't".

> +                     s->rx_desc_addr);
> +        }
> +        return 0;
> +    }
> +
>      if (s->can_rx_state != 0) {
>          s->can_rx_state = 0;
>          DB_PRINT("can receive 0x%x\n", s->rx_desc_addr);
> @@ -1144,7 +1153,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>              /* Reset to start of Q when transmit disabled. */
>              s->tx_desc_addr = s->regs[GEM_TXQBASE];
>          }
> -        if (val & GEM_NWCTRL_RXENA) {
> +        if (gem_can_receive(qemu_get_queue(s->nic))) {
>              qemu_flush_queued_packets(qemu_get_queue(s->nic));
>          }
>          break;
> --
> 1.8.4.4
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 03/13] net/cadence_gem: Don't assert against 0 buffer address

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:10, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> This has no real hardware analog. Leave the error message in is as
>> it is almost certainly a guest error, but fallthrough to the expected
>> behaviour.
>
> Maybe we should qemu_log_mask(LOG_GUEST_ERROR, ...)  then?
>

I'm starting to think this is not a peripherals assumption to make.
Theres nothing in the GEM design that suggests a 0 RX buffer address
is functionally incorrect. Its not good for Zynq, as theres an ARM
vector table there but thats Zynq's problem not GEMs. Peripherals
DMAing to non-sensical addresses is a SoC/Board level problem and I
think it should be solved there. I'm just going to delete this whole
check.

Regards,
Peter

Re: [Qemu-devel] [PATCH arm-devs v1 04/13] net/cadence_gem: simplify rx buf descriptor walking

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:12 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:11, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> There was a replication of the rx descriptor address walking logic.
>> Reorder the flow control to remove. This refactoring also obsoletes
>> the local variables packet_desc_addr and last_desc_addr.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/net/cadence_gem.c | 39 ++++++++++-----------------------------
>>  1 file changed, 10 insertions(+), 29 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 3f30caf..73ac0d8 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -586,7 +586,6 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
>>  static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>  {
>>      unsigned    desc[2];
>> -    hwaddr packet_desc_addr, last_desc_addr;
>>      GemState *s;
>>      unsigned   rxbufsize, bytes_to_copy;
>>      unsigned   rxbuf_offset;
>> @@ -667,17 +666,16 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>
>>      DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
>>
>> -    packet_desc_addr = s->rx_desc_addr;
>> -    while (1) {
>> -        DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);
>> +    while (bytes_to_copy) {
>> +        DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
>
> Can we get here with bytes_to_copy == 0 ?  The logic sets size to a minimum
> of 64 bytes but we've already set up bytes_to_copy before that.
>

Should we be able to get here? Wouldn't that means you are trying to
recieve a length 0 packet? Could just convert to do loop to receive a
length 0 packet when gem_recieve is called with size = 0.


Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 06/13] net/cadence_gem: Implement RX descriptor match mode flags

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:19 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:12, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> The various Rx packet address matching mode flags were not being set in
>> the rx descriptor. Implement.
>
>> -#define GEM_RX_REJECT  1
>> -#define GEM_RX_ACCEPT  0
>> +#define GEM_RX_REJECT                   (-1)
>> +#define GEM_RX_PROM_ACCEPT              (-2)
>> +#define GEM_RX_BROADCAST_ACCEPT         (-3)
>> +#define GEM_RX_MULTICAST_HASH_ACCEPT    (-4)
>> +#define GEM_RX_UNICAST_HASH_ACCEPT      (-5)
>> +
>> +#define GEM_RX_SAR_ACCEPT               0
>
>> @@ -527,9 +559,6 @@ static unsigned calc_mac_hash(const uint8_t *mac)
>>  /*
>>   * gem_mac_address_filter:
>>   * Accept or reject this destination address?
>> - * Returns:
>> - * GEM_RX_REJECT: reject
>> - * GEM_RX_ACCEPT: accept
>>   */
>
> It would be nice to update this comment about the return value rather
> than just deleting it. I think the new semantics are
> "Returns a GEM_RX_ constant, or a valid SAR", right
> (though I may have mangled the SAR terminology, feel free to
> correct/expand).
>

Done.

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 08/13] net/cadence_gem: Implement SAR (de)activation

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:23 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:13, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> The Specific address registers can be enabled or disabled by software.
>> QEMU was assuming they where always enabled. Implement the
>
> "were"
>

Fixed

>> disable/enable feature. SARs are disabled by writing to the lower half
>> register. They are re-enabled by then writing the upper half.
>>
>> Reported-by: Deepika Dhamija <deepika@xilinx.com>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/net/cadence_gem.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 6f11d6a..c6eb9ab 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -382,6 +382,7 @@ typedef struct GemState {
>>
>>      unsigned rx_desc[2];
>>
>> +    bool sar_active[4];
>
> This state needs to be added to the VMState struct as well.
>

Done.

>>  } GemState;
>>
>>  /* The broadcast MAC address: 0xFFFFFFFFFFFF */
>> @@ -603,7 +604,7 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
>>      /* Check all 4 specific addresses */
>>      gem_spaddr = (uint8_t *)&(s->regs[GEM_SPADDR1LO]);
>>      for (i = 3; i >= 0; i--) {
>> -        if (!memcmp(packet, gem_spaddr + 8 * i, 6)) {
>> +        if (s->sar_active[i] && !memcmp(packet, gem_spaddr + 8 * i, 6)) {
>>              return GEM_RX_SAR_ACCEPT + i;
>>          }
>>      }
>> @@ -985,6 +986,7 @@ static void gem_phy_reset(GemState *s)
>>
>>  static void gem_reset(DeviceState *d)
>>  {
>> +    int i;
>>      GemState *s = GEM(d);
>>
>>      DB_PRINT("\n");
>> @@ -1004,6 +1006,10 @@ static void gem_reset(DeviceState *d)
>>      s->regs[GEM_DESCONF5] = 0x002f2145;
>>      s->regs[GEM_DESCONF6] = 0x00000200;
>>
>> +    for (i = 0; i < 4; ++i) {
>
> "i++" is more idiomatic for C.
>
> Otherwise looks good.
>

Fixed, although I do see both used quite liberally. I'll use foo++
when local coding style doesn't contradict.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 05/13] net/cadence_gem: Prefetch rx descriptors ASAP

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:11, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> The real hardware prefetches rx buffer descriptors ASAP and
>> potentially throws relevant interrupts following the fetch
>> even in the absence of a recieved packet.
>>
>> Reported-by: Deepika Dhamija <deepika@xilinx.com>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/net/cadence_gem.c | 64 +++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 36 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 73ac0d8..e5a6d87 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -346,6 +346,8 @@ typedef struct GemState {
>>      uint32_t rx_desc_addr;
>>      uint32_t tx_desc_addr;
>>
>> +    unsigned rx_desc[2];
>> +
>>  } GemState;
>>
>>  /* The broadcast MAC address: 0xFFFFFFFFFFFF */
>> @@ -579,13 +581,30 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
>>      return GEM_RX_REJECT;
>>  }
>>
>> +static void gem_get_rx_desc(GemState *s)
>> +{
>> +        DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
>> +        /* read current descriptor */
>> +        cpu_physical_memory_read(s->rx_desc_addr,
>> +                                 (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
>> +
>> +        /* Descriptor owned by software ? */
>> +        if (rx_desc_get_ownership(s->rx_desc) == 1) {
>> +            DB_PRINT("descriptor 0x%x owned by sw.\n",
>> +                     (unsigned)s->rx_desc_addr);
>> +            s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
>> +            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
>> +            /* Handle interrupt consequences */
>> +            gem_update_int_status(s);
>> +        }
>> +}
>> +
>
> Looks OK codewise but your indent here is wrong...
>

Fixed,

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 12/13] net/cadence_gem: Improve can_receive debug printfery

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:30 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:15, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Currently this just floods indicating that can_receive has been called
>> by the net framework. Instead, save the result of the most recent
>> can_recieve callback as state and only print a message if the result
>> changes (indicating some sort of actual state change in GEM). Make said
>> debug message more meaningful as well.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/net/cadence_gem.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 7f79925..8c2d8fc 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -380,6 +380,8 @@ typedef struct GemState {
>>      uint32_t rx_desc_addr;
>>      uint32_t tx_desc_addr;
>>
>> +    uint8_t can_rx_state; /* Debug only */
>> +
>>      unsigned rx_desc[2];
>>
>>      bool sar_active[4];
>> @@ -452,13 +454,19 @@ static int gem_can_receive(NetClientState *nc)
>>
>>      s = qemu_get_nic_opaque(nc);
>>
>> -    DB_PRINT("\n");
>> -
>>      /* Do nothing if receive is not enabled. */
>>      if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_RXENA)) {
>> +        if (s->can_rx_state != 1) {
>> +            s->can_rx_state = 1;
>> +            DB_PRINT("cant receive - noenable\n");
>
> "can't".
>

Fixed,

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 13/13] net/cadence_gem: Dont rx packets when no rx buffer available

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:16, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>
> Missing apostrophe in Subject.
>
>> Return false from can_recieve() when no valid buffer descriptor is
>
> "can_receive"
>
>> available. Ensures against mass packet droppage in some applications.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/net/cadence_gem.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 8c2d8fc..a0499ac 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -463,6 +463,15 @@ static int gem_can_receive(NetClientState *nc)
>>          return 0;
>>      }
>>
>> +    if (rx_desc_get_ownership(s->rx_desc) == 1) {
>> +        if (s->can_rx_state != 2) {
>> +            s->can_rx_state = 2;
>> +            DB_PRINT("cant receive - busy buffer descriptor 0x%x\n",
>
> "can't".
>

All fixed,

Regards,
Peter

>> +                     s->rx_desc_addr);
>> +        }
>> +        return 0;
>> +    }
>> +
>>      if (s->can_rx_state != 0) {
>>          s->can_rx_state = 0;
>>          DB_PRINT("can receive 0x%x\n", s->rx_desc_addr);
>> @@ -1144,7 +1153,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>>              /* Reset to start of Q when transmit disabled. */
>>              s->tx_desc_addr = s->regs[GEM_TXQBASE];
>>          }
>> -        if (val & GEM_NWCTRL_RXENA) {
>> +        if (gem_can_receive(qemu_get_queue(s->nic))) {
>>              qemu_flush_queued_packets(qemu_get_queue(s->nic));
>>          }
>>          break;
>> --
>> 1.8.4.4
>>
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 10/13] net/cadence_gem: Fix small packet FCS stripping

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:26 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 December 2013 07:14, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> The minimum packet size is 64, however this is before FCS stripping
>> occurs. So when FCS stripping the minimum packet size is 60. Fix.
>>
>> Reported-by: Deepika Dhamija <deepika@xilinx.com>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/net/cadence_gem.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index eb0fa95..babd39d 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -674,6 +674,14 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>      rxbuf_offset = (s->regs[GEM_NWCFG] & GEM_NWCFG_BUFF_OFST_M) >>
>>                     GEM_NWCFG_BUFF_OFST_S;
>>
>> +    /* Pad to minimum length. Assume FCS field is stripped, logic
>> +     * below will increment it to the real minimum of 64 when
>> +     * not FCS stripping
>> +     */
>> +    if (size < 60) {
>> +        size = 60;
>> +    }
>> +
>>      /* The configure size of each receive buffer.  Determines how many
>>       * buffers needed to hold this packet.
>>       */
>> @@ -707,11 +715,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>          size += 4;
>>      }
>>
>> -    /* Pad to minimum length */
>> -    if (size < 64) {
>> -        size = 64;
>> -    }
>> -
>
> This change moves the padding of size from below the point where
> we initialize bytes_to_copy to above it, so now bytes_to_copy will
> get the padded value rather than the unpadded value. If this is deliberate
> it should probably be spelled out somewhere. (See also comments on
> earlier patch.)
>

So I cant see a good reason for that change. Reverted - just moved the
added hunk to below the bytes_to_copy =. Stress tests and linux tests
still pass.

Regards,
Peter

> thanks
> -- PMM
>