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

[Rob Landley <rob@landley.net>]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

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...

Rob

[-- Attachment #2: blah.patch --]
[-- Type: text/x-patch, Size: 2586 bytes --]

diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index f226d03420..36ed01c615 100644
--- a/hw/pci-host/gt64120.c
+++ b/hw/pci-host/gt64120.c
@@ -320,13 +320,6 @@ static void gt64120_isd_mapping(GT64120State *s)
 
 static void gt64120_update_pci_cfgdata_mapping(GT64120State *s)
 {
-    /* Indexed on MByteSwap bit, see Table 158: PCI_0 Command, Offset: 0xc00 */
-    static const MemoryRegionOps *pci_host_conf_ops[] = {
-        &pci_host_conf_be_ops, &pci_host_conf_le_ops
-    };
-    static const MemoryRegionOps *pci_host_data_ops[] = {
-        &pci_host_data_be_ops, &pci_host_data_le_ops
-    };
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
 
     memory_region_transaction_begin();
@@ -339,22 +332,13 @@ static void gt64120_update_pci_cfgdata_mapping(GT64120State *s)
      * - Table 16: 32-bit PCI Transaction Endianess
      * - Table 158: PCI_0 Command, Offset: 0xc00
      */
-    if (memory_region_is_mapped(&phb->conf_mem)) {
-        memory_region_del_subregion(&s->ISD_mem, &phb->conf_mem);
-        object_unparent(OBJECT(&phb->conf_mem));
-    }
-    memory_region_init_io(&phb->conf_mem, OBJECT(phb),
-                          pci_host_conf_ops[s->regs[GT_PCI0_CMD] & 1],
-                          s, "pci-conf-idx", 4);
-    memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGADDR << 2,
-                                        &phb->conf_mem, 1);
-
     if (memory_region_is_mapped(&phb->data_mem)) {
         memory_region_del_subregion(&s->ISD_mem, &phb->data_mem);
         object_unparent(OBJECT(&phb->data_mem));
     }
     memory_region_init_io(&phb->data_mem, OBJECT(phb),
-                          pci_host_data_ops[s->regs[GT_PCI0_CMD] & 1],
+                          (s->regs[GT_PCI0_CMD] & 1) ? &pci_host_data_le_ops
+                                                     : &pci_host_data_be_ops,
                           s, "pci-conf-data", 4);
     memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGDATA << 2,
                                         &phb->data_mem, 1);
@@ -1207,6 +1191,11 @@ static void gt64120_realize(DeviceState *dev, Error **errp)
                                 get_system_io(),
                                 PCI_DEVFN(18, 0), TYPE_PCI_BUS);
 
+    memory_region_init_io(&phb->conf_mem, OBJECT(phb), &pci_host_conf_le_ops,
+                          s, "pci-conf-idx", 4);
+    memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGADDR << 2,
+                                        &phb->conf_mem, 1);
+
     pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
 
     /*