[PATCH v4 0/2] GT64120 PCI endianness fixes and cleanup

[PATCH v4 0/2] GT64120 PCI endianness fixes and cleanup

[Rakesh Jeyasingh]

*** BLURB HERE ***

Changes since v3:
In [PATCH v3 1/2] hw/pci-host/gt64120: Fix endianness handling
1.Set .min_access_size = 4 in MemoryRegionOps
2.Remove the now-unnecessary bswap16 handling

v3: https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06942.html


Rakesh Jeyasingh (2):
  hw/pci-host/gt64120: Fix endianness handling
  hw/pci-host: Remove unused pci_host_data_be_ops

 hw/pci-host/gt64120.c      | 91 ++++++++++++++++++++++++--------------
 hw/pci/pci_host.c          |  6 ---
 include/hw/pci-host/dino.h |  4 --
 include/hw/pci/pci_host.h  |  1 -
 4 files changed, 57 insertions(+), 45 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] hw/pci-host/gt64120: Fix endianness handling

[Rakesh Jeyasingh]

The GT-64120 PCI controller requires special handling where:
1. Host bridge(bus 0 ,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 patch:
- Removes gt64120_update_pci_cfgdata_mapping(), moving data_mem initialization
  to gt64120_realize()
- Adds custom read/write handlers
- Replace raw bit check with FIELD_EX32 for MByteSwap .
- Use extract32 for bus/device check (bus 0, device 0).
- Implement size-specific swaps (bswap16 for 2-byte, bswap32 for 4-byte)
  per MemoryRegionOps requirements.

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 | 91 +++++++++++++++++++++++++++----------------
 1 file changed, 57 insertions(+), 34 deletions(-)

diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index d5c13a89b6..3595d8127e 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,57 @@ static const MemoryRegionOps isd_mem_ops = {
     },
 };
 
+static bool is_phb_dev0(const PCIHostState *phb)
+{
+    /*Checks if the current PCI configuration access targets the host bridge(bus 0, device 0)*/
+    return extract32(phb->config_reg, 11, 5/*dev*/ + 8/*bus*/) == 0;
+}
+
+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;
+    bool le_mode = FIELD_EX32(s->regs[GT_PCI0_CMD], GT_PCI0_CMD, MByteSwap);
+
+    if (!(phb->config_reg & (1 << 31))) {
+        val = 0xffffffff;
+    } else {
+        val = pci_data_read(phb->bus, phb->config_reg | (addr & 3), size);
+    }
+
+    /* Only swap for non-bridge devices in big-endian mode */
+    if (!le_mode && !is_phb_dev0(phb)) {
+        val = bswap32(val);
+    }
+    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);
+    bool le_mode = FIELD_EX32(s->regs[GT_PCI0_CMD], GT_PCI0_CMD, MByteSwap);
+
+    if (!le_mode && !is_phb_dev0(phb)) {
+        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 = 4,
+        .max_access_size = 4,
+    },
+};
+
 static void gt64120_reset(DeviceState *dev)
 {
     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
@@ -1178,7 +1196,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 +1219,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),
+                          &gt64120_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

Re: [PATCH v4 1/2] hw/pci-host/gt64120: Fix endianness handling

[Rakesh J]

[-- Attachment #1: Type: text/plain, Size: 6834 bytes --]

Hi ,

Thank you for reviewing the previous versions of this patch. I've
incorporated all the feedback in v4:
1. set .min_access_size=4
2. Simplified swapping to bswap32 only

Note: I realized after sending v4 that the commit message still referenced
the old size-specific swap approach -"Implement size-specific swaps
(bswap16 for 2-byte, bswap32 for 4-byte)
  per MemoryRegionOps requirements". The actual patch content correctly
implements the simplified DWORD-only behavior.

Before this moves forward, could you please confirm:

   1. Are there any remaining corrections needed?
   2. Would you like me to resend with an updated commit message?

Appreciate your guidance, and apologies for the message oversight!

Regards,
Rakesh

On Tue, Apr 1, 2025 at 12:18 AM Rakesh Jeyasingh <rakeshjb010@gmail.com>
wrote:

> The GT-64120 PCI controller requires special handling where:
> 1. Host bridge(bus 0 ,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 patch:
> - Removes gt64120_update_pci_cfgdata_mapping(), moving data_mem
> initialization
>   to gt64120_realize()
> - Adds custom read/write handlers
> - Replace raw bit check with FIELD_EX32 for MByteSwap .
> - Use extract32 for bus/device check (bus 0, device 0).
> - Implement size-specific swaps (bswap16 for 2-byte, bswap32 for 4-byte)
>   per MemoryRegionOps requirements.
>
> 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 | 91 +++++++++++++++++++++++++++----------------
>  1 file changed, 57 insertions(+), 34 deletions(-)
>
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index d5c13a89b6..3595d8127e 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,57 @@ static const MemoryRegionOps isd_mem_ops = {
>      },
>  };
>
> +static bool is_phb_dev0(const PCIHostState *phb)
> +{
> +    /*Checks if the current PCI configuration access targets the host
> bridge(bus 0, device 0)*/
> +    return extract32(phb->config_reg, 11, 5/*dev*/ + 8/*bus*/) == 0;
> +}
> +
> +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;
> +    bool le_mode = FIELD_EX32(s->regs[GT_PCI0_CMD], GT_PCI0_CMD,
> MByteSwap);
> +
> +    if (!(phb->config_reg & (1 << 31))) {
> +        val = 0xffffffff;
> +    } else {
> +        val = pci_data_read(phb->bus, phb->config_reg | (addr & 3), size);
> +    }
> +
> +    /* Only swap for non-bridge devices in big-endian mode */
> +    if (!le_mode && !is_phb_dev0(phb)) {
> +        val = bswap32(val);
> +    }
> +    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);
> +    bool le_mode = FIELD_EX32(s->regs[GT_PCI0_CMD], GT_PCI0_CMD,
> MByteSwap);
> +
> +    if (!le_mode && !is_phb_dev0(phb)) {
> +        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 = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
>  static void gt64120_reset(DeviceState *dev)
>  {
>      GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
> @@ -1178,7 +1196,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 +1219,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),
> +                          &gt64120_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
>
>

[-- Attachment #2: Type: text/html, Size: 8260 bytes --]

[PATCH v4 2/2] hw/pci-host: Remove unused pci_host_data_be_ops

[Rakesh Jeyasingh]

pci_host_data_be_ops became unused after endianness fixes

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Rakesh Jeyasingh <rakeshjb010@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/pci_host.c          | 6 ------
 include/hw/pci-host/dino.h | 4 ----
 include/hw/pci/pci_host.h  | 1 -
 3 files changed, 11 deletions(-)

diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 80f91f409f..56f7f28a1a 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -217,12 +217,6 @@ const MemoryRegionOps pci_host_data_le_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-const MemoryRegionOps pci_host_data_be_ops = {
-    .read = pci_host_data_read,
-    .write = pci_host_data_write,
-    .endianness = DEVICE_BIG_ENDIAN,
-};
-
 static bool pci_host_needed(void *opaque)
 {
     PCIHostState *s = opaque;
diff --git a/include/hw/pci-host/dino.h b/include/hw/pci-host/dino.h
index fd7975c798..5dc8cdf610 100644
--- a/include/hw/pci-host/dino.h
+++ b/include/hw/pci-host/dino.h
@@ -109,10 +109,6 @@ static const uint32_t reg800_keep_bits[DINO800_REGS] = {
 struct DinoState {
     PCIHostState parent_obj;
 
-    /*
-     * 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.
-     */
     uint32_t config_reg_dino; /* keep original copy, including 2 lowest bits */
 
     uint32_t iar0;
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index e52d8ec2cd..954dd446fa 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -68,6 +68,5 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, unsigned len);
 extern const MemoryRegionOps pci_host_conf_le_ops;
 extern const MemoryRegionOps pci_host_conf_be_ops;
 extern const MemoryRegionOps pci_host_data_le_ops;
-extern const MemoryRegionOps pci_host_data_be_ops;
 
 #endif /* PCI_HOST_H */
-- 
2.43.0