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

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

[Mark Cave-Ayland]

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

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

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

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 11810c9b60..d16ade2b19 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.20.1

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

[Finn Thain]

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

> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses
> to the registers were 32-bit 

As I said, that assumption was not made there.

If commit 3fe9a838ec is deficient it is probably because I am unaware of 
the ability of the QEMU memory API to accomplish the desired result. 

That's not to say that the API can't do it, just that I don't know enough 
about the API.

> 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.
> 

Again, that is an over-simplification. To explain: in Apple hardware at 
least, the access size that the SONIC chip sees is a consequence of bus 
sizing logic that is not part of the CPU and is not part of the SONIC chip 
either.

AIUI, this logic is what Philippe alluded to when he said about this 
patch, "This sounds to me like the 'QEMU doesn't model busses so we end 
using kludge to hide bugs' pattern".

> 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 and
> .impl.max_accessto 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")

There is a 'fixes' tag here but it's unclear what bug is being fixed. I 
think this commit log entry would be more helpful if it mentioned the bug 
that was observed.

> ---
>  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 11810c9b60..d16ade2b19 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,
>  };
>  
> 

Again, this patch breaks my Linux/mipsel guest. Perhaps you did not 
receive my message about that regression? It did make it into the list 
archives... 
https://lore.kernel.org/qemu-devel/20210703141947.352295-1-f4bug@amsat.org/T/#m8ef6d91fd8e38b01e375083058902342970b8833

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

[Mark Cave-Ayland]

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

> On Sun, 4 Jul 2021, Mark Cave-Ayland wrote:
> 
>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses
>> to the registers were 32-bit
> 
> As I said, that assumption was not made there.
> 
> If commit 3fe9a838ec is deficient it is probably because I am unaware of
> the ability of the QEMU memory API to accomplish the desired result.
> 
> That's not to say that the API can't do it, just that I don't know enough
> about the API.
> 
>> 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.
>>
> 
> Again, that is an over-simplification. To explain: in Apple hardware at
> least, the access size that the SONIC chip sees is a consequence of bus
> sizing logic that is not part of the CPU and is not part of the SONIC chip
> either.
> 
> AIUI, this logic is what Philippe alluded to when he said about this
> patch, "This sounds to me like the 'QEMU doesn't model busses so we end
> using kludge to hide bugs' pattern".

Sure I understand this, and some of the interesting logic Apple has for decoding 
memory accesses. However the MacOS toolbox ROM works fine with this change combining 
the 2 separate accesses, and it is the jazzsonic driver accesses which are failing 
and need to be understood.

>> 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 and
>> .impl.max_accessto 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")
> 
> There is a 'fixes' tag here but it's unclear what bug is being fixed. I
> think this commit log entry would be more helpful if it mentioned the bug
> that was observed.
> 
>> ---
>>   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 11810c9b60..d16ade2b19 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,
>>   };
>>   
>>
> 
> Again, this patch breaks my Linux/mipsel guest. Perhaps you did not
> receive my message about that regression? It did make it into the list
> archives...
> https://lore.kernel.org/qemu-devel/20210703141947.352295-1-f4bug@amsat.org/T/#m8ef6d91fd8e38b01e375083058902342970b8833

I did see this, but as per my follow up message I wanted to make sure that everyone 
was using the same patches first as you needed a combination of an in-flight PR plus 
the correct versions of all the patches from over the weekend.

Looking at the jazzsonic code I see the SONIC_READ() macro at 
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/natsemi/jazzsonic.c#L56 
is using a pointer to an unsigned int. Is an unsigned int 4 bytes on mips64el? If so 
the proposed change would return 2 registers in the same 4-byte result which is what 
is likely to be confusing the driver.

I think the problem is because of the interaction of .impl.max_access_size = 2 and 
the it_shift property specifying a stride of 4 bytes: when the 4 byte access is split 
into 2 x 2 byte accesses then for a read reg = addr >> s->it_shift causes the second 
access to be a duplicate of the first rather than containing zeros.

Again if you can provide a link to your existing vmlinux and busybox rootfs then I 
should be able to get to the bottom of this fairly quickly.


ATB,

Mark.

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

[Mark Cave-Ayland]

On 05/07/2021 08:52, Mark Cave-Ayland wrote:

> I think the problem is because of the interaction of .impl.max_access_size = 2 and 
> the it_shift property specifying a stride of 4 bytes: when the 4 byte access is split 
> into 2 x 2 byte accesses then for a read reg = addr >> s->it_shift causes the second 
> access to be a duplicate of the first rather than containing zeros.

After some more experiments I'm fairly confident this is the issue: if you rely on 
applying it_shift within the MMIO access itself then you lose the zero extension of 
the result to the access size as done by the memory API.

I'll come up with a new version which I shall send as part of an updated and tested 
v2 of Phil's housekeeping patchset, since the endian changes were really helpful when 
studying the descriptors.


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

On 7/5/21 3:44 AM, Finn Thain wrote:
> On Sun, 4 Jul 2021, Mark Cave-Ayland wrote:
> 
>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses
>> to the registers were 32-bit 
> 
> As I said, that assumption was not made there.
> 
> If commit 3fe9a838ec is deficient it is probably because I am unaware of 
> the ability of the QEMU memory API to accomplish the desired result. 
> 
> That's not to say that the API can't do it, just that I don't know enough 
> about the API.
> 
>> 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.
>>
> 
> Again, that is an over-simplification. To explain: in Apple hardware at 
> least, the access size that the SONIC chip sees is a consequence of bus 
> sizing logic that is not part of the CPU and is not part of the SONIC chip 
> either.
> 
> AIUI, this logic is what Philippe alluded to when he said about this 
> patch, "This sounds to me like the 'QEMU doesn't model busses so we end 
> using kludge to hide bugs' pattern".

For the Jazz Magnum, the bus is partly represented in this datasheet:
https://datasheet.datasheetarchive.com/originals/scans/Scans-054/DSAIH000102184.pdf

I found the individual technical datasheet once, now I need to find them
again :/

I'll try to implement the missing parts in the next dev cycle.

Regards,

Phil.

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

[Philippe Mathieu-Daudé]

On 7/5/21 9:33 PM, Mark Cave-Ayland wrote:
> On 05/07/2021 08:52, Mark Cave-Ayland wrote:
> 
>> I think the problem is because of the interaction of
>> .impl.max_access_size = 2 and the it_shift property specifying a
>> stride of 4 bytes: when the 4 byte access is split into 2 x 2 byte
>> accesses then for a read reg = addr >> s->it_shift causes the second
>> access to be a duplicate of the first rather than containing zeros.

I believe this is something I tried to fix earlier, see:

"hw/misc: Add support for interleaved memory accesses"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg730721.html
(in particular:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg730725.html)

and

"hw/misc: Add memory_region_add_subregion_aliased() helper"
https://www.mail-archive.com/qemu-block@nongnu.org/msg83742.html

I plan to respin during next dev cycle...

> After some more experiments I'm fairly confident this is the issue: if
> you rely on applying it_shift within the MMIO access itself then you
> lose the zero extension of the result to the access size as done by the
> memory API.
> 
> I'll come up with a new version which I shall send as part of an updated
> and tested v2 of Phil's housekeeping patchset, since the endian changes
> were really helpful when studying the descriptors.
I'm fine if we take the patch Finn has tested (I'll amend a comment
explaining the limitations) and we fix the details in the next cycle.