Re: [PATCH] hw/pci-host/gt64120.c: Fix PCI host bridge endianness handling

[BALATON Zoltan]

On Thu, 27 Mar 2025, rakeshj wrote:
The GT-64120 PCI controller requires special handling where:
1. Host bridge (device 0) must use native endianness
2. Other devices follow MByteSwap bit in GT_PCI0_CMD
Previous implementation accidentally swapped all accesses, breaking
host bridge detection (lspci -d 11ab:4620). This fix:

- Adds device filtering via (phb->config_reg & 0x00FFF800)
- Preserves native endianness for host bridge
- Maintains swapping for other devices in big-endian mode

Fixes: 145e2198 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE 
MemoryRegionOps")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2826
Signed-off-by: rakeshj <rakeshjb...@gmail.com>
---
hw/pci-host/gt64120.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index d5c13a89b6..098f8e5988 100644
--- a/hw/pci-host/gt64120.c
+++ b/hw/pci-host/gt64120.c
@@ -320,11 +320,46 @@ static void gt64120_isd_mapping(GT64120State *s)
    memory_region_transaction_commit();
}

+static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, unsigned size)
+{
+    GT64120State *s = opaque;
+    PCIHostState *phb = PCI_HOST_BRIDGE(s);
+    uint32_t val = pci_data_read(phb->bus, phb->config_reg, size);
+
+    /* Only swap for non-bridge devices in big-endian mode */
+    if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
+        val = bswap32(val);

I don't know if this is the best way to fix this issue as I don't know 
what the issue is in the first place (isn't PCI usually little endian?) 
but I think you can't just use bswap here because it also needs to take 
into account the endianness of the host QEMU is running on. So most likely 
you need le32_to_cpu or be32_to_cpu or similar here as you're converting a 
value with known endianness to a variable that is in host endian and the 
*_to_cpu functions are for that. But I could be wrong, these endianness 
issues are quite confusing. It's best if you can test on both LE and BE 
host.

Regards,
BALATON Zoltan

+    }
+    return val;
+}
+
+static void gt64120_pci_data_write(void *opaque, hwaddr addr,
+    uint64_t val, unsigned size)
+{
+    GT64120State *s = opaque;
+    PCIHostState *phb = PCI_HOST_BRIDGE(s);
+    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 | (addr & 3), val, size);
+}
+
+static const MemoryRegionOps gt64120_pci_data_ops = {
+    .read = gt64120_pci_data_read,
+    .write = gt64120_pci_data_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
+};
+
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_data_ops[] = {
-        &pci_host_data_be_ops, &pci_host_data_le_ops
+        &gt64120_pci_data_ops, &pci_host_data_le_ops
    };
    PCIHostState *phb = PCI_HOST_BRIDGE(s);