[PATCH v3 0/8] dp8393x: fixes and improvements

[PATCH v3 0/8] dp8393x: fixes and improvements

[Philippe Mathieu-Daudé]

Hi Mark, Finn.

This respin aims go group all the fixes sent/suggested on the list
the last weeks around the dp8393x device.

Mark, can you send your S-o-b for patches 4 & 6?

The last 2 patches are included for Mark, but I don't plan to merge
them without Finn's Ack, and apparently they require more work.

Tested with NetBSD guest on Magnum Jazz.

Based-on mips-next.

Mark Cave-Ayland (1):
  NOTFORMERGE dp8393x: don't force 32-bit register access

Philippe Mathieu-Daudé (7):
  dp8393x: Replace address_space_rw(is_write=1) by address_space_write()
  dp8393x: Replace 0x40 magic value by SONIC_REG16_COUNT definition
  dp8393x: Only shift the device registers mapping by 1 bit
  dp8393x: Store CAM registers as 16-bit
  dp8393x: Migrate registers as array of uint16
  dp8393x: Store CRC using device configured endianess
  NOTFORMERGE dp8393x: Rewrite dp8393x_get() / dp8393x_put()

 hw/m68k/q800.c   |   2 +-
 hw/mips/jazz.c   |   2 +-
 hw/net/dp8393x.c | 219 +++++++++++++++++++++--------------------------
 3 files changed, 99 insertions(+), 124 deletions(-)

-- 
2.31.1

[PATCH v3 1/8] dp8393x: Replace address_space_rw(is_write=1) by address_space_write()

[Philippe Mathieu-Daudé]

Replace address_space_rw(is_write=1) by address_space_write()
and remove pointless cast.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 11810c9b600..9118364aa33 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -816,8 +816,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         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);
+        address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+                            s->data, size);
 
         /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
@@ -846,8 +846,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,
+                            "\xFF\xFF\xFF", size);
         address += size;
     }
 
-- 
2.31.1

Re: [PATCH v3 1/8] dp8393x: Replace address_space_rw(is_write=1) by address_space_write()

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> Replace address_space_rw(is_write=1) by address_space_write()
> and remove pointless cast.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b600..9118364aa33 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -816,8 +816,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>           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);
> +        address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> +                            s->data, size);
>   
>           /* Move to next descriptor */
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -846,8 +846,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,
> +                            "\xFF\xFF\xFF", size);
>           address += size;
>       }

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


ATB,

Mark.

[PATCH v3 2/8] dp8393x: Replace 0x40 magic value by SONIC_REG16_COUNT definition

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 9118364aa33..d1e147a82a6 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -85,6 +85,7 @@ static const char *reg_names[] = {
 #define SONIC_MPT    0x2e
 #define SONIC_MDT    0x2f
 #define SONIC_DCR2   0x3f
+#define SONIC_REG_COUNT  0x40
 
 #define SONIC_CR_HTX     0x0001
 #define SONIC_CR_TXP     0x0002
@@ -158,7 +159,7 @@ struct dp8393xState {
 
     /* Registers */
     uint8_t cam[16][6];
-    uint16_t regs[0x40];
+    uint16_t regs[SONIC_REG_COUNT];
 
     /* Temporaries */
     uint8_t tx_buffer[0x10000];
@@ -972,7 +973,7 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
 
     address_space_init(&s->as, s->dma_mr, "dp8393x");
     memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
-                          "dp8393x-regs", 0x40 << s->it_shift);
+                          "dp8393x-regs", SONIC_REG_COUNT << s->it_shift);
 
     s->nic = qemu_new_nic(&net_dp83932_info, &s->conf,
                           object_get_typename(OBJECT(dev)), dev->id, s);
@@ -987,7 +988,7 @@ static const VMStateDescription vmstate_dp8393x = {
     .minimum_version_id = 0,
     .fields = (VMStateField []) {
         VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
-        VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
+        VMSTATE_UINT16_ARRAY(regs, dp8393xState, SONIC_REG_COUNT),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.31.1

Re: [PATCH v3 2/8] dp8393x: Replace 0x40 magic value by SONIC_REG16_COUNT definition

[Philippe Mathieu-Daudé]

Typo 'SONIC_REG_COUNT' in subject.

On Sat, Jul 10, 2021 at 7:50 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/net/dp8393x.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 9118364aa33..d1e147a82a6 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -85,6 +85,7 @@ static const char *reg_names[] = {
>  #define SONIC_MPT    0x2e
>  #define SONIC_MDT    0x2f
>  #define SONIC_DCR2   0x3f
> +#define SONIC_REG_COUNT  0x40
>
>  #define SONIC_CR_HTX     0x0001
>  #define SONIC_CR_TXP     0x0002
> @@ -158,7 +159,7 @@ struct dp8393xState {
>
>      /* Registers */
>      uint8_t cam[16][6];
> -    uint16_t regs[0x40];
> +    uint16_t regs[SONIC_REG_COUNT];
>
>      /* Temporaries */
>      uint8_t tx_buffer[0x10000];
> @@ -972,7 +973,7 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>
>      address_space_init(&s->as, s->dma_mr, "dp8393x");
>      memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
> -                          "dp8393x-regs", 0x40 << s->it_shift);
> +                          "dp8393x-regs", SONIC_REG_COUNT << s->it_shift);
>
>      s->nic = qemu_new_nic(&net_dp83932_info, &s->conf,
>                            object_get_typename(OBJECT(dev)), dev->id, s);
> @@ -987,7 +988,7 @@ static const VMStateDescription vmstate_dp8393x = {
>      .minimum_version_id = 0,
>      .fields = (VMStateField []) {
>          VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
> -        VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
> +        VMSTATE_UINT16_ARRAY(regs, dp8393xState, SONIC_REG_COUNT),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> --
> 2.31.1
>

Re: [PATCH v3 2/8] dp8393x: Replace 0x40 magic value by SONIC_REG16_COUNT definition

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 9118364aa33..d1e147a82a6 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -85,6 +85,7 @@ static const char *reg_names[] = {
>   #define SONIC_MPT    0x2e
>   #define SONIC_MDT    0x2f
>   #define SONIC_DCR2   0x3f
> +#define SONIC_REG_COUNT  0x40
>   
>   #define SONIC_CR_HTX     0x0001
>   #define SONIC_CR_TXP     0x0002
> @@ -158,7 +159,7 @@ struct dp8393xState {
>   
>       /* Registers */
>       uint8_t cam[16][6];
> -    uint16_t regs[0x40];
> +    uint16_t regs[SONIC_REG_COUNT];
>   
>       /* Temporaries */
>       uint8_t tx_buffer[0x10000];
> @@ -972,7 +973,7 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>   
>       address_space_init(&s->as, s->dma_mr, "dp8393x");
>       memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
> -                          "dp8393x-regs", 0x40 << s->it_shift);
> +                          "dp8393x-regs", SONIC_REG_COUNT << s->it_shift);
>   
>       s->nic = qemu_new_nic(&net_dp83932_info, &s->conf,
>                             object_get_typename(OBJECT(dev)), dev->id, s);
> @@ -987,7 +988,7 @@ static const VMStateDescription vmstate_dp8393x = {
>       .minimum_version_id = 0,
>       .fields = (VMStateField []) {
>           VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
> -        VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
> +        VMSTATE_UINT16_ARRAY(regs, dp8393xState, SONIC_REG_COUNT),
>           VMSTATE_END_OF_LIST()
>       }
>   };

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


ATB,

Mark.

[PATCH v3 3/8] dp8393x: Only shift the device registers mapping by 1 bit

[Philippe Mathieu-Daudé]

The SONIC device only allows 16/32-bit accesses. From the machine
view (from the bus), it is only shifted by 1 bit. Another bit is
shifted, but it is an implementation detail of the QEMU model.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/m68k/q800.c   | 2 +-
 hw/mips/jazz.c   | 2 +-
 hw/net/dp8393x.c | 2 ++
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 6817c8b5d1a..d78edfe40e8 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -316,7 +316,7 @@ static void q800_init(MachineState *machine)
 
     dev = qdev_new("dp8393x");
     qdev_set_nic_properties(dev, &nd_table[0]);
-    qdev_prop_set_uint8(dev, "it_shift", 2);
+    qdev_prop_set_uint8(dev, "it_shift", 1);
     qdev_prop_set_bit(dev, "big_endian", true);
     object_property_set_link(OBJECT(dev), "dma_mr",
                              OBJECT(get_system_memory()), &error_abort);
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index d6183e18821..7f8680a189d 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -295,7 +295,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_uint8(dev, "it_shift", 1);
             qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
             object_property_set_link(OBJECT(dev), "dma_mr",
                                      OBJECT(rc4030_dma_mr), &error_abort);
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index d1e147a82a6..64f3b0fc3ea 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -971,6 +971,8 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
 {
     dp8393xState *s = DP8393X(dev);
 
+    s->it_shift += 1; /* Registers are 16-bit wide */
+
     address_space_init(&s->as, s->dma_mr, "dp8393x");
     memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
                           "dp8393x-regs", SONIC_REG_COUNT << s->it_shift);
-- 
2.31.1

Re: [PATCH v3 3/8] dp8393x: Only shift the device registers mapping by 1 bit

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> The SONIC device only allows 16/32-bit accesses. From the machine
> view (from the bus), it is only shifted by 1 bit. Another bit is
> shifted, but it is an implementation detail of the QEMU model.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/m68k/q800.c   | 2 +-
>   hw/mips/jazz.c   | 2 +-
>   hw/net/dp8393x.c | 2 ++
>   3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 6817c8b5d1a..d78edfe40e8 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -316,7 +316,7 @@ static void q800_init(MachineState *machine)
>   
>       dev = qdev_new("dp8393x");
>       qdev_set_nic_properties(dev, &nd_table[0]);
> -    qdev_prop_set_uint8(dev, "it_shift", 2);
> +    qdev_prop_set_uint8(dev, "it_shift", 1);
>       qdev_prop_set_bit(dev, "big_endian", true);
>       object_property_set_link(OBJECT(dev), "dma_mr",
>                                OBJECT(get_system_memory()), &error_abort);
> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> index d6183e18821..7f8680a189d 100644
> --- a/hw/mips/jazz.c
> +++ b/hw/mips/jazz.c
> @@ -295,7 +295,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_uint8(dev, "it_shift", 1);
>               qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
>               object_property_set_link(OBJECT(dev), "dma_mr",
>                                        OBJECT(rc4030_dma_mr), &error_abort);
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index d1e147a82a6..64f3b0fc3ea 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -971,6 +971,8 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>   {
>       dp8393xState *s = DP8393X(dev);
>   
> +    s->it_shift += 1; /* Registers are 16-bit wide */
> +
>       address_space_init(&s->as, s->dma_mr, "dp8393x");
>       memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
>                             "dp8393x-regs", SONIC_REG_COUNT << s->it_shift);

This patch looks wrong to me: by convention it_shift is used to implement the 
register stride, so by reducing this from 2 to 1 you've immediately broken the POLA 
regarding the it_shift property. There could be an argument that there is an 
intermediate bus that could be modelled for 16-bit accesses here, but this is worked 
around (and clearly commented) in patch 8.


ATB,

Mark.

[PATCH v3 4/8] dp8393x: Store CAM registers as 16-bit

[Philippe Mathieu-Daudé]

Per the DP83932C datasheet from July 1995:

  4.0 SONIC Registers
  4.1 THE CAM UNIT

    The Content Addressable Memory (CAM) consists of sixteen
    48-bit entries for complete address filtering of network
    packets. Each entry corresponds to a 48-bit destination
    address that is user programmable and can contain any
    combination of Multicast or Physical addresses. Each entry
    is partitioned into three 16-bit CAM cells accessible
    through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with
    CAP0 corresponding to the least significant 16 bits of
    the Destination Address and CAP2 corresponding to the
    most significant bits.

Store the CAM registers as 16-bit as it simplifies the code.
There is no change in the migration stream.

Co-developed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Co-developed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Missing:
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/dp8393x.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 64f3b0fc3ea..2c3047c8688 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -158,7 +158,7 @@ struct dp8393xState {
     MemoryRegion mmio;
 
     /* Registers */
-    uint8_t cam[16][6];
+    uint16_t cam[16][3];
     uint16_t regs[SONIC_REG_COUNT];
 
     /* Temporaries */
@@ -281,15 +281,13 @@ static void dp8393x_do_load_cam(dp8393xState *s)
         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;
-        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;
-        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]);
+        s->cam[index][0] = dp8393x_get(s, width, 1);
+        s->cam[index][1] = dp8393x_get(s, width, 2);
+        s->cam[index][2] = dp8393x_get(s, width, 3);
+        trace_dp8393x_load_cam(index,
+                               s->cam[index][0] >> 8, s->cam[index][0] & 0xff,
+                               s->cam[index][1] >> 8, s->cam[index][1] & 0xff,
+                               s->cam[index][2] >> 8, s->cam[index][2] & 0xff);
         /* Move to next entry */
         s->regs[SONIC_CDC]--;
         s->regs[SONIC_CDP] += size;
@@ -592,8 +590,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
     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)];
+            val = s->cam[s->regs[SONIC_CEP] & 0xf][SONIC_CAP0 - reg];
         }
         break;
     /* All other registers have no special contraints */
@@ -989,7 +986,7 @@ static const VMStateDescription vmstate_dp8393x = {
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField []) {
-        VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
+        VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
         VMSTATE_UINT16_ARRAY(regs, dp8393xState, SONIC_REG_COUNT),
         VMSTATE_END_OF_LIST()
     }
-- 
2.31.1

Re: [PATCH v3 4/8] dp8393x: Store CAM registers as 16-bit

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> Per the DP83932C datasheet from July 1995:
> 
>    4.0 SONIC Registers
>    4.1 THE CAM UNIT
> 
>      The Content Addressable Memory (CAM) consists of sixteen
>      48-bit entries for complete address filtering of network
>      packets. Each entry corresponds to a 48-bit destination
>      address that is user programmable and can contain any
>      combination of Multicast or Physical addresses. Each entry
>      is partitioned into three 16-bit CAM cells accessible
>      through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with
>      CAP0 corresponding to the least significant 16 bits of
>      the Destination Address and CAP2 corresponding to the
>      most significant bits.
> 
> Store the CAM registers as 16-bit as it simplifies the code.
> There is no change in the migration stream.
> 
> Co-developed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Co-developed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Missing:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/net/dp8393x.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 64f3b0fc3ea..2c3047c8688 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -158,7 +158,7 @@ struct dp8393xState {
>       MemoryRegion mmio;
>   
>       /* Registers */
> -    uint8_t cam[16][6];
> +    uint16_t cam[16][3];
>       uint16_t regs[SONIC_REG_COUNT];
>   
>       /* Temporaries */
> @@ -281,15 +281,13 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>           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;
> -        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;
> -        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]);
> +        s->cam[index][0] = dp8393x_get(s, width, 1);
> +        s->cam[index][1] = dp8393x_get(s, width, 2);
> +        s->cam[index][2] = dp8393x_get(s, width, 3);
> +        trace_dp8393x_load_cam(index,
> +                               s->cam[index][0] >> 8, s->cam[index][0] & 0xff,
> +                               s->cam[index][1] >> 8, s->cam[index][1] & 0xff,
> +                               s->cam[index][2] >> 8, s->cam[index][2] & 0xff);
>           /* Move to next entry */
>           s->regs[SONIC_CDC]--;
>           s->regs[SONIC_CDP] += size;
> @@ -592,8 +590,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
>       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)];
> +            val = s->cam[s->regs[SONIC_CEP] & 0xf][SONIC_CAP0 - reg];
>           }
>           break;
>       /* All other registers have no special contraints */
> @@ -989,7 +986,7 @@ static const VMStateDescription vmstate_dp8393x = {
>       .version_id = 0,
>       .minimum_version_id = 0,
>       .fields = (VMStateField []) {
> -        VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
> +        VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
>           VMSTATE_UINT16_ARRAY(regs, dp8393xState, SONIC_REG_COUNT),
>           VMSTATE_END_OF_LIST()
>       }

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


ATB,

Mark.

[PATCH v3 5/8] dp8393x: Migrate registers as array of uint16

[Philippe Mathieu-Daudé]

The CAM registers are now arrays of 3 uint16_t. We can avoid using
the VMSTATE_BUFFER_UNSAFE() macro by using VMSTATE_UINT16_2DARRAY()
which is more appropriate.

Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 2c3047c8688..68516241a1f 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -983,10 +983,10 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_dp8393x = {
     .name = "dp8393x",
-    .version_id = 0,
-    .minimum_version_id = 0,
+    .version_id = 1,
+    .minimum_version_id = 1,
     .fields = (VMStateField []) {
-        VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
+        VMSTATE_UINT16_2DARRAY(cam, dp8393xState, 16, 3),
         VMSTATE_UINT16_ARRAY(regs, dp8393xState, SONIC_REG_COUNT),
         VMSTATE_END_OF_LIST()
     }
-- 
2.31.1

Re: [PATCH v3 5/8] dp8393x: Migrate registers as array of uint16

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> The CAM registers are now arrays of 3 uint16_t. We can avoid using
> the VMSTATE_BUFFER_UNSAFE() macro by using VMSTATE_UINT16_2DARRAY()
> which is more appropriate.
> 
> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 2c3047c8688..68516241a1f 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -983,10 +983,10 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>   
>   static const VMStateDescription vmstate_dp8393x = {
>       .name = "dp8393x",
> -    .version_id = 0,
> -    .minimum_version_id = 0,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
>       .fields = (VMStateField []) {
> -        VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
> +        VMSTATE_UINT16_2DARRAY(cam, dp8393xState, 16, 3),
>           VMSTATE_UINT16_ARRAY(regs, dp8393xState, SONIC_REG_COUNT),
>           VMSTATE_END_OF_LIST()
>       }

I'm not convinced that this needs an extra patch and couldn't be squashed into the 
previous patch, but still:

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


ATB,

Mark.

[PATCH v3 6/8] dp8393x: Store CRC using device configured endianess

[Philippe Mathieu-Daudé]

Little-Endian CRC is dubious. The datasheet does not
specify it being little-endian. Use big-endian access
when the device is configured in such endianess.
(This is a theoretical bug fix.)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 68516241a1f..ac93412f70b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -827,7 +827,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
 
     /* Calculate the ethernet checksum */
-    checksum = cpu_to_le32(crc32(0, buf, pkt_size));
+    checksum = crc32(0, buf, pkt_size);
 
     /* Put packet into RBA */
     trace_dp8393x_receive_packet(dp8393x_crba(s));
@@ -837,8 +837,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     address += pkt_size;
 
     /* Put frame checksum into RBA */
-    address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
-                        &checksum, sizeof(checksum));
+    if (s->big_endian) {
+        address_space_stl_be(&s->as, address, checksum,
+                             MEMTXATTRS_UNSPECIFIED, NULL);
+    } else {
+        address_space_stl_le(&s->as, address, checksum,
+                             MEMTXATTRS_UNSPECIFIED, NULL);
+    }
     address += sizeof(checksum);
 
     /* Pad short packets to keep pointers aligned */
-- 
2.31.1

Re: [PATCH v3 6/8] dp8393x: Store CRC using device configured endianess

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> Little-Endian CRC is dubious. The datasheet does not
> specify it being little-endian. Use big-endian access
> when the device is configured in such endianess.
> (This is a theoretical bug fix.)
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 68516241a1f..ac93412f70b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -827,7 +827,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
>   
>       /* Calculate the ethernet checksum */
> -    checksum = cpu_to_le32(crc32(0, buf, pkt_size));
> +    checksum = crc32(0, buf, pkt_size);
>   
>       /* Put packet into RBA */
>       trace_dp8393x_receive_packet(dp8393x_crba(s));
> @@ -837,8 +837,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       address += pkt_size;
>   
>       /* Put frame checksum into RBA */
> -    address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> -                        &checksum, sizeof(checksum));
> +    if (s->big_endian) {
> +        address_space_stl_be(&s->as, address, checksum,
> +                             MEMTXATTRS_UNSPECIFIED, NULL);
> +    } else {
> +        address_space_stl_le(&s->as, address, checksum,
> +                             MEMTXATTRS_UNSPECIFIED, NULL);
> +    }
>       address += sizeof(checksum);
>   
>       /* Pad short packets to keep pointers aligned */

This is obviously new to the series: I can test this on big endian m68k but are you 
sure that this won't break big endian MIPS? Or do we not care for now since we don't 
have a working test image?


ATB,

Mark.

[NOTFORMERGE PATCH v3 7/8] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Philippe Mathieu-Daudé]

Instead of accessing N registers via a single address_space API
call using a temporary buffer (stored in the device state) and
updating each register, move the address_space call in the
register put/get. The load/store and word size checks are moved
to put/get too. This simplifies a bit, making the code easier
to read.

Co-developed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Co-developed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Missing:
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/dp8393x.c | 160 +++++++++++++++++++----------------------------
 1 file changed, 63 insertions(+), 97 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index ac93412f70b..a9224a5bc88 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -163,7 +163,6 @@ struct dp8393xState {
 
     /* Temporaries */
     uint8_t tx_buffer[0x10000];
-    uint16_t data[12];
     int loopback_packet;
 
     /* Memory access */
@@ -220,34 +219,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, int offset)
 {
+    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 += offset << 2;
+        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 += offset << 1;
+        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, int offset, 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 += offset << 2;
+        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 += offset << 1;
+        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);
         }
     }
 }
@@ -278,12 +291,10 @@ static void dp8393x_do_load_cam(dp8393xState *s)
 
     while (s->regs[SONIC_CDC] & 0x1f) {
         /* 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);
-        s->cam[index][1] = dp8393x_get(s, width, 2);
-        s->cam[index][2] = dp8393x_get(s, width, 3);
+        index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf;
+        s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1);
+        s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2);
+        s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3);
         trace_dp8393x_load_cam(index,
                                s->cam[index][0] >> 8, s->cam[index][0] & 0xff,
                                s->cam[index][1] >> 8, s->cam[index][1] & 0xff,
@@ -294,9 +305,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
     }
 
     /* Read CAM enable */
-    address_space_read(&s->as, dp8393x_cdp(s),
-                       MEMTXATTRS_UNSPECIFIED, s->data, size);
-    s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
+    s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0);
     trace_dp8393x_load_cam_done(s->regs[SONIC_CE]);
 
     /* Done */
@@ -312,14 +321,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]);
 
@@ -415,28 +422,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), 1) & 0xf000;
+        s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 2);
+        s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 3);
+        s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 4);
+        s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 5);
+        s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 6);
 
         /* Handle programmable interrupt */
         if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) {
@@ -458,15 +459,12 @@ 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),
+                                                  4 + 3 * i);
+                s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s),
+                                                  5 + 3 * i);
+                s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s),
+                                                 6 + 3 * i);
             }
         }
 
@@ -499,22 +497,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),
+                                              4 + 3 * s->regs[SONIC_TFC]);
             if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
                 /* EOL detected */
                 break;
@@ -762,7 +750,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;
 
@@ -775,10 +763,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;
     }
 
@@ -799,11 +785,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;
@@ -811,11 +793,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_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];
@@ -874,32 +852,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];
-- 
2.31.1

Re: [NOTFORMERGE PATCH v3 7/8] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> Instead of accessing N registers via a single address_space API
> call using a temporary buffer (stored in the device state) and
> updating each register, move the address_space call in the
> register put/get. The load/store and word size checks are moved
> to put/get too. This simplifies a bit, making the code easier
> to read.
> 
> Co-developed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Co-developed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Missing:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/net/dp8393x.c | 160 +++++++++++++++++++----------------------------
>   1 file changed, 63 insertions(+), 97 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index ac93412f70b..a9224a5bc88 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -163,7 +163,6 @@ struct dp8393xState {
>   
>       /* Temporaries */
>       uint8_t tx_buffer[0x10000];
> -    uint16_t data[12];
>       int loopback_packet;
>   
>       /* Memory access */
> @@ -220,34 +219,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, int offset)
>   {
> +    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 += offset << 2;
> +        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 += offset << 1;
> +        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, int offset, 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 += offset << 2;
> +        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 += offset << 1;
> +        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);
>           }
>       }
>   }
> @@ -278,12 +291,10 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>   
>       while (s->regs[SONIC_CDC] & 0x1f) {
>           /* 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);
> -        s->cam[index][1] = dp8393x_get(s, width, 2);
> -        s->cam[index][2] = dp8393x_get(s, width, 3);
> +        index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf;
> +        s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1);
> +        s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2);
> +        s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3);
>           trace_dp8393x_load_cam(index,
>                                  s->cam[index][0] >> 8, s->cam[index][0] & 0xff,
>                                  s->cam[index][1] >> 8, s->cam[index][1] & 0xff,
> @@ -294,9 +305,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>       }
>   
>       /* Read CAM enable */
> -    address_space_read(&s->as, dp8393x_cdp(s),
> -                       MEMTXATTRS_UNSPECIFIED, s->data, size);
> -    s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> +    s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0);
>       trace_dp8393x_load_cam_done(s->regs[SONIC_CE]);
>   
>       /* Done */
> @@ -312,14 +321,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]);
>   
> @@ -415,28 +422,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), 1) & 0xf000;
> +        s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 2);
> +        s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 3);
> +        s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 4);
> +        s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 5);
> +        s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 6);
>   
>           /* Handle programmable interrupt */
>           if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) {
> @@ -458,15 +459,12 @@ 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),
> +                                                  4 + 3 * i);
> +                s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s),
> +                                                  5 + 3 * i);
> +                s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s),
> +                                                 6 + 3 * i);
>               }
>           }
>   
> @@ -499,22 +497,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),
> +                                              4 + 3 * s->regs[SONIC_TFC]);
>               if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
>                   /* EOL detected */
>                   break;
> @@ -762,7 +750,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;
>   
> @@ -775,10 +763,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;
>       }
>   
> @@ -799,11 +785,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;
> @@ -811,11 +793,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_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];
> @@ -874,32 +852,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];

As per the updated cover letter this works for me on MacOS/m68k, NetBSD/m68k and 
Linux/m68k so I'm happy to give a:

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

Why has this been demoted to NOTFORMERGE?


ATB,

Mark.

[NOTFORMERGE PATCH v3 8/8] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set .impl.min_access_size
and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver which uses
32-bit accesses.

The problem with forcing the register access to 32-bit in this way is that since the
dp8393x uses 16-bit registers, a manual endian swap is required for devices on big
endian machines with 32-bit accesses.

For both access sizes and machine endians the QEMU memory API can do the right thing
automatically: all that is needed is to set .impl.min_access_size to 2 to declare that
the dp8393x implements 16-bit registers.

Normally .impl.max_access_size should also be set to 2, however that doesn't quite
work in this case since the register stride is specified using a (dynamic) it_shift
property which is applied during the MMIO access itself. The effect of this is that
for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use of
it_shift within the MMIO access itself causes the register value to be repeated in both
the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the stride to be
zero-extended up to access size and therefore fails to correctly detect the dp8393x
device due to the extra data in the top 16-bits.

The solution here is to remove .impl.max_access_size so that the memory API will
correctly zero-extend the 16-bit registers to the access size up to and including
it_shift. Since it_shift is never greater than 2 than this will always do the right
thing for both 16-bit and 32-bit accesses regardless of the machine endian, allowing
the manual endian swap code to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
Message-Id: <20210705214929.17222-2-mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index a9224a5bc88..71c82a07c23 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -588,15 +588,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);
 
@@ -677,11 +676,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
     }
 }
 
+/*
+ * Since .impl.max_access_size is effectively controlled by the it_shift
+ * property, leave it unspecified for now to allow the memory API to
+ * correctly zero extend the 16-bit register values to the access size up to and
+ * including it_shift.
+ */
 static const MemoryRegionOps dp8393x_ops = {
     .read = dp8393x_read,
     .write = dp8393x_write,
-    .impl.min_access_size = 4,
-    .impl.max_access_size = 4,
+    .impl.min_access_size = 2,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.31.1

Re: [NOTFORMERGE PATCH v3 8/8] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set .impl.min_access_size
> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver which uses
> 32-bit accesses.
> 
> The problem with forcing the register access to 32-bit in this way is that since the
> dp8393x uses 16-bit registers, a manual endian swap is required for devices on big
> endian machines with 32-bit accesses.
> 
> For both access sizes and machine endians the QEMU memory API can do the right thing
> automatically: all that is needed is to set .impl.min_access_size to 2 to declare that
> the dp8393x implements 16-bit registers.
> 
> Normally .impl.max_access_size should also be set to 2, however that doesn't quite
> work in this case since the register stride is specified using a (dynamic) it_shift
> property which is applied during the MMIO access itself. The effect of this is that
> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use of
> it_shift within the MMIO access itself causes the register value to be repeated in both
> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the stride to be
> zero-extended up to access size and therefore fails to correctly detect the dp8393x
> device due to the extra data in the top 16-bits.
> 
> The solution here is to remove .impl.max_access_size so that the memory API will
> correctly zero-extend the 16-bit registers to the access size up to and including
> it_shift. Since it_shift is never greater than 2 than this will always do the right
> thing for both 16-bit and 32-bit accesses regardless of the machine endian, allowing
> the manual endian swap code to be removed.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> Message-Id: <20210705214929.17222-2-mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a9224a5bc88..71c82a07c23 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -588,15 +588,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);
>   
> @@ -677,11 +676,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>       }
>   }
>   
> +/*
> + * Since .impl.max_access_size is effectively controlled by the it_shift
> + * property, leave it unspecified for now to allow the memory API to
> + * correctly zero extend the 16-bit register values to the access size up to and
> + * including it_shift.
> + */
>   static const MemoryRegionOps dp8393x_ops = {
>       .read = dp8393x_read,
>       .write = dp8393x_write,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> +    .impl.min_access_size = 2,
>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };

Why has this been demoted to NOTFORMERGE? Removing the explicit .impl.max_access_size 
= 4 means that we fall back to the default .impl.max_access_size = 4 which already 
has a T-b tag from Finn for the series at 
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg07167.html which includes 
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg06814.html.

This patch with the explicit .impl.max_access_size = 4 was the primary motivation for 
posting the original series nearly a month ago: see the original version at 
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg03199.html.


ATB,

Mark.

Re: [PATCH v3 0/8] dp8393x: fixes and improvements

[Finn Thain]

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

On Sat, 10 Jul 2021, Philippe Mathieu-Daudé wrote:

> 
> The last 2 patches are included for Mark, but I don't plan to merge
> 
> them without Finn's Ack, and apparently they require more work.
> 


I tested the patch series both with and without the last 2 patches. Both 
builds worked fine with my NetBSD/arc, Linux/mipsel and Linux/m68k guests.

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

I have no objection to patch 8/8 ("dp8393x: don't force 32-bit register 
access"). I asked Mark to explain why it was a bug fix (since it didn't 
change QEMU behaviour in my tests) but when I looked into it I found that 
he is quite right, the patch does fix a theoretical bug.

My only objection to patch 7/8 ("dp8393x: Rewrite dp8393x_get() / 
dp8393x_put()") was that it could be churn.

If I'm right that the big_endian flag should go away, commit b1600ff195 
("hw/mips/jazz: specify correct endian for dp8393x device") has already 
taken mainline in the wrong direction and amounts to churn.

I have the same reservations about patch 6/8 ("dp8393x: Store CRC using 
device configured endianess"). Perhaps that should be NOTFORMERGE too 
(even though it too a theoretical bug fix).

Is there a good way to avoid using big_endian for storing the CRC and the 
other DMA operations?

BTW, if you see "sn0: receive buffers exhausted" occasionally logged by 
the NetBSD 9.2 kernel, accompanied by packet loss, it's not a regression 
in QEMU. I first observed it last year when stress testing dp8393x with 
NetBSD 5.1. I believe this to be an old NetBSD sn driver bug because Linux 
is unaffected.

Re: [PATCH v3 0/8] dp8393x: fixes and improvements

[Philippe Mathieu-Daudé]

On 7/11/21 4:08 AM, Finn Thain wrote:
> On Sat, 10 Jul 2021, Philippe Mathieu-Daudé wrote:
> 
>>
>> The last 2 patches are included for Mark, but I don't plan to merge
>>
>> them without Finn's Ack, and apparently they require more work.
>>
> 
> 
> I tested the patch series both with and without the last 2 patches. Both 
> builds worked fine with my NetBSD/arc, Linux/mipsel and Linux/m68k guests.
> 
> Tested-by: Finn Thain <fthain@linux-m68k.org>

Thank you very much :)

> I have no objection to patch 8/8 ("dp8393x: don't force 32-bit register 
> access"). I asked Mark to explain why it was a bug fix (since it didn't 
> change QEMU behaviour in my tests) but when I looked into it I found that 
> he is quite right, the patch does fix a theoretical bug.

OK.

> My only objection to patch 7/8 ("dp8393x: Rewrite dp8393x_get() / 
> dp8393x_put()") was that it could be churn.
> 
> If I'm right that the big_endian flag should go away, commit b1600ff195 
> ("hw/mips/jazz: specify correct endian for dp8393x device") has already 
> taken mainline in the wrong direction and amounts to churn.

We might figure out with a BE guest image, the remove the endian flag.
I don't think the patch is worth removing, because it simplifies and
we'll only have to fix the endianess in 2 places, dp8393x_get/put. I
prefer to restrict the address space accesses there.

> I have the same reservations about patch 6/8 ("dp8393x: Store CRC using 
> device configured endianess"). Perhaps that should be NOTFORMERGE too 
> (even though it too a theoretical bug fix).

OK, dropped.

> Is there a good way to avoid using big_endian for storing the CRC and the 
> other DMA operations?

Could be, but I'd rather see this fixed generically in the MemoryRegion
API, not in this particular device model.

> BTW, if you see "sn0: receive buffers exhausted" occasionally logged by 
> the NetBSD 9.2 kernel, accompanied by packet loss, it's not a regression 
> in QEMU. I first observed it last year when stress testing dp8393x with 
> NetBSD 5.1. I believe this to be an old NetBSD sn driver bug because Linux 
> is unaffected.
> 

Re: [PATCH v3 0/8] dp8393x: fixes and improvements

[Finn Thain]

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

On Sun, 11 Jul 2021, Philippe Mathieu-Daudé wrote:

> 
> > If I'm right that the big_endian flag should go away, commit 
> > b1600ff195 ("hw/mips/jazz: specify correct endian for dp8393x device") 
> > has already taken mainline in the wrong direction and amounts to 
> > churn.
> 
> We might figure out with a BE guest image, the remove the endian flag.

Yes, it's hard to make progress without a BE guest. However, for testing 
dp8393x we probably don't need a disk image. I think we only need working 
firmware, since the RISC/os firmware appears to implement BOOTP and TFTP 
and appears to contain a SONIC driver.

This page discusses using the "MIPS Monitor" firmware on a Mips Magnum 
3000 machine to netboot NetBSD/mipsco: 
https://www.ludd.ltu.se/~ragge/htdocs/Ports/mipsco/install.html

Note that the firmware banner message looks like this:
Rx3230 MIPS Monitor: Version 5.43 OPT Mon May 13 17:31:12 PDT 1991 root 

It appears that there may be a similar firmware for MIPS Magnum at, 
https://gunkies.org/wiki/Installing_Windows_NT_4.0_on_Qemu(MIPS)

(I suppose this is the firmware floppy that was once found at ftp.sgi.com, 
after SGI acquired MIPS?)

The file RISCOS.RAW found in SETUP.ZIP appears to contain various string 
constants, both LE and BE, including:

$ swap64 < RISCOS.RAW |strings |egrep "Version|MIPS|Rx|Monitor"
...
Version 5.60 OPT-EB Wed Jun 17 11:23:28 PDT 1992 root
MIPS
Rx4230
%s %s Monitor: %s
...

This looks like the same banner, only for a different machine (as you'd 
expect). Unfortunately, nothing happened when I tried to boot that 
firmware:

$ ln -s RISCOS.RAW mips_bios.bin 
$ qemu-system-mips64 -M magnum -L . -global ds1225y.filename=mips-nvram -serial mon:stdio -serial null -nic bridge,model=dp83932,mac=00:00:00:aa:bb:cc

I don't know enough about this platform or about QEMU to go much further 
with this so I hope that others will be able to help.

I did find this link, which talks a little about the early boot code.
https://web.archive.org/web/20180823065803/http://www.sensi.org/~alec/mips/mips-history.html

Re: [PATCH v3 0/8] dp8393x: fixes and improvements

[Finn Thain]

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

On Mon, 12 Jul 2021, Finn Thain wrote:

> On Sun, 11 Jul 2021, Philippe Mathieu-Daudé wrote:
> 
> > 
> > > If I'm right that the big_endian flag should go away, commit 
> > > b1600ff195 ("hw/mips/jazz: specify correct endian for dp8393x 
> > > device") has already taken mainline in the wrong direction and 
> > > amounts to churn.
> > 
> > We might figure out with a BE guest image, the remove the endian flag.
> 
> Yes, it's hard to make progress without a BE guest. However, for testing 
> dp8393x we probably don't need a disk image. I think we only need 
> working firmware, since the RISC/os firmware appears to implement BOOTP 
> and TFTP and appears to contain a SONIC driver.

I think we probably can install RISC/os once the firmware can be made to 
work.

The file "RISCos_5.01.iso", found in the Bitsavers archive, contains 
several kernel binaries, one of which is "unix.r4030eb_std".

From the "r4030" in its name, and from the symbol names and string 
constants it contains, this binary appears to have all the drivers for the 
MIPS Magnum 4000.