qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup
@ 2025-04-29 17:03 Rakesh Jeyasingh
  2025-04-29 17:03 ` [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling Rakesh Jeyasingh
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Rakesh Jeyasingh @ 2025-04-29 17:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, philmd, thuth, balaton, rakeshjb010

Changes since v4:
1.Introduced needs_bswap() helper for clean endianness logic
2.use the existing pci_host_data_le_ops.read/write from hw/pci/pci_host.c

v4:https://patchew.org/QEMU/20250331184820.34673-1-rakeshjb010@gmail.com/

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      | 82 ++++++++++++++++++++++----------------
 hw/pci/pci_host.c          |  6 ---
 include/hw/pci-host/dino.h |  4 --
 include/hw/pci/pci_host.h  |  1 -
 4 files changed, 48 insertions(+), 45 deletions(-)

-- 
2.43.0



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling
  2025-04-29 17:03 [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup Rakesh Jeyasingh
@ 2025-04-29 17:03 ` Rakesh Jeyasingh
  2025-05-17  5:25   ` Rakesh Jeyasingh
  2025-05-22 11:32   ` Michael Tokarev
  2025-04-29 17:03 ` [PATCH v5 2/2] hw/pci-host: Remove unused pci_host_data_be_ops Rakesh Jeyasingh
  2025-05-16 18:14 ` [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup Thomas Huth
  2 siblings, 2 replies; 7+ messages in thread
From: Rakesh Jeyasingh @ 2025-04-29 17:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, philmd, thuth, balaton, rakeshjb010

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),
+                          &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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 2/2] hw/pci-host: Remove unused pci_host_data_be_ops
  2025-04-29 17:03 [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup Rakesh Jeyasingh
  2025-04-29 17:03 ` [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling Rakesh Jeyasingh
@ 2025-04-29 17:03 ` Rakesh Jeyasingh
  2025-05-16 18:14 ` [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup Thomas Huth
  2 siblings, 0 replies; 7+ messages in thread
From: Rakesh Jeyasingh @ 2025-04-29 17:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, philmd, thuth, balaton, rakeshjb010

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 abe83bbab8..7179d99178 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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup
  2025-04-29 17:03 [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup Rakesh Jeyasingh
  2025-04-29 17:03 ` [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling Rakesh Jeyasingh
  2025-04-29 17:03 ` [PATCH v5 2/2] hw/pci-host: Remove unused pci_host_data_be_ops Rakesh Jeyasingh
@ 2025-05-16 18:14 ` Thomas Huth
  2 siblings, 0 replies; 7+ messages in thread
From: Thomas Huth @ 2025-05-16 18:14 UTC (permalink / raw)
  To: Rakesh Jeyasingh, qemu-devel, philmd; +Cc: pbonzini, balaton

On 29/04/2025 19.03, Rakesh Jeyasingh wrote:
> Changes since v4:
> 1.Introduced needs_bswap() helper for clean endianness logic
> 2.use the existing pci_host_data_le_ops.read/write from hw/pci/pci_host.c
> 
> v4:https://patchew.org/QEMU/20250331184820.34673-1-rakeshjb010@gmail.com/
> 
> 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      | 82 ++++++++++++++++++++++----------------
>   hw/pci/pci_host.c          |  6 ---
>   include/hw/pci-host/dino.h |  4 --
>   include/hw/pci/pci_host.h  |  1 -
>   4 files changed, 48 insertions(+), 45 deletions(-)

Thanks for tackling this ... I gave it a try on both, little and big endian 
host and it seems to work as expected now.

Series
Tested-by: Thomas Huth <thuth@redhat.com>



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling
  2025-04-29 17:03 ` [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling Rakesh Jeyasingh
@ 2025-05-17  5:25   ` Rakesh Jeyasingh
  2025-05-17 14:16     ` Paolo Bonzini
  2025-05-22 11:32   ` Michael Tokarev
  1 sibling, 1 reply; 7+ messages in thread
From: Rakesh Jeyasingh @ 2025-05-17  5:25 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Philippe Mathieu-Daudé, thuth, BALATON Zoltan

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

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),
> +                          &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: 7406 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling
  2025-05-17  5:25   ` Rakesh Jeyasingh
@ 2025-05-17 14:16     ` Paolo Bonzini
  0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2025-05-17 14:16 UTC (permalink / raw)
  To: Rakesh Jeyasingh, qemu-devel
  Cc: Philippe Mathieu-Daudé, thuth, BALATON Zoltan

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling
  2025-04-29 17:03 ` [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling Rakesh Jeyasingh
  2025-05-17  5:25   ` Rakesh Jeyasingh
@ 2025-05-22 11:32   ` Michael Tokarev
  1 sibling, 0 replies; 7+ messages in thread
From: Michael Tokarev @ 2025-05-22 11:32 UTC (permalink / raw)
  To: Rakesh Jeyasingh, qemu-devel
  Cc: pbonzini, philmd, thuth, balaton, qemu-stable

On 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-05-22 11:33 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-29 17:03 [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup Rakesh Jeyasingh
2025-04-29 17:03 ` [PATCH v5 1/2] hw/pci-host/gt64120: Fix endianness handling Rakesh Jeyasingh
2025-05-17  5:25   ` Rakesh Jeyasingh
2025-05-17 14:16     ` Paolo Bonzini
2025-05-22 11:32   ` Michael Tokarev
2025-04-29 17:03 ` [PATCH v5 2/2] hw/pci-host: Remove unused pci_host_data_be_ops Rakesh Jeyasingh
2025-05-16 18:14 ` [PATCH v5 0/2] GT64120 PCI endianness fixes and cleanup Thomas Huth

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).