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

[Thomas Huth]

On 29/03/2023 18.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 ¯\_(ツ)_/¯

I've bisected now on a big endian s390x machine, and it looks like it has 
been broken here:

0c8427baf0f66bdaecae41891304f6e15242e682 is the first bad commit
commit 0c8427baf0f66bdaecae41891304f6e15242e682
Author: Jiaxun Yang <jiaxun.yang@flygoat.com>
Date:   Wed Oct 26 21:18:21 2022 +0200

    hw/mips/malta: Use bootloader helper to set BAR registers

... we should maybe really run a selected list of avocado jobs on a big 
endian host (either .travis-ci.yml or the gitlab-CI custom runner?) to avoid 
such regressions?

 Thomas