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

[Thomas Huth <thuth@redhat.com>]

On 28/03/2023 19.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.
> 
> Is it worth fixing old bugs nobody hit / reported?
> Should we stop wasting CI resources testing MIPS on big-endian hosts?

This rather sounds like a blind spot in our CI ... we still have some big 
endian s390x machines there, so maybe this just needs a proper test to avoid 
regressions? Would it be feasible to add a test to 
tests/qtest/boot-serial-test.c for this, for example?

  Thomas