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 <rakeshjb...@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 <rakeshjb...@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),
> + >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
>
>
