Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 29/3/23 18:48, Rob Landley wrote:
> 
> 
> On 3/29/23 11:07, Philippe Mathieu-Daudé wrote:
>> On 29/3/23 18:09, Rob Landley wrote:
>>> On 3/28/23 12:02, Philippe Mathieu-Daudé wrote:
>>>> On 20/3/23 17:58, Nathan Chancellor wrote:
>>>>> On Wed, Mar 08, 2023 at 12:33:38AM +0100, Philippe Mathieu-Daudé wrote:
>>>>>> On 23/2/23 17:19, Jiaxun Yang wrote:
>>>>>>> 145e2198d749 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE
>>>>>>> MemoryRegionOps") converted CFGADDR/CFGDATA registers to use PCI_HOST_BRIDGE's
>>>>>>> accessor facility and enabled byte swap for both CFGADDR/CFGDATA register.
>>>>>>>
>>>>>>> However CFGADDR as a ISD internal register is not controled by MByteSwap
>>>>>>> bit, it follows endian of all other ISD register, which means it ties to
>>>>>>> little endian.
>>>>>>>
>>>>>>> Move mapping of CFGADDR out of gt64120_update_pci_cfgdata_mapping to disable
>>>>>>> endian-swapping.
>>>>>>>
>>>>>>> This should fix some recent reports about poweroff hang.
>>>>>>>
>>>>>>> Fixes: 145e2198d749 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps")
>>>>>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>>>> ---
>>>>>>>      hw/pci-host/gt64120.c | 18 ++++++------------
>>>>>>>      1 file changed, 6 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> So this works on little-endian hosts, but fails on
>>>>>> big-endian ones :(
>>>>>>
>>>>>> I.e. on Linux we have early_console_write() -> prom_putchar()
>>>>>> looping:
>>>>>>
>>>>>> IN: prom_putchar
>>>>>> 0x8010fab8:  lbu	v0,0(v1)
>>>>>> 0x8010fabc:  andi	v0,v0,0x20
>>>>>> 0x8010fac0:  beqz	v0,0x8010fab8
>>>>>> 0x8010fac4:  andi	v0,a0,0xff
>>>>>>
>>>>>> gt64120: Illegal register read reg:0x3fc size:4 value:0x00000000
>>>>>> gt64120: Illegal register read reg:0x3fc size:4 value:0x00000000
>>>>>> gt64120: Illegal register read reg:0x3fc size:4 value:0x00000000
>>>>>> gt64120: Illegal register read reg:0x3fc size:4 value:0x00000000
>>>>>> gt64120: Illegal register read reg:0x3fc size:4 value:0x00000000
>>>>>> ...
>>>>>>
>>>>>
>>>>> Is there going to be a new version of this patch or a different solution
>>>>> to the poweroff hang then? I am still seeing that with tip of tree QEMU
>>>>> and I see 8.0.0-rc0 has been tagged; I would hate for this to end up in
>>>>> a release version.
>>>>
>>>> I couldn't work a fix, however I ran our (new) tests on merge
>>>> commit 3db29dcac2 which is before the offending commit 145e2198d749,
>>>> and they fail. So I suppose Malta on big-endian host is badly broken
>>>> since quite some time. Thus clearly nobody tests/runs Malta there.
>>>
>>> I test/run malta with the mips and mipsel binaries at
>>> https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/ but I'm still
>>> locally applying the first patch I saw to fix this (attached) until upstream
>>> sorts itself out.
>>>
>>> Works fine for me. Somebody said it was the wrong fix but I don't remember why...
>>
>> This is a correct /partial/ fix. With this patch, Malta works on little
>> endian hosts. No luck with big-endian hosts, but this was broken
>> previous to 3db29dcac2 rework, so apparently not a big deal ¯\_(ツ)_/¯
> 
> No, big endian worked for me with that patch?
> 
> The build in my $PATH is QEMU emulator version 7.2.50
> (v7.2.0-873-g65cc5ccf06-dirty) with that patch, and if you wget
> https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/mips.tgz and
> ./run-emulator.sh in there, the virtual net can wget http://site (the sample

Oh, we are having some QEMU semantic confusion here...

You are testing a QEMU big-endian *guest* (or "target") in this example.

I presume you are testing on a little-endian *host* (x86_64, aarch64,
ppc64el or mips64el).

> image hasn't got https:// support enabled because I didn't include the build
> dependency), and the virtual disk works (if you do "./run-emulator.sh -hda
> blah.img" anyway, the test wrapper I mentioned creates a squashfs image for it
> to mount)). Without the patch I don't even get a PCI bus. Running "file
> /bin/toybox" says MSB, and the mipsel image is the little endian one anyway. I

Here you describe the little-endian MIPS *target* image.

> also test s390x (which is big endian 64 bit), but I don't think this needed a
> patch? (Hadn't been broken last I checked?)

Here you describe big-endian s390x *target* image.

> 
> I vaguely recall having tested newer qemu, but couldn't say when that was (early
> february at the latest, and if so I didn't install it into /usr/bin/local. It
> takes a while to build all the targets so I only really do it quarterly, usually
> when I'm about to cut a toybox release and want to make sure qemu hasn't broken
> anything important while I wasn't looking...)

Currently, QEMU MIPS (32 and 64-bit) big-endian *targets* regressed
(regardless on the host architecture).

This patch fixes QEMU MIPS (32 and 64-bit) big-endian *targets* on
little-endian *hosts* (x86_64, aarch64, ppc64el or mips64el).

However, QEMU MIPS (32 and 64-bit) big-endian *targets* are still
broken on big-endian *hosts* (s390x, ppc64, mips64, sparc64, ...).
But this was broken previous to commit 3db29dcac2.

I expect if you run your test with QEMU v7.2.0-873-g65cc5ccf06-dirty
on any big-endian *host* (like a s390x), the test fails.

It that clear? Sorry for the confusion...

Phil.