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

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

[Hu Tao]

Hi,

This is v3 of PCI clenaup series. See each patch for the detail.

Regards,
Hu

changes:

v3:
  - rebase on top of 7fb8da2b886, all 5 patches applied cleanly.
  - new patch: pci: introduce PCI_DEVFN_AUTO

v2:
  - remove patch 3 from v1 which is incorrect.
  - rename defined macros as per Marcel's suggestion
  - place macros in pci_host.h as per Marcel's suggestion
  - new patch 'pci: reorganize QEMU_PCI_CAP_*'

Hu Tao (6):
  pci: reorganize QEMU_PCI_CAP_*
  pci: introduce pci_host_config_enabled()
  pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and
    PCI_HOST_BRIDGE_CONFIG_DATA.
  pci: remove the limit parameter of pci_host_config_read_common
  pci: remove the limit parameter of pci_host_config_write_common
  pci: introduce PCI_DEVFN_AUTO

 hw/core/qdev-properties.c |  1 +
 hw/mips/gt64xxx_pci.c     |  4 ++--
 hw/pci-host/piix.c        |  8 ++++----
 hw/pci-host/prep.c        |  6 ++++--
 hw/pci-host/q35.c         |  8 ++++----
 hw/pci/pci.c              |  5 ++---
 hw/pci/pci_host.c         | 33 ++++++++++++++++++++++++---------
 hw/pci/pcie_host.c        | 18 ++----------------
 hw/ppc/spapr_pci.c        |  6 ++----
 include/hw/pci-host/q35.h |  3 ---
 include/hw/pci/pci.h      | 41 ++++++++++++++++++++++-------------------
 include/hw/pci/pci_host.h | 14 ++++++++++++--
 tests/libqos/pci-pc.c     | 25 +++++++++++++------------
 13 files changed, 92 insertions(+), 80 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/6] pci: reorganize QEMU_PCI_CAP_*

[Hu Tao]

This makes code more readable.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 include/hw/pci/pci.h | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c352c7b..b18759a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -142,25 +142,26 @@ enum {
 
 /* Bits in cap_present field. */
 enum {
-    QEMU_PCI_CAP_MSI = 0x1,
-    QEMU_PCI_CAP_MSIX = 0x2,
-    QEMU_PCI_CAP_EXPRESS = 0x4,
-
-    /* multifunction capable device */
-#define QEMU_PCI_CAP_MULTIFUNCTION_BITNR        3
-    QEMU_PCI_CAP_MULTIFUNCTION = (1 << QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
-
-    /* command register SERR bit enabled */
-#define QEMU_PCI_CAP_SERR_BITNR 4
-    QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
-    /* Standard hot plug controller. */
-#define QEMU_PCI_SHPC_BITNR 5
-    QEMU_PCI_CAP_SHPC = (1 << QEMU_PCI_SHPC_BITNR),
-#define QEMU_PCI_SLOTID_BITNR 6
-    QEMU_PCI_CAP_SLOTID = (1 << QEMU_PCI_SLOTID_BITNR),
-    /* PCI Express capability - Power Controller Present */
-#define QEMU_PCIE_SLTCAP_PCP_BITNR 7
-    QEMU_PCIE_SLTCAP_PCP = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
+    QEMU_PCI_CAP_MSI_BITNR = 0,
+    QEMU_PCI_CAP_MSIX_BITNR,
+    QEMU_PCI_CAP_EXPRESS_BITNR,
+    QEMU_PCI_CAP_MULTIFUNCTION_BITNR, /* multifunction capable device */
+    QEMU_PCI_CAP_SERR_BITNR,          /* command register SERR bit enabled */
+    QEMU_PCI_SHPC_BITNR,              /* Standard hot plug controller */
+    QEMU_PCI_SLOTID_BITNR,
+    QEMU_PCIE_SLTCAP_PCP_BITNR, /* PCI Express capability - Power Controller
+                                   Present */
+};
+
+enum {
+    QEMU_PCI_CAP_MSI            = (1 << QEMU_PCI_CAP_MSI_BITNR),
+    QEMU_PCI_CAP_MSIX           = (1 << QEMU_PCI_CAP_MSIX_BITNR),
+    QEMU_PCI_CAP_EXPRESS        = (1 << QEMU_PCI_CAP_EXPRESS_BITNR),
+    QEMU_PCI_CAP_MULTIFUNCTION  = (1 << QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
+    QEMU_PCI_CAP_SERR           = (1 << QEMU_PCI_CAP_SERR_BITNR),
+    QEMU_PCI_CAP_SHPC           = (1 << QEMU_PCI_SHPC_BITNR),
+    QEMU_PCI_CAP_SLOTID         = (1 << QEMU_PCI_SLOTID_BITNR),
+    QEMU_PCIE_SLTCAP_PCP        = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
 };
 
 #define TYPE_PCI_DEVICE "pci-device"
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/6] pci: introduce pci_host_config_enabled()

[Hu Tao]

This makes code more readable.

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

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 1f2fe5f..f118c9c 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 (pci_host_config_enabled(phb)) {
             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 (!pci_host_config_enabled(phb)) {
             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..9bc47d8 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 (pci_host_config_enabled(s)) {
         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 (!pci_host_config_enabled(s)) {
         return 0xffffffff;
     }
     val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index ba31595..b48791d 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -65,6 +65,11 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
 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);
 
+static inline bool pci_host_config_enabled(struct PCIHostState *pci_host)
+{
+    return pci_host->config_reg & (1U << 31);
+}
+
 extern const MemoryRegionOps pci_host_conf_le_ops;
 extern const MemoryRegionOps pci_host_conf_be_ops;
 extern const MemoryRegionOps pci_host_data_le_ops;
-- 
1.9.3

[Qemu-devel] [PATCH v3 3/6] pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA.

[Hu Tao]

PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA are
defined in PCI specification, so move them to common place.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci-host/piix.c        |  8 ++++----
 hw/pci-host/prep.c        |  6 ++++--
 hw/pci-host/q35.c         |  8 ++++----
 include/hw/pci-host/q35.h |  3 ---
 include/hw/pci/pci_host.h |  5 +++++
 tests/libqos/pci-pc.c     | 25 +++++++++++++------------
 6 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 1530038..76f3757 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, PCI_HOST_BRIDGE_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
-    sysbus_init_ioports(sbd, 0xcfc, 4);
+    sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, 4);
 }
 
 static int i440fx_initfn(PCIDevice *dev)
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 1de3681..2ae21ad 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -228,11 +228,13 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
                           "pci-conf-idx", 4);
-    memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
+    memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_ADDR,
+                                &h->conf_mem);
 
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
                           "pci-conf-data", 4);
-    memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
+    memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_DATA,
+                                &h->data_mem);
 
     memory_region_init_io(&h->mmcfg, OBJECT(s), &raven_pci_io_ops, s,
                           "pciio", 0x00400000);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b20bad8..666afea 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, PCI_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
+    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_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, PCI_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
+    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_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_host.h b/include/hw/pci/pci_host.h
index b48791d..2bae45a 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -30,6 +30,11 @@
 
 #include "hw/sysbus.h"
 
+/* PCI configuration */
+
+#define PCI_HOST_BRIDGE_CONFIG_ADDR      0xcf8
+#define PCI_HOST_BRIDGE_CONFIG_DATA      0xcfc
+
 #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
 #define PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index 6dba0db..e4c3c11 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -14,6 +14,7 @@
 #include "libqos/pci-pc.h"
 
 #include "hw/pci/pci_regs.h"
+#include "hw/pci/pci_host.h"
 
 #include "qemu-common.h"
 #include "qemu/host-utils.h"
@@ -113,38 +114,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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inb(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inw(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inl(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outb(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outw(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outl(PCI_HOST_BRIDGE_CONFIG_DATA, value);
 }
 
 static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)
-- 
1.9.3

[Qemu-devel] [PATCH v3 4/6] 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 9bc47d8..2b11551 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 21b95b3..59c6608 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 2bae45a..72a1b8b 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -65,7 +65,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 v3 5/6] 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 2b11551..4a59b0e 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 59c6608..8c566dd 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 72a1b8b..67e007f 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -63,7 +63,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

[Qemu-devel] [PATCH v3 6/6] pci: introduce PCI_DEVFN_AUTO

[Hu Tao]

Introduce PCI_DEVFN_AUTO rather than using -1 in code.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/core/qdev-properties.c | 1 +
 hw/pci/pci.c              | 5 ++---
 include/hw/pci/pci.h      | 2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 2e47f70..df4ad14 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -6,6 +6,7 @@
 #include "net/hub.h"
 #include "qapi/visitor.h"
 #include "sysemu/char.h"
+#include "hw/pci/pci.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 371699c..73c7dec 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -50,7 +50,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
 static void pcibus_reset(BusState *qbus);
 
 static Property pci_props[] = {
-    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
+    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, PCI_DEVFN_AUTO),
     DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
     DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
     DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
@@ -801,7 +801,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
     address_space_destroy(&pci_dev->bus_master_as);
 }
 
-/* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn)
 {
@@ -810,7 +809,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     PCIConfigWriteFunc *config_write = pc->config_write;
     AddressSpace *dma_as;
 
-    if (devfn < 0) {
+    if (devfn == PCI_DEVFN_AUTO) {
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
             devfn += PCI_FUNC_MAX) {
             if (!bus->devices[devfn])
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b18759a..9206b12 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -164,6 +164,8 @@ enum {
     QEMU_PCIE_SLTCAP_PCP        = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
 };
 
+#define PCI_DEVFN_AUTO -1
+
 #define TYPE_PCI_DEVICE "pci-device"
 #define PCI_DEVICE(obj) \
      OBJECT_CHECK(PCIDevice, (obj), TYPE_PCI_DEVICE)
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 6/6] pci: introduce PCI_DEVFN_AUTO

[Hu Tao]

On Thu, Dec 11, 2014 at 10:20:28AM +0800, Hu Tao wrote:
> Introduce PCI_DEVFN_AUTO rather than using -1 in code.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/core/qdev-properties.c | 1 +
>  hw/pci/pci.c              | 5 ++---
>  include/hw/pci/pci.h      | 2 ++
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2e47f70..df4ad14 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -6,6 +6,7 @@
>  #include "net/hub.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/char.h"
> +#include "hw/pci/pci.h"

Oops, this one shouldn't be here, will resend.

Regards,
Hu

[Qemu-devel] [PATCH v3 RESEND 6/6] pci: introduce PCI_DEVFN_AUTO

[Hu Tao]

Introduce PCI_DEVFN_AUTO rather than using -1 in code.

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 371699c..73c7dec 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -50,7 +50,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
 static void pcibus_reset(BusState *qbus);
 
 static Property pci_props[] = {
-    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
+    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, PCI_DEVFN_AUTO),
     DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
     DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
     DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
@@ -801,7 +801,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
     address_space_destroy(&pci_dev->bus_master_as);
 }
 
-/* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn)
 {
@@ -810,7 +809,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     PCIConfigWriteFunc *config_write = pc->config_write;
     AddressSpace *dma_as;
 
-    if (devfn < 0) {
+    if (devfn == PCI_DEVFN_AUTO) {
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
             devfn += PCI_FUNC_MAX) {
             if (!bus->devices[devfn])
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b18759a..9206b12 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -164,6 +164,8 @@ enum {
     QEMU_PCIE_SLTCAP_PCP        = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
 };
 
+#define PCI_DEVFN_AUTO -1
+
 #define TYPE_PCI_DEVICE "pci-device"
 #define PCI_DEVICE(obj) \
      OBJECT_CHECK(PCIDevice, (obj), TYPE_PCI_DEVICE)
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 0/6] Some PCI related cleanup patches

[Hu Tao]

ping...

On Thu, Dec 11, 2014 at 10:20:22AM +0800, Hu Tao wrote:
> Hi,
> 
> This is v3 of PCI clenaup series. See each patch for the detail.
> 
> Regards,
> Hu
> 
> changes:
> 
> v3:
>   - rebase on top of 7fb8da2b886, all 5 patches applied cleanly.
>   - new patch: pci: introduce PCI_DEVFN_AUTO
> 
> v2:
>   - remove patch 3 from v1 which is incorrect.
>   - rename defined macros as per Marcel's suggestion
>   - place macros in pci_host.h as per Marcel's suggestion
>   - new patch 'pci: reorganize QEMU_PCI_CAP_*'
> 
> Hu Tao (6):
>   pci: reorganize QEMU_PCI_CAP_*
>   pci: introduce pci_host_config_enabled()
>   pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and
>     PCI_HOST_BRIDGE_CONFIG_DATA.
>   pci: remove the limit parameter of pci_host_config_read_common
>   pci: remove the limit parameter of pci_host_config_write_common
>   pci: introduce PCI_DEVFN_AUTO
> 
>  hw/core/qdev-properties.c |  1 +
>  hw/mips/gt64xxx_pci.c     |  4 ++--
>  hw/pci-host/piix.c        |  8 ++++----
>  hw/pci-host/prep.c        |  6 ++++--
>  hw/pci-host/q35.c         |  8 ++++----
>  hw/pci/pci.c              |  5 ++---
>  hw/pci/pci_host.c         | 33 ++++++++++++++++++++++++---------
>  hw/pci/pcie_host.c        | 18 ++----------------
>  hw/ppc/spapr_pci.c        |  6 ++----
>  include/hw/pci-host/q35.h |  3 ---
>  include/hw/pci/pci.h      | 41 ++++++++++++++++++++++-------------------
>  include/hw/pci/pci_host.h | 14 ++++++++++++--
>  tests/libqos/pci-pc.c     | 25 +++++++++++++------------
>  13 files changed, 92 insertions(+), 80 deletions(-)
> 
> -- 
> 1.9.3
> 
> 

Re: [Qemu-devel] [PATCH v3 1/6] pci: reorganize QEMU_PCI_CAP_*

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:23AM +0800, Hu Tao wrote:
> This makes code more readable.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  include/hw/pci/pci.h | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index c352c7b..b18759a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -142,25 +142,26 @@ enum {
>  
>  /* Bits in cap_present field. */
>  enum {
> -    QEMU_PCI_CAP_MSI = 0x1,
> -    QEMU_PCI_CAP_MSIX = 0x2,
> -    QEMU_PCI_CAP_EXPRESS = 0x4,
> -
> -    /* multifunction capable device */
> -#define QEMU_PCI_CAP_MULTIFUNCTION_BITNR        3
> -    QEMU_PCI_CAP_MULTIFUNCTION = (1 << QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
> -
> -    /* command register SERR bit enabled */
> -#define QEMU_PCI_CAP_SERR_BITNR 4
> -    QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
> -    /* Standard hot plug controller. */
> -#define QEMU_PCI_SHPC_BITNR 5
> -    QEMU_PCI_CAP_SHPC = (1 << QEMU_PCI_SHPC_BITNR),
> -#define QEMU_PCI_SLOTID_BITNR 6
> -    QEMU_PCI_CAP_SLOTID = (1 << QEMU_PCI_SLOTID_BITNR),
> -    /* PCI Express capability - Power Controller Present */
> -#define QEMU_PCIE_SLTCAP_PCP_BITNR 7
> -    QEMU_PCIE_SLTCAP_PCP = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
> +    QEMU_PCI_CAP_MSI_BITNR = 0,
> +    QEMU_PCI_CAP_MSIX_BITNR,
> +    QEMU_PCI_CAP_EXPRESS_BITNR,
> +    QEMU_PCI_CAP_MULTIFUNCTION_BITNR, /* multifunction capable device */
> +    QEMU_PCI_CAP_SERR_BITNR,          /* command register SERR bit enabled */
> +    QEMU_PCI_SHPC_BITNR,              /* Standard hot plug controller */
> +    QEMU_PCI_SLOTID_BITNR,
> +    QEMU_PCIE_SLTCAP_PCP_BITNR, /* PCIE Slot - Power Controller
>+                                    Present */

Pls shorten so it fits on one line: /* PCIE Slot - Power Controller Present */

> +};
> +
> +enum {
> +    QEMU_PCI_CAP_MSI            = (1 << QEMU_PCI_CAP_MSI_BITNR),
> +    QEMU_PCI_CAP_MSIX           = (1 << QEMU_PCI_CAP_MSIX_BITNR),
> +    QEMU_PCI_CAP_EXPRESS        = (1 << QEMU_PCI_CAP_EXPRESS_BITNR),
> +    QEMU_PCI_CAP_MULTIFUNCTION  = (1 << QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
> +    QEMU_PCI_CAP_SERR           = (1 << QEMU_PCI_CAP_SERR_BITNR),
> +    QEMU_PCI_CAP_SHPC           = (1 << QEMU_PCI_SHPC_BITNR),
> +    QEMU_PCI_CAP_SLOTID         = (1 << QEMU_PCI_SLOTID_BITNR),
> +    QEMU_PCIE_SLTCAP_PCP        = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
>  };
>  
>  #define TYPE_PCI_DEVICE "pci-device"
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 2/6] pci: introduce pci_host_config_enabled()

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:24AM +0800, Hu Tao wrote:
> This makes code more readable.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/mips/gt64xxx_pci.c     | 4 ++--
>  hw/pci/pci_host.c         | 5 +++--
>  include/hw/pci/pci_host.h | 5 +++++
>  3 files changed, 10 insertions(+), 4 deletions(-)

We have a ton of other places hard-coding 1<<31,
why special-case these?

> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 1f2fe5f..f118c9c 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 (pci_host_config_enabled(phb)) {
>              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 (!pci_host_config_enabled(phb)) {
>              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..9bc47d8 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 (pci_host_config_enabled(s)) {
>          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 (!pci_host_config_enabled(s)) {
>          return 0xffffffff;
>      }
>      val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index ba31595..b48791d 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -65,6 +65,11 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
>  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);
>  
> +static inline bool pci_host_config_enabled(struct PCIHostState *pci_host)
> +{
> +    return pci_host->config_reg & (1U << 31);
> +}
> +

Better:

#define PCI_HOST_CONFIG_ENABLE (1U << 31)

then everyone can just do s->config_reg & PCI_HOST_CONFIG_ENABLE

better as it'll work for code like this:
	phb->config_reg = (pciaddr) | (1u << 31);


>  extern const MemoryRegionOps pci_host_conf_le_ops;
>  extern const MemoryRegionOps pci_host_conf_be_ops;
>  extern const MemoryRegionOps pci_host_data_le_ops;
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 3/6] pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA.

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:25AM +0800, Hu Tao wrote:
> PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA are
> defined in PCI specification, so move them to common place.

they are listed in the spec, but they are still PC specific.
Spec says:

	Two DWORD I/O locations are used to generate configuration transactions
---->	for PC-AT compatible systems.
	The first DWORD location (CF8h) references a read/write register
	that is named CONFIG_ADDRESS. The second DWORD address (CFCh) references a
	read/write register named CONFIG_DATA.


So this should at least have PC_ included in name, and probably go into
some pc - specific header.


> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/pci-host/piix.c        |  8 ++++----
>  hw/pci-host/prep.c        |  6 ++++--
>  hw/pci-host/q35.c         |  8 ++++----
>  include/hw/pci-host/q35.h |  3 ---
>  include/hw/pci/pci_host.h |  5 +++++
>  tests/libqos/pci-pc.c     | 25 +++++++++++++------------
>  6 files changed, 30 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 1530038..76f3757 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, PCI_HOST_BRIDGE_CONFIG_ADDR, &s->conf_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, 4);
>  
> -    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
> -    sysbus_init_ioports(sbd, 0xcfc, 4);
> +    sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, &s->data_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, 4);
>  }
>  
>  static int i440fx_initfn(PCIDevice *dev)
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 1de3681..2ae21ad 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -228,11 +228,13 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>  
>      memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
>                            "pci-conf-idx", 4);
> -    memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
> +    memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_ADDR,
> +                                &h->conf_mem);
>  
>      memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
>                            "pci-conf-data", 4);
> -    memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
> +    memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_DATA,
> +                                &h->data_mem);
>  
>      memory_region_init_io(&h->mmcfg, OBJECT(s), &raven_pci_io_ops, s,
>                            "pciio", 0x00400000);
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b20bad8..666afea 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, PCI_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_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, PCI_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_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_host.h b/include/hw/pci/pci_host.h
> index b48791d..2bae45a 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -30,6 +30,11 @@
>  
>  #include "hw/sysbus.h"
>  
> +/* PCI configuration */
> +
> +#define PCI_HOST_BRIDGE_CONFIG_ADDR      0xcf8
> +#define PCI_HOST_BRIDGE_CONFIG_DATA      0xcfc
> +
>  #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
>  #define PCI_HOST_BRIDGE(obj) \
>      OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 6dba0db..e4c3c11 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -14,6 +14,7 @@
>  #include "libqos/pci-pc.h"
>  
>  #include "hw/pci/pci_regs.h"
> +#include "hw/pci/pci_host.h"
>  
>  #include "qemu-common.h"
>  #include "qemu/host-utils.h"
> @@ -113,38 +114,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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inb(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inw(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inl(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outb(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outw(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outl(PCI_HOST_BRIDGE_CONFIG_DATA, value);
>  }
>  
>  static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)
> -- 
> 1.9.3

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

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:26AM +0800, Hu Tao wrote:
> 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>

Not true e.g. for pci_data_read, is it?
Need a bit more comments to explain why it's
correct there.

> ---
>  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 9bc47d8..2b11551 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 21b95b3..59c6608 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 2bae45a..72a1b8b 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -65,7 +65,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

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

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:27AM +0800, Hu Tao wrote:
> 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.

Same comment.

> 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 2b11551..4a59b0e 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 59c6608..8c566dd 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 72a1b8b..67e007f 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -63,7 +63,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 v3 6/6] pci: introduce PCI_DEVFN_AUTO

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:28AM +0800, Hu Tao wrote:
> Introduce PCI_DEVFN_AUTO rather than using -1 in code.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/core/qdev-properties.c | 1 +
>  hw/pci/pci.c              | 5 ++---
>  include/hw/pci/pci.h      | 2 ++
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2e47f70..df4ad14 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -6,6 +6,7 @@
>  #include "net/hub.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/char.h"
> +#include "hw/pci/pci.h"
>  
>  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>                                    Error **errp)
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 371699c..73c7dec 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -50,7 +50,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static void pcibus_reset(BusState *qbus);
>  
>  static Property pci_props[] = {
> -    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> +    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, PCI_DEVFN_AUTO),
>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
>      DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> @@ -801,7 +801,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
>      address_space_destroy(&pci_dev->bus_master_as);
>  }
>  
> -/* -1 for devfn means auto assign */
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                           const char *name, int devfn)
>  {
> @@ -810,7 +809,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      PCIConfigWriteFunc *config_write = pc->config_write;
>      AddressSpace *dma_as;
>  
> -    if (devfn < 0) {
> +    if (devfn == PCI_DEVFN_AUTO) {

How do we know we found all places that do this?
How do we know parameter was already validate for
other values < 0?
Can you pls mention in the commit log why you think
it's safe?


>          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>              devfn += PCI_FUNC_MAX) {
>              if (!bus->devices[devfn])
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b18759a..9206b12 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -164,6 +164,8 @@ enum {
>      QEMU_PCIE_SLTCAP_PCP        = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
>  };
>  
> +#define PCI_DEVFN_AUTO -1
> +
>  #define TYPE_PCI_DEVICE "pci-device"
>  #define PCI_DEVICE(obj) \
>       OBJECT_CHECK(PCIDevice, (obj), TYPE_PCI_DEVICE)
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 0/6] Some PCI related cleanup patches

[Michael S. Tsirkin]

On Wed, Jan 21, 2015 at 02:41:33PM +0800, Hu Tao wrote:
> ping...

At some point you said "will resend".

> On Thu, Dec 11, 2014 at 10:20:22AM +0800, Hu Tao wrote:
> > Hi,
> > 
> > This is v3 of PCI clenaup series. See each patch for the detail.
> > 
> > Regards,
> > Hu
> > 
> > changes:
> > 
> > v3:
> >   - rebase on top of 7fb8da2b886, all 5 patches applied cleanly.
> >   - new patch: pci: introduce PCI_DEVFN_AUTO
> > 
> > v2:
> >   - remove patch 3 from v1 which is incorrect.
> >   - rename defined macros as per Marcel's suggestion
> >   - place macros in pci_host.h as per Marcel's suggestion
> >   - new patch 'pci: reorganize QEMU_PCI_CAP_*'
> > 
> > Hu Tao (6):
> >   pci: reorganize QEMU_PCI_CAP_*
> >   pci: introduce pci_host_config_enabled()
> >   pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and
> >     PCI_HOST_BRIDGE_CONFIG_DATA.
> >   pci: remove the limit parameter of pci_host_config_read_common
> >   pci: remove the limit parameter of pci_host_config_write_common
> >   pci: introduce PCI_DEVFN_AUTO
> > 
> >  hw/core/qdev-properties.c |  1 +
> >  hw/mips/gt64xxx_pci.c     |  4 ++--
> >  hw/pci-host/piix.c        |  8 ++++----
> >  hw/pci-host/prep.c        |  6 ++++--
> >  hw/pci-host/q35.c         |  8 ++++----
> >  hw/pci/pci.c              |  5 ++---
> >  hw/pci/pci_host.c         | 33 ++++++++++++++++++++++++---------
> >  hw/pci/pcie_host.c        | 18 ++----------------
> >  hw/ppc/spapr_pci.c        |  6 ++----
> >  include/hw/pci-host/q35.h |  3 ---
> >  include/hw/pci/pci.h      | 41 ++++++++++++++++++++++-------------------
> >  include/hw/pci/pci_host.h | 14 ++++++++++++--
> >  tests/libqos/pci-pc.c     | 25 +++++++++++++------------
> >  13 files changed, 92 insertions(+), 80 deletions(-)
> > 
> > -- 
> > 1.9.3
> > 
> > 

Re: [Qemu-devel] [PATCH v3 6/6] pci: introduce PCI_DEVFN_AUTO

[Hu Tao]

On Wed, Jan 21, 2015 at 02:00:02PM +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 11, 2014 at 10:20:28AM +0800, Hu Tao wrote:
> > Introduce PCI_DEVFN_AUTO rather than using -1 in code.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/core/qdev-properties.c | 1 +
> >  hw/pci/pci.c              | 5 ++---
> >  include/hw/pci/pci.h      | 2 ++
> >  3 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 2e47f70..df4ad14 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -6,6 +6,7 @@
> >  #include "net/hub.h"
> >  #include "qapi/visitor.h"
> >  #include "sysemu/char.h"
> > +#include "hw/pci/pci.h"
> >  
> >  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
> >                                    Error **errp)
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 371699c..73c7dec 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -50,7 +50,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
> >  static void pcibus_reset(BusState *qbus);
> >  
> >  static Property pci_props[] = {
> > -    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > +    DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, PCI_DEVFN_AUTO),
> >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> >      DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> >      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> > @@ -801,7 +801,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
> >      address_space_destroy(&pci_dev->bus_master_as);
> >  }
> >  
> > -/* -1 for devfn means auto assign */
> >  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >                                           const char *name, int devfn)
> >  {
> > @@ -810,7 +809,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >      PCIConfigWriteFunc *config_write = pc->config_write;
> >      AddressSpace *dma_as;
> >  
> > -    if (devfn < 0) {
> > +    if (devfn == PCI_DEVFN_AUTO) {
> 
> How do we know we found all places that do this?

Good question! Actually I found other places creating devfn=-1 pci
devices, by calling pci_create_simple_multifunction() and
pci_create_simple(). But I don't have a solid way to discover all these
places. So we should keep the original code here.

Please drop this patch.

Thanks,
Hu

> How do we know parameter was already validate for
> other values < 0?
> Can you pls mention in the commit log why you think
> it's safe?
> 
> 
> >          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >              devfn += PCI_FUNC_MAX) {
> >              if (!bus->devices[devfn])
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index b18759a..9206b12 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -164,6 +164,8 @@ enum {
> >      QEMU_PCIE_SLTCAP_PCP        = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
> >  };
> >  
> > +#define PCI_DEVFN_AUTO -1
> > +
> >  #define TYPE_PCI_DEVICE "pci-device"
> >  #define PCI_DEVICE(obj) \
> >       OBJECT_CHECK(PCIDevice, (obj), TYPE_PCI_DEVICE)
> > -- 
> > 1.9.3

Re: [Qemu-devel] [PATCH v3 0/6] Some PCI related cleanup patches

[Hu Tao]

On Wed, Jan 21, 2015 at 02:00:58PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 21, 2015 at 02:41:33PM +0800, Hu Tao wrote:
> > ping...
> 
> At some point you said "will resend".

I meant resend patch 6.

Patches 1-5 can still apply cleanly.

Regards,
Hu

> 
> > On Thu, Dec 11, 2014 at 10:20:22AM +0800, Hu Tao wrote:
> > > Hi,
> > > 
> > > This is v3 of PCI clenaup series. See each patch for the detail.
> > > 
> > > Regards,
> > > Hu
> > > 
> > > changes:
> > > 
> > > v3:
> > >   - rebase on top of 7fb8da2b886, all 5 patches applied cleanly.
> > >   - new patch: pci: introduce PCI_DEVFN_AUTO
> > > 
> > > v2:
> > >   - remove patch 3 from v1 which is incorrect.
> > >   - rename defined macros as per Marcel's suggestion
> > >   - place macros in pci_host.h as per Marcel's suggestion
> > >   - new patch 'pci: reorganize QEMU_PCI_CAP_*'
> > > 
> > > Hu Tao (6):
> > >   pci: reorganize QEMU_PCI_CAP_*
> > >   pci: introduce pci_host_config_enabled()
> > >   pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and
> > >     PCI_HOST_BRIDGE_CONFIG_DATA.
> > >   pci: remove the limit parameter of pci_host_config_read_common
> > >   pci: remove the limit parameter of pci_host_config_write_common
> > >   pci: introduce PCI_DEVFN_AUTO
> > > 
> > >  hw/core/qdev-properties.c |  1 +
> > >  hw/mips/gt64xxx_pci.c     |  4 ++--
> > >  hw/pci-host/piix.c        |  8 ++++----
> > >  hw/pci-host/prep.c        |  6 ++++--
> > >  hw/pci-host/q35.c         |  8 ++++----
> > >  hw/pci/pci.c              |  5 ++---
> > >  hw/pci/pci_host.c         | 33 ++++++++++++++++++++++++---------
> > >  hw/pci/pcie_host.c        | 18 ++----------------
> > >  hw/ppc/spapr_pci.c        |  6 ++----
> > >  include/hw/pci-host/q35.h |  3 ---
> > >  include/hw/pci/pci.h      | 41 ++++++++++++++++++++++-------------------
> > >  include/hw/pci/pci_host.h | 14 ++++++++++++--
> > >  tests/libqos/pci-pc.c     | 25 +++++++++++++------------
> > >  13 files changed, 92 insertions(+), 80 deletions(-)
> > > 
> > > -- 
> > > 1.9.3
> > > 
> > > 

Re: [Qemu-devel] [PATCH v3 3/6] pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA.

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:25AM +0800, Hu Tao wrote:
> PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA are
> defined in PCI specification, so move them to common place.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

If you read spec carefully you will see that it says
that these are in fact PC specific.
So the definitions don't belong in hw/pci/.

> ---
>  hw/pci-host/piix.c        |  8 ++++----
>  hw/pci-host/prep.c        |  6 ++++--
>  hw/pci-host/q35.c         |  8 ++++----
>  include/hw/pci-host/q35.h |  3 ---
>  include/hw/pci/pci_host.h |  5 +++++
>  tests/libqos/pci-pc.c     | 25 +++++++++++++------------
>  6 files changed, 30 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 1530038..76f3757 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, PCI_HOST_BRIDGE_CONFIG_ADDR, &s->conf_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, 4);
>  
> -    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
> -    sysbus_init_ioports(sbd, 0xcfc, 4);
> +    sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, &s->data_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, 4);
>  }
>  
>  static int i440fx_initfn(PCIDevice *dev)
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 1de3681..2ae21ad 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -228,11 +228,13 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>  
>      memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
>                            "pci-conf-idx", 4);
> -    memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
> +    memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_ADDR,
> +                                &h->conf_mem);
>  
>      memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
>                            "pci-conf-data", 4);
> -    memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
> +    memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_DATA,
> +                                &h->data_mem);
>  
>      memory_region_init_io(&h->mmcfg, OBJECT(s), &raven_pci_io_ops, s,
>                            "pciio", 0x00400000);
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b20bad8..666afea 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, PCI_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_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, PCI_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
> +    sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_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_host.h b/include/hw/pci/pci_host.h
> index b48791d..2bae45a 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -30,6 +30,11 @@
>  
>  #include "hw/sysbus.h"
>  
> +/* PCI configuration */
> +
> +#define PCI_HOST_BRIDGE_CONFIG_ADDR      0xcf8
> +#define PCI_HOST_BRIDGE_CONFIG_DATA      0xcfc
> +
>  #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
>  #define PCI_HOST_BRIDGE(obj) \
>      OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 6dba0db..e4c3c11 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -14,6 +14,7 @@
>  #include "libqos/pci-pc.h"
>  
>  #include "hw/pci/pci_regs.h"
> +#include "hw/pci/pci_host.h"
>  
>  #include "qemu-common.h"
>  #include "qemu/host-utils.h"
> @@ -113,38 +114,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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inb(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inw(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    return inl(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outb(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outw(PCI_HOST_BRIDGE_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(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
> +    outl(PCI_HOST_BRIDGE_CONFIG_DATA, value);
>  }
>  
>  static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)
> -- 
> 1.9.3

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

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:26AM +0800, Hu Tao wrote:
> Since the limit parameter is always set to the size of pci device's
> configuration space,

That's not true in case of pci_data_read, is it?

While the patch might be correct, a better analysis
why that's so seems called for.

> 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 9bc47d8..2b11551 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 21b95b3..59c6608 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 2bae45a..72a1b8b 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -65,7 +65,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