* [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up
@ 2010-01-12 8:52 Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 1/6] sh_pci: use PCIHostState instead of PCIBus Isaku Yamahata
` (6 more replies)
0 siblings, 7 replies; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 8:52 UTC (permalink / raw)
To: qemu-devel; +Cc: yamahata, agraf, mst
During reviewing Alexander's PPC patches, it proved that
pci_data_{read, write}() should take PCIConfigAddress as an argument.
this patch series is inspired by his patch and some of them
are based on it.
This patch would make Alexander's PPC work easier.
Isaku Yamahata (6):
sh_pci: use PCIHostState instead of PCIBus.
sh_pci: s/sh_pci_data_write/sh_pci_mem_write/g for consistency.
versatile_pci: user PCIHostState instead of PCIBus
pci_host: remove code duplication in pci_host_template.h
pci: introduce PCIAddress, PCIConfigAddress and helper functions.
pci host: make pci_data_{write, read}() get PCIConfigAddress.
hw/apb_pci.c | 12 ++++--
hw/gt64xxx.c | 20 ++++++----
hw/pci.h | 7 ++++
hw/pci_host.c | 61 +++++++++++++++++++++++++-------
hw/pci_host.h | 21 ++++++++++-
hw/pci_host_template.h | 85 +++++++++----------------------------------
hw/pci_host_template_all.h | 23 ++++++++++++
hw/prep_pci.c | 28 +++++++++++---
hw/sh_pci.c | 42 +++++++++++++++-------
hw/versatile_pci.c | 47 +++++++++++++++++++-----
qemu-common.h | 2 +
11 files changed, 225 insertions(+), 123 deletions(-)
create mode 100644 hw/pci_host_template_all.h
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 1/6] sh_pci: use PCIHostState instead of PCIBus.
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
@ 2010-01-12 8:52 ` Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 2/6] sh_pci: s/sh_pci_data_write/sh_pci_mem_write/g for consistency Isaku Yamahata
` (5 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 8:52 UTC (permalink / raw)
To: qemu-devel; +Cc: yamahata, Takashi YOSHII, agraf, mst
To use pci_host framework, use PCIHostState instead of PCIBus.
Cc: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/sh_pci.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index abe4c75..d76d390 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -29,7 +29,7 @@
#include "bswap.h"
typedef struct {
- PCIBus *bus;
+ PCIHostState s;
PCIDevice *dev;
uint32_t par;
uint32_t mbr;
@@ -53,7 +53,7 @@ static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
pcic->iobr = val;
break;
case 0x220:
- pci_data_write(pcic->bus, pcic->par, val, 4);
+ pci_data_write(pcic->s.bus, pcic->par, val, 4);
break;
}
}
@@ -67,7 +67,7 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
case 0x1c0:
return pcic->par;
case 0x220:
- return pci_data_read(pcic->bus, pcic->par, 4);
+ return pci_data_read(pcic->s.bus, pcic->par, 4);
}
return 0;
}
@@ -75,13 +75,13 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
static void sh_pci_data_write (SHPCIC *pcic, target_phys_addr_t addr,
uint32_t val, int size)
{
- pci_data_write(pcic->bus, addr + pcic->mbr, val, size);
+ pci_data_write(pcic->s.bus, addr + pcic->mbr, val, size);
}
static uint32_t sh_pci_mem_read (SHPCIC *pcic, target_phys_addr_t addr,
int size)
{
- return pci_data_read(pcic->bus, addr + pcic->mbr, size);
+ return pci_data_read(pcic->s.bus, addr + pcic->mbr, size);
}
static void sh_pci_writeb (void *p, target_phys_addr_t addr, uint32_t val)
@@ -176,10 +176,10 @@ PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
int mem, reg, iop;
p = qemu_mallocz(sizeof(SHPCIC));
- p->bus = pci_register_bus(NULL, "pci",
- set_irq, map_irq, opaque, devfn_min, nirq);
+ p->s.bus = pci_register_bus(NULL, "pci",
+ set_irq, map_irq, opaque, devfn_min, nirq);
- p->dev = pci_register_device(p->bus, "SH PCIC", sizeof(PCIDevice),
+ p->dev = pci_register_device(p->s.bus, "SH PCIC", sizeof(PCIDevice),
-1, NULL, NULL);
reg = cpu_register_io_memory(sh_pci_reg.r, sh_pci_reg.w, p);
iop = cpu_register_io_memory(sh_pci_iop.r, sh_pci_iop.w, p);
@@ -198,5 +198,5 @@ PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
p->dev->config[0x06] = 0x90;
p->dev->config[0x07] = 0x02;
- return p->bus;
+ return p->s.bus;
}
--
1.6.5.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 2/6] sh_pci: s/sh_pci_data_write/sh_pci_mem_write/g for consistency.
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 1/6] sh_pci: use PCIHostState instead of PCIBus Isaku Yamahata
@ 2010-01-12 8:52 ` Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 3/6] versatile_pci: user PCIHostState instead of PCIBus Isaku Yamahata
` (4 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 8:52 UTC (permalink / raw)
To: qemu-devel; +Cc: yamahata, Takashi YOSHII, agraf, mst
rename sh_pci_data_write to sh_pci_mem_write for consistency.
Later sh_pci_data_write will be introduced again, so this patch
is preliminary patch.
Cc: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/sh_pci.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index d76d390..046db2e 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -72,7 +72,7 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
return 0;
}
-static void sh_pci_data_write (SHPCIC *pcic, target_phys_addr_t addr,
+static void sh_pci_mem_write (SHPCIC *pcic, target_phys_addr_t addr,
uint32_t val, int size)
{
pci_data_write(pcic->s.bus, addr + pcic->mbr, val, size);
@@ -86,17 +86,17 @@ static uint32_t sh_pci_mem_read (SHPCIC *pcic, target_phys_addr_t addr,
static void sh_pci_writeb (void *p, target_phys_addr_t addr, uint32_t val)
{
- sh_pci_data_write(p, addr, val, 1);
+ sh_pci_mem_write(p, addr, val, 1);
}
static void sh_pci_writew (void *p, target_phys_addr_t addr, uint32_t val)
{
- sh_pci_data_write(p, addr, val, 2);
+ sh_pci_mem_write(p, addr, val, 2);
}
static void sh_pci_writel (void *p, target_phys_addr_t addr, uint32_t val)
{
- sh_pci_data_write(p, addr, val, 4);
+ sh_pci_mem_write(p, addr, val, 4);
}
static uint32_t sh_pci_readb (void *p, target_phys_addr_t addr)
--
1.6.5.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/6] versatile_pci: user PCIHostState instead of PCIBus
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 1/6] sh_pci: use PCIHostState instead of PCIBus Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 2/6] sh_pci: s/sh_pci_data_write/sh_pci_mem_write/g for consistency Isaku Yamahata
@ 2010-01-12 8:52 ` Isaku Yamahata
2010-01-13 13:02 ` Paul Brook
2010-01-12 8:52 ` [Qemu-devel] [PATCH 4/6] pci_host: remove code duplication in pci_host_template.h Isaku Yamahata
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 8:52 UTC (permalink / raw)
To: qemu-devel; +Cc: yamahata, Paul Brook, agraf, mst
To use pci host framework, use PCIHostState instead of PCIBus in PCIVPBState.
Cc: Paul Brook <paul@codesourcery.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/versatile_pci.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 153c651..d99b7fa 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -12,7 +12,7 @@
#include "pci_host.h"
typedef struct {
- SysBusDevice busdev;
+ PCIHostState pci_host;
qemu_irq irq[4];
int realview;
int mem_config;
@@ -26,38 +26,43 @@ static inline uint32_t vpb_pci_config_addr(target_phys_addr_t addr)
static void pci_vpb_config_writeb (void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- pci_data_write(opaque, vpb_pci_config_addr (addr), val, 1);
+ PCIHostState *s = opaque;
+ pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 1);
}
static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
uint32_t val)
{
+ PCIHostState *s = opaque;
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap16(val);
#endif
- pci_data_write(opaque, vpb_pci_config_addr (addr), val, 2);
+ pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 2);
}
static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
uint32_t val)
{
+ PCIHostState *s = opaque;
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap32(val);
#endif
- pci_data_write(opaque, vpb_pci_config_addr (addr), val, 4);
+ pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 4);
}
static uint32_t pci_vpb_config_readb (void *opaque, target_phys_addr_t addr)
{
+ PCIHostState *s = opaque;
uint32_t val;
- val = pci_data_read(opaque, vpb_pci_config_addr (addr), 1);
+ val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 1);
return val;
}
static uint32_t pci_vpb_config_readw (void *opaque, target_phys_addr_t addr)
{
+ PCIHostState *s = opaque;
uint32_t val;
- val = pci_data_read(opaque, vpb_pci_config_addr (addr), 2);
+ val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 2);
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap16(val);
#endif
@@ -66,8 +71,9 @@ static uint32_t pci_vpb_config_readw (void *opaque, target_phys_addr_t addr)
static uint32_t pci_vpb_config_readl (void *opaque, target_phys_addr_t addr)
{
+ PCIHostState *s = opaque;
uint32_t val;
- val = pci_data_read(opaque, vpb_pci_config_addr (addr), 4);
+ val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 4);
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap32(val);
#endif
@@ -114,7 +120,8 @@ static void pci_vpb_map(SysBusDevice *dev, target_phys_addr_t base)
static int pci_vpb_init(SysBusDevice *dev)
{
- PCIVPBState *s = FROM_SYSBUS(PCIVPBState, dev);
+ PCIHostState *pci_host = FROM_SYSBUS(PCIHostState, dev);
+ PCIVPBState *s = DO_UPCAST(PCIVPBState, pci_host, pci_host);
PCIBus *bus;
int i;
@@ -124,11 +131,12 @@ static int pci_vpb_init(SysBusDevice *dev)
bus = pci_register_bus(&dev->qdev, "pci",
pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
11 << 3, 4);
+ pci_host->bus = bus;
/* ??? Register memory space. */
s->mem_config = cpu_register_io_memory(pci_vpb_config_read,
- pci_vpb_config_write, bus);
+ pci_vpb_config_write, pci_host);
sysbus_init_mmio_cb(dev, 0x04000000, pci_vpb_map);
pci_create_simple(bus, -1, "versatile_pci_host");
@@ -137,7 +145,8 @@ static int pci_vpb_init(SysBusDevice *dev)
static int pci_realview_init(SysBusDevice *dev)
{
- PCIVPBState *s = FROM_SYSBUS(PCIVPBState, dev);
+ PCIHostState *pci_host = FROM_SYSBUS(PCIHostState, dev);
+ PCIVPBState *s = DO_UPCAST(PCIVPBState, pci_host, pci_host);
s->realview = 1;
return pci_vpb_init(dev);
}
--
1.6.5.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 4/6] pci_host: remove code duplication in pci_host_template.h
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
` (2 preceding siblings ...)
2010-01-12 8:52 ` [Qemu-devel] [PATCH 3/6] versatile_pci: user PCIHostState instead of PCIBus Isaku Yamahata
@ 2010-01-12 8:52 ` Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions Isaku Yamahata
` (2 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 8:52 UTC (permalink / raw)
To: qemu-devel; +Cc: yamahata, agraf, mst
Remove code duplication in pci_host_template.h
by using cpp trick.
Based on Alexander Graf patch.
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/pci_host.c | 4 +-
hw/pci_host_template.h | 82 +++++++++----------------------------------
hw/pci_host_template_all.h | 23 ++++++++++++
3 files changed, 42 insertions(+), 67 deletions(-)
create mode 100644 hw/pci_host_template_all.h
diff --git a/hw/pci_host.c b/hw/pci_host.c
index 6289ead..307f7d4 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -190,7 +190,7 @@ void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
#define PCI_ADDR_T target_phys_addr_t
#define PCI_HOST_SUFFIX _mmio
-#include "pci_host_template.h"
+#include "pci_host_template_all.h"
static CPUWriteMemoryFunc * const pci_host_data_write_mmio[] = {
pci_host_data_writeb_mmio,
@@ -217,7 +217,7 @@ int pci_host_data_register_mmio(PCIHostState *s)
#define PCI_ADDR_T uint32_t
#define PCI_HOST_SUFFIX _ioport
-#include "pci_host_template.h"
+#include "pci_host_template_all.h"
void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
{
diff --git a/hw/pci_host_template.h b/hw/pci_host_template.h
index 11e6c88..2c508bf 100644
--- a/hw/pci_host_template.h
+++ b/hw/pci_host_template.h
@@ -25,85 +25,37 @@
/* Worker routines for a PCI host controller that uses an {address,data}
register pair to access PCI configuration space. */
-static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)(
+static void glue(glue(pci_host_data_write, PCI_HOST_BWL), PCI_HOST_SUFFIX)(
void* opaque, PCI_ADDR_T addr, uint32_t val)
{
PCIHostState *s = opaque;
- PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n",
- (target_phys_addr_t)addr, val);
- if (s->config_reg & (1u << 31))
- pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
-}
-
-static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
- void* opaque, PCI_ADDR_T addr, uint32_t val)
-{
- PCIHostState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
- val = bswap16(val);
-#endif
- PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n",
- (target_phys_addr_t)addr, val);
- if (s->config_reg & (1u << 31))
- pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
-}
-
-static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
- void* opaque, PCI_ADDR_T addr, uint32_t val)
-{
- PCIHostState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
- val = bswap32(val);
+#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
+ val = glue(bswap, PCI_HOST_BITS)(val);
#endif
- PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n",
- (target_phys_addr_t)addr, val);
+ PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
+ __func__, (target_phys_addr_t)addr, val);
if (s->config_reg & (1u << 31))
- pci_data_write(s->bus, s->config_reg, val, 4);
+ pci_data_write(s->bus, s->config_reg | (addr & 3), val,
+ sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
}
-static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
+static uint32_t glue(glue(pci_host_data_read, PCI_HOST_BWL), PCI_HOST_SUFFIX)(
void* opaque, PCI_ADDR_T addr)
{
PCIHostState *s = opaque;
uint32_t val;
- if (!(s->config_reg & (1 << 31)))
- return 0xff;
- val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
- PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
- (target_phys_addr_t)addr, val);
- return val;
-}
+ if (!(s->config_reg & (1 << 31))) {
+ return ( 1ULL << PCI_HOST_BITS ) - 1;
+ }
-static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)(
- void* opaque, PCI_ADDR_T addr)
-{
- PCIHostState *s = opaque;
- uint32_t val;
- if (!(s->config_reg & (1 << 31)))
- return 0xffff;
- val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
- PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
- (target_phys_addr_t)addr, val);
-#ifdef TARGET_WORDS_BIGENDIAN
- val = bswap16(val);
-#endif
- return val;
-}
-
-static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)(
- void* opaque, PCI_ADDR_T addr)
-{
- PCIHostState *s = opaque;
- uint32_t val;
- if (!(s->config_reg & (1 << 31)))
- return 0xffffffff;
- val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
- PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
- (target_phys_addr_t)addr, val);
-#ifdef TARGET_WORDS_BIGENDIAN
- val = bswap32(val);
+ val = pci_data_read(s->bus, s->config_reg | (addr & 3),
+ sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
+ PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %x\n",
+ __func__, (target_phys_addr_t)addr, val);
+#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
+ val = glue(bswap, PCI_HOST_BITS)(val);
#endif
return val;
}
diff --git a/hw/pci_host_template_all.h b/hw/pci_host_template_all.h
new file mode 100644
index 0000000..74b3e84
--- /dev/null
+++ b/hw/pci_host_template_all.h
@@ -0,0 +1,23 @@
+#define PCI_HOST_BWL b
+#define PCI_HOST_BITS 8
+
+#include "pci_host_template.h"
+
+#undef PCI_HOST_BWL
+#undef PCI_HOST_BITS
+
+#define PCI_HOST_BWL w
+#define PCI_HOST_BITS 16
+
+#include "pci_host_template.h"
+
+#undef PCI_HOST_BWL
+#undef PCI_HOST_BITS
+
+#define PCI_HOST_BWL l
+#define PCI_HOST_BITS 32
+
+#include "pci_host_template.h"
+
+#undef PCI_HOST_BWL
+#undef PCI_HOST_BITS
--
1.6.5.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions.
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
` (3 preceding siblings ...)
2010-01-12 8:52 ` [Qemu-devel] [PATCH 4/6] pci_host: remove code duplication in pci_host_template.h Isaku Yamahata
@ 2010-01-12 8:52 ` Isaku Yamahata
2010-01-12 10:04 ` [Qemu-devel] " Michael S. Tsirkin
2010-01-12 10:06 ` Michael S. Tsirkin
2010-01-12 8:52 ` [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress Isaku Yamahata
2010-01-12 10:18 ` [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up Michael S. Tsirkin
6 siblings, 2 replies; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 8:52 UTC (permalink / raw)
To: qemu-devel; +Cc: yamahata, agraf, mst
Introduce PCIAddress, PCIConfigAddress and helper functions.
They will be used later to clean up pci_data_{read, write}().
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/pci.h | 7 +++++++
hw/pci_host.c | 32 ++++++++++++++++++++++++++++++++
hw/pci_host.h | 16 ++++++++++++++++
qemu-common.h | 2 ++
4 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/hw/pci.h b/hw/pci.h
index ed048f5..eb87762 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -10,6 +10,13 @@
/* PCI bus */
+struct PCIAddress {
+ PCIBus *domain;
+ uint8_t bus;
+ uint8_t slot;
+ uint8_t fn;
+};
+
#define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
#define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
#define PCI_FUNC(devfn) ((devfn) & 0x07)
diff --git a/hw/pci_host.c b/hw/pci_host.c
index 307f7d4..fa194e2 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -39,6 +39,38 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
* bit 0 - 7: offset in configuration space of a given pci device
*/
+static void pci_host_decode_config_addr(const PCIHostState *s,
+ uint32_t config_reg,
+ PCIConfigAddress *decoded)
+{
+ uint32_t devfn;
+
+ decoded->addr.domain = s->bus;
+ decoded->addr.bus = (config_reg >> 16) & 0xff;
+ devfn = (config_reg >> 8) & 0xff;
+ decoded->addr.slot = PCI_SLOT(devfn);
+ decoded->addr.fn = PCI_FUNC(devfn);
+ decoded->offset = config_reg & (PCI_CONFIG_SPACE_SIZE - 1);
+ decoded->addr_mask = 3;
+}
+
+#define PCI_HOST_CFGE (1u << 31) /* configuration enable */
+void pci_host_decode_config_addr_cfge(const PCIHostState *s,
+ uint32_t config_reg,
+ PCIConfigAddress *decoded)
+{
+ pci_host_decode_config_addr(s, config_reg, decoded);
+ decoded->valid = (config_reg & PCI_HOST_CFGE) ? true : false;
+}
+
+void pci_host_decode_config_addr_valid(const PCIHostState *s,
+ uint32_t config_reg,
+ PCIConfigAddress *decoded)
+{
+ pci_host_decode_config_addr(s, config_reg, decoded);
+ decoded->valid = true;
+}
+
/* the helper functio to get a PCIDeice* for a given pci address */
static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
{
diff --git a/hw/pci_host.h b/hw/pci_host.h
index a006687..ebc95f2 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -30,12 +30,28 @@
#include "sysbus.h"
+/* for config space access */
+struct PCIConfigAddress {
+ PCIAddress addr;
+ uint32_t addr_mask;
+ uint16_t offset;
+ bool valid;
+};
+
struct PCIHostState {
SysBusDevice busdev;
uint32_t config_reg;
PCIBus *bus;
};
+void pci_host_decode_config_addr_cfge(const PCIHostState *s,
+ uint32_t config_reg,
+ PCIConfigAddress *decoded);
+
+void pci_host_decode_config_addr_valid(const PCIHostState *s,
+ uint32_t config_reg,
+ PCIConfigAddress *decoded);
+
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);
diff --git a/qemu-common.h b/qemu-common.h
index 8630f8c..14e9205 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -207,6 +207,8 @@ typedef struct SMBusDevice SMBusDevice;
typedef struct QEMUTimer QEMUTimer;
typedef struct PCIHostState PCIHostState;
typedef struct PCIExpressHost PCIExpressHost;
+typedef struct PCIAddress PCIAddress;
+typedef struct PCIConfigAddress PCIConfigAddress;
typedef struct PCIBus PCIBus;
typedef struct PCIDevice PCIDevice;
typedef struct SerialState SerialState;
--
1.6.5.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
` (4 preceding siblings ...)
2010-01-12 8:52 ` [Qemu-devel] [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions Isaku Yamahata
@ 2010-01-12 8:52 ` Isaku Yamahata
2010-01-12 10:12 ` [Qemu-devel] " Michael S. Tsirkin
2010-01-12 17:54 ` [Qemu-devel] " Alexander Graf
2010-01-12 10:18 ` [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up Michael S. Tsirkin
6 siblings, 2 replies; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 8:52 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, agraf, Blue Swirl, yamahata, Paul Brook, Aurelien Jarno
Move out pci address decoding logic from pci_data_{write, read}()
by making pci_data_{write, read}() get PCIConfigAddress.
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Paul Brook <paul@codesourcery.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/apb_pci.c | 12 ++++++++----
hw/gt64xxx.c | 20 ++++++++++++--------
hw/pci_host.c | 25 ++++++++++++++-----------
hw/pci_host.h | 5 +++--
hw/pci_host_template.h | 15 +++++++--------
hw/prep_pci.c | 28 ++++++++++++++++++++++------
hw/sh_pci.c | 24 ++++++++++++++++++++----
hw/versatile_pci.c | 30 ++++++++++++++++++++++++------
8 files changed, 110 insertions(+), 49 deletions(-)
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index c14e1c0..3c098d2 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -107,18 +107,22 @@ static CPUReadMemoryFunc * const apb_config_read[] = {
static void apb_pci_config_write(APBState *s, target_phys_addr_t addr,
uint32_t val, int size)
{
+ PCIConfigAddress conf_addr;
APB_DPRINTF("%s: addr " TARGET_FMT_lx " val %x\n", __func__, addr, val);
- pci_data_write(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31), val,
- size);
+ pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
+ &conf_addr);
+ pci_data_write(&conf_addr, 0, val, size);
}
static uint32_t apb_pci_config_read(APBState *s, target_phys_addr_t addr,
int size)
{
+ PCIConfigAddress conf_addr;
uint32_t ret;
- ret = pci_data_read(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31),
- size);
+ pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
+ &conf_addr);
+ ret = pci_data_read(&conf_addr, 0, size);
APB_DPRINTF("%s: addr " TARGET_FMT_lx " -> %x\n", __func__, addr, ret);
return ret;
}
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index c8034e2..2e135dc 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -527,12 +527,15 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
case GT_PCI0_CFGADDR:
s->pci->config_reg = val & 0x80fffffc;
break;
- case GT_PCI0_CFGDATA:
+ case GT_PCI0_CFGDATA: {
+ PCIConfigAddress conf_addr;
if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
val = bswap32(val);
- if (s->pci->config_reg & (1u << 31))
- pci_data_write(s->pci->bus, s->pci->config_reg, val, 4);
+ pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
+ &conf_addr);
+ pci_data_write(&conf_addr, 0, val, 4);
break;
+ }
/* Interrupts */
case GT_INTRCAUSE:
@@ -767,14 +770,15 @@ static uint32_t gt64120_readl (void *opaque,
case GT_PCI0_CFGADDR:
val = s->pci->config_reg;
break;
- case GT_PCI0_CFGDATA:
- if (!(s->pci->config_reg & (1 << 31)))
- val = 0xffffffff;
- else
- val = pci_data_read(s->pci->bus, s->pci->config_reg, 4);
+ case GT_PCI0_CFGDATA: {
+ PCIConfigAddress conf_addr;
+ pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
+ &conf_addr);
+ val = pci_data_read(&conf_addr, 0, 4);
if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
val = bswap32(val);
break;
+ }
case GT_PCI0_CMD:
case GT_PCI0_TOR:
diff --git a/hw/pci_host.c b/hw/pci_host.c
index fa194e2..c837110 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -72,18 +72,21 @@ void pci_host_decode_config_addr_valid(const PCIHostState *s,
}
/* the helper functio to get a PCIDeice* for a given pci address */
-static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
+static inline PCIDevice *pci_dev_find_by_addr(
+ const PCIConfigAddress *conf_addr)
{
- uint8_t bus_num = addr >> 16;
- uint8_t devfn = addr >> 8;
-
- return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
+ const PCIAddress *addr = &conf_addr->addr;
+ if (!conf_addr->valid) {
+ return NULL;
+ }
+ return pci_find_device(addr->domain, addr->bus, addr->slot, addr->fn);
}
-void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
+void pci_data_write(PCIConfigAddress *conf_addr,
+ uint32_t addr, uint32_t val, int len)
{
- PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
- uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
+ PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
+ uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
if (!pci_dev)
return;
@@ -93,10 +96,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
pci_dev->config_write(pci_dev, config_addr, val, len);
}
-uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
+uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len)
{
- PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
- uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
+ PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
+ uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
uint32_t val;
assert(len == 1 || len == 2 || len == 4);
diff --git a/hw/pci_host.h b/hw/pci_host.h
index ebc95f2..d2d558d 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -52,8 +52,9 @@ void pci_host_decode_config_addr_valid(const PCIHostState *s,
uint32_t config_reg,
PCIConfigAddress *decoded);
-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);
+void pci_data_write(PCIConfigAddress *conf_addr,
+ uint32_t addr, uint32_t val, int len);
+uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len);
/* for mmio */
int pci_host_conf_register_mmio(PCIHostState *s);
diff --git a/hw/pci_host_template.h b/hw/pci_host_template.h
index 2c508bf..02f0ff1 100644
--- a/hw/pci_host_template.h
+++ b/hw/pci_host_template.h
@@ -29,28 +29,27 @@ static void glue(glue(pci_host_data_write, PCI_HOST_BWL), PCI_HOST_SUFFIX)(
void* opaque, PCI_ADDR_T addr, uint32_t val)
{
PCIHostState *s = opaque;
+ PCIConfigAddress conf_addr;
#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
val = glue(bswap, PCI_HOST_BITS)(val);
#endif
PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
__func__, (target_phys_addr_t)addr, val);
- if (s->config_reg & (1u << 31))
- pci_data_write(s->bus, s->config_reg | (addr & 3), val,
- sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
+ pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
+ pci_data_write(&conf_addr, addr, val,
+ sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
}
static uint32_t glue(glue(pci_host_data_read, PCI_HOST_BWL), PCI_HOST_SUFFIX)(
void* opaque, PCI_ADDR_T addr)
{
PCIHostState *s = opaque;
+ PCIConfigAddress conf_addr;
uint32_t val;
- if (!(s->config_reg & (1 << 31))) {
- return ( 1ULL << PCI_HOST_BITS ) - 1;
- }
-
- val = pci_data_read(s->bus, s->config_reg | (addr & 3),
+ pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
+ val = pci_data_read(&conf_addr, addr,
sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %x\n",
__func__, (target_phys_addr_t)addr, val);
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 19f028c..53862d7 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -40,10 +40,26 @@ static inline uint32_t PPC_PCIIO_config(target_phys_addr_t addr)
return (addr & 0x7ff) | (i << 11);
}
+static void PPC_pci_data_write(PREPPCIState *s, target_phys_addr_t addr,
+ uint32_t val, int len)
+{
+ PCIConfigAddress conf_addr;
+ pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
+ pci_data_write(&conf_addr, 0, val, len);
+}
+
+static uint32_t PPC_pci_data_read(PREPPCIState *s, target_phys_addr_t addr,
+ int len)
+{
+ PCIConfigAddress conf_addr;
+ pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
+ return pci_data_read(&conf_addr, 0, len);
+}
+
static void PPC_PCIIO_writeb (void *opaque, target_phys_addr_t addr, uint32_t val)
{
PREPPCIState *s = opaque;
- pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 1);
+ PPC_pci_data_write(s, addr, val, 1);
}
static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr, uint32_t val)
@@ -52,7 +68,7 @@ static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr, uint32_t va
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap16(val);
#endif
- pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 2);
+ PPC_pci_data_write(s, addr, val, 2);
}
static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr, uint32_t val)
@@ -61,14 +77,14 @@ static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr, uint32_t va
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap32(val);
#endif
- pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 4);
+ PPC_pci_data_write(s, addr, val, 4);
}
static uint32_t PPC_PCIIO_readb (void *opaque, target_phys_addr_t addr)
{
PREPPCIState *s = opaque;
uint32_t val;
- val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 1);
+ val = PPC_pci_data_read(s, addr, 1);
return val;
}
@@ -76,7 +92,7 @@ static uint32_t PPC_PCIIO_readw (void *opaque, target_phys_addr_t addr)
{
PREPPCIState *s = opaque;
uint32_t val;
- val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 2);
+ val = PPC_pci_data_read(s, addr, 2);
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap16(val);
#endif
@@ -87,7 +103,7 @@ static uint32_t PPC_PCIIO_readl (void *opaque, target_phys_addr_t addr)
{
PREPPCIState *s = opaque;
uint32_t val;
- val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 4);
+ val = PPC_pci_data_read(s, addr, 4);
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap32(val);
#endif
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index 046db2e..2a8f132 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -36,6 +36,22 @@ typedef struct {
uint32_t iobr;
} SHPCIC;
+static void sh_pci_data_write(SHPCIC *pcic, target_phys_addr_t addr,
+ uint32_t val, int size)
+{
+ PCIConfigAddress conf_addr;
+ pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
+ pci_data_write(&conf_addr, 0, val, size);
+}
+
+static uint32_t sh_pci_data_read(SHPCIC *pcic,
+ target_phys_addr_t addr, int size)
+{
+ PCIConfigAddress conf_addr;
+ pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
+ return pci_data_read(&conf_addr, 0, size);
+}
+
static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
{
SHPCIC *pcic = p;
@@ -53,7 +69,7 @@ static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
pcic->iobr = val;
break;
case 0x220:
- pci_data_write(pcic->s.bus, pcic->par, val, 4);
+ sh_pci_data_write(pcic, pcic->par, val, 4);
break;
}
}
@@ -67,7 +83,7 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
case 0x1c0:
return pcic->par;
case 0x220:
- return pci_data_read(pcic->s.bus, pcic->par, 4);
+ return sh_pci_data_read(pcic, pcic->par, 4);
}
return 0;
}
@@ -75,13 +91,13 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
static void sh_pci_mem_write (SHPCIC *pcic, target_phys_addr_t addr,
uint32_t val, int size)
{
- pci_data_write(pcic->s.bus, addr + pcic->mbr, val, size);
+ sh_pci_data_write(pcic, addr + pcic->mbr, val, size);
}
static uint32_t sh_pci_mem_read (SHPCIC *pcic, target_phys_addr_t addr,
int size)
{
- return pci_data_read(pcic->s.bus, addr + pcic->mbr, size);
+ return sh_pci_data_read(pcic, addr + pcic->mbr, size);
}
static void sh_pci_writeb (void *p, target_phys_addr_t addr, uint32_t val)
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index d99b7fa..14a728a 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -23,11 +23,29 @@ static inline uint32_t vpb_pci_config_addr(target_phys_addr_t addr)
return addr & 0xffffff;
}
+static void vpb_pci_data_write(PCIHostState *s, target_phys_addr_t addr,
+ uint32_t val, int len)
+{
+ PCIConfigAddress conf_addr;
+ pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
+ &conf_addr);
+ pci_data_write(&conf_addr, 0, val, len);
+}
+
+static uint32_t vpb_pci_data_read(PCIHostState *s, target_phys_addr_t addr,
+ int len)
+{
+ PCIConfigAddress conf_addr;
+ pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
+ &conf_addr);
+ return pci_data_read(&conf_addr, 0, len);
+}
+
static void pci_vpb_config_writeb (void *opaque, target_phys_addr_t addr,
uint32_t val)
{
PCIHostState *s = opaque;
- pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 1);
+ vpb_pci_data_write(s, addr, val, 1);
}
static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
@@ -37,7 +55,7 @@ static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap16(val);
#endif
- pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 2);
+ vpb_pci_data_write(s, addr, val, 2);
}
static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
@@ -47,14 +65,14 @@ static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap32(val);
#endif
- pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 4);
+ vpb_pci_data_write(s, addr, val, 4);
}
static uint32_t pci_vpb_config_readb (void *opaque, target_phys_addr_t addr)
{
PCIHostState *s = opaque;
uint32_t val;
- val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 1);
+ val = vpb_pci_data_read(s, addr, 1);
return val;
}
@@ -62,7 +80,7 @@ static uint32_t pci_vpb_config_readw (void *opaque, target_phys_addr_t addr)
{
PCIHostState *s = opaque;
uint32_t val;
- val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 2);
+ val = vpb_pci_data_read(s, addr, 2);
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap16(val);
#endif
@@ -73,7 +91,7 @@ static uint32_t pci_vpb_config_readl (void *opaque, target_phys_addr_t addr)
{
PCIHostState *s = opaque;
uint32_t val;
- val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 4);
+ val = vpb_pci_data_read(s, addr, 4);
#ifdef TARGET_WORDS_BIGENDIAN
val = bswap32(val);
#endif
--
1.6.5.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions.
2010-01-12 8:52 ` [Qemu-devel] [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions Isaku Yamahata
@ 2010-01-12 10:04 ` Michael S. Tsirkin
2010-01-12 10:06 ` Michael S. Tsirkin
1 sibling, 0 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2010-01-12 10:04 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: qemu-devel, agraf
On Tue, Jan 12, 2010 at 05:52:57PM +0900, Isaku Yamahata wrote:
> Introduce PCIAddress, PCIConfigAddress and helper functions.
> They will be used later to clean up pci_data_{read, write}().
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> hw/pci.h | 7 +++++++
> hw/pci_host.c | 32 ++++++++++++++++++++++++++++++++
> hw/pci_host.h | 16 ++++++++++++++++
> qemu-common.h | 2 ++
> 4 files changed, 57 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pci.h b/hw/pci.h
> index ed048f5..eb87762 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -10,6 +10,13 @@
>
> /* PCI bus */
>
> +struct PCIAddress {
> + PCIBus *domain;
> + uint8_t bus;
> + uint8_t slot;
> + uint8_t fn;
> +};
> +
This API looks good except probably should be in pci_host.h
and called PCIConfigAddress.
> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
> #define PCI_FUNC(devfn) ((devfn) & 0x07)
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 307f7d4..fa194e2 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -39,6 +39,38 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> * bit 0 - 7: offset in configuration space of a given pci device
> */
>
> +static void pci_host_decode_config_addr(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded)
> +{
> + uint32_t devfn;
> +
> + decoded->addr.domain = s->bus;
> + decoded->addr.bus = (config_reg >> 16) & 0xff;
> + devfn = (config_reg >> 8) & 0xff;
> + decoded->addr.slot = PCI_SLOT(devfn);
> + decoded->addr.fn = PCI_FUNC(devfn);
> + decoded->offset = config_reg & (PCI_CONFIG_SPACE_SIZE - 1);
> + decoded->addr_mask = 3;
> +}
> +
> +#define PCI_HOST_CFGE (1u << 31) /* configuration enable */
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded)
> +{
> + pci_host_decode_config_addr(s, config_reg, decoded);
> + decoded->valid = (config_reg & PCI_HOST_CFGE) ? true : false;
> +}
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded)
> +{
> + pci_host_decode_config_addr(s, config_reg, decoded);
> + decoded->valid = true;
> +}
> +
> /* the helper functio to get a PCIDeice* for a given pci address */
> static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
> {
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index a006687..ebc95f2 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -30,12 +30,28 @@
>
> #include "sysbus.h"
>
> +/* for config space access */
> +struct PCIConfigAddress {
> + PCIAddress addr;
> + uint32_t addr_mask;
> + uint16_t offset;
> + bool valid;
> +};
> +
This one seems to add ad-hock stuff that can be
calculated by pci_data_read/write just as well.
> struct PCIHostState {
> SysBusDevice busdev;
> uint32_t config_reg;
> PCIBus *bus;
> };
>
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded);
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded);
> +
So if you rename PCIAddress to PCIConfigAddress and get rid of
the extra fields, you will end up with a single
pci_host_get_config_addr, which is cleaner.
> 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);
>
> diff --git a/qemu-common.h b/qemu-common.h
> index 8630f8c..14e9205 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -207,6 +207,8 @@ typedef struct SMBusDevice SMBusDevice;
> typedef struct QEMUTimer QEMUTimer;
> typedef struct PCIHostState PCIHostState;
> typedef struct PCIExpressHost PCIExpressHost;
> +typedef struct PCIAddress PCIAddress;
> +typedef struct PCIConfigAddress PCIConfigAddress;
> typedef struct PCIBus PCIBus;
> typedef struct PCIDevice PCIDevice;
> typedef struct SerialState SerialState;
> --
> 1.6.5.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions.
2010-01-12 8:52 ` [Qemu-devel] [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions Isaku Yamahata
2010-01-12 10:04 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-01-12 10:06 ` Michael S. Tsirkin
1 sibling, 0 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2010-01-12 10:06 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: qemu-devel, agraf
On Tue, Jan 12, 2010 at 05:52:57PM +0900, Isaku Yamahata wrote:
> Introduce PCIAddress, PCIConfigAddress and helper functions.
> They will be used later to clean up pci_data_{read, write}().
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> hw/pci.h | 7 +++++++
> hw/pci_host.c | 32 ++++++++++++++++++++++++++++++++
> hw/pci_host.h | 16 ++++++++++++++++
> qemu-common.h | 2 ++
> 4 files changed, 57 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pci.h b/hw/pci.h
> index ed048f5..eb87762 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -10,6 +10,13 @@
>
> /* PCI bus */
>
> +struct PCIAddress {
> + PCIBus *domain;
> + uint8_t bus;
> + uint8_t slot;
> + uint8_t fn;
> +};
> +
> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
> #define PCI_FUNC(devfn) ((devfn) & 0x07)
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 307f7d4..fa194e2 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -39,6 +39,38 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> * bit 0 - 7: offset in configuration space of a given pci device
> */
>
> +static void pci_host_decode_config_addr(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded)
> +{
> + uint32_t devfn;
> +
> + decoded->addr.domain = s->bus;
Hang on, this is wrong. Domain is the root complex, if you need the bus
pointer just pass it to pci_data_read directly.
> + decoded->addr.bus = (config_reg >> 16) & 0xff;
> + devfn = (config_reg >> 8) & 0xff;
> + decoded->addr.slot = PCI_SLOT(devfn);
> + decoded->addr.fn = PCI_FUNC(devfn);
> + decoded->offset = config_reg & (PCI_CONFIG_SPACE_SIZE - 1);
> + decoded->addr_mask = 3;
> +}
> +
> +#define PCI_HOST_CFGE (1u << 31) /* configuration enable */
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded)
> +{
> + pci_host_decode_config_addr(s, config_reg, decoded);
> + decoded->valid = (config_reg & PCI_HOST_CFGE) ? true : false;
> +}
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded)
> +{
> + pci_host_decode_config_addr(s, config_reg, decoded);
> + decoded->valid = true;
> +}
> +
> /* the helper functio to get a PCIDeice* for a given pci address */
> static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
> {
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index a006687..ebc95f2 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -30,12 +30,28 @@
>
> #include "sysbus.h"
>
> +/* for config space access */
> +struct PCIConfigAddress {
> + PCIAddress addr;
> + uint32_t addr_mask;
> + uint16_t offset;
> + bool valid;
> +};
> +
> struct PCIHostState {
> SysBusDevice busdev;
> uint32_t config_reg;
> PCIBus *bus;
> };
>
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded);
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> + uint32_t config_reg,
> + PCIConfigAddress *decoded);
> +
> 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);
>
> diff --git a/qemu-common.h b/qemu-common.h
> index 8630f8c..14e9205 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -207,6 +207,8 @@ typedef struct SMBusDevice SMBusDevice;
> typedef struct QEMUTimer QEMUTimer;
> typedef struct PCIHostState PCIHostState;
> typedef struct PCIExpressHost PCIExpressHost;
> +typedef struct PCIAddress PCIAddress;
> +typedef struct PCIConfigAddress PCIConfigAddress;
> typedef struct PCIBus PCIBus;
> typedef struct PCIDevice PCIDevice;
> typedef struct SerialState SerialState;
> --
> 1.6.5.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 8:52 ` [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress Isaku Yamahata
@ 2010-01-12 10:12 ` Michael S. Tsirkin
2010-01-12 10:43 ` Isaku Yamahata
2010-01-13 13:08 ` Paul Brook
2010-01-12 17:54 ` [Qemu-devel] " Alexander Graf
1 sibling, 2 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2010-01-12 10:12 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: Blue Swirl, Paul Brook, qemu-devel, Aurelien Jarno, agraf
On Tue, Jan 12, 2010 at 05:52:58PM +0900, Isaku Yamahata wrote:
> Move out pci address decoding logic from pci_data_{write, read}()
> by making pci_data_{write, read}() get PCIConfigAddress.
>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Paul Brook <paul@codesourcery.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> hw/apb_pci.c | 12 ++++++++----
> hw/gt64xxx.c | 20 ++++++++++++--------
> hw/pci_host.c | 25 ++++++++++++++-----------
> hw/pci_host.h | 5 +++--
> hw/pci_host_template.h | 15 +++++++--------
> hw/prep_pci.c | 28 ++++++++++++++++++++++------
> hw/sh_pci.c | 24 ++++++++++++++++++++----
> hw/versatile_pci.c | 30 ++++++++++++++++++++++++------
> 8 files changed, 110 insertions(+), 49 deletions(-)
>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index c14e1c0..3c098d2 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -107,18 +107,22 @@ static CPUReadMemoryFunc * const apb_config_read[] = {
> static void apb_pci_config_write(APBState *s, target_phys_addr_t addr,
> uint32_t val, int size)
> {
> + PCIConfigAddress conf_addr;
> APB_DPRINTF("%s: addr " TARGET_FMT_lx " val %x\n", __func__, addr, val);
> - pci_data_write(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31), val,
> - size);
> + pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
> + &conf_addr);
> + pci_data_write(&conf_addr, 0, val, size);
> }
>
> static uint32_t apb_pci_config_read(APBState *s, target_phys_addr_t addr,
> int size)
> {
> + PCIConfigAddress conf_addr;
> uint32_t ret;
>
> - ret = pci_data_read(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31),
> - size);
> + pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
> + &conf_addr);
> + ret = pci_data_read(&conf_addr, 0, size);
> APB_DPRINTF("%s: addr " TARGET_FMT_lx " -> %x\n", __func__, addr, ret);
> return ret;
> }
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index c8034e2..2e135dc 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -527,12 +527,15 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
> case GT_PCI0_CFGADDR:
> s->pci->config_reg = val & 0x80fffffc;
> break;
> - case GT_PCI0_CFGDATA:
> + case GT_PCI0_CFGDATA: {
> + PCIConfigAddress conf_addr;
> if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
> val = bswap32(val);
> - if (s->pci->config_reg & (1u << 31))
> - pci_data_write(s->pci->bus, s->pci->config_reg, val, 4);
> + pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
> + &conf_addr);
> + pci_data_write(&conf_addr, 0, val, 4);
> break;
> + }
>
> /* Interrupts */
> case GT_INTRCAUSE:
> @@ -767,14 +770,15 @@ static uint32_t gt64120_readl (void *opaque,
> case GT_PCI0_CFGADDR:
> val = s->pci->config_reg;
> break;
> - case GT_PCI0_CFGDATA:
> - if (!(s->pci->config_reg & (1 << 31)))
> - val = 0xffffffff;
> - else
> - val = pci_data_read(s->pci->bus, s->pci->config_reg, 4);
> + case GT_PCI0_CFGDATA: {
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
> + &conf_addr);
> + val = pci_data_read(&conf_addr, 0, 4);
> if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
> val = bswap32(val);
> break;
> + }
>
> case GT_PCI0_CMD:
> case GT_PCI0_TOR:
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index fa194e2..c837110 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -72,18 +72,21 @@ void pci_host_decode_config_addr_valid(const PCIHostState *s,
> }
>
> /* the helper functio to get a PCIDeice* for a given pci address */
> -static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
> +static inline PCIDevice *pci_dev_find_by_addr(
> + const PCIConfigAddress *conf_addr)
> {
> - uint8_t bus_num = addr >> 16;
> - uint8_t devfn = addr >> 8;
> -
> - return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
> + const PCIAddress *addr = &conf_addr->addr;
> + if (!conf_addr->valid) {
> + return NULL;
> + }
> + return pci_find_device(addr->domain, addr->bus, addr->slot, addr->fn);
> }
>
> -void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> +void pci_data_write(PCIConfigAddress *conf_addr,
> + uint32_t addr, uint32_t val, int len)
> {
> - PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> + PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
> + uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
>
> if (!pci_dev)
> return;
> @@ -93,10 +96,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> pci_dev->config_write(pci_dev, config_addr, val, len);
> }
>
> -uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> +uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len)
> {
> - PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> + PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
> + uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
> uint32_t val;
>
> assert(len == 1 || len == 2 || len == 4);
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index ebc95f2..d2d558d 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -52,8 +52,9 @@ void pci_host_decode_config_addr_valid(const PCIHostState *s,
> uint32_t config_reg,
> PCIConfigAddress *decoded);
>
> -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);
> +void pci_data_write(PCIConfigAddress *conf_addr,
> + uint32_t addr, uint32_t val, int len);
> +uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len);
>
> /* for mmio */
> int pci_host_conf_register_mmio(PCIHostState *s);
> diff --git a/hw/pci_host_template.h b/hw/pci_host_template.h
> index 2c508bf..02f0ff1 100644
> --- a/hw/pci_host_template.h
> +++ b/hw/pci_host_template.h
> @@ -29,28 +29,27 @@ static void glue(glue(pci_host_data_write, PCI_HOST_BWL), PCI_HOST_SUFFIX)(
> void* opaque, PCI_ADDR_T addr, uint32_t val)
> {
> PCIHostState *s = opaque;
> + PCIConfigAddress conf_addr;
>
> #if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
> val = glue(bswap, PCI_HOST_BITS)(val);
> #endif
> PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
> __func__, (target_phys_addr_t)addr, val);
> - if (s->config_reg & (1u << 31))
> - pci_data_write(s->bus, s->config_reg | (addr & 3), val,
> - sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
> + pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
> + pci_data_write(&conf_addr, addr, val,
> + sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
> }
>
> static uint32_t glue(glue(pci_host_data_read, PCI_HOST_BWL), PCI_HOST_SUFFIX)(
> void* opaque, PCI_ADDR_T addr)
> {
> PCIHostState *s = opaque;
> + PCIConfigAddress conf_addr;
> uint32_t val;
>
> - if (!(s->config_reg & (1 << 31))) {
> - return ( 1ULL << PCI_HOST_BITS ) - 1;
> - }
> -
> - val = pci_data_read(s->bus, s->config_reg | (addr & 3),
> + pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
> + val = pci_data_read(&conf_addr, addr,
> sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %x\n",
> __func__, (target_phys_addr_t)addr, val);
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 19f028c..53862d7 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -40,10 +40,26 @@ static inline uint32_t PPC_PCIIO_config(target_phys_addr_t addr)
> return (addr & 0x7ff) | (i << 11);
> }
>
> +static void PPC_pci_data_write(PREPPCIState *s, target_phys_addr_t addr,
> + uint32_t val, int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
> + pci_data_write(&conf_addr, 0, val, len);
> +}
> +
> +static uint32_t PPC_pci_data_read(PREPPCIState *s, target_phys_addr_t addr,
> + int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
> + return pci_data_read(&conf_addr, 0, len);
> +}
> +
> static void PPC_PCIIO_writeb (void *opaque, target_phys_addr_t addr, uint32_t val)
> {
> PREPPCIState *s = opaque;
> - pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 1);
> + PPC_pci_data_write(s, addr, val, 1);
> }
>
> static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr, uint32_t val)
> @@ -52,7 +68,7 @@ static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr, uint32_t va
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> - pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 2);
> + PPC_pci_data_write(s, addr, val, 2);
> }
>
> static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr, uint32_t val)
> @@ -61,14 +77,14 @@ static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr, uint32_t va
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> - pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 4);
> + PPC_pci_data_write(s, addr, val, 4);
> }
>
> static uint32_t PPC_PCIIO_readb (void *opaque, target_phys_addr_t addr)
> {
> PREPPCIState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 1);
> + val = PPC_pci_data_read(s, addr, 1);
> return val;
> }
>
> @@ -76,7 +92,7 @@ static uint32_t PPC_PCIIO_readw (void *opaque, target_phys_addr_t addr)
> {
> PREPPCIState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 2);
> + val = PPC_pci_data_read(s, addr, 2);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> @@ -87,7 +103,7 @@ static uint32_t PPC_PCIIO_readl (void *opaque, target_phys_addr_t addr)
> {
> PREPPCIState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 4);
> + val = PPC_pci_data_read(s, addr, 4);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> diff --git a/hw/sh_pci.c b/hw/sh_pci.c
> index 046db2e..2a8f132 100644
> --- a/hw/sh_pci.c
> +++ b/hw/sh_pci.c
> @@ -36,6 +36,22 @@ typedef struct {
> uint32_t iobr;
> } SHPCIC;
>
> +static void sh_pci_data_write(SHPCIC *pcic, target_phys_addr_t addr,
> + uint32_t val, int size)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
> + pci_data_write(&conf_addr, 0, val, size);
> +}
> +
> +static uint32_t sh_pci_data_read(SHPCIC *pcic,
> + target_phys_addr_t addr, int size)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
> + return pci_data_read(&conf_addr, 0, size);
> +}
> +
> static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
> {
> SHPCIC *pcic = p;
> @@ -53,7 +69,7 @@ static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
> pcic->iobr = val;
> break;
> case 0x220:
> - pci_data_write(pcic->s.bus, pcic->par, val, 4);
> + sh_pci_data_write(pcic, pcic->par, val, 4);
> break;
> }
> }
> @@ -67,7 +83,7 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
> case 0x1c0:
> return pcic->par;
> case 0x220:
> - return pci_data_read(pcic->s.bus, pcic->par, 4);
> + return sh_pci_data_read(pcic, pcic->par, 4);
> }
> return 0;
> }
> @@ -75,13 +91,13 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
> static void sh_pci_mem_write (SHPCIC *pcic, target_phys_addr_t addr,
> uint32_t val, int size)
> {
> - pci_data_write(pcic->s.bus, addr + pcic->mbr, val, size);
> + sh_pci_data_write(pcic, addr + pcic->mbr, val, size);
> }
>
> static uint32_t sh_pci_mem_read (SHPCIC *pcic, target_phys_addr_t addr,
> int size)
> {
> - return pci_data_read(pcic->s.bus, addr + pcic->mbr, size);
> + return sh_pci_data_read(pcic, addr + pcic->mbr, size);
> }
>
> static void sh_pci_writeb (void *p, target_phys_addr_t addr, uint32_t val)
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index d99b7fa..14a728a 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -23,11 +23,29 @@ static inline uint32_t vpb_pci_config_addr(target_phys_addr_t addr)
> return addr & 0xffffff;
> }
>
> +static void vpb_pci_data_write(PCIHostState *s, target_phys_addr_t addr,
> + uint32_t val, int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> + &conf_addr);
> + pci_data_write(&conf_addr, 0, val, len);
> +}
> +
> +static uint32_t vpb_pci_data_read(PCIHostState *s, target_phys_addr_t addr,
> + int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> + &conf_addr);
> + return pci_data_read(&conf_addr, 0, len);
> +}
> +
I thought we will get rid of vpb_pci_config_addr, and fill in
fields in PCIConfigAddress directly. If we don't, and still
recode into PC format, this is not making code any prettier
so I don't really see what this buys us.
> static void pci_vpb_config_writeb (void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> PCIHostState *s = opaque;
> - pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 1);
> + vpb_pci_data_write(s, addr, val, 1);
> }
>
> static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
> @@ -37,7 +55,7 @@ static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> - pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 2);
> + vpb_pci_data_write(s, addr, val, 2);
> }
>
> static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
> @@ -47,14 +65,14 @@ static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> - pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 4);
> + vpb_pci_data_write(s, addr, val, 4);
> }
>
> static uint32_t pci_vpb_config_readb (void *opaque, target_phys_addr_t addr)
> {
> PCIHostState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 1);
> + val = vpb_pci_data_read(s, addr, 1);
> return val;
> }
>
> @@ -62,7 +80,7 @@ static uint32_t pci_vpb_config_readw (void *opaque, target_phys_addr_t addr)
> {
> PCIHostState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 2);
> + val = vpb_pci_data_read(s, addr, 2);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> @@ -73,7 +91,7 @@ static uint32_t pci_vpb_config_readl (void *opaque, target_phys_addr_t addr)
> {
> PCIHostState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 4);
> + val = vpb_pci_data_read(s, addr, 4);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> --
> 1.6.5.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
` (5 preceding siblings ...)
2010-01-12 8:52 ` [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress Isaku Yamahata
@ 2010-01-12 10:18 ` Michael S. Tsirkin
2010-01-12 10:31 ` Alexander Graf
2010-01-12 10:39 ` Alexander Graf
6 siblings, 2 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2010-01-12 10:18 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: qemu-devel, agraf
On Tue, Jan 12, 2010 at 05:52:52PM +0900, Isaku Yamahata wrote:
> During reviewing Alexander's PPC patches, it proved that
> pci_data_{read, write}() should take PCIConfigAddress as an argument.
> this patch series is inspired by his patch and some of them
> are based on it.
>
> This patch would make Alexander's PPC work easier.
I was waiting for revision of Alexander's patches (they looked good but
there were some unaddressed comments on last version, correct?). If
they are posted soon I think I'll apply them first and this series will
have to be rebased on top. Alexander, if you prefer to wait for this
refactoring to take shape instead, or work on top of this series, let me
know.
> Isaku Yamahata (6):
> sh_pci: use PCIHostState instead of PCIBus.
> sh_pci: s/sh_pci_data_write/sh_pci_mem_write/g for consistency.
> versatile_pci: user PCIHostState instead of PCIBus
> pci_host: remove code duplication in pci_host_template.h
> pci: introduce PCIAddress, PCIConfigAddress and helper functions.
> pci host: make pci_data_{write, read}() get PCIConfigAddress.
>
> hw/apb_pci.c | 12 ++++--
> hw/gt64xxx.c | 20 ++++++----
> hw/pci.h | 7 ++++
> hw/pci_host.c | 61 +++++++++++++++++++++++++-------
> hw/pci_host.h | 21 ++++++++++-
> hw/pci_host_template.h | 85 +++++++++----------------------------------
> hw/pci_host_template_all.h | 23 ++++++++++++
> hw/prep_pci.c | 28 +++++++++++---
> hw/sh_pci.c | 42 +++++++++++++++-------
> hw/versatile_pci.c | 47 +++++++++++++++++++-----
> qemu-common.h | 2 +
> 11 files changed, 225 insertions(+), 123 deletions(-)
> create mode 100644 hw/pci_host_template_all.h
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up
2010-01-12 10:18 ` [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up Michael S. Tsirkin
@ 2010-01-12 10:31 ` Alexander Graf
2010-01-12 10:39 ` Alexander Graf
1 sibling, 0 replies; 21+ messages in thread
From: Alexander Graf @ 2010-01-12 10:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Isaku Yamahata, qemu-devel
On 12.01.2010, at 11:18, Michael S. Tsirkin wrote:
> On Tue, Jan 12, 2010 at 05:52:52PM +0900, Isaku Yamahata wrote:
>> During reviewing Alexander's PPC patches, it proved that
>> pci_data_{read, write}() should take PCIConfigAddress as an argument.
>> this patch series is inspired by his patch and some of them
>> are based on it.
>>
>> This patch would make Alexander's PPC work easier.
>
> I was waiting for revision of Alexander's patches (they looked good but
> there were some unaddressed comments on last version, correct?). If
> they are posted soon I think I'll apply them first and this series will
> have to be rebased on top. Alexander, if you prefer to wait for this
> refactoring to take shape instead, or work on top of this series, let me
> know.
Hm, the only one I could find in the mails is the missing inline function that generates an x86 style config space descriptor. And that should be fixed by not converting to x86 format, but to a struct. I did send out the new version that does it with a struct, right?
Am I missing a mail? :)
Alex
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up
2010-01-12 10:18 ` [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up Michael S. Tsirkin
2010-01-12 10:31 ` Alexander Graf
@ 2010-01-12 10:39 ` Alexander Graf
1 sibling, 0 replies; 21+ messages in thread
From: Alexander Graf @ 2010-01-12 10:39 UTC (permalink / raw)
To: Michael S.Tsirkin; +Cc: Isaku Yamahata, qemu-devel
On 12.01.2010, at 11:18, Michael S. Tsirkin wrote:
> On Tue, Jan 12, 2010 at 05:52:52PM +0900, Isaku Yamahata wrote:
>> During reviewing Alexander's PPC patches, it proved that
>> pci_data_{read, write}() should take PCIConfigAddress as an argument.
>> this patch series is inspired by his patch and some of them
>> are based on it.
>>
>> This patch would make Alexander's PPC work easier.
>
> I was waiting for revision of Alexander's patches (they looked good but
> there were some unaddressed comments on last version, correct?). If
> they are posted soon I think I'll apply them first and this series will
> have to be rebased on top. Alexander, if you prefer to wait for this
> refactoring to take shape instead, or work on top of this series, let me
> know.
Yes, I was missing mails, sorry. Will address them shortly.
Alex
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 10:12 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-01-12 10:43 ` Isaku Yamahata
2010-01-12 17:54 ` Alexander Graf
2010-01-13 13:08 ` Paul Brook
1 sibling, 1 reply; 21+ messages in thread
From: Isaku Yamahata @ 2010-01-12 10:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Blue Swirl, Paul Brook, qemu-devel, Aurelien Jarno, agraf
On Tue, Jan 12, 2010 at 12:12:36PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 12, 2010 at 05:52:58PM +0900, Isaku Yamahata wrote:
> > diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> > index d99b7fa..14a728a 100644
> > --- a/hw/versatile_pci.c
> > +++ b/hw/versatile_pci.c
> > @@ -23,11 +23,29 @@ static inline uint32_t vpb_pci_config_addr(target_phys_addr_t addr)
> > return addr & 0xffffff;
> > }
> >
> > +static void vpb_pci_data_write(PCIHostState *s, target_phys_addr_t addr,
> > + uint32_t val, int len)
> > +{
> > + PCIConfigAddress conf_addr;
> > + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> > + &conf_addr);
> > + pci_data_write(&conf_addr, 0, val, len);
> > +}
> > +
> > +static uint32_t vpb_pci_data_read(PCIHostState *s, target_phys_addr_t addr,
> > + int len)
> > +{
> > + PCIConfigAddress conf_addr;
> > + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> > + &conf_addr);
> > + return pci_data_read(&conf_addr, 0, len);
> > +}
> > +
>
>
> I thought we will get rid of vpb_pci_config_addr, and fill in
> fields in PCIConfigAddress directly. If we don't, and still
> recode into PC format, this is not making code any prettier
> so I don't really see what this buys us.
I should have explain my plan.
After introducing decode callback by Alexander patch,
I'd like to remove them.
I was waiting for his respin, but he seems to be busy.
So I created this patch series to make his respin easier (hopefully).
For that sake, I kept the original structures.
I'm fine with either ways. Waiting for his patch merged or
make this patch series into shape.
--
yamahata
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 8:52 ` [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress Isaku Yamahata
2010-01-12 10:12 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-01-12 17:54 ` Alexander Graf
2010-01-12 18:33 ` Michael S. Tsirkin
1 sibling, 1 reply; 21+ messages in thread
From: Alexander Graf @ 2010-01-12 17:54 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: Blue Swirl, Paul Brook, qemu-devel, Aurelien Jarno, mst
Isaku Yamahata wrote:
> Move out pci address decoding logic from pci_data_{write, read}()
> by making pci_data_{write, read}() get PCIConfigAddress.
>
I think this is going in the wrong direction. You are keeping the device
specific MMIO/PIO based config space access handlers and just move the
address decoding into a separate function.
The whole idea of my patch was to only use pci_host to handle the MMIOs
and have a simple callback into a bus specific decoder. That way we
could probably remove most of the functions you're patching in this patch.
Or am I wrong here?
Alex
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 10:43 ` Isaku Yamahata
@ 2010-01-12 17:54 ` Alexander Graf
0 siblings, 0 replies; 21+ messages in thread
From: Alexander Graf @ 2010-01-12 17:54 UTC (permalink / raw)
To: Isaku Yamahata
Cc: Blue Swirl, qemu-devel, Paul Brook, Aurelien Jarno,
Michael S. Tsirkin
Isaku Yamahata wrote:
> On Tue, Jan 12, 2010 at 12:12:36PM +0200, Michael S. Tsirkin wrote:
>
>> On Tue, Jan 12, 2010 at 05:52:58PM +0900, Isaku Yamahata wrote:
>>
>>> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
>>> index d99b7fa..14a728a 100644
>>> --- a/hw/versatile_pci.c
>>> +++ b/hw/versatile_pci.c
>>> @@ -23,11 +23,29 @@ static inline uint32_t vpb_pci_config_addr(target_phys_addr_t addr)
>>> return addr & 0xffffff;
>>> }
>>>
>>> +static void vpb_pci_data_write(PCIHostState *s, target_phys_addr_t addr,
>>> + uint32_t val, int len)
>>> +{
>>> + PCIConfigAddress conf_addr;
>>> + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
>>> + &conf_addr);
>>> + pci_data_write(&conf_addr, 0, val, len);
>>> +}
>>> +
>>> +static uint32_t vpb_pci_data_read(PCIHostState *s, target_phys_addr_t addr,
>>> + int len)
>>> +{
>>> + PCIConfigAddress conf_addr;
>>> + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
>>> + &conf_addr);
>>> + return pci_data_read(&conf_addr, 0, len);
>>> +}
>>> +
>>>
>> I thought we will get rid of vpb_pci_config_addr, and fill in
>> fields in PCIConfigAddress directly. If we don't, and still
>> recode into PC format, this is not making code any prettier
>> so I don't really see what this buys us.
>>
>
> I should have explain my plan.
>
> After introducing decode callback by Alexander patch,
> I'd like to remove them.
> I was waiting for his respin, but he seems to be busy.
> So I created this patch series to make his respin easier (hopefully).
> For that sake, I kept the original structures.
>
> I'm fine with either ways. Waiting for his patch merged or
> make this patch series into shape.
>
Oh, I guess I read this mail too late. Sorry about that :-).
Alex
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 17:54 ` [Qemu-devel] " Alexander Graf
@ 2010-01-12 18:33 ` Michael S. Tsirkin
2010-01-13 12:09 ` Alexander Graf
0 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2010-01-12 18:33 UTC (permalink / raw)
To: Alexander Graf
Cc: Blue Swirl, Isaku Yamahata, qemu-devel, Aurelien Jarno,
Paul Brook
Guys, I was wondering whether the following helper function will be
helpful, as a lot of common code seems to be around identical b/w/l
callbacks. Comments?
diff --git a/cpu-common.h b/cpu-common.h
index 0ec9b72..be7e992 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -42,6 +42,24 @@ int cpu_register_io_memory(CPUReadMemoryFunc * const *mem_read,
void *opaque);
void cpu_unregister_io_memory(int table_address);
+typedef struct CpuIoMemoryHandler CpuIoMemoryHandler;
+/* len is guaranteed to be one of 1, 2 or 4 and does not need
+ * to be range checked. */
+typedef void CpuWriteMemorySimple(CpuIoMemoryHandler *,
+ target_phys_addr_t addr,
+ uint32_t value, int len);
+typedef uint32_t CpuReadMemorySimple(CpuIoMemoryHandler *,
+ target_phys_addr_t addr, int len);
+
+struct CpuIoMemoryHandler {
+ CpuWriteMemorySimple *write;
+ CpuReadMemorySimple *read;
+};
+
+/* Helper routine for when we want to use a single routine with
+ * length instead. */
+int cpu_register_io_memory_simple(struct CpuIoMemoryHandler *);
+
void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
int len, int is_write);
static inline void cpu_physical_memory_read(target_phys_addr_t addr,
diff --git a/exec.c b/exec.c
index 8a1c08e..cc0bf43 100644
--- a/exec.c
+++ b/exec.c
@@ -3065,6 +3065,67 @@ void cpu_unregister_io_memory(int io_table_address)
io_mem_used[io_index] = 0;
}
+static void cpu_io_memory_simple_writeb(void *opaque, target_phys_addr_t addr,
+ uint32_t value)
+{
+ struct CpuIoMemoryHandler *handler = opaque;
+ handler->write(handler, addr, value, 1);
+}
+
+static uint32_t cpu_io_memory_simple_readb(void *opaque,
+ target_phys_addr_t addr)
+{
+ struct CpuIoMemoryHandler *handler = opaque;
+ return handler->read(handler, addr, 1);
+}
+
+static void cpu_io_memory_simple_writew(void *opaque, target_phys_addr_t addr,
+ uint32_t value)
+{
+ struct CpuIoMemoryHandler *handler = opaque;
+ handler->write(handler, addr, value, 2);
+}
+
+static uint32_t cpu_io_memory_simple_readw(void *opaque,
+ target_phys_addr_t addr)
+{
+ struct CpuIoMemoryHandler *handler = opaque;
+ return handler->read(handler, addr, 2);
+}
+
+static void cpu_io_memory_simple_writel(void *opaque, target_phys_addr_t addr,
+ uint32_t value)
+{
+ struct CpuIoMemoryHandler *handler = opaque;
+ handler->write(handler, addr, value, 4);
+}
+
+static uint32_t cpu_io_memory_simple_readl(void *opaque,
+ target_phys_addr_t addr)
+{
+ struct CpuIoMemoryHandler *handler = opaque;
+ return handler->read(handler, addr, 4);
+}
+
+static CPUWriteMemoryFunc * const cpu_io_memory_simple_write[] = {
+ &cpu_io_memory_simple_writeb,
+ &cpu_io_memory_simple_writew,
+ &cpu_io_memory_simple_writel,
+};
+
+static CPUReadMemoryFunc * const cpu_io_memory_simple_read[] = {
+ &cpu_io_memory_simple_readb,
+ &cpu_io_memory_simple_readw,
+ &cpu_io_memory_simple_readl,
+};
+
+int cpu_register_io_memory_simple(struct CPUIoMemoryHandler *handler)
+{
+ return cpu_register_io_memory(cpu_io_memory_simple_read,
+ cpu_io_memory_simple_write,
+ handler);
+}
+
static void io_mem_init(void)
{
int i;
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 18:33 ` Michael S. Tsirkin
@ 2010-01-13 12:09 ` Alexander Graf
0 siblings, 0 replies; 21+ messages in thread
From: Alexander Graf @ 2010-01-13 12:09 UTC (permalink / raw)
To: Michael S.Tsirkin
Cc: Blue Swirl, Isaku Yamahata, qemu-devel, Aurelien Jarno,
Paul Brook
On 12.01.2010, at 19:33, Michael S. Tsirkin wrote:
> Guys, I was wondering whether the following helper function will be
> helpful, as a lot of common code seems to be around identical b/w/l
> callbacks. Comments?
I like the idea. That could potentially clean up quite a bit of code in qemu.
Alex
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] versatile_pci: user PCIHostState instead of PCIBus
2010-01-12 8:52 ` [Qemu-devel] [PATCH 3/6] versatile_pci: user PCIHostState instead of PCIBus Isaku Yamahata
@ 2010-01-13 13:02 ` Paul Brook
2010-01-13 13:04 ` Michael S. Tsirkin
0 siblings, 1 reply; 21+ messages in thread
From: Paul Brook @ 2010-01-13 13:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Isaku Yamahata, agraf, mst
On Tuesday 12 January 2010, Isaku Yamahata wrote:
> To use pci host framework, use PCIHostState instead of PCIBus in
> PCIVPBState.
No.
pci_host.[ch] provides very specific functionality, it is not a generic PCI
host device. Specifically it provides indirect access to PCI config space via
a memory mapped {address,data} pair. The versatile PCI host exposes PCI config
space directly, so should not be using this code.
If you want a generic framework for PCI hosts then you need to use something
else. If nothing else, assuming that a PCI host bridge is always is SysBus
device is wrong.
Paul
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] versatile_pci: user PCIHostState instead of PCIBus
2010-01-13 13:02 ` Paul Brook
@ 2010-01-13 13:04 ` Michael S. Tsirkin
0 siblings, 0 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2010-01-13 13:04 UTC (permalink / raw)
To: Paul Brook; +Cc: Isaku Yamahata, qemu-devel, agraf
On Wed, Jan 13, 2010 at 01:02:50PM +0000, Paul Brook wrote:
> On Tuesday 12 January 2010, Isaku Yamahata wrote:
> > To use pci host framework, use PCIHostState instead of PCIBus in
> > PCIVPBState.
>
> No.
>
> pci_host.[ch] provides very specific functionality, it is not a generic PCI
> host device. Specifically it provides indirect access to PCI config space via
> a memory mapped {address,data} pair. The versatile PCI host exposes PCI config
> space directly, so should not be using this code.
>
> If you want a generic framework for PCI hosts then you need to use something
> else. If nothing else, assuming that a PCI host bridge is always is SysBus
> device is wrong.
>
> Paul
What most people seem to want is callback that will get length is a
parameter instead of supplying 3 functions. pci_host does it
but we do not need pci_host for this.
--
MST
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.
2010-01-12 10:12 ` [Qemu-devel] " Michael S. Tsirkin
2010-01-12 10:43 ` Isaku Yamahata
@ 2010-01-13 13:08 ` Paul Brook
1 sibling, 0 replies; 21+ messages in thread
From: Paul Brook @ 2010-01-13 13:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Blue Swirl, Isaku Yamahata, qemu-devel, Aurelien Jarno, agraf
> I thought we will get rid of vpb_pci_config_addr, and fill in
> fields in PCIConfigAddress directly. If we don't, and still
> recode into PC format, this is not making code any prettier
> so I don't really see what this buys us.
I agree. This patch seems to be introducing churn for no benefit.
See also comments about direct v.s. indirect access to PCI config space.
I suspect you're trying to use a common implementation for two fundamentally
different things.
Paul
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2010-01-13 13:08 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-12 8:52 [Qemu-devel] [PATCH 0/6] pci: pci_data_{write, read}() clean up Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 1/6] sh_pci: use PCIHostState instead of PCIBus Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 2/6] sh_pci: s/sh_pci_data_write/sh_pci_mem_write/g for consistency Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 3/6] versatile_pci: user PCIHostState instead of PCIBus Isaku Yamahata
2010-01-13 13:02 ` Paul Brook
2010-01-13 13:04 ` Michael S. Tsirkin
2010-01-12 8:52 ` [Qemu-devel] [PATCH 4/6] pci_host: remove code duplication in pci_host_template.h Isaku Yamahata
2010-01-12 8:52 ` [Qemu-devel] [PATCH 5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions Isaku Yamahata
2010-01-12 10:04 ` [Qemu-devel] " Michael S. Tsirkin
2010-01-12 10:06 ` Michael S. Tsirkin
2010-01-12 8:52 ` [Qemu-devel] [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress Isaku Yamahata
2010-01-12 10:12 ` [Qemu-devel] " Michael S. Tsirkin
2010-01-12 10:43 ` Isaku Yamahata
2010-01-12 17:54 ` Alexander Graf
2010-01-13 13:08 ` Paul Brook
2010-01-12 17:54 ` [Qemu-devel] " Alexander Graf
2010-01-12 18:33 ` Michael S. Tsirkin
2010-01-13 12:09 ` Alexander Graf
2010-01-12 10:18 ` [Qemu-devel] Re: [PATCH 0/6] pci: pci_data_{write, read}() clean up Michael S. Tsirkin
2010-01-12 10:31 ` Alexander Graf
2010-01-12 10:39 ` Alexander Graf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).