Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
> 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.

Hmm I'm not sure. This sounds to me like the "QEMU doesn't model
busses so we end using kludge to hide bugs" pattern. Can you
provide a QTest (ideally) or a "-trace memory_region_ops_\*" log
of your firmware accessing the dp8393x please?

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> ---
>  hw/net/dp8393x.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 252c0a2664..6789bcd3af 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,7 +693,7 @@ 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.min_access_size = 2,
>      .impl.max_access_size = 4,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>