[Qemu-devel] [PATCH 0/5] Some PCI related cleanup patches

[Qemu-devel] [PATCH 0/5] Some PCI related cleanup patches

[Hu Tao]

Hi,

This series includes 5 PCI clenaup patches. See each patch for the
detail.

Hu Tao (5):
  pci: introduce PC_PCI_CONFIG_ENABLED()
  pc: define PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA
  pci: move initialization of pci's conf_addr and conf_data to common
    place
  pci: remove the limit parameter of pci_host_config_read_common
  pci: remove the limit parameter of pci_host_config_write_common

 hw/mips/gt64xxx_pci.c     |  4 +--
 hw/pci-host/piix.c        | 20 ---------------
 hw/pci-host/q35.c         | 15 +++--------
 hw/pci/pci_host.c         | 65 ++++++++++++++++++++++++++++++++++++++++-------
 hw/pci/pcie_host.c        | 18 ++-----------
 hw/ppc/spapr_pci.c        |  6 ++---
 include/hw/pci-host/q35.h |  3 ---
 include/hw/pci/pci.h      |  7 +++++
 include/hw/pci/pci_host.h |  4 +--
 tests/libqos/pci-pc.c     | 24 ++++++++---------
 10 files changed, 87 insertions(+), 79 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/5] pci: introduce PC_PCI_CONFIG_ENABLED()

[Hu Tao]

This makes code more readable.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/mips/gt64xxx_pci.c | 4 ++--
 hw/pci/pci_host.c     | 5 +++--
 include/hw/pci/pci.h  | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 1f2fe5f..a49dbd7 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr,
         if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
             val = bswap32(val);
         }
-        if (phb->config_reg & (1u << 31)) {
+        if (PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
             pci_data_write(phb->bus, phb->config_reg, val, 4);
         }
         break;
@@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
         val = phb->config_reg;
         break;
     case GT_PCI0_CFGDATA:
-        if (!(phb->config_reg & (1 << 31))) {
+        if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
             val = 0xffffffff;
         } else {
             val = pci_data_read(phb->bus, phb->config_reg, 4);
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..f2a69ea 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
     PCIHostState *s = opaque;
     PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n",
                 addr, len, (unsigned)val);
-    if (s->config_reg & (1u << 31))
+    if (PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+    }
 }
 
 static uint64_t pci_host_data_read(void *opaque,
@@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
 {
     PCIHostState *s = opaque;
     uint32_t val;
-    if (!(s->config_reg & (1U << 31))) {
+    if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         return 0xffffffff;
     }
     val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c352c7b..3d42d7f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -13,6 +13,8 @@
 
 #include "hw/pci/pcie.h"
 
+#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
+
 /* PCI bus */
 
 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
-- 
1.9.3

[Qemu-devel] [PATCH 2/5] pc: define PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA

[Hu Tao]

PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA are defined in PCI
specification, so move them to common place.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/pci-host/piix.c        |  8 ++++----
 hw/pci-host/q35.c         |  8 ++++----
 include/hw/pci-host/q35.h |  3 ---
 include/hw/pci/pci.h      |  5 +++++
 tests/libqos/pci-pc.c     | 24 ++++++++++++------------
 5 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 1530038..eb92bde 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -288,11 +288,11 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
     PCIHostState *s = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    sysbus_add_io(sbd, 0xcf8, &s->conf_mem);
-    sysbus_init_ioports(sbd, 0xcf8, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
-    sysbus_init_ioports(sbd, 0xcfc, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 }
 
 static int i440fx_initfn(PCIDevice *dev)
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b20bad8..9e66835 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -41,11 +41,11 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     Q35PCIHost *s = Q35_HOST_DEVICE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
-    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &pci->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
-    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &pci->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 
     pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
                            s->mch.pci_address_space, s->mch.address_space_io,
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 025d6e6..3a026b0 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -82,9 +82,6 @@ typedef struct Q35PCIHost {
 /* PCI configuration */
 #define MCH_HOST_BRIDGE                        "MCH"
 
-#define MCH_HOST_BRIDGE_CONFIG_ADDR            0xcf8
-#define MCH_HOST_BRIDGE_CONFIG_DATA            0xcfc
-
 /* D0:F0 configuration space */
 #define MCH_HOST_BRIDGE_REVISION_DEFAULT       0x0
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 3d42d7f..e42589a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -13,6 +13,11 @@
 
 #include "hw/pci/pcie.h"
 
+/* PCI configuration */
+
+#define PC_PCI_CONFIG_ADDR      0xcf8
+#define PC_PCI_CONFIG_DATA      0xcfc
+
 #define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
 
 /* PCI bus */
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index 6dba0db..2762608 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -113,38 +113,38 @@ static void qpci_pc_io_writel(QPCIBus *bus, void *addr, uint32_t value)
 
 static uint8_t qpci_pc_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inb(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inb(PC_PCI_CONFIG_DATA);
 }
 
 static uint16_t qpci_pc_config_readw(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inw(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inw(PC_PCI_CONFIG_DATA);
 }
 
 static uint32_t qpci_pc_config_readl(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inl(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inl(PC_PCI_CONFIG_DATA);
 }
 
 static void qpci_pc_config_writeb(QPCIBus *bus, int devfn, uint8_t offset, uint8_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outb(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outb(PC_PCI_CONFIG_DATA, value);
 }
 
 static void qpci_pc_config_writew(QPCIBus *bus, int devfn, uint8_t offset, uint16_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outw(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outw(PC_PCI_CONFIG_DATA, value);
 }
 
 static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint32_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outl(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outl(PC_PCI_CONFIG_DATA, value);
 }
 
 static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)
-- 
1.9.3

[Qemu-devel] [PATCH 3/5] pci: move initialization of pci's conf_addr and conf_data to common place

[Hu Tao]

So that standard pci host device can share them.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/pci-host/piix.c | 20 --------------------
 hw/pci-host/q35.c  |  7 -------
 hw/pci/pci_host.c  | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index eb92bde..683465c 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -256,14 +256,8 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 
 static void i440fx_pcihost_initfn(Object *obj)
 {
-    PCIHostState *s = PCI_HOST_BRIDGE(obj);
     I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
 
-    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
-                          "pci-conf-data", 4);
-
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
                         i440fx_pcihost_get_pci_hole_start,
                         NULL, NULL, NULL, NULL);
@@ -283,18 +277,6 @@ static void i440fx_pcihost_initfn(Object *obj)
     d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
-static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
-{
-    PCIHostState *s = PCI_HOST_BRIDGE(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
-}
-
 static int i440fx_initfn(PCIDevice *dev)
 {
     PCII440FXState *d = I440FX_PCI_DEVICE(dev);
@@ -755,8 +737,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
 
     hc->root_bus_path = i440fx_pcihost_root_bus_path;
-    dc->realize = i440fx_pcihost_realize;
-    dc->fw_name = "pci";
     dc->props = i440fx_props;
 }
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 9e66835..81eddd7 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -138,18 +138,11 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
     dc->realize = q35_host_realize;
     dc->props = mch_props;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->fw_name = "pci";
 }
 
 static void q35_host_initfn(Object *obj)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
-    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
-
-    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
-                          "pci-conf-data", 4);
 
     object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
     object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index f2a69ea..406c747 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -176,12 +176,44 @@ const MemoryRegionOps pci_host_data_be_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void pci_host_initfn(Object *obj)
+{
+    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
+
+    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
+                          "pci-conf-idx", 4);
+    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
+                          "pci-conf-data", 4);
+}
+
+static void pci_host_realize(DeviceState *dev, Error **errp)
+{
+    PCIHostState *s = PCI_HOST_BRIDGE(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
+}
+
+static void pci_host_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = pci_host_realize;
+    dc->fw_name = "pci";
+}
+
 static const TypeInfo pci_host_type_info = {
     .name = TYPE_PCI_HOST_BRIDGE,
     .parent = TYPE_SYS_BUS_DEVICE,
     .abstract = true,
     .class_size = sizeof(PCIHostBridgeClass),
+    .class_init = pci_host_class_init,
     .instance_size = sizeof(PCIHostState),
+    .instance_init = pci_host_initfn,
 };
 
 static void pci_host_register_types(void)
-- 
1.9.3

[Qemu-devel] [PATCH 4/5] pci: remove the limit parameter of pci_host_config_read_common

[Hu Tao]

Since the limit parameter is always set to the size of pci device's
configuration space, and we can determine the size from the type of pci
device.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/pci/pci_host.c         | 15 +++++++++++----
 hw/pci/pcie_host.c        |  9 +--------
 hw/ppc/spapr_pci.c        |  3 +--
 include/hw/pci/pci_host.h |  2 +-
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 406c747..937660c 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -58,12 +58,20 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
 }
 
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len)
+                                     uint32_t len)
 {
+    uint32_t limit = pci_config_size(pci_dev);
     uint32_t ret;
 
     assert(len <= 4);
-    ret = pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
+
+    if (limit <= addr) {
+        /* conventional pci device can be behind pcie-to-pci bridge.
+           256 <= addr < 4K has no effects. */
+        ret = ~0x0;
+    } else {
+        ret = pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
+    }
     trace_pci_cfg_read(pci_dev->name, PCI_SLOT(pci_dev->devfn),
                        PCI_FUNC(pci_dev->devfn), addr, ret);
 
@@ -95,8 +103,7 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
         return ~0x0;
     }
 
-    val = pci_host_config_read_common(pci_dev, config_addr,
-                                      PCI_CONFIG_SPACE_SIZE, len);
+    val = pci_host_config_read_common(pci_dev, config_addr, len);
     PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
                 __func__, pci_dev->name, config_addr, val, len);
 
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index 3db038f..cf8587b 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -62,19 +62,12 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return ~0x0;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return ~0x0;
-    }
-    return pci_host_config_read_common(pci_dev, addr, limit, len);
+    return pci_host_config_read_common(pci_dev, addr, len);
 }
 
 static const MemoryRegionOps pcie_mmcfg_ops = {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ad0da7f..7f38117 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -105,8 +105,7 @@ static void finish_read_pci_config(sPAPREnvironment *spapr, uint64_t buid,
         return;
     }
 
-    val = pci_host_config_read_common(pci_dev, addr,
-                                      pci_config_size(pci_dev), size);
+    val = pci_host_config_read_common(pci_dev, addr, size);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     rtas_st(rets, 1, val);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index ba31595..4a79945 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -60,7 +60,7 @@ typedef struct PCIHostBridgeClass {
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
                                   uint32_t limit, uint32_t val, uint32_t len);
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len);
+                                     uint32_t len);
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
-- 
1.9.3

[Qemu-devel] [PATCH 5/5] pci: remove the limit parameter of pci_host_config_write_common

[Hu Tao]

Since the limit parameter is always set to the size of pci device's
configuration space, and we can determine the size from the type of pci
device.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/pci/pci_host.c         | 13 ++++++++++---
 hw/pci/pcie_host.c        |  9 +--------
 hw/ppc/spapr_pci.c        |  3 +--
 include/hw/pci/pci_host.h |  2 +-
 4 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 937660c..f525fe5 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -49,8 +49,16 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 }
 
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len)
+                                  uint32_t val, uint32_t len)
 {
+    uint32_t limit = pci_config_size(pci_dev);
+
+    if (limit <= addr) {
+        /* conventional pci device can be behind pcie-to-pci bridge.
+           256 <= addr < 4K has no effects. */
+        return;
+    }
+
     assert(len <= 4);
     trace_pci_cfg_write(pci_dev->name, PCI_SLOT(pci_dev->devfn),
                         PCI_FUNC(pci_dev->devfn), addr, val);
@@ -89,8 +97,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 
     PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n",
                 __func__, pci_dev->name, config_addr, val, len);
-    pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
-                                 val, len);
+    pci_host_config_write_common(pci_dev, config_addr, val, len);
 }
 
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index cf8587b..e3a2a80 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -39,19 +39,12 @@ static void pcie_mmcfg_data_write(void *opaque, hwaddr mmcfg_addr,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return;
-    }
-    pci_host_config_write_common(pci_dev, addr, limit, val, len);
+    pci_host_config_write_common(pci_dev, addr, val, len);
 }
 
 static uint64_t pcie_mmcfg_data_read(void *opaque,
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7f38117..f306d42 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -171,8 +171,7 @@ static void finish_write_pci_config(sPAPREnvironment *spapr, uint64_t buid,
         return;
     }
 
-    pci_host_config_write_common(pci_dev, addr, pci_config_size(pci_dev),
-                                 val, size);
+    pci_host_config_write_common(pci_dev, addr, val, size);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 4a79945..9364f08 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -58,7 +58,7 @@ typedef struct PCIHostBridgeClass {
 
 /* common internal helpers for PCI/PCIe hosts, cut off overflows */
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len);
+                                  uint32_t val, uint32_t len);
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
                                      uint32_t len);
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/5] pci: introduce PC_PCI_CONFIG_ENABLED()

[Marcel Apfelbaum]

Hi,

On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> This makes code more readable.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/mips/gt64xxx_pci.c | 4 ++--
>  hw/pci/pci_host.c     | 5 +++--
>  include/hw/pci/pci.h  | 2 ++
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 1f2fe5f..a49dbd7 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr,
>          if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
>              val = bswap32(val);
>          }
> -        if (phb->config_reg & (1u << 31)) {
> +        if (PC_PCI_CONFIG_ENABLED(phb->config_reg)) 
I really like readable MACROS instead of magic numbers,
however I have 3 suggestions:
1. PC_PCI_CONFIG_ENABLED is still not really clear, because
of the "PC" prefix that is too wide in my IMHO (maybe PCI_HOST_BRIDGE_CONFIG_ENABLED)
when this macro refers only to host bridges.  
2. Maybe go the "extra mile" and let the macro receive
a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge).
3. Maybe make it an inline function? Just wondering
 
>              pci_data_write(phb->bus, phb->config_reg, val, 4);
>          }
>          break;
> @@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
>          val = phb->config_reg;
>          break;
>      case GT_PCI0_CFGDATA:
> -        if (!(phb->config_reg & (1 << 31))) {
> +        if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
>              val = 0xffffffff;
>          } else {
>              val = pci_data_read(phb->bus, phb->config_reg, 4);
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..f2a69ea 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
>      PCIHostState *s = opaque;
>      PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n",
>                  addr, len, (unsigned)val);
> -    if (s->config_reg & (1u << 31))
> +    if (PC_PCI_CONFIG_ENABLED(s->config_reg)) {
>          pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
> +    }
>  }
>  
>  static uint64_t pci_host_data_read(void *opaque,
> @@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> -    if (!(s->config_reg & (1U << 31))) {
> +    if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) {
>          return 0xffffffff;
>      }
>      val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index c352c7b..3d42d7f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -13,6 +13,8 @@
>  
>  #include "hw/pci/pcie.h"
>  
> +#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
Again, maybe move this to "hw/pci/pci_host" since is specific to host bridges?

Thanks,
Marcel
> +
>  /* PCI bus */
>  
>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))

Re: [Qemu-devel] [PATCH 2/5] pc: define PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA

[Marcel Apfelbaum]

On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA are defined in PCI
> specification, so move them to common place.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/pci-host/piix.c        |  8 ++++----
>  hw/pci-host/q35.c         |  8 ++++----
>  include/hw/pci-host/q35.h |  3 ---
>  include/hw/pci/pci.h      |  5 +++++
>  tests/libqos/pci-pc.c     | 24 ++++++++++++------------
>  5 files changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 1530038..eb92bde 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -288,11 +288,11 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
>      PCIHostState *s = PCI_HOST_BRIDGE(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> -    sysbus_add_io(sbd, 0xcf8, &s->conf_mem);
> -    sysbus_init_ioports(sbd, 0xcf8, 4);
> +    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
>  
> -    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
> -    sysbus_init_ioports(sbd, 0xcfc, 4);
> +    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
>  }
>  
>  static int i440fx_initfn(PCIDevice *dev)
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b20bad8..9e66835 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -41,11 +41,11 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>      Q35PCIHost *s = Q35_HOST_DEVICE(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> -    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
> +    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &pci->conf_mem);
> +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
>  
> -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
> -    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
> +    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &pci->data_mem);
> +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
>  
>      pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
>                             s->mch.pci_address_space, s->mch.address_space_io,
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 025d6e6..3a026b0 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -82,9 +82,6 @@ typedef struct Q35PCIHost {
>  /* PCI configuration */
>  #define MCH_HOST_BRIDGE                        "MCH"
>  
> -#define MCH_HOST_BRIDGE_CONFIG_ADDR            0xcf8
> -#define MCH_HOST_BRIDGE_CONFIG_DATA            0xcfc
> -
>  /* D0:F0 configuration space */
>  #define MCH_HOST_BRIDGE_REVISION_DEFAULT       0x0
>  
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 3d42d7f..e42589a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -13,6 +13,11 @@
>  
>  #include "hw/pci/pcie.h"
>  
> +/* PCI configuration */
> +
> +#define PC_PCI_CONFIG_ADDR      0xcf8
> +#define PC_PCI_CONFIG_DATA      0xcfc
I would move the macros also to hw/pci/pci_host.h,
and only personal opinion, change them to
PCI_HOST_BRIDGE_...

Thanks,
Marcel

> +
>  #define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
>  
>  /* PCI bus */
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 6dba0db..2762608 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -113,38 +113,38 @@ static void qpci_pc_io_writel(QPCIBus *bus, void *addr, uint32_t value)
>  
>  static uint8_t qpci_pc_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
>  {
> -    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
> -    return inb(0xcfc);
> +    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inb(PC_PCI_CONFIG_DATA);
>  }
>  
>  static uint16_t qpci_pc_config_readw(QPCIBus *bus, int devfn, uint8_t offset)
>  {
> -    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
> -    return inw(0xcfc);
> +    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inw(PC_PCI_CONFIG_DATA);
>  }
>  
>  static uint32_t qpci_pc_config_readl(QPCIBus *bus, int devfn, uint8_t offset)
>  {
> -    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
> -    return inl(0xcfc);
> +    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inl(PC_PCI_CONFIG_DATA);
>  }
>  
>  static void qpci_pc_config_writeb(QPCIBus *bus, int devfn, uint8_t offset, uint8_t value)
>  {
> -    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
> -    outb(0xcfc, value);
> +    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outb(PC_PCI_CONFIG_DATA, value);
>  }
>  
>  static void qpci_pc_config_writew(QPCIBus *bus, int devfn, uint8_t offset, uint16_t value)
>  {
> -    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
> -    outw(0xcfc, value);
> +    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outw(PC_PCI_CONFIG_DATA, value);
>  }
>  
>  static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint32_t value)
>  {
> -    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
> -    outl(0xcfc, value);
> +    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outl(PC_PCI_CONFIG_DATA, value);
>  }
>  
>  static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)

Re: [Qemu-devel] [PATCH 3/5] pci: move initialization of pci's conf_addr and conf_data to common place

[Marcel Apfelbaum]

On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> So that standard pci host device can share them.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/pci-host/piix.c | 20 --------------------
>  hw/pci-host/q35.c  |  7 -------
>  hw/pci/pci_host.c  | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index eb92bde..683465c 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -256,14 +256,8 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>  
>  static void i440fx_pcihost_initfn(Object *obj)
>  {
> -    PCIHostState *s = PCI_HOST_BRIDGE(obj);
>      I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
>  
> -    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
> -                          "pci-conf-idx", 4);
> -    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
> -                          "pci-conf-data", 4);
> -
>      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
>                          i440fx_pcihost_get_pci_hole_start,
>                          NULL, NULL, NULL, NULL);
> @@ -283,18 +277,6 @@ static void i440fx_pcihost_initfn(Object *obj)
>      d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>  }
>  
> -static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
> -{
> -    PCIHostState *s = PCI_HOST_BRIDGE(dev);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -
> -    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> -    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> -
> -    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> -    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
> -}
> -
>  static int i440fx_initfn(PCIDevice *dev)
>  {
>      PCII440FXState *d = I440FX_PCI_DEVICE(dev);
> @@ -755,8 +737,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>  
>      hc->root_bus_path = i440fx_pcihost_root_bus_path;
> -    dc->realize = i440fx_pcihost_realize;
> -    dc->fw_name = "pci";
>      dc->props = i440fx_props;
>  }
>  
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 9e66835..81eddd7 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -138,18 +138,11 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
>      dc->realize = q35_host_realize;
>      dc->props = mch_props;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    dc->fw_name = "pci";
>  }
>  
>  static void q35_host_initfn(Object *obj)
>  {
>      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> -    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> -
> -    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
> -                          "pci-conf-idx", 4);
> -    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
> -                          "pci-conf-data", 4);
>  
>      object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
>      object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index f2a69ea..406c747 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -176,12 +176,44 @@ const MemoryRegionOps pci_host_data_be_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> +static void pci_host_initfn(Object *obj)
> +{
> +    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> +
> +    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
> +                          "pci-conf-idx", 4);
> +    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
> +                          "pci-conf-data", 4);
> +}
> +
> +static void pci_host_realize(DeviceState *dev, Error **errp)
> +{
> +    PCIHostState *s = PCI_HOST_BRIDGE(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> +
> +    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
Hi,

If I got this right, now we have this pci_host_realize run
for each object of classes deriving from TYPE_PCI_HOST_BRIDGE.
Please correct me if I am wrong.
The machines PC and Q35 share the same code, however
we have other host bridges deriving that I think they behave different:

hw/mips/gt64xxx_pci.c:1207:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-bridge/dec.c:150:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/apb.c:839:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/bonito.c:832:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/grackle.c:156:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/ppce500.c:440:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/prep.c:392:    .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:456:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:470:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:484:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:498:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/versatile.c:508:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci/pci_host.c:179:    .name = TYPE_PCI_HOST_BRIDGE,
hw/ppc/ppc4xx_pci.c:405:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/ppc/spapr_pci.c:801:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/sh4/sh_pci.c:193:    .parent        = TYPE_PCI_HOST_BRIDGE,

If I am right, putting this so "high" in the hierarchy
may not be the right solution.

Thanks,
Marcel

> +}
> +
> +static void pci_host_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = pci_host_realize;
> +    dc->fw_name = "pci";
> +}
> +
>  static const TypeInfo pci_host_type_info = {
>      .name = TYPE_PCI_HOST_BRIDGE,
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .abstract = true,
>      .class_size = sizeof(PCIHostBridgeClass),
> +    .class_init = pci_host_class_init,
>      .instance_size = sizeof(PCIHostState),
> +    .instance_init = pci_host_initfn,
>  };
>  
>  static void pci_host_register_types(void)

Re: [Qemu-devel] [PATCH 1/5] pci: introduce PC_PCI_CONFIG_ENABLED()

[Hu Tao]

On Tue, Nov 04, 2014 at 03:41:11PM +0200, Marcel Apfelbaum wrote:
> Hi,
> 
> On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> > This makes code more readable.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/mips/gt64xxx_pci.c | 4 ++--
> >  hw/pci/pci_host.c     | 5 +++--
> >  include/hw/pci/pci.h  | 2 ++
> >  3 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> > index 1f2fe5f..a49dbd7 100644
> > --- a/hw/mips/gt64xxx_pci.c
> > +++ b/hw/mips/gt64xxx_pci.c
> > @@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr,
> >          if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
> >              val = bswap32(val);
> >          }
> > -        if (phb->config_reg & (1u << 31)) {
> > +        if (PC_PCI_CONFIG_ENABLED(phb->config_reg)) 
> I really like readable MACROS instead of magic numbers,
> however I have 3 suggestions:
> 1. PC_PCI_CONFIG_ENABLED is still not really clear, because
> of the "PC" prefix that is too wide in my IMHO (maybe PCI_HOST_BRIDGE_CONFIG_ENABLED)
> when this macro refers only to host bridges.  
> 2. Maybe go the "extra mile" and let the macro receive
> a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge).

Sounds reasonable. I changed it to an inline function and prefixed it
with pci_host_ to follow the convention in pci_host.c.

> 3. Maybe make it an inline function? Just wondering
>  
> >              pci_data_write(phb->bus, phb->config_reg, val, 4);
> >          }
> >          break;
> > @@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
> >          val = phb->config_reg;
> >          break;
> >      case GT_PCI0_CFGDATA:
> > -        if (!(phb->config_reg & (1 << 31))) {
> > +        if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
> >              val = 0xffffffff;
> >          } else {
> >              val = pci_data_read(phb->bus, phb->config_reg, 4);
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index 3e26f92..f2a69ea 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
> >      PCIHostState *s = opaque;
> >      PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n",
> >                  addr, len, (unsigned)val);
> > -    if (s->config_reg & (1u << 31))
> > +    if (PC_PCI_CONFIG_ENABLED(s->config_reg)) {
> >          pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
> > +    }
> >  }
> >  
> >  static uint64_t pci_host_data_read(void *opaque,
> > @@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
> >  {
> >      PCIHostState *s = opaque;
> >      uint32_t val;
> > -    if (!(s->config_reg & (1U << 31))) {
> > +    if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) {
> >          return 0xffffffff;
> >      }
> >      val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index c352c7b..3d42d7f 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -13,6 +13,8 @@
> >  
> >  #include "hw/pci/pcie.h"
> >  
> > +#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
> Again, maybe move this to "hw/pci/pci_host" since is specific to host bridges?

Done.

Thanks!

> 
> Thanks,
> Marcel
> > +
> >  /* PCI bus */
> >  
> >  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> 
> 
> 

Re: [Qemu-devel] [PATCH 3/5] pci: move initialization of pci's conf_addr and conf_data to common place

[Hu Tao]

On Tue, Nov 04, 2014 at 04:21:41PM +0200, Marcel Apfelbaum wrote:
> On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> > So that standard pci host device can share them.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/pci-host/piix.c | 20 --------------------
> >  hw/pci-host/q35.c  |  7 -------
> >  hw/pci/pci_host.c  | 32 ++++++++++++++++++++++++++++++++
> >  3 files changed, 32 insertions(+), 27 deletions(-)
> > 
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index eb92bde..683465c 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -256,14 +256,8 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
> >  
> >  static void i440fx_pcihost_initfn(Object *obj)
> >  {
> > -    PCIHostState *s = PCI_HOST_BRIDGE(obj);
> >      I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
> >  
> > -    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
> > -                          "pci-conf-idx", 4);
> > -    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
> > -                          "pci-conf-data", 4);
> > -
> >      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> >                          i440fx_pcihost_get_pci_hole_start,
> >                          NULL, NULL, NULL, NULL);
> > @@ -283,18 +277,6 @@ static void i440fx_pcihost_initfn(Object *obj)
> >      d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> >  }
> >  
> > -static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
> > -{
> > -    PCIHostState *s = PCI_HOST_BRIDGE(dev);
> > -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > -
> > -    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> > -    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> > -
> > -    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> > -    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
> > -}
> > -
> >  static int i440fx_initfn(PCIDevice *dev)
> >  {
> >      PCII440FXState *d = I440FX_PCI_DEVICE(dev);
> > @@ -755,8 +737,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> >      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> >  
> >      hc->root_bus_path = i440fx_pcihost_root_bus_path;
> > -    dc->realize = i440fx_pcihost_realize;
> > -    dc->fw_name = "pci";
> >      dc->props = i440fx_props;
> >  }
> >  
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 9e66835..81eddd7 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -138,18 +138,11 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
> >      dc->realize = q35_host_realize;
> >      dc->props = mch_props;
> >      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> > -    dc->fw_name = "pci";
> >  }
> >  
> >  static void q35_host_initfn(Object *obj)
> >  {
> >      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> > -    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> > -
> > -    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
> > -                          "pci-conf-idx", 4);
> > -    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
> > -                          "pci-conf-data", 4);
> >  
> >      object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
> >      object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index f2a69ea..406c747 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -176,12 +176,44 @@ const MemoryRegionOps pci_host_data_be_ops = {
> >      .endianness = DEVICE_BIG_ENDIAN,
> >  };
> >  
> > +static void pci_host_initfn(Object *obj)
> > +{
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> > +
> > +    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
> > +                          "pci-conf-idx", 4);
> > +    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
> > +                          "pci-conf-data", 4);
> > +}
> > +
> > +static void pci_host_realize(DeviceState *dev, Error **errp)
> > +{
> > +    PCIHostState *s = PCI_HOST_BRIDGE(dev);
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > +
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> > +
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
> Hi,
> 
> If I got this right, now we have this pci_host_realize run
> for each object of classes deriving from TYPE_PCI_HOST_BRIDGE.
> Please correct me if I am wrong.

You're absolutely right unless the subclass overides the realize
function.

> The machines PC and Q35 share the same code, however
> we have other host bridges deriving that I think they behave different:
> 
> hw/mips/gt64xxx_pci.c:1207:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-bridge/dec.c:150:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/apb.c:839:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/bonito.c:832:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/grackle.c:156:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/ppce500.c:440:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/prep.c:392:    .parent = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:456:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:470:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:484:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:498:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/versatile.c:508:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci/pci_host.c:179:    .name = TYPE_PCI_HOST_BRIDGE,
> hw/ppc/ppc4xx_pci.c:405:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/ppc/spapr_pci.c:801:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/sh4/sh_pci.c:193:    .parent        = TYPE_PCI_HOST_BRIDGE,
> 
> If I am right, putting this so "high" in the hierarchy
> may not be the right solution.

It is wrong indeed for those devices having different settings. So
please just ignore this patch.

Thanks for review!

> 
> Thanks,
> Marcel
> 
> > +}
> > +
> > +static void pci_host_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = pci_host_realize;
> > +    dc->fw_name = "pci";
> > +}
> > +
> >  static const TypeInfo pci_host_type_info = {
> >      .name = TYPE_PCI_HOST_BRIDGE,
> >      .parent = TYPE_SYS_BUS_DEVICE,
> >      .abstract = true,
> >      .class_size = sizeof(PCIHostBridgeClass),
> > +    .class_init = pci_host_class_init,
> >      .instance_size = sizeof(PCIHostState),
> > +    .instance_init = pci_host_initfn,
> >  };
> >  
> >  static void pci_host_register_types(void)
> 
> 
> 

Re: [Qemu-devel] [PATCH 2/5] pc: define PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA

[Hu Tao]

On Tue, Nov 04, 2014 at 03:44:35PM +0200, Marcel Apfelbaum wrote:
> On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> > PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA are defined in PCI
> > specification, so move them to common place.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/pci-host/piix.c        |  8 ++++----
> >  hw/pci-host/q35.c         |  8 ++++----
> >  include/hw/pci-host/q35.h |  3 ---
> >  include/hw/pci/pci.h      |  5 +++++
> >  tests/libqos/pci-pc.c     | 24 ++++++++++++------------
> >  5 files changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index 1530038..eb92bde 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -288,11 +288,11 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
> >      PCIHostState *s = PCI_HOST_BRIDGE(dev);
> >      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >  
> > -    sysbus_add_io(sbd, 0xcf8, &s->conf_mem);
> > -    sysbus_init_ioports(sbd, 0xcf8, 4);
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> >  
> > -    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
> > -    sysbus_init_ioports(sbd, 0xcfc, 4);
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
> >  }
> >  
> >  static int i440fx_initfn(PCIDevice *dev)
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index b20bad8..9e66835 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -41,11 +41,11 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
> >      Q35PCIHost *s = Q35_HOST_DEVICE(dev);
> >      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >  
> > -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> > -    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &pci->conf_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> >  
> > -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
> > -    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &pci->data_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
> >  
> >      pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
> >                             s->mch.pci_address_space, s->mch.address_space_io,
> > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> > index 025d6e6..3a026b0 100644
> > --- a/include/hw/pci-host/q35.h
> > +++ b/include/hw/pci-host/q35.h
> > @@ -82,9 +82,6 @@ typedef struct Q35PCIHost {
> >  /* PCI configuration */
> >  #define MCH_HOST_BRIDGE                        "MCH"
> >  
> > -#define MCH_HOST_BRIDGE_CONFIG_ADDR            0xcf8
> > -#define MCH_HOST_BRIDGE_CONFIG_DATA            0xcfc
> > -
> >  /* D0:F0 configuration space */
> >  #define MCH_HOST_BRIDGE_REVISION_DEFAULT       0x0
> >  
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 3d42d7f..e42589a 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -13,6 +13,11 @@
> >  
> >  #include "hw/pci/pcie.h"
> >  
> > +/* PCI configuration */
> > +
> > +#define PC_PCI_CONFIG_ADDR      0xcf8
> > +#define PC_PCI_CONFIG_DATA      0xcfc
> I would move the macros also to hw/pci/pci_host.h,
> and only personal opinion, change them to
> PCI_HOST_BRIDGE_...

Done.

Regards,
Hu