[PATCH v3 02/14] dp8393x: Always use 32-bit accesses

[PATCH v3 02/14] dp8393x: Always use 32-bit accesses

[Finn Thain]

The DP83932 and DP83934 have 32 data lines. The datasheet says,

    Data Bus: These bidirectional lines are used to transfer data on the
    system bus. When the SONIC is a bus master, 16-bit data is transferred
    on D15-D0 and 32-bit data is transferred on D31-D0. When the SONIC is
    accessed as a slave, register data is driven onto lines D15-D0.
    D31-D16 are held TRI-STATE if SONIC is in 16-bit mode. If SONIC is in
    32-bit mode, they are driven, but invalid.

Always use 32-bit accesses both as bus master and bus slave.

Force the MSW to zero in bus master mode.

This gets the Linux 'jazzsonic' driver working, and avoids the need for
prior hacks to make the NetBSD 'sn' driver work.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 hw/net/dp8393x.c | 47 +++++++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 14901c1445..b2fd44bc2f 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -246,9 +246,19 @@ static void dp8393x_put(dp8393xState *s, int width, int offset,
                         uint16_t val)
 {
     if (s->big_endian) {
-        s->data[offset * width + width - 1] = cpu_to_be16(val);
+        if (width == 2) {
+            s->data[offset * 2] = 0;
+            s->data[offset * 2 + 1] = cpu_to_be16(val);
+        } else {
+            s->data[offset] = cpu_to_be16(val);
+        }
     } else {
-        s->data[offset * width] = cpu_to_le16(val);
+        if (width == 2) {
+            s->data[offset * 2] = cpu_to_le16(val);
+            s->data[offset * 2 + 1] = 0;
+        } else {
+            s->data[offset] = cpu_to_le16(val);
+        }
     }
 }
 
@@ -588,7 +598,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
 
     DPRINTF("read 0x%04x from reg %s\n", val, reg_names[reg]);
 
-    return val;
+    return s->big_endian ? val << 16 : val;
 }
 
 static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
@@ -596,13 +606,14 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
 {
     dp8393xState *s = opaque;
     int reg = addr >> s->it_shift;
+    uint32_t val = s->big_endian ? data >> 16 : data;
 
-    DPRINTF("write 0x%04x to reg %s\n", (uint16_t)data, reg_names[reg]);
+    DPRINTF("write 0x%04x to reg %s\n", (uint16_t)val, reg_names[reg]);
 
     switch (reg) {
         /* Command register */
         case SONIC_CR:
-            dp8393x_do_command(s, data);
+            dp8393x_do_command(s, val);
             break;
         /* Prevent write to read-only registers */
         case SONIC_CAP2:
@@ -615,36 +626,36 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
         /* Accept write to some registers only when in reset mode */
         case SONIC_DCR:
             if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-                s->regs[reg] = data & 0xbfff;
+                s->regs[reg] = val & 0xbfff;
             } else {
                 DPRINTF("writing to DCR invalid\n");
             }
             break;
         case SONIC_DCR2:
             if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-                s->regs[reg] = data & 0xf017;
+                s->regs[reg] = val & 0xf017;
             } else {
                 DPRINTF("writing to DCR2 invalid\n");
             }
             break;
         /* 12 lower bytes are Read Only */
         case SONIC_TCR:
-            s->regs[reg] = data & 0xf000;
+            s->regs[reg] = val & 0xf000;
             break;
         /* 9 lower bytes are Read Only */
         case SONIC_RCR:
-            s->regs[reg] = data & 0xffe0;
+            s->regs[reg] = val & 0xffe0;
             break;
         /* Ignore most significant bit */
         case SONIC_IMR:
-            s->regs[reg] = data & 0x7fff;
+            s->regs[reg] = val & 0x7fff;
             dp8393x_update_irq(s);
             break;
         /* Clear bits by writing 1 to them */
         case SONIC_ISR:
-            data &= s->regs[reg];
-            s->regs[reg] &= ~data;
-            if (data & SONIC_ISR_RBE) {
+            val &= s->regs[reg];
+            s->regs[reg] &= ~val;
+            if (val & SONIC_ISR_RBE) {
                 dp8393x_do_read_rra(s);
             }
             dp8393x_update_irq(s);
@@ -657,17 +668,17 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
         case SONIC_REA:
         case SONIC_RRP:
         case SONIC_RWP:
-            s->regs[reg] = data & 0xfffe;
+            s->regs[reg] = val & 0xfffe;
             break;
         /* Invert written value for some registers */
         case SONIC_CRCT:
         case SONIC_FAET:
         case SONIC_MPT:
-            s->regs[reg] = data ^ 0xffff;
+            s->regs[reg] = val ^ 0xffff;
             break;
         /* All other registers have no special contrainst */
         default:
-            s->regs[reg] = data;
+            s->regs[reg] = val;
     }
 
     if (reg == SONIC_WT0 || reg == SONIC_WT1) {
@@ -678,8 +689,8 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
 static const MemoryRegionOps dp8393x_ops = {
     .read = dp8393x_read,
     .write = dp8393x_write,
-    .impl.min_access_size = 2,
-    .impl.max_access_size = 2,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.24.1

[PATCH v3 05/14] dp8393x: Update LLFA and CRDA registers from rx descriptor

[Finn Thain]

Follow the algorithm given in the National Semiconductor DP83932C
datasheet in section 3.4.7:

    At the next reception, the SONIC re-reads the last RXpkt.link field,
    and updates its CRDA register to point to the next descriptor.

The chip is designed to allow the host to provide a new list of
descriptors in this way.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
Changed since v1:
 - Update CRDA register from LLFA register after EOL is cleared.
---
 hw/net/dp8393x.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index ece72cbf42..249be403af 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -784,12 +784,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
         address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
                          (uint8_t *)s->data, size, 0);
-        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
+        s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
+        if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
             /* Still EOL ; stop reception */
             return -1;
-        } else {
-            s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         }
+        /* Link has been updated by host */
+        s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
     }
 
     /* Save current position */
@@ -837,7 +838,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     address_space_rw(&s->as, dp8393x_crda(s),
         MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
 
-    /* Move to next descriptor */
+    /* Check link field */
     size = sizeof(uint16_t) * width;
     address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
         MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
@@ -852,6 +853,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         dp8393x_put(s, width, 0, 0);
         address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
                          (uint8_t *)s->data, size, 1);
+
+        /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
-- 
2.24.1

[PATCH v3 07/14] dp8393x: Implement packet size limit and RBAE interrupt

[Finn Thain]

Add a bounds check to prevent a large packet from causing a buffer
overflow. This is defensive programming -- I haven't actually tried
sending an oversized packet or a jumbo ethernet frame.

The SONIC handles packets that are too big for the buffer by raising
the RBAE interrupt and dropping them. Linux uses that interrupt to
count dropped packets.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
Changed since v1:
 - Perform length check after Recieve Control Register initialization.
---
 hw/net/dp8393x.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 2e976781e2..0309365fda 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -137,6 +137,7 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
 #define SONIC_TCR_CRCI   0x2000
 #define SONIC_TCR_PINT   0x8000
 
+#define SONIC_ISR_RBAE   0x0010
 #define SONIC_ISR_RBE    0x0020
 #define SONIC_ISR_RDE    0x0040
 #define SONIC_ISR_TC     0x0080
@@ -770,6 +771,14 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
         SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
 
+    if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
+        DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
+        s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
+        dp8393x_update_irq(s);
+        dp8393x_do_read_rra(s);
+        return pkt_size;
+    }
+
     packet_type = dp8393x_receive_filter(s, buf, pkt_size);
     if (packet_type < 0) {
         DPRINTF("packet not for netcard\n");
-- 
2.24.1

[PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register

[Finn Thain]

The jazzsonic driver in Linux uses the Silicon Revision register value
to probe the chip. The driver fails unless the SR register contains 4.
Unfortunately, reading this register in QEMU usually returns 0 because
the s->regs[] array gets wiped after a software reset.

Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 hw/net/dp8393x.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 1b73a8703b..71af0fad51 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
                 val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 - reg)];
             }
             break;
+        /* Read-only */
+        case SONIC_SR:
+            val = 4; /* only revision recognized by Linux/mips */
+            break;
         /* All other registers have no special contrainst */
         default:
             val = s->regs[reg];
@@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
 
     s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
-    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
 
     memory_region_init_ram(&s->prom, OBJECT(dev),
                            "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
-- 
2.24.1

[PATCH v3 09/14] dp8393x: Use long-word-aligned RRA pointers in 32-bit mode

[Finn Thain]

Section 3.4.1 of the datasheet says,

    The alignment of the RRA is confined to either word or long word
    boundaries, depending upon the data width mode. In 16-bit mode,
    the RRA must be aligned to a word boundary (A0 is always zero)
    and in 32-bit mode, the RRA is aligned to a long word boundary
    (A0 and A1 are always zero).

This constraint has been implemented for 16-bit mode; implement it
for 32-bit mode too.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 947ceef37c..b052e2c854 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -663,12 +663,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
                 qemu_flush_queued_packets(qemu_get_queue(s->nic));
             }
             break;
-        /* Ignore least significant bit */
+        /* The guest is required to store aligned pointers here */
         case SONIC_RSA:
         case SONIC_REA:
         case SONIC_RRP:
         case SONIC_RWP:
-            s->regs[reg] = val & 0xfffe;
+            if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+                s->regs[reg] = val & 0xfffc;
+            } else {
+                s->regs[reg] = val & 0xfffe;
+            }
             break;
         /* Invert written value for some registers */
         case SONIC_CRCT:
-- 
2.24.1

[PATCH v3 11/14] dp8393x: Clear descriptor in_use field to release packet

[Finn Thain]

When the SONIC receives a packet into the last available descriptor, it
retains ownership of that descriptor for as long as necessary.

Section 3.4.7 of the datasheet says,

    When the system appends more descriptors, the SONIC releases ownership
    of the descriptor after writing 0000h to the RXpkt.in_use field.

The packet can now be processed by the host, so raise a PKTRX interrupt,
just like the normal case.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
Changed since v2:
 - Assert PKTRX interrupt when releasing withheld packet.
---
 hw/net/dp8393x.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 13513986f0..99c5dad7c4 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -809,7 +809,17 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
             return -1;
         }
         /* Link has been updated by host */
+
+        /* Clear in_use */
+        size = sizeof(uint16_t) * width;
+        address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
+        dp8393x_put(s, width, 0, 0);
+        address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+                         (uint8_t *)s->data, size, 1);
+
+        /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
+        s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
     }
 
     /* Save current position */
-- 
2.24.1

[PATCH v3 00/14] Fixes for DP8393X SONIC device emulation

[Finn Thain]

Hi All,

There are bugs in the emulated dp8393x device that can stop packet
reception in a Linux/m68k guest (q800 machine).

With a Linux/mips guest (magnum machine), the driver fails to probe
the dp8393x device.

With a NetBSD/arc 5.1 guest (magnum machine), the bugs in the device
can be fatal to the guest kernel.

Whilst debugging the device, I found that the receiver algorithm
differs from the one described in the National Semiconductor
datasheet.

This patch series resolves these bugs.

Note that the mainline Linux sonic driver also has bugs.
I have fixed the bugs that I know of in a series of patches at,
https://github.com/fthain/linux/commits/mac68k
---
Changed since v1:
 - Minor revisions as described in commit logs.
 - Dropped patches 4/10 and 7/10.
 - Added 5 new patches.

Changed since v2:
 - Minor revisions as described in commit logs.
 - Dropped patch 13/13.
 - Added 2 new patches.


Finn Thain (14):
  dp8393x: Mask EOL bit from descriptor addresses
  dp8393x: Always use 32-bit accesses
  dp8393x: Clean up endianness hacks
  dp8393x: Have dp8393x_receive() return the packet size
  dp8393x: Update LLFA and CRDA registers from rx descriptor
  dp8393x: Clear RRRA command register bit only when appropriate
  dp8393x: Implement packet size limit and RBAE interrupt
  dp8393x: Don't clobber packet checksum
  dp8393x: Use long-word-aligned RRA pointers in 32-bit mode
  dp8393x: Pad frames to word or long word boundary
  dp8393x: Clear descriptor in_use field to release packet
  dp8393x: Always update RRA pointers and sequence numbers
  dp8393x: Don't reset Silicon Revision register
  dp8393x: Don't stop reception upon RBE interrupt assertion

 hw/net/dp8393x.c | 205 +++++++++++++++++++++++++++++++----------------
 1 file changed, 137 insertions(+), 68 deletions(-)

-- 
2.24.1

[PATCH v3 06/14] dp8393x: Clear RRRA command register bit only when appropriate

[Finn Thain]

It doesn't make sense to clear the command register bit unless the
command was actually issued.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 249be403af..2e976781e2 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -352,9 +352,6 @@ static void dp8393x_do_read_rra(dp8393xState *s)
         s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
         dp8393x_update_irq(s);
     }
-
-    /* Done */
-    s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
 }
 
 static void dp8393x_do_software_reset(dp8393xState *s)
@@ -563,8 +560,10 @@ static void dp8393x_do_command(dp8393xState *s, uint16_t command)
         dp8393x_do_start_timer(s);
     if (command & SONIC_CR_RST)
         dp8393x_do_software_reset(s);
-    if (command & SONIC_CR_RRRA)
+    if (command & SONIC_CR_RRRA) {
         dp8393x_do_read_rra(s);
+        s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
+    }
     if (command & SONIC_CR_LCAM)
         dp8393x_do_load_cam(s);
 }
-- 
2.24.1

[PATCH v3 04/14] dp8393x: Have dp8393x_receive() return the packet size

[Finn Thain]

This function re-uses its 'size' argument as a scratch variable.
Instead, declare a local 'size' variable for that purpose so that the
function result doesn't get messed up.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 2d2ace2549..ece72cbf42 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -757,20 +757,21 @@ static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf,
 }
 
 static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
-                               size_t size)
+                               size_t pkt_size)
 {
     dp8393xState *s = qemu_get_nic_opaque(nc);
     int packet_type;
     uint32_t available, address;
-    int width, rx_len = size;
+    int width, rx_len = pkt_size;
     uint32_t checksum;
+    int size;
 
     width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
 
     s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
         SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
 
-    packet_type = dp8393x_receive_filter(s, buf, size);
+    packet_type = dp8393x_receive_filter(s, buf, pkt_size);
     if (packet_type < 0) {
         DPRINTF("packet not for netcard\n");
         return -1;
@@ -864,7 +865,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     /* Done */
     dp8393x_update_irq(s);
 
-    return size;
+    return pkt_size;
 }
 
 static void dp8393x_reset(DeviceState *dev)
-- 
2.24.1

[PATCH v3 10/14] dp8393x: Pad frames to word or long word boundary

[Finn Thain]

The existing code has a bug where the Remaining Buffer Word Count (RBWC)
is calculated with a truncating division, which gives the wrong result
for odd-sized packets.

Section 1.4.1 of the datasheet says,

    Once the end of the packet has been reached, the serializer will
    fill out the last word (16-bit mode) or long word (32-bit mode)
    if the last byte did not end on a word or long word boundary
    respectively. The fill byte will be 0FFh.

Implement buffer padding so that buffer limits are correctly enforced.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b052e2c854..13513986f0 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -766,16 +766,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     dp8393xState *s = qemu_get_nic_opaque(nc);
     int packet_type;
     uint32_t available, address;
-    int width, rx_len = pkt_size;
+    int width, rx_len, padded_len;
     uint32_t checksum;
     int size;
 
-    width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
-
     s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
         SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
 
-    if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
+    rx_len = pkt_size + sizeof(checksum);
+    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+        width = 2;
+        padded_len = ((rx_len - 1) | 3) + 1;
+    } else {
+        width = 1;
+        padded_len = ((rx_len - 1) | 1) + 1;
+    }
+
+    if (padded_len > dp8393x_rbwc(s) * 2) {
         DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
         s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
         dp8393x_update_irq(s);
@@ -810,22 +817,32 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
 
     /* Calculate the ethernet checksum */
-    checksum = cpu_to_le32(crc32(0, buf, rx_len));
+    checksum = cpu_to_le32(crc32(0, buf, pkt_size));
 
     /* Put packet into RBA */
     DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
     address = dp8393x_crba(s);
     address_space_rw(&s->as, address,
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len, 1);
-    address += rx_len;
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, pkt_size, 1);
+    address += pkt_size;
+
+    /* Put frame checksum into RBA */
     address_space_rw(&s->as, address,
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
-    address += 4;
-    rx_len += 4;
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, sizeof(checksum), 1);
+    address += sizeof(checksum);
+
+    /* Pad short packets to keep pointers aligned */
+    if (rx_len < padded_len) {
+        size = padded_len - rx_len;
+        address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+            (uint8_t *)"\xFF\xFF\xFF", size, 1);
+        address += size;
+    }
+
     s->regs[SONIC_CRBA1] = address >> 16;
     s->regs[SONIC_CRBA0] = address & 0xffff;
     available = dp8393x_rbwc(s);
-    available -= rx_len / 2;
+    available -= padded_len >> 1;
     s->regs[SONIC_RBWC1] = available >> 16;
     s->regs[SONIC_RBWC0] = available & 0xffff;
 
-- 
2.24.1

[PATCH v3 08/14] dp8393x: Don't clobber packet checksum

[Finn Thain]

A received packet consumes pkt_size bytes in the buffer and the frame
checksum that's appended to it consumes another 4 bytes. The Receive
Buffer Address register takes the former quantity into account but
not the latter. So the next packet written to the buffer overwrites
the frame checksum. Fix this.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 0309365fda..947ceef37c 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -816,6 +816,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     address += rx_len;
     address_space_rw(&s->as, address,
         MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
+    address += 4;
     rx_len += 4;
     s->regs[SONIC_CRBA1] = address >> 16;
     s->regs[SONIC_CRBA0] = address & 0xffff;
-- 
2.24.1

[PATCH v3 03/14] dp8393x: Clean up endianness hacks

[Finn Thain]

According to the datasheet, section 3.4.4, "in 32-bit mode ... the SONIC
always writes long words".

Therefore, use the same technique for the 'in_use' field that is used
everywhere else, and write the full long word.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
Changed since v1:
 - Use existing 'address' variable rather than declare a new one.

Laurent tells me that a similar clean-up has been tried before.
He referred me to commit c744cf7879 ("dp8393x: fix dp8393x_receive()")
and commit 409b52bfe1 ("net/dp8393x: correctly reset in_use field").
I believe the underlying issue has been fixed by the preceding patch,
as this no longer breaks NetBSD 5.1.
---
 hw/net/dp8393x.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b2fd44bc2f..2d2ace2549 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -776,8 +776,6 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         return -1;
     }
 
-    /* XXX: Check byte ordering */
-
     /* Check for EOL */
     if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* Are we still in resource exhaustion? */
@@ -847,15 +845,12 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
-        /* Clear in_use, but it is always 16bit wide */
-        int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
-        if (s->big_endian && width == 2) {
-            /* we need to adjust the offset of the 16bit field */
-            offset += sizeof(uint16_t);
-        }
-        s->data[0] = 0;
-        address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
-                         (uint8_t *)s->data, sizeof(uint16_t), 1);
+        /* Clear in_use */
+        size = sizeof(uint16_t) * width;
+        address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
+        dp8393x_put(s, width, 0, 0);
+        address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+                         (uint8_t *)s->data, size, 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
-- 
2.24.1

[PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses

[Finn Thain]

The Least Significant bit of a descriptor address register is used as
an EOL flag. It has to be masked when the register value is to be used
as an actual address for copying memory around. But when the registers
are to be updated the EOL bit should not be masked.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
Changed since v1:
 - Added macros to name constants as requested by Philippe Mathieu-Daudé.
---
 hw/net/dp8393x.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cdc2631c0c..14901c1445 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
 #define SONIC_ISR_PINT   0x0800
 #define SONIC_ISR_LCD    0x1000
 
+#define SONIC_DESC_EOL   0x0001
+#define SONIC_DESC_ADDR  0xFFFE
+
 #define TYPE_DP8393X "dp8393x"
 #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
 
@@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
 
 static uint32_t dp8393x_crda(dp8393xState *s)
 {
-    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
+    return (s->regs[SONIC_URDA] << 16) |
+           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
 }
 
 static uint32_t dp8393x_rbwc(dp8393xState *s)
@@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
 
 static uint32_t dp8393x_ttda(dp8393xState *s)
 {
-    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
+    return (s->regs[SONIC_UTDA] << 16) |
+           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
 }
 
 static uint32_t dp8393x_wt(dp8393xState *s)
@@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
                              sizeof(uint16_t) *
                              (4 + 3 * s->regs[SONIC_TFC]) * width,
                 MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
-            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-            if (dp8393x_get(s, width, 0) & 0x1) {
+            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
                 /* EOL detected */
                 break;
             }
@@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     /* XXX: Check byte ordering */
 
     /* Check for EOL */
-    if (s->regs[SONIC_LLFA] & 0x1) {
+    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* Are we still in resource exhaustion? */
         size = sizeof(uint16_t) * 1 * width;
         address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
         address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
                          (uint8_t *)s->data, size, 0);
-        if (dp8393x_get(s, width, 0) & 0x1) {
+        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
             /* Still EOL ; stop reception */
             return -1;
         } else {
@@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
         MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
     s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
-    if (s->regs[SONIC_LLFA] & 0x1) {
+    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
-- 
2.24.1

[PATCH v3 12/14] dp8393x: Always update RRA pointers and sequence numbers

[Finn Thain]

These operations have to take place regardless of whether or not rx
descriptors have been used up (that is, EOL flag was observed).

The algorithm is the now the same for a packet that was withheld as
for a packet that was not.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 99c5dad7c4..1b73a8703b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -897,12 +897,14 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
-        s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
+    }
 
-        if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
-            /* Read next RRA */
-            dp8393x_do_read_rra(s);
-        }
+    s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
+                         ((s->regs[SONIC_RSC] + 1) & 0x00ff);
+
+    if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
+        /* Read next RRA */
+        dp8393x_do_read_rra(s);
     }
 
     /* Done */
-- 
2.24.1

[PATCH v3 14/14] dp8393x: Don't stop reception upon RBE interrupt assertion

[Finn Thain]

Section 3.4.7 of the datasheet explains that,

    The RBE bit in the Interrupt Status register is set when the
    SONIC finishes using the second to last receive buffer and reads
    the last RRA descriptor. Actually, the SONIC is not truly out of
    resources, but gives the system an early warning of an impending
    out of resources condition.

RBE does not mean actual receive buffer exhaustion, and reception should
not be stopped. This is important because Linux will not check and clear
the RBE interrupt until it receives another packet. But that won't
happen if can_receive returns false. This bug causes the SONIC to become
deaf (until reset).

Fix this with a new flag to indicate actual receive buffer exhaustion.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
Changed since v2:
 - Don't use can_receive to suspend packet reception.
 - Don't misuse the RBE interrupt flag as a proxy for RRP == RWP.
---
 hw/net/dp8393x.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 71af0fad51..1ccee0c189 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -158,6 +158,7 @@ typedef struct dp8393xState {
     /* Hardware */
     uint8_t it_shift;
     bool big_endian;
+    bool last_rba_is_full;
     qemu_irq irq;
 #ifdef DEBUG_SONIC
     int irq_level;
@@ -347,12 +348,15 @@ static void dp8393x_do_read_rra(dp8393xState *s)
         s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
     }
 
-    /* Check resource exhaustion */
+    /* Warn the host if CRBA now has the last available resource */
     if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
     {
         s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
         dp8393x_update_irq(s);
     }
+
+    /* Allow packet reception */
+    s->last_rba_is_full = false;
 }
 
 static void dp8393x_do_software_reset(dp8393xState *s)
@@ -663,9 +667,6 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
                 dp8393x_do_read_rra(s);
             }
             dp8393x_update_irq(s);
-            if (dp8393x_can_receive(s->nic->ncs)) {
-                qemu_flush_queued_packets(qemu_get_queue(s->nic));
-            }
             break;
         /* The guest is required to store aligned pointers here */
         case SONIC_RSA:
@@ -725,8 +726,6 @@ static int dp8393x_can_receive(NetClientState *nc)
 
     if (!(s->regs[SONIC_CR] & SONIC_CR_RXEN))
         return 0;
-    if (s->regs[SONIC_ISR] & SONIC_ISR_RBE)
-        return 0;
     return 1;
 }
 
@@ -777,6 +776,10 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
         SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
 
+    if (s->last_rba_is_full) {
+        return pkt_size;
+    }
+
     rx_len = pkt_size + sizeof(checksum);
     if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
         width = 2;
@@ -790,8 +793,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
         s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
         dp8393x_update_irq(s);
-        dp8393x_do_read_rra(s);
-        return pkt_size;
+        s->regs[SONIC_RCR] |= SONIC_RCR_LPKT;
+        goto done;
     }
 
     packet_type = dp8393x_receive_filter(s, buf, pkt_size);
@@ -903,17 +906,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
     }
 
+    dp8393x_update_irq(s);
+
     s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
                          ((s->regs[SONIC_RSC] + 1) & 0x00ff);
 
+done:
+
     if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
-        /* Read next RRA */
-        dp8393x_do_read_rra(s);
+        if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
+            /* Stop packet reception */
+            s->last_rba_is_full = true;
+        } else {
+            /* Read next resource */
+            dp8393x_do_read_rra(s);
+        }
     }
 
-    /* Done */
-    dp8393x_update_irq(s);
-
     return pkt_size;
 }
 
-- 
2.24.1

Re: [PATCH v3 00/14] Fixes for DP8393X SONIC device emulation

[Laurent Vivier]

Le 19/01/2020 à 23:59, Finn Thain a écrit :
> Hi All,
> 
> There are bugs in the emulated dp8393x device that can stop packet
> reception in a Linux/m68k guest (q800 machine).
> 
> With a Linux/mips guest (magnum machine), the driver fails to probe
> the dp8393x device.
> 
> With a NetBSD/arc 5.1 guest (magnum machine), the bugs in the device
> can be fatal to the guest kernel.
> 
> Whilst debugging the device, I found that the receiver algorithm
> differs from the one described in the National Semiconductor
> datasheet.
> 
> This patch series resolves these bugs.
> 
> Note that the mainline Linux sonic driver also has bugs.
> I have fixed the bugs that I know of in a series of patches at,
> https://github.com/fthain/linux/commits/mac68k
> ---
> Changed since v1:
>  - Minor revisions as described in commit logs.
>  - Dropped patches 4/10 and 7/10.
>  - Added 5 new patches.
> 
> Changed since v2:
>  - Minor revisions as described in commit logs.
>  - Dropped patch 13/13.
>  - Added 2 new patches.
> 
> 
> Finn Thain (14):
>   dp8393x: Mask EOL bit from descriptor addresses
>   dp8393x: Always use 32-bit accesses
>   dp8393x: Clean up endianness hacks
>   dp8393x: Have dp8393x_receive() return the packet size
>   dp8393x: Update LLFA and CRDA registers from rx descriptor
>   dp8393x: Clear RRRA command register bit only when appropriate
>   dp8393x: Implement packet size limit and RBAE interrupt
>   dp8393x: Don't clobber packet checksum
>   dp8393x: Use long-word-aligned RRA pointers in 32-bit mode
>   dp8393x: Pad frames to word or long word boundary
>   dp8393x: Clear descriptor in_use field to release packet
>   dp8393x: Always update RRA pointers and sequence numbers
>   dp8393x: Don't reset Silicon Revision register
>   dp8393x: Don't stop reception upon RBE interrupt assertion
> 
>  hw/net/dp8393x.c | 205 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 137 insertions(+), 68 deletions(-)
> 

For the series with q800 machine type and kernel 5.5.0:

Tested-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v3 00/14] Fixes for DP8393X SONIC device emulation

[Finn Thain]

On Mon, 20 Jan 2020, Finn Thain wrote:

> Hi All,
> 
> There are bugs in the emulated dp8393x device that can stop packet
> reception in a Linux/m68k guest (q800 machine).
> 
> With a Linux/mips guest (magnum machine), the driver fails to probe
> the dp8393x device.
> 
> With a NetBSD/arc 5.1 guest (magnum machine), the bugs in the device
> can be fatal to the guest kernel.
> 
> Whilst debugging the device, I found that the receiver algorithm
> differs from the one described in the National Semiconductor
> datasheet.
> 
> This patch series resolves these bugs.
> 

Ironically, given the recent driver fixes in the Linux kernel, the bugs in 
stock QEMU make it possible to remotely trigger a kernel Oops (much like 
the NetBSD crash). This patch series is needed to resolve those issues.

> Note that the mainline Linux sonic driver also has bugs.
> I have fixed the bugs that I know of in a series of patches at,
> https://github.com/fthain/linux/commits/mac68k
> ---
> Changed since v1:
>  - Minor revisions as described in commit logs.
>  - Dropped patches 4/10 and 7/10.
>  - Added 5 new patches.
> 
> Changed since v2:
>  - Minor revisions as described in commit logs.
>  - Dropped patch 13/13.
>  - Added 2 new patches.
> 
> 
> Finn Thain (14):
>   dp8393x: Mask EOL bit from descriptor addresses
>   dp8393x: Always use 32-bit accesses
>   dp8393x: Clean up endianness hacks
>   dp8393x: Have dp8393x_receive() return the packet size
>   dp8393x: Update LLFA and CRDA registers from rx descriptor
>   dp8393x: Clear RRRA command register bit only when appropriate
>   dp8393x: Implement packet size limit and RBAE interrupt
>   dp8393x: Don't clobber packet checksum
>   dp8393x: Use long-word-aligned RRA pointers in 32-bit mode
>   dp8393x: Pad frames to word or long word boundary
>   dp8393x: Clear descriptor in_use field to release packet
>   dp8393x: Always update RRA pointers and sequence numbers
>   dp8393x: Don't reset Silicon Revision register
>   dp8393x: Don't stop reception upon RBE interrupt assertion
> 
>  hw/net/dp8393x.c | 205 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 137 insertions(+), 68 deletions(-)
> 
> 

Re: [PATCH v3 10/14] dp8393x: Pad frames to word or long word boundary

[Philippe Mathieu-Daudé]

On 1/19/20 11:59 PM, Finn Thain wrote:
> The existing code has a bug where the Remaining Buffer Word Count (RBWC)
> is calculated with a truncating division, which gives the wrong result
> for odd-sized packets.
> 
> Section 1.4.1 of the datasheet says,
> 
>      Once the end of the packet has been reached, the serializer will
>      fill out the last word (16-bit mode) or long word (32-bit mode)
>      if the last byte did not end on a word or long word boundary
>      respectively. The fill byte will be 0FFh.
> 
> Implement buffer padding so that buffer limits are correctly enforced.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 39 ++++++++++++++++++++++++++++-----------
>   1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index b052e2c854..13513986f0 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -766,16 +766,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       dp8393xState *s = qemu_get_nic_opaque(nc);
>       int packet_type;
>       uint32_t available, address;
> -    int width, rx_len = pkt_size;
> +    int width, rx_len, padded_len;
>       uint32_t checksum;
>       int size;
>   
> -    width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
> -
>       s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
>           SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
>   
> -    if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
> +    rx_len = pkt_size + sizeof(checksum);
> +    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> +        width = 2;
> +        padded_len = ((rx_len - 1) | 3) + 1;
> +    } else {
> +        width = 1;
> +        padded_len = ((rx_len - 1) | 1) + 1;
> +    }
> +
> +    if (padded_len > dp8393x_rbwc(s) * 2) {
>           DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
>           s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
>           dp8393x_update_irq(s);
> @@ -810,22 +817,32 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
>   
>       /* Calculate the ethernet checksum */
> -    checksum = cpu_to_le32(crc32(0, buf, rx_len));
> +    checksum = cpu_to_le32(crc32(0, buf, pkt_size));
>   
>       /* Put packet into RBA */
>       DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
>       address = dp8393x_crba(s);
>       address_space_rw(&s->as, address,
> -        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len, 1);
> -    address += rx_len;
> +        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, pkt_size, 1);
> +    address += pkt_size;
> +
> +    /* Put frame checksum into RBA */
>       address_space_rw(&s->as, address,
> -        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
> -    address += 4;
> -    rx_len += 4;
> +        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, sizeof(checksum), 1);
> +    address += sizeof(checksum);
> +
> +    /* Pad short packets to keep pointers aligned */
> +    if (rx_len < padded_len) {
> +        size = padded_len - rx_len;
> +        address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> +            (uint8_t *)"\xFF\xFF\xFF", size, 1);
> +        address += size;
> +    }
> +
>       s->regs[SONIC_CRBA1] = address >> 16;
>       s->regs[SONIC_CRBA0] = address & 0xffff;
>       available = dp8393x_rbwc(s);
> -    available -= rx_len / 2;
> +    available -= padded_len >> 1;
>       s->regs[SONIC_RBWC1] = available >> 16;
>       s->regs[SONIC_RBWC0] = available & 0xffff;
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 03/14] dp8393x: Clean up endianness hacks

[Philippe Mathieu-Daudé]

On 1/19/20 11:59 PM, Finn Thain wrote:
> According to the datasheet, section 3.4.4, "in 32-bit mode ... the SONIC
> always writes long words".
> 
> Therefore, use the same technique for the 'in_use' field that is used
> everywhere else, and write the full long word.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> ---
> Changed since v1:
>   - Use existing 'address' variable rather than declare a new one.
> 
> Laurent tells me that a similar clean-up has been tried before.
> He referred me to commit c744cf7879 ("dp8393x: fix dp8393x_receive()")
> and commit 409b52bfe1 ("net/dp8393x: correctly reset in_use field").
> I believe the underlying issue has been fixed by the preceding patch,
> as this no longer breaks NetBSD 5.1.
> ---
>   hw/net/dp8393x.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index b2fd44bc2f..2d2ace2549 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -776,8 +776,6 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>           return -1;
>       }
>   
> -    /* XXX: Check byte ordering */
> -
>       /* Check for EOL */
>       if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>           /* Are we still in resource exhaustion? */
> @@ -847,15 +845,12 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>           /* EOL detected */
>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>       } else {
> -        /* Clear in_use, but it is always 16bit wide */
> -        int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> -        if (s->big_endian && width == 2) {
> -            /* we need to adjust the offset of the 16bit field */
> -            offset += sizeof(uint16_t);
> -        }
> -        s->data[0] = 0;
> -        address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> -                         (uint8_t *)s->data, sizeof(uint16_t), 1);
> +        /* Clear in_use */
> +        size = sizeof(uint16_t) * width;
> +        address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> +        dp8393x_put(s, width, 0, 0);
> +        address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> +                         (uint8_t *)s->data, size, 1);
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 04/14] dp8393x: Have dp8393x_receive() return the packet size

[Philippe Mathieu-Daudé]

On 1/19/20 11:59 PM, Finn Thain wrote:
> This function re-uses its 'size' argument as a scratch variable.
> Instead, declare a local 'size' variable for that purpose so that the
> function result doesn't get messed up.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 2d2ace2549..ece72cbf42 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -757,20 +757,21 @@ static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf,
>   }
>   
>   static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> -                               size_t size)
> +                               size_t pkt_size)
>   {
>       dp8393xState *s = qemu_get_nic_opaque(nc);
>       int packet_type;
>       uint32_t available, address;
> -    int width, rx_len = size;
> +    int width, rx_len = pkt_size;
>       uint32_t checksum;
> +    int size;
>   
>       width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
>   
>       s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
>           SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
>   
> -    packet_type = dp8393x_receive_filter(s, buf, size);
> +    packet_type = dp8393x_receive_filter(s, buf, pkt_size);
>       if (packet_type < 0) {
>           DPRINTF("packet not for netcard\n");
>           return -1;
> @@ -864,7 +865,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       /* Done */
>       dp8393x_update_irq(s);
>   
> -    return size;
> +    return pkt_size;
>   }
>   
>   static void dp8393x_reset(DeviceState *dev)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 04/14] dp8393x: Have dp8393x_receive() return the packet size

[Philippe Mathieu-Daudé]

On Tue, Jan 28, 2020 at 12:03 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
> On 1/19/20 11:59 PM, Finn Thain wrote:
> > This function re-uses its 'size' argument as a scratch variable.
> > Instead, declare a local 'size' variable for that purpose so that the
> > function result doesn't get messed up.
> >
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Tested-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> >   hw/net/dp8393x.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index 2d2ace2549..ece72cbf42 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -757,20 +757,21 @@ static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf,
> >   }
> >
> >   static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> > -                               size_t size)
> > +                               size_t pkt_size)
> >   {
> >       dp8393xState *s = qemu_get_nic_opaque(nc);
> >       int packet_type;
> >       uint32_t available, address;
> > -    int width, rx_len = size;
> > +    int width, rx_len = pkt_size;
> >       uint32_t checksum;
> > +    int size;
> >
> >       width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
> >
> >       s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
> >           SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
> >
> > -    packet_type = dp8393x_receive_filter(s, buf, size);
> > +    packet_type = dp8393x_receive_filter(s, buf, pkt_size);
> >       if (packet_type < 0) {
> >           DPRINTF("packet not for netcard\n");
> >           return -1;
> > @@ -864,7 +865,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> >       /* Done */
> >       dp8393x_update_irq(s);
> >
> > -    return size;
> > +    return pkt_size;
> >   }
> >
> >   static void dp8393x_reset(DeviceState *dev)
> >
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

One R-b tag is enough btw ;)

Re: [PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register

[Philippe Mathieu-Daudé]

On 1/19/20 11:59 PM, Finn Thain wrote:
> The jazzsonic driver in Linux uses the Silicon Revision register value
> to probe the chip. The driver fails unless the SR register contains 4.
> Unfortunately, reading this register in QEMU usually returns 0 because
> the s->regs[] array gets wiped after a software reset.
> 
> Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> ---
>   hw/net/dp8393x.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 1b73a8703b..71af0fad51 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
>                   val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 - reg)];
>               }
>               break;
> +        /* Read-only */
> +        case SONIC_SR:
> +            val = 4; /* only revision recognized by Linux/mips */
> +            break;
>           /* All other registers have no special contrainst */
>           default:
>               val = s->regs[reg];
> @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>   
>       s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>   
>       memory_region_init_ram(&s->prom, OBJECT(dev),
>                              "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> 

Please fix in dp8393x_reset() instead:

-- >8 --
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cdc2631c0c..65eb9c23a7 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
      timer_del(s->watchdog);

      memset(s->regs, 0, sizeof(s->regs));
+    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
      s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
      s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
      s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | 
SONIC_RCR_BRD | SONIC_RCR_RNT);
@@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error 
**errp)
      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);

      s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
-    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */

      memory_region_init_ram(&s->prom, OBJECT(dev),
                             "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
---

Re: [PATCH v3 09/14] dp8393x: Use long-word-aligned RRA pointers in 32-bit mode

[Philippe Mathieu-Daudé]

On 1/19/20 11:59 PM, Finn Thain wrote:
> Section 3.4.1 of the datasheet says,
> 
>      The alignment of the RRA is confined to either word or long word
>      boundaries, depending upon the data width mode. In 16-bit mode,
>      the RRA must be aligned to a word boundary (A0 is always zero)
>      and in 32-bit mode, the RRA is aligned to a long word boundary
>      (A0 and A1 are always zero).
> 
> This constraint has been implemented for 16-bit mode; implement it
> for 32-bit mode too.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 947ceef37c..b052e2c854 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -663,12 +663,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>                   qemu_flush_queued_packets(qemu_get_queue(s->nic));
>               }
>               break;
> -        /* Ignore least significant bit */
> +        /* The guest is required to store aligned pointers here */
>           case SONIC_RSA:
>           case SONIC_REA:
>           case SONIC_RRP:
>           case SONIC_RWP:
> -            s->regs[reg] = val & 0xfffe;
> +            if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> +                s->regs[reg] = val & 0xfffc;
> +            } else {
> +                s->regs[reg] = val & 0xfffe;
> +            }
>               break;
>           /* Invert written value for some registers */
>           case SONIC_CRCT:
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses

[Philippe Mathieu-Daudé]

Hi Finn,

On 1/19/20 11:59 PM, Finn Thain wrote:
> The Least Significant bit of a descriptor address register is used as
> an EOL flag. It has to be masked when the register value is to be used
> as an actual address for copying memory around. But when the registers
> are to be updated the EOL bit should not be masked.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> ---
> Changed since v1:
>   - Added macros to name constants as requested by Philippe Mathieu-Daudé.
> ---
>   hw/net/dp8393x.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cdc2631c0c..14901c1445 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
>   #define SONIC_ISR_PINT   0x0800
>   #define SONIC_ISR_LCD    0x1000
>   
> +#define SONIC_DESC_EOL   0x0001
> +#define SONIC_DESC_ADDR  0xFFFE

I'd rather not add SONIC_DESC_ADDR and use ~SONIC_DESC_EOL instead.

Please consider it if you respin the series.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +
>   #define TYPE_DP8393X "dp8393x"
>   #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
>   
> @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
>   
>   static uint32_t dp8393x_crda(dp8393xState *s)
>   {
> -    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> +    return (s->regs[SONIC_URDA] << 16) |
> +           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
>   }
>   
>   static uint32_t dp8393x_rbwc(dp8393xState *s)
> @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
>   
>   static uint32_t dp8393x_ttda(dp8393xState *s)
>   {
> -    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> +    return (s->regs[SONIC_UTDA] << 16) |
> +           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
>   }
>   
>   static uint32_t dp8393x_wt(dp8393xState *s)
> @@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>                                sizeof(uint16_t) *
>                                (4 + 3 * s->regs[SONIC_TFC]) * width,
>                   MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> -            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> -            if (dp8393x_get(s, width, 0) & 0x1) {
> +            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> +            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
>                   /* EOL detected */
>                   break;
>               }
> @@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       /* XXX: Check byte ordering */
>   
>       /* Check for EOL */
> -    if (s->regs[SONIC_LLFA] & 0x1) {
> +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>           /* Are we still in resource exhaustion? */
>           size = sizeof(uint16_t) * 1 * width;
>           address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
>           address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
>                            (uint8_t *)s->data, size, 0);
> -        if (dp8393x_get(s, width, 0) & 0x1) {
> +        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
>               /* Still EOL ; stop reception */
>               return -1;
>           } else {
> @@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
>           MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
>       s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> -    if (s->regs[SONIC_LLFA] & 0x1) {
> +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>           /* EOL detected */
>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>       } else {
> 

Re: [PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register

[Finn Thain]

On Tue, 28 Jan 2020, Philippe Mathieu-Daud? wrote:

> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The jazzsonic driver in Linux uses the Silicon Revision register value
> > to probe the chip. The driver fails unless the SR register contains 4.
> > Unfortunately, reading this register in QEMU usually returns 0 because
> > the s->regs[] array gets wiped after a software reset.
> > 
> > Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > ---
> >   hw/net/dp8393x.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index 1b73a8703b..71af0fad51 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
> > unsigned int size)
> >                   val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 -
> > reg)];
> >               }
> >               break;
> > +        /* Read-only */
> > +        case SONIC_SR:
> > +            val = 4; /* only revision recognized by Linux/mips */
> > +            break;
> >           /* All other registers have no special contrainst */
> >           default:
> >               val = s->regs[reg];
> > @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> > **errp)
> >       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> >         s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> > -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> >         memory_region_init_ram(&s->prom, OBJECT(dev),
> >                              "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> > 
> 
> Please fix in dp8393x_reset() instead:
> 
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cdc2631c0c..65eb9c23a7 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
>      timer_del(s->watchdog);
> 
>      memset(s->regs, 0, sizeof(s->regs));
> +    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>      s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
>      s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
>      s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
> SONIC_RCR_RNT);
> @@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> **errp)
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> 
>      s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> 
>      memory_region_init_ram(&s->prom, OBJECT(dev),
>                             "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> ---
> 

This would allow the host to change the value of the Silicon Revision 
register. However, the datasheet says,

    4.3.13 Silicon Revision Register
    This is a 16-bit read only register. It contains information on the 
    current revision of the SONIC. The value of the DP83932CVF revision 
    register is 6h.

I haven't actually tried storing a different value in this register on 
National Semiconductor hardware, but I'm willing to do that test if you 
wish.

Re: [PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 4623 bytes --]

On Tue, 28 Jan 2020, Philippe Mathieu-Daudé wrote:

> Hi Finn,
> 
> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The Least Significant bit of a descriptor address register is used as
> > an EOL flag. It has to be masked when the register value is to be used
> > as an actual address for copying memory around. But when the registers
> > are to be updated the EOL bit should not be masked.
> > 
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > Tested-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> > Changed since v1:
> >   - Added macros to name constants as requested by Philippe Mathieu-Daudé.
> > ---
> >   hw/net/dp8393x.c | 19 ++++++++++++-------
> >   1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index cdc2631c0c..14901c1445 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
> > __VA_ARGS__); } while (0)
> >   #define SONIC_ISR_PINT   0x0800
> >   #define SONIC_ISR_LCD    0x1000
> >   +#define SONIC_DESC_EOL   0x0001
> > +#define SONIC_DESC_ADDR  0xFFFE
> 
> I'd rather not add SONIC_DESC_ADDR and use ~SONIC_DESC_EOL instead.
> 
> Please consider it if you respin the series.
> 

I chose to use 0xFFFE instead of ~SONIC_DESC_EOL because the former 
correctly implies an unsigned short word, while the latter mask may 
suggest some need for sign extension or longer words.

I agree that ~SONIC_DESC_EOL is easily understood as "all the other bits". 
But the bits in SONIC_DESC_EOL will never change, since this value is 
dictated by the hardware.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Thanks for reviewing this series.

> > +
> >   #define TYPE_DP8393X "dp8393x"
> >   #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
> >   @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
> >     static uint32_t dp8393x_crda(dp8393xState *s)
> >   {
> > -    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> > +    return (s->regs[SONIC_URDA] << 16) |
> > +           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
> >   }
> >     static uint32_t dp8393x_rbwc(dp8393xState *s)
> > @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
> >     static uint32_t dp8393x_ttda(dp8393xState *s)
> >   {
> > -    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> > +    return (s->regs[SONIC_UTDA] << 16) |
> > +           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
> >   }
> >     static uint32_t dp8393x_wt(dp8393xState *s)
> > @@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> >                                sizeof(uint16_t) *
> >                                (4 + 3 * s->regs[SONIC_TFC]) * width,
> >                   MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> > -            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> > -            if (dp8393x_get(s, width, 0) & 0x1) {
> > +            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> > +            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
> >                   /* EOL detected */
> >                   break;
> >               }
> > @@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> > const uint8_t * buf,
> >       /* XXX: Check byte ordering */
> >         /* Check for EOL */
> > -    if (s->regs[SONIC_LLFA] & 0x1) {
> > +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> >           /* Are we still in resource exhaustion? */
> >           size = sizeof(uint16_t) * 1 * width;
> >           address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> >           address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> >                            (uint8_t *)s->data, size, 0);
> > -        if (dp8393x_get(s, width, 0) & 0x1) {
> > +        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
> >               /* Still EOL ; stop reception */
> >               return -1;
> >           } else {
> > @@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
> > uint8_t * buf,
> >       address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 *
> > width,
> >           MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> >       s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> > -    if (s->regs[SONIC_LLFA] & 0x1) {
> > +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> >           /* EOL detected */
> >           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
> >       } else {
> > 
> 
> 

Re: [PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register

[Philippe Mathieu-Daudé]

Hi Finn,

On 1/28/20 11:28 PM, Finn Thain wrote:
> On Tue, 28 Jan 2020, Philippe Mathieu-Daud? wrote:
>> On 1/19/20 11:59 PM, Finn Thain wrote:
>>> The jazzsonic driver in Linux uses the Silicon Revision register value
>>> to probe the chip. The driver fails unless the SR register contains 4.
>>> Unfortunately, reading this register in QEMU usually returns 0 because
>>> the s->regs[] array gets wiped after a software reset.
>>>
>>> Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
>>> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>>> ---
>>>    hw/net/dp8393x.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>> index 1b73a8703b..71af0fad51 100644
>>> --- a/hw/net/dp8393x.c
>>> +++ b/hw/net/dp8393x.c
>>> @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
>>> unsigned int size)
>>>                    val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 -
>>> reg)];
>>>                }
>>>                break;
>>> +        /* Read-only */
>>> +        case SONIC_SR:
>>> +            val = 4; /* only revision recognized by Linux/mips */
>>> +            break;
>>>            /* All other registers have no special contrainst */
>>>            default:
>>>                val = s->regs[reg];
>>> @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error
>>> **errp)
>>>        qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>>>          s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
>>> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>>>          memory_region_init_ram(&s->prom, OBJECT(dev),
>>>                               "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
>>>
>>
>> Please fix in dp8393x_reset() instead:
>>
>> -- >8 --
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index cdc2631c0c..65eb9c23a7 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
>>       timer_del(s->watchdog);
>>
>>       memset(s->regs, 0, sizeof(s->regs));
>> +    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>>       s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
>>       s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
>>       s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
>> SONIC_RCR_RNT);
>> @@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error
>> **errp)
>>       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>>
>>       s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
>> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>>
>>       memory_region_init_ram(&s->prom, OBJECT(dev),
>>                              "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
>> ---
>>
> 
> This would allow the host to change the value of the Silicon Revision
> register.
How the guest can modify it? We have:

589 static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
590                           unsigned int size)
591 {
592     dp8393xState *s = opaque;
593     int reg = addr >> s->it_shift;
594
...
597     switch (reg) {
...
602         /* Prevent write to read-only registers */
...
606         case SONIC_SR:
...
608             DPRINTF("writing to reg %d invalid\n", reg);
609             break;

> However, the datasheet says,
> 
>      4.3.13 Silicon Revision Register
>      This is a 16-bit read only register. It contains information on the
>      current revision of the SONIC. The value of the DP83932CVF revision
>      register is 6h.
> 
> I haven't actually tried storing a different value in this register on
> National Semiconductor hardware, but I'm willing to do that test if you
> wish.
> 

Re: [PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

On Wed, 29 Jan 2020, Philippe Mathieu-Daudé wrote:

> > 
> > This would allow the host to change the value of the Silicon Revision 
> > register.
> How the guest can modify it? We have:
> 
> 589 static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> 590                           unsigned int size)
> 591 {
> 592     dp8393xState *s = opaque;
> 593     int reg = addr >> s->it_shift;
> 594
> ...
> 597     switch (reg) {
> ...
> 602         /* Prevent write to read-only registers */
> ...
> 606         case SONIC_SR:
> ...
> 608             DPRINTF("writing to reg %d invalid\n", reg);
> 609             break;
> 

My mistake. I had completely overlooked that logic.

I'll revise this patch in accordance with your suggestion.