[PATCH 0/2] hw/pci-host/gt64120: Fix regression on big-endian targets

[PATCH 0/2] hw/pci-host/gt64120: Fix regression on big-endian targets

[Philippe Mathieu-Daudé]

Klauss reported a regression on big-endian targets, introduced
by commit 145e2198d749 ("hw/mips/gt64xxx_pci: Endian-swap using
PCI_HOST_BRIDGE MemoryRegionOps"). Fix it and add the Klauss'
reproducer as Avocado test.

Philippe Mathieu-Daudé (2):
  hw/pci-host/gt64120: Fix PCI I/O config register endianness
  tests/avocado: Add test accessing NVMe on big-endian MIPS target

 hw/pci-host/gt64120.c               | 25 +++++-----------
 tests/avocado/boot_linux_console.py | 44 +++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 18 deletions(-)

-- 
2.38.1

[PATCH 1/2] hw/pci-host/gt64120: Fix PCI I/O config register endianness

[Philippe Mathieu-Daudé]

The MByteSwap bit only affects the data register endianness,
not the config register. Map the config register once in the
gt64120_realize() handler, and only remap the data register
when the mapping is updated.

Fixes: 145e2198d7 ("gt64xxx: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps")
Reported-by: Klaus Jensen <its@irrelevant.dk>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/gt64120.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index f226d03420..a88b59f576 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");
 
     /*
-- 
2.38.1

Re: [PATCH 1/2] hw/pci-host/gt64120: Fix PCI I/O config register endianness

[Klaus Jensen]

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

On Jan 18 10:40, Philippe Mathieu-Daudé wrote:
> The MByteSwap bit only affects the data register endianness,
> not the config register. Map the config register once in the
> gt64120_realize() handler, and only remap the data register
> when the mapping is updated.
> 
> Fixes: 145e2198d7 ("gt64xxx: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps")
> Reported-by: Klaus Jensen <its@irrelevant.dk>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Can confirm that this fixes the regression. Thanks Philippe!

Tested-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] tests/avocado: Add test accessing NVMe on big-endian MIPS target

[Philippe Mathieu-Daudé]

Add a Linux-boot test accessing PCI NVMe drive on big-endian MIPS:

$ avocado --show=app,console run -t device:nvme tests/avocado/
   (1/1) tests/avocado/boot_linux_console.py:BootLinuxConsole.test_mips64_malta_I6400_nvme:
  console: Linux version 6.2.0-rc4 (kbj@butter) (mips64-buildroot-linux-gnu-gcc.br_real (Buildroot 2022.11) 11.3.0, GNU ld (GNU Binutils) 2.38) #6 Tue Jan 17 18:48:25 CET 2023
  console: CPU0 revision is: 0001a900 (MIPS I6400)
  console: FPU revision is: 20f30300
  console: MIPS: machine is mti,malta
  ...
  console: PCI host bridge to bus 0000:00
  console: pci_bus 0000:00: root bus resource [mem 0x10000000-0x17ffffff]
  console: pci_bus 0000:00: root bus resource [io  0x1000-0x1fffff]
  console: pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]
  console: pci 0000:00:0a.0: [8086:7110] type 00 class 0x060100
  console: pci 0000:00:0a.1: [8086:7111] type 00 class 0x010180
  console: pci 0000:00:0a.1: reg 0x20: [io  0x0000-0x000f]
  console: pci 0000:00:0a.1: legacy IDE quirk: reg 0x10: [io  0x01f0-0x01f7]
  console: pci 0000:00:0a.1: legacy IDE quirk: reg 0x14: [io  0x03f6]
  console: pci 0000:00:0a.1: legacy IDE quirk: reg 0x18: [io  0x0170-0x0177]
  console: pci 0000:00:0a.1: legacy IDE quirk: reg 0x1c: [io  0x0376]
  console: pci 0000:00:0a.2: [8086:7112] type 00 class 0x0c0300
  console: pci 0000:00:12.0: [1b36:0010] type 00 class 0x010802
  console: pci 0000:00:12.0: reg 0x10: [mem 0x00000000-0x00003fff 64bit]
  console: pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to 00
  console: pci 0000:00:12.0: BAR 0: assigned [mem 0x10040000-0x10043fff 64bit]
  console: pci 0000:00:0a.1: BAR 4: assigned [io  0x1080-0x108f]
  ...
  console: ata_piix 0000:00:0a.1: enabling device (0000 -> 0001)
  console: nvme nvme0: pci function 0000:00:12.0
  console: nvme 0000:00:12.0: enabling device (0000 -> 0002)
  console: nvme nvme0: 1/0/0 default/read/poll queues
  console: nvme nvme0: Ignoring bogus Namespace Identifiers
  ...
  console: Run /sbin/init as init process
  console: EXT4-fs (nvme0n1): re-mounted bf659b11-5a77-4ab5-a337-3d71ced26114. Quota mode: disabled.
  ...
  console: Welcome to Buildroot
  console: buildroot login: root
  ...
  console: # reboot
  ...
  console: umount: devtmpfs busy - remounted read-only
  console: EXT4-fs (nvme0n1): re-mounted bf659b11-5a77-4ab5-a337-3d71ced26114. Quota mode: disabled.
  console: The system is going down NOW!
  console: Requesting system reboot
  console: reboot: Restarting system
  PASS (11.17 s)
  JOB TIME   : 11.91 s

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

diff --git a/tests/avocado/boot_linux_console.py b/tests/avocado/boot_linux_console.py
index 8c1d981586..176793482e 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -12,6 +12,7 @@
 import lzma
 import gzip
 import shutil
+import time
 
 from avocado import skip
 from avocado import skipUnless
@@ -269,6 +270,49 @@ def test_mips64el_malta_5KEc_cpio(self):
         # Wait for VM to shut down gracefully
         self.vm.wait()
 
+    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
+    def test_mips64_malta_I6400_nvme(self):
+        """
+        :avocado: tags=arch:mips64
+        :avocado: tags=machine:malta
+        :avocado: tags=endian:big
+        :avocado: tags=cpu:I6400
+        :avocado: tags=device:nvme
+        """
+        kernel_url = ('https://github.com/birkelund/qemu-nvme-boot/'
+                      'raw/main/mips64/images/vmlinux')
+        kernel_hash = '665662d7f7b17dc261ffb0e0ff4a1a7da91de948'
+        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+        rootfs_url = ('https://github.com/birkelund/qemu-nvme-boot/'
+                      'raw/main/mips64/images/rootfs.ext2.gz')
+        rootfs_hash = '66f5ca4ef20ab983ec424c3ed8462bab305bbb73'
+        rootfs_path_gz = self.fetch_asset(rootfs_url, asset_hash=rootfs_hash)
+        rootfs_path = os.path.join(self.workdir, "rootfs.ext2")
+        archive.gzip_uncompress(rootfs_path_gz, rootfs_path)
+
+        self.vm.set_console()
+        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE
+                               + 'console=ttyS0,115200 '
+                               + 'root=/dev/nvme0n1 '
+                               + 'rdinit=/sbin/init noreboot')
+        self.vm.add_args('-kernel', kernel_path,
+                         '-append', kernel_command_line,
+                         '-drive',
+                                f'file={rootfs_path},format=raw,if=none,id=d0',
+                         '-device', 'nvme,serial=default,drive=d0',
+                         '-nic', 'user,model=pcnet',
+                         '-no-reboot', '-snapshot', '-nodefaults')
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Welcome to Buildroot')
+        time.sleep(0.1)
+        exec_command(self, 'root')
+        time.sleep(0.1)
+
+        exec_command_and_wait_for_pattern(self, 'cat /proc/cpuinfo',
+                                                'MIPS I6400')
+        exec_command_and_wait_for_pattern(self, 'reboot',
+                                                'reboot: Restarting system')
+
     def do_test_mips_malta32el_nanomips(self, kernel_url, kernel_hash):
         kernel_path_xz = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
         kernel_path = self.workdir + "kernel"
-- 
2.38.1

Re: [PATCH 0/2] hw/pci-host/gt64120: Fix regression on big-endian targets

[Philippe Mathieu-Daudé]

On 18/1/23 10:40, Philippe Mathieu-Daudé wrote:
> Klauss reported a regression on big-endian targets, introduced
> by commit 145e2198d749 ("hw/mips/gt64xxx_pci: Endian-swap using
> PCI_HOST_BRIDGE MemoryRegionOps"). Fix it and add the Klauss'
> reproducer as Avocado test.

Sorry Klaus for adding an extra 's' in your name :\

Re: [PATCH 0/2] hw/pci-host/gt64120: Fix regression on big-endian targets

[Klaus Jensen]

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

On Jan 18 10:43, Philippe Mathieu-Daudé wrote:
> On 18/1/23 10:40, Philippe Mathieu-Daudé wrote:
> > Klauss reported a regression on big-endian targets, introduced
> > by commit 145e2198d749 ("hw/mips/gt64xxx_pci: Endian-swap using
> > PCI_HOST_BRIDGE MemoryRegionOps"). Fix it and add the Klauss'
> > reproducer as Avocado test.
> 
> Sorry Klaus for adding an extra 's' in your name :\

Don't worry about it ;) Thanks for the quick fix!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]