On Fri, 28 Mar 2025, Paolo Bonzini wrote:
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.
This may worth a comment but I'm still not convinced this works on big
endian host because I think pci_data_read returns val in host endianness
and if you want big endian then you only need to bswap on LE host not on
BE host. Was this tested on BE host and confirmed it works?
Regards,
BALATON Zoltan
*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
