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-ayl...@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-f4...@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.