[RFC PATCH 0/6] dp8393x: Housekeeping

[RFC PATCH 0/6] dp8393x: Housekeeping

[Philippe Mathieu-Daudé]

Housekeeping while reviewing Mark's "fixes for MacOS toolbox ROM"
series v2.
RFC because totally untested =) Just compiled.

Mark Cave-Ayland (2):
  dp8393x: fix CAM descriptor entry index
  dp8393x: don't force 32-bit register access

Philippe Mathieu-Daudé (4):
  dp8393x: Restrict bus access to 16/32-bit operations
  dp8393x: Store CAM registers as 16-bit
  dp8393x: Replace address_space_rw(is_write=1) by address_space_write()
  dp8393x: Rewrite dp8393x_get() / dp8393x_put()

 hw/net/dp8393x.c | 191 +++++++++++++++++++----------------------------
 1 file changed, 76 insertions(+), 115 deletions(-)

-- 
2.31.1

[PATCH 1/6] dp8393x: fix CAM descriptor entry index

[Philippe Mathieu-Daudé]

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

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>
Tested-by: Finn Thain <fthain@linux-m68k.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210625065401.30170-10-mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 252c0a26641..11810c9b600 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.31.1

[PATCH 2/6] 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" 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")
Tested-by: Finn Thain <fthain@linux-m68k.org>
Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk>
[PMD: dp8393x_ops.impl.max_access_size 4 -> 2]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 11810c9b600..d16ade2b198 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,8 +693,8 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
 static const MemoryRegionOps dp8393x_ops = {
     .read = dp8393x_read,
     .write = dp8393x_write,
-    .impl.min_access_size = 4,
-    .impl.max_access_size = 4,
+    .impl.min_access_size = 2,
+    .impl.max_access_size = 2,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.31.1

Re: [PATCH 2/6] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:

> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 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.

The change should work, but the commit message above needs a slight tweak - maybe 
something like this?

Since the registers are 16-bit then we can simply set both .impl.min_access and 
.impl.max_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")
> Tested-by: Finn Thain <fthain@linux-m68k.org>
> Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk>
> [PMD: dp8393x_ops.impl.max_access_size 4 -> 2]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b600..d16ade2b198 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,8 +693,8 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>   static const MemoryRegionOps dp8393x_ops = {
>       .read = dp8393x_read,
>       .write = dp8393x_write,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> +    .impl.min_access_size = 2,
> +    .impl.max_access_size = 2,
>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };


ATB,

Mark.

Re: [PATCH 2/6] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 7/3/21 4:39 PM, Mark Cave-Ayland wrote:
> On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> 
>> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> 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.
> 
> The change should work, but the commit message above needs a slight
> tweak - maybe something like this?
> 
> Since the registers are 16-bit then we can simply set both
> .impl.min_access and .impl.max_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.

Do you mind sending v3 of this patch reworded (and including the .valid
fields)?

>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
>> Tested-by: Finn Thain <fthain@linux-m68k.org>
>> Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk>
>> [PMD: dp8393x_ops.impl.max_access_size 4 -> 2]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/net/dp8393x.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)

Re: [PATCH 2/6] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

On 03/07/2021 17:29, Philippe Mathieu-Daudé wrote:

> On 7/3/21 4:39 PM, Mark Cave-Ayland wrote:
>> On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
>>
>>> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> 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.
>>
>> The change should work, but the commit message above needs a slight
>> tweak - maybe something like this?
>>
>> Since the registers are 16-bit then we can simply set both
>> .impl.min_access and .impl.max_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.
> 
> Do you mind sending v3 of this patch reworded (and including the .valid
> fields)?

I've sent a v3 with the rewording but dropping the .valid fields since that breaks 
MacOS and removed Finn's T-b tag as it may be there is an issue here with mips64el - 
whilst it works for me on all of my test images, I'm struggling to keep up with all 
the patches flying around everywhere :/

Now that your MIPS PR has been applied, perhaps it is worth sending a rebased v2 of 
your RFC "dp8393x: Housekeeping" patchset to ensure that everyone is up to date with 
the latest fixes? I won't be able to have a look at the CRC patchset for a few days 
though.


ATB,

Mark.

Re: [PATCH 2/6] dp8393x: don't force 32-bit register access

[Finn Thain]

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


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

> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 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")
> Tested-by: Finn Thain <fthain@linux-m68k.org>

I have to retract that tested-by tag for this new version. It breaks my 
Linux/mipsel guest. The jazzsonic driver now says,

SONIC ethernet controller not found (0x40004)

> Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk>
> [PMD: dp8393x_ops.impl.max_access_size 4 -> 2]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations

[Philippe Mathieu-Daudé]

Per the DP83932C datasheet from July 1995:

  1. Functional Description
  1.3 DATA WIDTH AND BYTE ORDERING

    The SONIC can be programmed to operate with
    either 32-bit or 16-bit wide memory.

Restrict the memory bus to reject 8/64-bit accesses.

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

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index d16ade2b198..c9b478c127c 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -695,6 +695,8 @@ static const MemoryRegionOps dp8393x_ops = {
     .write = dp8393x_write,
     .impl.min_access_size = 2,
     .impl.max_access_size = 2,
+    .valid.min_access_size = 2,
+    .valid.max_access_size = 4,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.31.1

Re: [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations

[Mark Cave-Ayland]

On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:

> Per the DP83932C datasheet from July 1995:
> 
>    1. Functional Description
>    1.3 DATA WIDTH AND BYTE ORDERING
> 
>      The SONIC can be programmed to operate with
>      either 32-bit or 16-bit wide memory.
> 
> Restrict the memory bus to reject 8/64-bit accesses.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index d16ade2b198..c9b478c127c 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -695,6 +695,8 @@ static const MemoryRegionOps dp8393x_ops = {
>       .write = dp8393x_write,
>       .impl.min_access_size = 2,
>       .impl.max_access_size = 2,
> +    .valid.min_access_size = 2,
> +    .valid.max_access_size = 4,
>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };

The changes to .valid are correct, but I don't think the above reference to the 
datasheet is. My reading of the sentence in section 1.3 "The SONIC can be programmed 
to operate with either 32-bit or 16-bit wide memory" (along with subsequent 
references to the DW bit in the DCR) is that this section is describing bus master 
transfers as per figure 1-5 instead of register accesses which is what this patch 
changes.

I think you could simply squash this patch into the previous one since there will 
already be a behaviour change there.


ATB,

Mark.

Re: [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations

[Mark Cave-Ayland]

On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:

> Per the DP83932C datasheet from July 1995:
> 
>    1. Functional Description
>    1.3 DATA WIDTH AND BYTE ORDERING
> 
>      The SONIC can be programmed to operate with
>      either 32-bit or 16-bit wide memory.
> 
> Restrict the memory bus to reject 8/64-bit accesses.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index d16ade2b198..c9b478c127c 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -695,6 +695,8 @@ static const MemoryRegionOps dp8393x_ops = {
>       .write = dp8393x_write,
>       .impl.min_access_size = 2,
>       .impl.max_access_size = 2,
> +    .valid.min_access_size = 2,
> +    .valid.max_access_size = 4,
>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };

Unfortunately this patch breaks MacOS - it seems very early in startup the MacOS 
toolbox probes for the presence of the network adapter using single byte accesses 
which are rejected by this change :(

I'd suggest that we simply drop this patch.


ATB,

Mark.

[RFC PATCH 4/6] 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.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 c9b478c127c..e0055b178b1 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -157,7 +157,7 @@ struct dp8393xState {
     MemoryRegion mmio;
 
     /* Registers */
-    uint8_t cam[16][6];
+    uint16_t cam[16][3];
     uint16_t regs[0x40];
 
     /* Temporaries */
@@ -280,15 +280,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;
@@ -591,8 +589,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][2 * (SONIC_CAP0 - reg)];
         }
         break;
     /* All other registers have no special contraints */
@@ -987,7 +984,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, 0x40),
         VMSTATE_END_OF_LIST()
     }
-- 
2.31.1

Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit

[Mark Cave-Ayland]

On 03/07/2021 15:19, 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   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 c9b478c127c..e0055b178b1 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -157,7 +157,7 @@ struct dp8393xState {
>       MemoryRegion mmio;
>   
>       /* Registers */
> -    uint8_t cam[16][6];
> +    uint16_t cam[16][3];
>       uint16_t regs[0x40];
>   
>       /* Temporaries */
> @@ -280,15 +280,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;
> @@ -591,8 +589,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][2 * (SONIC_CAP0 - reg)];
>           }
>           break;
>       /* All other registers have no special contraints */
> @@ -987,7 +984,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, 0x40),
>           VMSTATE_END_OF_LIST()
>       }

I need to do some testing first to make sure dp8393x_get() and dp8393x_put() are 
doing the right thing here, but it looks correct.

Also given that the migration stream for MIPS machines has been broken for many years 
until your latest PR, I would have no objection if you wanted to change 
VMSTATE_BUFFER_UNSAFE to VMSTATE_UINT16_ARRAY.


ATB,

Mark.

Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit

[Mark Cave-Ayland]

On 03/07/2021 15:19, 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   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 c9b478c127c..e0055b178b1 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -157,7 +157,7 @@ struct dp8393xState {
>       MemoryRegion mmio;
>   
>       /* Registers */
> -    uint8_t cam[16][6];
> +    uint16_t cam[16][3];
>       uint16_t regs[0x40];
>   
>       /* Temporaries */
> @@ -280,15 +280,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;
> @@ -591,8 +589,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][2 * (SONIC_CAP0 - reg)];
>           }
>           break;
>       /* All other registers have no special contraints */
> @@ -987,7 +984,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, 0x40),
>           VMSTATE_END_OF_LIST()
>       }

I'd still be inclined to change VMSTATE_BUFFER_UNSAFE for VMSTATE_UINT16_ARRAY whilst 
you can do it without having to worry about the migration stream being already 
broken, but anyhow:

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


ATB,

Mark.

Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit

[Philippe Mathieu-Daudé]

On 7/4/21 4:48 PM, Mark Cave-Ayland wrote:
> On 03/07/2021 15:19, 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.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/net/dp8393x.c | 23 ++++++++++-------------
>>   1 file changed, 10 insertions(+), 13 deletions(-)

>> @@ -987,7 +984,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, 0x40),
>>           VMSTATE_END_OF_LIST()
>>       }
> 
> I'd still be inclined to change VMSTATE_BUFFER_UNSAFE for
> VMSTATE_UINT16_ARRAY whilst you can do it without having to worry about
> the migration stream being already broken, but anyhow:
> 
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Do you want me to squash:

-- >8 --
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 1d1837dbd38..4c2fa0aabbd 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -951,10 +951,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_ARRAY(cam, dp8393xState, 0, 16 * 3),
         VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
         VMSTATE_END_OF_LIST()
     }
---

Or send it as a new patch?

Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit

[Mark Cave-Ayland]

On 06/07/2021 18:29, Philippe Mathieu-Daudé wrote:

> On 7/4/21 4:48 PM, Mark Cave-Ayland wrote:
>> On 03/07/2021 15:19, 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.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>    hw/net/dp8393x.c | 23 ++++++++++-------------
>>>    1 file changed, 10 insertions(+), 13 deletions(-)
> 
>>> @@ -987,7 +984,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, 0x40),
>>>            VMSTATE_END_OF_LIST()
>>>        }
>>
>> I'd still be inclined to change VMSTATE_BUFFER_UNSAFE for
>> VMSTATE_UINT16_ARRAY whilst you can do it without having to worry about
>> the migration stream being already broken, but anyhow:
>>
>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Do you want me to squash:
> 
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 1d1837dbd38..4c2fa0aabbd 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -951,10 +951,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_ARRAY(cam, dp8393xState, 0, 16 * 3),
>           VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
>           VMSTATE_END_OF_LIST()
>       }
> ---
> 
> Or send it as a new patch?

I don't mind either way. I think VMSTATE_UINT16_ARRAY is nicer, and it's very rare 
that you get the freedom to make a migration change like this without having to worry 
about compatibility. It's also really quick and easy to test.

If it passes your local tests and you would prefer to squash rather than re-post then 
you can also add a:

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

to the patchset. I included the list of guests I tested in the cover note, but forgot 
to explicitly add the T-b tag.


ATB,

Mark.

[PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write()

[Philippe Mathieu-Daudé]

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 e0055b178b1..bbe241ef9db 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -814,8 +814,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,
+                            (uint8_t *)s->data, size);
 
         /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
@@ -844,8 +844,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;
     }
 
-- 
2.31.1

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

[Mark Cave-Ayland]

On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:

> 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 e0055b178b1..bbe241ef9db 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -814,8 +814,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,
> +                            (uint8_t *)s->data, size);
>   
>           /* Move to next descriptor */
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -844,8 +844,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;
>       }

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


ATB,

Mark.

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

[Mark Cave-Ayland]

On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:

> 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 e0055b178b1..bbe241ef9db 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -814,8 +814,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,
> +                            (uint8_t *)s->data, size);
>   
>           /* Move to next descriptor */
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -844,8 +844,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;
>       }

I've also verified that this works so it can also have:

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


ATB,

Mark.

[RFC PATCH 6/6] 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.

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

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index bbe241ef9db..db9cfd786f5 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -162,7 +162,6 @@ struct dp8393xState {
 
     /* Temporaries */
     uint8_t tx_buffer[0x10000];
-    uint16_t data[12];
     int loopback_packet;
 
     /* Memory access */
@@ -219,34 +218,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);
         }
     }
 }
@@ -277,12 +290,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,
@@ -293,9 +304,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 */
@@ -311,14 +320,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]);
 
@@ -414,28 +421,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) {
@@ -457,15 +458,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);
             }
         }
 
@@ -498,22 +493,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 +747,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 +760,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 +782,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 +790,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,
-                            (uint8_t *)s->data, size);
+        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
 
         /* Move to next descriptor */
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
@@ -869,32 +844,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: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Mark Cave-Ayland]

On 03/07/2021 15:19, 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 157 ++++++++++++++++++-----------------------------
>   1 file changed, 60 insertions(+), 97 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index bbe241ef9db..db9cfd786f5 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -162,7 +162,6 @@ struct dp8393xState {
>   
>       /* Temporaries */
>       uint8_t tx_buffer[0x10000];
> -    uint16_t data[12];
>       int loopback_packet;
>   
>       /* Memory access */
> @@ -219,34 +218,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);
>           }
>       }
>   }
> @@ -277,12 +290,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,
> @@ -293,9 +304,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 */
> @@ -311,14 +320,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]);
>   
> @@ -414,28 +421,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) {
> @@ -457,15 +458,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);
>               }
>           }
>   
> @@ -498,22 +493,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 +747,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 +760,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 +782,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 +790,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,
> -                            (uint8_t *)s->data, size);
> +        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
>   
>           /* Move to next descriptor */
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -869,32 +844,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];

This seems like a nice tidy-up. I'll need a bit of time to give some more R-b and T-b 
tags since MacOS is running on a very diverged branch, so I'll need to pick through 
and update the changes to run across all my test images accordingly.

With this patch is it now possible to remove s->data completely?


ATB,

Mark.

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Philippe Mathieu-Daudé]

On 7/3/21 5:00 PM, Mark Cave-Ayland wrote:
> On 03/07/2021 15:19, 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.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/net/dp8393x.c | 157 ++++++++++++++++++-----------------------------
>>   1 file changed, 60 insertions(+), 97 deletions(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index bbe241ef9db..db9cfd786f5 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -162,7 +162,6 @@ struct dp8393xState {
>>         /* Temporaries */
>>       uint8_t tx_buffer[0x10000];
>> -    uint16_t data[12];
>>       int loopback_packet;
>>         /* Memory access */

> This seems like a nice tidy-up. I'll need a bit of time to give some
> more R-b and T-b tags since MacOS is running on a very diverged branch,
> so I'll need to pick through and update the changes to run across all my
> test images accordingly.

No hurry.

> With this patch is it now possible to remove s->data completely?

First line remove it ;) (It was not part of the migration stream).

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Finn Thain]

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


On Sat, 3 Jul 2021, 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I tried this series with a Linux/m68k guest but network activity hanged 
the emulator. The cause of the problem is somewhere in this patch.

BTW, I've become suspicious of the word "rewrite". In this case I think it 
describes a commit that is attempting to do too much and needs to be split 
up to make it easier to review (and debug).

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Mark Cave-Ayland]

On 03/07/2021 15:19, 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 157 ++++++++++++++++++-----------------------------
>   1 file changed, 60 insertions(+), 97 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index bbe241ef9db..db9cfd786f5 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -162,7 +162,6 @@ struct dp8393xState {
>   
>       /* Temporaries */
>       uint8_t tx_buffer[0x10000];
> -    uint16_t data[12];
>       int loopback_packet;
>   
>       /* Memory access */
> @@ -219,34 +218,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);
>           }
>       }
>   }
> @@ -277,12 +290,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,
> @@ -293,9 +304,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 */
> @@ -311,14 +320,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]);
>   
> @@ -414,28 +421,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) {
> @@ -457,15 +458,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);
>               }
>           }
>   
> @@ -498,22 +493,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 +747,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 +760,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 +782,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 +790,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,
> -                            (uint8_t *)s->data, size);
> +        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
>   
>           /* Move to next descriptor */
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -869,32 +844,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];

This patch breaks networking in its current form, but I was able to make it work by 
applying the following diff:


diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index db9cfd786f..b08843172b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -218,20 +218,20 @@ static uint32_t dp8393x_wt(dp8393xState *s)
      return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
  }

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

      if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
-        addr += 2 * ofs16;
+        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 {
-        addr += 1 * ofs16;
+        addr += offset << 1;
          if (s->big_endian) {
              val = address_space_lduw_be(&s->as, addr, attrs, NULL);
          } else {
@@ -243,19 +243,19 @@ static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, 
unsigned ofs16)
  }

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

      if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
-        addr += 2 * ofs16;
+        addr += offset << 2;
          if (s->big_endian) {
              address_space_stl_be(&s->as, addr, val, attrs, NULL);
          } else {
              address_space_stl_le(&s->as, addr, val, attrs, NULL);
          }
      } else {
-        addr += 1 * ofs16;
+        addr += offset << 1;
          if (s->big_endian) {
              address_space_stw_be(&s->as, addr, val, attrs, NULL);
          } else {
@@ -431,12 +431,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
          tx_len = 0;

          /* Update registers */
-        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);
+        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,9 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
              i++;
              if (i != s->regs[SONIC_TFC]) {
                  /* Read next fragment details */
-                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);
+                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);
              }
          }


The main change is that the offset argument for dp8393x_get() and dp8393x_put() is 
actually an entry number and not a count of 16 bit words. Other than that there were 
a couple of offset calculations that needed adjustment to get things working again.

Taking git master and applying your outstanding PR + this series without patch 3 + 
this diff gave me working networking on Linux/m68k, MacOS 8.0 and NetBSD/arc.

Unfortunately I don't have a test mips64el image available to see if this combination 
works for Linux. Phil, do you have a suitable test kernel and rootfs image available 
to allow this to be tested?


ATB,

Mark.

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Finn Thain]

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

On Sun, 4 Jul 2021, Mark Cave-Ayland wrote:

> On 03/07/2021 15:19, 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.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >   hw/net/dp8393x.c | 157 ++++++++++++++++++-----------------------------
> >   1 file changed, 60 insertions(+), 97 deletions(-)
> > 
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index bbe241ef9db..db9cfd786f5 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -162,7 +162,6 @@ struct dp8393xState {
> >         /* Temporaries */
> >       uint8_t tx_buffer[0x10000];
> > -    uint16_t data[12];
> >       int loopback_packet;
> >         /* Memory access */
> > @@ -219,34 +218,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);
> >           }
> >       }
> >   }
> > @@ -277,12 +290,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,
> > @@ -293,9 +304,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 */
> > @@ -311,14 +320,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]);
> >   @@ -414,28 +421,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) {
> > @@ -457,15 +458,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);
> >               }
> >           }
> >   @@ -498,22 +493,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 +747,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 +760,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 +782,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 +790,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,
> > -                            (uint8_t *)s->data, size);
> > +        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
> >             /* Move to next descriptor */
> >           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> > @@ -869,32 +844,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];
> 
> This patch breaks networking in its current form, but I was able to make it
> work by applying the following diff:
> 
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index db9cfd786f..b08843172b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -218,20 +218,20 @@ static uint32_t dp8393x_wt(dp8393xState *s)
>      return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
>  }
> 
> -static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16)
> +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, int offset)
>  {
>      const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>      uint16_t val;
> 
>      if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> -        addr += 2 * ofs16;
> +        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 {
> -        addr += 1 * ofs16;
> +        addr += offset << 1;
>          if (s->big_endian) {
>              val = address_space_lduw_be(&s->as, addr, attrs, NULL);
>          } else {
> @@ -243,19 +243,19 @@ static uint16_t dp8393x_get(dp8393xState *s, hwaddr
> addr, unsigned ofs16)
>  }
> 
>  static void dp8393x_put(dp8393xState *s,
> -                        hwaddr addr, unsigned ofs16, uint16_t val)
> +                        hwaddr addr, int offset, uint16_t val)
>  {
>      const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> 
>      if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> -        addr += 2 * ofs16;
> +        addr += offset << 2;
>          if (s->big_endian) {
>              address_space_stl_be(&s->as, addr, val, attrs, NULL);
>          } else {
>              address_space_stl_le(&s->as, addr, val, attrs, NULL);
>          }
>      } else {
> -        addr += 1 * ofs16;
> +        addr += offset << 1;
>          if (s->big_endian) {
>              address_space_stw_be(&s->as, addr, val, attrs, NULL);
>          } else {
> @@ -431,12 +431,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>          tx_len = 0;
> 
>          /* Update registers */
> -        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);
> +        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,9 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>              i++;
>              if (i != s->regs[SONIC_TFC]) {
>                  /* Read next fragment details */
> -                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);
> +                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);
>              }
>          }
> 
> 
> The main change is that the offset argument for dp8393x_get() and
> dp8393x_put() is actually an entry number and not a count of 16 bit words.
> Other than that there were a couple of offset calculations that needed
> adjustment to get things working again.
> 
> Taking git master and applying your outstanding PR + this series without patch
> 3 + this diff gave me working networking on Linux/m68k, MacOS 8.0 and
> NetBSD/arc.
> 

Nice job getting MacOS 8.0 to run! That functionality could be very useful 
for working on the Linux bootloaders (Penguin and EMILE) as they don't 
work under MESS/MAME or Mini VMac etc.

> Unfortunately I don't have a test mips64el image available to see if this
> combination works for Linux. Phil, do you have a suitable test kernel and
> rootfs image available to allow this to be tested?
> 

You can build and boot a mipsel vmlinux by following the steps I described 
previously. In the kernel messages you'll see the jazzsonic driver attempt 
to probe the device. When it succeeds, you'll see the MAC address 
reported. You can also observe the regression I reported with regards to 
patch 2/6, "dp8393x: don't force 32-bit register access".

> 
> ATB,
> 
> Mark.
> 

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Mark Cave-Ayland]

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

>> Unfortunately I don't have a test mips64el image available to see if this
>> combination works for Linux. Phil, do you have a suitable test kernel and
>> rootfs image available to allow this to be tested?
>>
> 
> You can build and boot a mipsel vmlinux by following the steps I described
> previously. In the kernel messages you'll see the jazzsonic driver attempt
> to probe the device. When it succeeds, you'll see the MAC address
> reported. You can also observe the regression I reported with regards to
> patch 2/6, "dp8393x: don't force 32-bit register access".

Those instructions are useful, but since I am not a MIPS developer I don't have an 
existing toolchain/kernel tree and rootfs available to test this.

If you can provide me with a link to your vmlinux and rootfs with busybox or similar 
in it, I can take a look to see what is happening here. Otherwise it's almost 
impossible for me to understand and debug the problem you are seeing on your setup.


ATB,

Mark.

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Finn Thain]

On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:

> On 05/07/2021 02:36, Finn Thain wrote:
> 
> > > Unfortunately I don't have a test mips64el image available to see if 
> > > this combination works for Linux. Phil, do you have a suitable test 
> > > kernel and rootfs image available to allow this to be tested?
> > > 
> > 
> > You can build and boot a mipsel vmlinux by following the steps I 
> > described previously. In the kernel messages you'll see the jazzsonic 
> > driver attempt to probe the device. When it succeeds, you'll see the 
> > MAC address reported. You can also observe the regression I reported 
> > with regards to patch 2/6, "dp8393x: don't force 32-bit register 
> > access".
> 
> Those instructions are useful, but since I am not a MIPS developer I 
> don't have an existing toolchain/kernel tree and rootfs available to 
> test this.
> 

You don't need a rootfs to see the jazzsonic driver messages. But if you 
still want one, you could try the mipsel builds from these distros (not 
the 64-bit ones):

https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/
https://landley.net/aboriginal/downloads/binaries/

> If you can provide me with a link to your vmlinux and rootfs with 
> busybox or similar in it, I can take a look to see what is happening 
> here. Otherwise it's almost impossible for me to understand and debug 
> the problem you are seeing on your setup.
> 

Uploading kernels is a hassle (for me) as it brings a trust question and 
requires a file hosting service. I really should use PGP and organise a 
web of trust but that's very difficult given my rural location.

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Mark Cave-Ayland]

On 07/07/2021 02:30, Finn Thain wrote:

> On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:
> 
>> On 05/07/2021 02:36, Finn Thain wrote:
>>
>>>> Unfortunately I don't have a test mips64el image available to see if
>>>> this combination works for Linux. Phil, do you have a suitable test
>>>> kernel and rootfs image available to allow this to be tested?
>>>>
>>>
>>> You can build and boot a mipsel vmlinux by following the steps I
>>> described previously. In the kernel messages you'll see the jazzsonic
>>> driver attempt to probe the device. When it succeeds, you'll see the
>>> MAC address reported. You can also observe the regression I reported
>>> with regards to patch 2/6, "dp8393x: don't force 32-bit register
>>> access".
>>
>> Those instructions are useful, but since I am not a MIPS developer I
>> don't have an existing toolchain/kernel tree and rootfs available to
>> test this.
>>
> 
> You don't need a rootfs to see the jazzsonic driver messages. But if you
> still want one, you could try the mipsel builds from these distros (not
> the 64-bit ones):
> 
> https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/
> https://landley.net/aboriginal/downloads/binaries/

That's true, but then this wouldn't enable testing of Phil's proposed CRC changes. 
Having a simple shell with ping and wget/curl is a real help here.

>> If you can provide me with a link to your vmlinux and rootfs with
>> busybox or similar in it, I can take a look to see what is happening
>> here. Otherwise it's almost impossible for me to understand and debug
>> the problem you are seeing on your setup.
>>
> 
> Uploading kernels is a hassle (for me) as it brings a trust question and
> requires a file hosting service. I really should use PGP and organise a
> web of trust but that's very difficult given my rural location.

Given that these are only running in a VM I'm not too worried about trust. I also 
have a VPS with scp access that I could temporarily grant you access via an SSH 
public key if that helps?


ATB,

Mark.

Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Finn Thain]

On Wed, 7 Jul 2021, Mark Cave-Ayland wrote:

> > You don't need a rootfs to see the jazzsonic driver messages. But if 
> > you still want one, you could try the mipsel builds from these distros 
> > (not the 64-bit ones):
> > 
> > https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/

When I tried following my own advice I ran into ABI compatibility 
problems. It looks like my kernel build doesn't like those binaries, but 
maybe it's a limitation of the Magnum CPU...

...
Run /bin/sh as init process
request_module: kmod_concurrent_max (0) close to 0 (max_modprobes: 50), for module binfmt-464c, throttling...
request_module: modprobe binfmt-464c cannot be processed, kmod busy with 50 threads for more than 5 seconds now
Kernel panic - not syncing: Requested init /bin/sh failed (error -8).

> > https://landley.net/aboriginal/downloads/binaries/

The binaries from Aboriginal Linux work okay (that is, rootfs.cpio.gz 
found in system-image-mipsel.tar.gz). I got a shell prompt using 
init=/bin/sh but there's no networking support in this minimal busybox 
build unfortunately.

Those binaries have the same ABI as the ones in my busybox build:

$ file busybox
busybox: ELF 32-bit LSB executable, MIPS, MIPS-I version 1 (SYSV), statically linked, stripped

Whereas, Debian/mipsel binaries (like the Gentoo ones) look like this:

$ file busybox
busybox: ELF 32-bit LSB pie executable, MIPS, MIPS32 rel2 version 1 (SYSV), dynamically linked, interpreter /lib/ld.so.1, for GNU/Linux 3.2.0, BuildID[sha1]=febe1809f2ad8dacb067dfd74505b19c6c69ba65, stripped

Eventually I found this page, https://wiki.debian.org/MIPSPort 
which explains that the Debian/mipsel port switched ABI between Debian 8 
and Debian 9.

Unfortunately, the Debian 7 and 8 installer ISO images have no initrd so 
they are no use. I got them from this archive:
https://cdimage.debian.org/cdimage/archive/

Anyway, the Debian 8 binaries look like this, and they work too:

# file bin/dash
bin/dash: ELF 32-bit LSB shared object, MIPS, MIPS-II version 1 (SYSV), dynamically linked, interpreter /lib/ld.so.1, for GNU/Linux 2.6.32, BuildID[sha1]=44f7c1d61d9941db2b1de5dd9629c99e06c30ea8, stripped

> 
> That's true, but then this wouldn't enable testing of Phil's proposed 
> CRC changes. Having a simple shell with ping and wget/curl is a real 
> help here.
> 

To generate network traffic you can get the kernel to configure the NIC 
over DHCP but that does require a different kernel config (see below).

> > > If you can provide me with a link to your vmlinux and rootfs with 
> > > busybox or similar in it, I can take a look to see what is happening 
> > > here. Otherwise it's almost impossible for me to understand and 
> > > debug the problem you are seeing on your setup.
> > > 
> > 
> > Uploading kernels is a hassle (for me) as it brings a trust question 
> > and requires a file hosting service. I really should use PGP and 
> > organise a web of trust but that's very difficult given my rural 
> > location.
> 
> Given that these are only running in a VM I'm not too worried about 
> trust.

Well, untrusted images are okay as long as we are talking about debugging 
QEMU issues that are not exploitable...

With a bit of searching I was able to find a Debian/mipsel 8 rootfs at 
https://github.com/jubinson/debian-rootfs

I can't vouch for it though. It appears to be a page of links to tar files 
in someone's dropbox.

$ sha256sum mipsel-rootfs-20170318T103423Z.tar.gz 
e6ed1871b29317c85170a07621966a013951ced1c5fb8d679b7519996b803fe8  mipsel-rootfs-20170318T103423Z.tar.gz

> I also have a VPS with scp access that I could temporarily grant you 
> access via an SSH public key if that helps?
> 

Thanks for the offer. But that wouldn't help anyone else reading this.

In anycase, I wanted to see whether a real distro could be used. So my 
plan was to cross-compile a Linux kernel and debootstrap a Debian/mipsel 
rootfs disk image. The first stage of a debootstrap installation runs on 
the host...


sudo -s
truncate -s 1G jessie-mipsel.img
mke2fs jessie-mipsel.img 
mount -o loop jessie-mipsel.img /mnt

wget -q -O- https://ftp-master.debian.org/keys/release-8.asc | gpg --import --no-default-keyring --keyring ./debian-release-8.gpg

debootstrap --keyring=./debian-release-8.gpg --foreign --arch=mipsel jessie /mnt http://archive.debian.org/debian/

umount /mnt
exit

git clone git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
cd linux
git checkout linux-5.10.y
make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- clean jazz_defconfig
scripts/config -d IPV6 -d WIRELESS -d WLAN -d DEBUG_KERNEL -d EXPERT -d CC_OPTIMIZE_FOR_PERFORMANCE -e CC_OPTIMIZE_FOR_SIZE -e ISO9660_FS -m EXT3_FS -e NFS_FS -e IP_PNP -e ROOT_NFS -e NFS_V2 -e IP_PNP_DHCP -e CMDLINE_BOOL -e MIPS_CMDLINE_BUILTIN_EXTEND --set-str CMDLINE "console=ttyS0 root=/dev/nfs rw"
make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- olddefconfig vmlinux

mkisofs -o vmlinux.iso -J -iso-level 3 vmlinux

qemu-system-mips64el -M magnum -L . -drive if=scsi,unit=0,format=raw,file=jessie-mipsel.img -drive if=scsi,unit=2,readonly=y,media=cdrom,format=raw,file=vmlinux.iso -drive if=scsi,unit=4,readonly=y,media=cdrom,format=raw,file=NetBSD-9.2-arc.iso -global ds1225y.filename=nvram -serial mon:stdio -serial null -net nic,model=dp83932,addr=00:00:00:aa:bb:cc -net bridge


 Actions:

     Start Windows NT 
->   Run a program
     Run setup


scsi(0)cdrom(4)fdisk(0)boot scsi(0)cdrom(2)fdisk(0)vmlinux root=/dev/sda init=/bin/sh


This boots to a shell prompt. Well, so far, so good. Now the rest of the 
debootstrap installation can be completed by the guest...


mount -t devtmpfs none /dev
mount -t proc none /proc
mount -t sysfs none /sys

/debootstrap/debootstrap --second-stage


Unfortunately, this produced a badly corrupted root filesystem and the 
installation failed. Seems to be a bug in either Linux/mipsel or QEMU. 
(Has anyone tried a NetBSD installation?)

I was able to avoid all that block IO entirely by using NFS. That way, the 
installation completed successfully...


mount -o loop jessie-mipsel.img /export/jessie-mipsel
echo "/export/jessie-mipsel 192.168.66.0/24(sync,rw,insecure,no_root_squash,no_subtree_check)" >> /etc/exports
/etc/init.d/nfs start

qemu-system-mips64el -M magnum -L . -drive if=scsi,unit=2,readonly=y,media=cdrom,format=raw,file=vmlinux.iso -drive if=scsi,unit=4,readonly=y,media=cdrom,format=raw,file=NetBSD-9.2-arc.iso -global ds1225y.filename=nvram -serial mon:stdio -serial null -net nic,model=dp83932,addr=00:00:00:aa:bb:cc -net bridge


 Actions:

     Start Windows NT 
->   Run a program
     Run setup


scsi(0)cdrom(4)fdisk(0)boot scsi(0)cdrom(2)fdisk(0)vmlinux ip=dhcp nfsroot=192.168.66.1:/export/jessie-mipsel init=/bin/sh


NetBSD/arc Bootstrap, Revision 1.1 (Wed May 12 13:15:55 UTC 2021)
devopen: scsi(0)cdrom(2)fdisk(0) type disk file vmlinux
5706728+198952 [840960+1017335]=0x767d14
Linux version 5.10.47 (fthain@nippy) (mipsel-linux-gnu-gcc (btc) 6.4.0, GNU ld (btc) 2.28) #2 Fri Jul 9 16:28:12 AEST 2021
ARCH: Microsoft-Jazz
PROMLIB: ARC firmware Version 1 Revision 2
CPU0 revision is: 00000400 (R4000PC)
FPU revision is: 00000500
Primary instruction cache 8kB, VIPT, direct mapped, linesize 16 bytes.
Primary data cache 8kB, direct mapped, VIPT, cache aliases, linesize 16 bytes
Zone ranges:
  DMA      [mem 0x0000000000000000-0x0000000000ffffff]
  Normal   [mem 0x0000000001000000-0x0000000007ffffff]
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x0000000000000000-0x0000000007ffffff]
Initmem setup node 0 [mem 0x0000000000000000-0x0000000007ffffff]
Built 1 zonelists, mobility grouping on.  Total pages: 32512
Kernel command line: console=ttyS0 root=/dev/nfs rw scsi(0)cdrom(2)fdisk(0)vmlinux ip=dhcp nfsroot=192.168.66.1:/export/jessie-mipsel init=/bin/sh
Dentry cache hash table entries: 16384 (order: 4, 65536 bytes, linear)
Inode-cache hash table entries: 8192 (order: 3, 32768 bytes, linear)
mem auto-init: stack:off, heap alloc:off, heap free:off
Memory: 123668K/131072K available (4249K kernel code, 234K rwdata, 928K rodata, 212K init, 135K bss, 7404K reserved, 0K cma-reserved)
NR_IRQS: 256
random: get_random_bytes called from start_kernel+0x300/0x4b0 with crng_init=0
Console: colour dummy device 80x25
sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every 21474836475000000ns
Calibrating delay loop... 1404.10 BogoMIPS (lpj=7020544)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
devtmpfs: initialized
clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
futex hash table entries: 256 (order: -1, 3072 bytes, linear)
NET: Registered protocol family 16
VDMA: R4030 DMA pagetables initialized.
SCSI subsystem initialized
NET: Registered protocol family 2
IP idents hash table entries: 2048 (order: 2, 16384 bytes, linear)
tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes, linear)
TCP established hash table entries: 1024 (order: 0, 4096 bytes, linear)
TCP bind hash table entries: 1024 (order: 0, 4096 bytes, linear)
TCP: Hash tables configured (established 1024 bind 1024)
UDP hash table entries: 256 (order: 0, 4096 bytes, linear)
UDP-Lite hash table entries: 256 (order: 0, 4096 bytes, linear)
NET: Registered protocol family 1
RPC: Registered named UNIX socket transport module.
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
RPC: Registered tcp NFSv4.1 backchannel transport module.
workingset: timestamp_bits=30 max_order=15 bucket_order=0
Block layer SCSI generic (bsg) driver version 0.4 loaded (major 254)
io scheduler mq-deadline registered
io scheduler kyber registered
Console: switching to colour frame buffer device 100x37
Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
printk: console [ttyS0] disabled
serial8250.0: ttyS0 at MMIO 0xe0006000 (irq = 32, base_baud = 115200) is a 16550A
printk: console [ttyS0] enabled
serial8250.0: ttyS1 at MMIO 0xe0007000 (irq = 33, base_baud = 115200) is a 16550A
jazz_esp jazz_esp.0: esp0: regs[(ptrval):0] irq[29]
jazz_esp jazz_esp.0: esp0: is a FAS100A, 40 MHz (ccf=0), SCSI ID 7
random: fast init done
scsi host0: esp
PDC20230-C/20630 VLB ATA controller detected.
scsi host1: pata_legacy
ata1: PATA max PIO2 cmd 0x1f0 ctl 0x3f6 irq 14
scsi 0:0:2:0: CD-ROM            QEMU     QEMU CD-ROM      2.5+ PQ: 0 ANSI: 5
scsi target0:0:2: Beginning Domain Validation
VDMA: Channel 0: Memory error!
VDMA: Channel 0: Memory error!
VDMA: Channel 0: Memory error!
scsi target0:0:2: Domain Validation skipping write tests
scsi target0:0:2: Ending Domain Validation
VDMA: Channel 0: Memory error!
scsi 0:0:4:0: CD-ROM            QEMU     QEMU CD-ROM      2.5+ PQ: 0 ANSI: 5
scsi target0:0:4: Beginning Domain Validation
VDMA: Channel 0: Memory error!
VDMA: Channel 0: Memory error!
VDMA: Channel 0: Memory error!
scsi target0:0:4: Domain Validation skipping write tests
scsi target0:0:4: Ending Domain Validation
VDMA: Channel 0: Memory error!
scsi host1: pata_legacy
ata2: PATA max PIO4 cmd 0x170 ctl 0x376 irq 15
scsi host1: pata_legacy
ata3: PATA max PIO4 cmd 0x1e8 ctl 0x3ee irq 11
scsi host1: pata_legacy
ata4: PATA max PIO4 cmd 0x168 ctl 0x36e irq 10
scsi host1: pata_legacy
ata5: PATA max PIO4 cmd 0x1e0 ctl 0x3e6 irq 8
scsi host1: pata_legacy
ata6: PATA max PIO4 cmd 0x160 ctl 0x366 irq 12
SONIC ethernet @e0001000, MAC 52:54:00:12:34:56, IRQ 28
serio: i8042 KBD port at 0xe0005000,0xe0005001 irq 30
serio: i8042 AUX port at 0xe0005000,0xe0005001 irq 31
input: AT Raw Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
Sending DHCP requests ., OK
IP-Config: Got DHCP answer from 192.168.66.1, my address is 192.168.66.112
IP-Config: Complete:
     device=eth0, hwaddr=52:54:00:12:34:56, ipaddr=192.168.66.112, mask=255.255.255.0, gw=192.168.66.1
     host=192.168.66.112, domain=local, nis-domain=(none)
     bootserver=0.0.0.0, rootserver=192.168.66.1, rootpath=
     nameserver0=139.130.4.4
input: ImExPS/2 Generic Explorer Mouse as /devices/platform/i8042/serio1/input/input2
VFS: Mounted root (nfs filesystem) on device 0:12.
Freeing prom memory: 376k freed
Freeing prom memory: 60k freed
Freeing prom memory: 4k freed
Freeing unused kernel memory: 212K
This architecture does not have kernel memory protection.
Run /bin/sh as init process
process '/bin/dash' started with executable stack
/bin/sh: 0: can't access tty; job control turned off
# mount -t devtmpfs none /dev
# mount -t proc none /proc
# mount -t sysfs none /sys
# /debootstrap/debootstrap --second-stage
...
# ping -c3 192.168.66.1
PING 192.168.66.1 (192.168.66.1) 56(84) bytes of data.
64 bytes from 192.168.66.1: icmp_seq=1 ttl=64 time=10.0 ms
64 bytes from 192.168.66.1: icmp_seq=2 ttl=64 time=0.000 ms
64 bytes from 192.168.66.1: icmp_seq=3 ttl=64 time=0.000 ms

--- 192.168.66.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2030ms
rtt min/avg/max/mdev = 0.000/3.333/10.000/4.714 ms
# 

After the debootstrap command completes, the last steps are manual 
configuration, as per the usual debootstrap procedure:
https://wiki.debian.org/Debootstrap
https://gist.github.com/varqox/42e213b6b2dde2b636ef

Note that this only _barely_ works. A slightly larger vmlinux breaks the 
bootloader, and a slightly longer boot command breaks ARC.