On 3/27/25 21:50, BALATON Zoltan wrote:
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?)
Yes but this particular PCI host bridge is the exception, as it is the
only user of pci_host_data_be_ops.
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.
It should be fine. You should take into account:
- the endianness produced by pci_data_read/pci_data_write (always little
endian)
- the endianness expected by the guest (big endian under the conditions
in the patch)
- the endianness expected by memory.c (always little endian, as
specified in gt64120_pci_data_ops)
Because there is either zero or one mismatch, bswap32 is fine.
*However*, there is some extra complication that is unnecessary after
this patch:
- right now the !(s->regs[GT_PCI0_CMD] & 1) that you have added is dead
code: if it was ever 1, gt64120_update_pci_cfgdata_mapping() would pick
pci_host_data_ops[1] and gt64120_pci_data_read/write would not run at all!
- but alternatively you could keep that conditional, and get rid
completely of gt64120_update_pci_cfgdata_mapping(). Just keep the
initialization code
memory_region_init_io(&phb->data_mem, OBJECT(phb),
>64120_pci_data_ops,
s, "pci-conf-data", 4);
memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGDATA << 2,
&phb->data_mem, 1);
at the end of gt64120_realize() where the sister region phb->conf_mem is
already being initialized. This would actually make me happy.
Either way, pci_host_data_be_ops is dead after this patch and you can
remove it; and since you are at it, you may also want to remove the
wrong comment
/*
* PCI_CONFIG_ADDR is parent_obj.config_reg, via pci_host_conf_be_ops,
* so that we can map PCI_CONFIG_DATA to pci_host_data_be_ops.
*/
in include/hw/pci-host/dino.h: DINO emulation has *never* used
pci_host_data_be_ops.
This has snowballed a bit, but it should not be a problem. :)
Paolo
