* [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2
@ 2010-01-04 7:32 Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 1/6] PCI config space access overhaul Alexander Graf
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-04 7:32 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Aurelien Jarno
I'm trying to get the PPC64 system emulation target working finally.
While doing so, I ran into several issues, all related to PCI this time.
This patchset fixes all the PCI config space access and PCI interrupt
mapping issues I've found on PPC64. Using this and a patched OpenBIOS
version, I can successfully access IDE devices and was booting a guest
into the shell from IDE using serial console.
To leverage this patch, you also need a few patches to OpenBIOS. I'll
present them to the OpenBIOS list, but in general getting patches into
Qemu is harder than getting them into OpenBIOS. So I want to wait for
the rewiew process here first.
Find the OpenBIOS patch at: http://alex.csgraf.de/openbios-ppc-u3.patch
v1 -> v2:
- use decoding function for config space bits
- merge config space data access functions
- introduce encoding function for x86 style encodings
- convert Uninorth to use config space decoder
Alexander Graf (6):
PCI config space access overhaul
Add config space conversion function for uni_north
Use Mac99_U3 type on ppc64
Include dump of lspci -nn on real G5
Make interrupts work
Enable secondary cmd64x
hw/apb_pci.c | 1 +
hw/grackle_pci.c | 1 +
hw/gt64xxx.c | 1 +
hw/pci.h | 13 ++++
hw/pci_host.c | 48 ++++++++++++---
hw/pci_host.h | 16 +++++
hw/pci_host_template.h | 90 ++++++++--------------------
hw/pci_host_template_all.h | 23 +++++++
hw/pci_ids.h | 1 +
hw/piix_pci.c | 1 +
hw/ppc.h | 1 +
hw/ppc4xx_pci.c | 1 +
hw/ppc_mac.h | 1 +
hw/ppc_newworld.c | 14 ++++-
hw/ppce500_pci.c | 1 +
hw/unin_pci.c | 141 +++++++++++++++++++++++++++++++++++++++++++-
16 files changed, 274 insertions(+), 80 deletions(-)
create mode 100644 hw/pci_host_template_all.h
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 1/6] PCI config space access overhaul
2010-01-04 7:32 [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2 Alexander Graf
@ 2010-01-04 7:32 ` Alexander Graf
2010-01-05 12:46 ` Isaku Yamahata
2010-01-05 22:16 ` [Qemu-devel] " Michael S. Tsirkin
2010-01-04 7:32 ` [Qemu-devel] [PATCH 2/6] Add config space conversion function for uni_north Alexander Graf
` (4 subsequent siblings)
5 siblings, 2 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-04 7:32 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Aurelien Jarno, Michael S. Tsirkin
Different host buses may have different layouts for config space accessors.
The Mac U3 for example uses the following define to access Type 0 (directly
attached) devices:
#define MACRISC_CFA0(devfn, off) \
((1 << (unsigned int)PCI_SLOT(dev_fn)) \
| (((unsigned int)PCI_FUNC(dev_fn)) << 8) \
| (((unsigned int)(off)) & 0xFCUL))
Obviously, this is different from the encoding we interpret in qemu. In
order to let host controllers take some influence there, we can just
introduce a decoding function that takes whatever accessor we have and
decodes it into understandable fields.
To not touch all different code paths in qemu, I added this on top of
the existing config_reg element. In the future, we should work on gradually
moving references to config_reg to config_reg_dec and then phase out
config_reg.
This patch is the groundwork for Uninorth PCI config space fixes.
Signed-off-by: Alexander Graf <agraf@suse.de>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
v1 -> v2:
- merge data access functions
- use decoding function for config space bits
- introduce encoding function for x86 style encodings
---
hw/apb_pci.c | 1 +
hw/grackle_pci.c | 1 +
hw/gt64xxx.c | 1 +
hw/pci.h | 13 ++++++
hw/pci_host.c | 48 ++++++++++++++++++-----
hw/pci_host.h | 16 ++++++++
hw/pci_host_template.h | 90 +++++++++++++-------------------------------
hw/pci_host_template_all.h | 23 +++++++++++
hw/piix_pci.c | 1 +
hw/ppc4xx_pci.c | 1 +
hw/ppce500_pci.c | 1 +
hw/unin_pci.c | 1 +
12 files changed, 123 insertions(+), 74 deletions(-)
create mode 100644 hw/pci_host_template_all.h
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index f05308b..fedb84c 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -222,6 +222,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
/* mem_data */
sysbus_mmio_map(s, 3, mem_base);
d = FROM_SYSBUS(APBState, s);
+ pci_host_init(&d->host_state);
d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
pci_apb_set_irq, pci_pbm_map_irq, pic,
0, 32);
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index ee4fed5..7feba63 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -88,6 +88,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic)
qdev_init_nofail(dev);
s = sysbus_from_qdev(dev);
d = FROM_SYSBUS(GrackleState, s);
+ pci_host_init(&d->host_state);
d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
pci_grackle_set_irq,
pci_grackle_map_irq,
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index fb7f5bd..8cf93ca 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -1120,6 +1120,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
s = qemu_mallocz(sizeof(GT64120State));
s->pci = qemu_mallocz(sizeof(GT64120PCIState));
+ pci_host_init(s->pci);
s->pci->bus = pci_register_bus(NULL, "pci",
pci_gt64120_set_irq, pci_gt64120_map_irq,
pic, 144, 4);
diff --git a/hw/pci.h b/hw/pci.h
index fd16460..cfaa0a9 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -10,10 +10,23 @@
/* PCI bus */
+typedef struct PCIAddress {
+ PCIBus *domain;
+ uint8_t bus;
+ uint8_t slot;
+ uint8_t fn;
+} PCIAddress;
+
#define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
#define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
#define PCI_FUNC(devfn) ((devfn) & 0x07)
+static inline uint32_t pci_addr_to_devfn(PCIAddress *addr, uint32_t offset)
+{
+ return ((addr->bus & 0xff) << 16) | (PCI_DEVFN(addr->slot, addr->fn) << 8) |
+ offset;
+}
+
/* Class, Vendor and Device IDs from Linux's pci_ids.h */
#include "pci_ids.h"
diff --git a/hw/pci_host.c b/hw/pci_host.c
index eeb8dee..746ffc2 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -32,13 +32,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
#define PCI_DPRINTF(fmt, ...)
#endif
-/*
- * PCI address
- * bit 16 - 24: bus number
- * bit 8 - 15: devfun number
- * bit 0 - 7: offset in configuration space of a given pci device
- */
-
/* 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)
{
@@ -56,7 +49,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
if (!pci_dev)
return;
- PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
+ PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
__func__, pci_dev->name, config_addr, val, len);
pci_dev->config_write(pci_dev, config_addr, val, len);
}
@@ -89,7 +82,9 @@ static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
#endif
PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
__func__, addr, val);
+
s->config_reg = val;
+ s->decode_config_reg(s, val, &s->config_reg_dec);
}
static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
@@ -131,7 +126,9 @@ static void pci_host_config_writel_noswap(void *opaque,
PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
__func__, addr, val);
+
s->config_reg = val;
+ s->decode_config_reg(s, val, &s->config_reg_dec);
}
static uint32_t pci_host_config_readl_noswap(void *opaque,
@@ -169,7 +166,9 @@ static void pci_host_config_writel_ioport(void *opaque,
PCIHostState *s = opaque;
PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, val);
+
s->config_reg = val;
+ s->decode_config_reg(s, val, &s->config_reg_dec);
}
static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
@@ -190,7 +189,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 +216,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)
{
@@ -228,3 +227,32 @@ void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
}
+
+/* This implements the default x86 way of interpreting a PCI address
+ *
+ * PCI address
+ * bit 16 - 24: bus number
+ * bit 11 - 15: slot number
+ * bit 8 - 10: function number
+ * bit 0 - 7: offset in configuration space of a given pci device
+ */
+
+void pci_host_decode_config_reg(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;
+ decoded->addr.slot = (config_reg >> 11) & 0x1f;
+ decoded->addr.fn = (config_reg >> 8) & 0x7;
+ decoded->offset = config_reg & 0xff;
+ decoded->addr_mask = 3;
+ decoded->valid = (config_reg & (1u << 31)) ? true : false;
+}
+
+void pci_host_init(PCIHostState *s)
+{
+ s->decode_config_reg = pci_host_decode_config_reg;
+}
diff --git a/hw/pci_host.h b/hw/pci_host.h
index a006687..0fcdf64 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -30,14 +30,30 @@
#include "sysbus.h"
+/* for config space access */
+typedef struct PCIConfigAddress {
+ PCIAddress addr;
+ uint32_t addr_mask;
+ uint16_t offset;
+ bool valid;
+} PCIConfigAddress;
+
+typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg,
+ PCIConfigAddress *conf);
+
struct PCIHostState {
SysBusDevice busdev;
+ pci_config_reg_fn decode_config_reg;
+ PCIConfigAddress config_reg_dec;
uint32_t config_reg;
PCIBus *bus;
};
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_host_init(PCIHostState *s);
+void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg,
+ PCIConfigAddress *decoded);
/* 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 11e6c88..d989c25 100644
--- a/hw/pci_host_template.h
+++ b/hw/pci_host_template.h
@@ -25,85 +25,47 @@
/* 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;
+ PCIConfigAddress *config_reg = &s->config_reg_dec;
+ int offset = config_reg->offset;
- 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);
+#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
+ val = glue(bswap, PCI_HOST_BITS)(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);
-#endif
- PCI_DPRINTF("writel 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, val, 4);
+ offset |= (addr & config_reg->addr_mask);
+ PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
+ __func__, (target_phys_addr_t)addr, val);
+ if (config_reg->valid) {
+ pci_data_write(s->bus, pci_addr_to_devfn(&config_reg->addr, offset), 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;
+ PCIConfigAddress *config_reg = &s->config_reg_dec;
+ int offset = config_reg->offset;
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 (!config_reg->valid) {
+ 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;
-}
+ offset |= (addr & config_reg->addr_mask);
+ val = pci_data_read(s->bus, pci_addr_to_devfn(&config_reg->addr, offset),
+ 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);
-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);
+#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
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 1b67475..97500e0 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -211,6 +211,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
dev = qdev_create(NULL, "i440FX-pcihost");
s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
+ pci_host_init(s);
b = pci_bus_new(&s->busdev.qdev, NULL, 0);
s->bus = b;
qdev_init_nofail(dev);
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 2d00b61..dd8e290 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -357,6 +357,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
controller = qemu_mallocz(sizeof(PPC4xxPCIState));
+ pci_host_init(&controller->pci_state);
controller->pci_state.bus = pci_register_bus(NULL, "pci",
ppc4xx_pci_set_irq,
ppc4xx_pci_map_irq,
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index a72fb86..5914183 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -278,6 +278,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
controller = qemu_mallocz(sizeof(PPCE500PCIState));
+ pci_host_init(&controller->pci_state);
controller->pci_state.bus = pci_register_bus(NULL, "pci",
mpc85xx_pci_set_irq,
mpc85xx_pci_map_irq,
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 3ae4e7a..fdb9401 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -84,6 +84,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
/* Uninorth main bus */
s = FROM_SYSBUS(UNINState, dev);
+ pci_host_init(&s->host_state);
pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
pci_mem_data = pci_host_data_register_mmio(&s->host_state);
sysbus_init_mmio(dev, 0x1000, pci_mem_config);
--
1.6.0.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 2/6] Add config space conversion function for uni_north
2010-01-04 7:32 [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2 Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 1/6] PCI config space access overhaul Alexander Graf
@ 2010-01-04 7:32 ` Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 3/6] Use Mac99_U3 type on ppc64 Alexander Graf
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-04 7:32 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Aurelien Jarno, Michael S. Tsirkin
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]
As stated in the previous patch, the Uninorth PCI bridge requires different
layouts in its PCI config space accessors.
This patch introduces a conversion function that makes it compatible with
the way Linux accesses it.
I also kept an OpenBIOS compatibility hack in. I think it'd be better to
take small steps here and do the config space access rework in OpenBIOS
later on. When that's done we can remove that hack.
Signed-off-by: Alexander Graf <agraf@suse.de>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
v1 -> v2:
- convert Uninorth to use config space decoder
---
hw/unin_pci.c | 31 +++++++++++++++++++++++++++++++
1 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index fdb9401..5e69725 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -75,6 +75,36 @@ static void pci_unin_reset(void *opaque)
{
}
+static void unin_decode_config_reg(PCIHostState *s, uint32_t config_reg,
+ PCIConfigAddress *decoded)
+{
+ decoded->addr.domain = s->bus;
+ decoded->addr_mask = 7;
+ decoded->offset = config_reg & 0xfc;
+ decoded->addr.fn = (config_reg >> 8) & 0x7;
+
+ if (config_reg & (1u << 31)) {
+ /* XXX OpenBIOS compatibility hack */
+ pci_host_decode_config_reg(s, config_reg, decoded);
+ } else if (config_reg & 1) {
+ /* CFA1 style values */
+
+ decoded->valid = (config_reg & 1) ? true : false;
+ decoded->addr.bus = (config_reg >> 16) & 0xff;
+ decoded->addr.slot = (config_reg >> 11) & 0x1f;
+ } else {
+ /* CFA0 style values */
+
+ decoded->valid = true;
+ decoded->addr.bus = 0;
+ decoded->addr.slot = ffs(config_reg & 0xfffff800) - 1;
+ }
+
+ UNIN_DPRINTF("Converted config space accessor %08x -> %02x %02x %x\n",
+ config_reg, decoded->addr.bus, decoded->addr.slot,
+ decoded->addr.fn);
+}
+
static int pci_unin_main_init_device(SysBusDevice *dev)
{
UNINState *s;
@@ -85,6 +115,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
s = FROM_SYSBUS(UNINState, dev);
pci_host_init(&s->host_state);
+ s->host_state.decode_config_reg = unin_decode_config_reg;
pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
pci_mem_data = pci_host_data_register_mmio(&s->host_state);
sysbus_init_mmio(dev, 0x1000, pci_mem_config);
--
1.6.0.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 3/6] Use Mac99_U3 type on ppc64
2010-01-04 7:32 [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2 Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 1/6] PCI config space access overhaul Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 2/6] Add config space conversion function for uni_north Alexander Graf
@ 2010-01-04 7:32 ` Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 4/6] Include dump of lspci -nn on real G5 Alexander Graf
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-04 7:32 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Aurelien Jarno
The "Mac99" type so far defines a "U2" based configuration. Unfortunately,
there have never been any U2 based PPC64 machines. That's what the U3 was
developed for.
So let's split the Mac99 machine in a PPC64 and a PPC32 machine. The PPC32
machine stays "Mac99", while the PPC64 one becomes "Mac99_U3". All peripherals
stay the same.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
v1 -> v2:
- s/get_config/decode_config/
---
hw/pci_ids.h | 1 +
hw/ppc.h | 1 +
hw/ppc_mac.h | 1 +
hw/ppc_newworld.c | 12 +++++++-
hw/unin_pci.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/hw/pci_ids.h b/hw/pci_ids.h
index 63379c2..fe7a121 100644
--- a/hw/pci_ids.h
+++ b/hw/pci_ids.h
@@ -63,6 +63,7 @@
#define PCI_VENDOR_ID_APPLE 0x106b
#define PCI_DEVICE_ID_APPLE_UNI_N_AGP 0x0020
+#define PCI_DEVICE_ID_APPLE_U3_AGP 0x004b
#define PCI_VENDOR_ID_SUN 0x108e
#define PCI_DEVICE_ID_SUN_EBUS 0x1000
diff --git a/hw/ppc.h b/hw/ppc.h
index bbf3a98..b9a12a1 100644
--- a/hw/ppc.h
+++ b/hw/ppc.h
@@ -40,6 +40,7 @@ enum {
ARCH_PREP = 0,
ARCH_MAC99,
ARCH_HEATHROW,
+ ARCH_MAC99_U3,
};
#define FW_CFG_PPC_WIDTH (FW_CFG_ARCH_LOCAL + 0x00)
diff --git a/hw/ppc_mac.h b/hw/ppc_mac.h
index a04dffe..89f96bb 100644
--- a/hw/ppc_mac.h
+++ b/hw/ppc_mac.h
@@ -58,6 +58,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic);
/* UniNorth PCI */
PCIBus *pci_pmac_init(qemu_irq *pic);
+PCIBus *pci_pmac_u3_init(qemu_irq *pic);
/* Mac NVRAM */
typedef struct MacIONVRAMState MacIONVRAMState;
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index a4c714a..308e102 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -114,6 +114,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
void *fw_cfg;
void *dbdma;
uint8_t *vga_bios_ptr;
+ int machine_arch;
linux_boot = (kernel_filename != NULL);
@@ -317,7 +318,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
}
}
pic = openpic_init(NULL, &pic_mem_index, smp_cpus, openpic_irqs, NULL);
- pci_bus = pci_pmac_init(pic);
+ if (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) {
+ /* 970 gets a U3 bus */
+ pci_bus = pci_pmac_u3_init(pic);
+ machine_arch = ARCH_MAC99_U3;
+ } else {
+ pci_bus = pci_pmac_init(pic);
+ machine_arch = ARCH_MAC99;
+ }
/* init basic PC hardware */
pci_vga_init(pci_bus, vga_bios_offset, vga_bios_size);
@@ -364,7 +372,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
- fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_MAC99);
+ fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, kernel_base);
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
if (kernel_cmdline) {
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 5e69725..214ae3c 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -142,6 +142,27 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
return 0;
}
+static int pci_u3_agp_init_device(SysBusDevice *dev)
+{
+ UNINState *s;
+ int pci_mem_config, pci_mem_data;
+
+ /* Uninorth U3 AGP bus */
+ s = FROM_SYSBUS(UNINState, dev);
+
+ pci_host_init(&s->host_state);
+ s->host_state.decode_config_reg = unin_decode_config_reg;
+ pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
+ pci_mem_data = pci_host_data_register_mmio(&s->host_state);
+ sysbus_init_mmio(dev, 0x1000, pci_mem_config);
+ sysbus_init_mmio(dev, 0x1000, pci_mem_data);
+
+ register_savevm("uninorth", 0, 1, pci_unin_save, pci_unin_load, &s->host_state);
+ qemu_register_reset(pci_unin_reset, &s->host_state);
+
+ return 0;
+}
+
static int pci_unin_agp_init_device(SysBusDevice *dev)
{
UNINState *s;
@@ -223,6 +244,31 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
return d->host_state.bus;
}
+PCIBus *pci_pmac_u3_init(qemu_irq *pic)
+{
+ DeviceState *dev;
+ SysBusDevice *s;
+ UNINState *d;
+
+ /* Uninorth AGP bus */
+
+ dev = qdev_create(NULL, "u3-agp");
+ qdev_init_nofail(dev);
+ s = sysbus_from_qdev(dev);
+ d = FROM_SYSBUS(UNINState, s);
+
+ d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
+ pci_unin_set_irq, pci_unin_map_irq,
+ pic, 11 << 3, 4);
+
+ sysbus_mmio_map(s, 0, 0xf0800000);
+ sysbus_mmio_map(s, 1, 0xf0c00000);
+
+ pci_create_simple(d->host_state.bus, 11 << 3, "u3-agp");
+
+ return d->host_state.bus;
+}
+
static int unin_main_pci_host_init(PCIDevice *d)
{
pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_APPLE);
@@ -278,6 +324,21 @@ static int unin_agp_pci_host_init(PCIDevice *d)
return 0;
}
+static int u3_agp_pci_host_init(PCIDevice *d)
+{
+ pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_APPLE);
+ pci_config_set_device_id(d->config, PCI_DEVICE_ID_APPLE_U3_AGP);
+ /* revision */
+ d->config[0x08] = 0x00;
+ pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
+ /* cache line size */
+ d->config[0x0C] = 0x08;
+ /* latency timer */
+ d->config[0x0D] = 0x10;
+ d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
+ return 0;
+}
+
static int unin_internal_pci_host_init(PCIDevice *d)
{
pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_APPLE);
@@ -303,6 +364,12 @@ static PCIDeviceInfo dec_21154_pci_host_info = {
.init = dec_21154_pci_host_init,
};
+static PCIDeviceInfo u3_agp_pci_host_info = {
+ .qdev.name = "u3-agp",
+ .qdev.size = sizeof(PCIDevice),
+ .init = u3_agp_pci_host_init,
+};
+
static PCIDeviceInfo unin_agp_pci_host_info = {
.qdev.name = "uni-north-agp",
.qdev.size = sizeof(PCIDevice),
@@ -323,6 +390,9 @@ static void unin_register_devices(void)
sysbus_register_dev("dec-21154", sizeof(UNINState),
pci_dec_21154_init_device);
pci_qdev_register(&dec_21154_pci_host_info);
+ sysbus_register_dev("u3-agp", sizeof(UNINState),
+ pci_u3_agp_init_device);
+ pci_qdev_register(&u3_agp_pci_host_info);
sysbus_register_dev("uni-north-agp", sizeof(UNINState),
pci_unin_agp_init_device);
pci_qdev_register(&unin_agp_pci_host_info);
--
1.6.0.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 4/6] Include dump of lspci -nn on real G5
2010-01-04 7:32 [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2 Alexander Graf
` (2 preceding siblings ...)
2010-01-04 7:32 ` [Qemu-devel] [PATCH 3/6] Use Mac99_U3 type on ppc64 Alexander Graf
@ 2010-01-04 7:32 ` Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 5/6] Make interrupts work Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 6/6] Enable secondary cmd64x Alexander Graf
5 siblings, 0 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-04 7:32 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Aurelien Jarno
To ease debugging and to know what we're lacking, I found it really useful to
have an lspci dump of a real U3 based G5 around. So I added a comment for it.
If people don't think it's important enough to include this information in the
sources, just don't apply this patch.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
hw/unin_pci.c | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 214ae3c..a3b0435 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -244,6 +244,31 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
return d->host_state.bus;
}
+/*
+ * PCI bus layout on a real G5 (U3 based):
+ *
+ * 0000:f0:0b.0 Host bridge [0600]: Apple Computer Inc. U3 AGP [106b:004b]
+ * 0000:f0:10.0 VGA compatible controller [0300]: ATI Technologies Inc RV350 AP [Radeon 9600] [1002:4150]
+ * 0001:00:00.0 Host bridge [0600]: Apple Computer Inc. CPC945 HT Bridge [106b:004a]
+ * 0001:00:01.0 PCI bridge [0604]: Advanced Micro Devices [AMD] AMD-8131 PCI-X Bridge [1022:7450] (rev 12)
+ * 0001:00:02.0 PCI bridge [0604]: Advanced Micro Devices [AMD] AMD-8131 PCI-X Bridge [1022:7450] (rev 12)
+ * 0001:00:03.0 PCI bridge [0604]: Apple Computer Inc. K2 HT-PCI Bridge [106b:0045]
+ * 0001:00:04.0 PCI bridge [0604]: Apple Computer Inc. K2 HT-PCI Bridge [106b:0046]
+ * 0001:00:05.0 PCI bridge [0604]: Apple Computer Inc. K2 HT-PCI Bridge [106b:0047]
+ * 0001:00:06.0 PCI bridge [0604]: Apple Computer Inc. K2 HT-PCI Bridge [106b:0048]
+ * 0001:00:07.0 PCI bridge [0604]: Apple Computer Inc. K2 HT-PCI Bridge [106b:0049]
+ * 0001:01:07.0 Class [ff00]: Apple Computer Inc. K2 KeyLargo Mac/IO [106b:0041] (rev 20)
+ * 0001:01:08.0 USB Controller [0c03]: Apple Computer Inc. K2 KeyLargo USB [106b:0040]
+ * 0001:01:09.0 USB Controller [0c03]: Apple Computer Inc. K2 KeyLargo USB [106b:0040]
+ * 0001:02:0b.0 USB Controller [0c03]: NEC Corporation USB [1033:0035] (rev 43)
+ * 0001:02:0b.1 USB Controller [0c03]: NEC Corporation USB [1033:0035] (rev 43)
+ * 0001:02:0b.2 USB Controller [0c03]: NEC Corporation USB 2.0 [1033:00e0] (rev 04)
+ * 0001:03:0d.0 Class [ff00]: Apple Computer Inc. K2 ATA/100 [106b:0043]
+ * 0001:03:0e.0 FireWire (IEEE 1394) [0c00]: Apple Computer Inc. K2 FireWire [106b:0042]
+ * 0001:04:0f.0 Ethernet controller [0200]: Apple Computer Inc. K2 GMAC (Sun GEM) [106b:004c]
+ * 0001:05:0c.0 IDE interface [0101]: Broadcom K2 SATA [1166:0240]
+ *
+ */
PCIBus *pci_pmac_u3_init(qemu_irq *pic)
{
DeviceState *dev;
--
1.6.0.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 5/6] Make interrupts work
2010-01-04 7:32 [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2 Alexander Graf
` (3 preceding siblings ...)
2010-01-04 7:32 ` [Qemu-devel] [PATCH 4/6] Include dump of lspci -nn on real G5 Alexander Graf
@ 2010-01-04 7:32 ` Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 6/6] Enable secondary cmd64x Alexander Graf
5 siblings, 0 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-04 7:32 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Aurelien Jarno
The interrupt code as is didn't really work for me. I couldn't even convince
Linux to take interrupt 9 in an interrupt-map.
So let's do this right. Let's map all PCI interrupts to 0x1b - 0x1e. That way
we're at least a small step closer to what real hardware does.
I also took the interrupt pin to line conversion from OpenBIOS, which at least
assures us we're compatible with our firmware :-).
A dump of the PCI interrupt-map from a U2 (iBook):
00009000 00000000 00000000 00000000 ff97c528 00000034 00000001
0000d800 00000000 00000000 00000000 ff97c528 0000003f 00000001
0000c000 00000000 00000000 00000000 ff97c528 0000001b 00000001
0000c800 00000000 00000000 00000000 ff97c528 0000001c 00000001
0000d000 00000000 00000000 00000000 ff97c528 0000001d 00000001
Signed-off-by: Alexander Graf <agraf@suse.de>
---
hw/unin_pci.c | 14 +++++++++++---
1 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index a3b0435..3fa7850 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -36,22 +36,30 @@
#define UNIN_DPRINTF(fmt, ...)
#endif
+static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
+
typedef struct UNINState {
SysBusDevice busdev;
PCIHostState host_state;
} UNINState;
-/* Don't know if this matches real hardware, but it agrees with OHW. */
static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
{
- return (irq_num + (pci_dev->devfn >> 3)) & 3;
+ int retval;
+ int devfn = pci_dev->devfn & 0x00FFFFFF;
+
+ retval = (((devfn >> 11) & 0x1F) + irq_num) & 3;
+
+ return retval;
}
static void pci_unin_set_irq(void *opaque, int irq_num, int level)
{
qemu_irq *pic = opaque;
- qemu_set_irq(pic[irq_num + 8], level);
+ UNIN_DPRINTF("%s: setting INT %d = %d\n", __func__,
+ unin_irq_line[irq_num], level);
+ qemu_set_irq(pic[unin_irq_line[irq_num]], level);
}
static void pci_unin_save(QEMUFile* f, void *opaque)
--
1.6.0.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 6/6] Enable secondary cmd64x
2010-01-04 7:32 [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2 Alexander Graf
` (4 preceding siblings ...)
2010-01-04 7:32 ` [Qemu-devel] [PATCH 5/6] Make interrupts work Alexander Graf
@ 2010-01-04 7:32 ` Alexander Graf
5 siblings, 0 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-04 7:32 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Aurelien Jarno
We're not using any macio IDE devices, so let's enable the secondary cmd64x
IDE device, so we get all four possible IDE devices exposed to the guest.
Later we definitely need to enable macio or any other device that Linux
understands in default configurations.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
hw/ppc_newworld.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index 308e102..d66860b 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -343,7 +343,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
}
dbdma = DBDMA_init(&dbdma_mem_index);
- pci_cmd646_ide_init(pci_bus, hd, 0);
+ pci_cmd646_ide_init(pci_bus, hd, 1);
/* cuda also initialize ADB */
cuda_init(&cuda_mem_index, pic[0x19]);
--
1.6.0.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] PCI config space access overhaul
2010-01-04 7:32 ` [Qemu-devel] [PATCH 1/6] PCI config space access overhaul Alexander Graf
@ 2010-01-05 12:46 ` Isaku Yamahata
2010-01-05 13:11 ` Michael S. Tsirkin
2010-01-12 10:36 ` Alexander Graf
2010-01-05 22:16 ` [Qemu-devel] " Michael S. Tsirkin
1 sibling, 2 replies; 13+ messages in thread
From: Isaku Yamahata @ 2010-01-05 12:46 UTC (permalink / raw)
To: Alexander Graf
Cc: Blue Swirl, QEMU Developers, Aurelien Jarno, Michael S. Tsirkin
Basically it looks good.
Some minor comments below.
Although further clean up is possible,
this is first small step.
On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote:
> Different host buses may have different layouts for config space accessors.
>
> The Mac U3 for example uses the following define to access Type 0 (directly
> attached) devices:
>
> #define MACRISC_CFA0(devfn, off) \
> ((1 << (unsigned int)PCI_SLOT(dev_fn)) \
> | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \
> | (((unsigned int)(off)) & 0xFCUL))
>
> Obviously, this is different from the encoding we interpret in qemu. In
> order to let host controllers take some influence there, we can just
> introduce a decoding function that takes whatever accessor we have and
> decodes it into understandable fields.
>
> To not touch all different code paths in qemu, I added this on top of
> the existing config_reg element. In the future, we should work on gradually
> moving references to config_reg to config_reg_dec and then phase out
> config_reg.
>
> This patch is the groundwork for Uninorth PCI config space fixes.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> ---
>
> v1 -> v2:
>
> - merge data access functions
> - use decoding function for config space bits
> - introduce encoding function for x86 style encodings
>
> ---
> hw/apb_pci.c | 1 +
> hw/grackle_pci.c | 1 +
> hw/gt64xxx.c | 1 +
> hw/pci.h | 13 ++++++
> hw/pci_host.c | 48 ++++++++++++++++++-----
> hw/pci_host.h | 16 ++++++++
> hw/pci_host_template.h | 90 +++++++++++++-------------------------------
> hw/pci_host_template_all.h | 23 +++++++++++
> hw/piix_pci.c | 1 +
> hw/ppc4xx_pci.c | 1 +
> hw/ppce500_pci.c | 1 +
> hw/unin_pci.c | 1 +
> 12 files changed, 123 insertions(+), 74 deletions(-)
> create mode 100644 hw/pci_host_template_all.h
>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index f05308b..fedb84c 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -222,6 +222,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
> /* mem_data */
> sysbus_mmio_map(s, 3, mem_base);
> d = FROM_SYSBUS(APBState, s);
> + pci_host_init(&d->host_state);
> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> pci_apb_set_irq, pci_pbm_map_irq, pic,
> 0, 32);
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index ee4fed5..7feba63 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -88,6 +88,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic)
> qdev_init_nofail(dev);
> s = sysbus_from_qdev(dev);
> d = FROM_SYSBUS(GrackleState, s);
> + pci_host_init(&d->host_state);
> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> pci_grackle_set_irq,
> pci_grackle_map_irq,
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index fb7f5bd..8cf93ca 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1120,6 +1120,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
> s = qemu_mallocz(sizeof(GT64120State));
> s->pci = qemu_mallocz(sizeof(GT64120PCIState));
>
> + pci_host_init(s->pci);
> s->pci->bus = pci_register_bus(NULL, "pci",
> pci_gt64120_set_irq, pci_gt64120_map_irq,
> pic, 144, 4);
> diff --git a/hw/pci.h b/hw/pci.h
> index fd16460..cfaa0a9 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -10,10 +10,23 @@
>
> /* PCI bus */
>
> +typedef struct PCIAddress {
> + PCIBus *domain;
> + uint8_t bus;
> + uint8_t slot;
> + uint8_t fn;
> +} PCIAddress;
> +
> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
> #define PCI_FUNC(devfn) ((devfn) & 0x07)
>
> +static inline uint32_t pci_addr_to_devfn(PCIAddress *addr, uint32_t offset)
> +{
> + return ((addr->bus & 0xff) << 16) | (PCI_DEVFN(addr->slot, addr->fn) << 8) |
> + offset;
> +}
> +
Hmm, this name doesn't seem good.
devfn sounds something like encoded (slot, fn) pair (to me),
but the function returns something else.
This function is used for pci_data_{read, write}()
which again decodes bus, slot, fn.
So shouldn't they be changed to accept PCIAddress itself?
> /* Class, Vendor and Device IDs from Linux's pci_ids.h */
> #include "pci_ids.h"
>
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index eeb8dee..746ffc2 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -32,13 +32,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> #define PCI_DPRINTF(fmt, ...)
> #endif
>
> -/*
> - * PCI address
> - * bit 16 - 24: bus number
> - * bit 8 - 15: devfun number
> - * bit 0 - 7: offset in configuration space of a given pci device
> - */
> -
> /* 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)
> {
> @@ -56,7 +49,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> if (!pci_dev)
> return;
>
> - PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> + PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
Good catch. This part should be split into another patch.
> __func__, pci_dev->name, config_addr, val, len);
> pci_dev->config_write(pci_dev, config_addr, val, len);
> }
> @@ -89,7 +82,9 @@ static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> #endif
> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> __func__, addr, val);
> +
> s->config_reg = val;
> + s->decode_config_reg(s, val, &s->config_reg_dec);
> }
>
> static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
> @@ -131,7 +126,9 @@ static void pci_host_config_writel_noswap(void *opaque,
>
> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> __func__, addr, val);
> +
> s->config_reg = val;
> + s->decode_config_reg(s, val, &s->config_reg_dec);
> }
>
> static uint32_t pci_host_config_readl_noswap(void *opaque,
> @@ -169,7 +166,9 @@ static void pci_host_config_writel_ioport(void *opaque,
> PCIHostState *s = opaque;
>
> PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, val);
> +
> s->config_reg = val;
> + s->decode_config_reg(s, val, &s->config_reg_dec);
> }
>
> static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
> @@ -190,7 +189,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 +216,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)
> {
> @@ -228,3 +227,32 @@ void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
> register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
> register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
> }
> +
> +/* This implements the default x86 way of interpreting a PCI address
> + *
> + * PCI address
> + * bit 16 - 24: bus number
> + * bit 11 - 15: slot number
> + * bit 8 - 10: function number
> + * bit 0 - 7: offset in configuration space of a given pci device
> + */
> +
> +void pci_host_decode_config_reg(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;
> + decoded->addr.slot = (config_reg >> 11) & 0x1f;
> + decoded->addr.fn = (config_reg >> 8) & 0x7;
> + decoded->offset = config_reg & 0xff;
> + decoded->addr_mask = 3;
> + decoded->valid = (config_reg & (1u << 31)) ? true : false;
> +}
> +
> +void pci_host_init(PCIHostState *s)
> +{
> + s->decode_config_reg = pci_host_decode_config_reg;
> +}
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index a006687..0fcdf64 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -30,14 +30,30 @@
>
> #include "sysbus.h"
>
> +/* for config space access */
> +typedef struct PCIConfigAddress {
> + PCIAddress addr;
> + uint32_t addr_mask;
> + uint16_t offset;
> + bool valid;
> +} PCIConfigAddress;
> +
> +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg,
> + PCIConfigAddress *conf);
> +
> struct PCIHostState {
> SysBusDevice busdev;
> + pci_config_reg_fn decode_config_reg;
> + PCIConfigAddress config_reg_dec;
> uint32_t config_reg;
> PCIBus *bus;
> };
>
> 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_host_init(PCIHostState *s);
> +void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg,
> + PCIConfigAddress *decoded);
>
> /* 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 11e6c88..d989c25 100644
> --- a/hw/pci_host_template.h
> +++ b/hw/pci_host_template.h
> @@ -25,85 +25,47 @@
> /* 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;
> + PCIConfigAddress *config_reg = &s->config_reg_dec;
> + int offset = config_reg->offset;
>
> - 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);
> +#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
> + val = glue(bswap, PCI_HOST_BITS)(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);
> -#endif
> - PCI_DPRINTF("writel 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, val, 4);
> + offset |= (addr & config_reg->addr_mask);
> + PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
> + __func__, (target_phys_addr_t)addr, val);
> + if (config_reg->valid) {
> + pci_data_write(s->bus, pci_addr_to_devfn(&config_reg->addr, offset), 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;
> + PCIConfigAddress *config_reg = &s->config_reg_dec;
> + int offset = config_reg->offset;
> 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 (!config_reg->valid) {
> + return ( 1ULL << PCI_HOST_BITS ) - 1;
Are you using intentionally 1ULL (64bit) not to overflow it?
> + }
>
> -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;
> -}
> + offset |= (addr & config_reg->addr_mask);
> + val = pci_data_read(s->bus, pci_addr_to_devfn(&config_reg->addr, offset),
> + 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);
>
> -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);
> +#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
Oh, another new cpp tricks.
I'm ok with this trick. However Michael may have his own idea.
This trick would be split out into independent patch.
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 1b67475..97500e0 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -211,6 +211,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
>
> dev = qdev_create(NULL, "i440FX-pcihost");
> s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
> + pci_host_init(s);
> b = pci_bus_new(&s->busdev.qdev, NULL, 0);
> s->bus = b;
> qdev_init_nofail(dev);
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 2d00b61..dd8e290 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -357,6 +357,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
>
> controller = qemu_mallocz(sizeof(PPC4xxPCIState));
>
> + pci_host_init(&controller->pci_state);
> controller->pci_state.bus = pci_register_bus(NULL, "pci",
> ppc4xx_pci_set_irq,
> ppc4xx_pci_map_irq,
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index a72fb86..5914183 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -278,6 +278,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>
> controller = qemu_mallocz(sizeof(PPCE500PCIState));
>
> + pci_host_init(&controller->pci_state);
> controller->pci_state.bus = pci_register_bus(NULL, "pci",
> mpc85xx_pci_set_irq,
> mpc85xx_pci_map_irq,
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 3ae4e7a..fdb9401 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -84,6 +84,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
> /* Uninorth main bus */
> s = FROM_SYSBUS(UNINState, dev);
>
> + pci_host_init(&s->host_state);
> pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
> pci_mem_data = pci_host_data_register_mmio(&s->host_state);
> sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> --
> 1.6.0.2
>
>
>
--
yamahata
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] PCI config space access overhaul
2010-01-05 12:46 ` Isaku Yamahata
@ 2010-01-05 13:11 ` Michael S. Tsirkin
2010-01-12 10:36 ` Alexander Graf
1 sibling, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2010-01-05 13:11 UTC (permalink / raw)
To: Isaku Yamahata
Cc: Blue Swirl, Alexander Graf, Aurelien Jarno, QEMU Developers
On Tue, Jan 05, 2010 at 09:46:38PM +0900, Isaku Yamahata wrote:
> > 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
>
> Oh, another new cpp tricks.
> I'm ok with this trick. However Michael may have his own idea.
I'm ok, yes. Though long term, we should think about switching to an
API that does not result in all this horrible boilerplate code that we
are then forced to work around with macros. And it need not be hard: we
just want
1. bswap(addr, len)
2. wrapper around cpu_register_io_memory that gets
length and passes it on.
> This trick would be split out into independent patch.
Yes.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 1/6] PCI config space access overhaul
2010-01-04 7:32 ` [Qemu-devel] [PATCH 1/6] PCI config space access overhaul Alexander Graf
2010-01-05 12:46 ` Isaku Yamahata
@ 2010-01-05 22:16 ` Michael S. Tsirkin
2010-01-12 10:38 ` Alexander Graf
1 sibling, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2010-01-05 22:16 UTC (permalink / raw)
To: Alexander Graf; +Cc: Blue Swirl, QEMU Developers, Aurelien Jarno
On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote:
> +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg,
> + PCIConfigAddress *conf);
> +
pci_decode_config_addr_fn would be a better name.
> struct PCIHostState {
> SysBusDevice busdev;
> + pci_config_reg_fn decode_config_reg;
> + PCIConfigAddress config_reg_dec;
decode_config_addr
and
config_addr
would be better names
> uint32_t config_reg;
> PCIBus *bus;
> };
>
> 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_host_init(PCIHostState *s);
> +void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg,
> + PCIConfigAddress *decoded);
Shouldn't this be static?
And again, pci_host_decode_config_addr would be a better name IMO.
--
MST
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] PCI config space access overhaul
2010-01-05 12:46 ` Isaku Yamahata
2010-01-05 13:11 ` Michael S. Tsirkin
@ 2010-01-12 10:36 ` Alexander Graf
2010-01-12 10:59 ` Michael S. Tsirkin
1 sibling, 1 reply; 13+ messages in thread
From: Alexander Graf @ 2010-01-12 10:36 UTC (permalink / raw)
To: Isaku Yamahata
Cc: Blue Swirl, QEMU Developers, Aurelien Jarno, Michael S. Tsirkin
On 05.01.2010, at 13:46, Isaku Yamahata wrote:
> Basically it looks good.
> Some minor comments below.
>
> Although further clean up is possible,
> this is first small step.
>
> On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote:
>> Different host buses may have different layouts for config space accessors.
>>
>> The Mac U3 for example uses the following define to access Type 0 (directly
>> attached) devices:
>>
>> #define MACRISC_CFA0(devfn, off) \
>> ((1 << (unsigned int)PCI_SLOT(dev_fn)) \
>> | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \
>> | (((unsigned int)(off)) & 0xFCUL))
>>
>> Obviously, this is different from the encoding we interpret in qemu. In
>> order to let host controllers take some influence there, we can just
>> introduce a decoding function that takes whatever accessor we have and
>> decodes it into understandable fields.
>>
>> To not touch all different code paths in qemu, I added this on top of
>> the existing config_reg element. In the future, we should work on gradually
>> moving references to config_reg to config_reg_dec and then phase out
>> config_reg.
>>
>> This patch is the groundwork for Uninorth PCI config space fixes.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> ---
>>
>> v1 -> v2:
>>
>> - merge data access functions
>> - use decoding function for config space bits
>> - introduce encoding function for x86 style encodings
>>
>> ---
>> hw/apb_pci.c | 1 +
>> hw/grackle_pci.c | 1 +
>> hw/gt64xxx.c | 1 +
>> hw/pci.h | 13 ++++++
>> hw/pci_host.c | 48 ++++++++++++++++++-----
>> hw/pci_host.h | 16 ++++++++
>> hw/pci_host_template.h | 90 +++++++++++++-------------------------------
>> hw/pci_host_template_all.h | 23 +++++++++++
>> hw/piix_pci.c | 1 +
>> hw/ppc4xx_pci.c | 1 +
>> hw/ppce500_pci.c | 1 +
>> hw/unin_pci.c | 1 +
>> 12 files changed, 123 insertions(+), 74 deletions(-)
>> create mode 100644 hw/pci_host_template_all.h
>>
>> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
>> index f05308b..fedb84c 100644
>> --- a/hw/apb_pci.c
>> +++ b/hw/apb_pci.c
>> @@ -222,6 +222,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
>> /* mem_data */
>> sysbus_mmio_map(s, 3, mem_base);
>> d = FROM_SYSBUS(APBState, s);
>> + pci_host_init(&d->host_state);
>> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
>> pci_apb_set_irq, pci_pbm_map_irq, pic,
>> 0, 32);
>> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
>> index ee4fed5..7feba63 100644
>> --- a/hw/grackle_pci.c
>> +++ b/hw/grackle_pci.c
>> @@ -88,6 +88,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic)
>> qdev_init_nofail(dev);
>> s = sysbus_from_qdev(dev);
>> d = FROM_SYSBUS(GrackleState, s);
>> + pci_host_init(&d->host_state);
>> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
>> pci_grackle_set_irq,
>> pci_grackle_map_irq,
>> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
>> index fb7f5bd..8cf93ca 100644
>> --- a/hw/gt64xxx.c
>> +++ b/hw/gt64xxx.c
>> @@ -1120,6 +1120,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
>> s = qemu_mallocz(sizeof(GT64120State));
>> s->pci = qemu_mallocz(sizeof(GT64120PCIState));
>>
>> + pci_host_init(s->pci);
>> s->pci->bus = pci_register_bus(NULL, "pci",
>> pci_gt64120_set_irq, pci_gt64120_map_irq,
>> pic, 144, 4);
>> diff --git a/hw/pci.h b/hw/pci.h
>> index fd16460..cfaa0a9 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -10,10 +10,23 @@
>>
>> /* PCI bus */
>>
>> +typedef struct PCIAddress {
>> + PCIBus *domain;
>> + uint8_t bus;
>> + uint8_t slot;
>> + uint8_t fn;
>> +} PCIAddress;
>> +
>> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
>> #define PCI_FUNC(devfn) ((devfn) & 0x07)
>>
>> +static inline uint32_t pci_addr_to_devfn(PCIAddress *addr, uint32_t offset)
>> +{
>> + return ((addr->bus & 0xff) << 16) | (PCI_DEVFN(addr->slot, addr->fn) << 8) |
>> + offset;
>> +}
>> +
>
> Hmm, this name doesn't seem good.
> devfn sounds something like encoded (slot, fn) pair (to me),
> but the function returns something else.
I'm open for naming suggestions.
>
> This function is used for pci_data_{read, write}()
> which again decodes bus, slot, fn.
> So shouldn't they be changed to accept PCIAddress itself?
>
>
>> /* Class, Vendor and Device IDs from Linux's pci_ids.h */
>> #include "pci_ids.h"
>>
>> diff --git a/hw/pci_host.c b/hw/pci_host.c
>> index eeb8dee..746ffc2 100644
>> --- a/hw/pci_host.c
>> +++ b/hw/pci_host.c
>> @@ -32,13 +32,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>> #define PCI_DPRINTF(fmt, ...)
>> #endif
>>
>> -/*
>> - * PCI address
>> - * bit 16 - 24: bus number
>> - * bit 8 - 15: devfun number
>> - * bit 0 - 7: offset in configuration space of a given pci device
>> - */
>> -
>> /* 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)
>> {
>> @@ -56,7 +49,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>> if (!pci_dev)
>> return;
>>
>> - PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
>> + PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
>
> Good catch. This part should be split into another patch.
Right :).
>
>> __func__, pci_dev->name, config_addr, val, len);
>> pci_dev->config_write(pci_dev, config_addr, val, len);
>> }
>> @@ -89,7 +82,9 @@ static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
>> #endif
>> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
>> __func__, addr, val);
>> +
>> s->config_reg = val;
>> + s->decode_config_reg(s, val, &s->config_reg_dec);
>> }
>>
>> static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
>> @@ -131,7 +126,9 @@ static void pci_host_config_writel_noswap(void *opaque,
>>
>> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
>> __func__, addr, val);
>> +
>> s->config_reg = val;
>> + s->decode_config_reg(s, val, &s->config_reg_dec);
>> }
>>
>> static uint32_t pci_host_config_readl_noswap(void *opaque,
>> @@ -169,7 +166,9 @@ static void pci_host_config_writel_ioport(void *opaque,
>> PCIHostState *s = opaque;
>>
>> PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, val);
>> +
>> s->config_reg = val;
>> + s->decode_config_reg(s, val, &s->config_reg_dec);
>> }
>>
>> static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
>> @@ -190,7 +189,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 +216,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)
>> {
>> @@ -228,3 +227,32 @@ void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
>> register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
>> register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
>> }
>> +
>> +/* This implements the default x86 way of interpreting a PCI address
>> + *
>> + * PCI address
>> + * bit 16 - 24: bus number
>> + * bit 11 - 15: slot number
>> + * bit 8 - 10: function number
>> + * bit 0 - 7: offset in configuration space of a given pci device
>> + */
>> +
>> +void pci_host_decode_config_reg(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;
>> + decoded->addr.slot = (config_reg >> 11) & 0x1f;
>> + decoded->addr.fn = (config_reg >> 8) & 0x7;
>> + decoded->offset = config_reg & 0xff;
>> + decoded->addr_mask = 3;
>> + decoded->valid = (config_reg & (1u << 31)) ? true : false;
>> +}
>> +
>> +void pci_host_init(PCIHostState *s)
>> +{
>> + s->decode_config_reg = pci_host_decode_config_reg;
>> +}
>> diff --git a/hw/pci_host.h b/hw/pci_host.h
>> index a006687..0fcdf64 100644
>> --- a/hw/pci_host.h
>> +++ b/hw/pci_host.h
>> @@ -30,14 +30,30 @@
>>
>> #include "sysbus.h"
>>
>> +/* for config space access */
>> +typedef struct PCIConfigAddress {
>> + PCIAddress addr;
>> + uint32_t addr_mask;
>> + uint16_t offset;
>> + bool valid;
>> +} PCIConfigAddress;
>> +
>> +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg,
>> + PCIConfigAddress *conf);
>> +
>> struct PCIHostState {
>> SysBusDevice busdev;
>> + pci_config_reg_fn decode_config_reg;
>> + PCIConfigAddress config_reg_dec;
>> uint32_t config_reg;
>> PCIBus *bus;
>> };
>>
>> 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_host_init(PCIHostState *s);
>> +void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg,
>> + PCIConfigAddress *decoded);
>>
>> /* 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 11e6c88..d989c25 100644
>> --- a/hw/pci_host_template.h
>> +++ b/hw/pci_host_template.h
>> @@ -25,85 +25,47 @@
>> /* 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;
>> + PCIConfigAddress *config_reg = &s->config_reg_dec;
>> + int offset = config_reg->offset;
>>
>> - 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);
>> +#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
>> + val = glue(bswap, PCI_HOST_BITS)(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);
>> -#endif
>> - PCI_DPRINTF("writel 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, val, 4);
>> + offset |= (addr & config_reg->addr_mask);
>> + PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
>> + __func__, (target_phys_addr_t)addr, val);
>> + if (config_reg->valid) {
>> + pci_data_write(s->bus, pci_addr_to_devfn(&config_reg->addr, offset), 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;
>> + PCIConfigAddress *config_reg = &s->config_reg_dec;
>> + int offset = config_reg->offset;
>> 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 (!config_reg->valid) {
>> + return ( 1ULL << PCI_HOST_BITS ) - 1;
>
> Are you using intentionally 1ULL (64bit) not to overflow it?
We are overflowing it for a short amount of time.
1 << PCI_HOST_BITS = 0x100000000
0x100000000 - 1 = 0xffffffff
That's why we need the ULL.
>
>
>> + }
>>
>> -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;
>> -}
>> + offset |= (addr & config_reg->addr_mask);
>> + val = pci_data_read(s->bus, pci_addr_to_devfn(&config_reg->addr, offset),
>> + 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);
>>
>> -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);
>> +#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
>
> Oh, another new cpp tricks.
> I'm ok with this trick. However Michael may have his own idea.
> This trick would be split out into independent patch.
Well I got fed up with having to change 10 lines of code that all look the same ;-).
Alex
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 1/6] PCI config space access overhaul
2010-01-05 22:16 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-01-12 10:38 ` Alexander Graf
0 siblings, 0 replies; 13+ messages in thread
From: Alexander Graf @ 2010-01-12 10:38 UTC (permalink / raw)
To: Michael S.Tsirkin; +Cc: Blue Swirl, QEMU Developers, Aurelien Jarno
On 05.01.2010, at 23:16, Michael S. Tsirkin wrote:
> On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote:
>> +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg,
>> + PCIConfigAddress *conf);
>> +
>
> pci_decode_config_addr_fn would be a better name.
>
>> struct PCIHostState {
>> SysBusDevice busdev;
>> + pci_config_reg_fn decode_config_reg;
>> + PCIConfigAddress config_reg_dec;
>
> decode_config_addr
> and
> config_addr
>
> would be better names
>
>> uint32_t config_reg;
>> PCIBus *bus;
>> };
>>
>> 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_host_init(PCIHostState *s);
>> +void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg,
>> + PCIConfigAddress *decoded);
>
> Shouldn't this be static?
No, since I want to call it from unin for the OpenBIOS compatibility call. We still use the x86 encoding for the BIOS (for now).
> And again, pci_host_decode_config_addr would be a better name IMO.
Alrighty.
Alex
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] PCI config space access overhaul
2010-01-12 10:36 ` Alexander Graf
@ 2010-01-12 10:59 ` Michael S. Tsirkin
0 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2010-01-12 10:59 UTC (permalink / raw)
To: Alexander Graf
Cc: Blue Swirl, Isaku Yamahata, QEMU Developers, Aurelien Jarno
On Tue, Jan 12, 2010 at 11:36:58AM +0100, Alexander Graf wrote:
>
> On 05.01.2010, at 13:46, Isaku Yamahata wrote:
>
> > Basically it looks good.
> > Some minor comments below.
> >
> > Although further clean up is possible,
> > this is first small step.
> >
> > On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote:
> >> Different host buses may have different layouts for config space accessors.
> >>
> >> The Mac U3 for example uses the following define to access Type 0 (directly
> >> attached) devices:
> >>
> >> #define MACRISC_CFA0(devfn, off) \
> >> ((1 << (unsigned int)PCI_SLOT(dev_fn)) \
> >> | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \
> >> | (((unsigned int)(off)) & 0xFCUL))
> >>
> >> Obviously, this is different from the encoding we interpret in qemu. In
> >> order to let host controllers take some influence there, we can just
> >> introduce a decoding function that takes whatever accessor we have and
> >> decodes it into understandable fields.
> >>
> >> To not touch all different code paths in qemu, I added this on top of
> >> the existing config_reg element. In the future, we should work on gradually
> >> moving references to config_reg to config_reg_dec and then phase out
> >> config_reg.
> >>
> >> This patch is the groundwork for Uninorth PCI config space fixes.
> >>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> CC: Michael S. Tsirkin <mst@redhat.com>
> >> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >>
> >> - merge data access functions
> >> - use decoding function for config space bits
> >> - introduce encoding function for x86 style encodings
> >>
> >> ---
> >> hw/apb_pci.c | 1 +
> >> hw/grackle_pci.c | 1 +
> >> hw/gt64xxx.c | 1 +
> >> hw/pci.h | 13 ++++++
> >> hw/pci_host.c | 48 ++++++++++++++++++-----
> >> hw/pci_host.h | 16 ++++++++
> >> hw/pci_host_template.h | 90 +++++++++++++-------------------------------
> >> hw/pci_host_template_all.h | 23 +++++++++++
> >> hw/piix_pci.c | 1 +
> >> hw/ppc4xx_pci.c | 1 +
> >> hw/ppce500_pci.c | 1 +
> >> hw/unin_pci.c | 1 +
> >> 12 files changed, 123 insertions(+), 74 deletions(-)
> >> create mode 100644 hw/pci_host_template_all.h
> >>
> >> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> >> index f05308b..fedb84c 100644
> >> --- a/hw/apb_pci.c
> >> +++ b/hw/apb_pci.c
> >> @@ -222,6 +222,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
> >> /* mem_data */
> >> sysbus_mmio_map(s, 3, mem_base);
> >> d = FROM_SYSBUS(APBState, s);
> >> + pci_host_init(&d->host_state);
> >> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> >> pci_apb_set_irq, pci_pbm_map_irq, pic,
> >> 0, 32);
> >> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> >> index ee4fed5..7feba63 100644
> >> --- a/hw/grackle_pci.c
> >> +++ b/hw/grackle_pci.c
> >> @@ -88,6 +88,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic)
> >> qdev_init_nofail(dev);
> >> s = sysbus_from_qdev(dev);
> >> d = FROM_SYSBUS(GrackleState, s);
> >> + pci_host_init(&d->host_state);
> >> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> >> pci_grackle_set_irq,
> >> pci_grackle_map_irq,
> >> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> >> index fb7f5bd..8cf93ca 100644
> >> --- a/hw/gt64xxx.c
> >> +++ b/hw/gt64xxx.c
> >> @@ -1120,6 +1120,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
> >> s = qemu_mallocz(sizeof(GT64120State));
> >> s->pci = qemu_mallocz(sizeof(GT64120PCIState));
> >>
> >> + pci_host_init(s->pci);
> >> s->pci->bus = pci_register_bus(NULL, "pci",
> >> pci_gt64120_set_irq, pci_gt64120_map_irq,
> >> pic, 144, 4);
> >> diff --git a/hw/pci.h b/hw/pci.h
> >> index fd16460..cfaa0a9 100644
> >> --- a/hw/pci.h
> >> +++ b/hw/pci.h
> >> @@ -10,10 +10,23 @@
> >>
> >> /* PCI bus */
> >>
> >> +typedef struct PCIAddress {
> >> + PCIBus *domain;
> >> + uint8_t bus;
> >> + uint8_t slot;
> >> + uint8_t fn;
> >> +} PCIAddress;
> >> +
> >> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> >> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
> >> #define PCI_FUNC(devfn) ((devfn) & 0x07)
> >>
> >> +static inline uint32_t pci_addr_to_devfn(PCIAddress *addr, uint32_t offset)
> >> +{
> >> + return ((addr->bus & 0xff) << 16) | (PCI_DEVFN(addr->slot, addr->fn) << 8) |
> >> + offset;
> >> +}
> >> +
> >
> > Hmm, this name doesn't seem good.
> > devfn sounds something like encoded (slot, fn) pair (to me),
> > but the function returns something else.
>
> I'm open for naming suggestions.
Does it encode to config_reg format? If so pci_addr_to_config_reg?
> >
> > This function is used for pci_data_{read, write}()
> > which again decodes bus, slot, fn.
> > So shouldn't they be changed to accept PCIAddress itself?
> >
> >
> >> /* Class, Vendor and Device IDs from Linux's pci_ids.h */
> >> #include "pci_ids.h"
> >>
> >> diff --git a/hw/pci_host.c b/hw/pci_host.c
> >> index eeb8dee..746ffc2 100644
> >> --- a/hw/pci_host.c
> >> +++ b/hw/pci_host.c
> >> @@ -32,13 +32,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> >> #define PCI_DPRINTF(fmt, ...)
> >> #endif
> >>
> >> -/*
> >> - * PCI address
> >> - * bit 16 - 24: bus number
> >> - * bit 8 - 15: devfun number
> >> - * bit 0 - 7: offset in configuration space of a given pci device
> >> - */
> >> -
> >> /* 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)
> >> {
> >> @@ -56,7 +49,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> >> if (!pci_dev)
> >> return;
> >>
> >> - PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> >> + PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> >
> > Good catch. This part should be split into another patch.
>
> Right :).
>
> >
> >> __func__, pci_dev->name, config_addr, val, len);
> >> pci_dev->config_write(pci_dev, config_addr, val, len);
> >> }
> >> @@ -89,7 +82,9 @@ static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> >> #endif
> >> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> >> __func__, addr, val);
> >> +
> >> s->config_reg = val;
> >> + s->decode_config_reg(s, val, &s->config_reg_dec);
> >> }
> >>
> >> static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
> >> @@ -131,7 +126,9 @@ static void pci_host_config_writel_noswap(void *opaque,
> >>
> >> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> >> __func__, addr, val);
> >> +
> >> s->config_reg = val;
> >> + s->decode_config_reg(s, val, &s->config_reg_dec);
> >> }
> >>
> >> static uint32_t pci_host_config_readl_noswap(void *opaque,
> >> @@ -169,7 +166,9 @@ static void pci_host_config_writel_ioport(void *opaque,
> >> PCIHostState *s = opaque;
> >>
> >> PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, val);
> >> +
> >> s->config_reg = val;
> >> + s->decode_config_reg(s, val, &s->config_reg_dec);
> >> }
> >>
> >> static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
> >> @@ -190,7 +189,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 +216,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)
> >> {
> >> @@ -228,3 +227,32 @@ void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
> >> register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
> >> register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
> >> }
> >> +
> >> +/* This implements the default x86 way of interpreting a PCI address
> >> + *
> >> + * PCI address
> >> + * bit 16 - 24: bus number
> >> + * bit 11 - 15: slot number
> >> + * bit 8 - 10: function number
> >> + * bit 0 - 7: offset in configuration space of a given pci device
> >> + */
> >> +
> >> +void pci_host_decode_config_reg(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;
> >> + decoded->addr.slot = (config_reg >> 11) & 0x1f;
> >> + decoded->addr.fn = (config_reg >> 8) & 0x7;
> >> + decoded->offset = config_reg & 0xff;
> >> + decoded->addr_mask = 3;
> >> + decoded->valid = (config_reg & (1u << 31)) ? true : false;
> >> +}
> >> +
> >> +void pci_host_init(PCIHostState *s)
> >> +{
> >> + s->decode_config_reg = pci_host_decode_config_reg;
> >> +}
> >> diff --git a/hw/pci_host.h b/hw/pci_host.h
> >> index a006687..0fcdf64 100644
> >> --- a/hw/pci_host.h
> >> +++ b/hw/pci_host.h
> >> @@ -30,14 +30,30 @@
> >>
> >> #include "sysbus.h"
> >>
> >> +/* for config space access */
> >> +typedef struct PCIConfigAddress {
> >> + PCIAddress addr;
> >> + uint32_t addr_mask;
> >> + uint16_t offset;
> >> + bool valid;
> >> +} PCIConfigAddress;
> >> +
> >> +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg,
> >> + PCIConfigAddress *conf);
> >> +
> >> struct PCIHostState {
> >> SysBusDevice busdev;
> >> + pci_config_reg_fn decode_config_reg;
> >> + PCIConfigAddress config_reg_dec;
> >> uint32_t config_reg;
> >> PCIBus *bus;
> >> };
> >>
> >> 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_host_init(PCIHostState *s);
> >> +void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg,
> >> + PCIConfigAddress *decoded);
> >>
> >> /* 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 11e6c88..d989c25 100644
> >> --- a/hw/pci_host_template.h
> >> +++ b/hw/pci_host_template.h
> >> @@ -25,85 +25,47 @@
> >> /* 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;
> >> + PCIConfigAddress *config_reg = &s->config_reg_dec;
> >> + int offset = config_reg->offset;
> >>
> >> - 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);
> >> +#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
> >> + val = glue(bswap, PCI_HOST_BITS)(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);
> >> -#endif
> >> - PCI_DPRINTF("writel 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, val, 4);
> >> + offset |= (addr & config_reg->addr_mask);
> >> + PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
> >> + __func__, (target_phys_addr_t)addr, val);
> >> + if (config_reg->valid) {
> >> + pci_data_write(s->bus, pci_addr_to_devfn(&config_reg->addr, offset), 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;
> >> + PCIConfigAddress *config_reg = &s->config_reg_dec;
> >> + int offset = config_reg->offset;
> >> 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 (!config_reg->valid) {
> >> + return ( 1ULL << PCI_HOST_BITS ) - 1;
> >
> > Are you using intentionally 1ULL (64bit) not to overflow it?
>
> We are overflowing it for a short amount of time.
>
> 1 << PCI_HOST_BITS = 0x100000000
> 0x100000000 - 1 = 0xffffffff
>
> That's why we need the ULL.
OK. However, please do not put space after ( and before ).
I also suspect just return ~0x0 will do the trick as well
(but not sure).
> >
> >
> >> + }
> >>
> >> -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;
> >> -}
> >> + offset |= (addr & config_reg->addr_mask);
> >> + val = pci_data_read(s->bus, pci_addr_to_devfn(&config_reg->addr, offset),
> >> + 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);
> >>
> >> -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);
> >> +#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
> >
> > Oh, another new cpp tricks.
> > I'm ok with this trick. However Michael may have his own idea.
> > This trick would be split out into independent patch.
>
> Well I got fed up with having to change 10 lines of code that all look the same ;-).
>
> Alex
Yea, ok. Maybe split it out but it's not critical either.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-01-12 11:02 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-04 7:32 [Qemu-devel] [PATCH 0/6] PPC NewWorld fixery v2 Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 1/6] PCI config space access overhaul Alexander Graf
2010-01-05 12:46 ` Isaku Yamahata
2010-01-05 13:11 ` Michael S. Tsirkin
2010-01-12 10:36 ` Alexander Graf
2010-01-12 10:59 ` Michael S. Tsirkin
2010-01-05 22:16 ` [Qemu-devel] " Michael S. Tsirkin
2010-01-12 10:38 ` Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 2/6] Add config space conversion function for uni_north Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 3/6] Use Mac99_U3 type on ppc64 Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 4/6] Include dump of lspci -nn on real G5 Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 5/6] Make interrupts work Alexander Graf
2010-01-04 7:32 ` [Qemu-devel] [PATCH 6/6] Enable secondary cmd64x 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).