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

[Finn Thain <fthain@linux-m68k.org>]

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