diff mbox series

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

Message ID 20230104133935.4639-6-philmd@linaro.org
State Superseded
Headers show
Series hw/mips/gt64xxx_pci: Fix endianness swap on big-endian hosts | expand

Commit Message

Philippe Mathieu-Daudé Jan. 4, 2023, 1:39 p.m. UTC
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(-)

Comments

Nathan Chancellor Jan. 23, 2023, 9:52 p.m. UTC | #1
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
BALATON Zoltan Jan. 24, 2023, 2:17 a.m. UTC | #2
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
>
>
Alex Bennée Feb. 20, 2023, 10:43 p.m. UTC | #3
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.
diff mbox series

Patch

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)