[PATCH-for-9.0 v2 00/11] hw/net/lan9118: Fix overflow in TX FIFO

[PATCH-for-9.0 v2 00/11] hw/net/lan9118: Fix overflow in TX FIFO

[Philippe Mathieu-Daudé]

Fix for https://gitlab.com/qemu-project/qemu/-/issues/2267

Since v1:
- Renamed definition as MIL_TXFIFO_SIZE
- Addressed Peter review comments in patches 1 & 2
  (add comment, return TXE INT)
- Trivial patches while digesting Peter's analysis [*]

More work expected during 9.1.

[*] https://lore.kernel.org/qemu-devel/CAFEAcA8vvURMn2FaDP9tXtP5eCMs6-XFOCR9ypo=WBH+6g5prw@mail.gmail.com/

Philippe Mathieu-Daudé (11):
  hw/net/lan9118: Replace magic '2048' value by MIL_TXFIFO_SIZE
    definition
  hw/net/lan9118: Fix overflow in MIL TX FIFO
  hw/net/lan9118: Remove duplicated assignment
  hw/net/lan9118: Replace magic '5' value by TX_FIF_SZ_RESET definition
  hw/net/lan9118: Add definitions for FIFO allocated sizes
  hw/net/lan9118: Use TX_DATA_FIFO_BYTES definition
  hw/net/lan9118: Rename tx_fifo_size -> tx_fifo_bytes
  hw/net/lan9118: Use RX_STATUS_FIFO_BYTES definition
  hw/net/lan9118: Rename rx_status_fifo_size -> rx_status_fifo_wordcount
  hw/net/lan9118: Use RX_DATA_FIFO_BYTES definition
  hw/net/lan9118: Rename rx_fifo_size -> rx_fifo_wordcount

 hw/net/lan9118.c | 104 +++++++++++++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 34 deletions(-)

-- 
2.41.0

[PATCH-for-9.0 v2 01/11] hw/net/lan9118: Replace magic '2048' value by MIL_TXFIFO_SIZE definition

[Philippe Mathieu-Daudé]

The magic 2048 is explained in the LAN9211 datasheet (DS00002414A)
in chapter 1.4, "10/100 Ethernet MAC":

  The MAC Interface Layer (MIL), within the MAC, contains a
  2K Byte transmit and a 128 Byte receive FIFO which is separate
  from the TX and RX FIFOs. [...]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
(Not including Peter R-b from v1 due to semantic change)

 hw/net/lan9118.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 47ff25b441..8214569a2c 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -150,6 +150,12 @@ do { printf("lan9118: " fmt , ## __VA_ARGS__); } while (0)
 
 #define GPT_TIMER_EN    0x20000000
 
+/*
+ * The MAC Interface Layer (MIL), within the MAC, contains a 2K Byte transmit
+ * and a 128 Byte receive FIFO which is separate from the TX and RX FIFOs.
+ */
+#define MIL_TXFIFO_SIZE         2048
+
 enum tx_state {
     TX_IDLE,
     TX_B,
@@ -166,7 +172,7 @@ typedef struct {
     int32_t pad;
     int32_t fifo_used;
     int32_t len;
-    uint8_t data[2048];
+    uint8_t data[MIL_TXFIFO_SIZE];
 } LAN9118Packet;
 
 static const VMStateDescription vmstate_lan9118_packet = {
@@ -182,7 +188,7 @@ static const VMStateDescription vmstate_lan9118_packet = {
         VMSTATE_INT32(pad, LAN9118Packet),
         VMSTATE_INT32(fifo_used, LAN9118Packet),
         VMSTATE_INT32(len, LAN9118Packet),
-        VMSTATE_UINT8_ARRAY(data, LAN9118Packet, 2048),
+        VMSTATE_UINT8_ARRAY(data, LAN9118Packet, MIL_TXFIFO_SIZE),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -544,7 +550,7 @@ static ssize_t lan9118_receive(NetClientState *nc, const uint8_t *buf,
         return -1;
     }
 
-    if (size >= 2048 || size < 14) {
+    if (size >= MIL_TXFIFO_SIZE || size < 14) {
         return -1;
     }
 
-- 
2.41.0

[PATCH-for-9.0 v2 02/11] hw/net/lan9118: Fix overflow in MIL TX FIFO

[Philippe Mathieu-Daudé]

When the MAC Interface Layer (MIL) transmit FIFO is full,
truncate the packet, and raise the Transmitter Error (TXE)
flag.

Broken since model introduction in commit 2a42499017
("LAN9118 emulation").

When using the reproducer from
https://gitlab.com/qemu-project/qemu/-/issues/2267 we get:

  hw/net/lan9118.c:798:17: runtime error:
  index 2048 out of bounds for type 'uint8_t[2048]' (aka 'unsigned char[2048]')
    #0 0x563ec9a057b1 in tx_fifo_push hw/net/lan9118.c:798:43
    #1 0x563ec99fbb28 in lan9118_writel hw/net/lan9118.c:1042:9
    #2 0x563ec99f2de2 in lan9118_16bit_mode_write hw/net/lan9118.c:1205:9
    #3 0x563ecbf78013 in memory_region_write_accessor system/memory.c:497:5
    #4 0x563ecbf776f5 in access_with_adjusted_size system/memory.c:573:18
    #5 0x563ecbf75643 in memory_region_dispatch_write system/memory.c:1521:16
    #6 0x563ecc01bade in flatview_write_continue_step system/physmem.c:2713:18
    #7 0x563ecc01b374 in flatview_write_continue system/physmem.c:2743:19
    #8 0x563ecbff1c9b in flatview_write system/physmem.c:2774:12
    #9 0x563ecbff1768 in address_space_write system/physmem.c:2894:18
    ...

[*] LAN9118 DS00002266B.pdf, Table 5.3.3 "INTERRUPT STATUS REGISTER"

Reported-by: Will Lester
Reported-by: Chuhong Yuan <hslester96@gmail.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2267
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 8214569a2c..91d81b410b 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -799,8 +799,22 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
             /* Documentation is somewhat unclear on the ordering of bytes
                in FIFO words.  Empirical results show it to be little-endian.
                */
-            /* TODO: FIFO overflow checking.  */
             while (n--) {
+                if (s->txp->len == MIL_TXFIFO_SIZE) {
+                    /*
+                     * No more space in the FIFO. The datasheet is not
+                     * precise about this case. We choose what is easiest
+                     * to model: the packet is truncated, and TXE is raised.
+                     *
+                     * Note, it could be a fragmented packet, but we currently
+                     * do not handle that (see earlier TX_B case).
+                     */
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "MIL TX FIFO overrun, discarding %u byte%s\n",
+                                  n, n > 1 ? "s" : "");
+                    s->int_sts |= TXE_INT;
+                    break;
+                }
                 s->txp->data[s->txp->len] = val & 0xff;
                 s->txp->len++;
                 val >>= 8;
-- 
2.41.0

[PATCH-for-9.1 v2 03/11] hw/net/lan9118: Remove duplicated assignment

[Philippe Mathieu-Daudé]

s->txp->fifo_used is zeroed in the next 3 lines.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 91d81b410b..d6f0e37eb1 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -438,7 +438,6 @@ static void lan9118_reset(DeviceState *d)
     s->hw_cfg = s->mode_16bit ? 0x00050000 : 0x00050004;
     s->pmt_ctrl &= 0x45;
     s->gpio_cfg = 0;
-    s->txp->fifo_used = 0;
     s->txp->state = TX_IDLE;
     s->txp->cmd_a = 0xffffffffu;
     s->txp->cmd_b = 0xffffffffu;
-- 
2.41.0

[PATCH-for-9.1 v2 04/11] hw/net/lan9118: Replace magic '5' value by TX_FIF_SZ_RESET definition

[Philippe Mathieu-Daudé]

TX_FIF_SZ is described in chapter 5.3.9,
"HW_CFG — HARDWARE CONFIGURATION REGISTER".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index d6f0e37eb1..a6a869de32 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -71,6 +71,8 @@ do { printf("lan9118: " fmt , ## __VA_ARGS__); } while (0)
 #define CSR_E2P_CMD     0xb0
 #define CSR_E2P_DATA    0xb4
 
+#define TX_FIF_SZ_RESET 5
+
 #define E2P_CMD_MAC_ADDR_LOADED 0x100
 
 /* IRQ_CFG */
@@ -435,7 +437,7 @@ static void lan9118_reset(DeviceState *d)
     s->fifo_int = 0x48000000;
     s->rx_cfg = 0;
     s->tx_cfg = 0;
-    s->hw_cfg = s->mode_16bit ? 0x00050000 : 0x00050004;
+    s->hw_cfg = (TX_FIF_SZ_RESET << 16) | (s->mode_16bit << 2);
     s->pmt_ctrl &= 0x45;
     s->gpio_cfg = 0;
     s->txp->state = TX_IDLE;
-- 
2.41.0

[PATCH-for-9.1 v2 05/11] hw/net/lan9118: Add definitions for FIFO allocated sizes

[Philippe Mathieu-Daudé]

Add definitions for the TX_FIF_SZ=5 case, per TABLE 5-3
"VALID TX/RX FIFO ALLOCATIONS".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index a6a869de32..00409927fe 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -158,6 +158,17 @@ do { printf("lan9118: " fmt , ## __VA_ARGS__); } while (0)
  */
 #define MIL_TXFIFO_SIZE         2048
 
+/*
+ * TX and RX FIFO space is configurable through the TX FIFO Size (TX_FIF_SZ)
+ * field in the hardware configuration (CSR HW_CFG) register. These are the
+ * default configuration settings for TX_FIF_SZ = 5
+ * (see TABLE 5-3: VALID TX/RX FIFO ALLOCATIONS).
+ */
+#define TX_DATA_FIFO_BYTES      4608    /* 1152 words */
+#define TX_STATUS_FIFO_BYTES    512     /* 128 words */
+#define RX_DATA_FIFO_BYTES      10560   /* 2640 words */
+#define RX_STATUS_FIFO_BYTES    704     /* 176 words */
+
 enum tx_state {
     TX_IDLE,
     TX_B,
-- 
2.41.0

[PATCH-for-9.1 v2 06/11] hw/net/lan9118: Use TX_DATA_FIFO_BYTES definition

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 00409927fe..ba92681e2e 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -456,7 +456,7 @@ static void lan9118_reset(DeviceState *d)
     s->txp->cmd_b = 0xffffffffu;
     s->txp->len = 0;
     s->txp->fifo_used = 0;
-    s->tx_fifo_size = 4608;
+    s->tx_fifo_size = TX_DATA_FIFO_BYTES;
     s->tx_status_fifo_used = 0;
     s->rx_status_fifo_size = 704;
     s->rx_fifo_size = 2640;
-- 
2.41.0

[PATCH-for-9.1 v2 07/11] hw/net/lan9118: Rename tx_fifo_size -> tx_fifo_bytes

[Philippe Mathieu-Daudé]

tx_fifo_size is a byte count, rename it to avoid confusion.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index ba92681e2e..a983ce193b 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -251,7 +251,7 @@ struct lan9118_state {
     int32_t eeprom_writable;
     uint8_t eeprom[128];
 
-    int32_t tx_fifo_size;
+    int32_t tx_fifo_bytes;
     LAN9118Packet *txp;
     LAN9118Packet tx_packet;
 
@@ -322,7 +322,7 @@ static const VMStateDescription vmstate_lan9118 = {
         VMSTATE_UINT32(phy_int_mask, lan9118_state),
         VMSTATE_INT32(eeprom_writable, lan9118_state),
         VMSTATE_UINT8_ARRAY(eeprom, lan9118_state, 128),
-        VMSTATE_INT32(tx_fifo_size, lan9118_state),
+        VMSTATE_INT32(tx_fifo_bytes, lan9118_state),
         /* txp always points at tx_packet so need not be saved */
         VMSTATE_STRUCT(tx_packet, lan9118_state, 0,
                        vmstate_lan9118_packet, LAN9118Packet),
@@ -456,7 +456,7 @@ static void lan9118_reset(DeviceState *d)
     s->txp->cmd_b = 0xffffffffu;
     s->txp->len = 0;
     s->txp->fifo_used = 0;
-    s->tx_fifo_size = TX_DATA_FIFO_BYTES;
+    s->tx_fifo_bytes = TX_DATA_FIFO_BYTES;
     s->tx_status_fifo_used = 0;
     s->rx_status_fifo_size = 704;
     s->rx_fifo_size = 2640;
@@ -757,7 +757,7 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
 {
     int n;
 
-    if (s->txp->fifo_used == s->tx_fifo_size) {
+    if (s->txp->fifo_used == s->tx_fifo_bytes) {
         s->int_sts |= TDFO_INT;
         return;
     }
@@ -1285,7 +1285,7 @@ static uint64_t lan9118_readl(void *opaque, hwaddr offset,
         return (s->rx_status_fifo_used << 16) | (s->rx_fifo_used << 2);
     case CSR_TX_FIFO_INF:
         return (s->tx_status_fifo_used << 16)
-               | (s->tx_fifo_size - s->txp->fifo_used);
+               | (s->tx_fifo_bytes - s->txp->fifo_used);
     case CSR_PMT_CTRL:
         return s->pmt_ctrl;
     case CSR_GPIO_CFG:
-- 
2.41.0

[PATCH-for-9.1 v2 08/11] hw/net/lan9118: Use RX_STATUS_FIFO_BYTES definition

[Philippe Mathieu-Daudé]

rx_status_fifo[] is an array of words,
rx_status_fifo_size is a word count.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index a983ce193b..cace22381d 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -262,7 +262,7 @@ struct lan9118_state {
     int32_t rx_status_fifo_size;
     int32_t rx_status_fifo_used;
     int32_t rx_status_fifo_head;
-    uint32_t rx_status_fifo[896];
+    uint32_t rx_status_fifo[RX_STATUS_FIFO_BYTES / 4];
     int32_t rx_fifo_size;
     int32_t rx_fifo_used;
     int32_t rx_fifo_head;
@@ -332,7 +332,9 @@ static const VMStateDescription vmstate_lan9118 = {
         VMSTATE_INT32(rx_status_fifo_size, lan9118_state),
         VMSTATE_INT32(rx_status_fifo_used, lan9118_state),
         VMSTATE_INT32(rx_status_fifo_head, lan9118_state),
-        VMSTATE_UINT32_ARRAY(rx_status_fifo, lan9118_state, 896),
+        VMSTATE_UINT32_ARRAY(rx_status_fifo, lan9118_state,
+                             RX_STATUS_FIFO_BYTES / 4),
+        VMSTATE_UNUSED(896 * 4 - RX_STATUS_FIFO_BYTES),
         VMSTATE_INT32(rx_fifo_size, lan9118_state),
         VMSTATE_INT32(rx_fifo_used, lan9118_state),
         VMSTATE_INT32(rx_fifo_head, lan9118_state),
@@ -458,10 +460,9 @@ static void lan9118_reset(DeviceState *d)
     s->txp->fifo_used = 0;
     s->tx_fifo_bytes = TX_DATA_FIFO_BYTES;
     s->tx_status_fifo_used = 0;
-    s->rx_status_fifo_size = 704;
     s->rx_fifo_size = 2640;
     s->rx_fifo_used = 0;
-    s->rx_status_fifo_size = 176;
+    s->rx_status_fifo_size = RX_STATUS_FIFO_BYTES / 4;
     s->rx_status_fifo_used = 0;
     s->rxp_offset = 0;
     s->rxp_size = 0;
-- 
2.41.0

[PATCH-for-9.1 v2 09/11] hw/net/lan9118: Rename rx_status_fifo_size -> rx_status_fifo_wordcount

[Philippe Mathieu-Daudé]

rx_status_fifo_size is a word count, rename it to avoid confusion.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index cace22381d..663776f575 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -259,7 +259,7 @@ struct lan9118_state {
     int32_t tx_status_fifo_head;
     uint32_t tx_status_fifo[512];
 
-    int32_t rx_status_fifo_size;
+    int32_t rx_status_fifo_wordcount;
     int32_t rx_status_fifo_used;
     int32_t rx_status_fifo_head;
     uint32_t rx_status_fifo[RX_STATUS_FIFO_BYTES / 4];
@@ -329,7 +329,7 @@ static const VMStateDescription vmstate_lan9118 = {
         VMSTATE_INT32(tx_status_fifo_used, lan9118_state),
         VMSTATE_INT32(tx_status_fifo_head, lan9118_state),
         VMSTATE_UINT32_ARRAY(tx_status_fifo, lan9118_state, 512),
-        VMSTATE_INT32(rx_status_fifo_size, lan9118_state),
+        VMSTATE_INT32(rx_status_fifo_wordcount, lan9118_state),
         VMSTATE_INT32(rx_status_fifo_used, lan9118_state),
         VMSTATE_INT32(rx_status_fifo_head, lan9118_state),
         VMSTATE_UINT32_ARRAY(rx_status_fifo, lan9118_state,
@@ -462,7 +462,7 @@ static void lan9118_reset(DeviceState *d)
     s->tx_status_fifo_used = 0;
     s->rx_fifo_size = 2640;
     s->rx_fifo_used = 0;
-    s->rx_status_fifo_size = RX_STATUS_FIFO_BYTES / 4;
+    s->rx_status_fifo_wordcount = RX_STATUS_FIFO_BYTES / 4;
     s->rx_status_fifo_used = 0;
     s->rxp_offset = 0;
     s->rxp_size = 0;
@@ -568,7 +568,7 @@ static ssize_t lan9118_receive(NetClientState *nc, const uint8_t *buf,
     }
 
     /* TODO: Implement FIFO overflow notification.  */
-    if (s->rx_status_fifo_used == s->rx_status_fifo_size) {
+    if (s->rx_status_fifo_used == s->rx_status_fifo_wordcount) {
         return -1;
     }
 
@@ -609,8 +609,8 @@ static ssize_t lan9118_receive(NetClientState *nc, const uint8_t *buf,
         rx_fifo_push(s, crc);
     }
     n = s->rx_status_fifo_head + s->rx_status_fifo_used;
-    if (n >= s->rx_status_fifo_size) {
-        n -= s->rx_status_fifo_size;
+    if (n >= s->rx_status_fifo_wordcount) {
+        n -= s->rx_status_fifo_wordcount;
     }
     s->rx_packet_size[s->rx_packet_size_tail] = fifo_len;
     s->rx_packet_size_tail = (s->rx_packet_size_tail + 1023) & 1023;
@@ -732,8 +732,8 @@ static uint32_t rx_status_fifo_pop(lan9118_state *s)
     if (s->rx_status_fifo_used != 0) {
         s->rx_status_fifo_used--;
         s->rx_status_fifo_head++;
-        if (s->rx_status_fifo_head >= s->rx_status_fifo_size) {
-            s->rx_status_fifo_head -= s->rx_status_fifo_size;
+        if (s->rx_status_fifo_head >= s->rx_status_fifo_wordcount) {
+            s->rx_status_fifo_head -= s->rx_status_fifo_wordcount;
         }
         /* ??? What value should be returned when the FIFO is empty?  */
         DPRINTF("RX status pop 0x%08x\n", val);
-- 
2.41.0

[PATCH-for-9.1 v2 10/11] hw/net/lan9118: Use RX_DATA_FIFO_BYTES definition

[Philippe Mathieu-Daudé]

rx_fifo[] is an array of words,
rx_fifo_size is a word count.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 663776f575..56cc52d450 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -266,7 +266,7 @@ struct lan9118_state {
     int32_t rx_fifo_size;
     int32_t rx_fifo_used;
     int32_t rx_fifo_head;
-    uint32_t rx_fifo[3360];
+    uint32_t rx_fifo[RX_DATA_FIFO_BYTES / 4];
     int32_t rx_packet_size_head;
     int32_t rx_packet_size_tail;
     int32_t rx_packet_size[1024];
@@ -338,7 +338,9 @@ static const VMStateDescription vmstate_lan9118 = {
         VMSTATE_INT32(rx_fifo_size, lan9118_state),
         VMSTATE_INT32(rx_fifo_used, lan9118_state),
         VMSTATE_INT32(rx_fifo_head, lan9118_state),
-        VMSTATE_UINT32_ARRAY(rx_fifo, lan9118_state, 3360),
+        VMSTATE_UINT32_ARRAY(rx_fifo, lan9118_state,
+                             RX_DATA_FIFO_BYTES / 4),
+        VMSTATE_UNUSED(3360 * 4 - RX_DATA_FIFO_BYTES),
         VMSTATE_INT32(rx_packet_size_head, lan9118_state),
         VMSTATE_INT32(rx_packet_size_tail, lan9118_state),
         VMSTATE_INT32_ARRAY(rx_packet_size, lan9118_state, 1024),
@@ -460,7 +462,7 @@ static void lan9118_reset(DeviceState *d)
     s->txp->fifo_used = 0;
     s->tx_fifo_bytes = TX_DATA_FIFO_BYTES;
     s->tx_status_fifo_used = 0;
-    s->rx_fifo_size = 2640;
+    s->rx_fifo_size = RX_DATA_FIFO_BYTES / 4;
     s->rx_fifo_used = 0;
     s->rx_status_fifo_wordcount = RX_STATUS_FIFO_BYTES / 4;
     s->rx_status_fifo_used = 0;
-- 
2.41.0

[PATCH-for-9.1 v2 11/11] hw/net/lan9118: Rename rx_fifo_size -> rx_fifo_wordcount

[Philippe Mathieu-Daudé]

rx_fifo_size is a word count, rename it to avoid confusion.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/lan9118.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 56cc52d450..3db6bae908 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -263,7 +263,7 @@ struct lan9118_state {
     int32_t rx_status_fifo_used;
     int32_t rx_status_fifo_head;
     uint32_t rx_status_fifo[RX_STATUS_FIFO_BYTES / 4];
-    int32_t rx_fifo_size;
+    int32_t rx_fifo_wordcount;
     int32_t rx_fifo_used;
     int32_t rx_fifo_head;
     uint32_t rx_fifo[RX_DATA_FIFO_BYTES / 4];
@@ -335,7 +335,7 @@ static const VMStateDescription vmstate_lan9118 = {
         VMSTATE_UINT32_ARRAY(rx_status_fifo, lan9118_state,
                              RX_STATUS_FIFO_BYTES / 4),
         VMSTATE_UNUSED(896 * 4 - RX_STATUS_FIFO_BYTES),
-        VMSTATE_INT32(rx_fifo_size, lan9118_state),
+        VMSTATE_INT32(rx_fifo_wordcount, lan9118_state),
         VMSTATE_INT32(rx_fifo_used, lan9118_state),
         VMSTATE_INT32(rx_fifo_head, lan9118_state),
         VMSTATE_UINT32_ARRAY(rx_fifo, lan9118_state,
@@ -462,7 +462,7 @@ static void lan9118_reset(DeviceState *d)
     s->txp->fifo_used = 0;
     s->tx_fifo_bytes = TX_DATA_FIFO_BYTES;
     s->tx_status_fifo_used = 0;
-    s->rx_fifo_size = RX_DATA_FIFO_BYTES / 4;
+    s->rx_fifo_wordcount = RX_DATA_FIFO_BYTES / 4;
     s->rx_fifo_used = 0;
     s->rx_status_fifo_wordcount = RX_STATUS_FIFO_BYTES / 4;
     s->rx_status_fifo_used = 0;
@@ -504,8 +504,9 @@ static void rx_fifo_push(lan9118_state *s, uint32_t val)
 {
     int fifo_pos;
     fifo_pos = s->rx_fifo_head + s->rx_fifo_used;
-    if (fifo_pos >= s->rx_fifo_size)
-      fifo_pos -= s->rx_fifo_size;
+    if (fifo_pos >= s->rx_fifo_wordcount) {
+      fifo_pos -= s->rx_fifo_wordcount;
+    }
     s->rx_fifo[fifo_pos] = val;
     s->rx_fifo_used++;
 }
@@ -584,7 +585,7 @@ static ssize_t lan9118_receive(NetClientState *nc, const uint8_t *buf,
     fifo_len = (size + n + 3) >> 2;
     /* Add a word for the CRC.  */
     fifo_len++;
-    if (s->rx_fifo_size - s->rx_fifo_used < fifo_len) {
+    if (s->rx_fifo_wordcount - s->rx_fifo_used < fifo_len) {
         return -1;
     }
 
@@ -672,8 +673,8 @@ static uint32_t rx_fifo_pop(lan9118_state *s)
     } else if (s->rxp_size > 0) {
         s->rxp_size--;
         val = s->rx_fifo[s->rx_fifo_head++];
-        if (s->rx_fifo_head >= s->rx_fifo_size) {
-            s->rx_fifo_head -= s->rx_fifo_size;
+        if (s->rx_fifo_head >= s->rx_fifo_wordcount) {
+            s->rx_fifo_head -= s->rx_fifo_wordcount;
         }
         s->rx_fifo_used--;
     } else if (s->rxp_pad > 0) {
@@ -1135,8 +1136,8 @@ static void lan9118_writel(void *opaque, hwaddr offset,
                 s->rxp_offset = 0;
             }
             s->rx_fifo_head += s->rxp_size;
-            if (s->rx_fifo_head >= s->rx_fifo_size) {
-                s->rx_fifo_head -= s->rx_fifo_size;
+            if (s->rx_fifo_head >= s->rx_fifo_wordcount) {
+                s->rx_fifo_head -= s->rx_fifo_wordcount;
             }
         }
         break;
-- 
2.41.0

Re: [PATCH-for-9.0 v2 01/11] hw/net/lan9118: Replace magic '2048' value by MIL_TXFIFO_SIZE definition

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The magic 2048 is explained in the LAN9211 datasheet (DS00002414A)
> in chapter 1.4, "10/100 Ethernet MAC":
>
>   The MAC Interface Layer (MIL), within the MAC, contains a
>   2K Byte transmit and a 128 Byte receive FIFO which is separate
>   from the TX and RX FIFOs. [...]
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> (Not including Peter R-b from v1 due to semantic change)

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

Though the use of the constant in lan9118_receive()
reveals that our implementation is using the same buffer
for both tx and rx...

thanks
-- PMM

Re: [PATCH-for-9.0 v2 02/11] hw/net/lan9118: Fix overflow in MIL TX FIFO

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:38, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> When the MAC Interface Layer (MIL) transmit FIFO is full,
> truncate the packet, and raise the Transmitter Error (TXE)
> flag.
>
> Broken since model introduction in commit 2a42499017
> ("LAN9118 emulation").
>
> When using the reproducer from
> https://gitlab.com/qemu-project/qemu/-/issues/2267 we get:
>
>   hw/net/lan9118.c:798:17: runtime error:
>   index 2048 out of bounds for type 'uint8_t[2048]' (aka 'unsigned char[2048]')
>     #0 0x563ec9a057b1 in tx_fifo_push hw/net/lan9118.c:798:43
>     #1 0x563ec99fbb28 in lan9118_writel hw/net/lan9118.c:1042:9
>     #2 0x563ec99f2de2 in lan9118_16bit_mode_write hw/net/lan9118.c:1205:9
>     #3 0x563ecbf78013 in memory_region_write_accessor system/memory.c:497:5
>     #4 0x563ecbf776f5 in access_with_adjusted_size system/memory.c:573:18
>     #5 0x563ecbf75643 in memory_region_dispatch_write system/memory.c:1521:16
>     #6 0x563ecc01bade in flatview_write_continue_step system/physmem.c:2713:18
>     #7 0x563ecc01b374 in flatview_write_continue system/physmem.c:2743:19
>     #8 0x563ecbff1c9b in flatview_write system/physmem.c:2774:12
>     #9 0x563ecbff1768 in address_space_write system/physmem.c:2894:18
>     ...
>
> [*] LAN9118 DS00002266B.pdf, Table 5.3.3 "INTERRUPT STATUS REGISTER"
>
> Reported-by: Will Lester
> Reported-by: Chuhong Yuan <hslester96@gmail.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2267
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

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

thanks
-- PMM

Re: [PATCH-for-9.1 v2 03/11] hw/net/lan9118: Remove duplicated assignment

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> s->txp->fifo_used is zeroed in the next 3 lines.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/net/lan9118.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index 91d81b410b..d6f0e37eb1 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -438,7 +438,6 @@ static void lan9118_reset(DeviceState *d)
>      s->hw_cfg = s->mode_16bit ? 0x00050000 : 0x00050004;
>      s->pmt_ctrl &= 0x45;
>      s->gpio_cfg = 0;
> -    s->txp->fifo_used = 0;
>      s->txp->state = TX_IDLE;
>      s->txp->cmd_a = 0xffffffffu;
>      s->txp->cmd_b = 0xffffffffu;

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

thanks
-- PMM

Re: [PATCH-for-9.1 v2 04/11] hw/net/lan9118: Replace magic '5' value by TX_FIF_SZ_RESET definition

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> TX_FIF_SZ is described in chapter 5.3.9,
> "HW_CFG — HARDWARE CONFIGURATION REGISTER".
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

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

thanks
-- PMM

Re: [PATCH-for-9.1 v2 05/11] hw/net/lan9118: Add definitions for FIFO allocated sizes

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Add definitions for the TX_FIF_SZ=5 case, per TABLE 5-3
> "VALID TX/RX FIFO ALLOCATIONS".
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/net/lan9118.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index a6a869de32..00409927fe 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -158,6 +158,17 @@ do { printf("lan9118: " fmt , ## __VA_ARGS__); } while (0)
>   */
>  #define MIL_TXFIFO_SIZE         2048
>
> +/*
> + * TX and RX FIFO space is configurable through the TX FIFO Size (TX_FIF_SZ)
> + * field in the hardware configuration (CSR HW_CFG) register. These are the
> + * default configuration settings for TX_FIF_SZ = 5
> + * (see TABLE 5-3: VALID TX/RX FIFO ALLOCATIONS).
> + */
> +#define TX_DATA_FIFO_BYTES      4608    /* 1152 words */
> +#define TX_STATUS_FIFO_BYTES    512     /* 128 words */
> +#define RX_DATA_FIFO_BYTES      10560   /* 2640 words */
> +#define RX_STATUS_FIFO_BYTES    704     /* 176 words */

We could make these do the actual calculations, rather
than hardcoding the results:

#define TX_STATUS_FIFO_BYTES 512
#define TX_TOTAL_FIFO_BYTES(TX_FIF_SZ) (1024 * (TX_FIF_SZ))
#define RX_TOTAL_FIFO_BYTES(TX_FIF_SZ) (16384 - TX_TOTAL_FIFO_BYTES(TX_FIF_SZ))
#define TX_DATA_FIFO_BYTES(TX_FIF_SZ) (TX_TOTAL_FIFO_BYTES(TX_FIF_SZ)
- TX_STATUS_FIFO_BYTES)
#define RX_STATUS_FIFO_BYTES (RX_TOTAL_FIFO_BYTES(TX_FIF_SZ) >> 4)
#define RX_DATA_FIFO_BYTES(TX_FIF_SZ) \
    (RX_TOTAL_FIFO_BYTES(TX_FIF_SZ) - RX_STATUS_FIFO_BYTES(TX_FIF_SZ))

thanks
-- PMM

Re: [PATCH-for-9.1 v2 07/11] hw/net/lan9118: Rename tx_fifo_size -> tx_fifo_bytes

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> tx_fifo_size is a byte count, rename it to avoid confusion.

But we don't consistently use it as a byte count.
In tx_fifo_push():

    if (s->txp->fifo_used == s->tx_fifo_size) {
        s->int_sts |= TDFO_INT;
        return;
    }

where fifo_used is incremented for every word handled by the
tx_fifo, not for every byte.

thanks
-- PMM

Re: [PATCH-for-9.1 v2 08/11] hw/net/lan9118: Use RX_STATUS_FIFO_BYTES definition

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> rx_status_fifo[] is an array of words,
> rx_status_fifo_size is a word count.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/net/lan9118.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index a983ce193b..cace22381d 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -262,7 +262,7 @@ struct lan9118_state {
>      int32_t rx_status_fifo_size;
>      int32_t rx_status_fifo_used;
>      int32_t rx_status_fifo_head;
> -    uint32_t rx_status_fifo[896];
> +    uint32_t rx_status_fifo[RX_STATUS_FIFO_BYTES / 4];
>      int32_t rx_fifo_size;
>      int32_t rx_fifo_used;
>      int32_t rx_fifo_head;
> @@ -332,7 +332,9 @@ static const VMStateDescription vmstate_lan9118 = {
>          VMSTATE_INT32(rx_status_fifo_size, lan9118_state),
>          VMSTATE_INT32(rx_status_fifo_used, lan9118_state),
>          VMSTATE_INT32(rx_status_fifo_head, lan9118_state),
> -        VMSTATE_UINT32_ARRAY(rx_status_fifo, lan9118_state, 896),
> +        VMSTATE_UINT32_ARRAY(rx_status_fifo, lan9118_state,
> +                             RX_STATUS_FIFO_BYTES / 4),
> +        VMSTATE_UNUSED(896 * 4 - RX_STATUS_FIFO_BYTES),
>          VMSTATE_INT32(rx_fifo_size, lan9118_state),
>          VMSTATE_INT32(rx_fifo_used, lan9118_state),
>          VMSTATE_INT32(rx_fifo_head, lan9118_state),


Ideally in the state struct we should have the arrays be the
size of the largest possible FIFO, not the size of the
default-out-of-reset FIFO, to leave the way open for making the FIFO
size be runtime configurable in future if we want that.

thanks
-- PMM

Re: [PATCH-for-9.1 v2 09/11] hw/net/lan9118: Rename rx_status_fifo_size -> rx_status_fifo_wordcount

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:38, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> rx_status_fifo_size is a word count, rename it to avoid confusion.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/net/lan9118.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index cace22381d..663776f575 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -259,7 +259,7 @@ struct lan9118_state {
>      int32_t tx_status_fifo_head;
>      uint32_t tx_status_fifo[512];
>
> -    int32_t rx_status_fifo_size;
> +    int32_t rx_status_fifo_wordcount;
>      int32_t rx_status_fifo_used;
>      int32_t rx_status_fifo_head;

True, but rx_status_fifo_used and rx_status_fifo_head
are also word counts. Should we try to indicate units in
all these names, or is that getting unwieldy?

-- PMM

Re: [PATCH-for-9.1 v2 10/11] hw/net/lan9118: Use RX_DATA_FIFO_BYTES definition

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> rx_fifo[] is an array of words,
> rx_fifo_size is a word count.

True, but that's not why rx_fifo[] has been sized to 3360.
It's 3360 because that is the worst-case RX data FIFO size
in words (if TX_FIF_SZ is 2 then the RX data FIFO is 13440
bytes, which is 3360 words). So in this case the array size
is correct.

thanks
-- PMM

Re: [PATCH-for-9.0 v2 00/11] hw/net/lan9118: Fix overflow in TX FIFO

[Philippe Mathieu-Daudé]

On 9/4/24 15:37, Philippe Mathieu-Daudé wrote:
> Fix for https://gitlab.com/qemu-project/qemu/-/issues/2267
> 
> Since v1:
> - Renamed definition as MIL_TXFIFO_SIZE
> - Addressed Peter review comments in patches 1 & 2
>    (add comment, return TXE INT)
> - Trivial patches while digesting Peter's analysis [*]
> 
> More work expected during 9.1.
> 
> [*] https://lore.kernel.org/qemu-devel/CAFEAcA8vvURMn2FaDP9tXtP5eCMs6-XFOCR9ypo=WBH+6g5prw@mail.gmail.com/
> 
> Philippe Mathieu-Daudé (11):
>    hw/net/lan9118: Replace magic '2048' value by MIL_TXFIFO_SIZE
>      definition
>    hw/net/lan9118: Fix overflow in MIL TX FIFO
>    hw/net/lan9118: Remove duplicated assignment

Patches 1 & 2 queued.