The GT-64120 PCI controller requires special handling where:
1. Host bridge(bus 0 ,device 0) must never be byte-swapped
2. Other devices follow MByteSwap bit in GT_PCI0_CMD
The previous implementation incorrectly swapped all accesses, breaking
host bridge detection (lspci -d 11ab:4620).
Changes made:
1. Removed gt64120_update_pci_cfgdata_mapping() and moved data_mem initialization
to gt64120_realize() for cleaner setup
2. Implemented custom read/write handlers that:
- Preserve host bridge accesses (extract32(config_reg,11,13)==0)
- apply swapping only for non-bridge 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: Rakesh Jeyasingh <rakeshjb010@gmail.com>
---
hw/pci-host/gt64120.c | 82 +++++++++++++++++++++++++------------------
1 file changed, 48 insertions(+), 34 deletions(-)
diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index 56a6ef93b7..ecb203a3d0 100644
--- a/hw/pci-host/gt64120.c
+++ b/hw/pci-host/gt64120.c
@@ -320,38 +320,6 @@ 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_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 Endianness bit in the CPU
- * Configuration Register. See:
- * - Table 16: 32-bit PCI Transaction Endianness
- * - Table 158: PCI_0 Command, Offset: 0xc00
- */
-
- 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();
@@ -645,7 +613,6 @@ 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:
@@ -1024,6 +991,48 @@ static const MemoryRegionOps isd_mem_ops = {
},
};
+static bool bswap(const GT64120State *s)
+{
+ PCIHostState *phb = PCI_HOST_BRIDGE(s);
+ /*check for bus == 0 && device == 0, Bits 11:15 = Device , Bits 16:23 = Bus*/
+ bool is_phb_dev0 = extract32(phb->config_reg, 11, 13) == 0;
+ bool le_mode = FIELD_EX32(s->regs[GT_PCI0_CMD], GT_PCI0_CMD, MByteSwap);
+ /* Only swap for non-bridge devices in big-endian mode */
+ return !le_mode && !is_phb_dev0;
+}
+
+static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, unsigned size)
+{
+ GT64120State *s = opaque;
+ uint64_t val = pci_host_data_le_ops.read(opaque, addr, size);
+
+ if (bswap(s)) {
+ val = bswap32(val);
+ }
+ return val;
+}
+
+static void gt64120_pci_data_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+ GT64120State *s = opaque;
+
+ if (bswap(s)) {
+ val = bswap32(val);
+ }
+ pci_host_data_le_ops.write(opaque, addr, 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 = 4,
+ .max_access_size = 4,
+ },
+};
+
static void gt64120_reset(DeviceState *dev)
{
GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
@@ -1178,7 +1187,6 @@ 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)
@@ -1202,6 +1210,12 @@ static void gt64120_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGADDR << 2,
&phb->conf_mem, 1);
+ 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);
+
/*
* The whole address space decoded by the GT-64120A doesn't generate
--
2.43.0On 29.04.2025 20:03, Rakesh Jeyasingh wrote:
> The GT-64120 PCI controller requires special handling where:
> 1. Host bridge(bus 0 ,device 0) must never be byte-swapped
> 2. Other devices follow MByteSwap bit in GT_PCI0_CMD
>
> The previous implementation incorrectly swapped all accesses, breaking
> host bridge detection (lspci -d 11ab:4620).
>
> Changes made:
> 1. Removed gt64120_update_pci_cfgdata_mapping() and moved data_mem initialization
> to gt64120_realize() for cleaner setup
> 2. Implemented custom read/write handlers that:
> - Preserve host bridge accesses (extract32(config_reg,11,13)==0)
> - apply swapping only for non-bridge 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: Rakesh Jeyasingh <rakeshjb010@gmail.com>
This seems to be qemu-stable material.
I picked this one up for 9.2 and 10.0,
please let me know if I shouldn't.
Thanks,
/mjt
On Tue, Apr 29, 2025 at 10:34 PM Rakesh Jeyasingh <rakeshjb010@gmail.com>
wrote:
> The GT-64120 PCI controller requires special handling where:
> 1. Host bridge(bus 0 ,device 0) must never be byte-swapped
> 2. Other devices follow MByteSwap bit in GT_PCI0_CMD
>
> The previous implementation incorrectly swapped all accesses, breaking
> host bridge detection (lspci -d 11ab:4620).
>
> Changes made:
> 1. Removed gt64120_update_pci_cfgdata_mapping() and moved data_mem
> initialization
> to gt64120_realize() for cleaner setup
> 2. Implemented custom read/write handlers that:
> - Preserve host bridge accesses (extract32(config_reg,11,13)==0)
> - apply swapping only for non-bridge 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: Rakesh Jeyasingh <rakeshjb010@gmail.com>
> ---
> hw/pci-host/gt64120.c | 82 +++++++++++++++++++++++++------------------
> 1 file changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index 56a6ef93b7..ecb203a3d0 100644
> --- a/hw/pci-host/gt64120.c
> +++ b/hw/pci-host/gt64120.c
> @@ -320,38 +320,6 @@ 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_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 Endianness bit in
> the CPU
> - * Configuration Register. See:
> - * - Table 16: 32-bit PCI Transaction Endianness
> - * - Table 158: PCI_0 Command, Offset: 0xc00
> - */
> -
> - 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();
> @@ -645,7 +613,6 @@ 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:
> @@ -1024,6 +991,48 @@ static const MemoryRegionOps isd_mem_ops = {
> },
> };
>
> +static bool bswap(const GT64120State *s)
> +{
> + PCIHostState *phb = PCI_HOST_BRIDGE(s);
> + /*check for bus == 0 && device == 0, Bits 11:15 = Device , Bits 16:23
> = Bus*/
> + bool is_phb_dev0 = extract32(phb->config_reg, 11, 13) == 0;
> + bool le_mode = FIELD_EX32(s->regs[GT_PCI0_CMD], GT_PCI0_CMD,
> MByteSwap);
> + /* Only swap for non-bridge devices in big-endian mode */
> + return !le_mode && !is_phb_dev0;
> +}
> +
> +static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, unsigned
> size)
> +{
> + GT64120State *s = opaque;
> + uint64_t val = pci_host_data_le_ops.read(opaque, addr, size);
>
Hi , I just noticed that I made a mistake in the declaring data length of
read val. In previous commits declared as uint32_t (as I think the PCI
controller handles 32-bit values, while MemoryRegionOps uses uint64_t
returns)
Should i consider:
1.sending corrected patch(hoping it not yet merged)
2.or any suggestion on fixing it?
> +
> + if (bswap(s)) {
> + val = bswap32(val);
> + }
> + return val;
> +}
> +
> +static void gt64120_pci_data_write(void *opaque, hwaddr addr,
> + uint64_t val, unsigned size)
> +{
> + GT64120State *s = opaque;
> +
> + if (bswap(s)) {
> + val = bswap32(val);
> + }
> + pci_host_data_le_ops.write(opaque, addr, 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 = 4,
> + .max_access_size = 4,
> + },
> +};
> +
> static void gt64120_reset(DeviceState *dev)
> {
> GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
> @@ -1178,7 +1187,6 @@ 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)
> @@ -1202,6 +1210,12 @@ static void gt64120_realize(DeviceState *dev, Error
> **errp)
> memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGADDR << 2,
> &phb->conf_mem, 1);
>
> + 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);
> +
>
> /*
> * The whole address space decoded by the GT-64120A doesn't generate
> --
> 2.43.0
>
>
On 5/17/25 07:25, Rakesh Jeyasingh wrote:
>
>
> On Tue, Apr 29, 2025 at 10:34 PM Rakesh Jeyasingh <rakeshjb010@gmail.com
> <mailto:rakeshjb010@gmail.com>> wrote:
>
> The GT-64120 PCI controller requires special handling where:
> 1. Host bridge(bus 0 ,device 0) must never be byte-swapped
> 2. Other devices follow MByteSwap bit in GT_PCI0_CMD
>
> The previous implementation incorrectly swapped all accesses, breaking
> host bridge detection (lspci -d 11ab:4620).
>
> Changes made:
> 1. Removed gt64120_update_pci_cfgdata_mapping() and moved data_mem
> initialization
> to gt64120_realize() for cleaner setup
> 2. Implemented custom read/write handlers that:
> - Preserve host bridge accesses (extract32(config_reg,11,13)==0)
> - apply swapping only for non-bridge 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
> <https://gitlab.com/qemu-project/qemu/-/issues/2826>
>
> Signed-off-by: Rakesh Jeyasingh <rakeshjb010@gmail.com
> <mailto:rakeshjb010@gmail.com>>
> ---
> hw/pci-host/gt64120.c | 82 +++++++++++++++++++++++++------------------
> 1 file changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index 56a6ef93b7..ecb203a3d0 100644
> --- a/hw/pci-host/gt64120.c
> +++ b/hw/pci-host/gt64120.c
> @@ -320,38 +320,6 @@ 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_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 Endianness bit
> in the CPU
> - * Configuration Register. See:
> - * - Table 16: 32-bit PCI Transaction Endianness
> - * - Table 158: PCI_0 Command, Offset: 0xc00
> - */
> -
> - 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();
> @@ -645,7 +613,6 @@ 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:
> @@ -1024,6 +991,48 @@ static const MemoryRegionOps isd_mem_ops = {
> },
> };
>
> +static bool bswap(const GT64120State *s)
> +{
> + PCIHostState *phb = PCI_HOST_BRIDGE(s);
> + /*check for bus == 0 && device == 0, Bits 11:15 = Device , Bits
> 16:23 = Bus*/
> + bool is_phb_dev0 = extract32(phb->config_reg, 11, 13) == 0;
> + bool le_mode = FIELD_EX32(s->regs[GT_PCI0_CMD], GT_PCI0_CMD,
> MByteSwap);
> + /* Only swap for non-bridge devices in big-endian mode */
> + return !le_mode && !is_phb_dev0;
> +}
> +
> +static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr,
> unsigned size)
> +{
> + GT64120State *s = opaque;
> + uint64_t val = pci_host_data_le_ops.read(opaque, addr, size);
>
> Hi , I just noticed that I made a mistake in the declaring data length
> of read val. In previous commits declared as uint32_t (as I think the
> PCI controller handles 32-bit values, while MemoryRegionOps uses
> uint64_t returns)
> Should i consider:
> 1.sending corrected patch(hoping it not yet merged)
> 2.or any suggestion on fixing it?
It works with uint64_t as well, no worries. But I can also change it (I
haven't applied the patch yet because I was expecting someone else to do
it, but I will pick it up now).
Paolo
© 2016 - 2025 Red Hat, Inc.