| Message ID | 20230118095751.49728-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci-host/gt64120: Fix regression on big-endian targets | expand |
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]
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)); } memory_region_init_io(&phb->data_mem, OBJECT(phb), - pci_host_data_ops[s->regs[GT_PCI0_CMD] & 1], + (s->regs[GT_PCI0_CMD] & 1) ? &pci_host_data_le_ops + : &pci_host_data_be_ops, s, "pci-conf-data", 4); memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGDATA << 2, &phb->data_mem, 1); @@ -1207,6 +1191,11 @@ static void gt64120_realize(DeviceState *dev, Error **errp) get_system_io(), PCI_DEVFN(18, 0), TYPE_PCI_BUS); + memory_region_init_io(&phb->conf_mem, OBJECT(phb), &pci_host_conf_le_ops, + s, "pci-conf-idx", 4); + memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGADDR << 2, + &phb->conf_mem, 1); + pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci"); /*
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(-)