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

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

[Jiaxun Yang]

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
-- 
2.37.1 (Apple Git-137.1)

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

[Philippe Mathieu-Daudé]

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>

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

[Nathan Chancellor]

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

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

[Philippe Mathieu-Daudé]

+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

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

[Philippe Mathieu-Daudé]

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

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

[Nathan Chancellor]

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

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

[Philippe Mathieu-Daudé]

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.

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

[Thomas Huth]

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

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

[Philippe Mathieu-Daudé]

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 ¯\_(ツ)_/¯

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

[Rob Landley]

[-- 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");
 
     /*

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

[Rob Landley]

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

[Rob Landley]

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

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

[Philippe Mathieu-Daudé]

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.

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

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

[Rob Landley]

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

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

[Thomas Huth]

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

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

[Philippe Mathieu-Daudé]

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!