Re: [PATCH v2 1/2] hw/pci-host/gt64120: Fix PCI I/O config register endianness

[Philippe Mathieu-Daudé]

On 18/1/23 10:57, Philippe Mathieu-Daudé wrote:
> The MByteSwap bit only affects the data register endianness,
> not the config register. Map the config register once in the
> gt64120_realize() handler, and only remap the data register
> when the mapping is updated.
>
> Fixes: 145e2198d7 ("gt64xxx: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps")
> Reported-by: Klaus Jensen <its@irrelevant.dk>
> Tested-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/gt64120.c | 25 +++++++------------------
>   1 file changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index f226d03420..36ed01c615 100644
> --- a/hw/pci-host/gt64120.c
> +++ b/hw/pci-host/gt64120.c
> @@ -320,13 +320,6 @@ static void gt64120_isd_mapping(GT64120State *s)
>   
>   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_conf_ops[] = {
> -        &pci_host_conf_be_ops, &pci_host_conf_le_ops
> -    };
> -    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();
> @@ -339,22 +332,13 @@ static void 
> gt64120_update_pci_cfgdata_mapping(GT64120State *s)
>        * - Table 16: 32-bit PCI Transaction Endianess
>        * - Table 158: PCI_0 Command, Offset: 0xc00
>        */
> -    if (memory_region_is_mapped(&phb->conf_mem)) {
> -        memory_region_del_subregion(&s->ISD_mem, &phb->conf_mem);
> -        object_unparent(OBJECT(&phb->conf_mem));
> -    }
> -    memory_region_init_io(&phb->conf_mem, OBJECT(phb),
> -                          pci_host_conf_ops[s->regs[GT_PCI0_CMD] & 1],
> -                          s, "pci-conf-idx", 4);
> -    memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGADDR << 2,
> -                                        &phb->conf_mem, 1);
> -
>       if (memory_region_is_mapped(&phb->data_mem)) {
>           memory_region_del_subregion(&s->ISD_mem, &phb->data_mem);
>           object_unparent(OBJECT(&phb->data_mem));
>       }

Self-NACK since the config space reads are swapped:

  $ lspci -v
- 00:00.0 Host bridge: Marvell Technology Group Ltd. 
GT-64120/64120A/64121A System Controller (rev 10)
-     Subsystem: Red Hat, Inc Device 1100
-     Flags: medium devsel
-     Memory at <unassigned> (32-bit, prefetchable) [disabled]
-     Memory at 01000000 (32-bit, prefetchable) [disabled] [size=16M]
-     Memory at <ignored> (32-bit, non-prefetchable) [disabled]
-     Memory at <ignored> (32-bit, non-prefetchable) [disabled]
-     Memory at <ignored> (32-bit, non-prefetchable) [disabled]
-     I/O ports at <ignored> [disabled]
+ 00:00.0 Network and computing encryption device: Device 2046:ab11 (rev 06)
+     Subsystem: Device 0011:f41a
+     Flags: fast devsel
+     Memory at <ignored> (32-bit, non-prefetchable)
+     I/O ports at <ignored> [disabled]
+     Memory at 10040020 (64-bit, prefetchable) [size=16]
+     Memory at 10040030 (64-bit, non-prefetchable) [size=16]