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