| Message ID | 20230223161958.48696-1-jiaxun.yang@flygoat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR | expand |
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(-) > > diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c > index f226d0342039..82c15edb4698 100644 > --- a/hw/pci-host/gt64120.c > +++ b/hw/pci-host/gt64120.c > @@ -321,9 +321,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 > }; > @@ -339,15 +336,6 @@ 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); > @@ -1208,6 +1196,12 @@ static void gt64120_realize(DeviceState *dev, Error **errp) > PCI_DEVFN(18, 0), TYPE_PCI_BUS); > > pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci"); > + 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); Cool! This is what I had in mind but my brain couldn't context-switch to open the GT64120 datasheet again. Thank you very much for the fix! Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Thu, Feb 23, 2023 at 04:19:58PM +0000, 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> Thanks for the fix! Tested-by: Nathan Chancellor <nathan@kernel.org> > --- > hw/pci-host/gt64120.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c > index f226d0342039..82c15edb4698 100644 > --- a/hw/pci-host/gt64120.c > +++ b/hw/pci-host/gt64120.c > @@ -321,9 +321,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 > }; > @@ -339,15 +336,6 @@ 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); > @@ -1208,6 +1196,12 @@ static void gt64120_realize(DeviceState *dev, Error **errp) > PCI_DEVFN(18, 0), TYPE_PCI_BUS); > > pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci"); > + 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); > + > > /* > * The whole address space decoded by the GT-64120A doesn't generate > -- > 2.37.1 (Apple Git-137.1) > >
+Rob 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(-) > > diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c > index f226d0342039..82c15edb4698 100644 > --- a/hw/pci-host/gt64120.c > +++ b/hw/pci-host/gt64120.c > @@ -321,9 +321,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 > }; > @@ -339,15 +336,6 @@ 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); > @@ -1208,6 +1196,12 @@ static void gt64120_realize(DeviceState *dev, Error **errp) > PCI_DEVFN(18, 0), TYPE_PCI_BUS); > > pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci"); > + 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); > + > > /* > * The whole address space decoded by the GT-64120A doesn't generate
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 ...
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. Cheers, Nathan
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? Meanwhile, queuing this again.
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
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 ¯\_(ツ)_/¯
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
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
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 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 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?) 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...) Rob
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.
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
On 3/29/23 12:23, Philippe Mathieu-Daudé wrote: > On 29/3/23 18:48, Rob Landley wrote: >>>> 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). Ah, yes. I have not tried running qemu on a big endian host system in forever, but there are some IBM people with great interest in supporting every possible thing on s390x. Elizabeth Joseph would be one and would know a bunch more: https://floss.social/@pleia2/110095815201601529 >> 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. Which was broken without that patch, yes. So that's why the fix was "partial"... >> 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 don't have s390x hardware to run it on. I do have an sh2eb board but it's nommu and only has 128 megs of ram, so running qemu on it would be... unlikely. > 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. I don't have powerpc mac hardware, which seems the easiest way to get such a test system. (Well, ok, the EASY way would be to feed qemu-system-s390x a couple gigs of ram and then build and run qemu within qemu. While I do have a native toolchain for s390x, qemu's grown an insane dependency stack these days that would be a pain to bootstrap under a musl beyond-linux-from-scratch environment...) Rob
On 30/03/2023 15.12, Rob Landley wrote: ... >> 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. > > I don't have powerpc mac hardware, which seems the easiest way to get such a > test system. > > (Well, ok, the EASY way would be to feed qemu-system-s390x a couple gigs of ram > and then build and run qemu within qemu. While I do have a native toolchain for > s390x, qemu's grown an insane dependency stack these days that would be a pain > to bootstrap under a musl beyond-linux-from-scratch environment...) If you've got a github account, you can still run QEMU tests on a real s390x machine via Travis, see e.g. my recent run here: https://app.travis-ci.com/github/huth/qemu/builds/261557106 Thomas
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(-) Patch finally queued for 8.0-rc3, thanks!
diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c index f226d0342039..82c15edb4698 100644 --- a/hw/pci-host/gt64120.c +++ b/hw/pci-host/gt64120.c @@ -321,9 +321,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 }; @@ -339,15 +336,6 @@ 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); @@ -1208,6 +1196,12 @@ static void gt64120_realize(DeviceState *dev, Error **errp) PCI_DEVFN(18, 0), TYPE_PCI_BUS); pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci"); + 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); + /* * The whole address space decoded by the GT-64120A doesn't generate
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(-)