[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR
From: |
Rob Landley |
Subject: |
Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR |
Date: |
Wed, 29 Mar 2023 11:33:10 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 |
On 3/29/23 03:55, Thomas Huth wrote:
> 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?
I have my own automated test infrastructure for the toybox project, which does a
basic automated smoketest against all the support qemu images.
https://github.com/landley/toybox/blob/master/scripts/test_mkroot.sh
I also have a 300 line bash script that builds and packages all the Linux test
systems from source (it's mkroot.sh in the same directory if you're wondering
how to build a bootable Linux system for a dozen targets in 300 lines of bash,
and it's documented at https://landley.net/toybox/faq.html#mkroot and that links
to prebuilt binaries, and the instructions and scripts to build the cross
compilers I use, and prebuilt binaries for those too...
Anyway, tl;dr I both care and can regression test this easily, but haven't seen
an agreed on "try this patch instead of the other patch" go by? (Might have
missed it?)
Rob
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Philippe Mathieu-Daudé, 2023/03/07
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Nathan Chancellor, 2023/03/20
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Philippe Mathieu-Daudé, 2023/03/28
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Rob Landley, 2023/03/29
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Philippe Mathieu-Daudé, 2023/03/29
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Rob Landley, 2023/03/29
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Philippe Mathieu-Daudé, 2023/03/29
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Rob Landley, 2023/03/30
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Thomas Huth, 2023/03/30
- Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR, Thomas Huth, 2023/03/30