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...
> 20210703141947.352295-1-f4bug@amsat.org/T/#m8ef6d91fd8e38b01e375083058902342970b8833">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.