hw/pci-host/gt64120.c | 83 ++++++++++++++++++++++---------------- hw/pci/pci_host.c | 6 --- include/hw/pci-host/dino.h | 5 +-- include/hw/pci/pci_host.h | 1 - 4 files changed, 50 insertions(+), 45 deletions(-)
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 patch:
- Adds custom read/write handlers to preserve native big-endian for the host
bridge (phb->config_reg & 0x00fff800 == 0).
- Swaps non-bridge devices when MByteSwap = 0, using size-appropriate swaps
(bswap16 for 2-byte, bswap32 for 4-byte) to convert PCI's little-endian data
to match the MIPS guest's big-endian expectation; no swap occurs for the host
bridge or when MByteSwap = 1 (little-endian mode).
- Removes gt64120_update_pci_cfgdata_mapping(), moving data_mem initialization
to gt64120_realize()
- Removes unused pci_host_data_be_ops and a misleading comment in dino.h.
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 | 83 ++++++++++++++++++++++----------------
hw/pci/pci_host.c | 6 ---
include/hw/pci-host/dino.h | 5 +--
include/hw/pci/pci_host.h | 1 -
4 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index d5c13a89b6..4e45d0aa53 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,49 @@ static const MemoryRegionOps isd_mem_ops = {
},
};
+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)) {
+ if (size == 2) {
+ val = bswap16(val);
+ } else if (size == 4) {
+ 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);
+ if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
+ if (size == 2) {
+ val = bswap16(val);
+ } else if (size == 4) {
+ 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 = 1,
+ .max_access_size = 4,
+ },
+};
+
static void gt64120_reset(DeviceState *dev)
{
GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
@@ -1178,7 +1188,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 +1211,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
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..df509dbc18 100644
--- a/include/hw/pci-host/dino.h
+++ b/include/hw/pci-host/dino.h
@@ -109,10 +109,7 @@ 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.0Hi Rakesh,
On 29/3/25 01:49, 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 patch:
> - Adds custom read/write handlers to preserve native big-endian for the host
> bridge (phb->config_reg & 0x00fff800 == 0).
Here you check for bus == 0 && device == 0, is that what you want? (I'm
confused because you only describe "for the host bridge").
If so I'd rather add a self-describing method:
static bool is_phb_dev0(const PCIHostState *phb)
{
return extract32(phb->config_reg, 11, 5 /* dev */ + 8 /* bus) == 0;
}
> - Swaps non-bridge devices when MByteSwap = 0, using size-appropriate swaps
> (bswap16 for 2-byte, bswap32 for 4-byte) to convert PCI's little-endian data
> to match the MIPS guest's big-endian expectation; no swap occurs for the host
> bridge or when MByteSwap = 1 (little-endian mode).
> - Removes gt64120_update_pci_cfgdata_mapping(), moving data_mem initialization
> to gt64120_realize()
> - Removes unused pci_host_data_be_ops and a misleading comment in dino.h.
>
> 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 | 83 ++++++++++++++++++++++----------------
> hw/pci/pci_host.c | 6 ---
> include/hw/pci-host/dino.h | 5 +--
> include/hw/pci/pci_host.h | 1 -
> 4 files changed, 50 insertions(+), 45 deletions(-)
>
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index d5c13a89b6..4e45d0aa53 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,49 @@ static const MemoryRegionOps isd_mem_ops = {
> },
> };
>
> +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);
You check the Enable bit in the write path but not here, any reason?
> +
> + /* Only swap for non-bridge devices in big-endian mode */
> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
Please use instead of (s->regs[GT_PCI0_CMD] & 1):
bswap = FIELD_EX32(s->regs[GT_PCI0_CMD]m GT_PCI0_CMD, MByteSwap);
> + if (size == 2) {
> + val = bswap16(val);
> + } else if (size == 4) {
> + 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);
> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
> + if (size == 2) {
> + val = bswap16(val);
> + } else if (size == 4) {
> + 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 = 1,
> + .max_access_size = 4,
> + },
> +};
Per GT64120 rev 1.4, chapter "6.2.2 PCI Master CPU Byte Swapping":
When the GT–64120 is configured for 64-bit PCI mode, byte
swapping occurs across all 8 byte lanes when the ByteSwap
bit is set for PCI_0.
> static void gt64120_reset(DeviceState *dev)
> {
> GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
> @@ -1178,7 +1188,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 +1211,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
Please split the changes below in a distinct cleanup patch.
> 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..df509dbc18 100644
> --- a/include/hw/pci-host/dino.h
> +++ b/include/hw/pci-host/dino.h
> @@ -109,10 +109,7 @@ 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 */
On Sat, Mar 29, 2025 at 5:18 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:
> Hi Rakesh,
>
> On 29/3/25 01:49, 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 patch:
> > - Adds custom read/write handlers to preserve native big-endian for the
> host
> > bridge (phb->config_reg & 0x00fff800 == 0).
>
> Here you check for bus == 0 && device == 0, is that what you want? (I'm
> confused because you only describe "for the host bridge").
yes, I meant bus 0, device 0
>
If so I'd rather add a self-describing method:
>
> static bool is_phb_dev0(const PCIHostState *phb)
> {
> return extract32(phb->config_reg, 11, 5 /* dev */ + 8 /* bus) == 0;
> }
>
> > - Swaps non-bridge devices when MByteSwap = 0, using size-appropriate
> swaps
> > (bswap16 for 2-byte, bswap32 for 4-byte) to convert PCI's
> little-endian data
> > to match the MIPS guest's big-endian expectation; no swap occurs for
> the host
> > bridge or when MByteSwap = 1 (little-endian mode).
> > - Removes gt64120_update_pci_cfgdata_mapping(), moving data_mem
> initialization
> > to gt64120_realize()
> > - Removes unused pci_host_data_be_ops and a misleading comment in dino.h.
> >
> > 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 | 83 ++++++++++++++++++++++----------------
> > hw/pci/pci_host.c | 6 ---
> > include/hw/pci-host/dino.h | 5 +--
> > include/hw/pci/pci_host.h | 1 -
> > 4 files changed, 50 insertions(+), 45 deletions(-)
> >
> > diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> > index d5c13a89b6..4e45d0aa53 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,49 @@ static const MemoryRegionOps isd_mem_ops = {
> > },
> > };
> >
> > +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);
>
> You check the Enable bit in the write path but not here, any reason?
>
thanks for pointing it out, I missed to notice it
> > +
> > + /* Only swap for non-bridge devices in big-endian mode */
> > + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
>
> Please use instead of (s->regs[GT_PCI0_CMD] & 1):
>
> bswap = FIELD_EX32(s->regs[GT_PCI0_CMD]m GT_PCI0_CMD, MByteSwap);
>
> > + if (size == 2) {
> > + val = bswap16(val);
> > + } else if (size == 4) {
> > + 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);
> > + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
> > + if (size == 2) {
> > + val = bswap16(val);
> > + } else if (size == 4) {
> > + 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 = 1,
> > + .max_access_size = 4,
> > + },
> > +};
>
> Per GT64120 rev 1.4, chapter "6.2.2 PCI Master CPU Byte Swapping":
>
> When the GT–64120 is configured for 64-bit PCI mode, byte
> swapping occurs across all 8 byte lanes when the ByteSwap
> bit is set for PCI_0.
>
> > static void gt64120_reset(DeviceState *dev)
> > {
> > GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
> > @@ -1178,7 +1188,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 +1211,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
>
> Please split the changes below in a distinct cleanup patch.
>
> > 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..df509dbc18 100644
> > --- a/include/hw/pci-host/dino.h
> > +++ b/include/hw/pci-host/dino.h
> > @@ -109,10 +109,7 @@ 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 */
>
Proposed v3 Changes:
1. Use extract32 in is_phb_dev0() for bus 0, device 0 check (Philippe).
2. FIELD_EX32 instead of (s->regs[GT_PCI0_CMD] & 1) (Philippe).
3. Size-specific swaps: bswap16 for 2-byte, bswap32 for 4-byte (Paolo).
4. check the Enable bit in the read path (Philippe).
5. Split cleanup into [PATCH v3 2/2].
On Sat, 29 Mar 2025, Philippe Mathieu-Daudé wrote:
> Hi Rakesh,
>
> On 29/3/25 01:49, 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 patch:
>> - Adds custom read/write handlers to preserve native big-endian for the
>> host
>> bridge (phb->config_reg & 0x00fff800 == 0).
>
> Here you check for bus == 0 && device == 0, is that what you want? (I'm
> confused because you only describe "for the host bridge").
>
> If so I'd rather add a self-describing method:
>
> static bool is_phb_dev0(const PCIHostState *phb)
> {
> return extract32(phb->config_reg, 11, 5 /* dev */ + 8 /* bus) == 0;
> }
There are already macros such as PCI_BUS_NUM PCI_FUNC. Are they any use
instead of another function?
Regards,
BALATON Zoltan
>> - Swaps non-bridge devices when MByteSwap = 0, using size-appropriate swaps
>> (bswap16 for 2-byte, bswap32 for 4-byte) to convert PCI's little-endian
>> data
>> to match the MIPS guest's big-endian expectation; no swap occurs for the
>> host
>> bridge or when MByteSwap = 1 (little-endian mode).
>> - Removes gt64120_update_pci_cfgdata_mapping(), moving data_mem
>> initialization
>> to gt64120_realize()
>> - Removes unused pci_host_data_be_ops and a misleading comment in dino.h.
>>
>> 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 | 83 ++++++++++++++++++++++----------------
>> hw/pci/pci_host.c | 6 ---
>> include/hw/pci-host/dino.h | 5 +--
>> include/hw/pci/pci_host.h | 1 -
>> 4 files changed, 50 insertions(+), 45 deletions(-)
>>
>> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
>> index d5c13a89b6..4e45d0aa53 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,49 @@ static const MemoryRegionOps isd_mem_ops = {
>> },
>> };
>> +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);
>
> You check the Enable bit in the write path but not here, any reason?
>> +
>> + /* Only swap for non-bridge devices in big-endian mode */
>> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
>
> Please use instead of (s->regs[GT_PCI0_CMD] & 1):
>
> bswap = FIELD_EX32(s->regs[GT_PCI0_CMD]m GT_PCI0_CMD, MByteSwap);
>
>> + if (size == 2) {
>> + val = bswap16(val);
>> + } else if (size == 4) {
>> + 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);
>> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
>> + if (size == 2) {
>> + val = bswap16(val);
>> + } else if (size == 4) {
>> + 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 = 1,
>> + .max_access_size = 4,
>> + },
>> +};
>
> Per GT64120 rev 1.4, chapter "6.2.2 PCI Master CPU Byte Swapping":
>
> When the GT–64120 is configured for 64-bit PCI mode, byte
> swapping occurs across all 8 byte lanes when the ByteSwap
> bit is set for PCI_0.
>
>> static void gt64120_reset(DeviceState *dev)
>> {
>> GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
>> @@ -1178,7 +1188,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 +1211,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
>
> Please split the changes below in a distinct cleanup patch.
>
>> 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..df509dbc18 100644
>> --- a/include/hw/pci-host/dino.h
>> +++ b/include/hw/pci-host/dino.h
>> @@ -109,10 +109,7 @@ 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 */
>
>
Thanks, BALATON
I looked into PCI_BUS_NUM and PCI_SLOT from include/hw/pci/pci.h (L15-24):
- PCI_BUS_NUM(x) (((x) >> 8) & 0xff)) --> bits 15-8.
- PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)),
which don’t align properly with the 32-bit phb->config_reg layout used in
your GT-64120 . Since these macros are intended for a 16-bit BDF
(Bus-Device-Function) format rather than the full 32-bit PCI config
address, For phb->config_reg (32-bit: bus 23-16, device 15-11) these
extract the wrong bits
On Sat, Mar 29, 2025 at 6:48 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Sat, 29 Mar 2025, Philippe Mathieu-Daudé wrote:
> > Hi Rakesh,
> >
> > On 29/3/25 01:49, 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 patch:
> >> - Adds custom read/write handlers to preserve native big-endian for the
> >> host
> >> bridge (phb->config_reg & 0x00fff800 == 0).
> >
> > Here you check for bus == 0 && device == 0, is that what you want? (I'm
> > confused because you only describe "for the host bridge").
> >
> > If so I'd rather add a self-describing method:
> >
> > static bool is_phb_dev0(const PCIHostState *phb)
> > {
> > return extract32(phb->config_reg, 11, 5 /* dev */ + 8 /* bus) == 0;
> > }
>
> There are already macros such as PCI_BUS_NUM PCI_FUNC. Are they any use
> instead of another function?
>
> Regards,
> BALATON Zoltan
>
> >> - Swaps non-bridge devices when MByteSwap = 0, using size-appropriate
> swaps
> >> (bswap16 for 2-byte, bswap32 for 4-byte) to convert PCI's
> little-endian
> >> data
> >> to match the MIPS guest's big-endian expectation; no swap occurs for
> the
> >> host
> >> bridge or when MByteSwap = 1 (little-endian mode).
> >> - Removes gt64120_update_pci_cfgdata_mapping(), moving data_mem
> >> initialization
> >> to gt64120_realize()
> >> - Removes unused pci_host_data_be_ops and a misleading comment in
> dino.h.
> >>
> >> 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 | 83 ++++++++++++++++++++++----------------
> >> hw/pci/pci_host.c | 6 ---
> >> include/hw/pci-host/dino.h | 5 +--
> >> include/hw/pci/pci_host.h | 1 -
> >> 4 files changed, 50 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> >> index d5c13a89b6..4e45d0aa53 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,49 @@ static const MemoryRegionOps isd_mem_ops = {
> >> },
> >> };
> >> +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);
> >
> > You check the Enable bit in the write path but not here, any reason?
> >> +
> >> + /* Only swap for non-bridge devices in big-endian mode */
> >> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800))
> {
> >
> > Please use instead of (s->regs[GT_PCI0_CMD] & 1):
> >
> > bswap = FIELD_EX32(s->regs[GT_PCI0_CMD]m GT_PCI0_CMD, MByteSwap);
> >
> >> + if (size == 2) {
> >> + val = bswap16(val);
> >> + } else if (size == 4) {
> >> + 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);
> >> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800))
> {
> >> + if (size == 2) {
> >> + val = bswap16(val);
> >> + } else if (size == 4) {
> >> + 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 = 1,
> >> + .max_access_size = 4,
> >> + },
> >> +};
> >
> > Per GT64120 rev 1.4, chapter "6.2.2 PCI Master CPU Byte Swapping":
> >
> > When the GT–64120 is configured for 64-bit PCI mode, byte
> > swapping occurs across all 8 byte lanes when the ByteSwap
> > bit is set for PCI_0.
> >
> >> static void gt64120_reset(DeviceState *dev)
> >> {
> >> GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
> >> @@ -1178,7 +1188,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 +1211,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
> >
> > Please split the changes below in a distinct cleanup patch.
> >
> >> 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..df509dbc18 100644
> >> --- a/include/hw/pci-host/dino.h
> >> +++ b/include/hw/pci-host/dino.h
> >> @@ -109,10 +109,7 @@ 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 */
> >
> >
© 2016 - 2025 Red Hat, Inc.