[PATCH v3 00/11] Cadence GEM Fixes

[PATCH v3 00/11] Cadence GEM Fixes

[Sai Pavan Boddu]

Hi,

Following patch series fixes issues with priority queues,
Adds JUMBO Frame support,
Makes Debug statements compilable &
Fixes related to multicast frames.

Changes for V2:
	Fixed build failure on fedora docker machine
	Fix buggy debug print to use sized integer casting
Changes for V3:
	1/11: Fixed debug statments to use %u and %zd
	      Remove rxoffset for buffer address
	2/11: Add inline functions to get tx/rx queue base address.
	4/11: fix read only mask
	5/11: Move packet buffers to CadenceGEMState
	6/11: Add JUMBO MAX LEN register

Sai Pavan Boddu (10):
  net: cadence_gem: Fix debug statements
  net: cadence_gem: Fix the queue address update during wrap around
  net: cadence_gem: Fix irq update w.r.t queue
  net: cadence_gem: Define access permission for interrupt registers
  net: cadence_gem: Set ISR according to queue in use
  net: cadence_gem: Move tx/rx packet buffert to CadenceGEMState
  net: cadence_gem: Add support for jumbo frames
  net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg
  net: cadence_gem: Update the reset value for interrupt mask register
  net: cadence_gem: TX_LAST bit should be set by guest

Tong Ho (1):
  net: cadence_gem: Fix RX address filtering

 hw/net/cadence_gem.c         | 236 ++++++++++++++++++++++++++-----------------
 include/hw/net/cadence_gem.h |   3 +
 2 files changed, 145 insertions(+), 94 deletions(-)

-- 
2.7.4

[PATCH v3 01/11] net: cadence_gem: Fix debug statements

[Sai Pavan Boddu]

Enabling debug breaks the build, Fix them and make debug statements
always compilable. Fix few statements to use sized integer casting.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 22a0b1b..5476c62 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -35,14 +35,13 @@
 #include "sysemu/dma.h"
 #include "net/checksum.h"
 
-#ifdef CADENCE_GEM_ERR_DEBUG
-#define DB_PRINT(...) do { \
-    fprintf(stderr,  ": %s: ", __func__); \
-    fprintf(stderr, ## __VA_ARGS__); \
-    } while (0)
-#else
-    #define DB_PRINT(...)
-#endif
+#define CADENCE_GEM_ERR_DEBUG 0
+#define DB_PRINT(...) do {\
+    if (CADENCE_GEM_ERR_DEBUG) {   \
+        qemu_log(": %s: ", __func__); \
+        qemu_log(__VA_ARGS__); \
+    } \
+} while (0)
 
 #define GEM_NWCTRL        (0x00000000/4) /* Network Control reg */
 #define GEM_NWCFG         (0x00000004/4) /* Network Config reg */
@@ -979,7 +978,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         size += 4;
     }
 
-    DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
+    DB_PRINT("config bufsize: %u packet size: %zd\n", rxbufsize, size);
 
     /* Find which queue we are targeting */
     q = get_queue_from_screen(s, rxbuf_ptr, rxbufsize);
@@ -992,9 +991,9 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
             return -1;
         }
 
-        DB_PRINT("copy %u bytes to 0x%" PRIx64 "\n",
-                 MIN(bytes_to_copy, rxbufsize),
-                 rx_desc_get_buffer(s, s->rx_desc[q]));
+        DB_PRINT("copy %" PRIu32 " bytes to 0x%" PRIx64 "\n",
+                MIN(bytes_to_copy, rxbufsize),
+                rx_desc_get_buffer(s, s->rx_desc[q]));
 
         /* Copy packet data to emulated DMA buffer */
         address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) +
@@ -1160,8 +1159,8 @@ static void gem_transmit(CadenceGEMState *s)
              */
             if ((tx_desc_get_buffer(s, desc) == 0) ||
                 (tx_desc_get_length(desc) == 0)) {
-                DB_PRINT("Invalid TX descriptor @ 0x%x\n",
-                         (unsigned)packet_desc_addr);
+                DB_PRINT("Invalid TX descriptor @ 0x%" HWADDR_PRIx "\n",
+                         packet_desc_addr);
                 break;
             }
 
-- 
2.7.4

[PATCH v3 02/11] net: cadence_gem: Fix the queue address update during wrap around

[Sai Pavan Boddu]

During wrap around and reset, queues are pointing to initial base
address of queue 0, irrespective of what queue we are dealing with.
Fix it by assigning proper base address every time.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 5476c62..e6dc436 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -845,6 +845,35 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
     return 0;
 }
 
+static uint32_t gem_get_queue_base_addr(CadenceGEMState *s, bool tx, int q)
+{
+    uint32_t base_addr = 0;
+
+    switch (q) {
+    case 0:
+        base_addr = s->regs[tx ? GEM_TXQBASE : GEM_RXQBASE];
+        break;
+    case 1 ... (MAX_PRIORITY_QUEUES - 1):
+        base_addr = s->regs[(tx ? GEM_TRANSMIT_Q1_PTR :
+                                 GEM_RECEIVE_Q1_PTR) + q - 1];
+        break;
+    default:
+        g_assert_not_reached();
+    };
+
+    return base_addr;
+}
+
+static uint32_t gem_get_tx_queue_base_addr(CadenceGEMState *s, int q)
+{
+    return gem_get_queue_base_addr(s, true, q);
+}
+
+static uint32_t gem_get_rx_queue_base_addr(CadenceGEMState *s, int q)
+{
+    return gem_get_queue_base_addr(s, false, q);
+}
+
 static hwaddr gem_get_desc_addr(CadenceGEMState *s, bool tx, int q)
 {
     hwaddr desc_addr = 0;
@@ -1043,7 +1072,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         /* Next descriptor */
         if (rx_desc_get_wrap(s->rx_desc[q])) {
             DB_PRINT("wrapping RX descriptor list\n");
-            s->rx_desc_addr[q] = s->regs[GEM_RXQBASE];
+            s->rx_desc_addr[q] = gem_get_rx_queue_base_addr(s, q);
         } else {
             DB_PRINT("incrementing RX descriptor list\n");
             s->rx_desc_addr[q] += 4 * gem_get_desc_len(s, true);
@@ -1199,7 +1228,7 @@ static void gem_transmit(CadenceGEMState *s)
                                     sizeof(desc_first));
                 /* Advance the hardware current descriptor past this packet */
                 if (tx_desc_get_wrap(desc)) {
-                    s->tx_desc_addr[q] = s->regs[GEM_TXQBASE];
+                    s->tx_desc_addr[q] = gem_get_tx_queue_base_addr(s, q);
                 } else {
                     s->tx_desc_addr[q] = packet_desc_addr +
                                          4 * gem_get_desc_len(s, false);
@@ -1251,7 +1280,7 @@ static void gem_transmit(CadenceGEMState *s)
                 } else {
                     packet_desc_addr = 0;
                 }
-                packet_desc_addr |= s->regs[GEM_TXQBASE];
+                packet_desc_addr |= gem_get_tx_queue_base_addr(s, q);
             } else {
                 packet_desc_addr += 4 * gem_get_desc_len(s, false);
             }
@@ -1457,7 +1486,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
         if (!(val & GEM_NWCTRL_TXENA)) {
             /* Reset to start of Q when transmit disabled. */
             for (i = 0; i < s->num_priority_queues; i++) {
-                s->tx_desc_addr[i] = s->regs[GEM_TXQBASE];
+                s->tx_desc_addr[i] = gem_get_tx_queue_base_addr(s, i);
             }
         }
         if (gem_can_receive(qemu_get_queue(s->nic))) {
-- 
2.7.4

[PATCH v3 03/11] net: cadence_gem: Fix irq update w.r.t queue

[Sai Pavan Boddu]

Set irq's specific to a queue, present implementation is setting q1 irq
based on q0 status.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index e6dc436..fefb360 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -554,29 +554,10 @@ static void gem_update_int_status(CadenceGEMState *s)
 {
     int i;
 
-    if (!s->regs[GEM_ISR]) {
-        /* ISR isn't set, clear all the interrupts */
-        for (i = 0; i < s->num_priority_queues; ++i) {
-            qemu_set_irq(s->irq[i], 0);
-        }
-        return;
-    }
+    qemu_set_irq(s->irq[0], !!s->regs[GEM_ISR]);
 
-    /* If we get here we know s->regs[GEM_ISR] is set, so we don't need to
-     * check it again.
-     */
-    if (s->num_priority_queues == 1) {
-        /* No priority queues, just trigger the interrupt */
-        DB_PRINT("asserting int.\n");
-        qemu_set_irq(s->irq[0], 1);
-        return;
-    }
-
-    for (i = 0; i < s->num_priority_queues; ++i) {
-        if (s->regs[GEM_INT_Q1_STATUS + i]) {
-            DB_PRINT("asserting int. (q=%d)\n", i);
-            qemu_set_irq(s->irq[i], 1);
-        }
+    for (i = 1; i < s->num_priority_queues; ++i) {
+        qemu_set_irq(s->irq[i], !!s->regs[GEM_INT_Q1_STATUS + i - 1]);
     }
 }
 
-- 
2.7.4

[PATCH v3 04/11] net: cadence_gem: Define access permission for interrupt registers

[Sai Pavan Boddu]

Q1 to Q7 ISR's are clear-on-read, IER/IDR registers
are write-only, mask reg are read-only.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index fefb360..74ef447 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -458,6 +458,7 @@ static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
  */
 static void gem_init_register_masks(CadenceGEMState *s)
 {
+    unsigned int i;
     /* Mask of register bits which are read only */
     memset(&s->regs_ro[0], 0, sizeof(s->regs_ro));
     s->regs_ro[GEM_NWCTRL]   = 0xFFF80000;
@@ -470,10 +471,19 @@ static void gem_init_register_masks(CadenceGEMState *s)
     s->regs_ro[GEM_ISR]      = 0xFFFFFFFF;
     s->regs_ro[GEM_IMR]      = 0xFFFFFFFF;
     s->regs_ro[GEM_MODID]    = 0xFFFFFFFF;
+    for (i = 0; i < s->num_priority_queues; i++) {
+        s->regs_ro[GEM_INT_Q1_STATUS + i] = 0xFFFFFFFF;
+        s->regs_ro[GEM_INT_Q1_ENABLE + i] = 0xFFFFF319;
+        s->regs_ro[GEM_INT_Q1_DISABLE + i] = 0xFFFFF319;
+        s->regs_ro[GEM_INT_Q1_MASK + i] = 0xFFFFFFFF;
+    }
 
     /* Mask of register bits which are clear on read */
     memset(&s->regs_rtc[0], 0, sizeof(s->regs_rtc));
     s->regs_rtc[GEM_ISR]      = 0xFFFFFFFF;
+    for (i = 0; i < s->num_priority_queues; i++) {
+        s->regs_rtc[GEM_INT_Q1_STATUS + i] = 0x00000CE6;
+    }
 
     /* Mask of register bits which are write 1 to clear */
     memset(&s->regs_w1c[0], 0, sizeof(s->regs_w1c));
@@ -485,6 +495,10 @@ static void gem_init_register_masks(CadenceGEMState *s)
     s->regs_wo[GEM_NWCTRL]   = 0x00073E60;
     s->regs_wo[GEM_IER]      = 0x07FFFFFF;
     s->regs_wo[GEM_IDR]      = 0x07FFFFFF;
+    for (i = 0; i < s->num_priority_queues; i++) {
+        s->regs_wo[GEM_INT_Q1_ENABLE + i] = 0x00000CE6;
+        s->regs_wo[GEM_INT_Q1_DISABLE + i] = 0x00000CE6;
+    }
 }
 
 /*
-- 
2.7.4

[PATCH v3 05/11] net: cadence_gem: Set ISR according to queue in use

[Sai Pavan Boddu]

Set ISR according to queue in use, added interrupt support for
all queues.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 74ef447..77a0588 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -451,6 +451,16 @@ static inline void rx_desc_set_sar(uint32_t *desc, int sar_idx)
 /* The broadcast MAC address: 0xFFFFFFFFFFFF */
 static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
 
+static void gem_set_isr(CadenceGEMState *s, int q, uint32_t flag)
+{
+    if (q == 0) {
+        s->regs[GEM_ISR] |= flag & ~(s->regs[GEM_IMR]);
+    } else {
+        s->regs[GEM_INT_Q1_STATUS + q - 1] |= flag &
+                                      ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
+    }
+}
+
 /*
  * gem_init_register_masks:
  * One time initialization.
@@ -906,7 +916,7 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
     if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
         DB_PRINT("descriptor 0x%" HWADDR_PRIx " owned by sw.\n", desc_addr);
         s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
-        s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
+        gem_set_isr(s, q, GEM_INT_RXUSED);
         /* Handle interrupt consequences */
         gem_update_int_status(s);
     }
@@ -1080,7 +1090,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     gem_receive_updatestats(s, buf, size);
 
     s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_FRMRCVD;
-    s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
+    gem_set_isr(s, q, GEM_INT_RXCMPL);
 
     /* Handle interrupt consequences */
     gem_update_int_status(s);
@@ -1231,13 +1241,7 @@ static void gem_transmit(CadenceGEMState *s)
                 DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[q]);
 
                 s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
-                s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
-
-                /* Update queue interrupt status */
-                if (s->num_priority_queues > 1) {
-                    s->regs[GEM_INT_Q1_STATUS + q] |=
-                            GEM_INT_TXCMPL & ~(s->regs[GEM_INT_Q1_MASK + q]);
-                }
+                gem_set_isr(s, q, GEM_INT_TXCMPL);
 
                 /* Handle interrupt consequences */
                 gem_update_int_status(s);
@@ -1287,7 +1291,10 @@ static void gem_transmit(CadenceGEMState *s)
 
         if (tx_desc_get_used(desc)) {
             s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_USED;
-            s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
+            /* IRQ TXUSED is defined only for queue 0 */
+            if (q == 0) {
+                gem_set_isr(s, 0, GEM_INT_TXUSED);
+            }
             gem_update_int_status(s);
         }
     }
-- 
2.7.4

[PATCH v3 06/11] net: cadence_gem: Move tx/rx packet buffert to CadenceGEMState

[Sai Pavan Boddu]

Moving this buffers to CadenceGEMState, as their size will be increased
more when JUMBO frames support is added.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c         | 52 ++++++++++++++++++++++++++------------------
 include/hw/net/cadence_gem.h |  2 ++
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 77a0588..5ccec1a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -314,6 +314,8 @@
 
 #define GEM_MODID_VALUE 0x00020118
 
+#define MAX_FRAME_SIZE 2048
+
 static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
 {
     uint64_t ret = desc[0];
@@ -928,17 +930,14 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
  */
 static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
-    CadenceGEMState *s;
+    CadenceGEMState *s = qemu_get_nic_opaque(nc);
     unsigned   rxbufsize, bytes_to_copy;
     unsigned   rxbuf_offset;
-    uint8_t    rxbuf[2048];
     uint8_t   *rxbuf_ptr;
     bool first_desc = true;
     int maf;
     int q = 0;
 
-    s = qemu_get_nic_opaque(nc);
-
     /* Is this destination MAC address "for us" ? */
     maf = gem_mac_address_filter(s, buf);
     if (maf == GEM_RX_REJECT) {
@@ -994,19 +993,19 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     } else {
         unsigned crc_val;
 
-        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
-            size = sizeof(rxbuf) - sizeof(crc_val);
+        if (size > MAX_FRAME_SIZE - sizeof(crc_val)) {
+            size = MAX_FRAME_SIZE - sizeof(crc_val);
         }
         bytes_to_copy = size;
         /* The application wants the FCS field, which QEMU does not provide.
          * We must try and calculate one.
          */
 
-        memcpy(rxbuf, buf, size);
-        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
-        rxbuf_ptr = rxbuf;
-        crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
-        memcpy(rxbuf + size, &crc_val, sizeof(crc_val));
+        memcpy(s->rx_packet, buf, size);
+        memset(s->rx_packet + size, 0, MAX_FRAME_SIZE - size);
+        rxbuf_ptr = s->rx_packet;
+        crc_val = cpu_to_le32(crc32(0, s->rx_packet, MAX(size, 60)));
+        memcpy(s->rx_packet + size, &crc_val, sizeof(crc_val));
 
         bytes_to_copy += 4;
         size += 4;
@@ -1152,7 +1151,6 @@ static void gem_transmit(CadenceGEMState *s)
 {
     uint32_t desc[DESC_MAX_NUM_WORDS];
     hwaddr packet_desc_addr;
-    uint8_t     tx_packet[2048];
     uint8_t     *p;
     unsigned    total_bytes;
     int q = 0;
@@ -1168,7 +1166,7 @@ static void gem_transmit(CadenceGEMState *s)
      * Packets scattered across multiple descriptors are gathered to this
      * one contiguous buffer first.
      */
-    p = tx_packet;
+    p = s->tx_packet;
     total_bytes = 0;
 
     for (q = s->num_priority_queues - 1; q >= 0; q--) {
@@ -1198,12 +1196,12 @@ static void gem_transmit(CadenceGEMState *s)
                 break;
             }
 
-            if (tx_desc_get_length(desc) > sizeof(tx_packet) -
-                                               (p - tx_packet)) {
+            if (tx_desc_get_length(desc) > MAX_FRAME_SIZE -
+                                               (p - s->tx_packet)) {
                 DB_PRINT("TX descriptor @ 0x%" HWADDR_PRIx \
                          " too large: size 0x%x space 0x%zx\n",
                          packet_desc_addr, tx_desc_get_length(desc),
-                         sizeof(tx_packet) - (p - tx_packet));
+                         MAX_FRAME_SIZE - (p - s->tx_packet));
                 break;
             }
 
@@ -1248,24 +1246,24 @@ static void gem_transmit(CadenceGEMState *s)
 
                 /* Is checksum offload enabled? */
                 if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
-                    net_checksum_calculate(tx_packet, total_bytes);
+                    net_checksum_calculate(s->tx_packet, total_bytes);
                 }
 
                 /* Update MAC statistics */
-                gem_transmit_updatestats(s, tx_packet, total_bytes);
+                gem_transmit_updatestats(s, s->tx_packet, total_bytes);
 
                 /* Send the packet somewhere */
                 if (s->phy_loop || (s->regs[GEM_NWCTRL] &
                                     GEM_NWCTRL_LOCALLOOP)) {
-                    gem_receive(qemu_get_queue(s->nic), tx_packet,
+                    gem_receive(qemu_get_queue(s->nic), s->tx_packet,
                                 total_bytes);
                 } else {
-                    qemu_send_packet(qemu_get_queue(s->nic), tx_packet,
+                    qemu_send_packet(qemu_get_queue(s->nic), s->tx_packet,
                                      total_bytes);
                 }
 
                 /* Prepare for next packet */
-                p = tx_packet;
+                p = s->tx_packet;
                 total_bytes = 0;
             }
 
@@ -1612,6 +1610,17 @@ static void gem_realize(DeviceState *dev, Error **errp)
 
     s->nic = qemu_new_nic(&net_gem_info, &s->conf,
                           object_get_typename(OBJECT(dev)), dev->id, s);
+
+    s->tx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
+    s->rx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
+}
+
+static void gem_unrealize(DeviceState *dev, Error **errp)
+{
+    CadenceGEMState *s = CADENCE_GEM(dev);
+
+    g_free(s->tx_packet);
+    g_free(s->rx_packet);
 }
 
 static void gem_init(Object *obj)
@@ -1669,6 +1678,7 @@ static void gem_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = gem_realize;
+    dc->unrealize = gem_unrealize;
     device_class_set_props(dc, gem_properties);
     dc->vmsd = &vmstate_cadence_gem;
     dc->reset = gem_reset;
diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
index 5c83036..8dbbaa3 100644
--- a/include/hw/net/cadence_gem.h
+++ b/include/hw/net/cadence_gem.h
@@ -80,6 +80,8 @@ typedef struct CadenceGEMState {
 
     uint8_t can_rx_state; /* Debug only */
 
+    uint8_t *tx_packet;
+    uint8_t *rx_packet;
     uint32_t rx_desc[MAX_PRIORITY_QUEUES][DESC_MAX_NUM_WORDS];
 
     bool sar_active[4];
-- 
2.7.4

[PATCH v3 07/11] net: cadence_gem: Add support for jumbo frames

[Sai Pavan Boddu]

Add a property "jumbo-max-len", which can be configured for jumbo frame size
up to 16,383 bytes, and also introduce new register GEM_JUMBO_MAX_LEN.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c         | 21 +++++++++++++++++++--
 include/hw/net/cadence_gem.h |  1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 5ccec1a..45c50ab 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -61,6 +61,7 @@
 #define GEM_TXPAUSE       (0x0000003C/4) /* TX Pause Time reg */
 #define GEM_TXPARTIALSF   (0x00000040/4) /* TX Partial Store and Forward */
 #define GEM_RXPARTIALSF   (0x00000044/4) /* RX Partial Store and Forward */
+#define GEM_JUMBO_MAX_LEN (0x00000048 / 4) /* Max Jumbo Frame Size */
 #define GEM_HASHLO        (0x00000080/4) /* Hash Low address reg */
 #define GEM_HASHHI        (0x00000084/4) /* Hash High address reg */
 #define GEM_SPADDR1LO     (0x00000088/4) /* Specific addr 1 low reg */
@@ -314,7 +315,8 @@
 
 #define GEM_MODID_VALUE 0x00020118
 
-#define MAX_FRAME_SIZE 2048
+#define MAX_JUMBO_FRAME_SIZE_MASK 0x3FFF
+#define MAX_FRAME_SIZE MAX_JUMBO_FRAME_SIZE_MASK
 
 static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
 {
@@ -1343,9 +1345,10 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_RXPARTIALSF] = 0x000003ff;
     s->regs[GEM_MODID] = s->revision;
     s->regs[GEM_DESCONF] = 0x02500111;
-    s->regs[GEM_DESCONF2] = 0x2ab13fff;
+    s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len;
     s->regs[GEM_DESCONF5] = 0x002f2045;
     s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
+    s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len;
 
     if (s->num_priority_queues > 1) {
         queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
@@ -1420,6 +1423,9 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
         DB_PRINT("lowering irqs on ISR read\n");
         /* The interrupts get updated at the end of the function. */
         break;
+    case GEM_JUMBO_MAX_LEN:
+        retval = s->jumbo_max_len;
+        break;
     case GEM_PHYMNTNC:
         if (retval & GEM_PHYMNTNC_OP_R) {
             uint32_t phy_addr, reg_num;
@@ -1516,6 +1522,9 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
         s->regs[GEM_IMR] &= ~val;
         gem_update_int_status(s);
         break;
+    case GEM_JUMBO_MAX_LEN:
+        s->jumbo_max_len = val & MAX_JUMBO_FRAME_SIZE_MASK;
+        break;
     case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE:
         s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val;
         gem_update_int_status(s);
@@ -1611,6 +1620,12 @@ static void gem_realize(DeviceState *dev, Error **errp)
     s->nic = qemu_new_nic(&net_gem_info, &s->conf,
                           object_get_typename(OBJECT(dev)), dev->id, s);
 
+    if (s->jumbo_max_len > MAX_FRAME_SIZE) {
+        g_warning("jumbo-max-len is grater than %d",
+                  MAX_FRAME_SIZE);
+        s->jumbo_max_len = MAX_FRAME_SIZE;
+    }
+
     s->tx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
     s->rx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
 }
@@ -1670,6 +1685,8 @@ static Property gem_properties[] = {
                       num_type1_screeners, 4),
     DEFINE_PROP_UINT8("num-type2-screeners", CadenceGEMState,
                       num_type2_screeners, 4),
+    DEFINE_PROP_UINT16("jumbo-max-len", CadenceGEMState,
+                       jumbo_max_len, 10240),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
index 8dbbaa3..ef85737 100644
--- a/include/hw/net/cadence_gem.h
+++ b/include/hw/net/cadence_gem.h
@@ -82,6 +82,7 @@ typedef struct CadenceGEMState {
 
     uint8_t *tx_packet;
     uint8_t *rx_packet;
+    uint16_t jumbo_max_len;
     uint32_t rx_desc[MAX_PRIORITY_QUEUES][DESC_MAX_NUM_WORDS];
 
     bool sar_active[4];
-- 
2.7.4

[PATCH v3 08/11] net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg

[Sai Pavan Boddu]

Advertise support of clear-on-read for ISR registers.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@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 45c50ab..65b29cc 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1344,7 +1344,7 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_TXPARTIALSF] = 0x000003ff;
     s->regs[GEM_RXPARTIALSF] = 0x000003ff;
     s->regs[GEM_MODID] = s->revision;
-    s->regs[GEM_DESCONF] = 0x02500111;
+    s->regs[GEM_DESCONF] = 0x02D00111;
     s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len;
     s->regs[GEM_DESCONF5] = 0x002f2045;
     s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
-- 
2.7.4

[PATCH v3 09/11] net: cadence_gem: Update the reset value for interrupt mask register

[Sai Pavan Boddu]

Mask all interrupt on reset.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 65b29cc..45c7390 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1348,6 +1348,7 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len;
     s->regs[GEM_DESCONF5] = 0x002f2045;
     s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
+    s->regs[GEM_INT_Q1_MASK] = 0x00000CE6;
     s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len;
 
     if (s->num_priority_queues > 1) {
-- 
2.7.4

[PATCH v3 10/11] net: cadence_gem: TX_LAST bit should be set by guest

[Sai Pavan Boddu]

TX_LAST bit should not be set by hardware, its set by guest to inform
the last bd of the frame.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 45c7390..f8cea63 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -348,11 +348,6 @@ static inline unsigned tx_desc_get_last(uint32_t *desc)
     return (desc[1] & DESC_1_TX_LAST) ? 1 : 0;
 }
 
-static inline void tx_desc_set_last(uint32_t *desc)
-{
-    desc[1] |= DESC_1_TX_LAST;
-}
-
 static inline unsigned tx_desc_get_length(uint32_t *desc)
 {
     return desc[1] & DESC_1_LENGTH;
@@ -1271,7 +1266,6 @@ static void gem_transmit(CadenceGEMState *s)
 
             /* read next descriptor */
             if (tx_desc_get_wrap(desc)) {
-                tx_desc_set_last(desc);
 
                 if (s->regs[GEM_DMACFG] & GEM_DMACFG_ADDR_64B) {
                     packet_desc_addr = s->regs[GEM_TBQPH];
-- 
2.7.4

[PATCH v3 11/11] net: cadence_gem: Fix RX address filtering

[Sai Pavan Boddu]

From: Tong Ho <tong.ho@xilinx.com>

Two defects are fixed:

1/ Detection of multicast frames
2/ Treating drop of mis-addressed frames as non-error

Signed-off-by: Tong Ho <tong.ho@xilinx.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index f8cea63..ccd087d 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -34,6 +34,7 @@
 #include "qemu/module.h"
 #include "sysemu/dma.h"
 #include "net/checksum.h"
+#include "net/eth.h"
 
 #define CADENCE_GEM_ERR_DEBUG 0
 #define DB_PRINT(...) do {\
@@ -682,7 +683,7 @@ static unsigned calc_mac_hash(const uint8_t *mac)
 static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
 {
     uint8_t *gem_spaddr;
-    int i;
+    int i, is_mc;
 
     /* Promiscuous mode? */
     if (s->regs[GEM_NWCFG] & GEM_NWCFG_PROMISC) {
@@ -698,22 +699,17 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
     }
 
     /* Accept packets -w- hash match? */
-    if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
-        (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
+    is_mc = is_multicast_ether_addr(packet);
+    if ((is_mc && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
+        (!is_mc && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
+        uint64_t buckets;
         unsigned hash_index;
 
         hash_index = calc_mac_hash(packet);
-        if (hash_index < 32) {
-            if (s->regs[GEM_HASHLO] & (1<<hash_index)) {
-                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 packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
-                                           GEM_RX_UNICAST_HASH_ACCEPT;
-            }
+        buckets = ((uint64_t)s->regs[GEM_HASHHI] << 32) | s->regs[GEM_HASHLO];
+        if ((buckets >> hash_index) & 1) {
+            return is_mc ? GEM_RX_MULTICAST_HASH_ACCEPT
+                         : GEM_RX_UNICAST_HASH_ACCEPT;
         }
     }
 
@@ -938,7 +934,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     /* Is this destination MAC address "for us" ? */
     maf = gem_mac_address_filter(s, buf);
     if (maf == GEM_RX_REJECT) {
-        return -1;
+        return size;  /* no, drop siliently b/c it's not an error */
     }
 
     /* Discard packets with receive length error enabled ? */
-- 
2.7.4

Re: [PATCH v3 01/11] net: cadence_gem: Fix debug statements

[Edgar E. Iglesias]

On Fri, May 08, 2020 at 04:30:35PM +0530, Sai Pavan Boddu wrote:
> Enabling debug breaks the build, Fix them and make debug statements
> always compilable. Fix few statements to use sized integer casting.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 22a0b1b..5476c62 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -35,14 +35,13 @@
>  #include "sysemu/dma.h"
>  #include "net/checksum.h"
>  
> -#ifdef CADENCE_GEM_ERR_DEBUG
> -#define DB_PRINT(...) do { \
> -    fprintf(stderr,  ": %s: ", __func__); \
> -    fprintf(stderr, ## __VA_ARGS__); \
> -    } while (0)
> -#else
> -    #define DB_PRINT(...)
> -#endif
> +#define CADENCE_GEM_ERR_DEBUG 0
> +#define DB_PRINT(...) do {\
> +    if (CADENCE_GEM_ERR_DEBUG) {   \
> +        qemu_log(": %s: ", __func__); \
> +        qemu_log(__VA_ARGS__); \
> +    } \
> +} while (0)
>  
>  #define GEM_NWCTRL        (0x00000000/4) /* Network Control reg */
>  #define GEM_NWCFG         (0x00000004/4) /* Network Config reg */
> @@ -979,7 +978,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          size += 4;
>      }
>  
> -    DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
> +    DB_PRINT("config bufsize: %u packet size: %zd\n", rxbufsize, size);
>  
>      /* Find which queue we are targeting */
>      q = get_queue_from_screen(s, rxbuf_ptr, rxbufsize);
> @@ -992,9 +991,9 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>              return -1;
>          }
>  
> -        DB_PRINT("copy %u bytes to 0x%" PRIx64 "\n",
> -                 MIN(bytes_to_copy, rxbufsize),
> -                 rx_desc_get_buffer(s, s->rx_desc[q]));
> +        DB_PRINT("copy %" PRIu32 " bytes to 0x%" PRIx64 "\n",
> +                MIN(bytes_to_copy, rxbufsize),
> +                rx_desc_get_buffer(s, s->rx_desc[q]));
>  
>          /* Copy packet data to emulated DMA buffer */
>          address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) +
> @@ -1160,8 +1159,8 @@ static void gem_transmit(CadenceGEMState *s)
>               */
>              if ((tx_desc_get_buffer(s, desc) == 0) ||
>                  (tx_desc_get_length(desc) == 0)) {
> -                DB_PRINT("Invalid TX descriptor @ 0x%x\n",
> -                         (unsigned)packet_desc_addr);
> +                DB_PRINT("Invalid TX descriptor @ 0x%" HWADDR_PRIx "\n",
> +                         packet_desc_addr);
>                  break;
>              }
>  
> -- 
> 2.7.4
> 

Re: [PATCH v3 02/11] net: cadence_gem: Fix the queue address update during wrap around

[Edgar E. Iglesias]

On Fri, May 08, 2020 at 04:30:36PM +0530, Sai Pavan Boddu wrote:
> During wrap around and reset, queues are pointing to initial base
> address of queue 0, irrespective of what queue we are dealing with.
> Fix it by assigning proper base address every time.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 5476c62..e6dc436 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -845,6 +845,35 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
>      return 0;
>  }
>  
> +static uint32_t gem_get_queue_base_addr(CadenceGEMState *s, bool tx, int q)
> +{
> +    uint32_t base_addr = 0;
> +
> +    switch (q) {
> +    case 0:
> +        base_addr = s->regs[tx ? GEM_TXQBASE : GEM_RXQBASE];
> +        break;
> +    case 1 ... (MAX_PRIORITY_QUEUES - 1):
> +        base_addr = s->regs[(tx ? GEM_TRANSMIT_Q1_PTR :
> +                                 GEM_RECEIVE_Q1_PTR) + q - 1];
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    };
> +
> +    return base_addr;
> +}
> +
> +static uint32_t gem_get_tx_queue_base_addr(CadenceGEMState *s, int q)
> +{
> +    return gem_get_queue_base_addr(s, true, q);
> +}
> +
> +static uint32_t gem_get_rx_queue_base_addr(CadenceGEMState *s, int q)
> +{
> +    return gem_get_queue_base_addr(s, false, q);
> +}
> +
>  static hwaddr gem_get_desc_addr(CadenceGEMState *s, bool tx, int q)
>  {
>      hwaddr desc_addr = 0;
> @@ -1043,7 +1072,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          /* Next descriptor */
>          if (rx_desc_get_wrap(s->rx_desc[q])) {
>              DB_PRINT("wrapping RX descriptor list\n");
> -            s->rx_desc_addr[q] = s->regs[GEM_RXQBASE];
> +            s->rx_desc_addr[q] = gem_get_rx_queue_base_addr(s, q);
>          } else {
>              DB_PRINT("incrementing RX descriptor list\n");
>              s->rx_desc_addr[q] += 4 * gem_get_desc_len(s, true);
> @@ -1199,7 +1228,7 @@ static void gem_transmit(CadenceGEMState *s)
>                                      sizeof(desc_first));
>                  /* Advance the hardware current descriptor past this packet */
>                  if (tx_desc_get_wrap(desc)) {
> -                    s->tx_desc_addr[q] = s->regs[GEM_TXQBASE];
> +                    s->tx_desc_addr[q] = gem_get_tx_queue_base_addr(s, q);
>                  } else {
>                      s->tx_desc_addr[q] = packet_desc_addr +
>                                           4 * gem_get_desc_len(s, false);
> @@ -1251,7 +1280,7 @@ static void gem_transmit(CadenceGEMState *s)
>                  } else {
>                      packet_desc_addr = 0;
>                  }
> -                packet_desc_addr |= s->regs[GEM_TXQBASE];
> +                packet_desc_addr |= gem_get_tx_queue_base_addr(s, q);
>              } else {
>                  packet_desc_addr += 4 * gem_get_desc_len(s, false);
>              }
> @@ -1457,7 +1486,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>          if (!(val & GEM_NWCTRL_TXENA)) {
>              /* Reset to start of Q when transmit disabled. */
>              for (i = 0; i < s->num_priority_queues; i++) {
> -                s->tx_desc_addr[i] = s->regs[GEM_TXQBASE];
> +                s->tx_desc_addr[i] = gem_get_tx_queue_base_addr(s, i);
>              }
>          }
>          if (gem_can_receive(qemu_get_queue(s->nic))) {
> -- 
> 2.7.4
> 

Re: [PATCH v3 06/11] net: cadence_gem: Move tx/rx packet buffert to CadenceGEMState

[Edgar E. Iglesias]

On Fri, May 08, 2020 at 04:30:40PM +0530, Sai Pavan Boddu wrote:
> Moving this buffers to CadenceGEMState, as their size will be increased
> more when JUMBO frames support is added.
> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c         | 52 ++++++++++++++++++++++++++------------------
>  include/hw/net/cadence_gem.h |  2 ++
>  2 files changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 77a0588..5ccec1a 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -314,6 +314,8 @@
>  
>  #define GEM_MODID_VALUE 0x00020118
>  
> +#define MAX_FRAME_SIZE 2048
> +
>  static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
>  {
>      uint64_t ret = desc[0];
> @@ -928,17 +930,14 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
>   */
>  static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>  {
> -    CadenceGEMState *s;
> +    CadenceGEMState *s = qemu_get_nic_opaque(nc);
>      unsigned   rxbufsize, bytes_to_copy;
>      unsigned   rxbuf_offset;
> -    uint8_t    rxbuf[2048];
>      uint8_t   *rxbuf_ptr;
>      bool first_desc = true;
>      int maf;
>      int q = 0;
>  
> -    s = qemu_get_nic_opaque(nc);
> -
>      /* Is this destination MAC address "for us" ? */
>      maf = gem_mac_address_filter(s, buf);
>      if (maf == GEM_RX_REJECT) {
> @@ -994,19 +993,19 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      } else {
>          unsigned crc_val;
>  
> -        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
> -            size = sizeof(rxbuf) - sizeof(crc_val);
> +        if (size > MAX_FRAME_SIZE - sizeof(crc_val)) {
> +            size = MAX_FRAME_SIZE - sizeof(crc_val);
>          }
>          bytes_to_copy = size;
>          /* The application wants the FCS field, which QEMU does not provide.
>           * We must try and calculate one.
>           */
>  
> -        memcpy(rxbuf, buf, size);
> -        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
> -        rxbuf_ptr = rxbuf;
> -        crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
> -        memcpy(rxbuf + size, &crc_val, sizeof(crc_val));
> +        memcpy(s->rx_packet, buf, size);
> +        memset(s->rx_packet + size, 0, MAX_FRAME_SIZE - size);
> +        rxbuf_ptr = s->rx_packet;
> +        crc_val = cpu_to_le32(crc32(0, s->rx_packet, MAX(size, 60)));
> +        memcpy(s->rx_packet + size, &crc_val, sizeof(crc_val));
>  
>          bytes_to_copy += 4;
>          size += 4;
> @@ -1152,7 +1151,6 @@ static void gem_transmit(CadenceGEMState *s)
>  {
>      uint32_t desc[DESC_MAX_NUM_WORDS];
>      hwaddr packet_desc_addr;
> -    uint8_t     tx_packet[2048];
>      uint8_t     *p;
>      unsigned    total_bytes;
>      int q = 0;
> @@ -1168,7 +1166,7 @@ static void gem_transmit(CadenceGEMState *s)
>       * Packets scattered across multiple descriptors are gathered to this
>       * one contiguous buffer first.
>       */
> -    p = tx_packet;
> +    p = s->tx_packet;
>      total_bytes = 0;
>  
>      for (q = s->num_priority_queues - 1; q >= 0; q--) {
> @@ -1198,12 +1196,12 @@ static void gem_transmit(CadenceGEMState *s)
>                  break;
>              }
>  
> -            if (tx_desc_get_length(desc) > sizeof(tx_packet) -
> -                                               (p - tx_packet)) {
> +            if (tx_desc_get_length(desc) > MAX_FRAME_SIZE -
> +                                               (p - s->tx_packet)) {
>                  DB_PRINT("TX descriptor @ 0x%" HWADDR_PRIx \
>                           " too large: size 0x%x space 0x%zx\n",
>                           packet_desc_addr, tx_desc_get_length(desc),
> -                         sizeof(tx_packet) - (p - tx_packet));
> +                         MAX_FRAME_SIZE - (p - s->tx_packet));
>                  break;
>              }
>  
> @@ -1248,24 +1246,24 @@ static void gem_transmit(CadenceGEMState *s)
>  
>                  /* Is checksum offload enabled? */
>                  if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
> -                    net_checksum_calculate(tx_packet, total_bytes);
> +                    net_checksum_calculate(s->tx_packet, total_bytes);
>                  }
>  
>                  /* Update MAC statistics */
> -                gem_transmit_updatestats(s, tx_packet, total_bytes);
> +                gem_transmit_updatestats(s, s->tx_packet, total_bytes);
>  
>                  /* Send the packet somewhere */
>                  if (s->phy_loop || (s->regs[GEM_NWCTRL] &
>                                      GEM_NWCTRL_LOCALLOOP)) {
> -                    gem_receive(qemu_get_queue(s->nic), tx_packet,
> +                    gem_receive(qemu_get_queue(s->nic), s->tx_packet,
>                                  total_bytes);
>                  } else {
> -                    qemu_send_packet(qemu_get_queue(s->nic), tx_packet,
> +                    qemu_send_packet(qemu_get_queue(s->nic), s->tx_packet,
>                                       total_bytes);
>                  }
>  
>                  /* Prepare for next packet */
> -                p = tx_packet;
> +                p = s->tx_packet;
>                  total_bytes = 0;
>              }
>  
> @@ -1612,6 +1610,17 @@ static void gem_realize(DeviceState *dev, Error **errp)
>  
>      s->nic = qemu_new_nic(&net_gem_info, &s->conf,
>                            object_get_typename(OBJECT(dev)), dev->id, s);
> +
> +    s->tx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
> +    s->rx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);

Hi Sai,

Since you're only using MAX_FRAME_SIZE these could be arrays
in CadenceGEMState.

With that change:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Re: [PATCH v3 07/11] net: cadence_gem: Add support for jumbo frames

[Edgar E. Iglesias]

On Fri, May 08, 2020 at 04:30:41PM +0530, Sai Pavan Boddu wrote:
> Add a property "jumbo-max-len", which can be configured for jumbo frame size
> up to 16,383 bytes, and also introduce new register GEM_JUMBO_MAX_LEN.
> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c         | 21 +++++++++++++++++++--
>  include/hw/net/cadence_gem.h |  1 +
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 5ccec1a..45c50ab 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -61,6 +61,7 @@
>  #define GEM_TXPAUSE       (0x0000003C/4) /* TX Pause Time reg */
>  #define GEM_TXPARTIALSF   (0x00000040/4) /* TX Partial Store and Forward */
>  #define GEM_RXPARTIALSF   (0x00000044/4) /* RX Partial Store and Forward */
> +#define GEM_JUMBO_MAX_LEN (0x00000048 / 4) /* Max Jumbo Frame Size */

Would be nice to align this in the same way as all the others...



>  #define GEM_HASHLO        (0x00000080/4) /* Hash Low address reg */
>  #define GEM_HASHHI        (0x00000084/4) /* Hash High address reg */
>  #define GEM_SPADDR1LO     (0x00000088/4) /* Specific addr 1 low reg */
> @@ -314,7 +315,8 @@
>  
>  #define GEM_MODID_VALUE 0x00020118
>  
> -#define MAX_FRAME_SIZE 2048
> +#define MAX_JUMBO_FRAME_SIZE_MASK 0x3FFF
> +#define MAX_FRAME_SIZE MAX_JUMBO_FRAME_SIZE_MASK
>  
>  static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
>  {
> @@ -1343,9 +1345,10 @@ static void gem_reset(DeviceState *d)
>      s->regs[GEM_RXPARTIALSF] = 0x000003ff;
>      s->regs[GEM_MODID] = s->revision;
>      s->regs[GEM_DESCONF] = 0x02500111;
> -    s->regs[GEM_DESCONF2] = 0x2ab13fff;
> +    s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len;
>      s->regs[GEM_DESCONF5] = 0x002f2045;
>      s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
> +    s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len;
>  
>      if (s->num_priority_queues > 1) {
>          queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
> @@ -1420,6 +1423,9 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>          DB_PRINT("lowering irqs on ISR read\n");
>          /* The interrupts get updated at the end of the function. */
>          break;
> +    case GEM_JUMBO_MAX_LEN:
> +        retval = s->jumbo_max_len;
> +        break;
>      case GEM_PHYMNTNC:
>          if (retval & GEM_PHYMNTNC_OP_R) {
>              uint32_t phy_addr, reg_num;
> @@ -1516,6 +1522,9 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>          s->regs[GEM_IMR] &= ~val;
>          gem_update_int_status(s);
>          break;
> +    case GEM_JUMBO_MAX_LEN:
> +        s->jumbo_max_len = val & MAX_JUMBO_FRAME_SIZE_MASK;

I don't think writing to this register may increase the max len
beyond the max-len selected at design time (the property).
TBH I'm surprised this register is RW in the spec.

We may need two variables here, one for the design-time configured
max and another for the runtime configurable max.


> +        break;
>      case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE:
>          s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val;
>          gem_update_int_status(s);
> @@ -1611,6 +1620,12 @@ static void gem_realize(DeviceState *dev, Error **errp)
>      s->nic = qemu_new_nic(&net_gem_info, &s->conf,
>                            object_get_typename(OBJECT(dev)), dev->id, s);
>  
> +    if (s->jumbo_max_len > MAX_FRAME_SIZE) {
> +        g_warning("jumbo-max-len is grater than %d",


You've got a typo here "grater".

I also think we could error out here if wrong values are chosen.

Best regards,
Edgar

RE: [PATCH v3 07/11] net: cadence_gem: Add support for jumbo frames

[Sai Pavan Boddu]

Hi Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Sent: Friday, May 8, 2020 5:14 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Peter Maydell
> <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; Markus
> Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Tong Ho <tongh@xilinx.com>; Ramon Fried
> <rfried.dev@gmail.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org
> Subject: Re: [PATCH v3 07/11] net: cadence_gem: Add support for jumbo
> frames
> 
> On Fri, May 08, 2020 at 04:30:41PM +0530, Sai Pavan Boddu wrote:
> > Add a property "jumbo-max-len", which can be configured for jumbo
> > frame size up to 16,383 bytes, and also introduce new register
> GEM_JUMBO_MAX_LEN.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  hw/net/cadence_gem.c         | 21 +++++++++++++++++++--
> >  include/hw/net/cadence_gem.h |  1 +
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> > 5ccec1a..45c50ab 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -61,6 +61,7 @@
> >  #define GEM_TXPAUSE       (0x0000003C/4) /* TX Pause Time reg */
> >  #define GEM_TXPARTIALSF   (0x00000040/4) /* TX Partial Store and
> Forward */
> >  #define GEM_RXPARTIALSF   (0x00000044/4) /* RX Partial Store and
> Forward */
> > +#define GEM_JUMBO_MAX_LEN (0x00000048 / 4) /* Max Jumbo Frame
> Size */
> 
> Would be nice to align this in the same way as all the others...
> 
> 
> 
> >  #define GEM_HASHLO        (0x00000080/4) /* Hash Low address reg */
> >  #define GEM_HASHHI        (0x00000084/4) /* Hash High address reg */
> >  #define GEM_SPADDR1LO     (0x00000088/4) /* Specific addr 1 low reg */
> > @@ -314,7 +315,8 @@
> >
> >  #define GEM_MODID_VALUE 0x00020118
> >
> > -#define MAX_FRAME_SIZE 2048
> > +#define MAX_JUMBO_FRAME_SIZE_MASK 0x3FFF #define
> MAX_FRAME_SIZE
> > +MAX_JUMBO_FRAME_SIZE_MASK
> >
> >  static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s,
> > uint32_t *desc)  { @@ -1343,9 +1345,10 @@ static void
> > gem_reset(DeviceState *d)
> >      s->regs[GEM_RXPARTIALSF] = 0x000003ff;
> >      s->regs[GEM_MODID] = s->revision;
> >      s->regs[GEM_DESCONF] = 0x02500111;
> > -    s->regs[GEM_DESCONF2] = 0x2ab13fff;
> > +    s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len;
> >      s->regs[GEM_DESCONF5] = 0x002f2045;
> >      s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
> > +    s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len;
> >
> >      if (s->num_priority_queues > 1) {
> >          queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
> > @@ -1420,6 +1423,9 @@ static uint64_t gem_read(void *opaque, hwaddr
> offset, unsigned size)
> >          DB_PRINT("lowering irqs on ISR read\n");
> >          /* The interrupts get updated at the end of the function. */
> >          break;
> > +    case GEM_JUMBO_MAX_LEN:
> > +        retval = s->jumbo_max_len;
> > +        break;
> >      case GEM_PHYMNTNC:
> >          if (retval & GEM_PHYMNTNC_OP_R) {
> >              uint32_t phy_addr, reg_num; @@ -1516,6 +1522,9 @@ static
> > void gem_write(void *opaque, hwaddr offset, uint64_t val,
> >          s->regs[GEM_IMR] &= ~val;
> >          gem_update_int_status(s);
> >          break;
> > +    case GEM_JUMBO_MAX_LEN:
> > +        s->jumbo_max_len = val & MAX_JUMBO_FRAME_SIZE_MASK;
> 
> I don't think writing to this register may increase the max len beyond the
> max-len selected at design time (the property).
> TBH I'm surprised this register is RW in the spec.
> 
> We may need two variables here, one for the design-time configured max
> and another for the runtime configurable max.
[Sai Pavan Boddu] Better way is to, use new created property  to set the reset value of  this register. Which can be overwritten by guest on runtime by writing to regs[GEM_JUMBO_MAX_LEN].

I would add few checks, so that frames does not cross JUMBO max length configured.

Thanks,
Sai Pavan
> 
> 
> > +        break;
> >      case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE:
> >          s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &=
> ~val;
> >          gem_update_int_status(s);
> > @@ -1611,6 +1620,12 @@ static void gem_realize(DeviceState *dev, Error
> **errp)
> >      s->nic = qemu_new_nic(&net_gem_info, &s->conf,
> >                            object_get_typename(OBJECT(dev)), dev->id,
> > s);
> >
> > +    if (s->jumbo_max_len > MAX_FRAME_SIZE) {
> > +        g_warning("jumbo-max-len is grater than %d",
> 
> 
> You've got a typo here "grater".
> 
> I also think we could error out here if wrong values are chosen.
> 
> Best regards,
> Edgar

RE: [PATCH v3 06/11] net: cadence_gem: Move tx/rx packet buffert to CadenceGEMState

[Sai Pavan Boddu]

Hi Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Sent: Friday, May 8, 2020 5:00 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Peter Maydell
> <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; Markus
> Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Tong Ho <tongh@xilinx.com>; Ramon Fried
> <rfried.dev@gmail.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org
> Subject: Re: [PATCH v3 06/11] net: cadence_gem: Move tx/rx packet buffert
> to CadenceGEMState
> 
> On Fri, May 08, 2020 at 04:30:40PM +0530, Sai Pavan Boddu wrote:
> > Moving this buffers to CadenceGEMState, as their size will be
> > increased more when JUMBO frames support is added.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  hw/net/cadence_gem.c         | 52 ++++++++++++++++++++++++++---------
> ---------
> >  include/hw/net/cadence_gem.h |  2 ++
> >  2 files changed, 33 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> > 77a0588..5ccec1a 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -314,6 +314,8 @@
> >
> >  #define GEM_MODID_VALUE 0x00020118
> >
> > +#define MAX_FRAME_SIZE 2048
> > +
> >  static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s,
> > uint32_t *desc)  {
> >      uint64_t ret = desc[0];
> > @@ -928,17 +930,14 @@ static void gem_get_rx_desc(CadenceGEMState
> *s, int q)
> >   */
> >  static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf,
> > size_t size)  {
> > -    CadenceGEMState *s;
> > +    CadenceGEMState *s = qemu_get_nic_opaque(nc);
> >      unsigned   rxbufsize, bytes_to_copy;
> >      unsigned   rxbuf_offset;
> > -    uint8_t    rxbuf[2048];
> >      uint8_t   *rxbuf_ptr;
> >      bool first_desc = true;
> >      int maf;
> >      int q = 0;
> >
> > -    s = qemu_get_nic_opaque(nc);
> > -
> >      /* Is this destination MAC address "for us" ? */
> >      maf = gem_mac_address_filter(s, buf);
> >      if (maf == GEM_RX_REJECT) {
> > @@ -994,19 +993,19 @@ static ssize_t gem_receive(NetClientState *nc,
> const uint8_t *buf, size_t size)
> >      } else {
> >          unsigned crc_val;
> >
> > -        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
> > -            size = sizeof(rxbuf) - sizeof(crc_val);
> > +        if (size > MAX_FRAME_SIZE - sizeof(crc_val)) {
> > +            size = MAX_FRAME_SIZE - sizeof(crc_val);
> >          }
> >          bytes_to_copy = size;
> >          /* The application wants the FCS field, which QEMU does not provide.
> >           * We must try and calculate one.
> >           */
> >
> > -        memcpy(rxbuf, buf, size);
> > -        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
> > -        rxbuf_ptr = rxbuf;
> > -        crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
> > -        memcpy(rxbuf + size, &crc_val, sizeof(crc_val));
> > +        memcpy(s->rx_packet, buf, size);
> > +        memset(s->rx_packet + size, 0, MAX_FRAME_SIZE - size);
> > +        rxbuf_ptr = s->rx_packet;
> > +        crc_val = cpu_to_le32(crc32(0, s->rx_packet, MAX(size, 60)));
> > +        memcpy(s->rx_packet + size, &crc_val, sizeof(crc_val));
> >
> >          bytes_to_copy += 4;
> >          size += 4;
> > @@ -1152,7 +1151,6 @@ static void gem_transmit(CadenceGEMState *s)  {
> >      uint32_t desc[DESC_MAX_NUM_WORDS];
> >      hwaddr packet_desc_addr;
> > -    uint8_t     tx_packet[2048];
> >      uint8_t     *p;
> >      unsigned    total_bytes;
> >      int q = 0;
> > @@ -1168,7 +1166,7 @@ static void gem_transmit(CadenceGEMState *s)
> >       * Packets scattered across multiple descriptors are gathered to this
> >       * one contiguous buffer first.
> >       */
> > -    p = tx_packet;
> > +    p = s->tx_packet;
> >      total_bytes = 0;
> >
> >      for (q = s->num_priority_queues - 1; q >= 0; q--) { @@ -1198,12
> > +1196,12 @@ static void gem_transmit(CadenceGEMState *s)
> >                  break;
> >              }
> >
> > -            if (tx_desc_get_length(desc) > sizeof(tx_packet) -
> > -                                               (p - tx_packet)) {
> > +            if (tx_desc_get_length(desc) > MAX_FRAME_SIZE -
> > +                                               (p - s->tx_packet)) {
> >                  DB_PRINT("TX descriptor @ 0x%" HWADDR_PRIx \
> >                           " too large: size 0x%x space 0x%zx\n",
> >                           packet_desc_addr, tx_desc_get_length(desc),
> > -                         sizeof(tx_packet) - (p - tx_packet));
> > +                         MAX_FRAME_SIZE - (p - s->tx_packet));
> >                  break;
> >              }
> >
> > @@ -1248,24 +1246,24 @@ static void gem_transmit(CadenceGEMState
> *s)
> >
> >                  /* Is checksum offload enabled? */
> >                  if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
> > -                    net_checksum_calculate(tx_packet, total_bytes);
> > +                    net_checksum_calculate(s->tx_packet,
> > + total_bytes);
> >                  }
> >
> >                  /* Update MAC statistics */
> > -                gem_transmit_updatestats(s, tx_packet, total_bytes);
> > +                gem_transmit_updatestats(s, s->tx_packet,
> > + total_bytes);
> >
> >                  /* Send the packet somewhere */
> >                  if (s->phy_loop || (s->regs[GEM_NWCTRL] &
> >                                      GEM_NWCTRL_LOCALLOOP)) {
> > -                    gem_receive(qemu_get_queue(s->nic), tx_packet,
> > +                    gem_receive(qemu_get_queue(s->nic), s->tx_packet,
> >                                  total_bytes);
> >                  } else {
> > -                    qemu_send_packet(qemu_get_queue(s->nic), tx_packet,
> > +                    qemu_send_packet(qemu_get_queue(s->nic),
> > + s->tx_packet,
> >                                       total_bytes);
> >                  }
> >
> >                  /* Prepare for next packet */
> > -                p = tx_packet;
> > +                p = s->tx_packet;
> >                  total_bytes = 0;
> >              }
> >
> > @@ -1612,6 +1610,17 @@ static void gem_realize(DeviceState *dev, Error
> > **errp)
> >
> >      s->nic = qemu_new_nic(&net_gem_info, &s->conf,
> >                            object_get_typename(OBJECT(dev)), dev->id,
> > s);
> > +
> > +    s->tx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
> > +    s->rx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
> 
> Hi Sai,
> 
> Since you're only using MAX_FRAME_SIZE these could be arrays in
> CadenceGEMState.
[Sai Pavan Boddu] Ok, I will put this in v4.

Thanks,
Sai Pavan
> 
> With that change:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>