[PATCH v2 0/7] hw/pci-host: Re-use generic pci_host_data_le_ops MemoryRegionOps

[PATCH v2 0/7] hw/pci-host: Re-use generic pci_host_data_le_ops MemoryRegionOps

[Philippe Mathieu-Daudé]

Add the 'config-reg-check-high-bit' property in order to re-use
the generic pci_host_data_le_ops MemoryRegionOps.

Philippe Mathieu-Daudé (7):
  hw/pci/pci_host: Add 'config-reg-check-high-bit' property
  hw/pci-host/astro: Re-use generic pci_host_data_le_ops MemoryRegionOps
  hw/pci-host/dino: Re-use generic pci_host_data_le_ops MemoryRegionOps
  hw/pci-host/sabre: Remove pointless OBJECT() cast
  hw/pci-host/sabre: Include 'host' in host bridge method names
  hw/pci-host/sabre: Re-use generic pci_host_data_le_ops MemoryRegionOps
  hw/pci-host/typhoon: Re-use generic pci_host_data_le_ops
    MemoryRegionOps

 hw/alpha/alpha_sys.h      |  1 -
 include/hw/pci/pci_host.h |  1 +
 hw/alpha/pci.c            | 26 -------------
 hw/alpha/typhoon.c        |  4 +-
 hw/pci-host/astro.c       | 35 ++++-------------
 hw/pci-host/dino.c        | 30 ++++-----------
 hw/pci-host/sabre.c       | 80 +++++++++++++--------------------------
 hw/pci/pci_host.c         | 10 +++--
 hw/pci-host/trace-events  |  4 --
 9 files changed, 52 insertions(+), 139 deletions(-)

-- 
2.51.0

[PATCH v2 1/7] hw/pci/pci_host: Add 'config-reg-check-high-bit' property

[Philippe Mathieu-Daudé]

In order to have more PCI host bridges to re-use the
generic pci_host_data_le_ops MemoryRegionOps, add the
'config-reg-check-high-bit' property (%true by default).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci/pci_host.h |  1 +
 hw/pci/pci_host.c         | 10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 954dd446fa4..c04a567ec57 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -43,6 +43,7 @@ struct PCIHostState {
     MemoryRegion data_mem;
     MemoryRegion mmcfg;
     uint32_t config_reg;
+    bool config_reg_check_high_bit;
     bool mig_enabled;
     PCIBus *bus;
     bool bypass_iommu;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index b5c624e12e8..d6db365e327 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -184,8 +184,10 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
 {
     PCIHostState *s = opaque;
 
-    if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+    if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) {
+        return;
+    }
+    pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
 }
 
 static uint64_t pci_host_data_read(void *opaque,
@@ -193,7 +195,7 @@ static uint64_t pci_host_data_read(void *opaque,
 {
     PCIHostState *s = opaque;
 
-    if (!(s->config_reg & (1U << 31))) {
+    if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) {
         return 0xffffffff;
     }
     return pci_data_read(s->bus, s->config_reg | (addr & 3), len);
@@ -235,6 +237,8 @@ const VMStateDescription vmstate_pcihost = {
 };
 
 static const Property pci_host_properties_common[] = {
+    DEFINE_PROP_BOOL("config-reg-check-high-bit", PCIHostState,
+                     config_reg_check_high_bit, true),
     DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
                      mig_enabled, true),
     DEFINE_PROP_BOOL(PCI_HOST_BYPASS_IOMMU, PCIHostState, bypass_iommu, false),
-- 
2.51.0

[PATCH v2 2/7] hw/pci-host/astro: Re-use generic pci_host_data_le_ops MemoryRegionOps

[Philippe Mathieu-Daudé]

Avoid duplicating code, clear the "config-reg-check-high-bit"
property in .instance_init() in order to re-use the generic
pci_host_data_le_ops MemoryRegionOps.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/astro.c      | 35 ++++++++---------------------------
 hw/pci-host/trace-events |  2 --
 2 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/hw/pci-host/astro.c b/hw/pci-host/astro.c
index 0bd66ab3de3..72e625c2508 100644
--- a/hw/pci-host/astro.c
+++ b/hw/pci-host/astro.c
@@ -230,32 +230,6 @@ static const MemoryRegionOps elroy_chip_ops = {
 };
 
 
-/* Unlike pci_config_data_le_ops, no check of high bit set in config_reg.  */
-
-static uint64_t elroy_config_data_read(void *opaque, hwaddr addr, unsigned len)
-{
-    uint64_t val;
-
-    PCIHostState *s = opaque;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
-    trace_elroy_pci_config_data_read(s->config_reg | (addr & 3), len, val);
-    return val;
-}
-
-static void elroy_config_data_write(void *opaque, hwaddr addr,
-                                   uint64_t val, unsigned len)
-{
-    PCIHostState *s = opaque;
-    pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
-    trace_elroy_pci_config_data_write(s->config_reg | (addr & 3), len, val);
-}
-
-static const MemoryRegionOps elroy_config_data_ops = {
-    .read = elroy_config_data_read,
-    .write = elroy_config_data_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static uint64_t elroy_config_addr_read(void *opaque, hwaddr addr, unsigned len)
 {
     ElroyState *s = opaque;
@@ -424,6 +398,12 @@ static void elroy_reset(DeviceState *dev)
     }
 }
 
+static void elroy_pcihost_instance_init(Object *obj)
+{
+    object_property_set_bool(obj, "config-reg-check-high-bit", false,
+                             &error_fatal);
+}
+
 static void elroy_pcihost_realize(DeviceState *dev, Error **errp)
 {
     ElroyState *s = ELROY_PCI_HOST_BRIDGE(dev);
@@ -440,7 +420,7 @@ static void elroy_pcihost_realize(DeviceState *dev, Error **errp)
                           &elroy_config_addr_ops, dev,
                           "pci-conf-idx", 8);
     memory_region_init_io(&phb->data_mem, obj,
-                          &elroy_config_data_ops, dev,
+                          &pci_host_data_le_ops, phb,
                           "pci-conf-data", 8);
     memory_region_add_subregion(&s->this_mem, 0x40,
                                 &phb->conf_mem);
@@ -497,6 +477,7 @@ static const TypeInfo elroy_pcihost_info = {
     .name          = TYPE_ELROY_PCI_HOST_BRIDGE,
     .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(ElroyState),
+    .instance_init = elroy_pcihost_instance_init,
     .class_init    = elroy_pcihost_class_init,
 };
 
diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
index a6fd88c2c46..792ab25729b 100644
--- a/hw/pci-host/trace-events
+++ b/hw/pci-host/trace-events
@@ -76,7 +76,5 @@ astro_chip_read(uint64_t addr, int size, uint64_t val) "addr 0x%"PRIx64" size %d
 astro_chip_write(uint64_t addr, int size, uint64_t val) "addr 0x%"PRIx64" size %d val 0x%"PRIx64
 elroy_read(uint64_t addr, int size, uint64_t val) "addr 0x%"PRIx64" size %d val 0x%"PRIx64
 elroy_write(uint64_t addr, int size, uint64_t val) "addr 0x%"PRIx64" size %d val 0x%"PRIx64
-elroy_pci_config_data_read(uint64_t addr, int size, uint64_t val) "addr 0x%"PRIx64" size %d val 0x%"PRIx64
-elroy_pci_config_data_write(uint64_t addr, int size, uint64_t val) "addr 0x%"PRIx64" size %d val 0x%"PRIx64
 iosapic_reg_write(uint64_t reg_select, int size, uint64_t val) "reg_select 0x%"PRIx64" size %d val 0x%"PRIx64
 iosapic_reg_read(uint64_t reg_select, int size, uint64_t val) "reg_select 0x%"PRIx64" size %d val 0x%"PRIx64
-- 
2.51.0

[PATCH v2 3/7] hw/pci-host/dino: Re-use generic pci_host_data_le_ops MemoryRegionOps

[Philippe Mathieu-Daudé]

Avoid duplicating code, clear the "config-reg-check-high-bit"
property in .instance_init() in order to re-use the generic
pci_host_data_le_ops MemoryRegionOps.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/dino.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/hw/pci-host/dino.c b/hw/pci-host/dino.c
index 924053499c1..31ffe8fa60d 100644
--- a/hw/pci-host/dino.c
+++ b/hw/pci-host/dino.c
@@ -302,27 +302,6 @@ static const VMStateDescription vmstate_dino = {
     }
 };
 
-/* Unlike pci_config_data_le_ops, no check of high bit set in config_reg.  */
-
-static uint64_t dino_config_data_read(void *opaque, hwaddr addr, unsigned len)
-{
-    PCIHostState *s = opaque;
-    return pci_data_read(s->bus, s->config_reg | (addr & 3), len);
-}
-
-static void dino_config_data_write(void *opaque, hwaddr addr,
-                                   uint64_t val, unsigned len)
-{
-    PCIHostState *s = opaque;
-    pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
-}
-
-static const MemoryRegionOps dino_config_data_ops = {
-    .read = dino_config_data_read,
-    .write = dino_config_data_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static uint64_t dino_config_addr_read(void *opaque, hwaddr addr, unsigned len)
 {
     DinoState *s = opaque;
@@ -410,6 +389,12 @@ static void dino_pcihost_reset(DeviceState *dev)
     s->toc_addr = 0xFFFA0030; /* IO_COMMAND of CPU */
 }
 
+static void dino_pcihost_instance_init(Object *obj)
+{
+    object_property_set_bool(obj, "config-reg-check-high-bit", false,
+                             &error_fatal);
+}
+
 static void dino_pcihost_realize(DeviceState *dev, Error **errp)
 {
     DinoState *s = DINO_PCI_HOST_BRIDGE(dev);
@@ -424,7 +409,7 @@ static void dino_pcihost_realize(DeviceState *dev, Error **errp)
                           &dino_config_addr_ops, DEVICE(s),
                           "pci-conf-idx", 4);
     memory_region_init_io(&phb->data_mem, OBJECT(phb),
-                          &dino_config_data_ops, DEVICE(s),
+                          &pci_host_data_le_ops, phb,
                           "pci-conf-data", 4);
     memory_region_add_subregion(&s->this_mem, DINO_PCI_CONFIG_ADDR,
                                 &phb->conf_mem);
@@ -505,6 +490,7 @@ static const TypeInfo dino_pcihost_info = {
     .name          = TYPE_DINO_PCI_HOST_BRIDGE,
     .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(DinoState),
+    .instance_init = dino_pcihost_instance_init,
     .class_init    = dino_pcihost_class_init,
 };
 
-- 
2.51.0

[PATCH v2 4/7] hw/pci-host/sabre: Remove pointless OBJECT() cast

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/sabre.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index f95e5db583a..f63d832efc0 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -429,18 +429,18 @@ static void sabre_init(Object *obj)
                              0);
 
     /* sabre_config */
-    memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
+    memory_region_init_io(&s->sabre_config, obj, &sabre_config_ops, s,
                           "sabre-config", 0x10000);
     /* at region 0 */
     sysbus_init_mmio(sbd, &s->sabre_config);
 
-    memory_region_init_io(&s->pci_config, OBJECT(s), &pci_config_ops, s,
+    memory_region_init_io(&s->pci_config, obj, &pci_config_ops, s,
                           "sabre-pci-config", 0x1000000);
     /* at region 1 */
     sysbus_init_mmio(sbd, &s->pci_config);
 
     /* pci_ioport */
-    memory_region_init(&s->pci_ioport, OBJECT(s), "sabre-pci-ioport",
+    memory_region_init(&s->pci_ioport, obj, "sabre-pci-ioport",
                        0x1000000);
 
     /* at region 2 */
-- 
2.51.0

[PATCH v2 5/7] hw/pci-host/sabre: Include 'host' in host bridge method names

[Philippe Mathieu-Daudé]

Rename various methods to help distinguish between
sabre_host* (for host bridge block) and sabre_pci*
(for the PCI function).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/sabre.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index f63d832efc0..669191b6c7e 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -116,8 +116,8 @@ static const PCIIOMMUOps sabre_iommu_ops = {
     .get_address_space = sabre_pci_dma_iommu,
 };
 
-static void sabre_config_write(void *opaque, hwaddr addr,
-                               uint64_t val, unsigned size)
+static void sabre_host_config_write(void *opaque, hwaddr addr,
+                                    uint64_t val, unsigned size)
 {
     SabreState *s = opaque;
 
@@ -194,8 +194,8 @@ static void sabre_config_write(void *opaque, hwaddr addr,
     }
 }
 
-static uint64_t sabre_config_read(void *opaque,
-                                  hwaddr addr, unsigned size)
+static uint64_t sabre_host_config_read(void *opaque,
+                                       hwaddr addr, unsigned size)
 {
     SabreState *s = opaque;
     uint32_t val = 0;
@@ -240,9 +240,9 @@ static uint64_t sabre_config_read(void *opaque,
     return val;
 }
 
-static const MemoryRegionOps sabre_config_ops = {
-    .read = sabre_config_read,
-    .write = sabre_config_write,
+static const MemoryRegionOps sabre_host_config_ops = {
+    .read = sabre_host_config_read,
+    .write = sabre_host_config_write,
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
@@ -329,7 +329,7 @@ static void pci_sabre_set_irq(void *opaque, int irq_num, int level)
     }
 }
 
-static void sabre_reset(DeviceState *d)
+static void sabre_host_reset(DeviceState *d)
 {
     SabreState *s = SABRE(d);
     PCIDevice *pci_dev;
@@ -367,7 +367,7 @@ static const MemoryRegionOps pci_config_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static void sabre_realize(DeviceState *dev, Error **errp)
+static void sabre_host_realize(DeviceState *dev, Error **errp)
 {
     SabreState *s = SABRE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
@@ -402,7 +402,7 @@ static void sabre_realize(DeviceState *dev, Error **errp)
     pci_realize_and_unref(pci_dev, phb->bus, &error_fatal);
 }
 
-static void sabre_init(Object *obj)
+static void sabre_host_instance_init(Object *obj)
 {
     SabreState *s = SABRE(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
@@ -429,7 +429,7 @@ static void sabre_init(Object *obj)
                              0);
 
     /* sabre_config */
-    memory_region_init_io(&s->sabre_config, obj, &sabre_config_ops, s,
+    memory_region_init_io(&s->sabre_config, obj, &sabre_host_config_ops, s,
                           "sabre-config", 0x10000);
     /* at region 0 */
     sysbus_init_mmio(sbd, &s->sabre_config);
@@ -483,7 +483,7 @@ static const TypeInfo sabre_pci_info = {
     },
 };
 
-static char *sabre_ofw_unit_address(const SysBusDevice *dev)
+static char *sabre_host_ofw_unit_address(const SysBusDevice *dev)
 {
     SabreState *s = SABRE(dev);
 
@@ -492,34 +492,34 @@ static char *sabre_ofw_unit_address(const SysBusDevice *dev)
                (uint32_t)(s->special_base & 0xffffffff));
 }
 
-static const Property sabre_properties[] = {
+static const Property sabre_host_properties[] = {
     DEFINE_PROP_UINT64("special-base", SabreState, special_base, 0),
     DEFINE_PROP_UINT64("mem-base", SabreState, mem_base, 0),
 };
 
-static void sabre_class_init(ObjectClass *klass, const void *data)
+static void sabre_host_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
 
-    dc->realize = sabre_realize;
-    device_class_set_legacy_reset(dc, sabre_reset);
-    device_class_set_props(dc, sabre_properties);
+    dc->realize = sabre_host_realize;
+    device_class_set_legacy_reset(dc, sabre_host_reset);
+    device_class_set_props(dc, sabre_host_properties);
     dc->fw_name = "pci";
-    sbc->explicit_ofw_unit_address = sabre_ofw_unit_address;
+    sbc->explicit_ofw_unit_address = sabre_host_ofw_unit_address;
 }
 
-static const TypeInfo sabre_info = {
+static const TypeInfo sabre_host_info = {
     .name          = TYPE_SABRE,
     .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(SabreState),
-    .instance_init = sabre_init,
-    .class_init    = sabre_class_init,
+    .instance_init = sabre_host_instance_init,
+    .class_init    = sabre_host_class_init,
 };
 
 static void sabre_register_types(void)
 {
-    type_register_static(&sabre_info);
+    type_register_static(&sabre_host_info);
     type_register_static(&sabre_pci_info);
 }
 
-- 
2.51.0

[PATCH v2 6/7] hw/pci-host/sabre: Re-use generic pci_host_data_le_ops MemoryRegionOps

[Philippe Mathieu-Daudé]

Avoid duplicating code, re-use the generic generic
pci_host_data_le_ops MemoryRegionOps.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/sabre.c      | 34 +++-------------------------------
 hw/pci-host/trace-events |  2 --
 2 files changed, 3 insertions(+), 33 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 669191b6c7e..eb3dbe3361f 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -246,28 +246,6 @@ static const MemoryRegionOps sabre_host_config_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void sabre_pci_config_write(void *opaque, hwaddr addr,
-                                   uint64_t val, unsigned size)
-{
-    SabreState *s = opaque;
-    PCIHostState *phb = PCI_HOST_BRIDGE(s);
-
-    trace_sabre_pci_config_write(addr, val);
-    pci_data_write(phb->bus, addr, val, size);
-}
-
-static uint64_t sabre_pci_config_read(void *opaque, hwaddr addr,
-                                      unsigned size)
-{
-    uint32_t ret;
-    SabreState *s = opaque;
-    PCIHostState *phb = PCI_HOST_BRIDGE(s);
-
-    ret = pci_data_read(phb->bus, addr, size);
-    trace_sabre_pci_config_read(addr, ret);
-    return ret;
-}
-
 /* The sabre host has an IRQ line for each IRQ line of each slot.  */
 static int pci_sabre_map_irq(PCIDevice *pci_dev, int irq_num)
 {
@@ -361,12 +339,6 @@ static void sabre_host_reset(DeviceState *d)
     pci_bridge_update_mappings(PCI_BRIDGE(pci_dev));
 }
 
-static const MemoryRegionOps pci_config_ops = {
-    .read = sabre_pci_config_read,
-    .write = sabre_pci_config_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static void sabre_host_realize(DeviceState *dev, Error **errp)
 {
     SabreState *s = SABRE(dev);
@@ -430,12 +402,12 @@ static void sabre_host_instance_init(Object *obj)
 
     /* sabre_config */
     memory_region_init_io(&s->sabre_config, obj, &sabre_host_config_ops, s,
-                          "sabre-config", 0x10000);
+                          "pci-conf-idx", 0x10000);
     /* at region 0 */
     sysbus_init_mmio(sbd, &s->sabre_config);
 
-    memory_region_init_io(&s->pci_config, obj, &pci_config_ops, s,
-                          "sabre-pci-config", 0x1000000);
+    memory_region_init_io(&s->pci_config, obj, &pci_host_data_le_ops, s,
+                          "pci-data-idx", 0x1000000);
     /* at region 1 */
     sysbus_init_mmio(sbd, &s->pci_config);
 
diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
index 792ab25729b..20c3cae47a2 100644
--- a/hw/pci-host/trace-events
+++ b/hw/pci-host/trace-events
@@ -35,8 +35,6 @@ sabre_set_request(int irq_num) "request irq %d"
 sabre_clear_request(int irq_num) "clear request irq %d"
 sabre_config_write(uint64_t addr, uint64_t val) "addr 0x%"PRIx64" val 0x%"PRIx64
 sabre_config_read(uint64_t addr, uint64_t val) "addr 0x%"PRIx64" val 0x%"PRIx64
-sabre_pci_config_write(uint64_t addr, uint64_t val) "addr 0x%"PRIx64" val 0x%"PRIx64
-sabre_pci_config_read(uint64_t addr, uint64_t val) "addr 0x%"PRIx64" val 0x%"PRIx64
 sabre_pci_set_irq(int irq_num, int level) "set irq_in %d level %d"
 sabre_pci_set_obio_irq(int irq_num, int level) "set irq %d level %d"
 
-- 
2.51.0

[PATCH v2 7/7] hw/pci-host/typhoon: Re-use generic pci_host_data_le_ops MemoryRegionOps

[Philippe Mathieu-Daudé]

Avoid duplicating code, re-use the generic generic
pci_host_data_le_ops MemoryRegionOps.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/alpha/alpha_sys.h |  1 -
 hw/alpha/pci.c       | 26 --------------------------
 hw/alpha/typhoon.c   |  4 ++--
 3 files changed, 2 insertions(+), 29 deletions(-)

diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h
index a303c584383..c193f0a9b56 100644
--- a/hw/alpha/alpha_sys.h
+++ b/hw/alpha/alpha_sys.h
@@ -14,7 +14,6 @@ PCIBus *typhoon_init(MemoryRegion *, qemu_irq *, qemu_irq *, AlphaCPU *[4],
 
 /* alpha_pci.c.  */
 extern const MemoryRegionOps alpha_pci_ignore_ops;
-extern const MemoryRegionOps alpha_pci_conf1_ops;
 extern const MemoryRegionOps alpha_pci_iack_ops;
 
 #endif
diff --git a/hw/alpha/pci.c b/hw/alpha/pci.c
index 7c18297177b..d44cee570bf 100644
--- a/hw/alpha/pci.c
+++ b/hw/alpha/pci.c
@@ -38,32 +38,6 @@ const MemoryRegionOps alpha_pci_ignore_ops = {
     },
 };
 
-
-/* PCI config space reads/writes, to byte-word addressable memory.  */
-static uint64_t bw_conf1_read(void *opaque, hwaddr addr,
-                              unsigned size)
-{
-    PCIBus *b = opaque;
-    return pci_data_read(b, addr, size);
-}
-
-static void bw_conf1_write(void *opaque, hwaddr addr,
-                           uint64_t val, unsigned size)
-{
-    PCIBus *b = opaque;
-    pci_data_write(b, addr, val, size);
-}
-
-const MemoryRegionOps alpha_pci_conf1_ops = {
-    .read = bw_conf1_read,
-    .write = bw_conf1_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 4,
-    },
-};
-
 /* PCI/EISA Interrupt Acknowledge Cycle.  */
 
 static uint64_t iack_read(void *opaque, hwaddr addr, unsigned size)
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 4c56f981d71..f5a9d6e6ed4 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -911,8 +911,8 @@ PCIBus *typhoon_init(MemoryRegion *ram, qemu_irq *p_isa_irq,
                                 &s->pchip.reg_iack);
 
     /* Pchip0 PCI configuration, 0x801.FE00.0000, 16MB.  */
-    memory_region_init_io(&s->pchip.reg_conf, OBJECT(s), &alpha_pci_conf1_ops,
-                          b, "pci0-conf", 16 * MiB);
+    memory_region_init_io(&s->pchip.reg_conf, OBJECT(s), &pci_host_data_le_ops,
+                          phb, "pci0-data-idx", 16 * MiB);
     memory_region_add_subregion(addr_space, 0x801fe000000ULL,
                                 &s->pchip.reg_conf);
 
-- 
2.51.0

Re: [PATCH v2 7/7] hw/pci-host/typhoon: Re-use generic pci_host_data_le_ops MemoryRegionOps

[Philippe Mathieu-Daudé]

On 27/10/25 17:53, Philippe Mathieu-Daudé wrote:
> Avoid duplicating code, re-use the generic generic
> pci_host_data_le_ops MemoryRegionOps.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/alpha/alpha_sys.h |  1 -
>   hw/alpha/pci.c       | 26 --------------------------
>   hw/alpha/typhoon.c   |  4 ++--
>   3 files changed, 2 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h
> index a303c584383..c193f0a9b56 100644
> --- a/hw/alpha/alpha_sys.h
> +++ b/hw/alpha/alpha_sys.h
> @@ -14,7 +14,6 @@ PCIBus *typhoon_init(MemoryRegion *, qemu_irq *, qemu_irq *, AlphaCPU *[4],
>   
>   /* alpha_pci.c.  */
>   extern const MemoryRegionOps alpha_pci_ignore_ops;
> -extern const MemoryRegionOps alpha_pci_conf1_ops;
>   extern const MemoryRegionOps alpha_pci_iack_ops;
>   
>   #endif
> diff --git a/hw/alpha/pci.c b/hw/alpha/pci.c
> index 7c18297177b..d44cee570bf 100644
> --- a/hw/alpha/pci.c
> +++ b/hw/alpha/pci.c
> @@ -38,32 +38,6 @@ const MemoryRegionOps alpha_pci_ignore_ops = {
>       },
>   };
>   
> -
> -/* PCI config space reads/writes, to byte-word addressable memory.  */
> -static uint64_t bw_conf1_read(void *opaque, hwaddr addr,
> -                              unsigned size)
> -{
> -    PCIBus *b = opaque;
> -    return pci_data_read(b, addr, size);
> -}
> -
> -static void bw_conf1_write(void *opaque, hwaddr addr,
> -                           uint64_t val, unsigned size)
> -{
> -    PCIBus *b = opaque;
> -    pci_data_write(b, addr, val, size);

Hmm, likely incorrect, better ignore.

> -}

Re: [PATCH v2 6/7] hw/pci-host/sabre: Re-use generic pci_host_data_le_ops MemoryRegionOps

[Philippe Mathieu-Daudé]

On 27/10/25 17:53, Philippe Mathieu-Daudé wrote:
> Avoid duplicating code, re-use the generic generic
> pci_host_data_le_ops MemoryRegionOps.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/sabre.c      | 34 +++-------------------------------
>   hw/pci-host/trace-events |  2 --
>   2 files changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index 669191b6c7e..eb3dbe3361f 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -246,28 +246,6 @@ static const MemoryRegionOps sabre_host_config_ops = {
>       .endianness = DEVICE_BIG_ENDIAN,
>   };
>   
> -static void sabre_pci_config_write(void *opaque, hwaddr addr,
> -                                   uint64_t val, unsigned size)
> -{
> -    SabreState *s = opaque;
> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> -
> -    trace_sabre_pci_config_write(addr, val);
> -    pci_data_write(phb->bus, addr, val, size);
> -}
> -
> -static uint64_t sabre_pci_config_read(void *opaque, hwaddr addr,
> -                                      unsigned size)
> -{
> -    uint32_t ret;
> -    SabreState *s = opaque;
> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> -
> -    ret = pci_data_read(phb->bus, addr, size);

Likely incorrect, please ignore.

> -    trace_sabre_pci_config_read(addr, ret);
> -    return ret;
> -}

Re: [PATCH v2 1/7] hw/pci/pci_host: Add 'config-reg-check-high-bit' property

[Peter Maydell]

On Mon, 27 Oct 2025 at 16:54, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> In order to have more PCI host bridges to re-use the
> generic pci_host_data_le_ops MemoryRegionOps, add the
> 'config-reg-check-high-bit' property (%true by default).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index b5c624e12e8..d6db365e327 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -184,8 +184,10 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
>  {
>      PCIHostState *s = opaque;
>
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
> +    if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) {
> +        return;
> +    }
> +    pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
>  }
>
>  static uint64_t pci_host_data_read(void *opaque,
> @@ -193,7 +195,7 @@ static uint64_t pci_host_data_read(void *opaque,
>  {
>      PCIHostState *s = opaque;
>
> -    if (!(s->config_reg & (1U << 31))) {
> +    if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) {
>          return 0xffffffff;
>      }
>      return pci_data_read(s->bus, s->config_reg | (addr & 3), len);
> @@ -235,6 +237,8 @@ const VMStateDescription vmstate_pcihost = {
>  };
>
>  static const Property pci_host_properties_common[] = {
> +    DEFINE_PROP_BOOL("config-reg-check-high-bit", PCIHostState,
> +                     config_reg_check_high_bit, true),

I think it might be useful to name and document this
property at a slightly higher level of abstraction.

Specifically, this code is handling the behaviour of
the CONFIG_ADDRESS register which is part of the PCI
Configuration Access Method (CAM). For x86 the top bit of
CONFIG_ADDRESS is an Enable bit, which must be set to
cause accesses to CONFIG_DATA to actually do something.
For PCI controllers like Dino there is no Enable bit
defined in CONFIG_ADDRESS[*] and CONFIG_DATA accesses always
take effect.

[*] http://ftp.parisc-linux.org/docs/chips/dino_ers.pdf page 49

So perhaps we could call this "config-address-reg-has-enable-bit" ?

A documentation comment about its purpose and noting that
the expectation is that this is set by the subclass,
not by end-users, might also be helpful.

thanks
-- PMM

Re: [PATCH v2 1/7] hw/pci/pci_host: Add 'config-reg-check-high-bit' property

[Marc-André Lureau]

Hi

On Mon, Oct 27, 2025 at 9:31 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 27 Oct 2025 at 16:54, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > In order to have more PCI host bridges to re-use the
> > generic pci_host_data_le_ops MemoryRegionOps, add the
> > 'config-reg-check-high-bit' property (%true by default).
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index b5c624e12e8..d6db365e327 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -184,8 +184,10 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
> >  {
> >      PCIHostState *s = opaque;
> >
> > -    if (s->config_reg & (1u << 31))
> > -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
> > +    if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) {
> > +        return;
> > +    }
> > +    pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
> >  }
> >
> >  static uint64_t pci_host_data_read(void *opaque,
> > @@ -193,7 +195,7 @@ static uint64_t pci_host_data_read(void *opaque,
> >  {
> >      PCIHostState *s = opaque;
> >
> > -    if (!(s->config_reg & (1U << 31))) {
> > +    if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) {
> >          return 0xffffffff;
> >      }
> >      return pci_data_read(s->bus, s->config_reg | (addr & 3), len);
> > @@ -235,6 +237,8 @@ const VMStateDescription vmstate_pcihost = {
> >  };
> >
> >  static const Property pci_host_properties_common[] = {
> > +    DEFINE_PROP_BOOL("config-reg-check-high-bit", PCIHostState,
> > +                     config_reg_check_high_bit, true),
>
> I think it might be useful to name and document this
> property at a slightly higher level of abstraction.
>
> Specifically, this code is handling the behaviour of
> the CONFIG_ADDRESS register which is part of the PCI
> Configuration Access Method (CAM). For x86 the top bit of
> CONFIG_ADDRESS is an Enable bit, which must be set to
> cause accesses to CONFIG_DATA to actually do something.
> For PCI controllers like Dino there is no Enable bit
> defined in CONFIG_ADDRESS[*] and CONFIG_DATA accesses always
> take effect.
>
> [*] http://ftp.parisc-linux.org/docs/chips/dino_ers.pdf page 49
>
> So perhaps we could call this "config-address-reg-has-enable-bit" ?
>
> A documentation comment about its purpose and noting that
> the expectation is that this is set by the subclass,
> not by end-users, might also be helpful.

If this shouldn't be tweaked by users and is specific to a device
kind, could it be a bool on the PCIHostBridgeClass ?

-- 
Marc-André Lureau