[PATCH 0/6] hw/mips/gt64xxx_pci: Fix endianness swap on big-endian hosts

[PATCH 0/6] hw/mips/gt64xxx_pci: Fix endianness swap on big-endian hosts

[Philippe Mathieu-Daudé]

While working on endianness consolidation I figured
a long-standing bug in the GT64120 while accessing
PCI config/data registers from the CPU bus (via the
ISD).
While the debugging was painful, the fix is quite
easy: simply use the endianness MemoryRegionOps
provided by the abstract PCI_HOST_BRIDGE class.

Patches 1-3 were useful while debugging.
patch 5 is the fix and patch 6 add a test to keep
testing on BE hosts.

Philippe Mathieu-Daudé (6):
  hw/pci/pci_host: Trace config accesses on unexisting functions
  hw/mips/malta: Split FPGA LEDs/ASCII display updates
  hw/mips/malta: Trace FPGA LEDs/ASCII display updates
  hw/mips/gt64xxx_pci: Accumulate address space changes
  hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps
  tests/avocado: Add tests booting YAMON ROM on MIPS Malta machines

 hw/mips/gt64xxx_pci.c               | 78 +++++++++++++++++++++--------
 hw/mips/malta.c                     | 16 ++++--
 hw/mips/trace-events                |  4 ++
 hw/pci/pci_host.c                   |  6 +++
 tests/avocado/machine_mips_malta.py | 52 +++++++++++++++++--
 5 files changed, 127 insertions(+), 29 deletions(-)

-- 
2.38.1

[PATCH 1/6] hw/pci/pci_host: Trace config accesses on unexisting functions

[Philippe Mathieu-Daudé]

Currently we only emit trace events for existing PCI functions.
In order to ease debugging PCI enumeration process, also emit
for unexisting functions:

  $ qemu-system-foo -trace pci_cfg_\*
  ...
  pci_cfg_read empty 00:0a.4 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:0a.5 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:0a.6 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:0a.7 @0x0 -> 0xffffffff
  pci_cfg_read pcnet 00:0b.0 @0x0 -> 0x20001022
  pci_cfg_read empty 00:0c.0 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:0d.0 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:0e.0 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:0f.0 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:10.0 @0x0 -> 0xffffffff
  pci_cfg_read empty 00:11.0 @0x0 -> 0xffffffff
  pci_cfg_read cirrus-vga 00:12.0 @0x0 -> 0xb81013

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/pci_host.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index eaf217ff55..ead1d3e61c 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -118,6 +118,9 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, unsigned len)
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
     if (!pci_dev) {
+        trace_pci_cfg_write("empty", extract32(addr, 16, 8),
+                            extract32(addr, 11, 5), extract32(addr, 8, 3),
+                            config_addr, val);
         return;
     }
 
@@ -131,6 +134,9 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, unsigned len)
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
     if (!pci_dev) {
+        trace_pci_cfg_read("empty", extract32(addr, 16, 8),
+                           extract32(addr, 11, 5), extract32(addr, 8, 3),
+                           config_addr, ~0x0);
         return ~0x0;
     }
 
-- 
2.38.1

[PATCH 2/6] hw/mips/malta: Split FPGA LEDs/ASCII display updates

[Philippe Mathieu-Daudé]

No need to refresh the ASCII bar when a LED is toggled
(and vice versa).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/malta.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index c0a2e0ab04..e9424150aa 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -106,11 +106,10 @@ static struct _loaderparams {
 } loaderparams;
 
 /* Malta FPGA */
-static void malta_fpga_update_display(void *opaque)
+static void malta_fpga_update_display_leds(MaltaFPGAState *s)
 {
     char leds_text[9];
     int i;
-    MaltaFPGAState *s = opaque;
 
     for (i = 7 ; i >= 0 ; i--) {
         if (s->leds & (1 << i)) {
@@ -123,6 +122,10 @@ static void malta_fpga_update_display(void *opaque)
 
     qemu_chr_fe_printf(&s->display, "\e[H\n\n|\e[32m%-8.8s\e[00m|\r\n",
                        leds_text);
+}
+
+static void malta_fpga_update_display_ascii(MaltaFPGAState *s)
+{
     qemu_chr_fe_printf(&s->display, "\n\n\n\n|\e[31m%-8.8s\e[00m|",
                        s->display_text);
 }
@@ -457,13 +460,13 @@ static void malta_fpga_write(void *opaque, hwaddr addr,
     /* LEDBAR Register */
     case 0x00408:
         s->leds = val & 0xff;
-        malta_fpga_update_display(s);
+        malta_fpga_update_display_leds(s);
         break;
 
     /* ASCIIWORD Register */
     case 0x00410:
         snprintf(s->display_text, 9, "%08X", (uint32_t)val);
-        malta_fpga_update_display(s);
+        malta_fpga_update_display_ascii(s);
         break;
 
     /* ASCIIPOS0 to ASCIIPOS7 Registers */
@@ -476,7 +479,7 @@ static void malta_fpga_write(void *opaque, hwaddr addr,
     case 0x00448:
     case 0x00450:
         s->display_text[(saddr - 0x00418) >> 3] = (char) val;
-        malta_fpga_update_display(s);
+        malta_fpga_update_display_ascii(s);
         break;
 
     /* SOFTRES Register */
-- 
2.38.1

[PATCH 3/6] hw/mips/malta: Trace FPGA LEDs/ASCII display updates

[Philippe Mathieu-Daudé]

The FPGA LEDs/ASCII display is mostly used by the bootloader
to show very low-level debug info. QEMU connects its output
to a character device backend, which is not very practical
to correlate with ASM instruction executed, interrupts or
MMIO accesses. Also, the display discard the previous states.

To ease bootloader debugging experience, add a pair of trace
events. Such events can be analyzed over time or diff-ed
between different runs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/malta.c      | 3 +++
 hw/mips/trace-events | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index e9424150aa..44d88a24a7 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -58,6 +58,7 @@
 #include "semihosting/semihost.h"
 #include "hw/mips/cps.h"
 #include "hw/qdev-clock.h"
+#include "trace.h"
 
 #define ENVP_PADDR          0x2000
 #define ENVP_VADDR          cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR)
@@ -120,12 +121,14 @@ static void malta_fpga_update_display_leds(MaltaFPGAState *s)
     }
     leds_text[8] = '\0';
 
+    trace_malta_fpga_leds(leds_text);
     qemu_chr_fe_printf(&s->display, "\e[H\n\n|\e[32m%-8.8s\e[00m|\r\n",
                        leds_text);
 }
 
 static void malta_fpga_update_display_ascii(MaltaFPGAState *s)
 {
+    trace_malta_fpga_display(s->display_text);
     qemu_chr_fe_printf(&s->display, "\n\n\n\n|\e[31m%-8.8s\e[00m|",
                        s->display_text);
 }
diff --git a/hw/mips/trace-events b/hw/mips/trace-events
index 13ee731a48..b5b882c6c2 100644
--- a/hw/mips/trace-events
+++ b/hw/mips/trace-events
@@ -4,3 +4,7 @@ gt64120_write(uint64_t addr, uint64_t value) "gt64120 write 0x%03"PRIx64" value:
 gt64120_read_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 read %s size:%u value:0x%08" PRIx64
 gt64120_write_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 write %s size:%u value:0x%08" PRIx64
 gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " -> 0x%08" PRIx64 "@0x%08" PRIx64
+
+# malta.c
+malta_fpga_leds(const char *text) "LEDs %s"
+malta_fpga_display(const char *text) "ASCII '%s'"
-- 
2.38.1

[PATCH 4/6] hw/mips/gt64xxx_pci: Accumulate address space changes

[Philippe Mathieu-Daudé]

Single registers access in ISD can produce multiple changes
in the address spaces. To reduce computational effort,
accumulate these as a single memory transaction.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/gt64xxx_pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 19d0d9889f..d84c2b7349 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -282,6 +282,8 @@ static void gt64120_isd_mapping(GT64120State *s)
     hwaddr start = ((hwaddr)s->regs[GT_ISD] << 21) & 0xFFFE00000ull;
     hwaddr length = 0x1000;
 
+    memory_region_transaction_begin();
+
     if (s->ISD_length) {
         memory_region_del_subregion(get_system_memory(), &s->ISD_mem);
     }
@@ -292,10 +294,14 @@ static void gt64120_isd_mapping(GT64120State *s)
     s->ISD_start = start;
     s->ISD_length = length;
     memory_region_add_subregion(get_system_memory(), s->ISD_start, &s->ISD_mem);
+
+    memory_region_transaction_commit();
 }
 
 static void gt64120_pci_mapping(GT64120State *s)
 {
+    memory_region_transaction_begin();
+
     /* Update PCI0IO mapping */
     if ((s->regs[GT_PCI0IOLD] & 0x7f) <= s->regs[GT_PCI0IOHD]) {
         /* Unmap old IO address */
@@ -354,6 +360,8 @@ static void gt64120_pci_mapping(GT64120State *s)
                                         &s->PCI0M1_mem);
         }
     }
+
+    memory_region_transaction_commit();
 }
 
 static int gt64120_post_load(void *opaque, int version_id)
-- 
2.38.1

[PATCH 5/6] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps

[Philippe Mathieu-Daudé]

GT64120's PCI endianness swapping works on little-endian hosts,
but doesn't on big-endian ones. Instead of complicating how
CFGADDR/CFGDATA registers deal with endianness, use the existing
MemoryRegionOps from hw/pci/pci_host.c. Doing so also reduce the
access to internal PCI_HOST_BRIDGE fields.

Map the PCI_HOST_BRIDGE MemoryRegionOps into the corresponding
CFGADDR/CFGDATA regions in the ISD MMIO and remove the unused
code in the current ISD read/write handlers.

Update the mapping when PCI0_CMD register is accessed (in case
the endianness is changed).

This allows using the GT64120 on a big-endian host (and boot
the MIPS Malta machine in little-endian).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/gt64xxx_pci.c | 70 ++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 20 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index d84c2b7349..b92536d3ab 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -298,6 +298,50 @@ static void gt64120_isd_mapping(GT64120State *s)
     memory_region_transaction_commit();
 }
 
+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();
+
+    /*
+     * The setting of the MByteSwap bit and MWordSwap bit in the PCI Internal
+     * Command Register determines how data transactions from the CPU to/from
+     * PCI are handled along with the setting of the Endianess bit in the CPU
+     * Configuration Register. See:
+     * - 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, "pci-conf-data", 4);
+    memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGDATA << 2,
+                                        &phb->data_mem, 1);
+
+    memory_region_transaction_commit();
+}
+
 static void gt64120_pci_mapping(GT64120State *s)
 {
     memory_region_transaction_begin();
@@ -389,7 +433,6 @@ static void gt64120_writel(void *opaque, hwaddr addr,
                            uint64_t val, unsigned size)
 {
     GT64120State *s = opaque;
-    PCIHostState *phb = PCI_HOST_BRIDGE(s);
     uint32_t saddr = addr >> 2;
 
     trace_gt64120_write(addr, val);
@@ -592,6 +635,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
     case GT_PCI0_CMD:
     case GT_PCI1_CMD:
         s->regs[saddr] = val & 0x0401fc0f;
+        gt64120_update_pci_cfgdata_mapping(s);
         break;
     case GT_PCI0_TOR:
     case GT_PCI0_BS_SCS10:
@@ -632,15 +676,9 @@ static void gt64120_writel(void *opaque, hwaddr addr,
                       saddr << 2, size, size << 1, val);
         break;
     case GT_PCI0_CFGADDR:
-        phb->config_reg = val & 0x80fffffc;
-        break;
     case GT_PCI0_CFGDATA:
-        if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
-            val = bswap32(val);
-        }
-        if (phb->config_reg & (1u << 31)) {
-            pci_data_write(phb->bus, phb->config_reg, val, 4);
-        }
+        /* Mapped via in gt64120_pci_mapping() */
+        g_assert_not_reached();
         break;
 
     /* Interrupts */
@@ -698,7 +736,6 @@ static uint64_t gt64120_readl(void *opaque,
                               hwaddr addr, unsigned size)
 {
     GT64120State *s = opaque;
-    PCIHostState *phb = PCI_HOST_BRIDGE(s);
     uint32_t val;
     uint32_t saddr = addr >> 2;
 
@@ -883,17 +920,9 @@ static uint64_t gt64120_readl(void *opaque,
 
     /* PCI Internal */
     case GT_PCI0_CFGADDR:
-        val = phb->config_reg;
-        break;
     case GT_PCI0_CFGDATA:
-        if (!(phb->config_reg & (1 << 31))) {
-            val = 0xffffffff;
-        } else {
-            val = pci_data_read(phb->bus, phb->config_reg, 4);
-        }
-        if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
-            val = bswap32(val);
-        }
+        /* Mapped via in gt64120_pci_mapping() */
+        g_assert_not_reached();
         break;
 
     case GT_PCI0_CMD:
@@ -1153,6 +1182,7 @@ static void gt64120_reset(DeviceState *dev)
 
     gt64120_isd_mapping(s);
     gt64120_pci_mapping(s);
+    gt64120_update_pci_cfgdata_mapping(s);
 }
 
 static void gt64120_realize(DeviceState *dev, Error **errp)
-- 
2.38.1

[PATCH 6/6] tests/avocado: Add tests booting YAMON ROM on MIPS Malta machines

[Philippe Mathieu-Daudé]

Add quick tests booting YAMON:

  $ avocado --show=app,console run -t machine:malta tests/avocado/machine_mips_malta.py
   (1/2) tests/avocado/machine_mips_malta.py:MaltaMachine.test_mipsel_malta_yamon:
  console: YAMON ROM Monitor, Revision 02.22.
  console: Copyright (c) 1999-2007 MIPS Technologies, Inc. - All Rights Reserved.
  console: For a list of available commands, type 'help'.
  console: Compilation time =              May 24 2013  12:16:34 (pburton)
  console: Board type/revision =           0x02 (Malta) / 0x00
  console: Core board type/revision =      0x01 (CoreLV) / 0x00
  console: System controller/revision =    Galileo / GT_64120A-B-0
  console: FPGA revision =                 0x0000
  console: MAC address =                   ff.ff.ff.ff.ff.ff
  console: Board S/N =                     0123456789
  console: PCI bus frequency =             33.33 MHz
  console: Processor Company ID/options =  0x01 (MIPS Technologies, Inc.) / 0x00
  console: Processor ID/revision =         0x93 (MIPS 24Kf) / 0x00
  console: Endianness =                    Little
  console: CPU/Bus frequency =             333 MHz / 419 MHz
  console: Coherency =                     None
  console: Flash memory size =             4 MByte
  console: SDRAM size =                    128 MByte
  console: First free SDRAM address =      0x800c32f0
  console: WARNING: Environment variable flash area is invalid!
  console: HINT   : Perform "erase -e"
  console: YAMON>
  PASS (1.88 s)
   (2/2) tests/avocado/machine_mips_malta.py:MaltaMachine.test_mips64el_malta_yamon:
  ...
  console: System controller/revision =    Galileo / GT_64120A-B-0
  console: Processor Company ID/options =  0x01 (MIPS Technologies, Inc.) / 0x00
  console: Processor ID/revision =         0x82 (MIPS 20Kc) / 0xa0
  ...
  console: YAMON>
  PASS (1.89 s)
  RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
  JOB TIME   : 4.57 s

YAMON does some endian-swapped acceses on the ISD<->PCI CFG/DATA
registers. These tests are useful to debug cross-endianness issues,
in particular on big-endian host.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/avocado/machine_mips_malta.py | 52 ++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/tests/avocado/machine_mips_malta.py b/tests/avocado/machine_mips_malta.py
index f1895d59f3..a3b0b55305 100644
--- a/tests/avocado/machine_mips_malta.py
+++ b/tests/avocado/machine_mips_malta.py
@@ -11,11 +11,13 @@
 import gzip
 import logging
 
-from avocado import skipUnless
-from avocado_qemu import QemuSystemTest
-from avocado_qemu import wait_for_console_pattern
-from avocado.utils import archive
 from avocado import skipIf
+from avocado import skipUnless
+from avocado.utils import archive
+from avocado_qemu import QemuSystemTest
+from avocado_qemu import exec_command_and_wait_for_pattern
+from avocado_qemu import interrupt_interactive_console_until_pattern
+from avocado_qemu import wait_for_console_pattern
 
 
 NUMPY_AVAILABLE = True
@@ -118,3 +120,45 @@ def test_mips_malta_i6400_framebuffer_logo_8cores(self):
         :avocado: tags=mips:smp
         """
         self.do_test_i6400_framebuffer_logo(8)
+
+class MaltaMachine(QemuSystemTest):
+
+    def do_test_yamon(self):
+        """
+        :avocado: tags=arch:mipsel
+        :avocado: tags=arch:mips64el
+        :avocado: tags=machine:malta
+        """
+        rom_url = ('http://www.imgtec.com/tools/mips-tools/downloads/'
+                   'yamon/yamon-bin-02.22.zip')
+        rom_hash = '8da7ecddbc5312704b8b324341ee238189bde480'
+        zip_path = self.fetch_asset(rom_url, asset_hash=rom_hash)
+
+        archive.extract(zip_path, self.workdir)
+        yamon_path = os.path.join(self.workdir, 'yamon-02.22.bin')
+
+        self.vm.set_console()
+        self.vm.add_args('-bios', yamon_path)
+        self.vm.launch()
+
+        prompt =  'YAMON>'
+        pattern = 'YAMON ROM Monitor'
+        interrupt_interactive_console_until_pattern(self, pattern, prompt)
+        wait_for_console_pattern(self, prompt)
+        self.vm.shutdown()
+
+    def test_mipsel_malta_yamon(self):
+        """
+        :avocado: tags=arch:mipsel
+        :avocado: tags=machine:malta
+        :avocado: tags=endian:little
+        """
+        self.do_test_yamon()
+
+    def test_mips64el_malta_yamon(self):
+        """
+        :avocado: tags=arch:mips64el
+        :avocado: tags=machine:malta
+        :avocado: tags=endian:little
+        """
+        self.do_test_yamon()
-- 
2.38.1

Re: [PATCH 6/6] tests/avocado: Add tests booting YAMON ROM on MIPS Malta machines

[Philippe Mathieu-Daudé]

On 4/1/23 14:39, Philippe Mathieu-Daudé wrote:
> Add quick tests booting YAMON:
> 
>    $ avocado --show=app,console run -t machine:malta tests/avocado/machine_mips_malta.py
>     (1/2) tests/avocado/machine_mips_malta.py:MaltaMachine.test_mipsel_malta_yamon:
>    console: YAMON ROM Monitor, Revision 02.22.
>    console: Copyright (c) 1999-2007 MIPS Technologies, Inc. - All Rights Reserved.
>    console: For a list of available commands, type 'help'.
>    console: Compilation time =              May 24 2013  12:16:34 (pburton)
>    console: Board type/revision =           0x02 (Malta) / 0x00
>    console: Core board type/revision =      0x01 (CoreLV) / 0x00
>    console: System controller/revision =    Galileo / GT_64120A-B-0
>    console: FPGA revision =                 0x0000
>    console: MAC address =                   ff.ff.ff.ff.ff.ff
>    console: Board S/N =                     0123456789
>    console: PCI bus frequency =             33.33 MHz
>    console: Processor Company ID/options =  0x01 (MIPS Technologies, Inc.) / 0x00
>    console: Processor ID/revision =         0x93 (MIPS 24Kf) / 0x00
>    console: Endianness =                    Little
>    console: CPU/Bus frequency =             333 MHz / 419 MHz
>    console: Coherency =                     None
>    console: Flash memory size =             4 MByte
>    console: SDRAM size =                    128 MByte
>    console: First free SDRAM address =      0x800c32f0
>    console: WARNING: Environment variable flash area is invalid!
>    console: HINT   : Perform "erase -e"
>    console: YAMON>
>    PASS (1.88 s)
>     (2/2) tests/avocado/machine_mips_malta.py:MaltaMachine.test_mips64el_malta_yamon:
>    ...
>    console: System controller/revision =    Galileo / GT_64120A-B-0
>    console: Processor Company ID/options =  0x01 (MIPS Technologies, Inc.) / 0x00
>    console: Processor ID/revision =         0x82 (MIPS 20Kc) / 0xa0
>    ...
>    console: YAMON>
>    PASS (1.89 s)
>    RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
>    JOB TIME   : 4.57 s
> 
> YAMON does some endian-swapped acceses on the ISD<->PCI CFG/DATA
> registers. These tests are useful to debug cross-endianness issues,
> in particular on big-endian host.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/avocado/machine_mips_malta.py | 52 ++++++++++++++++++++++++++---
>   1 file changed, 48 insertions(+), 4 deletions(-)

> +class MaltaMachine(QemuSystemTest):
> +
> +    def do_test_yamon(self):

Not important but this block ... -->

> +        """
> +        :avocado: tags=arch:mipsel
> +        :avocado: tags=arch:mips64el
> +        :avocado: tags=machine:malta
> +        """

  <-- ... shouldn't be here.

> +        rom_url = ('http://www.imgtec.com/tools/mips-tools/downloads/'
> +                   'yamon/yamon-bin-02.22.zip')
> +        rom_hash = '8da7ecddbc5312704b8b324341ee238189bde480'
> +        zip_path = self.fetch_asset(rom_url, asset_hash=rom_hash)
> +
> +        archive.extract(zip_path, self.workdir)
> +        yamon_path = os.path.join(self.workdir, 'yamon-02.22.bin')
> +
> +        self.vm.set_console()
> +        self.vm.add_args('-bios', yamon_path)
> +        self.vm.launch()
> +
> +        prompt =  'YAMON>'
> +        pattern = 'YAMON ROM Monitor'
> +        interrupt_interactive_console_until_pattern(self, pattern, prompt)
> +        wait_for_console_pattern(self, prompt)
> +        self.vm.shutdown()
> +
> +    def test_mipsel_malta_yamon(self):
> +        """
> +        :avocado: tags=arch:mipsel
> +        :avocado: tags=machine:malta
> +        :avocado: tags=endian:little
> +        """
> +        self.do_test_yamon()
> +
> +    def test_mips64el_malta_yamon(self):
> +        """
> +        :avocado: tags=arch:mips64el
> +        :avocado: tags=machine:malta
> +        :avocado: tags=endian:little
> +        """
> +        self.do_test_yamon()

Re: [PATCH 0/6] hw/mips/gt64xxx_pci: Fix endianness swap on big-endian hosts

[Richard Henderson]

On 1/4/23 05:39, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (6):
>    hw/pci/pci_host: Trace config accesses on unexisting functions
>    hw/mips/malta: Split FPGA LEDs/ASCII display updates
>    hw/mips/malta: Trace FPGA LEDs/ASCII display updates
>    hw/mips/gt64xxx_pci: Accumulate address space changes
>    hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps
>    tests/avocado: Add tests booting YAMON ROM on MIPS Malta machines

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/6] hw/mips/gt64xxx_pci: Fix endianness swap on big-endian hosts

[Philippe Mathieu-Daudé]

On 4/1/23 14:39, Philippe Mathieu-Daudé wrote:

> Philippe Mathieu-Daudé (6):
>    hw/pci/pci_host: Trace config accesses on unexisting functions
>    hw/mips/malta: Split FPGA LEDs/ASCII display updates
>    hw/mips/malta: Trace FPGA LEDs/ASCII display updates
>    hw/mips/gt64xxx_pci: Accumulate address space changes
>    hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps
>    tests/avocado: Add tests booting YAMON ROM on MIPS Malta machines

Applied to mips-next.

Re: [PATCH 5/6] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps

[Nathan Chancellor]

Hi Philippe,

On Wed, Jan 04, 2023 at 02:39:34PM +0100, Philippe Mathieu-Daudé wrote:
> GT64120's PCI endianness swapping works on little-endian hosts,
> but doesn't on big-endian ones. Instead of complicating how
> CFGADDR/CFGDATA registers deal with endianness, use the existing
> MemoryRegionOps from hw/pci/pci_host.c. Doing so also reduce the
> access to internal PCI_HOST_BRIDGE fields.
> 
> Map the PCI_HOST_BRIDGE MemoryRegionOps into the corresponding
> CFGADDR/CFGDATA regions in the ISD MMIO and remove the unused
> code in the current ISD read/write handlers.
> 
> Update the mapping when PCI0_CMD register is accessed (in case
> the endianness is changed).
> 
> This allows using the GT64120 on a big-endian host (and boot
> the MIPS Malta machine in little-endian).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

This change as commit 145e2198d7 ("hw/mips/gt64xxx_pci: Endian-swap
using PCI_HOST_BRIDGE MemoryRegionOps") in QEMU master causes a hang
when trying to poweroff a malta_defconfig + CONFIG_CPU_BIG_ENDIAN=y
kernel on an x86_64 host. The kernel has been built from latest mainline
using the kernel.org toolchains [1], just in case it matters.

$ timeout --foreground 30s qemu-system-mips \
-cpu 24Kf \
-machine malta \
-kernel vmlinux \
-display none \
-initrd rootfs.cpio \
-m 512m \
-nodefaults \
-no-reboot \
-serial mon:stdio
...
Run /init as init process
process '/bin/busybox' started with executable stack
Starting syslogd: OK
Starting klogd: OK
Running sysctl: OK
Saving random seed: OK
Starting network: OK
Linux version 6.2.0-rc5-00013-g2475bf0250de (tuxmake@tuxmake) (mips-linux-gcc (GCC) 12.2.0, GNU ld (GNU Binutils) 2.39) #1 SMP @1674418498
Stopping network: OK
Saving random seed: OK
Stopping klogd: OK
Stopping syslogd: OK
umount: devtmpfs busy - remounted read-only
umount: can't unmount /: Invalid argument
The system is going down NOW!
Sent SIGTERM to all processes
Sent SIGKILL to all processes
Requesting system poweroff
reboot: System halted
qemu-system-mips: terminating on signal 15 from pid 2213875 (timeout)

The rootfs is available at [2], if it is necessary. It is a simple
buildroot initramfs that just prints the version string and shutsdown
the machine

If there is any additional information that I can provide or patches I
can test, please let me know.

[1]: https://mirrors.edge.kernel.org/pub/tools/crosstool/
[2]: https://github.com/ClangBuiltLinux/boot-utils/tree/1b837f3b0fca441e0cc694c9b587120e81299554/images/mips

Cheers,
Nathan

# bad: [00b1faea41d283e931256aa78aa975a369ec3ae6] Merge tag 'pull-target-arm-20230123' of https://git.linaro.org/people/pmaydell/qemu-arm into staging
# good: [886fb67020e32ce6a2cf7049c6f017acf1f0d69a] Merge tag 'pull-target-arm-20230113' of https://git.linaro.org/people/pmaydell/qemu-arm into staging
git bisect start '00b1faea41d283e931256aa78aa975a369ec3ae6' '886fb67020e32ce6a2cf7049c6f017acf1f0d69a'
# bad: [239b8b0699a222fd21da1c5fdeba0a2456085a47] Merge tag 'trivial-branch-for-8.0-pull-request' of https://gitlab.com/laurent_vivier/qemu into staging
git bisect bad 239b8b0699a222fd21da1c5fdeba0a2456085a47
# bad: [a48f692929828212f75eb6e8d11bbb6cdffad153] hw/usb: Mark the XLNX_VERSAL-related files as target-independent
git bisect bad a48f692929828212f75eb6e8d11bbb6cdffad153
# bad: [a844873512400fae6bed9e87694dc96ff2f15f39] mips: Remove support for trap and emulate KVM
git bisect bad a844873512400fae6bed9e87694dc96ff2f15f39
# bad: [cd5066f8618bc6c80ec9088923c58f4a42ab0e7a] hw/mips/bootloader: Handle buffers as opaque arrays
git bisect bad cd5066f8618bc6c80ec9088923c58f4a42ab0e7a
# bad: [37e506b69a6791bede30677f05081296f3b77f77] hw/mips/gt64xxx_pci: Let the GT64120 manage the lower 512MiB hole
git bisect bad 37e506b69a6791bede30677f05081296f3b77f77
# good: [65423e6efeac1ee1057870361337c572c941140c] hw/mips/gt64xxx_pci: Accumulate address space changes
git bisect good 65423e6efeac1ee1057870361337c572c941140c
# bad: [7c032bfbe838c24dcbdc8f9c452553b24f20daad] hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c
git bisect bad 7c032bfbe838c24dcbdc8f9c452553b24f20daad
# bad: [145e2198d749ec09a405f1607a9932499b76f1eb] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps
git bisect bad 145e2198d749ec09a405f1607a9932499b76f1eb
# first bad commit: [145e2198d749ec09a405f1607a9932499b76f1eb] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps

Re: [PATCH 5/6] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps

[BALATON Zoltan]

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

On Mon, 23 Jan 2023, Nathan Chancellor wrote:
> Hi Philippe,
> On Wed, Jan 04, 2023 at 02:39:34PM +0100, Philippe Mathieu-Daudé wrote:
>> GT64120's PCI endianness swapping works on little-endian hosts,
>> but doesn't on big-endian ones. Instead of complicating how
>> CFGADDR/CFGDATA registers deal with endianness, use the existing
>> MemoryRegionOps from hw/pci/pci_host.c. Doing so also reduce the
>> access to internal PCI_HOST_BRIDGE fields.
>>
>> Map the PCI_HOST_BRIDGE MemoryRegionOps into the corresponding
>> CFGADDR/CFGDATA regions in the ISD MMIO and remove the unused
>> code in the current ISD read/write handlers.
>>
>> Update the mapping when PCI0_CMD register is accessed (in case
>> the endianness is changed).
>>
>> This allows using the GT64120 on a big-endian host (and boot
>> the MIPS Malta machine in little-endian).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> This change as commit 145e2198d7 ("hw/mips/gt64xxx_pci: Endian-swap
> using PCI_HOST_BRIDGE MemoryRegionOps") in QEMU master causes a hang
> when trying to poweroff a malta_defconfig + CONFIG_CPU_BIG_ENDIAN=y
> kernel on an x86_64 host. The kernel has been built from latest mainline

Could this be a similar issue like this:

https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg03025.html

For that the patch I've proposed was this:

https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg01871.html

but I was told it's in memory layer and then we just gave up after several 
pings:

https://lists.nongnu.org/archive/html/qemu-devel/2022-02/msg04775.html

The patch may still work as an interim fix though.

Regards,
BALATON Zoltan

> using the kernel.org toolchains [1], just in case it matters.
>
> $ timeout --foreground 30s qemu-system-mips \
> -cpu 24Kf \
> -machine malta \
> -kernel vmlinux \
> -display none \
> -initrd rootfs.cpio \
> -m 512m \
> -nodefaults \
> -no-reboot \
> -serial mon:stdio
> ...
> Run /init as init process
> process '/bin/busybox' started with executable stack
> Starting syslogd: OK
> Starting klogd: OK
> Running sysctl: OK
> Saving random seed: OK
> Starting network: OK
> Linux version 6.2.0-rc5-00013-g2475bf0250de (tuxmake@tuxmake) (mips-linux-gcc (GCC) 12.2.0, GNU ld (GNU Binutils) 2.39) #1 SMP @1674418498
> Stopping network: OK
> Saving random seed: OK
> Stopping klogd: OK
> Stopping syslogd: OK
> umount: devtmpfs busy - remounted read-only
> umount: can't unmount /: Invalid argument
> The system is going down NOW!
> Sent SIGTERM to all processes
> Sent SIGKILL to all processes
> Requesting system poweroff
> reboot: System halted
> qemu-system-mips: terminating on signal 15 from pid 2213875 (timeout)
>
> The rootfs is available at [2], if it is necessary. It is a simple
> buildroot initramfs that just prints the version string and shutsdown
> the machine
>
> If there is any additional information that I can provide or patches I
> can test, please let me know.
>
> [1]: https://mirrors.edge.kernel.org/pub/tools/crosstool/
> [2]: https://github.com/ClangBuiltLinux/boot-utils/tree/1b837f3b0fca441e0cc694c9b587120e81299554/images/mips
>
> Cheers,
> Nathan
>
> # bad: [00b1faea41d283e931256aa78aa975a369ec3ae6] Merge tag 'pull-target-arm-20230123' of https://git.linaro.org/people/pmaydell/qemu-arm into staging
> # good: [886fb67020e32ce6a2cf7049c6f017acf1f0d69a] Merge tag 'pull-target-arm-20230113' of https://git.linaro.org/people/pmaydell/qemu-arm into staging
> git bisect start '00b1faea41d283e931256aa78aa975a369ec3ae6' '886fb67020e32ce6a2cf7049c6f017acf1f0d69a'
> # bad: [239b8b0699a222fd21da1c5fdeba0a2456085a47] Merge tag 'trivial-branch-for-8.0-pull-request' of https://gitlab.com/laurent_vivier/qemu into staging
> git bisect bad 239b8b0699a222fd21da1c5fdeba0a2456085a47
> # bad: [a48f692929828212f75eb6e8d11bbb6cdffad153] hw/usb: Mark the XLNX_VERSAL-related files as target-independent
> git bisect bad a48f692929828212f75eb6e8d11bbb6cdffad153
> # bad: [a844873512400fae6bed9e87694dc96ff2f15f39] mips: Remove support for trap and emulate KVM
> git bisect bad a844873512400fae6bed9e87694dc96ff2f15f39
> # bad: [cd5066f8618bc6c80ec9088923c58f4a42ab0e7a] hw/mips/bootloader: Handle buffers as opaque arrays
> git bisect bad cd5066f8618bc6c80ec9088923c58f4a42ab0e7a
> # bad: [37e506b69a6791bede30677f05081296f3b77f77] hw/mips/gt64xxx_pci: Let the GT64120 manage the lower 512MiB hole
> git bisect bad 37e506b69a6791bede30677f05081296f3b77f77
> # good: [65423e6efeac1ee1057870361337c572c941140c] hw/mips/gt64xxx_pci: Accumulate address space changes
> git bisect good 65423e6efeac1ee1057870361337c572c941140c
> # bad: [7c032bfbe838c24dcbdc8f9c452553b24f20daad] hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c
> git bisect bad 7c032bfbe838c24dcbdc8f9c452553b24f20daad
> # bad: [145e2198d749ec09a405f1607a9932499b76f1eb] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps
> git bisect bad 145e2198d749ec09a405f1607a9932499b76f1eb
> # first bad commit: [145e2198d749ec09a405f1607a9932499b76f1eb] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps
>
>

Re: [PATCH 5/6] hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps

[Alex Bennée]

Nathan Chancellor <nathan@kernel.org> writes:

> Hi Philippe,
>
> On Wed, Jan 04, 2023 at 02:39:34PM +0100, Philippe Mathieu-Daudé wrote:
>> GT64120's PCI endianness swapping works on little-endian hosts,
>> but doesn't on big-endian ones. Instead of complicating how
>> CFGADDR/CFGDATA registers deal with endianness, use the existing
>> MemoryRegionOps from hw/pci/pci_host.c. Doing so also reduce the
>> access to internal PCI_HOST_BRIDGE fields.
>> 
>> Map the PCI_HOST_BRIDGE MemoryRegionOps into the corresponding
>> CFGADDR/CFGDATA regions in the ISD MMIO and remove the unused
>> code in the current ISD read/write handlers.
>> 
>> Update the mapping when PCI0_CMD register is accessed (in case
>> the endianness is changed).
>> 
>> This allows using the GT64120 on a big-endian host (and boot
>> the MIPS Malta machine in little-endian).
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> This change as commit 145e2198d7 ("hw/mips/gt64xxx_pci: Endian-swap
> using PCI_HOST_BRIDGE MemoryRegionOps") in QEMU master causes a hang
> when trying to poweroff a malta_defconfig + CONFIG_CPU_BIG_ENDIAN=y
> kernel on an x86_64 host. The kernel has been built from latest mainline
> using the kernel.org toolchains [1], just in case it matters.
>
> $ timeout --foreground 30s qemu-system-mips \
> -cpu 24Kf \
> -machine malta \
> -kernel vmlinux \
> -display none \
> -initrd rootfs.cpio \
> -m 512m \
> -nodefaults \
> -no-reboot \
> -serial mon:stdio
> ...
<snip>

Ahh also this commit got fingered by the tuxrun test for the same
combination:

  ./run --runtime docker --qemu-binary /home/alex.bennee/lsrc/qemu.git/builds/bisect/qemu-system-mips64 --device qemu-mips64

with my in flight patches for tuxrun's host support.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro