[PATCH 0/4] dp8393x: fixes and improvements

[PATCH 0/4] dp8393x: fixes and improvements

[Mark Cave-Ayland]

(was: [RFC PATCH 0/6] dp8393x: Housekeeping)

This is an updated version of Phil's patchset previously posted at
https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg00440.html which
I've tested locally on Linux/m68k, MacOS and NetBSD/arc.

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


Changes since RFC:
- Drop patch 1 "dp8393x: fix CAM descriptor entry index" from this patchset as
  it has already been queued to mips-next
- Update patch 2 "dp8393x: don't force 32-bit register access" including improved
  comment explaining the current solution
- Drop patch 3 "dp8393x: Restrict bus access to 16/32-bit operations" since it
  causes a regression with MacOS
- Fix offsets in patch 6 "dp8393x: Rewrite dp8393x_get() / dp8393x_put()"

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


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

Philippe Mathieu-Daudé (3):
  dp8393x: Replace address_space_rw(is_write=1) by address_space_write()
  dp8393x: Store CAM registers as 16-bit
  dp8393x: Rewrite dp8393x_get() / dp8393x_put()

 hw/net/dp8393x.c | 195 ++++++++++++++++++++---------------------------
 1 file changed, 81 insertions(+), 114 deletions(-)

-- 
2.20.1

[PATCH 1/4] dp8393x: don't force 32-bit register access

[Mark Cave-Ayland]

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

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

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

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

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

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

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

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

[Philippe Mathieu-Daudé]

On 7/5/21 11:49 PM, Mark Cave-Ayland wrote:
> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set .impl.min_access_size
> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver which uses
> 32-bit accesses.
> 
> The problem with forcing the register access to 32-bit in this way is that since the
> dp8393x uses 16-bit registers, a manual endian swap is required for devices on big
> endian machines with 32-bit accesses.
> 
> For both access sizes and machine endians the QEMU memory API can do the right thing
> automatically: all that is needed is to set .impl.min_access_size to 2 to declare that
> the dp8393x implements 16-bit registers.
> 
> Normally .impl.max_access_size should also be set to 2, however that doesn't quite
> work in this case since the register stride is specified using a (dynamic) it_shift
> property which is applied during the MMIO access itself. The effect of this is that
> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use of
> it_shift within the MMIO access itself causes the register value to be repeated in both
> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the stride to be
> zero-extended up to access size and therefore fails to correctly detect the dp8393x
> device due to the extra data in the top 16-bits.
> 
> The solution here is to remove .impl.max_access_size so that the memory API will
> correctly zero-extend the 16-bit registers to the access size up to and including
> it_shift. Since it_shift is never greater than 2 than this will always do the right
> thing for both 16-bit and 32-bit accesses regardless of the machine endian, allowing
> the manual endian swap code to be removed.

Removing .impl.max_access_size means now it has default, which is 4.
See access_with_adjusted_size:

static MemTxResult access_with_adjusted_size(hwaddr addr,
                                      uint64_t *value,
                                      unsigned size,
                                      unsigned access_size_min,
                                      unsigned access_size_max,
    ...
    if (!access_size_min) {
        access_size_min = 1;
    }
    if (!access_size_max) {
        access_size_max = 4;
    }

called as:

    access_with_adjusted_size(addr, &data, size,
                              mr->ops->impl.min_access_size,
                              mr->ops->impl.max_access_size,
                              memory_region_write_with_attrs_accessor,
                              mr, attrs);

So if you don't mind I'll keep .impl.max_access_size = 4 and update
the comment.

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> ---
>  hw/net/dp8393x.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b60..44a1955015 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);
>  
> @@ -691,11 +690,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>      }
>  }
>  
> +/*
> + * Since .impl.max_access_size is effectively controlled by the it_shift
> + * property, leave it unspecified for now to allow the memory API to
> + * correctly zero extend the 16-bit register values to the access size up to and
> + * including it_shift.
> + */
>  static const MemoryRegionOps dp8393x_ops = {
>      .read = dp8393x_read,
>      .write = dp8393x_write,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> +    .impl.min_access_size = 2,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> 

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

[Mark Cave-Ayland]

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

> On 7/5/21 11:49 PM, Mark Cave-Ayland wrote:
>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set .impl.min_access_size
>> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver which uses
>> 32-bit accesses.
>>
>> The problem with forcing the register access to 32-bit in this way is that since the
>> dp8393x uses 16-bit registers, a manual endian swap is required for devices on big
>> endian machines with 32-bit accesses.
>>
>> For both access sizes and machine endians the QEMU memory API can do the right thing
>> automatically: all that is needed is to set .impl.min_access_size to 2 to declare that
>> the dp8393x implements 16-bit registers.
>>
>> Normally .impl.max_access_size should also be set to 2, however that doesn't quite
>> work in this case since the register stride is specified using a (dynamic) it_shift
>> property which is applied during the MMIO access itself. The effect of this is that
>> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use of
>> it_shift within the MMIO access itself causes the register value to be repeated in both
>> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the stride to be
>> zero-extended up to access size and therefore fails to correctly detect the dp8393x
>> device due to the extra data in the top 16-bits.
>>
>> The solution here is to remove .impl.max_access_size so that the memory API will
>> correctly zero-extend the 16-bit registers to the access size up to and including
>> it_shift. Since it_shift is never greater than 2 than this will always do the right
>> thing for both 16-bit and 32-bit accesses regardless of the machine endian, allowing
>> the manual endian swap code to be removed.
> 
> Removing .impl.max_access_size means now it has default, which is 4.
> See access_with_adjusted_size:
> 
> static MemTxResult access_with_adjusted_size(hwaddr addr,
>                                        uint64_t *value,
>                                        unsigned size,
>                                        unsigned access_size_min,
>                                        unsigned access_size_max,
>      ...
>      if (!access_size_min) {
>          access_size_min = 1;
>      }
>      if (!access_size_max) {
>          access_size_max = 4;
>      }
> 
> called as:
> 
>      access_with_adjusted_size(addr, &data, size,
>                                mr->ops->impl.min_access_size,
>                                mr->ops->impl.max_access_size,
>                                memory_region_write_with_attrs_accessor,
>                                mr, attrs);
> 
> So if you don't mind I'll keep .impl.max_access_size = 4 and update
> the comment.

As per the comment, the removal of .impl.max_access_size was to imply that the 
ultimate limit is determined by a dynamic property more than the hard-coded limit i.e 
if you wanted to increase the stride you would increase it_shift first and then 
adjust the .impl.max_access_size to match accordingly.

At this point we're probably heading into personal preference territory, so if you 
are happy to merge this via mips-next then I'm happy for you to make the final 
decision :)

>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
>> ---
>>   hw/net/dp8393x.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 11810c9b60..44a1955015 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);
>>   
>> @@ -691,11 +690,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>>       }
>>   }
>>   
>> +/*
>> + * Since .impl.max_access_size is effectively controlled by the it_shift
>> + * property, leave it unspecified for now to allow the memory API to
>> + * correctly zero extend the 16-bit register values to the access size up to and
>> + * including it_shift.
>> + */
>>   static const MemoryRegionOps dp8393x_ops = {
>>       .read = dp8393x_read,
>>       .write = dp8393x_write,
>> -    .impl.min_access_size = 4,
>> -    .impl.max_access_size = 4,
>> +    .impl.min_access_size = 2,
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>   };


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

On 7/6/21 9:22 PM, Mark Cave-Ayland wrote:
> On 06/07/2021 18:18, Philippe Mathieu-Daudé wrote:
>> On 7/5/21 11:49 PM, Mark Cave-Ayland wrote:
>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set
>>> .impl.min_access_size
>>> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic
>>> driver which uses
>>> 32-bit accesses.
>>>
>>> The problem with forcing the register access to 32-bit in this way is
>>> that since the
>>> dp8393x uses 16-bit registers, a manual endian swap is required for
>>> devices on big
>>> endian machines with 32-bit accesses.
>>>
>>> For both access sizes and machine endians the QEMU memory API can do
>>> the right thing
>>> automatically: all that is needed is to set .impl.min_access_size to
>>> 2 to declare that
>>> the dp8393x implements 16-bit registers.
>>>
>>> Normally .impl.max_access_size should also be set to 2, however that
>>> doesn't quite
>>> work in this case since the register stride is specified using a
>>> (dynamic) it_shift
>>> property which is applied during the MMIO access itself. The effect
>>> of this is that
>>> for a 32-bit access the memory API performs 2 x 16-bit accesses, but
>>> the use of
>>> it_shift within the MMIO access itself causes the register value to
>>> be repeated in both
>>> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver
>>> expects the stride to be
>>> zero-extended up to access size and therefore fails to correctly
>>> detect the dp8393x
>>> device due to the extra data in the top 16-bits.
>>>
>>> The solution here is to remove .impl.max_access_size so that the
>>> memory API will
>>> correctly zero-extend the 16-bit registers to the access size up to
>>> and including
>>> it_shift. Since it_shift is never greater than 2 than this will
>>> always do the right
>>> thing for both 16-bit and 32-bit accesses regardless of the machine
>>> endian, allowing
>>> the manual endian swap code to be removed.
>>
>> Removing .impl.max_access_size means now it has default, which is 4.
>> See access_with_adjusted_size:
>>
>> static MemTxResult access_with_adjusted_size(hwaddr addr,
>>                                        uint64_t *value,
>>                                        unsigned size,
>>                                        unsigned access_size_min,
>>                                        unsigned access_size_max,
>>      ...
>>      if (!access_size_min) {
>>          access_size_min = 1;
>>      }
>>      if (!access_size_max) {
>>          access_size_max = 4;
>>      }
>>
>> called as:
>>
>>      access_with_adjusted_size(addr, &data, size,
>>                                mr->ops->impl.min_access_size,
>>                                mr->ops->impl.max_access_size,
>>                                memory_region_write_with_attrs_accessor,
>>                                mr, attrs);
>>
>> So if you don't mind I'll keep .impl.max_access_size = 4 and update
>> the comment.
> 
> As per the comment, the removal of .impl.max_access_size was to imply
> that the ultimate limit is determined by a dynamic property more than
> the hard-coded limit i.e if you wanted to increase the stride you would
> increase it_shift first and then adjust the .impl.max_access_size to
> match accordingly.
> 
> At this point we're probably heading into personal preference territory,
> so if you are happy to merge this via mips-next then I'm happy for you
> to make the final decision :)

No, I'll take your patch. I missed the it_shift use, and I plan to kill
it next dev cycle because I'm tired by its misuses. Probably with a
new memory device similar to:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg795421.html

$ git grep -w it_shift | wc -l
50

Doable.

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

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

[Finn Thain]

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

> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set .impl.min_access_size
> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver which uses
> 32-bit accesses.
> 
> The problem with forcing the register access to 32-bit in this way is that since the
> dp8393x uses 16-bit registers, a manual endian swap is required for devices on big
> endian machines with 32-bit accesses.
> 
> For both access sizes and machine endians the QEMU memory API can do the right thing
> automatically: all that is needed is to set .impl.min_access_size to 2 to declare that
> the dp8393x implements 16-bit registers.
> 
> Normally .impl.max_access_size should also be set to 2, however that doesn't quite
> work in this case since the register stride is specified using a (dynamic) it_shift
> property which is applied during the MMIO access itself. The effect of this is that
> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use of
> it_shift within the MMIO access itself causes the register value to be repeated in both
> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the stride to be
> zero-extended up to access size and therefore fails to correctly detect the dp8393x
> device due to the extra data in the top 16-bits.
> 
> The solution here is to remove .impl.max_access_size so that the memory API will
> correctly zero-extend the 16-bit registers to the access size up to and including
> it_shift. Since it_shift is never greater than 2 than this will always do the right
> thing for both 16-bit and 32-bit accesses regardless of the machine endian, allowing
> the manual endian swap code to be removed.
> 

IIUC, this patch replaces an explicit word swap with an implicit byte 
swap. The explicit word swap was conditional on the big_endian flag.

This flag seems to work like the chip's BMODE pin which switches between 
Intel and Motorola bus modes (not just byte ordering but bus signalling in 
general). The BMODE pin or big_endian flag should effect a byte swap not a 
word swap so there must be a bug though it's not clear how that manifests.

Regardless of this patch, the big_endian flag also controls byte swapping 
during DMA by the device. IIUC, the flag is set to indicate that RAM is 
big_endian, so it's not actually a property of the dp8393x but of the 
RAM...

The Magnum hardware can run in big endian or little endian mode. But the 
SONIC chip must remain in little endian mode always because asserting 
BMODE would invoke Motorola signalling and that would contradict 
Philippe's datasheet which says that the SONIC device is attached to an 
"i386 compatible bus".

This seems contrary to mips_jazz_init(), which sets the dp8393x big_endian 
flag whenever TARGET_WORDS_BIGENDIAN is defined, i.e. risc/os guest. 

QEMU's dp8393x device has native endianness, so perhaps a big endian guest 
or a big endian host could trigger the bug that's being addressed in this 
patch.

Anyway, I think that this patch is heading in the right direction but 
can't it go further? Shouldn't the big_endian flag disappear altogether so 
that the memory API can also take care of the byte swapping needed by 
dp8393x_get() and dp8393x_put() for DMA?

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> ---
>  hw/net/dp8393x.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b60..44a1955015 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);
>  
> @@ -691,11 +690,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>      }
>  }
>  
> +/*
> + * Since .impl.max_access_size is effectively controlled by the it_shift
> + * property, leave it unspecified for now to allow the memory API to
> + * correctly zero extend the 16-bit register values to the access size up to and
> + * including it_shift.
> + */
>  static const MemoryRegionOps dp8393x_ops = {
>      .read = dp8393x_read,
>      .write = dp8393x_write,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> +    .impl.min_access_size = 2,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> 

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

[Mark Cave-Ayland]

On 07/07/2021 00:51, Finn Thain wrote:

> On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:
> 
>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set .impl.min_access_size
>> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver which uses
>> 32-bit accesses.
>>
>> The problem with forcing the register access to 32-bit in this way is that since the
>> dp8393x uses 16-bit registers, a manual endian swap is required for devices on big
>> endian machines with 32-bit accesses.
>>
>> For both access sizes and machine endians the QEMU memory API can do the right thing
>> automatically: all that is needed is to set .impl.min_access_size to 2 to declare that
>> the dp8393x implements 16-bit registers.
>>
>> Normally .impl.max_access_size should also be set to 2, however that doesn't quite
>> work in this case since the register stride is specified using a (dynamic) it_shift
>> property which is applied during the MMIO access itself. The effect of this is that
>> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use of
>> it_shift within the MMIO access itself causes the register value to be repeated in both
>> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the stride to be
>> zero-extended up to access size and therefore fails to correctly detect the dp8393x
>> device due to the extra data in the top 16-bits.
>>
>> The solution here is to remove .impl.max_access_size so that the memory API will
>> correctly zero-extend the 16-bit registers to the access size up to and including
>> it_shift. Since it_shift is never greater than 2 than this will always do the right
>> thing for both 16-bit and 32-bit accesses regardless of the machine endian, allowing
>> the manual endian swap code to be removed.
>>
> 
> IIUC, this patch replaces an explicit word swap with an implicit byte
> swap. The explicit word swap was conditional on the big_endian flag.
> 
> This flag seems to work like the chip's BMODE pin which switches between
> Intel and Motorola bus modes (not just byte ordering but bus signalling in
> general). The BMODE pin or big_endian flag should effect a byte swap not a
> word swap so there must be a bug though it's not clear how that manifests.
> 
> Regardless of this patch, the big_endian flag also controls byte swapping
> during DMA by the device. IIUC, the flag is set to indicate that RAM is
> big_endian, so it's not actually a property of the dp8393x but of the
> RAM...
> 
> The Magnum hardware can run in big endian or little endian mode. But the
> SONIC chip must remain in little endian mode always because asserting
> BMODE would invoke Motorola signalling and that would contradict
> Philippe's datasheet which says that the SONIC device is attached to an
> "i386 compatible bus".
> 
> This seems contrary to mips_jazz_init(), which sets the dp8393x big_endian
> flag whenever TARGET_WORDS_BIGENDIAN is defined, i.e. risc/os guest.
> 
> QEMU's dp8393x device has native endianness, so perhaps a big endian guest
> or a big endian host could trigger the bug that's being addressed in this
> patch.
> 
> Anyway, I think that this patch is heading in the right direction but
> can't it go further? Shouldn't the big_endian flag disappear altogether so
> that the memory API can also take care of the byte swapping needed by
> dp8393x_get() and dp8393x_put() for DMA?

Currently in QEMU the dp8393x device is connected directly to the system bus i.e. in 
effect the CPU. The CPU issues either a big-endian or little-endian access, the 
device declares what it would like to receive, and the memory API handles all the 
intermediate layers. In this case DEVICE_NATIVE_ENDIAN specifies it expects to 
receive accesses in the same endian as the machine and so everything "just works".

For this reason MMIO accesses generally shouldn't need any explicit byte swapping: if 
you're doing this then it's a good indicator that something is wrong. There are some 
special cases around for things like virtio, but we can ignore those for now.

The dp8393x_get() and dp8393x_put() functions are for bus master accesses from the 
device to the RAM, and these accesses are controlled by the "big_endian" property and 
the DW bit. As you point out the "big_endian" property is effectively the BMODE bit: 
in my datasheet I see nothing about this pin changing the signalling, only that it 
affects the byte ordering for RAM accesses.

Now it is highly likely that the BMODE bit should match the target endian, in which 
case we could eliminate the "big_endian" property as you suggest. However this 
conflicts with what you mention above that the SONIC is hard-coded into little-endian 
mode, in which case we would still need to keep it.

I've sent a fix for the CAP registers, but other than that I'm happy that the first 
patchset works fine here, and the consolidation of most of the endian swaps is a good 
tidy-up. If this works for you then I suggest we merge the first patchset including 
the CAP fix in time for freeze next week and go from there.

Certainly we can look to improve things in the future, but without anyone having a 
working big-endian MIPS image to test against, I don't think it's worth guessing what 
changes are required as we can easily double the length of this thread and still have 
no idea if any changes we've made are correct.


ATB,

Mark.

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

[Finn Thain]

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

> However this conflicts with what you mention above that the SONIC is 
> hard-coded into little-endian mode, in which case we would still need to 
> keep it.
> 

If you want to fully implement BMODE in QEMU then you'll need to abandon 
native endiannes for the device implementation. I was not proposing this 
as it implies more byte swapping.

In a real Magnum the SONIC chip is connected to a bus that's not modelled 
by QEMU. It follows that BMODE serves different purposes than big_endian. 

I pointed out several semantic differences between BMODE and big_endian, 
but I think the most significant of those was that endianness is already a 
property of the memory device being accessed for DMA. Yet big_endian is a 
property of the dp8393x device.

> Certainly we can look to improve things in the future, but without 
> anyone having a working big-endian MIPS image to test against, I don't 
> think it's worth guessing what changes are required as we can easily 
> double the length of this thread and still have no idea if any changes 
> we've made are correct.
> 

That argument can be applied to other patches in this series also.

Anyway, if we agree that the aim is ultimately to remove the big_endian 
flag then patch 4/4 should probably be re-evaluated in light of that.

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

[Mark Cave-Ayland]

On 08/07/2021 01:52, Finn Thain wrote:

> On Wed, 7 Jul 2021, Mark Cave-Ayland wrote:
> 
>> However this conflicts with what you mention above that the SONIC is
>> hard-coded into little-endian mode, in which case we would still need to
>> keep it.
>>
> 
> If you want to fully implement BMODE in QEMU then you'll need to abandon
> native endiannes for the device implementation. I was not proposing this
> as it implies more byte swapping.
> 
> In a real Magnum the SONIC chip is connected to a bus that's not modelled
> by QEMU. It follows that BMODE serves different purposes than big_endian.
> 
> I pointed out several semantic differences between BMODE and big_endian,
> but I think the most significant of those was that endianness is already a
> property of the memory device being accessed for DMA. Yet big_endian is a
> property of the dp8393x device.
> 
>> Certainly we can look to improve things in the future, but without
>> anyone having a working big-endian MIPS image to test against, I don't
>> think it's worth guessing what changes are required as we can easily
>> double the length of this thread and still have no idea if any changes
>> we've made are correct.
>>
> 
> That argument can be applied to other patches in this series also.
> 
> Anyway, if we agree that the aim is ultimately to remove the big_endian
> flag then patch 4/4 should probably be re-evaluated in light of that.

Other than fixing up the MMIO accesses to use the memory API, the patches in this 
series are just tidy-ups and refactorings i.e. no change in functionality related to 
the big_endian property. This is exactly the case for patch 4. If there were any 
changes to the big_endian logic required, then those would be handled by a separate 
patch (or patches) outside of this refactoring work.

As I said before I'm not opposed to this, but we're coming up to freeze in less than 
a week and no-one has been able to provide working MIPS big endian and little endian 
test images in a thread lasting 3 weeks. Therefore my recommendation would be to 
merge the series in its current form for 6.1 and then when someone has found time to 
generate working images and do the required analysis around the big_endian logic, we 
can consider further changes later.


ATB,

Mark.

[PATCH 2/4] dp8393x: Replace address_space_rw(is_write=1) by address_space_write()

[Mark Cave-Ayland]

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

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 44a1955015..cc7c001edb 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -820,8 +820,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];
@@ -850,8 +850,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.20.1

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

[Mark Cave-Ayland]

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

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 cc7c001edb..22ceea338c 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 */
@@ -990,7 +987,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.20.1

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

[Finn Thain]

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

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

> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> 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 cc7c001edb..22ceea338c 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 */

This patch incorrectly alters the behaviour of the jazzsonic.c driver 
which reads the MAC address from the CAP registers in sonic_probe1().

With mainline QEMU, the driver reports:
SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28

With this patch:
SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28

> @@ -990,7 +987,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()
>      }
> 

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

[Mark Cave-Ayland]

On 07/07/2021 00:48, Finn Thain wrote:

> On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:
> 
>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> 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 cc7c001edb..22ceea338c 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 */
> 
> This patch incorrectly alters the behaviour of the jazzsonic.c driver
> which reads the MAC address from the CAP registers in sonic_probe1().
> 
> With mainline QEMU, the driver reports:
> SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28
> 
> With this patch:
> SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28
> 
>> @@ -990,7 +987,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 was staring at the code in dp8393x_do_load_cam() trying to figure out how on earth 
this could be reading the wrong values from the CAM descriptors, when I realised the 
problem is actually in the read back from the CAP registers (it doesn't need the x2 
multiplier since the conversion from uint8_t to uint16_t).

This should be fixed by the following:

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 22ceea338c..09c34c3584 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -589,7 +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)];
+            val = s->cam[s->regs[SONIC_CEP] & 0xf][SONIC_CAP0 - reg];
          }
          break;
      /* All other registers have no special contraints */


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

On 7/7/21 11:08 AM, Mark Cave-Ayland wrote:
> On 07/07/2021 00:48, Finn Thain wrote:
> 
>> On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:
>>
>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> 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 cc7c001edb..22ceea338c 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 */
>>
>> This patch incorrectly alters the behaviour of the jazzsonic.c driver
>> which reads the MAC address from the CAP registers in sonic_probe1().
>>
>> With mainline QEMU, the driver reports:
>> SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28
>>
>> With this patch:
>> SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28
>>
>>> @@ -990,7 +987,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 was staring at the code in dp8393x_do_load_cam() trying to figure out
> how on earth this could be reading the wrong values from the CAM
> descriptors, when I realised the problem is actually in the read back
> from the CAP registers (it doesn't need the x2 multiplier since the
> conversion from uint8_t to uint16_t).
> 
> This should be fixed by the following:
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 22ceea338c..09c34c3584 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -589,7 +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)];
> +            val = s->cam[s->regs[SONIC_CEP] & 0xf][SONIC_CAP0 - reg];

Oops, thanks!

[PATCH 4/4] dp8393x: Rewrite dp8393x_get() / dp8393x_put()

[Mark Cave-Ayland]

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

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 | 160 +++++++++++++++++++----------------------------
 1 file changed, 63 insertions(+), 97 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 22ceea338c..a03f8f0837 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, int offset)
 {
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     uint16_t val;
 
-    if (s->big_endian) {
-        val = be16_to_cpu(s->data[offset * width + width - 1]);
+    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+        addr += offset << 2;
+        if (s->big_endian) {
+            val = address_space_ldl_be(&s->as, addr, attrs, NULL);
+        } else {
+            val = address_space_ldl_le(&s->as, addr, attrs, NULL);
+        }
     } else {
-        val = le16_to_cpu(s->data[offset * width]);
+        addr += offset << 1;
+        if (s->big_endian) {
+            val = address_space_lduw_be(&s->as, addr, attrs, NULL);
+        } else {
+            val = address_space_lduw_le(&s->as, addr, attrs, NULL);
+        }
     }
+
     return val;
 }
 
-static void dp8393x_put(dp8393xState *s, int width, int offset,
-                        uint16_t val)
+static void dp8393x_put(dp8393xState *s,
+                        hwaddr addr, int offset, uint16_t val)
 {
-    if (s->big_endian) {
-        if (width == 2) {
-            s->data[offset * 2] = 0;
-            s->data[offset * 2 + 1] = cpu_to_be16(val);
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+
+    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+        addr += offset << 2;
+        if (s->big_endian) {
+            address_space_stl_be(&s->as, addr, val, attrs, NULL);
         } else {
-            s->data[offset] = cpu_to_be16(val);
+            address_space_stl_le(&s->as, addr, val, attrs, NULL);
         }
     } else {
-        if (width == 2) {
-            s->data[offset * 2] = cpu_to_le16(val);
-            s->data[offset * 2 + 1] = 0;
+        addr += offset << 1;
+        if (s->big_endian) {
+            address_space_stw_be(&s->as, addr, val, attrs, NULL);
         } else {
-            s->data[offset] = cpu_to_le16(val);
+            address_space_stw_le(&s->as, addr, val, attrs, NULL);
         }
     }
 }
@@ -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), 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) {
@@ -457,15 +458,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
             i++;
             if (i != s->regs[SONIC_TFC]) {
                 /* Read next fragment details */
-                size = sizeof(uint16_t) * 3 * width;
-                address_space_read(&s->as,
-                                   dp8393x_ttda(s)
-                                   + sizeof(uint16_t) * width * (4 + 3 * i),
-                                   MEMTXATTRS_UNSPECIFIED, s->data,
-                                   size);
-                s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
-                s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
-                s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
+                s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s),
+                                                  4 + 3 * i);
+                s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s),
+                                                  5 + 3 * i);
+                s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s),
+                                                 6 + 3 * i);
             }
         }
 
@@ -498,22 +496,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;
@@ -765,7 +753,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;
 
@@ -778,10 +766,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;
     }
 
@@ -802,11 +788,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;
@@ -814,11 +796,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];
@@ -872,32 +850,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.20.1