Skip to contentSkip to conversation

Some PCI related cleanup patches

PATCH In review Hu Tao 2 participants 21 messages Dec 11, 2014 – Jan 27, 2015

lore
Share & tools
lore

Hu Tao

author
#0 lore raw

v3 cover Some PCI related cleanup patches

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(-)
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH v3 1/6 pci: reorganize QEMU_PCI_CAP_*

This makes code more readable.

Signed-off-by Hu Tao <[email protected]>
Reviewed-by Marcel Apfelbaum <[email protected]>
---
 include/hw/pci/pci.h | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)
include/hw/pci/pci.h +20 -19
index c352c7b..b18759a 100644
@@ -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"
Signature · 1 line
1.9.3
#2 lore raw

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

Quoted 34 lines · Hu Tao

This makes code more readable.

Signed-off-by Hu Tao <[email protected]>
Reviewed-by Marcel Apfelbaum <[email protected]>
---
 include/hw/pci/pci.h | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)
include/hw/pci/pci.h +0 -18
index c352c7b..b18759a 100644
@@ -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

include/hw/pci/pci.h enum { · line 153

Quoted 10 lines
include/hw/pci/pci.h +9 -1
-    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 */

Quoted 16 lines
include/hw/pci/pci.h +11 -0
+};
+
+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"
Signature · 1 line
1.9.3
author
#3 lore raw

PATCH v3 2/6 pci: introduce pci_host_config_enabled()

In reply to Hu Tao

This makes code more readable.

Signed-off-by Hu Tao <[email protected]>
Reviewed-by Marcel Apfelbaum <[email protected]>
---
 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(-)
hw/mips/gt64xxx_pci.c +2 -2
index 1f2fe5f..f118c9c 100644
@@ -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);
hw/pci/pci_host.c +3 -2
index 3e26f92..9bc47d8 100644
@@ -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);
include/hw/pci/pci_host.h +5 -0
index ba31595..b48791d 100644
@@ -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;
Signature · 1 line
1.9.3
#4 lore raw

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

Quoted 9 lines · Hu Tao

This makes code more readable.

Signed-off-by Hu Tao <[email protected]>
Reviewed-by Marcel Apfelbaum <[email protected]>
---
 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?

Quoted 51 lines
hw/mips/gt64xxx_pci.c +2 -2
index 1f2fe5f..f118c9c 100644
@@ -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);
hw/pci/pci_host.c +3 -2
index 3e26f92..9bc47d8 100644
@@ -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);
include/hw/pci/pci_host.h +0 -0
index ba31595..b48791d 100644

include/hw/pci/pci_host.h uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, · line 72

Quoted 8 lines
include/hw/pci/pci_host.h +5 -0
@@ -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);
Quoted 5 lines
include/hw/pci/pci_host.h +0 -0
 extern const MemoryRegionOps pci_host_conf_le_ops;
 extern const MemoryRegionOps pci_host_conf_be_ops;
 extern const MemoryRegionOps pci_host_data_le_ops;
Signature · 1 line
1.9.3
author
#5 lore raw

PATCH v3 3/6 pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA.

In reply to 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 <[email protected]>
Reviewed-by Marcel Apfelbaum <[email protected]>
---
 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(-)
hw/pci-host/piix.c +4 -4
index 1530038..76f3757 100644
@@ -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)
hw/pci-host/prep.c +4 -2
index 1de3681..2ae21ad 100644
@@ -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);
hw/pci-host/q35.c +4 -4
index b20bad8..666afea 100644
@@ -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,
include/hw/pci-host/q35.h +0 -3
index 025d6e6..3a026b0 100644
@@ -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
include/hw/pci/pci_host.h +5 -0
index b48791d..2bae45a 100644
@@ -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)
tests/libqos/pci-pc.c +13 -12
index 6dba0db..e4c3c11 100644
@@ -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)
Signature · 1 line
1.9.3
#6 lore raw

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

Quoted 2 lines · Hu Tao

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.

Quoted 166 lines
Signed-off-by Hu Tao <[email protected]>
Reviewed-by Marcel Apfelbaum <[email protected]>
---
 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(-)
hw/pci-host/piix.c +4 -4
index 1530038..76f3757 100644
@@ -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)
hw/pci-host/prep.c +4 -2
index 1de3681..2ae21ad 100644
@@ -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);
hw/pci-host/q35.c +4 -4
index b20bad8..666afea 100644
@@ -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,
include/hw/pci-host/q35.h +0 -3
index 025d6e6..3a026b0 100644
@@ -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
include/hw/pci/pci_host.h +5 -0
index b48791d..2bae45a 100644
@@ -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)
tests/libqos/pci-pc.c +13 -12
index 6dba0db..e4c3c11 100644
@@ -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)
Signature · 1 line
1.9.3
#7 lore raw

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

In reply to Hu Tao
Quoted 5 lines · 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 <[email protected]>
Reviewed-by Marcel Apfelbaum <[email protected]>

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/.

Quoted 164 lines
---
 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(-)
hw/pci-host/piix.c +4 -4
index 1530038..76f3757 100644
@@ -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)
hw/pci-host/prep.c +4 -2
index 1de3681..2ae21ad 100644
@@ -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);
hw/pci-host/q35.c +4 -4
index b20bad8..666afea 100644
@@ -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,
include/hw/pci-host/q35.h +0 -3
index 025d6e6..3a026b0 100644
@@ -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
include/hw/pci/pci_host.h +5 -0
index b48791d..2bae45a 100644
@@ -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)
tests/libqos/pci-pc.c +13 -12
index 6dba0db..e4c3c11 100644
@@ -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)
Signature · 1 line
1.9.3
author
#8 lore raw

PATCH v3 4/6 pci: remove the limit parameter of pci_host_config_read_common

In reply to 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 <[email protected]>
---
 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(-)
hw/pci/pci_host.c +11 -4
index 9bc47d8..2b11551 100644
@@ -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);
hw/pci/pcie_host.c +1 -8
index 3db038f..cf8587b 100644
@@ -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 = {
hw/ppc/spapr_pci.c +1 -2
index 21b95b3..59c6608 100644
@@ -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);
include/hw/pci/pci_host.h +1 -1
index 2bae45a..72a1b8b 100644
@@ -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);
Signature · 1 line
1.9.3
#9 lore raw

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

Quoted 5 lines · 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 <[email protected]>

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

Quoted 98 lines
---
 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(-)
hw/pci/pci_host.c +11 -4
index 9bc47d8..2b11551 100644
@@ -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);
hw/pci/pcie_host.c +1 -8
index 3db038f..cf8587b 100644
@@ -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 = {
hw/ppc/spapr_pci.c +1 -2
index 21b95b3..59c6608 100644
@@ -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);
include/hw/pci/pci_host.h +1 -1
index 2bae45a..72a1b8b 100644
@@ -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);
Signature · 1 line
1.9.3
#10 lore raw

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

In reply to Hu Tao
Quoted 2 lines · Hu Tao

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.

Quoted 102 lines

and we can determine the size from the type of pci device.

Signed-off-by Hu Tao <[email protected]>
---
 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(-)
hw/pci/pci_host.c +11 -4
index 9bc47d8..2b11551 100644
@@ -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);
hw/pci/pcie_host.c +1 -8
index 3db038f..cf8587b 100644
@@ -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 = {
hw/ppc/spapr_pci.c +1 -2
index 21b95b3..59c6608 100644
@@ -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);
include/hw/pci/pci_host.h +1 -1
index 2bae45a..72a1b8b 100644
@@ -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);
Signature · 1 line
1.9.3
author
#11 lore raw

PATCH v3 5/6 pci: remove the limit parameter of pci_host_config_write_common

In reply to 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 <[email protected]>
---
 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(-)
hw/pci/pci_host.c +10 -3
index 2b11551..4a59b0e 100644
@@ -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)
hw/pci/pcie_host.c +1 -8
index cf8587b..e3a2a80 100644
@@ -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,
hw/ppc/spapr_pci.c +1 -2
index 59c6608..8c566dd 100644
@@ -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);
 }
include/hw/pci/pci_host.h +1 -1
index 72a1b8b..67e007f 100644
@@ -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);
Signature · 1 line
1.9.3
#12 lore raw

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

Quoted 3 lines · 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.

Same comment.

Quoted 94 lines
Signed-off-by Hu Tao <[email protected]>
---
 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(-)
hw/pci/pci_host.c +10 -3
index 2b11551..4a59b0e 100644
@@ -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)
hw/pci/pcie_host.c +1 -8
index cf8587b..e3a2a80 100644
@@ -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,
hw/ppc/spapr_pci.c +1 -2
index 59c6608..8c566dd 100644
@@ -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);
 }
include/hw/pci/pci_host.h +1 -1
index 72a1b8b..67e007f 100644
@@ -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);
Signature · 1 line
1.9.3
author
#13 lore raw

PATCH v3 6/6 pci: introduce PCI_DEVFN_AUTO

In reply to Hu Tao

Introduce PCI_DEVFN_AUTO rather than using -1 in code.

Signed-off-by Hu Tao <[email protected]>
---
 hw/core/qdev-properties.c | 1 +
 hw/pci/pci.c              | 5 ++---
 include/hw/pci/pci.h      | 2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)
hw/core/qdev-properties.c +1 -0
index 2e47f70..df4ad14 100644
@@ -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)
hw/pci/pci.c +2 -3
index 371699c..73c7dec 100644
@@ -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])
include/hw/pci/pci.h +2 -0
index b18759a..9206b12 100644
@@ -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)
Signature · 1 line
1.9.3
author
#14 lore raw

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

Quoted 14 lines · Hu Tao

Introduce PCI_DEVFN_AUTO rather than using -1 in code.

Signed-off-by Hu Tao <[email protected]>
---
 hw/core/qdev-properties.c | 1 +
 hw/pci/pci.c              | 5 ++---
 include/hw/pci/pci.h      | 2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)
hw/core/qdev-properties.c +0 -0
index 2e47f70..df4ad14 100644

hw/core/qdev-properties.c line 9

Quoted 4 lines
hw/core/qdev-properties.c +1 -0
@@ -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

#15 lore raw

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

In reply to Hu Tao
Quoted 43 lines · Hu Tao

Introduce PCI_DEVFN_AUTO rather than using -1 in code.

Signed-off-by Hu Tao <[email protected]>
---
 hw/core/qdev-properties.c | 1 +
 hw/pci/pci.c              | 5 ++---
 include/hw/pci/pci.h      | 2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)
hw/core/qdev-properties.c +1 -0
index 2e47f70..df4ad14 100644
@@ -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)
hw/pci/pci.c +1 -2
index 371699c..73c7dec 100644
@@ -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)
 {

hw/pci/pci.c static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, · line 812

Quoted 5 lines
hw/pci/pci.c +1 -1
@@ -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?

Quoted 18 lines
hw/pci/pci.c +0 -0
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
             devfn += PCI_FUNC_MAX) {
             if (!bus->devices[devfn])
include/hw/pci/pci.h +2 -0
index b18759a..9206b12 100644
@@ -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)
Signature · 1 line
1.9.3
author
#16 lore raw

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

Quoted 47 lines · Michael S. Tsirkin
Quoted 46 lines · Hu Tao

Introduce PCI_DEVFN_AUTO rather than using -1 in code.

Signed-off-by Hu Tao <[email protected]>
---
 hw/core/qdev-properties.c | 1 +
 hw/pci/pci.c              | 5 ++---
 include/hw/pci/pci.h      | 2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)
hw/core/qdev-properties.c +1 -0
index 2e47f70..df4ad14 100644
@@ -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)
hw/pci/pci.c +1 -2
index 371699c..73c7dec 100644
@@ -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;
Quoted 4 lines

hw/pci/pci.c static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, · line 812

Quoted 2 lines
hw/pci/pci.c +1 -1
-    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

Quoted 24 lines

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?

Quoted 18 lines
hw/pci/pci.c +0 -0
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
             devfn += PCI_FUNC_MAX) {
             if (!bus->devices[devfn])
include/hw/pci/pci.h +2 -0
index b18759a..9206b12 100644
@@ -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)
Signature · 1 line
1.9.3
author
#17 lore raw

PATCH RESEND v3 6/6 pci: introduce PCI_DEVFN_AUTO

In reply to Hu Tao

Introduce PCI_DEVFN_AUTO rather than using -1 in code.

Signed-off-by Hu Tao <[email protected]>
---
 hw/pci/pci.c         | 5 ++---
 include/hw/pci/pci.h | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)
hw/pci/pci.c +2 -3
index 371699c..73c7dec 100644
@@ -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])
include/hw/pci/pci.h +2 -0
index b18759a..9206b12 100644
@@ -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)
Signature · 1 line
1.9.3
author
#18 lore raw
In reply to Hu Tao

ping...

Quoted 47 lines · 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(-)
Signature · 1 line
1.9.3
#19 lore raw
Quoted 1 line · Hu Tao

ping...

At some point you said "will resend".

Quoted 48 lines · Hu Tao
Quoted 47 lines

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(-)
Signature · 1 line
1.9.3
author
#20 lore raw
Quoted 4 lines · Michael S. Tsirkin
Quoted 1 line · Hu Tao

ping...

At some point you said "will resend".

I meant resend patch 6.

Patches 1-5 can still apply cleanly.

Regards,
Hu

Quoted 49 lines
Quoted 49 lines · Hu Tao
Quoted 47 lines

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(-)
Signature · 1 line
1.9.3