[PATCH v2 00/10] dp8393x: fixes for MacOS toolbox ROM

[PATCH v2 00/10] dp8393x: fixes for MacOS toolbox ROM

[Mark Cave-Ayland]

Here is the next set of patches from my attempts to boot MacOS under QEMU's
Q800 machine related to the Sonic network adapter.

Patches 1 and 2 sort out checkpatch and convert from DPRINTF macros to
trace-events.

The discussion for the v1 patchset concluded that the dp8393x device does
NOT have its own NVRAM (there is no mention of it on the datasheet) and so
patches 3 to 5 move the generation of the PROM to the q800 and jazz boards
separately to allow the formats to diverge.

Patch 6 adds an implementation of bitrev8 to bitops.h in preparation for
changing the q800 PROM storage format, whilst patch 7 updates the MAC address
storage and checksum for the q800 machine to match the format expected by the
MacOS toolbox ROM.

Patch 8 ensures that the CPU loads/stores are correctly converted to 16-bit
accesses for the network card and patch 9 fixes a bug when selecting the
index specified for CAM entries.

Finally since the MIPS magnum machine exists for both big-endian (mips64) and
little-endian (mips64el) configurations, patch 10 sets the dp8393x big_endian
property accordingly using a similar technique already used for the MIPS malta
machines.

Migration notes: the changes to the dp8393x PROM are a migration break, but we
don't care about this for now since a) the q800 machine will have more
breaking migration changes as further MacOS toolbox ROM support is upstreamed
and b) the magnum machine migration is currently broken (and has been for
quite some time).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


v2:
- Move PROM generation from dp8393x to q800 and magnum machines and remove
  the existing code from the device itself
- Add bitrev8 implementation to bitops.h so it can be used elsewhere in
  future. Use a shift/merge technique rather than a massive table lookup
  as we don't care about speed
- Add patch to set the big_endian property correctly depending upon whether
  a big-endian or little-endian configuration is being used


Mark Cave-Ayland (10):
  dp8393x: checkpatch fixes
  dp8393x: convert to trace-events
  hw/mips/jazz: move PROM and checksum calculation from dp8393x device
    to board
  hw/m68k/q800: move PROM and checksum calculation from dp8393x device
    to board
  dp8393x: remove onboard PROM containing MAC address and checksum
  qemu/bitops.h: add bitrev8 implementation
  hw/m68k/q800: fix PROM checksum and MAC address storage
  dp8393x: don't force 32-bit register access
  dp8393x: fix CAM descriptor entry index
  hw/mips/jazz: specify correct endian for dp8393x device

 hw/m68k/q800.c        |  21 ++-
 hw/mips/jazz.c        |  32 ++++-
 hw/net/dp8393x.c      | 313 +++++++++++++++++++-----------------------
 hw/net/trace-events   |  17 +++
 include/qemu/bitops.h |  22 +++
 5 files changed, 231 insertions(+), 174 deletions(-)

-- 
2.20.1

[PATCH v2 01/10] dp8393x: checkpatch fixes

[Mark Cave-Ayland]

Also fix a simple comment typo of "constrainst" to "constraints".

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/dp8393x.c | 231 +++++++++++++++++++++++++----------------------
 1 file changed, 122 insertions(+), 109 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 533a8304d0..56af08f0fe 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -29,14 +29,14 @@
 #include <zlib.h>
 #include "qom/object.h"
 
-//#define DEBUG_SONIC
+/* #define DEBUG_SONIC */
 
 #define SONIC_PROM_SIZE 0x1000
 
 #ifdef DEBUG_SONIC
 #define DPRINTF(fmt, ...) \
 do { printf("sonic: " fmt , ##  __VA_ARGS__); } while (0)
-static const char* reg_names[] = {
+static const char *reg_names[] = {
     "CR", "DCR", "RCR", "TCR", "IMR", "ISR", "UTDA", "CTDA",
     "TPS", "TFC", "TSA0", "TSA1", "TFS", "URDA", "CRDA", "CRBA0",
     "CRBA1", "RBWC0", "RBWC1", "EOBC", "URRA", "RSA", "REA", "RRP",
@@ -185,7 +185,8 @@ struct dp8393xState {
     AddressSpace as;
 };
 
-/* Accessor functions for values which are formed by
+/*
+ * Accessor functions for values which are formed by
  * concatenating two 16 bit device registers. By putting these
  * in their own functions with a uint32_t return type we avoid the
  * pitfall of implicit sign extension where ((x << 16) | y) is a
@@ -350,8 +351,7 @@ static void dp8393x_do_read_rra(dp8393xState *s)
     }
 
     /* Warn the host if CRBA now has the last available resource */
-    if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
-    {
+    if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
         s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
         dp8393x_update_irq(s);
     }
@@ -364,7 +364,8 @@ static void dp8393x_do_software_reset(dp8393xState *s)
 {
     timer_del(s->watchdog);
 
-    s->regs[SONIC_CR] &= ~(SONIC_CR_LCAM | SONIC_CR_RRRA | SONIC_CR_TXP | SONIC_CR_HTX);
+    s->regs[SONIC_CR] &= ~(SONIC_CR_LCAM | SONIC_CR_RRRA | SONIC_CR_TXP |
+                           SONIC_CR_HTX);
     s->regs[SONIC_CR] |= SONIC_CR_RST | SONIC_CR_RXDIS;
 }
 
@@ -490,8 +491,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
 
         /* Handle Ethernet checksum */
         if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
-            /* Don't append FCS there, to look like slirp packets
-             * which don't have one */
+            /*
+             * Don't append FCS there, to look like slirp packets
+             * which don't have one
+             */
         } else {
             /* Remove existing FCS */
             tx_len -= 4;
@@ -558,26 +561,34 @@ static void dp8393x_do_command(dp8393xState *s, uint16_t command)
 
     s->regs[SONIC_CR] |= (command & SONIC_CR_MASK);
 
-    if (command & SONIC_CR_HTX)
+    if (command & SONIC_CR_HTX) {
         dp8393x_do_halt_transmission(s);
-    if (command & SONIC_CR_TXP)
+    }
+    if (command & SONIC_CR_TXP) {
         dp8393x_do_transmit_packets(s);
-    if (command & SONIC_CR_RXDIS)
+    }
+    if (command & SONIC_CR_RXDIS) {
         dp8393x_do_receiver_disable(s);
-    if (command & SONIC_CR_RXEN)
+    }
+    if (command & SONIC_CR_RXEN) {
         dp8393x_do_receiver_enable(s);
-    if (command & SONIC_CR_STP)
+    }
+    if (command & SONIC_CR_STP) {
         dp8393x_do_stop_timer(s);
-    if (command & SONIC_CR_ST)
+    }
+    if (command & SONIC_CR_ST) {
         dp8393x_do_start_timer(s);
-    if (command & SONIC_CR_RST)
+    }
+    if (command & SONIC_CR_RST) {
         dp8393x_do_software_reset(s);
+    }
     if (command & SONIC_CR_RRRA) {
         dp8393x_do_read_rra(s);
         s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
     }
-    if (command & SONIC_CR_LCAM)
+    if (command & SONIC_CR_LCAM) {
         dp8393x_do_load_cam(s);
+    }
 }
 
 static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
@@ -587,24 +598,24 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
     uint16_t val = 0;
 
     switch (reg) {
-        /* Update data before reading it */
-        case SONIC_WT0:
-        case SONIC_WT1:
-            dp8393x_update_wt_regs(s);
-            val = s->regs[reg];
-            break;
-        /* Accept read to some registers only when in reset mode */
-        case SONIC_CAP2:
-        case SONIC_CAP1:
-        case SONIC_CAP0:
-            if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-                val = s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 - reg) + 1] << 8;
-                val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 - reg)];
-            }
-            break;
-        /* All other registers have no special contrainst */
-        default:
-            val = s->regs[reg];
+    /* Update data before reading it */
+    case SONIC_WT0:
+    case SONIC_WT1:
+        dp8393x_update_wt_regs(s);
+        val = s->regs[reg];
+        break;
+    /* Accept read to some registers only when in reset mode */
+    case SONIC_CAP2:
+    case SONIC_CAP1:
+    case SONIC_CAP0:
+        if (s->regs[SONIC_CR] & SONIC_CR_RST) {
+            val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) + 1] << 8;
+            val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)];
+        }
+        break;
+    /* All other registers have no special contraints */
+    default:
+        val = s->regs[reg];
     }
 
     DPRINTF("read 0x%04x from reg %s\n", val, reg_names[reg]);
@@ -622,75 +633,75 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
     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, val);
-            break;
-        /* Prevent write to read-only registers */
-        case SONIC_CAP2:
-        case SONIC_CAP1:
-        case SONIC_CAP0:
-        case SONIC_SR:
-        case SONIC_MDT:
-            DPRINTF("writing to reg %d invalid\n", reg);
-            break;
-        /* Accept write to some registers only when in reset mode */
-        case SONIC_DCR:
-            if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-                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] = val & 0xf017;
-            } else {
-                DPRINTF("writing to DCR2 invalid\n");
-            }
-            break;
-        /* 12 lower bytes are Read Only */
-        case SONIC_TCR:
-            s->regs[reg] = val & 0xf000;
-            break;
-        /* 9 lower bytes are Read Only */
-        case SONIC_RCR:
-            s->regs[reg] = val & 0xffe0;
-            break;
-        /* Ignore most significant bit */
-        case SONIC_IMR:
-            s->regs[reg] = val & 0x7fff;
-            dp8393x_update_irq(s);
-            break;
-        /* Clear bits by writing 1 to them */
-        case SONIC_ISR:
-            val &= s->regs[reg];
-            s->regs[reg] &= ~val;
-            if (val & SONIC_ISR_RBE) {
-                dp8393x_do_read_rra(s);
-            }
-            dp8393x_update_irq(s);
-            break;
-        /* The guest is required to store aligned pointers here */
-        case SONIC_RSA:
-        case SONIC_REA:
-        case SONIC_RRP:
-        case SONIC_RWP:
-            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:
-        case SONIC_FAET:
-        case SONIC_MPT:
-            s->regs[reg] = val ^ 0xffff;
-            break;
-        /* All other registers have no special contrainst */
-        default:
-            s->regs[reg] = val;
+    /* Command register */
+    case SONIC_CR:
+        dp8393x_do_command(s, val);
+        break;
+    /* Prevent write to read-only registers */
+    case SONIC_CAP2:
+    case SONIC_CAP1:
+    case SONIC_CAP0:
+    case SONIC_SR:
+    case SONIC_MDT:
+        DPRINTF("writing to reg %d invalid\n", reg);
+        break;
+    /* Accept write to some registers only when in reset mode */
+    case SONIC_DCR:
+        if (s->regs[SONIC_CR] & SONIC_CR_RST) {
+            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] = val & 0xf017;
+        } else {
+            DPRINTF("writing to DCR2 invalid\n");
+        }
+        break;
+    /* 12 lower bytes are Read Only */
+    case SONIC_TCR:
+        s->regs[reg] = val & 0xf000;
+        break;
+    /* 9 lower bytes are Read Only */
+    case SONIC_RCR:
+        s->regs[reg] = val & 0xffe0;
+        break;
+    /* Ignore most significant bit */
+    case SONIC_IMR:
+        s->regs[reg] = val & 0x7fff;
+        dp8393x_update_irq(s);
+        break;
+    /* Clear bits by writing 1 to them */
+    case SONIC_ISR:
+        val &= s->regs[reg];
+        s->regs[reg] &= ~val;
+        if (val & SONIC_ISR_RBE) {
+            dp8393x_do_read_rra(s);
+        }
+        dp8393x_update_irq(s);
+        break;
+    /* The guest is required to store aligned pointers here */
+    case SONIC_RSA:
+    case SONIC_REA:
+    case SONIC_RRP:
+    case SONIC_RWP:
+        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:
+    case SONIC_FAET:
+    case SONIC_MPT:
+        s->regs[reg] = val ^ 0xffff;
+        break;
+    /* All other registers have no special contrainst */
+    default:
+        s->regs[reg] = val;
     }
 
     if (reg == SONIC_WT0 || reg == SONIC_WT1) {
@@ -747,17 +758,18 @@ static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf,
     }
 
     /* Check broadcast */
-    if ((s->regs[SONIC_RCR] & SONIC_RCR_BRD) && !memcmp(buf, bcast, sizeof(bcast))) {
+    if ((s->regs[SONIC_RCR] & SONIC_RCR_BRD) &&
+         !memcmp(buf, bcast, sizeof(bcast))) {
         return SONIC_RCR_BC;
     }
 
     /* Check CAM */
     for (i = 0; i < 16; i++) {
         if (s->regs[SONIC_CE] & (1 << i)) {
-             /* Entry enabled */
-             if (!memcmp(buf, s->cam[i], sizeof(s->cam[i]))) {
-                 return 0;
-             }
+            /* Entry enabled */
+            if (!memcmp(buf, s->cam[i], sizeof(s->cam[i]))) {
+                return 0;
+            }
         }
     }
 
@@ -938,7 +950,8 @@ static void dp8393x_reset(DeviceState *dev)
     s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux/mips */
     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);
+    s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
+                            SONIC_RCR_RNT);
     s->regs[SONIC_TCR] |= SONIC_TCR_NCRS | SONIC_TCR_PTX;
     s->regs[SONIC_TCR] &= ~SONIC_TCR_BCM;
     s->regs[SONIC_IMR] = 0;
-- 
2.20.1

Re: [PATCH v2 01/10] dp8393x: checkpatch fixes

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> Also fix a simple comment typo of "constrainst" to "constraints".
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/dp8393x.c | 231 +++++++++++++++++++++++++----------------------
>  1 file changed, 122 insertions(+), 109 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 02/10] dp8393x: convert to trace-events

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/dp8393x.c    | 55 +++++++++++++++++----------------------------
 hw/net/trace-events | 17 ++++++++++++++
 2 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 56af08f0fe..ea5b22f680 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -28,14 +28,10 @@
 #include "qemu/timer.h"
 #include <zlib.h>
 #include "qom/object.h"
-
-/* #define DEBUG_SONIC */
+#include "trace.h"
 
 #define SONIC_PROM_SIZE 0x1000
 
-#ifdef DEBUG_SONIC
-#define DPRINTF(fmt, ...) \
-do { printf("sonic: " fmt , ##  __VA_ARGS__); } while (0)
 static const char *reg_names[] = {
     "CR", "DCR", "RCR", "TCR", "IMR", "ISR", "UTDA", "CTDA",
     "TPS", "TFC", "TSA0", "TSA1", "TFS", "URDA", "CRDA", "CRBA0",
@@ -45,12 +41,6 @@ static const char *reg_names[] = {
     "SR", "WT0", "WT1", "RSC", "CRCT", "FAET", "MPT", "MDT",
     "0x30", "0x31", "0x32", "0x33", "0x34", "0x35", "0x36", "0x37",
     "0x38", "0x39", "0x3a", "0x3b", "0x3c", "0x3d", "0x3e", "DCR2" };
-#else
-#define DPRINTF(fmt, ...) do {} while (0)
-#endif
-
-#define SONIC_ERROR(fmt, ...) \
-do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
 
 #define SONIC_CR     0x00
 #define SONIC_DCR    0x01
@@ -161,9 +151,7 @@ struct dp8393xState {
     bool big_endian;
     bool last_rba_is_full;
     qemu_irq irq;
-#ifdef DEBUG_SONIC
     int irq_level;
-#endif
     QEMUTimer *watchdog;
     int64_t wt_last_update;
     NICConf conf;
@@ -270,16 +258,14 @@ static void dp8393x_update_irq(dp8393xState *s)
 {
     int level = (s->regs[SONIC_IMR] & s->regs[SONIC_ISR]) ? 1 : 0;
 
-#ifdef DEBUG_SONIC
     if (level != s->irq_level) {
         s->irq_level = level;
         if (level) {
-            DPRINTF("raise irq, isr is 0x%04x\n", s->regs[SONIC_ISR]);
+            trace_dp8393x_raise_irq(s->regs[SONIC_ISR]);
         } else {
-            DPRINTF("lower irq\n");
+            trace_dp8393x_lower_irq();
         }
     }
-#endif
 
     qemu_set_irq(s->irq, level);
 }
@@ -302,9 +288,9 @@ static void dp8393x_do_load_cam(dp8393xState *s)
         s->cam[index][3] = dp8393x_get(s, width, 2) >> 8;
         s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff;
         s->cam[index][5] = dp8393x_get(s, width, 3) >> 8;
-        DPRINTF("load cam[%d] with %02x%02x%02x%02x%02x%02x\n", index,
-            s->cam[index][0], s->cam[index][1], s->cam[index][2],
-            s->cam[index][3], s->cam[index][4], s->cam[index][5]);
+        trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1],
+                               s->cam[index][2], s->cam[index][3],
+                               s->cam[index][4], s->cam[index][5]);
         /* Move to next entry */
         s->regs[SONIC_CDC]--;
         s->regs[SONIC_CDP] += size;
@@ -315,7 +301,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
     address_space_read(&s->as, dp8393x_cdp(s),
                        MEMTXATTRS_UNSPECIFIED, s->data, size);
     s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
-    DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
+    trace_dp8393x_load_cam_done(s->regs[SONIC_CE]);
 
     /* Done */
     s->regs[SONIC_CR] &= ~SONIC_CR_LCAM;
@@ -338,9 +324,8 @@ static void dp8393x_do_read_rra(dp8393xState *s)
     s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1);
     s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2);
     s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3);
-    DPRINTF("CRBA0/1: 0x%04x/0x%04x, RBWC0/1: 0x%04x/0x%04x\n",
-        s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
-        s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
+    trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
+                                s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
 
     /* Go to next entry */
     s->regs[SONIC_RRP] += size;
@@ -444,7 +429,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
         /* Read memory */
         size = sizeof(uint16_t) * 6 * width;
         s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
-        DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
+        trace_dp8393x_transmit_packet(dp8393x_ttda(s));
         address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
                            MEMTXATTRS_UNSPECIFIED, s->data, size);
         tx_len = 0;
@@ -499,7 +484,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
             /* Remove existing FCS */
             tx_len -= 4;
             if (tx_len < 0) {
-                SONIC_ERROR("tx_len is %d\n", tx_len);
+                trace_dp8393x_transmit_txlen_error(tx_len);
                 break;
             }
         }
@@ -618,7 +603,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
         val = s->regs[reg];
     }
 
-    DPRINTF("read 0x%04x from reg %s\n", val, reg_names[reg]);
+    trace_dp8393x_read(reg, reg_names[reg], val, size);
 
     return s->big_endian ? val << 16 : val;
 }
@@ -630,7 +615,7 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
     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)val, reg_names[reg]);
+    trace_dp8393x_write(reg, reg_names[reg], val, size);
 
     switch (reg) {
     /* Command register */
@@ -643,21 +628,21 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
     case SONIC_CAP0:
     case SONIC_SR:
     case SONIC_MDT:
-        DPRINTF("writing to reg %d invalid\n", reg);
+        trace_dp8393x_write_invalid(reg);
         break;
     /* Accept write to some registers only when in reset mode */
     case SONIC_DCR:
         if (s->regs[SONIC_CR] & SONIC_CR_RST) {
             s->regs[reg] = val & 0xbfff;
         } else {
-            DPRINTF("writing to DCR invalid\n");
+            trace_dp8393x_write_invalid_dcr("DCR");
         }
         break;
     case SONIC_DCR2:
         if (s->regs[SONIC_CR] & SONIC_CR_RST) {
             s->regs[reg] = val & 0xf017;
         } else {
-            DPRINTF("writing to DCR2 invalid\n");
+            trace_dp8393x_write_invalid_dcr("DCR2");
         }
         break;
     /* 12 lower bytes are Read Only */
@@ -803,7 +788,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     }
 
     if (padded_len > dp8393x_rbwc(s) * 2) {
-        DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
+        trace_dp8393x_receive_oversize(pkt_size);
         s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
         dp8393x_update_irq(s);
         s->regs[SONIC_RCR] |= SONIC_RCR_LPKT;
@@ -812,7 +797,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
 
     packet_type = dp8393x_receive_filter(s, buf, pkt_size);
     if (packet_type < 0) {
-        DPRINTF("packet not for netcard\n");
+        trace_dp8393x_receive_not_netcard();
         return -1;
     }
 
@@ -850,7 +835,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     checksum = cpu_to_le32(crc32(0, buf, pkt_size));
 
     /* Put packet into RBA */
-    DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
+    trace_dp8393x_receive_packet(dp8393x_crba(s));
     address = dp8393x_crba(s);
     address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
                         buf, pkt_size);
@@ -888,7 +873,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     }
 
     /* Write status to memory */
-    DPRINTF("Write status at %08x\n", dp8393x_crda(s));
+    trace_dp8393x_receive_write_status(dp8393x_crda(s));
     dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */
     dp8393x_put(s, width, 1, rx_len); /* byte count */
     dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */
diff --git a/hw/net/trace-events b/hw/net/trace-events
index c28b91ee1a..643338f610 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -436,3 +436,20 @@ npcm7xx_emc_received_packet(uint32_t len) "Received %u byte packet"
 npcm7xx_emc_rx_done(uint32_t crxdsa) "RX done, CRXDSA=0x%x"
 npcm7xx_emc_reg_read(int emc_num, uint32_t result, const char *name, int regno) "emc%d: 0x%x = reg[%s/%d]"
 npcm7xx_emc_reg_write(int emc_num, const char *name, int regno, uint32_t value) "emc%d: reg[%s/%d] = 0x%x"
+
+# dp8398x.c
+dp8393x_raise_irq(int isr) "raise irq, isr is 0x%04x"
+dp8393x_lower_irq(void) "lower irq"
+dp8393x_load_cam(int idx, int cam0, int cam1, int cam2, int cam3, int cam4, int cam5) "load cam[%d] with 0x%02x0x%02x0x%02x0x%02x0x%02x0x%02x"
+dp8393x_load_cam_done(int cen) "load cam done. cam enable mask 0x%04x"
+dp8393x_read_rra_regs(int crba0, int crba1, int rbwc0, int rbwc1) "CRBA0/1: 0x%04x/0x%04x, RBWC0/1: 0x%04x/0x%04x"
+dp8393x_transmit_packet(int ttda) "Transmit packet at 0x%"PRIx32
+dp8393x_transmit_txlen_error(int len) "tx_len is %d"
+dp8393x_read(int reg, const char *name, int val, int size) "reg=0x%x [%s] val=0x%04x size=%d"
+dp8393x_write(int reg, const char *name, int val, int size) "reg=0x%x [%s] val=0x%04x size=%d"
+dp8393x_write_invalid(int reg) "writing to reg %d invalid"
+dp8393x_write_invalid_dcr(const char *name) "writing to %s invalid"
+dp8393x_receive_oversize(int size) "oversize packet, pkt_size is %d"
+dp8393x_receive_not_netcard(void) "packet not for netcard"
+dp8393x_receive_packet(int crba) "Receive packet at 0x%"PRIx32
+dp8393x_receive_write_status(int crba) "Write status at 0x%"PRIx32
-- 
2.20.1

Re: [PATCH v2 02/10] dp8393x: convert to trace-events

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/dp8393x.c    | 55 +++++++++++++++++----------------------------
>  hw/net/trace-events | 17 ++++++++++++++
>  2 files changed, 37 insertions(+), 35 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 03/10] hw/mips/jazz: move PROM and checksum calculation from dp8393x device to board

[Mark Cave-Ayland]

This is in preparation for each board to have its own separate bit storage
format and checksum for storing the MAC address.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/mips/jazz.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 1e1cf8154e..89ca8bb910 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -119,6 +119,8 @@ static const MemoryRegionOps dma_dummy_ops = {
 #define MAGNUM_BIOS_SIZE                                                       \
         (BIOS_SIZE < MAGNUM_BIOS_SIZE_MAX ? BIOS_SIZE : MAGNUM_BIOS_SIZE_MAX)
 
+#define SONIC_PROM_SIZE 0x1000
+
 static void mips_jazz_init(MachineState *machine,
                            enum jazz_model_e jazz_model)
 {
@@ -137,6 +139,7 @@ static void mips_jazz_init(MachineState *machine,
     MemoryRegion *rtc = g_new(MemoryRegion, 1);
     MemoryRegion *i8042 = g_new(MemoryRegion, 1);
     MemoryRegion *dma_dummy = g_new(MemoryRegion, 1);
+    MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
     NICInfo *nd;
     DeviceState *dev, *rc4030;
     SysBusDevice *sysbus;
@@ -228,6 +231,10 @@ static void mips_jazz_init(MachineState *machine,
                           NULL, "dummy_dma", 0x1000);
     memory_region_add_subregion(address_space, 0x8000d000, dma_dummy);
 
+    memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-jazz.prom",
+                           SONIC_PROM_SIZE, &error_fatal);
+    memory_region_add_subregion(address_space, 0x8000b000, dp8393x_prom);
+
     /* ISA bus: IO space at 0x90000000, mem space at 0x91000000 */
     memory_region_init(isa_io, NULL, "isa-io", 0x00010000);
     memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
@@ -275,6 +282,9 @@ static void mips_jazz_init(MachineState *machine,
             nd->model = g_strdup("dp83932");
         }
         if (strcmp(nd->model, "dp83932") == 0) {
+            int checksum, i;
+            uint8_t *prom;
+
             qemu_check_nic_model(nd, "dp83932");
 
             dev = qdev_new("dp8393x");
@@ -285,8 +295,19 @@ static void mips_jazz_init(MachineState *machine,
             sysbus = SYS_BUS_DEVICE(dev);
             sysbus_realize_and_unref(sysbus, &error_fatal);
             sysbus_mmio_map(sysbus, 0, 0x80001000);
-            sysbus_mmio_map(sysbus, 1, 0x8000b000);
             sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 4));
+
+            /* Add MAC address with valid checksum to PROM */
+            prom = memory_region_get_ram_ptr(dp8393x_prom);
+            checksum = 0;
+            for (i = 0; i < 6; i++) {
+                prom[i] = nd->macaddr.a[i];
+                checksum += prom[i];
+                if (checksum > 0xff) {
+                    checksum = (checksum + 1) & 0xff;
+                }
+            }
+            prom[7] = 0xff - checksum;
             break;
         } else if (is_help_option(nd->model)) {
             error_report("Supported NICs: dp83932");
-- 
2.20.1

Re: [PATCH v2 03/10] hw/mips/jazz: move PROM and checksum calculation from dp8393x device to board

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> This is in preparation for each board to have its own separate bit storage
> format and checksum for storing the MAC address.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/mips/jazz.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 04/10] hw/m68k/q800: move PROM and checksum calculation from dp8393x device to board

[Mark Cave-Ayland]

This is in preparation for each board to have its own separate bit storage
format and checksum for storing the MAC address.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 11376daa85..491f283a17 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -70,6 +70,8 @@
 #define NUBUS_SUPER_SLOT_BASE 0x60000000
 #define NUBUS_SLOT_BASE       0xf0000000
 
+#define SONIC_PROM_SIZE       0x1000
+
 /*
  * the video base, whereas it a Nubus address,
  * is needed by the kernel to have early display and
@@ -211,8 +213,10 @@ static void q800_init(MachineState *machine)
     int32_t initrd_size;
     MemoryRegion *rom;
     MemoryRegion *io;
+    MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
+    uint8_t *prom;
     const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
-    int i;
+    int i, checksum;
     ram_addr_t ram_size = machine->ram_size;
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
@@ -319,9 +323,25 @@ static void q800_init(MachineState *machine)
     sysbus = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 0, SONIC_BASE);
-    sysbus_mmio_map(sysbus, 1, SONIC_PROM_BASE);
     sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 2));
 
+    memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
+                           SONIC_PROM_SIZE, &error_fatal);
+    memory_region_add_subregion(get_system_memory(), SONIC_PROM_BASE,
+                                dp8393x_prom);
+
+    /* Add MAC address with valid checksum to PROM */
+    prom = memory_region_get_ram_ptr(dp8393x_prom);
+    checksum = 0;
+    for (i = 0; i < 6; i++) {
+        prom[i] = nd_table[0].macaddr.a[i];
+        checksum += prom[i];
+        if (checksum > 0xff) {
+            checksum = (checksum + 1) & 0xff;
+        }
+    }
+    prom[7] = 0xff - checksum;
+
     /* SCC */
 
     dev = qdev_new(TYPE_ESCC);
-- 
2.20.1

Re: [PATCH v2 04/10] hw/m68k/q800: move PROM and checksum calculation from dp8393x device to board

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> This is in preparation for each board to have its own separate bit storage
> format and checksum for storing the MAC address.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 05/10] dp8393x: remove onboard PROM containing MAC address and checksum

[Mark Cave-Ayland]

According to the datasheet the dp8393x chipset does not contain any NVRAM capable
of storing a MAC address or checksum. Now that both the MIPS jazz and m68k q800
boards generate the PROM region and checksum themselves, remove the generated
PROM from the dp8393x device itself.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/dp8393x.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index ea5b22f680..252c0a2664 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -30,8 +30,6 @@
 #include "qom/object.h"
 #include "trace.h"
 
-#define SONIC_PROM_SIZE 0x1000
-
 static const char *reg_names[] = {
     "CR", "DCR", "RCR", "TCR", "IMR", "ISR", "UTDA", "CTDA",
     "TPS", "TFC", "TSA0", "TSA1", "TFS", "URDA", "CRDA", "CRBA0",
@@ -157,7 +155,6 @@ struct dp8393xState {
     NICConf conf;
     NICState *nic;
     MemoryRegion mmio;
-    MemoryRegion prom;
 
     /* Registers */
     uint8_t cam[16][6];
@@ -966,16 +963,12 @@ static void dp8393x_instance_init(Object *obj)
     dp8393xState *s = DP8393X(obj);
 
     sysbus_init_mmio(sbd, &s->mmio);
-    sysbus_init_mmio(sbd, &s->prom);
     sysbus_init_irq(sbd, &s->irq);
 }
 
 static void dp8393x_realize(DeviceState *dev, Error **errp)
 {
     dp8393xState *s = DP8393X(dev);
-    int i, checksum;
-    uint8_t *prom;
-    Error *local_err = NULL;
 
     address_space_init(&s->as, s->dma_mr, "dp8393x");
     memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
@@ -986,23 +979,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);
-
-    memory_region_init_rom(&s->prom, OBJECT(dev), "dp8393x-prom",
-                           SONIC_PROM_SIZE, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    prom = memory_region_get_ram_ptr(&s->prom);
-    checksum = 0;
-    for (i = 0; i < 6; i++) {
-        prom[i] = s->conf.macaddr.a[i];
-        checksum += prom[i];
-        if (checksum > 0xff) {
-            checksum = (checksum + 1) & 0xff;
-        }
-    }
-    prom[7] = 0xff - checksum;
 }
 
 static const VMStateDescription vmstate_dp8393x = {
-- 
2.20.1

Re: [PATCH v2 05/10] dp8393x: remove onboard PROM containing MAC address and checksum

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> According to the datasheet the dp8393x chipset does not contain any NVRAM capable
> of storing a MAC address or checksum. Now that both the MIPS jazz and m68k q800
> boards generate the PROM region and checksum themselves, remove the generated
> PROM from the dp8393x device itself.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/dp8393x.c | 24 ------------------------
>  1 file changed, 24 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 06/10] qemu/bitops.h: add bitrev8 implementation

[Mark Cave-Ayland]

This will be required for an upcoming checksum calculation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/qemu/bitops.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 03213ce952..110c56e099 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -618,4 +618,26 @@ static inline uint64_t half_unshuffle64(uint64_t x)
     return x;
 }
 
+/**
+ * bitrev8:
+ * @x: 8-bit value to be reversed
+ *
+ * Given an input value with bits::
+ *
+ *   ABCDEFGH
+ *
+ * return the value with its bits reversed from left to right::
+ *
+ *   HGFEDCBA
+ *
+ * Returns: the bit-reversed value.
+ */
+static inline uint8_t bitrev8(uint8_t x)
+{
+    x = ((x >> 1) & 0x55) | ((x << 1) & 0xaa);
+    x = ((x >> 2) & 0x33) | ((x << 2) & 0xcc);
+    x = (x >> 4) | (x << 4) ;
+    return x;
+}
+
 #endif
-- 
2.20.1

Re: [PATCH v2 06/10] qemu/bitops.h: add bitrev8 implementation

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> This will be required for an upcoming checksum calculation.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  include/qemu/bitops.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 07/10] hw/m68k/q800: fix PROM checksum and MAC address storage

[Mark Cave-Ayland]

The checksum used by MacOS to validate the PROM content is an exclusive-OR
rather than a sum over the corresponding bytes. In addition the MAC address
must be stored in bit-reversed format as indicated in comments in Linux's
macsonic.c.

With the PROM contents fixed MacOS starts to probe the device registers
when AppleTalk is enabled in the Control Panel.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 491f283a17..6817c8b5d1 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -334,11 +334,8 @@ static void q800_init(MachineState *machine)
     prom = memory_region_get_ram_ptr(dp8393x_prom);
     checksum = 0;
     for (i = 0; i < 6; i++) {
-        prom[i] = nd_table[0].macaddr.a[i];
-        checksum += prom[i];
-        if (checksum > 0xff) {
-            checksum = (checksum + 1) & 0xff;
-        }
+        prom[i] = bitrev8(nd_table[0].macaddr.a[i]);
+        checksum ^= prom[i];
     }
     prom[7] = 0xff - checksum;
 
-- 
2.20.1

[PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses
to the registers were 32-bit but this is actually not the case. The access size is
determined by the CPU instruction used and not the number of physical address lines.

The big_endian workaround applied to the register read/writes was actually caused
by forcing the access size to 32-bit when the guest OS was using a 16-bit access.
Since the registers are 16-bit then we can simply set .impl.min_access to 2 and
then the memory API will automatically do the right thing for both 16-bit accesses
used by Linux and 32-bit accesses used by the MacOS toolbox ROM.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
---
 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 252c0a2664..6789bcd3af 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
 
     trace_dp8393x_read(reg, reg_names[reg], val, size);
 
-    return s->big_endian ? val << 16 : val;
+    return val;
 }
 
-static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
+static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val,
                           unsigned int size)
 {
     dp8393xState *s = opaque;
     int reg = addr >> s->it_shift;
-    uint32_t val = s->big_endian ? data >> 16 : data;
 
     trace_dp8393x_write(reg, reg_names[reg], val, size);
 
@@ -694,7 +693,7 @@ 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 = 4,
+    .impl.min_access_size = 2,
     .impl.max_access_size = 4,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
-- 
2.20.1

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses
> to the registers were 32-bit but this is actually not the case. The access size is
> determined by the CPU instruction used and not the number of physical address lines.
> 
> The big_endian workaround applied to the register read/writes was actually caused
> by forcing the access size to 32-bit when the guest OS was using a 16-bit access.
> Since the registers are 16-bit then we can simply set .impl.min_access to 2 and
> then the memory API will automatically do the right thing for both 16-bit accesses
> used by Linux and 32-bit accesses used by the MacOS toolbox ROM.

Hmm I'm not sure. This sounds to me like the "QEMU doesn't model
busses so we end using kludge to hide bugs" pattern. Can you
provide a QTest (ideally) or a "-trace memory_region_ops_\*" log
of your firmware accessing the dp8393x please?

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> ---
>  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 252c0a2664..6789bcd3af 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
>  
>      trace_dp8393x_read(reg, reg_names[reg], val, size);
>  
> -    return s->big_endian ? val << 16 : val;
> +    return val;
>  }
>  
> -static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> +static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val,
>                            unsigned int size)
>  {
>      dp8393xState *s = opaque;
>      int reg = addr >> s->it_shift;
> -    uint32_t val = s->big_endian ? data >> 16 : data;
>  
>      trace_dp8393x_write(reg, reg_names[reg], val, size);
>  
> @@ -694,7 +693,7 @@ 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 = 4,
> +    .impl.min_access_size = 2,
>      .impl.max_access_size = 4,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
> 

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Finn Thain]

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

On Thu, 1 Jul 2021, Philippe Mathieu-Daudé wrote:

> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> > Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that 
> > all accesses to the registers were 32-bit 

No, that assumption was not made there. Just take a look at my commits in 
Linux that make 16-bit accesses. If commit 3fe9a838ec worked by accident, 
it probably just reflects my inadequate knowledge of QEMU internals.

> > but this is actually not the case. The access size is determined by 
> > the CPU instruction used and not the number of physical address lines.
> > 

I think that's an over-simplification (in the context of commit 
3fe9a838ec).

> > The big_endian workaround applied to the register read/writes was 
> > actually caused by forcing the access size to 32-bit when the guest OS 
> > was using a 16-bit access. Since the registers are 16-bit then we can 
> > simply set .impl.min_access to 2 and then the memory API will 
> > automatically do the right thing for both 16-bit accesses used by 
> > Linux and 32-bit accesses used by the MacOS toolbox ROM.
> 
> Hmm I'm not sure. This sounds to me like the "QEMU doesn't model busses 
> so we end using kludge to hide bugs" pattern. Can you provide a QTest 
> (ideally) or a "-trace memory_region_ops_\*" log of your firmware 
> accessing the dp8393x please?
> 

The DP83932 chip is highly configurable, so I'm not sure that the 
behaviour of any given firmware would resolve the question.

Anyway, as far as the DP83932 hardware is concerned, the behaviour of the 
upper 16-bits of the data bus depends on the configuration programmed into 
the DP83932 registers, and whether the chip is accessed as a slave or 
performing DMA as a master.

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

On 02/07/2021 05:36, Finn Thain wrote:

>> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that
>>> all accesses to the registers were 32-bit
> 
> No, that assumption was not made there. Just take a look at my commits in
> Linux that make 16-bit accesses. If commit 3fe9a838ec worked by accident,
> it probably just reflects my inadequate knowledge of QEMU internals.
> 
>>> but this is actually not the case. The access size is determined by
>>> the CPU instruction used and not the number of physical address lines.
>>>
> 
> I think that's an over-simplification (in the context of commit
> 3fe9a838ec).

Let me try and clarify this a bit more: there are 2 different changes incorporated 
into 3fe9a838ec. The first (as you mention below and also detailed in the commit 
messge), is related to handling writes to the upper 16-bits of a word from the device 
and ensuring that 32-bit accesses are handled correctly. This part seems correct to 
me based upon reading the datasheet and the commit message:

@@ -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);
+        }
      }
  }

The second change incorporated into 3fe9a838ec (and the one this patch fixes) is a 
similar change made to the MMIO *register* accesses:

@@ -590,7 +600,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;
  }

and:

@@ -598,13 +608,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;

This is not correct since the QEMU memory API handles any access size and endian 
conversion before the MMIO access reaches the device. It is this change which breaks 
the 32-bit accesses used by MacOS to read/write the dp8393x registers because it 
applies an additional endian swap on top of that already done by the memory API.

>>> The big_endian workaround applied to the register read/writes was
>>> actually caused by forcing the access size to 32-bit when the guest OS
>>> was using a 16-bit access. Since the registers are 16-bit then we can
>>> simply set .impl.min_access to 2 and then the memory API will
>>> automatically do the right thing for both 16-bit accesses used by
>>> Linux and 32-bit accesses used by the MacOS toolbox ROM.
>>
>> Hmm I'm not sure. This sounds to me like the "QEMU doesn't model busses
>> so we end using kludge to hide bugs" pattern. Can you provide a QTest
>> (ideally) or a "-trace memory_region_ops_\*" log of your firmware
>> accessing the dp8393x please?
>>
> The DP83932 chip is highly configurable, so I'm not sure that the
> behaviour of any given firmware would resolve the question.

Indeed, I don't think that will help much here. Phil, if you would still like me to 
send you some traces after reading the explanation above then do let me know.

> Anyway, as far as the DP83932 hardware is concerned, the behaviour of the
> upper 16-bits of the data bus depends on the configuration programmed into
> the DP83932 registers, and whether the chip is accessed as a slave or
> performing DMA as a master.

The important part of the commit and its associated message is that it only changes 
the *register* accesses which were introduced as part of 3fe9a838ec.

In the end all the patch does is remove the manual endian swap from the MMIO 
registers since QEMU's memory API does the right thing all by itself, and adds the 
tweak below:

@@ -694,7 +693,7 @@ 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 = 4,
+    .impl.min_access_size = 2,
      .impl.max_access_size = 4,
      .endianness = DEVICE_NATIVE_ENDIAN,
  };

As Finn points out the dp8393x registers are 16-bit so we declare the implementation 
size as 2 bytes and the maximum size as 4 bytes. This allows Linux to function 
correctly with 16-bit accesses, whilst 32-bit accesses done by MacOS are split into 2 
separate 16-bit accesses and combined automatically by the memory API.


ATB,

Mark.

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 7/3/21 8:21 AM, Mark Cave-Ayland wrote:
> On 02/07/2021 05:36, Finn Thain wrote:
> 
>>> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
>>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that
>>>> all accesses to the registers were 32-bit
>>
>> No, that assumption was not made there. Just take a look at my commits in
>> Linux that make 16-bit accesses. If commit 3fe9a838ec worked by accident,
>> it probably just reflects my inadequate knowledge of QEMU internals.
>>
>>>> but this is actually not the case. The access size is determined by
>>>> the CPU instruction used and not the number of physical address lines.
>>>>
>>
>> I think that's an over-simplification (in the context of commit
>> 3fe9a838ec).
> 
> Let me try and clarify this a bit more: there are 2 different changes
> incorporated into 3fe9a838ec. The first (as you mention below and also
> detailed in the commit messge), is related to handling writes to the
> upper 16-bits of a word from the device and ensuring that 32-bit
> accesses are handled correctly. This part seems correct to me based upon
> reading the datasheet and the commit message:
> 
> @@ -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);
> +        }
>      }
>  }
> 
> The second change incorporated into 3fe9a838ec (and the one this patch
> fixes) is a similar change made to the MMIO *register* accesses:
> 
> @@ -590,7 +600,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;
>  }
> 
> and:
> 
> @@ -598,13 +608,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;
> 
> This is not correct since the QEMU memory API handles any access size
> and endian conversion before the MMIO access reaches the device. It is
> this change which breaks the 32-bit accesses used by MacOS to read/write
> the dp8393x registers because it applies an additional endian swap on
> top of that already done by the memory API.
> 
>>>> The big_endian workaround applied to the register read/writes was
>>>> actually caused by forcing the access size to 32-bit when the guest OS
>>>> was using a 16-bit access. Since the registers are 16-bit then we can
>>>> simply set .impl.min_access to 2 and then the memory API will
>>>> automatically do the right thing for both 16-bit accesses used by
>>>> Linux and 32-bit accesses used by the MacOS toolbox ROM.
>>>
>>> Hmm I'm not sure. This sounds to me like the "QEMU doesn't model busses
>>> so we end using kludge to hide bugs" pattern. Can you provide a QTest
>>> (ideally) or a "-trace memory_region_ops_\*" log of your firmware
>>> accessing the dp8393x please?
>>>
>> The DP83932 chip is highly configurable, so I'm not sure that the
>> behaviour of any given firmware would resolve the question.
> 
> Indeed, I don't think that will help much here. Phil, if you would still
> like me to send you some traces after reading the explanation above then
> do let me know.

I read it and still would have few traces. We can hand-write them too.

I'd like to add qtests for some read/write,16/32(A)==B.

>> Anyway, as far as the DP83932 hardware is concerned, the behaviour of the
>> upper 16-bits of the data bus depends on the configuration programmed
>> into
>> the DP83932 registers, and whether the chip is accessed as a slave or
>> performing DMA as a master.
> 
> The important part of the commit and its associated message is that it
> only changes the *register* accesses which were introduced as part of
> 3fe9a838ec.
> 
> In the end all the patch does is remove the manual endian swap from the
> MMIO registers since QEMU's memory API does the right thing all by
> itself, and adds the tweak below:
> 
> @@ -694,7 +693,7 @@ 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 = 4,
> +    .impl.min_access_size = 2,
>      .impl.max_access_size = 4,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
> 
> As Finn points out the dp8393x registers are 16-bit so we declare the
> implementation size as 2 bytes and the maximum size as 4 bytes. This
> allows Linux to function correctly with 16-bit accesses, whilst 32-bit
> accesses done by MacOS are split into 2 separate 16-bit accesses and
> combined automatically by the memory API.

I hadn't check the datasheet yet but will have a look.

Thanks,

Phil.

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

On 03/07/2021 09:52, Philippe Mathieu-Daudé wrote:

> On 7/3/21 8:21 AM, Mark Cave-Ayland wrote:
>> On 02/07/2021 05:36, Finn Thain wrote:
>>
>>>> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
>>>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that
>>>>> all accesses to the registers were 32-bit
>>>
>>> No, that assumption was not made there. Just take a look at my commits in
>>> Linux that make 16-bit accesses. If commit 3fe9a838ec worked by accident,
>>> it probably just reflects my inadequate knowledge of QEMU internals.
>>>
>>>>> but this is actually not the case. The access size is determined by
>>>>> the CPU instruction used and not the number of physical address lines.
>>>>>
>>>
>>> I think that's an over-simplification (in the context of commit
>>> 3fe9a838ec).
>>
>> Let me try and clarify this a bit more: there are 2 different changes
>> incorporated into 3fe9a838ec. The first (as you mention below and also
>> detailed in the commit messge), is related to handling writes to the
>> upper 16-bits of a word from the device and ensuring that 32-bit
>> accesses are handled correctly. This part seems correct to me based upon
>> reading the datasheet and the commit message:
>>
>> @@ -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);
>> +        }
>>       }
>>   }
>>
>> The second change incorporated into 3fe9a838ec (and the one this patch
>> fixes) is a similar change made to the MMIO *register* accesses:
>>
>> @@ -590,7 +600,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;
>>   }
>>
>> and:
>>
>> @@ -598,13 +608,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;
>>
>> This is not correct since the QEMU memory API handles any access size
>> and endian conversion before the MMIO access reaches the device. It is
>> this change which breaks the 32-bit accesses used by MacOS to read/write
>> the dp8393x registers because it applies an additional endian swap on
>> top of that already done by the memory API.
>>
>>>>> The big_endian workaround applied to the register read/writes was
>>>>> actually caused by forcing the access size to 32-bit when the guest OS
>>>>> was using a 16-bit access. Since the registers are 16-bit then we can
>>>>> simply set .impl.min_access to 2 and then the memory API will
>>>>> automatically do the right thing for both 16-bit accesses used by
>>>>> Linux and 32-bit accesses used by the MacOS toolbox ROM.
>>>>
>>>> Hmm I'm not sure. This sounds to me like the "QEMU doesn't model busses
>>>> so we end using kludge to hide bugs" pattern. Can you provide a QTest
>>>> (ideally) or a "-trace memory_region_ops_\*" log of your firmware
>>>> accessing the dp8393x please?
>>>>
>>> The DP83932 chip is highly configurable, so I'm not sure that the
>>> behaviour of any given firmware would resolve the question.
>>
>> Indeed, I don't think that will help much here. Phil, if you would still
>> like me to send you some traces after reading the explanation above then
>> do let me know.
> 
> I read it and still would have few traces. We can hand-write them too.
> 
> I'd like to add qtests for some read/write,16/32(A)==B.

Sigh. I was just about to attempt some traces when I realised looking at the patch 
that I made a mistake, and that in order for the memory API to automatically handle 
the 4 byte accesses as 2 x 2 byte accesses then both .impl.min_access_size and 
.impl.max_access_size need to be set to 2 :(  The remainder of the patch is the same 
but dp8393x_ops now looks like this:

static const MemoryRegionOps dp8393x_ops = {
     .read = dp8393x_read,
     .write = dp8393x_write,
     .impl.min_access_size = 2,
     .impl.max_access_size = 2,
     .endianness = DEVICE_NATIVE_ENDIAN,
};

I've tested this under Linux/m68k, NetBSD/arc and MacOS and networking seems fine 
after a quick test in each OS. The slight curiosity is that the 4 byte accesses used 
by MacOS are split into 2 x 2 byte accesses, but since MacOS only uses the bottom 
16-bit of the result and ignores the top 16-bits then despite there being 2 accesses, 
everything still works as expected (see below).


READ:

dp8393x_read reg=0x28 [SR] val=0x0004 size=2
memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a0 value 0x4 size 2
dp8393x_read reg=0x28 [SR] val=0x0004 size=2
memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a2 value 0x4 size 2
memory_region_ops_read cpu 0 mr 0x55f210c9c330 addr 0x50f0a0a0 value 0x40004 size 4

WRITE:

memory_region_ops_write cpu 0 mr 0x55f210c9c330 addr 0x50f0a01c value 0x248fe8 size 4
memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01c value 0x24 size 2
dp8393x_write reg=0x7 [CTDA] val=0x0024 size=2
memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01e value 0x8fe8 size 2
dp8393x_write reg=0x7 [CTDA] val=0x8fe8 size=2


If you're happy with this, I'll resubmit a revised version of the patch but with an 
updated commit message: the Fixes: tag is still the same, but the updated fix is to 
ensure that .impl.min_access_size and .impl.max_access_size match the real-life 
16-bit register size.


ATB,

Mark.

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 7/3/21 2:04 PM, Mark Cave-Ayland wrote:
> On 03/07/2021 09:52, Philippe Mathieu-Daudé wrote:
>> On 7/3/21 8:21 AM, Mark Cave-Ayland wrote:
>>> On 02/07/2021 05:36, Finn Thain wrote:
>>>
>>>>> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
>>>>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that
>>>>>> all accesses to the registers were 32-bit
>>>>
>>>> No, that assumption was not made there. Just take a look at my
>>>> commits in
>>>> Linux that make 16-bit accesses. If commit 3fe9a838ec worked by
>>>> accident,
>>>> it probably just reflects my inadequate knowledge of QEMU internals.
>>>>
>>>>>> but this is actually not the case. The access size is determined by
>>>>>> the CPU instruction used and not the number of physical address
>>>>>> lines.
>>>>>>
>>>>
>>>> I think that's an over-simplification (in the context of commit
>>>> 3fe9a838ec).
>>>
>>> Let me try and clarify this a bit more: there are 2 different changes
>>> incorporated into 3fe9a838ec. The first (as you mention below and also
>>> detailed in the commit messge), is related to handling writes to the
>>> upper 16-bits of a word from the device and ensuring that 32-bit
>>> accesses are handled correctly. This part seems correct to me based upon
>>> reading the datasheet and the commit message:
>>>
>>> @@ -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);
>>> +        }
>>>       }
>>>   }
>>>
>>> The second change incorporated into 3fe9a838ec (and the one this patch
>>> fixes) is a similar change made to the MMIO *register* accesses:
>>>
>>> @@ -590,7 +600,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;
>>>   }
>>>
>>> and:
>>>
>>> @@ -598,13 +608,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;
>>>
>>> This is not correct since the QEMU memory API handles any access size
>>> and endian conversion before the MMIO access reaches the device. It is
>>> this change which breaks the 32-bit accesses used by MacOS to read/write
>>> the dp8393x registers because it applies an additional endian swap on
>>> top of that already done by the memory API.
>>>
>>>>>> The big_endian workaround applied to the register read/writes was
>>>>>> actually caused by forcing the access size to 32-bit when the
>>>>>> guest OS
>>>>>> was using a 16-bit access. Since the registers are 16-bit then we can
>>>>>> simply set .impl.min_access to 2 and then the memory API will
>>>>>> automatically do the right thing for both 16-bit accesses used by
>>>>>> Linux and 32-bit accesses used by the MacOS toolbox ROM.
>>>>>
>>>>> Hmm I'm not sure. This sounds to me like the "QEMU doesn't model
>>>>> busses
>>>>> so we end using kludge to hide bugs" pattern. Can you provide a QTest
>>>>> (ideally) or a "-trace memory_region_ops_\*" log of your firmware
>>>>> accessing the dp8393x please?
>>>>>
>>>> The DP83932 chip is highly configurable, so I'm not sure that the
>>>> behaviour of any given firmware would resolve the question.
>>>
>>> Indeed, I don't think that will help much here. Phil, if you would still
>>> like me to send you some traces after reading the explanation above then
>>> do let me know.
>>
>> I read it and still would have few traces. We can hand-write them too.
>>
>> I'd like to add qtests for some read/write,16/32(A)==B.
> 
> Sigh. I was just about to attempt some traces when I realised looking at
> the patch that I made a mistake, and that in order for the memory API to
> automatically handle the 4 byte accesses as 2 x 2 byte accesses then
> both .impl.min_access_size and .impl.max_access_size need to be set to 2
> :(  The remainder of the patch is the same but dp8393x_ops now looks
> like this:
> 
> static const MemoryRegionOps dp8393x_ops = {
>     .read = dp8393x_read,
>     .write = dp8393x_write,
>     .impl.min_access_size = 2,
>     .impl.max_access_size = 2,
>     .endianness = DEVICE_NATIVE_ENDIAN,
> };

Yes :) This is closer to what I wrote during my review:

-- >8 --
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index bff359ed13f..6061268dc49 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -697,7 +697,9 @@ static const MemoryRegionOps dp8393x_ops = {
     .read = dp8393x_read,
     .write = dp8393x_write,
     .impl.min_access_size = 2,
-    .impl.max_access_size = 4,
+    .impl.max_access_size = 2,
+    .valid.min_access_size = 2,
+    .valid.max_access_size = 4,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
---

> 
> I've tested this under Linux/m68k, NetBSD/arc and MacOS and networking
> seems fine after a quick test in each OS. The slight curiosity is that
> the 4 byte accesses used by MacOS are split into 2 x 2 byte accesses,
> but since MacOS only uses the bottom 16-bit of the result and ignores
> the top 16-bits then despite there being 2 accesses, everything still
> works as expected (see below).
> 
> 
> READ:
> 
> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a0 value 0x4
> size 2
> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a2 value 0x4
> size 2
> memory_region_ops_read cpu 0 mr 0x55f210c9c330 addr 0x50f0a0a0 value
> 0x40004 size 4
> 
> WRITE:
> 
> memory_region_ops_write cpu 0 mr 0x55f210c9c330 addr 0x50f0a01c value
> 0x248fe8 size 4
> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01c value
> 0x24 size 2
> dp8393x_write reg=0x7 [CTDA] val=0x0024 size=2
> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01e value
> 0x8fe8 size 2
> dp8393x_write reg=0x7 [CTDA] val=0x8fe8 size=2
> 
> 
> If you're happy with this, I'll resubmit a revised version of the patch
> but with an updated commit message: the Fixes: tag is still the same,
> but the updated fix is to ensure that .impl.min_access_size and
> .impl.max_access_size match the real-life 16-bit register size.

Hold on a bit more, I'm still reviewing the datasheet ;)

My code note so far (untested) are:

-- >8 --
@@ -219,34 +225,48 @@ static uint32_t dp8393x_wt(dp8393xState *s)
     return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
 }

-static uint16_t dp8393x_get(dp8393xState *s, int width, int offset)
+static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16)
 {
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     uint16_t val;

-    if (s->big_endian) {
-        val = be16_to_cpu(s->data[offset * width + width - 1]);
+    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+        addr += 2 * ofs16;
+        if (s->big_endian) {
+            val = address_space_ldl_be(&s->as, addr, attrs, NULL);
+        } else {
+            val = address_space_ldl_le(&s->as, addr, attrs, NULL);
+        }
     } else {
-        val = le16_to_cpu(s->data[offset * width]);
+        addr += 1 * ofs16;
+        if (s->big_endian) {
+            val = address_space_lduw_be(&s->as, addr, attrs, NULL);
+        } else {
+            val = address_space_lduw_le(&s->as, addr, attrs, NULL);
+        }
     }
+
     return val;
 }

-static void dp8393x_put(dp8393xState *s, int width, int offset,
-                        uint16_t val)
+static void dp8393x_put(dp8393xState *s,
+                        hwaddr addr, unsigned ofs16, uint16_t val)
 {
-    if (s->big_endian) {
-        if (width == 2) {
-            s->data[offset * 2] = 0;
-            s->data[offset * 2 + 1] = cpu_to_be16(val);
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+
+    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+        addr += 2 * ofs16;
+        if (s->big_endian) {
+            address_space_stl_be(&s->as, addr, val, attrs, NULL);
         } else {
-            s->data[offset] = cpu_to_be16(val);
+            address_space_stl_le(&s->as, addr, val, attrs, NULL);
         }
     } else {
-        if (width == 2) {
-            s->data[offset * 2] = cpu_to_le16(val);
-            s->data[offset * 2 + 1] = 0;
+        addr += 1 * ofs16;
+        if (s->big_endian) {
+            address_space_stw_be(&s->as, addr, val, attrs, NULL);
         } else {
-            s->data[offset] = cpu_to_le16(val);
+            address_space_stw_le(&s->as, addr, val, attrs, NULL);
         }
     }
 }
@@ -313,14 +331,12 @@ static void dp8393x_do_read_rra(dp8393xState *s)
     /* Read memory */
     width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
     size = sizeof(uint16_t) * 4 * width;
-    address_space_read(&s->as, dp8393x_rrp(s),
-                       MEMTXATTRS_UNSPECIFIED, s->data, size);

     /* Update SONIC registers */
-    s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
-    s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1);
-    s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2);
-    s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3);
+    s->regs[SONIC_CRBA0] = dp8393x_get(s, dp8393x_rrp(s), 0);
+    s->regs[SONIC_CRBA1] = dp8393x_get(s, dp8393x_rrp(s), 1);
+    s->regs[SONIC_RBWC0] = dp8393x_get(s, dp8393x_rrp(s), 2);
+    s->regs[SONIC_RBWC1] = dp8393x_get(s, dp8393x_rrp(s), 3);
     trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
                                 s->regs[SONIC_RBWC0],
s->regs[SONIC_RBWC1]);

@@ -416,28 +432,22 @@ static void
dp8393x_do_receiver_disable(dp8393xState *s)
 static void dp8393x_do_transmit_packets(dp8393xState *s)
 {
     NetClientState *nc = qemu_get_queue(s->nic);
-    int width, size;
     int tx_len, len;
     uint16_t i;

-    width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
-
     while (1) {
         /* Read memory */
-        size = sizeof(uint16_t) * 6 * width;
         s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
         trace_dp8393x_transmit_packet(dp8393x_ttda(s));
-        address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) *
width,
-                           MEMTXATTRS_UNSPECIFIED, s->data, size);
         tx_len = 0;

         /* Update registers */
-        s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000;
-        s->regs[SONIC_TPS] = dp8393x_get(s, width, 1);
-        s->regs[SONIC_TFC] = dp8393x_get(s, width, 2);
-        s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3);
-        s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4);
-        s->regs[SONIC_TFS] = dp8393x_get(s, width, 5);
+        s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000;
+        s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1);
+        s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2);
+        s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3);
+        s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4);
+        s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5);

         /* Handle programmable interrupt */
         if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) {
@@ -459,15 +469,9 @@ static void
dp8393x_do_transmit_packets(dp8393xState *s)
             i++;
             if (i != s->regs[SONIC_TFC]) {
                 /* Read next fragment details */
-                size = sizeof(uint16_t) * 3 * width;
-                address_space_read(&s->as,
-                                   dp8393x_ttda(s)
-                                   + sizeof(uint16_t) * width * (4 + 3
* i),
-                                   MEMTXATTRS_UNSPECIFIED, s->data,
-                                   size);
-                s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
-                s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
-                s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
+                s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0);
+                s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1);
+                s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2);
             }
         }

@@ -500,22 +504,12 @@ static void
dp8393x_do_transmit_packets(dp8393xState *s)
         s->regs[SONIC_TCR] |= SONIC_TCR_PTX;

         /* Write status */
-        dp8393x_put(s, width, 0,
-                    s->regs[SONIC_TCR] & 0x0fff); /* status */
-        size = sizeof(uint16_t) * width;
-        address_space_write(&s->as, dp8393x_ttda(s),
-                            MEMTXATTRS_UNSPECIFIED, s->data, size);
+        dp8393x_put(s, dp8393x_ttda(s), 0, s->regs[SONIC_TCR] & 0x0fff);

         if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
             /* Read footer of packet */
-            size = sizeof(uint16_t) * width;
-            address_space_read(&s->as,
-                               dp8393x_ttda(s)
-                               + sizeof(uint16_t) * width
-                                 * (4 + 3 * s->regs[SONIC_TFC]),
-                               MEMTXATTRS_UNSPECIFIED, s->data,
-                               size);
-            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+            s->regs[SONIC_CTDA] = dp8393x_get(s, dp8393x_ttda(s),
+                                               s->regs[SONIC_TFC]);
             if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
                 /* EOL detected */
                 break;
@@ -764,7 +759,7 @@ 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, padded_len;
+    int rx_len, padded_len;
     uint32_t checksum;
     int size;

@@ -777,10 +772,8 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,

     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;
     }

@@ -801,11 +794,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,
     /* Check for EOL */
     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_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
-                           s->data, size);
-        s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
+        s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
         if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
             /* Still EOL ; stop reception */
             return -1;
@@ -813,11 +802,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,
         /* 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);
+        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);

         /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
@@ -846,8 +831,8 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,
     /* 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_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+                            (uint8_t *)"\xFF\xFF\xFF", size);
         address += size;
     }

@@ -871,32 +856,20 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,

     /* Write status to memory */
     trace_dp8393x_receive_write_status(dp8393x_crda(s));
-    dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */
-    dp8393x_put(s, width, 1, rx_len); /* byte count */
-    dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */
-    dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
-    dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
-    size = sizeof(uint16_t) * 5 * width;
-    address_space_write(&s->as, dp8393x_crda(s),
-                        MEMTXATTRS_UNSPECIFIED,
-                        s->data, size);
+    dp8393x_put(s, dp8393x_crda(s), 0, s->regs[SONIC_RCR]); /* status */
+    dp8393x_put(s, dp8393x_crda(s), 1, rx_len); /* byte count */
+    dp8393x_put(s, dp8393x_crda(s), 2, s->regs[SONIC_TRBA0]); /*
pkt_ptr0 */
+    dp8393x_put(s, dp8393x_crda(s), 3, s->regs[SONIC_TRBA1]); /*
pkt_ptr1 */
+    dp8393x_put(s, dp8393x_crda(s), 4, s->regs[SONIC_RSC]); /* seq_no */

     /* Check link field */
-    size = sizeof(uint16_t) * width;
-    address_space_read(&s->as,
-                       dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
-                       MEMTXATTRS_UNSPECIFIED, s->data, size);
-    s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
+    s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
     if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
         /* 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_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
-                            s->data, size);
+        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);

         /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
---

Now I'll look at the CAM then the cksum.

Regards,

Phil.

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

On 03/07/2021 14:10, Philippe Mathieu-Daudé wrote:

> On 7/3/21 2:04 PM, Mark Cave-Ayland wrote:
>> On 03/07/2021 09:52, Philippe Mathieu-Daudé wrote:
>>> On 7/3/21 8:21 AM, Mark Cave-Ayland wrote:
>>>> On 02/07/2021 05:36, Finn Thain wrote:
>>>>
>>>>>> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
>>>>>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that
>>>>>>> all accesses to the registers were 32-bit
>>>>>
>>>>> No, that assumption was not made there. Just take a look at my
>>>>> commits in
>>>>> Linux that make 16-bit accesses. If commit 3fe9a838ec worked by
>>>>> accident,
>>>>> it probably just reflects my inadequate knowledge of QEMU internals.
>>>>>
>>>>>>> but this is actually not the case. The access size is determined by
>>>>>>> the CPU instruction used and not the number of physical address
>>>>>>> lines.
>>>>>>>
>>>>>
>>>>> I think that's an over-simplification (in the context of commit
>>>>> 3fe9a838ec).
>>>>
>>>> Let me try and clarify this a bit more: there are 2 different changes
>>>> incorporated into 3fe9a838ec. The first (as you mention below and also
>>>> detailed in the commit messge), is related to handling writes to the
>>>> upper 16-bits of a word from the device and ensuring that 32-bit
>>>> accesses are handled correctly. This part seems correct to me based upon
>>>> reading the datasheet and the commit message:
>>>>
>>>> @@ -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);
>>>> +        }
>>>>        }
>>>>    }
>>>>
>>>> The second change incorporated into 3fe9a838ec (and the one this patch
>>>> fixes) is a similar change made to the MMIO *register* accesses:
>>>>
>>>> @@ -590,7 +600,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;
>>>>    }
>>>>
>>>> and:
>>>>
>>>> @@ -598,13 +608,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;
>>>>
>>>> This is not correct since the QEMU memory API handles any access size
>>>> and endian conversion before the MMIO access reaches the device. It is
>>>> this change which breaks the 32-bit accesses used by MacOS to read/write
>>>> the dp8393x registers because it applies an additional endian swap on
>>>> top of that already done by the memory API.
>>>>
>>>>>>> The big_endian workaround applied to the register read/writes was
>>>>>>> actually caused by forcing the access size to 32-bit when the
>>>>>>> guest OS
>>>>>>> was using a 16-bit access. Since the registers are 16-bit then we can
>>>>>>> simply set .impl.min_access to 2 and then the memory API will
>>>>>>> automatically do the right thing for both 16-bit accesses used by
>>>>>>> Linux and 32-bit accesses used by the MacOS toolbox ROM.
>>>>>>
>>>>>> Hmm I'm not sure. This sounds to me like the "QEMU doesn't model
>>>>>> busses
>>>>>> so we end using kludge to hide bugs" pattern. Can you provide a QTest
>>>>>> (ideally) or a "-trace memory_region_ops_\*" log of your firmware
>>>>>> accessing the dp8393x please?
>>>>>>
>>>>> The DP83932 chip is highly configurable, so I'm not sure that the
>>>>> behaviour of any given firmware would resolve the question.
>>>>
>>>> Indeed, I don't think that will help much here. Phil, if you would still
>>>> like me to send you some traces after reading the explanation above then
>>>> do let me know.
>>>
>>> I read it and still would have few traces. We can hand-write them too.
>>>
>>> I'd like to add qtests for some read/write,16/32(A)==B.
>>
>> Sigh. I was just about to attempt some traces when I realised looking at
>> the patch that I made a mistake, and that in order for the memory API to
>> automatically handle the 4 byte accesses as 2 x 2 byte accesses then
>> both .impl.min_access_size and .impl.max_access_size need to be set to 2
>> :(  The remainder of the patch is the same but dp8393x_ops now looks
>> like this:
>>
>> static const MemoryRegionOps dp8393x_ops = {
>>      .read = dp8393x_read,
>>      .write = dp8393x_write,
>>      .impl.min_access_size = 2,
>>      .impl.max_access_size = 2,
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>> };
> 
> Yes :) This is closer to what I wrote during my review:
> 
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index bff359ed13f..6061268dc49 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -697,7 +697,9 @@ static const MemoryRegionOps dp8393x_ops = {
>       .read = dp8393x_read,
>       .write = dp8393x_write,
>       .impl.min_access_size = 2,
> -    .impl.max_access_size = 4,
> +    .impl.max_access_size = 2,
> +    .valid.min_access_size = 2,
> +    .valid.max_access_size = 4,
>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };
> ---

Yes, that should work. Sorry I got confused during the initial review.

>> I've tested this under Linux/m68k, NetBSD/arc and MacOS and networking
>> seems fine after a quick test in each OS. The slight curiosity is that
>> the 4 byte accesses used by MacOS are split into 2 x 2 byte accesses,
>> but since MacOS only uses the bottom 16-bit of the result and ignores
>> the top 16-bits then despite there being 2 accesses, everything still
>> works as expected (see below).
>>
>>
>> READ:
>>
>> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
>> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a0 value 0x4
>> size 2
>> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
>> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a2 value 0x4
>> size 2
>> memory_region_ops_read cpu 0 mr 0x55f210c9c330 addr 0x50f0a0a0 value
>> 0x40004 size 4
>>
>> WRITE:
>>
>> memory_region_ops_write cpu 0 mr 0x55f210c9c330 addr 0x50f0a01c value
>> 0x248fe8 size 4
>> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01c value
>> 0x24 size 2
>> dp8393x_write reg=0x7 [CTDA] val=0x0024 size=2
>> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01e value
>> 0x8fe8 size 2
>> dp8393x_write reg=0x7 [CTDA] val=0x8fe8 size=2
>>
>>
>> If you're happy with this, I'll resubmit a revised version of the patch
>> but with an updated commit message: the Fixes: tag is still the same,
>> but the updated fix is to ensure that .impl.min_access_size and
>> .impl.max_access_size match the real-life 16-bit register size.
> 
> Hold on a bit more, I'm still reviewing the datasheet ;)
> 
> My code note so far (untested) are:
> 
> -- >8 --
> @@ -219,34 +225,48 @@ static uint32_t dp8393x_wt(dp8393xState *s)
>       return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
>   }
> 
> -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset)
> +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16)
>   {
> +    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>       uint16_t val;
> 
> -    if (s->big_endian) {
> -        val = be16_to_cpu(s->data[offset * width + width - 1]);
> +    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> +        addr += 2 * ofs16;
> +        if (s->big_endian) {
> +            val = address_space_ldl_be(&s->as, addr, attrs, NULL);
> +        } else {
> +            val = address_space_ldl_le(&s->as, addr, attrs, NULL);
> +        }
>       } else {
> -        val = le16_to_cpu(s->data[offset * width]);
> +        addr += 1 * ofs16;
> +        if (s->big_endian) {
> +            val = address_space_lduw_be(&s->as, addr, attrs, NULL);
> +        } else {
> +            val = address_space_lduw_le(&s->as, addr, attrs, NULL);
> +        }
>       }
> +
>       return val;
>   }
> 
> -static void dp8393x_put(dp8393xState *s, int width, int offset,
> -                        uint16_t val)
> +static void dp8393x_put(dp8393xState *s,
> +                        hwaddr addr, unsigned ofs16, uint16_t val)
>   {
> -    if (s->big_endian) {
> -        if (width == 2) {
> -            s->data[offset * 2] = 0;
> -            s->data[offset * 2 + 1] = cpu_to_be16(val);
> +    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> +
> +    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> +        addr += 2 * ofs16;
> +        if (s->big_endian) {
> +            address_space_stl_be(&s->as, addr, val, attrs, NULL);
>           } else {
> -            s->data[offset] = cpu_to_be16(val);
> +            address_space_stl_le(&s->as, addr, val, attrs, NULL);
>           }
>       } else {
> -        if (width == 2) {
> -            s->data[offset * 2] = cpu_to_le16(val);
> -            s->data[offset * 2 + 1] = 0;
> +        addr += 1 * ofs16;
> +        if (s->big_endian) {
> +            address_space_stw_be(&s->as, addr, val, attrs, NULL);
>           } else {
> -            s->data[offset] = cpu_to_le16(val);
> +            address_space_stw_le(&s->as, addr, val, attrs, NULL);
>           }
>       }
>   }
> @@ -313,14 +331,12 @@ static void dp8393x_do_read_rra(dp8393xState *s)
>       /* Read memory */
>       width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
>       size = sizeof(uint16_t) * 4 * width;
> -    address_space_read(&s->as, dp8393x_rrp(s),
> -                       MEMTXATTRS_UNSPECIFIED, s->data, size);
> 
>       /* Update SONIC registers */
> -    s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> -    s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1);
> -    s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2);
> -    s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3);
> +    s->regs[SONIC_CRBA0] = dp8393x_get(s, dp8393x_rrp(s), 0);
> +    s->regs[SONIC_CRBA1] = dp8393x_get(s, dp8393x_rrp(s), 1);
> +    s->regs[SONIC_RBWC0] = dp8393x_get(s, dp8393x_rrp(s), 2);
> +    s->regs[SONIC_RBWC1] = dp8393x_get(s, dp8393x_rrp(s), 3);
>       trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
>                                   s->regs[SONIC_RBWC0],
> s->regs[SONIC_RBWC1]);
> 
> @@ -416,28 +432,22 @@ static void
> dp8393x_do_receiver_disable(dp8393xState *s)
>   static void dp8393x_do_transmit_packets(dp8393xState *s)
>   {
>       NetClientState *nc = qemu_get_queue(s->nic);
> -    int width, size;
>       int tx_len, len;
>       uint16_t i;
> 
> -    width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
> -
>       while (1) {
>           /* Read memory */
> -        size = sizeof(uint16_t) * 6 * width;
>           s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
>           trace_dp8393x_transmit_packet(dp8393x_ttda(s));
> -        address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) *
> width,
> -                           MEMTXATTRS_UNSPECIFIED, s->data, size);
>           tx_len = 0;
> 
>           /* Update registers */
> -        s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000;
> -        s->regs[SONIC_TPS] = dp8393x_get(s, width, 1);
> -        s->regs[SONIC_TFC] = dp8393x_get(s, width, 2);
> -        s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3);
> -        s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4);
> -        s->regs[SONIC_TFS] = dp8393x_get(s, width, 5);
> +        s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000;
> +        s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1);
> +        s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2);
> +        s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3);
> +        s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4);
> +        s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5);
> 
>           /* Handle programmable interrupt */
>           if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) {
> @@ -459,15 +469,9 @@ static void
> dp8393x_do_transmit_packets(dp8393xState *s)
>               i++;
>               if (i != s->regs[SONIC_TFC]) {
>                   /* Read next fragment details */
> -                size = sizeof(uint16_t) * 3 * width;
> -                address_space_read(&s->as,
> -                                   dp8393x_ttda(s)
> -                                   + sizeof(uint16_t) * width * (4 + 3
> * i),
> -                                   MEMTXATTRS_UNSPECIFIED, s->data,
> -                                   size);
> -                s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> -                s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> -                s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> +                s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0);
> +                s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1);
> +                s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2);
>               }
>           }
> 
> @@ -500,22 +504,12 @@ static void
> dp8393x_do_transmit_packets(dp8393xState *s)
>           s->regs[SONIC_TCR] |= SONIC_TCR_PTX;
> 
>           /* Write status */
> -        dp8393x_put(s, width, 0,
> -                    s->regs[SONIC_TCR] & 0x0fff); /* status */
> -        size = sizeof(uint16_t) * width;
> -        address_space_write(&s->as, dp8393x_ttda(s),
> -                            MEMTXATTRS_UNSPECIFIED, s->data, size);
> +        dp8393x_put(s, dp8393x_ttda(s), 0, s->regs[SONIC_TCR] & 0x0fff);
> 
>           if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
>               /* Read footer of packet */
> -            size = sizeof(uint16_t) * width;
> -            address_space_read(&s->as,
> -                               dp8393x_ttda(s)
> -                               + sizeof(uint16_t) * width
> -                                 * (4 + 3 * s->regs[SONIC_TFC]),
> -                               MEMTXATTRS_UNSPECIFIED, s->data,
> -                               size);
> -            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> +            s->regs[SONIC_CTDA] = dp8393x_get(s, dp8393x_ttda(s),
> +                                               s->regs[SONIC_TFC]);
>               if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
>                   /* EOL detected */
>                   break;
> @@ -764,7 +759,7 @@ 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, padded_len;
> +    int rx_len, padded_len;
>       uint32_t checksum;
>       int size;
> 
> @@ -777,10 +772,8 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
> 
>       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;
>       }
> 
> @@ -801,11 +794,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
>       /* Check for EOL */
>       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_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> -                           s->data, size);
> -        s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> +        s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
>           if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>               /* Still EOL ; stop reception */
>               return -1;
> @@ -813,11 +802,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
>           /* 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);
> +        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
> 
>           /* Move to next descriptor */
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -846,8 +831,8 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
>       /* 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_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> +                            (uint8_t *)"\xFF\xFF\xFF", size);
>           address += size;
>       }
> 
> @@ -871,32 +856,20 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
> 
>       /* Write status to memory */
>       trace_dp8393x_receive_write_status(dp8393x_crda(s));
> -    dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */
> -    dp8393x_put(s, width, 1, rx_len); /* byte count */
> -    dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */
> -    dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> -    dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> -    size = sizeof(uint16_t) * 5 * width;
> -    address_space_write(&s->as, dp8393x_crda(s),
> -                        MEMTXATTRS_UNSPECIFIED,
> -                        s->data, size);
> +    dp8393x_put(s, dp8393x_crda(s), 0, s->regs[SONIC_RCR]); /* status */
> +    dp8393x_put(s, dp8393x_crda(s), 1, rx_len); /* byte count */
> +    dp8393x_put(s, dp8393x_crda(s), 2, s->regs[SONIC_TRBA0]); /*
> pkt_ptr0 */
> +    dp8393x_put(s, dp8393x_crda(s), 3, s->regs[SONIC_TRBA1]); /*
> pkt_ptr1 */
> +    dp8393x_put(s, dp8393x_crda(s), 4, s->regs[SONIC_RSC]); /* seq_no */
> 
>       /* Check link field */
> -    size = sizeof(uint16_t) * width;
> -    address_space_read(&s->as,
> -                       dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> -                       MEMTXATTRS_UNSPECIFIED, s->data, size);
> -    s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> +    s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
>       if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>           /* EOL detected */
>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>       } else {
>           /* 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_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> -                            s->data, size);
> +        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
> 
>           /* Move to next descriptor */
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> ---
> 
> Now I'll look at the CAM then the cksum.

I see, so you're also looking to consolidate all the address space accesses via the 
dp8393x_get() and dp8393x_put() functions instead of using s->data as an 
intermediatery - that makes sense to me. Once you have a patch that works for MIPS 
let me know and I can give a test on Linux/m68k and MacOS :)


ATB,

Mark.

Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 7/3/21 4:16 PM, Mark Cave-Ayland wrote:
> On 03/07/2021 14:10, Philippe Mathieu-Daudé wrote:
>> On 7/3/21 2:04 PM, Mark Cave-Ayland wrote:

>>> I've tested this under Linux/m68k, NetBSD/arc and MacOS and networking
>>> seems fine after a quick test in each OS. The slight curiosity is that
>>> the 4 byte accesses used by MacOS are split into 2 x 2 byte accesses,
>>> but since MacOS only uses the bottom 16-bit of the result and ignores
>>> the top 16-bits then despite there being 2 accesses, everything still
>>> works as expected (see below).
>>>
>>>
>>> READ:
>>>
>>> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
>>> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a0 value 0x4
>>> size 2
>>> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
>>> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a2 value 0x4
>>> size 2
>>> memory_region_ops_read cpu 0 mr 0x55f210c9c330 addr 0x50f0a0a0 value
>>> 0x40004 size 4
>>>
>>> WRITE:
>>>
>>> memory_region_ops_write cpu 0 mr 0x55f210c9c330 addr 0x50f0a01c value
>>> 0x248fe8 size 4
>>> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01c value
>>> 0x24 size 2
>>> dp8393x_write reg=0x7 [CTDA] val=0x0024 size=2
>>> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01e value
>>> 0x8fe8 size 2
>>> dp8393x_write reg=0x7 [CTDA] val=0x8fe8 size=2
>>>
>>>
>>> If you're happy with this, I'll resubmit a revised version of the patch
>>> but with an updated commit message: the Fixes: tag is still the same,
>>> but the updated fix is to ensure that .impl.min_access_size and
>>> .impl.max_access_size match the real-life 16-bit register size.
>>
>> Hold on a bit more, I'm still reviewing the datasheet ;)
>>
>> My code note so far (untested) are:
>>
>> -- >8 --
>> @@ -219,34 +225,48 @@ static uint32_t dp8393x_wt(dp8393xState *s)
>>       return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
>>   }
>>
>> -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset)
>> +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned
>> ofs16)
>>   {
>> +    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>>       uint16_t val;
>>
>> -    if (s->big_endian) {
>> -        val = be16_to_cpu(s->data[offset * width + width - 1]);
>> +    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
>> +        addr += 2 * ofs16;
>> +        if (s->big_endian) {
>> +            val = address_space_ldl_be(&s->as, addr, attrs, NULL);
>> +        } else {
>> +            val = address_space_ldl_le(&s->as, addr, attrs, NULL);
>> +        }
>>       } else {
>> -        val = le16_to_cpu(s->data[offset * width]);
>> +        addr += 1 * ofs16;
>> +        if (s->big_endian) {
>> +            val = address_space_lduw_be(&s->as, addr, attrs, NULL);
>> +        } else {
>> +            val = address_space_lduw_le(&s->as, addr, attrs, NULL);
>> +        }
>>       }
>> +
>>       return val;
>>   }

>>       /* Check link field */
>> -    size = sizeof(uint16_t) * width;
>> -    address_space_read(&s->as,
>> -                       dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
>> -                       MEMTXATTRS_UNSPECIFIED, s->data, size);
>> -    s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
>> +    s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
>>       if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>>           /* EOL detected */
>>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>>       } else {
>>           /* 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_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
>> -                            s->data, size);
>> +        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
>>
>>           /* Move to next descriptor */
>>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>> ---
>>
>> Now I'll look at the CAM then the cksum.
> 
> I see, so you're also looking to consolidate all the address space
> accesses via the dp8393x_get() and dp8393x_put() functions instead of
> using s->data as an intermediatery - that makes sense to me. Once you
> have a patch that works for MIPS let me know and I can give a test on
> Linux/m68k and MacOS :)

Done and sent. Thanks for the testing :)

[PATCH v2 09/10] dp8393x: fix CAM descriptor entry index

[Mark Cave-Ayland]

Currently when a LOAD CAM command is executed the entries are loaded into the
CAM from memory in order which is incorrect. According to the datasheet the
first entry in the CAM descriptor is the entry index which means that each
descriptor may update any single entry in the CAM rather than the Nth entry.

Decode the CAM entry index and use it store the descriptor in the appropriate
slot in the CAM. This fixes the issue where the MacOS toolbox loads a single
CAM descriptor into the final slot in order to perform a loopback test which
must succeed before the Ethernet port is enabled.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/dp8393x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 6789bcd3af..172fd06694 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -270,7 +270,7 @@ static void dp8393x_update_irq(dp8393xState *s)
 static void dp8393x_do_load_cam(dp8393xState *s)
 {
     int width, size;
-    uint16_t index = 0;
+    uint16_t index;
 
     width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
     size = sizeof(uint16_t) * 4 * width;
@@ -279,6 +279,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
         /* Fill current entry */
         address_space_read(&s->as, dp8393x_cdp(s),
                            MEMTXATTRS_UNSPECIFIED, s->data, size);
+        index = dp8393x_get(s, width, 0) & 0xf;
         s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
         s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
         s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
@@ -291,7 +292,6 @@ static void dp8393x_do_load_cam(dp8393xState *s)
         /* Move to next entry */
         s->regs[SONIC_CDC]--;
         s->regs[SONIC_CDP] += size;
-        index++;
     }
 
     /* Read CAM enable */
-- 
2.20.1

Re: [PATCH v2 09/10] dp8393x: fix CAM descriptor entry index

[Philippe Mathieu-Daudé]

On 6/25/21 8:54 AM, Mark Cave-Ayland wrote:
> Currently when a LOAD CAM command is executed the entries are loaded into the
> CAM from memory in order which is incorrect. According to the datasheet the
> first entry in the CAM descriptor is the entry index which means that each
> descriptor may update any single entry in the CAM rather than the Nth entry.
> 
> Decode the CAM entry index and use it store the descriptor in the appropriate
> slot in the CAM. This fixes the issue where the MacOS toolbox loads a single
> CAM descriptor into the final slot in order to perform a loopback test which
> must succeed before the Ethernet port is enabled.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/dp8393x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 09/10] dp8393x: fix CAM descriptor entry index

[Philippe Mathieu-Daudé]

On 6/25/21 8:54 AM, Mark Cave-Ayland wrote:
> Currently when a LOAD CAM command is executed the entries are loaded into the
> CAM from memory in order which is incorrect. According to the datasheet the
> first entry in the CAM descriptor is the entry index which means that each
> descriptor may update any single entry in the CAM rather than the Nth entry.
> 
> Decode the CAM entry index and use it store the descriptor in the appropriate
> slot in the CAM. This fixes the issue where the MacOS toolbox loads a single
> CAM descriptor into the final slot in order to perform a loopback test which
> must succeed before the Ethernet port is enabled.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/dp8393x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Patch queued to mips-next.

[PATCH v2 10/10] hw/mips/jazz: specify correct endian for dp8393x device

[Mark Cave-Ayland]

The MIPS magnum machines are available in both big endian (mips64) and little
endian (mips64el) configurations. Ensure that the dp893x big_endian property
is set accordingly using logic similar to that used for the MIPS malta
machines.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/mips/jazz.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 89ca8bb910..ee1789183e 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -126,7 +126,7 @@ static void mips_jazz_init(MachineState *machine,
 {
     MemoryRegion *address_space = get_system_memory();
     char *filename;
-    int bios_size, n;
+    int bios_size, n, big_endian;
     Clock *cpuclk;
     MIPSCPU *cpu;
     MIPSCPUClass *mcc;
@@ -158,6 +158,12 @@ static void mips_jazz_init(MachineState *machine,
         [JAZZ_PICA61] = {33333333, 4},
     };
 
+#ifdef TARGET_WORDS_BIGENDIAN
+    big_endian = 1;
+#else
+    big_endian = 0;
+#endif
+
     if (machine->ram_size > 256 * MiB) {
         error_report("RAM size more than 256Mb is not supported");
         exit(EXIT_FAILURE);
@@ -290,6 +296,7 @@ static void mips_jazz_init(MachineState *machine,
             dev = qdev_new("dp8393x");
             qdev_set_nic_properties(dev, nd);
             qdev_prop_set_uint8(dev, "it_shift", 2);
+            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
             object_property_set_link(OBJECT(dev), "dma_mr",
                                      OBJECT(rc4030_dma_mr), &error_abort);
             sysbus = SYS_BUS_DEVICE(dev);
-- 
2.20.1

Re: [PATCH v2 10/10] hw/mips/jazz: specify correct endian for dp8393x device

[Philippe Mathieu-Daudé]

On 6/25/21 8:54 AM, Mark Cave-Ayland wrote:
> The MIPS magnum machines are available in both big endian (mips64) and little
> endian (mips64el) configurations. Ensure that the dp893x big_endian property
> is set accordingly using logic similar to that used for the MIPS malta
> machines.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/mips/jazz.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> index 89ca8bb910..ee1789183e 100644
> --- a/hw/mips/jazz.c
> +++ b/hw/mips/jazz.c
> @@ -126,7 +126,7 @@ static void mips_jazz_init(MachineState *machine,
>  {
>      MemoryRegion *address_space = get_system_memory();
>      char *filename;
> -    int bios_size, n;
> +    int bios_size, n, big_endian;

Why not use a boolean directly?

>      Clock *cpuclk;
>      MIPSCPU *cpu;
>      MIPSCPUClass *mcc;
> @@ -158,6 +158,12 @@ static void mips_jazz_init(MachineState *machine,
>          [JAZZ_PICA61] = {33333333, 4},
>      };
>  
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    big_endian = 1;
> +#else
> +    big_endian = 0;
> +#endif
> +
>      if (machine->ram_size > 256 * MiB) {
>          error_report("RAM size more than 256Mb is not supported");
>          exit(EXIT_FAILURE);
> @@ -290,6 +296,7 @@ static void mips_jazz_init(MachineState *machine,
>              dev = qdev_new("dp8393x");
>              qdev_set_nic_properties(dev, nd);
>              qdev_prop_set_uint8(dev, "it_shift", 2);
> +            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
>              object_property_set_link(OBJECT(dev), "dma_mr",
>                                       OBJECT(rc4030_dma_mr), &error_abort);
>              sysbus = SYS_BUS_DEVICE(dev);
> 

Re: [PATCH v2 10/10] hw/mips/jazz: specify correct endian for dp8393x device

[Mark Cave-Ayland]

On 25/06/2021 09:51, Philippe Mathieu-Daudé wrote:
> On 6/25/21 8:54 AM, Mark Cave-Ayland wrote:
>> The MIPS magnum machines are available in both big endian (mips64) and little
>> endian (mips64el) configurations. Ensure that the dp893x big_endian property
>> is set accordingly using logic similar to that used for the MIPS malta
>> machines.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/mips/jazz.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
>> index 89ca8bb910..ee1789183e 100644
>> --- a/hw/mips/jazz.c
>> +++ b/hw/mips/jazz.c
>> @@ -126,7 +126,7 @@ static void mips_jazz_init(MachineState *machine,
>>   {
>>       MemoryRegion *address_space = get_system_memory();
>>       char *filename;
>> -    int bios_size, n;
>> +    int bios_size, n, big_endian;
> 
> Why not use a boolean directly?

Good point. I grepped the codebase for an existing example for using DEFINE_PROP_BOOL 
and setting the value using qdev_prop_set_bit(), and the first hit was in 
hw/arm/allwinner-h3.c for the "start-powered-off" property. The existing MIPS Malta 
code also used an integer variable to store the current endian and so that's what I 
went with.

I wonder why we don't have a qdev_prop_set_bool() to match DEFINE_PROP_BOOL?

>>       Clock *cpuclk;
>>       MIPSCPU *cpu;
>>       MIPSCPUClass *mcc;
>> @@ -158,6 +158,12 @@ static void mips_jazz_init(MachineState *machine,
>>           [JAZZ_PICA61] = {33333333, 4},
>>       };
>>   
>> +#ifdef TARGET_WORDS_BIGENDIAN
>> +    big_endian = 1;
>> +#else
>> +    big_endian = 0;
>> +#endif
>> +
>>       if (machine->ram_size > 256 * MiB) {
>>           error_report("RAM size more than 256Mb is not supported");
>>           exit(EXIT_FAILURE);
>> @@ -290,6 +296,7 @@ static void mips_jazz_init(MachineState *machine,
>>               dev = qdev_new("dp8393x");
>>               qdev_set_nic_properties(dev, nd);
>>               qdev_prop_set_uint8(dev, "it_shift", 2);
>> +            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
>>               object_property_set_link(OBJECT(dev), "dma_mr",
>>                                        OBJECT(rc4030_dma_mr), &error_abort);
>>               sysbus = SYS_BUS_DEVICE(dev);
>>


ATB,

Mark.

Re: [PATCH v2 10/10] hw/mips/jazz: specify correct endian for dp8393x device

[Philippe Mathieu-Daudé]

On 6/25/21 8:54 AM, Mark Cave-Ayland wrote:
> The MIPS magnum machines are available in both big endian (mips64) and little
> endian (mips64el) configurations. Ensure that the dp893x big_endian property
> is set accordingly using logic similar to that used for the MIPS malta
> machines.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/mips/jazz.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 00/10] dp8393x: fixes for MacOS toolbox ROM

[Finn Thain]

On Fri, 25 Jun 2021, Mark Cave-Ayland wrote:

> Here is the next set of patches from my attempts to boot MacOS under 
> QEMU's Q800 machine related to the Sonic network adapter.
> 
> Patches 1 and 2 sort out checkpatch and convert from DPRINTF macros to 
> trace-events.
> 
> The discussion for the v1 patchset concluded that the dp8393x device 
> does NOT have its own NVRAM (there is no mention of it on the datasheet) 
> and so patches 3 to 5 move the generation of the PROM to the q800 and 
> jazz boards separately to allow the formats to diverge.
> 
> Patch 6 adds an implementation of bitrev8 to bitops.h in preparation for 
> changing the q800 PROM storage format, whilst patch 7 updates the MAC 
> address storage and checksum for the q800 machine to match the format 
> expected by the MacOS toolbox ROM.
> 
> Patch 8 ensures that the CPU loads/stores are correctly converted to 
> 16-bit accesses for the network card and patch 9 fixes a bug when 
> selecting the index specified for CAM entries.
> 
> Finally since the MIPS magnum machine exists for both big-endian 
> (mips64) and little-endian (mips64el) configurations, patch 10 sets the 
> dp8393x big_endian property accordingly using a similar technique 
> already used for the MIPS malta machines.
> 
> Migration notes: the changes to the dp8393x PROM are a migration break, 
> but we don't care about this for now since a) the q800 machine will have 
> more breaking migration changes as further MacOS toolbox ROM support is 
> upstreamed and b) the magnum machine migration is currently broken (and 
> has been for quite some time).
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 

Tested-by: Finn Thain <fthain@linux-m68k.org>

My testing was much the same as last time: 'qemu-system-mips64el -M 
magnum' with Linux/mips and NetBSD/arc, and 'qemu-system-m68k -M q800' 
with Linux/m68k. In each case I checked the MAC address against the '-nic' 
option. The host is little-endian.

I have not tested 'qemu-system-mips64 -M magnum'. It appears MIPS RISC/os 
is needed for that but I don't have it.

> 
> v2:
> - Move PROM generation from dp8393x to q800 and magnum machines and remove
>   the existing code from the device itself
> - Add bitrev8 implementation to bitops.h so it can be used elsewhere in
>   future. Use a shift/merge technique rather than a massive table lookup
>   as we don't care about speed
> - Add patch to set the big_endian property correctly depending upon whether
>   a big-endian or little-endian configuration is being used
> 
> 
> Mark Cave-Ayland (10):
>   dp8393x: checkpatch fixes
>   dp8393x: convert to trace-events
>   hw/mips/jazz: move PROM and checksum calculation from dp8393x device
>     to board
>   hw/m68k/q800: move PROM and checksum calculation from dp8393x device
>     to board
>   dp8393x: remove onboard PROM containing MAC address and checksum
>   qemu/bitops.h: add bitrev8 implementation
>   hw/m68k/q800: fix PROM checksum and MAC address storage
>   dp8393x: don't force 32-bit register access
>   dp8393x: fix CAM descriptor entry index
>   hw/mips/jazz: specify correct endian for dp8393x device
> 
>  hw/m68k/q800.c        |  21 ++-
>  hw/mips/jazz.c        |  32 ++++-
>  hw/net/dp8393x.c      | 313 +++++++++++++++++++-----------------------
>  hw/net/trace-events   |  17 +++
>  include/qemu/bitops.h |  22 +++
>  5 files changed, 231 insertions(+), 174 deletions(-)
> 
> 

Re: [PATCH v2 00/10] dp8393x: fixes for MacOS toolbox ROM

[Philippe Mathieu-Daudé]

Hi Mark,

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> Here is the next set of patches from my attempts to boot MacOS under QEMU's
> Q800 machine related to the Sonic network adapter.
> 
> Patches 1 and 2 sort out checkpatch and convert from DPRINTF macros to
> trace-events.
> 
> The discussion for the v1 patchset concluded that the dp8393x device does
> NOT have its own NVRAM (there is no mention of it on the datasheet) and so
> patches 3 to 5 move the generation of the PROM to the q800 and jazz boards
> separately to allow the formats to diverge.
> 
> Patch 6 adds an implementation of bitrev8 to bitops.h in preparation for
> changing the q800 PROM storage format, whilst patch 7 updates the MAC address
> storage and checksum for the q800 machine to match the format expected by the
> MacOS toolbox ROM.
> 
> Patch 8 ensures that the CPU loads/stores are correctly converted to 16-bit
> accesses for the network card and patch 9 fixes a bug when selecting the
> index specified for CAM entries.
> 
> Finally since the MIPS magnum machine exists for both big-endian (mips64) and
> little-endian (mips64el) configurations, patch 10 sets the dp8393x big_endian
> property accordingly using a similar technique already used for the MIPS malta
> machines.
> 
> Migration notes: the changes to the dp8393x PROM are a migration break, but we
> don't care about this for now since a) the q800 machine will have more
> breaking migration changes as further MacOS toolbox ROM support is upstreamed
> and b) the magnum machine migration is currently broken (and has been for
> quite some time).
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> v2:
> - Move PROM generation from dp8393x to q800 and magnum machines and remove
>   the existing code from the device itself
> - Add bitrev8 implementation to bitops.h so it can be used elsewhere in
>   future. Use a shift/merge technique rather than a massive table lookup
>   as we don't care about speed
> - Add patch to set the big_endian property correctly depending upon whether
>   a big-endian or little-endian configuration is being used
> 
> 
> Mark Cave-Ayland (10):
>   dp8393x: checkpatch fixes
>   dp8393x: convert to trace-events
>   hw/mips/jazz: move PROM and checksum calculation from dp8393x device
>     to board
>   hw/m68k/q800: move PROM and checksum calculation from dp8393x device
>     to board
>   dp8393x: remove onboard PROM containing MAC address and checksum
>   qemu/bitops.h: add bitrev8 implementation
>   hw/m68k/q800: fix PROM checksum and MAC address storage
>   dp8393x: don't force 32-bit register access
>   dp8393x: fix CAM descriptor entry index
>   hw/mips/jazz: specify correct endian for dp8393x device

Since a MIPS machine is involved, I'm queuing patches 1-7,10
(PROM cksum) to mips-next. I'm leaving 8-9 for further discussion
after seeing the guest memory traces.

Regards,

Phil.

Re: [PATCH v2 00/10] dp8393x: fixes for MacOS toolbox ROM

[Mark Cave-Ayland]

On 02/07/2021 14:03, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
>> Here is the next set of patches from my attempts to boot MacOS under QEMU's
>> Q800 machine related to the Sonic network adapter.
>>
>> Patches 1 and 2 sort out checkpatch and convert from DPRINTF macros to
>> trace-events.
>>
>> The discussion for the v1 patchset concluded that the dp8393x device does
>> NOT have its own NVRAM (there is no mention of it on the datasheet) and so
>> patches 3 to 5 move the generation of the PROM to the q800 and jazz boards
>> separately to allow the formats to diverge.
>>
>> Patch 6 adds an implementation of bitrev8 to bitops.h in preparation for
>> changing the q800 PROM storage format, whilst patch 7 updates the MAC address
>> storage and checksum for the q800 machine to match the format expected by the
>> MacOS toolbox ROM.
>>
>> Patch 8 ensures that the CPU loads/stores are correctly converted to 16-bit
>> accesses for the network card and patch 9 fixes a bug when selecting the
>> index specified for CAM entries.
>>
>> Finally since the MIPS magnum machine exists for both big-endian (mips64) and
>> little-endian (mips64el) configurations, patch 10 sets the dp8393x big_endian
>> property accordingly using a similar technique already used for the MIPS malta
>> machines.
>>
>> Migration notes: the changes to the dp8393x PROM are a migration break, but we
>> don't care about this for now since a) the q800 machine will have more
>> breaking migration changes as further MacOS toolbox ROM support is upstreamed
>> and b) the magnum machine migration is currently broken (and has been for
>> quite some time).
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>>
>> v2:
>> - Move PROM generation from dp8393x to q800 and magnum machines and remove
>>    the existing code from the device itself
>> - Add bitrev8 implementation to bitops.h so it can be used elsewhere in
>>    future. Use a shift/merge technique rather than a massive table lookup
>>    as we don't care about speed
>> - Add patch to set the big_endian property correctly depending upon whether
>>    a big-endian or little-endian configuration is being used
>>
>>
>> Mark Cave-Ayland (10):
>>    dp8393x: checkpatch fixes
>>    dp8393x: convert to trace-events
>>    hw/mips/jazz: move PROM and checksum calculation from dp8393x device
>>      to board
>>    hw/m68k/q800: move PROM and checksum calculation from dp8393x device
>>      to board
>>    dp8393x: remove onboard PROM containing MAC address and checksum
>>    qemu/bitops.h: add bitrev8 implementation
>>    hw/m68k/q800: fix PROM checksum and MAC address storage
>>    dp8393x: don't force 32-bit register access
>>    dp8393x: fix CAM descriptor entry index
>>    hw/mips/jazz: specify correct endian for dp8393x device
> 
> Since a MIPS machine is involved, I'm queuing patches 1-7,10
> (PROM cksum) to mips-next. I'm leaving 8-9 for further discussion
> after seeing the guest memory traces.

Thanks for picking these up - I've been AFK for the past week :)

What was the issue with patch 9 "dp8393x: fix CAM descriptor entry index"? That patch 
ensures that the CAM index is read from the descriptor, and not taken from the for() 
loop i.e. it is unrelated to register access size. See section 4.1.1 "The Load CAM 
Command" in the DP83932C datasheet for more information.


ATB,

Mark.

Re: [PATCH v2 00/10] dp8393x: fixes for MacOS toolbox ROM

[Philippe Mathieu-Daudé]

On 7/3/21 8:32 AM, Mark Cave-Ayland wrote:
> On 02/07/2021 14:03, Philippe Mathieu-Daudé wrote:

> What was the issue with patch 9 "dp8393x: fix CAM descriptor entry
> index"? That patch ensures that the CAM index is read from the
> descriptor, and not taken from the for() loop i.e. it is unrelated to
> register access size.

No issue, simply nobody reviewed it and I was not confident enough.

> See section 4.1.1 "The Load CAM Command" in the
> DP83932C datasheet for more information.

I'll have a look. The patch could go via Jason's tree or Trivial
then.