Some PCI related cleanup patches

PATCH Superseded Hu Tao 2 participants 12 messages Nov 4, 2014 – Nov 5, 2014

lore

Share & tools

lore

Hu Tao

author

2014-11-04 09:15 UTC #0 lore raw

cover Some PCI related cleanup patches

Hi,

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

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

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

Signature · 1 line

1.9.3

Hu Tao

author

2014-11-04 09:15 UTC #1 lore raw

PATCH 1/5 pci: introduce PC_PCI_CONFIG_ENABLED()

This makes code more readable.

Signed-off-by Hu Tao <[email protected]>

---
 hw/mips/gt64xxx_pci.c | 4 ++--
 hw/pci/pci_host.c     | 5 +++--
 include/hw/pci/pci.h  | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

hw/mips/gt64xxx_pci.c +2 -2

index 1f2fe5f..a49dbd7 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 (PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
             pci_data_write(phb->bus, phb->config_reg, val, 4);
         }
         break;

@@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
         val = phb->config_reg;
         break;
     case GT_PCI0_CFGDATA:
-        if (!(phb->config_reg & (1 << 31))) {
+        if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
             val = 0xffffffff;
         } else {
             val = pci_data_read(phb->bus, phb->config_reg, 4);

hw/pci/pci_host.c +3 -2

index 3e26f92..f2a69ea 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 (PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+    }
 }
 
 static uint64_t pci_host_data_read(void *opaque,

@@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
 {
     PCIHostState *s = opaque;
     uint32_t val;
-    if (!(s->config_reg & (1U << 31))) {
+    if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         return 0xffffffff;
     }
     val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);

include/hw/pci/pci.h +2 -0

index c352c7b..3d42d7f 100644
@@ -13,6 +13,8 @@
 
 #include "hw/pci/pcie.h"
 
+#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
+
 /* PCI bus */
 
 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))

Signature · 1 line

1.9.3

Marcel Apfelbaum

2014-11-04 13:42 UTC #2 lore raw

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

Hi,

Quoted 14 lines · Hu Tao

This makes code more readable.
Signed-off-by Hu Tao <[email protected]>
---
 hw/mips/gt64xxx_pci.c | 4 ++--
 hw/pci/pci_host.c     | 5 +++--
 include/hw/pci/pci.h  | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)
hw/mips/gt64xxx_pci.c +0 -0
index 1f2fe5f..a49dbd7 100644

hw/mips/gt64xxx_pci.c static void gt64120_writel (void *opaque, hwaddr addr, · line 567

Quoted 5 lines

hw/mips/gt64xxx_pci.c +1 -1

@@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr,
         if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
             val = bswap32(val);
         }
-        if (phb->config_reg & (1u << 31)) {
+        if (PC_PCI_CONFIG_ENABLED(phb->config_reg))

I really like readable MACROS instead of magic numbers,
however I have 3 suggestions:
1. PC_PCI_CONFIG_ENABLED is still not really clear, because
of the "PC" prefix that is too wide in my IMHO (maybe PCI_HOST_BRIDGE_CONFIG_ENABLED) when this macro refers only to host bridges.
2. Maybe go the "extra mile" and let the macro receive
a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge).
3. Maybe make it an inline function? Just wondering

Quoted 41 lines

hw/mips/gt64xxx_pci.c +1 -1

             pci_data_write(phb->bus, phb->config_reg, val, 4);
         }
         break;

@@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
         val = phb->config_reg;
         break;
     case GT_PCI0_CFGDATA:
-        if (!(phb->config_reg & (1 << 31))) {
+        if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
             val = 0xffffffff;
         } else {
             val = pci_data_read(phb->bus, phb->config_reg, 4);

hw/pci/pci_host.c +3 -2

index 3e26f92..f2a69ea 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 (PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+    }
 }
 
 static uint64_t pci_host_data_read(void *opaque,

@@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
 {
     PCIHostState *s = opaque;
     uint32_t val;
-    if (!(s->config_reg & (1U << 31))) {
+    if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         return 0xffffffff;
     }
     val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);

include/hw/pci/pci.h +0 -0

index c352c7b..3d42d7f 100644

include/hw/pci/pci.h line 16

Quoted 4 lines

include/hw/pci/pci.h +1 -0

@@ -13,6 +13,8 @@
 
 #include "hw/pci/pcie.h"
 
+#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))

Again, maybe move this to "hw/pci/pci_host" since is specific to host bridges?

Thanks,
Marcel

Quoted 4 lines

include/hw/pci/pci.h +1 -0

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

Hu Tao

author

2014-11-05 08:00 UTC #3 lore raw

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

Quoted 21 lines · Marcel Apfelbaum

Hi,

Quoted 18 lines · Hu Tao

This makes code more readable.

Signed-off-by Hu Tao <[email protected]>

---
 hw/mips/gt64xxx_pci.c | 4 ++--
 hw/pci/pci_host.c     | 5 +++--
 include/hw/pci/pci.h  | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

hw/mips/gt64xxx_pci.c +0 -1

index 1f2fe5f..a49dbd7 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)) {

Quoted 8 lines

hw/mips/gt64xxx_pci.c static void gt64120_writel (void *opaque, hwaddr addr, · line 567
Quoted 1 line
hw/mips/gt64xxx_pci.c +1 -0
+        if (PC_PCI_CONFIG_ENABLED(phb->config_reg)) 
I really like readable MACROS instead of magic numbers,
however I have 3 suggestions:
1. PC_PCI_CONFIG_ENABLED is still not really clear, because
of the "PC" prefix that is too wide in my IMHO (maybe PCI_HOST_BRIDGE_CONFIG_ENABLED) when this macro refers only to host bridges.
2. Maybe go the "extra mile" and let the macro receive
a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge).

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

Quoted 44 lines

3. Maybe make it an inline function? Just wondering

Quoted 42 lines

hw/mips/gt64xxx_pci.c +1 -1

             pci_data_write(phb->bus, phb->config_reg, val, 4);
         }
         break;

@@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
         val = phb->config_reg;
         break;
     case GT_PCI0_CFGDATA:
-        if (!(phb->config_reg & (1 << 31))) {
+        if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) {
             val = 0xffffffff;
         } else {
             val = pci_data_read(phb->bus, phb->config_reg, 4);

hw/pci/pci_host.c +3 -2

index 3e26f92..f2a69ea 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 (PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+    }
 }
 
 static uint64_t pci_host_data_read(void *opaque,

@@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
 {
     PCIHostState *s = opaque;
     uint32_t val;
-    if (!(s->config_reg & (1U << 31))) {
+    if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) {
         return 0xffffffff;
     }
     val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);

include/hw/pci/pci.h +0 -0

index c352c7b..3d42d7f 100644
@@ -13,6 +13,8 @@

Quoted 4 lines

include/hw/pci/pci.h line 16
Quoted 3 lines
include/hw/pci/pci.h +1 -0
 #include "hw/pci/pcie.h"
 
+#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
Again, maybe move this to "hw/pci/pci_host" since is specific to host bridges?

Done.

Thanks!

Quoted 10 lines

Thanks,
Marcel
Quoted 4 lines
include/hw/pci/pci.h +1 -0
+
 /* PCI bus */
 
 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))

Hu Tao

author

2014-11-04 09:15 UTC #4 lore raw

PATCH 2/5 pc: define PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA

In reply to Hu Tao

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

Signed-off-by Hu Tao <[email protected]>

---
 hw/pci-host/piix.c        |  8 ++++----
 hw/pci-host/q35.c         |  8 ++++----
 include/hw/pci-host/q35.h |  3 ---
 include/hw/pci/pci.h      |  5 +++++
 tests/libqos/pci-pc.c     | 24 ++++++++++++------------
 5 files changed, 25 insertions(+), 23 deletions(-)

hw/pci-host/piix.c +4 -4

index 1530038..eb92bde 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, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
-    sysbus_init_ioports(sbd, 0xcfc, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 }
 
 static int i440fx_initfn(PCIDevice *dev)

hw/pci-host/q35.c +4 -4

index b20bad8..9e66835 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, PC_PCI_CONFIG_ADDR, &pci->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
-    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &pci->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 
     pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
                            s->mch.pci_address_space, s->mch.address_space_io,

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.h +5 -0

index 3d42d7f..e42589a 100644
@@ -13,6 +13,11 @@
 
 #include "hw/pci/pcie.h"
 
+/* PCI configuration */
+
+#define PC_PCI_CONFIG_ADDR      0xcf8
+#define PC_PCI_CONFIG_DATA      0xcfc
+
 #define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
 
 /* PCI bus */

tests/libqos/pci-pc.c +12 ~~-12~~

index 6dba0db..2762608 100644
@@ -113,38 +113,38 @@ static void qpci_pc_io_writel(QPCIBus *bus, void *addr, uint32_t value)
 
 static uint8_t qpci_pc_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inb(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inb(PC_PCI_CONFIG_DATA);
 }
 
 static uint16_t qpci_pc_config_readw(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inw(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inw(PC_PCI_CONFIG_DATA);
 }
 
 static uint32_t qpci_pc_config_readl(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inl(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inl(PC_PCI_CONFIG_DATA);
 }
 
 static void qpci_pc_config_writeb(QPCIBus *bus, int devfn, uint8_t offset, uint8_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outb(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outb(PC_PCI_CONFIG_DATA, value);
 }
 
 static void qpci_pc_config_writew(QPCIBus *bus, int devfn, uint8_t offset, uint16_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outw(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outw(PC_PCI_CONFIG_DATA, value);
 }
 
 static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint32_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outl(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outl(PC_PCI_CONFIG_DATA, value);
 }
 
 static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)

Signature · 1 line

1.9.3

Marcel Apfelbaum

2014-11-04 13:45 UTC #5 lore raw

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

Quoted 71 lines · Hu Tao

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

Signed-off-by Hu Tao <[email protected]>

---
 hw/pci-host/piix.c        |  8 ++++----
 hw/pci-host/q35.c         |  8 ++++----
 include/hw/pci-host/q35.h |  3 ---
 include/hw/pci/pci.h      |  5 +++++
 tests/libqos/pci-pc.c     | 24 ++++++++++++------------
 5 files changed, 25 insertions(+), 23 deletions(-)

hw/pci-host/piix.c +4 -4

index 1530038..eb92bde 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, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
-    sysbus_init_ioports(sbd, 0xcfc, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 }
 
 static int i440fx_initfn(PCIDevice *dev)

hw/pci-host/q35.c +4 -4

index b20bad8..9e66835 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, PC_PCI_CONFIG_ADDR, &pci->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
-    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &pci->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 
     pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
                            s->mch.pci_address_space, s->mch.address_space_io,

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.h +0 -0

index 3d42d7f..e42589a 100644

include/hw/pci/pci.h line 19

Quoted 7 lines

include/hw/pci/pci.h +4 -0

@@ -13,6 +13,11 @@
 
 #include "hw/pci/pcie.h"
 
+/* PCI configuration */
+
+#define PC_PCI_CONFIG_ADDR      0xcf8
+#define PC_PCI_CONFIG_DATA      0xcfc

I would move the macros also to hw/pci/pci_host.h, and only personal opinion, change them to PCI_HOST_BRIDGE_...

Thanks,
Marcel

Quoted 59 lines

include/hw/pci/pci.h +1 -0

+
 #define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31))
 
 /* PCI bus */

tests/libqos/pci-pc.c +12 ~~-12~~

index 6dba0db..2762608 100644
@@ -113,38 +113,38 @@ static void qpci_pc_io_writel(QPCIBus *bus, void *addr, uint32_t value)
 
 static uint8_t qpci_pc_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inb(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inb(PC_PCI_CONFIG_DATA);
 }
 
 static uint16_t qpci_pc_config_readw(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inw(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inw(PC_PCI_CONFIG_DATA);
 }
 
 static uint32_t qpci_pc_config_readl(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    return inl(0xcfc);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    return inl(PC_PCI_CONFIG_DATA);
 }
 
 static void qpci_pc_config_writeb(QPCIBus *bus, int devfn, uint8_t offset, uint8_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outb(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outb(PC_PCI_CONFIG_DATA, value);
 }
 
 static void qpci_pc_config_writew(QPCIBus *bus, int devfn, uint8_t offset, uint16_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outw(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outw(PC_PCI_CONFIG_DATA, value);
 }
 
 static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint32_t value)
 {
-    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
-    outl(0xcfc, value);
+    outl(PC_PCI_CONFIG_ADDR, (1U << 31) | (devfn << 8) | offset);
+    outl(PC_PCI_CONFIG_DATA, value);
 }
 
 static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)

Hu Tao

author

2014-11-05 09:00 UTC #6 lore raw

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

Quoted 75 lines · Marcel Apfelbaum

Quoted 74 lines · Hu Tao

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

Signed-off-by Hu Tao <[email protected]>

---
 hw/pci-host/piix.c        |  8 ++++----
 hw/pci-host/q35.c         |  8 ++++----
 include/hw/pci-host/q35.h |  3 ---
 include/hw/pci/pci.h      |  5 +++++
 tests/libqos/pci-pc.c     | 24 ++++++++++++------------
 5 files changed, 25 insertions(+), 23 deletions(-)

hw/pci-host/piix.c +4 -4

index 1530038..eb92bde 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, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
-    sysbus_init_ioports(sbd, 0xcfc, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 }
 
 static int i440fx_initfn(PCIDevice *dev)

hw/pci-host/q35.c +4 -4

index b20bad8..9e66835 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, PC_PCI_CONFIG_ADDR, &pci->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
-    sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &pci->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
 
     pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
                            s->mch.pci_address_space, s->mch.address_space_io,

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.h +0 -0

index 3d42d7f..e42589a 100644
@@ -13,6 +13,11 @@
 
 #include "hw/pci/pcie.h"

Quoted 7 lines

include/hw/pci/pci.h line 19
Quoted 4 lines
include/hw/pci/pci.h +4 -0
+/* PCI configuration */
+
+#define PC_PCI_CONFIG_ADDR      0xcf8
+#define PC_PCI_CONFIG_DATA      0xcfc
I would move the macros also to hw/pci/pci_host.h, and only personal opinion, change them to PCI_HOST_BRIDGE_...

Done.

Regards,
Hu

Hu Tao

author

2014-11-04 09:15 UTC #7 lore raw

PATCH 3/5 pci: move initialization of pci's conf_addr and conf_data to common place

In reply to Hu Tao

So that standard pci host device can share them.

Signed-off-by Hu Tao <[email protected]>

---
 hw/pci-host/piix.c | 20 --------------------
 hw/pci-host/q35.c  |  7 -------
 hw/pci/pci_host.c  | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 27 deletions(-)

hw/pci-host/piix.c +0 ~~-20~~

index eb92bde..683465c 100644
@@ -256,14 +256,8 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 
 static void i440fx_pcihost_initfn(Object *obj)
 {
-    PCIHostState *s = PCI_HOST_BRIDGE(obj);
     I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
 
-    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
-                          "pci-conf-data", 4);
-
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
                         i440fx_pcihost_get_pci_hole_start,
                         NULL, NULL, NULL, NULL);

@@ -283,18 +277,6 @@ static void i440fx_pcihost_initfn(Object *obj)
     d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
-static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
-{
-    PCIHostState *s = PCI_HOST_BRIDGE(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
-}
-
 static int i440fx_initfn(PCIDevice *dev)
 {
     PCII440FXState *d = I440FX_PCI_DEVICE(dev);

@@ -755,8 +737,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
 
     hc->root_bus_path = i440fx_pcihost_root_bus_path;
-    dc->realize = i440fx_pcihost_realize;
-    dc->fw_name = "pci";
     dc->props = i440fx_props;
 }

hw/pci-host/q35.c +0 -7

index 9e66835..81eddd7 100644
@@ -138,18 +138,11 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
     dc->realize = q35_host_realize;
     dc->props = mch_props;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->fw_name = "pci";
 }
 
 static void q35_host_initfn(Object *obj)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
-    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
-
-    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
-                          "pci-conf-data", 4);
 
     object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
     object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);

hw/pci/pci_host.c +32 -0

index f2a69ea..406c747 100644
@@ -176,12 +176,44 @@ const MemoryRegionOps pci_host_data_be_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void pci_host_initfn(Object *obj)
+{
+    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
+
+    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
+                          "pci-conf-idx", 4);
+    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
+                          "pci-conf-data", 4);
+}
+
+static void pci_host_realize(DeviceState *dev, Error **errp)
+{
+    PCIHostState *s = PCI_HOST_BRIDGE(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
+}
+
+static void pci_host_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = pci_host_realize;
+    dc->fw_name = "pci";
+}
+
 static const TypeInfo pci_host_type_info = {
     .name = TYPE_PCI_HOST_BRIDGE,
     .parent = TYPE_SYS_BUS_DEVICE,
     .abstract = true,
     .class_size = sizeof(PCIHostBridgeClass),
+    .class_init = pci_host_class_init,
     .instance_size = sizeof(PCIHostState),
+    .instance_init = pci_host_initfn,
 };
 
 static void pci_host_register_types(void)

Signature · 1 line

1.9.3

Marcel Apfelbaum

2014-11-04 14:22 UTC #8 lore raw

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

Quoted 97 lines · Hu Tao

So that standard pci host device can share them.

Signed-off-by Hu Tao <[email protected]>

---
 hw/pci-host/piix.c | 20 --------------------
 hw/pci-host/q35.c  |  7 -------
 hw/pci/pci_host.c  | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 27 deletions(-)

hw/pci-host/piix.c +0 ~~-20~~

index eb92bde..683465c 100644
@@ -256,14 +256,8 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 
 static void i440fx_pcihost_initfn(Object *obj)
 {
-    PCIHostState *s = PCI_HOST_BRIDGE(obj);
     I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
 
-    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
-                          "pci-conf-data", 4);
-
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
                         i440fx_pcihost_get_pci_hole_start,
                         NULL, NULL, NULL, NULL);

@@ -283,18 +277,6 @@ static void i440fx_pcihost_initfn(Object *obj)
     d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
-static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
-{
-    PCIHostState *s = PCI_HOST_BRIDGE(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
-}
-
 static int i440fx_initfn(PCIDevice *dev)
 {
     PCII440FXState *d = I440FX_PCI_DEVICE(dev);

@@ -755,8 +737,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
 
     hc->root_bus_path = i440fx_pcihost_root_bus_path;
-    dc->realize = i440fx_pcihost_realize;
-    dc->fw_name = "pci";
     dc->props = i440fx_props;
 }

hw/pci-host/q35.c +0 -7

index 9e66835..81eddd7 100644
@@ -138,18 +138,11 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
     dc->realize = q35_host_realize;
     dc->props = mch_props;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->fw_name = "pci";
 }
 
 static void q35_host_initfn(Object *obj)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
-    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
-
-    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
-                          "pci-conf-data", 4);
 
     object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
     object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);

hw/pci/pci_host.c +10 -0

index f2a69ea..406c747 100644
@@ -176,12 +176,44 @@ const MemoryRegionOps pci_host_data_be_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void pci_host_initfn(Object *obj)
+{
+    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
+
+    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
+                          "pci-conf-idx", 4);
+    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
+                          "pci-conf-data", 4);
+}
+

hw/pci/pci_host.c const MemoryRegionOps pci_host_data_be_ops = { · line 198

Quoted 10 lines

hw/pci/pci_host.c +10 -0

+static void pci_host_realize(DeviceState *dev, Error **errp)
+{
+    PCIHostState *s = PCI_HOST_BRIDGE(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);

Hi,

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

hw/mips/gt64xxx_pci.c:1207: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-bridge/dec.c:150: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/apb.c:839: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/bonito.c:832: .parent = TYPE_PCI_HOST_BRIDGE,

hw/pci-host/grackle.c:156:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/ppce500.c:440:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/prep.c:392:    .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:456:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:470:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:484:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:498:    .parent        = TYPE_PCI_HOST_BRIDGE,

hw/pci-host/versatile.c:508: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci/pci_host.c:179: .name = TYPE_PCI_HOST_BRIDGE,
hw/ppc/ppc4xx_pci.c:405: .parent = TYPE_PCI_HOST_BRIDGE,
hw/ppc/spapr_pci.c:801: .parent = TYPE_PCI_HOST_BRIDGE,
hw/sh4/sh_pci.c:193: .parent = TYPE_PCI_HOST_BRIDGE,

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

Thanks,
Marcel

Quoted 21 lines

hw/pci/pci_host.c +12 -0

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

Hu Tao

author

2014-11-05 06:06 UTC #9 lore raw

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

Quoted 110 lines · Marcel Apfelbaum

Quoted 97 lines · Hu Tao

So that standard pci host device can share them.

Signed-off-by Hu Tao <[email protected]>

---
 hw/pci-host/piix.c | 20 --------------------
 hw/pci-host/q35.c  |  7 -------
 hw/pci/pci_host.c  | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 27 deletions(-)

hw/pci-host/piix.c +0 ~~-20~~

index eb92bde..683465c 100644
@@ -256,14 +256,8 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 
 static void i440fx_pcihost_initfn(Object *obj)
 {
-    PCIHostState *s = PCI_HOST_BRIDGE(obj);
     I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
 
-    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
-                          "pci-conf-data", 4);
-
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
                         i440fx_pcihost_get_pci_hole_start,
                         NULL, NULL, NULL, NULL);

@@ -283,18 +277,6 @@ static void i440fx_pcihost_initfn(Object *obj)
     d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
-static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
-{
-    PCIHostState *s = PCI_HOST_BRIDGE(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
-
-    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
-    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
-}
-
 static int i440fx_initfn(PCIDevice *dev)
 {
     PCII440FXState *d = I440FX_PCI_DEVICE(dev);

@@ -755,8 +737,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
 
     hc->root_bus_path = i440fx_pcihost_root_bus_path;
-    dc->realize = i440fx_pcihost_realize;
-    dc->fw_name = "pci";
     dc->props = i440fx_props;
 }

hw/pci-host/q35.c +0 -7

index 9e66835..81eddd7 100644
@@ -138,18 +138,11 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
     dc->realize = q35_host_realize;
     dc->props = mch_props;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->fw_name = "pci";
 }
 
 static void q35_host_initfn(Object *obj)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
-    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
-
-    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
-                          "pci-conf-idx", 4);
-    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
-                          "pci-conf-data", 4);
 
     object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
     object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);

hw/pci/pci_host.c +10 -0

index f2a69ea..406c747 100644
@@ -176,12 +176,44 @@ const MemoryRegionOps pci_host_data_be_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void pci_host_initfn(Object *obj)
+{
+    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
+
+    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
+                          "pci-conf-idx", 4);
+    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
+                          "pci-conf-data", 4);
+}
+

hw/pci/pci_host.c const MemoryRegionOps pci_host_data_be_ops = { · line 198

Quoted 10 lines

hw/pci/pci_host.c +10 -0

+static void pci_host_realize(DeviceState *dev, Error **errp)
+{
+    PCIHostState *s = PCI_HOST_BRIDGE(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
+
+    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
+    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);

Hi,

Quoted 3 lines

If I got this right, now we have this pci_host_realize run for each object of classes deriving from TYPE_PCI_HOST_BRIDGE. Please correct me if I am wrong.

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

Quoted 14 lines

The machines PC and Q35 share the same code, however
we have other host bridges deriving that I think they behave different:
hw/mips/gt64xxx_pci.c:1207: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-bridge/dec.c:150: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/apb.c:839: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/bonito.c:832: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/grackle.c:156:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/ppce500.c:440:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/prep.c:392:    .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:456:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:470:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:484:    .parent        = TYPE_PCI_HOST_BRIDGE,
hw/pci-host/uninorth.c:498:    .parent        = TYPE_PCI_HOST_BRIDGE,

Quoted 8 lines

hw/pci-host/versatile.c:508: .parent = TYPE_PCI_HOST_BRIDGE,
hw/pci/pci_host.c:179: .name = TYPE_PCI_HOST_BRIDGE,
hw/ppc/ppc4xx_pci.c:405: .parent = TYPE_PCI_HOST_BRIDGE,
hw/ppc/spapr_pci.c:801: .parent = TYPE_PCI_HOST_BRIDGE,
hw/sh4/sh_pci.c:193: .parent = TYPE_PCI_HOST_BRIDGE,
If I am right, putting this so "high" in the hierarchy may not be the right solution.

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

Thanks for review!

Quoted 28 lines

Thanks,
Marcel

Quoted 21 lines

hw/pci/pci_host.c +12 -0

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

Hu Tao

author

2014-11-04 09:15 UTC #10 lore raw

PATCH 4/5 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 406c747..937660c 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 ad0da7f..7f38117 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 ba31595..4a79945 100644
@@ -60,7 +60,7 @@ typedef struct PCIHostBridgeClass {
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
                                   uint32_t limit, uint32_t val, uint32_t len);
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len);
+                                     uint32_t len);
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);

Signature · 1 line

1.9.3

Hu Tao

author

2014-11-04 09:15 UTC #11 lore raw

PATCH 5/5 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 937660c..f525fe5 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 7f38117..f306d42 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 4a79945..9364f08 100644
@@ -58,7 +58,7 @@ typedef struct PCIHostBridgeClass {
 
 /* common internal helpers for PCI/PCIe hosts, cut off overflows */
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len);
+                                  uint32_t val, uint32_t len);
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
                                      uint32_t len);

Signature · 1 line

1.9.3