[Qemu-devel] [PATCH 0/6] PPC NewWorld fixery

[Qemu-devel] [PATCH 0/6] PPC NewWorld fixery

[Alexander Graf]

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 patch at: http://alex.csgraf.de/openbios-ppc-u3.patch

Alexander Graf (6):
  Make config space accessor host bus trapable
  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_host.c          |   11 ++++
 hw/pci_host.h          |    5 ++
 hw/pci_host_template.h |   30 ++++++----
 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          |  145 +++++++++++++++++++++++++++++++++++++++++++++++-
 14 files changed, 196 insertions(+), 18 deletions(-)

[Qemu-devel] [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

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 converter function that takes whatever accessor we have and
makes a qemu understandable one out of it.

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>
---
 hw/apb_pci.c           |    1 +
 hw/grackle_pci.c       |    1 +
 hw/gt64xxx.c           |    1 +
 hw/pci_host.c          |   11 +++++++++++
 hw/pci_host.h          |    5 +++++
 hw/pci_host_template.h |   30 ++++++++++++++++++------------
 hw/piix_pci.c          |    1 +
 hw/ppc4xx_pci.c        |    1 +
 hw/ppce500_pci.c       |    1 +
 hw/unin_pci.c          |    1 +
 10 files changed, 41 insertions(+), 12 deletions(-)

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_host.c b/hw/pci_host.c
index eeb8dee..2d12887 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -228,3 +228,14 @@ 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 the config register */
+static uint32_t pci_host_get_config_reg(PCIHostState *s, uint32_t addr)
+{
+    return s->config_reg | (addr & 3);
+}
+
+void pci_host_init(PCIHostState *s)
+{
+    s->get_config_reg = pci_host_get_config_reg;
+}
diff --git a/hw/pci_host.h b/hw/pci_host.h
index a006687..90a4c63 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -30,14 +30,19 @@
 
 #include "sysbus.h"
 
+/* for config space access */
+typedef uint32_t (*pci_config_reg_fn)(PCIHostState *s, uint32_t addr);
+
 struct PCIHostState {
     SysBusDevice busdev;
+    pci_config_reg_fn get_config_reg;
     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);
 
 /* 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..55aa85e 100644
--- a/hw/pci_host_template.h
+++ b/hw/pci_host_template.h
@@ -29,48 +29,52 @@ static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)(
     void* opaque, PCI_ADDR_T addr, uint32_t val)
 {
     PCIHostState *s = opaque;
+    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
 
     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);
+    if (config_reg & (1u << 31))
+        pci_data_write(s->bus, config_reg, val, 1);
 }
 
 static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
     void* opaque, PCI_ADDR_T addr, uint32_t val)
 {
     PCIHostState *s = opaque;
+    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
 #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);
+    if (config_reg & (1u << 31))
+        pci_data_write(s->bus, config_reg, val, 2);
 }
 
 static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
     void* opaque, PCI_ADDR_T addr, uint32_t val)
 {
     PCIHostState *s = opaque;
+    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
 #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);
+    if (config_reg & (1u << 31))
+        pci_data_write(s->bus, config_reg, val, 4);
 }
 
 static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
     void* opaque, PCI_ADDR_T addr)
 {
     PCIHostState *s = opaque;
+    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
     uint32_t val;
 
-    if (!(s->config_reg & (1 << 31)))
+    if (!(config_reg & (1 << 31)))
         return 0xff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
+    val = pci_data_read(s->bus, config_reg, 1);
     PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
                 (target_phys_addr_t)addr, val);
     return val;
@@ -80,10 +84,11 @@ static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)(
     void* opaque, PCI_ADDR_T addr)
 {
     PCIHostState *s = opaque;
+    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
     uint32_t val;
-    if (!(s->config_reg & (1 << 31)))
+    if (!(config_reg & (1 << 31)))
         return 0xffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
+    val = pci_data_read(s->bus, config_reg, 2);
     PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
                 (target_phys_addr_t)addr, val);
 #ifdef TARGET_WORDS_BIGENDIAN
@@ -96,10 +101,11 @@ static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)(
     void* opaque, PCI_ADDR_T addr)
 {
     PCIHostState *s = opaque;
+    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
     uint32_t val;
-    if (!(s->config_reg & (1 << 31)))
+    if (!(config_reg))
         return 0xffffffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
+    val = pci_data_read(s->bus, config_reg, 4);
     PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
                 (target_phys_addr_t)addr, val);
 #ifdef TARGET_WORDS_BIGENDIAN
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

[Qemu-devel] [PATCH 2/6] Add config space conversion function for uni_north

[Alexander Graf]

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>
---
 hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index fdb9401..1c49008 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
 {
 }
 
+static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
+{
+    uint32_t retval;
+    uint32_t reg = s->config_reg;
+
+    if (reg & (1u << 31)) {
+        /* XXX OpenBIOS compatibility hack */
+        retval = reg;
+        addr |= reg & 7;
+    } else if (reg & 1) {
+        /* Set upper valid bit and remove lower one */
+        retval = (reg & ~3u) | (1u << 31);
+    } else {
+        uint32_t slot, func;
+        uint32_t devfn;
+
+        /* Grab CFA0 style values */
+        slot = ffs(reg & 0xfffff800) - 1;
+        func = (reg >> 8) & 7;
+        devfn = PCI_DEVFN(slot, func);
+
+        /* ... and then convert them to x86 format */
+        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
+    }
+
+    retval &= ~3u;
+    retval |= (addr & 7);
+
+    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
+                 reg, addr, retval);
+
+    return retval;
+}
+
 static int pci_unin_main_init_device(SysBusDevice *dev)
 {
     UNINState *s;
@@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
     s = FROM_SYSBUS(UNINState, dev);
 
     pci_host_init(&s->host_state);
+    s->host_state.get_config_reg = unin_get_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

[Qemu-devel] [PATCH 3/6] Use Mac99_U3 type on ppc64

[Alexander Graf]

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>
---
 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 1c49008..829c4cb 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -146,6 +146,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.get_config_reg = unin_get_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;
@@ -227,6 +248,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);
@@ -282,6 +328,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);
@@ -307,6 +368,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),
@@ -327,6 +394,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

[Qemu-devel] [PATCH 4/6] Include dump of lspci -nn on real G5

[Alexander Graf]

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 829c4cb..8060749 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -248,6 +248,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

[Qemu-devel] [PATCH 5/6] Make interrupts work

[Alexander Graf]

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 8060749..a334adb 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

[Qemu-devel] [PATCH 6/6] Enable secondary cmd64x

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> 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>
> ---
>  hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index fdb9401..1c49008 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>  {
>  }
>  
> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> +{
> +    uint32_t retval;
> +    uint32_t reg = s->config_reg;
> +
> +    if (reg & (1u << 31)) {
> +        /* XXX OpenBIOS compatibility hack */
> +        retval = reg;
> +        addr |= reg & 7;
> +    } else if (reg & 1) {
> +        /* Set upper valid bit and remove lower one */
> +        retval = (reg & ~3u) | (1u << 31);
> +    } else {
> +        uint32_t slot, func;
> +        uint32_t devfn;
> +
> +        /* Grab CFA0 style values */
> +        slot = ffs(reg & 0xfffff800) - 1;
> +        func = (reg >> 8) & 7;
> +        devfn = PCI_DEVFN(slot, func);
> +
> +        /* ... and then convert them to x86 format */
> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);

Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
number?  This way this encoding can be changed down the road.

> +    }
> +
> +    retval &= ~3u;
> +    retval |= (addr & 7);
> +
> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
> +                 reg, addr, retval);
> +
> +    return retval;
> +}
> +
>  static int pci_unin_main_init_device(SysBusDevice *dev)
>  {
>      UNINState *s;
> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>      s = FROM_SYSBUS(UNINState, dev);
>  
>      pci_host_init(&s->host_state);
> +    s->host_state.get_config_reg = unin_get_config_reg;

This looks slightly ugly: let's make pci_host_init accept
the conversion function instead?

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

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Alexander Graf]

On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
>> 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>
>> ---
>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>> 1 files changed, 35 insertions(+), 0 deletions(-)
>> 
>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
>> index fdb9401..1c49008 100644
>> --- a/hw/unin_pci.c
>> +++ b/hw/unin_pci.c
>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>> {
>> }
>> 
>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
>> +{
>> +    uint32_t retval;
>> +    uint32_t reg = s->config_reg;
>> +
>> +    if (reg & (1u << 31)) {
>> +        /* XXX OpenBIOS compatibility hack */
>> +        retval = reg;
>> +        addr |= reg & 7;
>> +    } else if (reg & 1) {
>> +        /* Set upper valid bit and remove lower one */
>> +        retval = (reg & ~3u) | (1u << 31);
>> +    } else {
>> +        uint32_t slot, func;
>> +        uint32_t devfn;
>> +
>> +        /* Grab CFA0 style values */
>> +        slot = ffs(reg & 0xfffff800) - 1;
>> +        func = (reg >> 8) & 7;
>> +        devfn = PCI_DEVFN(slot, func);
>> +
>> +        /* ... and then convert them to x86 format */
>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> 
> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> number?  This way this encoding can be changed down the road.

I don't think I understand this comment? :-)

> 
>> +    }
>> +
>> +    retval &= ~3u;
>> +    retval |= (addr & 7);
>> +
>> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
>> +                 reg, addr, retval);
>> +
>> +    return retval;
>> +}
>> +
>> static int pci_unin_main_init_device(SysBusDevice *dev)
>> {
>>     UNINState *s;
>> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>>     s = FROM_SYSBUS(UNINState, dev);
>> 
>>     pci_host_init(&s->host_state);
>> +    s->host_state.get_config_reg = unin_get_config_reg;
> 
> This looks slightly ugly: let's make pci_host_init accept
> the conversion function instead?

Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones.


Alex

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
> makes a qemu understandable one out of it.
> 
> 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>

Thanks!

It always bothered me that the code in pci_host uses x86 encoding and
other architectures are converted to x86.  As long as we are adding
redirection, maybe we should get rid of this, and make get_config_reg
return register and devfn separately, so we don't need to encode/decode
back and forth?

OTOH if we are after a quick fix, won't it be simpler to have
unin_pci simply use its own routines instead of pci_host_conf_register_mmio
etc?

> ---
>  hw/apb_pci.c           |    1 +
>  hw/grackle_pci.c       |    1 +
>  hw/gt64xxx.c           |    1 +
>  hw/pci_host.c          |   11 +++++++++++
>  hw/pci_host.h          |    5 +++++
>  hw/pci_host_template.h |   30 ++++++++++++++++++------------
>  hw/piix_pci.c          |    1 +
>  hw/ppc4xx_pci.c        |    1 +
>  hw/ppce500_pci.c       |    1 +
>  hw/unin_pci.c          |    1 +
>  10 files changed, 41 insertions(+), 12 deletions(-)
> 
> 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_host.c b/hw/pci_host.c
> index eeb8dee..2d12887 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -228,3 +228,14 @@ 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 the config register */
> +static uint32_t pci_host_get_config_reg(PCIHostState *s, uint32_t addr)
> +{
> +    return s->config_reg | (addr & 3);
> +}
> +
> +void pci_host_init(PCIHostState *s)
> +{
> +    s->get_config_reg = pci_host_get_config_reg;
> +}

So config_reg is always set but only used for PC now?
This is not very pretty ...

> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index a006687..90a4c63 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -30,14 +30,19 @@
>  
>  #include "sysbus.h"
>  
> +/* for config space access */
> +typedef uint32_t (*pci_config_reg_fn)(PCIHostState *s, uint32_t addr);
> +
>  struct PCIHostState {
>      SysBusDevice busdev;
> +    pci_config_reg_fn get_config_reg;
>      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);
>  
>  /* 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..55aa85e 100644
> --- a/hw/pci_host_template.h
> +++ b/hw/pci_host_template.h
> @@ -29,48 +29,52 @@ static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
> +    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
>  
>      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);
> +    if (config_reg & (1u << 31))
> +        pci_data_write(s->bus, config_reg, val, 1);
>  }
>  
>  static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
> +    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
>  #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);
> +    if (config_reg & (1u << 31))
> +        pci_data_write(s->bus, config_reg, val, 2);
>  }
>  
>  static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
> +    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
>  #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);
> +    if (config_reg & (1u << 31))
> +        pci_data_write(s->bus, config_reg, val, 4);
>  }
>  
>  static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr)
>  {
>      PCIHostState *s = opaque;
> +    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
>      uint32_t val;
>  
> -    if (!(s->config_reg & (1 << 31)))
> +    if (!(config_reg & (1 << 31)))
>          return 0xff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
> +    val = pci_data_read(s->bus, config_reg, 1);
>      PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
>                  (target_phys_addr_t)addr, val);
>      return val;
> @@ -80,10 +84,11 @@ static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr)
>  {
>      PCIHostState *s = opaque;
> +    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
>      uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> +    if (!(config_reg & (1 << 31)))
>          return 0xffff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
> +    val = pci_data_read(s->bus, config_reg, 2);
>      PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
>                  (target_phys_addr_t)addr, val);
>  #ifdef TARGET_WORDS_BIGENDIAN
> @@ -96,10 +101,11 @@ static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr)
>  {
>      PCIHostState *s = opaque;
> +    uint32_t config_reg = s->get_config_reg(s, (uint32_t)addr);
>      uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> +    if (!(config_reg))
>          return 0xffffffff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
> +    val = pci_data_read(s->bus, config_reg, 4);
>      PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
>                  (target_phys_addr_t)addr, val);
>  #ifdef TARGET_WORDS_BIGENDIAN
> 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
> 
> 

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >> 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>
> >> ---
> >> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >> 1 files changed, 35 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >> index fdb9401..1c49008 100644
> >> --- a/hw/unin_pci.c
> >> +++ b/hw/unin_pci.c
> >> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >> {
> >> }
> >> 
> >> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >> +{
> >> +    uint32_t retval;
> >> +    uint32_t reg = s->config_reg;
> >> +
> >> +    if (reg & (1u << 31)) {
> >> +        /* XXX OpenBIOS compatibility hack */
> >> +        retval = reg;
> >> +        addr |= reg & 7;
> >> +    } else if (reg & 1) {
> >> +        /* Set upper valid bit and remove lower one */
> >> +        retval = (reg & ~3u) | (1u << 31);
> >> +    } else {
> >> +        uint32_t slot, func;
> >> +        uint32_t devfn;
> >> +
> >> +        /* Grab CFA0 style values */
> >> +        slot = ffs(reg & 0xfffff800) - 1;
> >> +        func = (reg >> 8) & 7;
> >> +        devfn = PCI_DEVFN(slot, func);
> >> +
> >> +        /* ... and then convert them to x86 format */
> >> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> > 
> > Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> > number?  This way this encoding can be changed down the road.
> 
> I don't think I understand this comment? :-)

This puts reg+dev+fn in a format that pci_host can the understand
correct? So it would make sense to have an inline function
in pci host that gets 3 parameters and does the encoding.

> > 
> >> +    }
> >> +
> >> +    retval &= ~3u;
> >> +    retval |= (addr & 7);
> >> +
> >> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
> >> +                 reg, addr, retval);
> >> +
> >> +    return retval;
> >> +}
> >> +
> >> static int pci_unin_main_init_device(SysBusDevice *dev)
> >> {
> >>     UNINState *s;
> >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
> >>     s = FROM_SYSBUS(UNINState, dev);
> >> 
> >>     pci_host_init(&s->host_state);
> >> +    s->host_state.get_config_reg = unin_get_config_reg;
> > 
> > This looks slightly ugly: let's make pci_host_init accept
> > the conversion function instead?
> 
> Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones.
> 
> 
> Alex

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >> 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>
> >> ---
> >> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >> 1 files changed, 35 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >> index fdb9401..1c49008 100644
> >> --- a/hw/unin_pci.c
> >> +++ b/hw/unin_pci.c
> >> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >> {
> >> }
> >> 
> >> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >> +{
> >> +    uint32_t retval;
> >> +    uint32_t reg = s->config_reg;
> >> +
> >> +    if (reg & (1u << 31)) {
> >> +        /* XXX OpenBIOS compatibility hack */
> >> +        retval = reg;
> >> +        addr |= reg & 7;
> >> +    } else if (reg & 1) {
> >> +        /* Set upper valid bit and remove lower one */
> >> +        retval = (reg & ~3u) | (1u << 31);
> >> +    } else {
> >> +        uint32_t slot, func;
> >> +        uint32_t devfn;
> >> +
> >> +        /* Grab CFA0 style values */
> >> +        slot = ffs(reg & 0xfffff800) - 1;
> >> +        func = (reg >> 8) & 7;
> >> +        devfn = PCI_DEVFN(slot, func);
> >> +
> >> +        /* ... and then convert them to x86 format */
> >> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> > 
> > Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> > number?  This way this encoding can be changed down the road.
> 
> I don't think I understand this comment? :-)
> 
> > 
> >> +    }
> >> +
> >> +    retval &= ~3u;
> >> +    retval |= (addr & 7);
> >> +
> >> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
> >> +                 reg, addr, retval);
> >> +
> >> +    return retval;
> >> +}
> >> +
> >> static int pci_unin_main_init_device(SysBusDevice *dev)
> >> {
> >>     UNINState *s;
> >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
> >>     s = FROM_SYSBUS(UNINState, dev);
> >> 
> >>     pci_host_init(&s->host_state);
> >> +    s->host_state.get_config_reg = unin_get_config_reg;
> > 
> > This looks slightly ugly: let's make pci_host_init accept
> > the conversion function instead?
> 
> Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones.
> 

OK.

> Alex

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>> makes a qemu understandable one out of it.
>> 
>> 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>
> 
> Thanks!
> 
> It always bothered me that the code in pci_host uses x86 encoding and
> other architectures are converted to x86.  As long as we are adding
> redirection, maybe we should get rid of this, and make get_config_reg
> return register and devfn separately, so we don't need to encode/decode
> back and forth?

Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.

I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.

> OTOH if we are after a quick fix, won't it be simpler to have
> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
> etc?

Hm, I figured this is less work :-).

> 
>> ---
>> hw/apb_pci.c           |    1 +
>> hw/grackle_pci.c       |    1 +
>> hw/gt64xxx.c           |    1 +
>> hw/pci_host.c          |   11 +++++++++++
>> hw/pci_host.h          |    5 +++++
>> hw/pci_host_template.h |   30 ++++++++++++++++++------------
>> hw/piix_pci.c          |    1 +
>> hw/ppc4xx_pci.c        |    1 +
>> hw/ppce500_pci.c       |    1 +
>> hw/unin_pci.c          |    1 +
>> 10 files changed, 41 insertions(+), 12 deletions(-)
>> 
>> 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_host.c b/hw/pci_host.c
>> index eeb8dee..2d12887 100644
>> --- a/hw/pci_host.c
>> +++ b/hw/pci_host.c
>> @@ -228,3 +228,14 @@ 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 the config register */
>> +static uint32_t pci_host_get_config_reg(PCIHostState *s, uint32_t addr)
>> +{
>> +    return s->config_reg | (addr & 3);
>> +}
>> +
>> +void pci_host_init(PCIHostState *s)
>> +{
>> +    s->get_config_reg = pci_host_get_config_reg;
>> +}
> 
> So config_reg is always set but only used for PC now?
> This is not very pretty ...

config_reg is used by the template.h helpers. So everyone should use it. get_config_reg is always set. It's set to pci_host_get_config_reg as default and can be overridden by a host bus if it knows it's using a different layout.


Alex

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Alexander Graf]

On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
>>>> 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>
>>>> ---
>>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>>>> 1 files changed, 35 insertions(+), 0 deletions(-)
>>>> 
>>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
>>>> index fdb9401..1c49008 100644
>>>> --- a/hw/unin_pci.c
>>>> +++ b/hw/unin_pci.c
>>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>>>> {
>>>> }
>>>> 
>>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
>>>> +{
>>>> +    uint32_t retval;
>>>> +    uint32_t reg = s->config_reg;
>>>> +
>>>> +    if (reg & (1u << 31)) {
>>>> +        /* XXX OpenBIOS compatibility hack */
>>>> +        retval = reg;
>>>> +        addr |= reg & 7;
>>>> +    } else if (reg & 1) {
>>>> +        /* Set upper valid bit and remove lower one */
>>>> +        retval = (reg & ~3u) | (1u << 31);
>>>> +    } else {
>>>> +        uint32_t slot, func;
>>>> +        uint32_t devfn;
>>>> +
>>>> +        /* Grab CFA0 style values */
>>>> +        slot = ffs(reg & 0xfffff800) - 1;
>>>> +        func = (reg >> 8) & 7;
>>>> +        devfn = PCI_DEVFN(slot, func);
>>>> +
>>>> +        /* ... and then convert them to x86 format */
>>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
>>> 
>>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
>>> number?  This way this encoding can be changed down the road.
>> 
>> I don't think I understand this comment? :-)
> 
> This puts reg+dev+fn in a format that pci_host can the understand
> correct? So it would make sense to have an inline function
> in pci host that gets 3 parameters and does the encoding.

We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it. We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.

I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).

Alex

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> >> 
> >> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> >> 
> >>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >>>> 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>
> >>>> ---
> >>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >>>> 1 files changed, 35 insertions(+), 0 deletions(-)
> >>>> 
> >>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >>>> index fdb9401..1c49008 100644
> >>>> --- a/hw/unin_pci.c
> >>>> +++ b/hw/unin_pci.c
> >>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >>>> {
> >>>> }
> >>>> 
> >>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >>>> +{
> >>>> +    uint32_t retval;
> >>>> +    uint32_t reg = s->config_reg;
> >>>> +
> >>>> +    if (reg & (1u << 31)) {
> >>>> +        /* XXX OpenBIOS compatibility hack */
> >>>> +        retval = reg;
> >>>> +        addr |= reg & 7;
> >>>> +    } else if (reg & 1) {
> >>>> +        /* Set upper valid bit and remove lower one */
> >>>> +        retval = (reg & ~3u) | (1u << 31);
> >>>> +    } else {
> >>>> +        uint32_t slot, func;
> >>>> +        uint32_t devfn;
> >>>> +
> >>>> +        /* Grab CFA0 style values */
> >>>> +        slot = ffs(reg & 0xfffff800) - 1;
> >>>> +        func = (reg >> 8) & 7;
> >>>> +        devfn = PCI_DEVFN(slot, func);
> >>>> +
> >>>> +        /* ... and then convert them to x86 format */
> >>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> >>> 
> >>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> >>> number?  This way this encoding can be changed down the road.
> >> 
> >> I don't think I understand this comment? :-)
> > 
> > This puts reg+dev+fn in a format that pci_host can the understand
> > correct? So it would make sense to have an inline function
> > in pci host that gets 3 parameters and does the encoding.
> 
> We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it.
> 
> We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.
> 
> I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).
> 
> Alex

So what the comment above suggests, is adding helper routine
that gets register device, function and creates 32 bit value
in "default format".


-- 
MST

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Alexander Graf]

On 03.01.2010, at 17:20, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
>>>> 
>>>>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
>>>>>> 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>
>>>>>> ---
>>>>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>>>>>> 1 files changed, 35 insertions(+), 0 deletions(-)
>>>>>> 
>>>>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
>>>>>> index fdb9401..1c49008 100644
>>>>>> --- a/hw/unin_pci.c
>>>>>> +++ b/hw/unin_pci.c
>>>>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>>>>>> {
>>>>>> }
>>>>>> 
>>>>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
>>>>>> +{
>>>>>> +    uint32_t retval;
>>>>>> +    uint32_t reg = s->config_reg;
>>>>>> +
>>>>>> +    if (reg & (1u << 31)) {
>>>>>> +        /* XXX OpenBIOS compatibility hack */
>>>>>> +        retval = reg;
>>>>>> +        addr |= reg & 7;
>>>>>> +    } else if (reg & 1) {
>>>>>> +        /* Set upper valid bit and remove lower one */
>>>>>> +        retval = (reg & ~3u) | (1u << 31);
>>>>>> +    } else {
>>>>>> +        uint32_t slot, func;
>>>>>> +        uint32_t devfn;
>>>>>> +
>>>>>> +        /* Grab CFA0 style values */
>>>>>> +        slot = ffs(reg & 0xfffff800) - 1;
>>>>>> +        func = (reg >> 8) & 7;
>>>>>> +        devfn = PCI_DEVFN(slot, func);
>>>>>> +
>>>>>> +        /* ... and then convert them to x86 format */
>>>>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
>>>>> 
>>>>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
>>>>> number?  This way this encoding can be changed down the road.
>>>> 
>>>> I don't think I understand this comment? :-)
>>> 
>>> This puts reg+dev+fn in a format that pci_host can the understand
>>> correct? So it would make sense to have an inline function
>>> in pci host that gets 3 parameters and does the encoding.
>> 
>> We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it.
>> 
>> We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.
>> 
>> I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).
>> 
>> Alex
> 
> So what the comment above suggests, is adding helper routine
> that gets register device, function and creates 32 bit value
> in "default format".

Oh, so you mean that instead of returning a uint32_t that magically gets converted inside the conversion function, we'd create another function like this:

uint32_t pci_host_config_address(int bus, int dev, int fn)
{
    return (1u << 31) | (bus << 11) | (dev << 3) | fn;
}

which then would be called at the end of the conversion function?


Alex

[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 05:35:01PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 17:20, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote:
> >> 
> >> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:
> >> 
> >>> On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> >>>> 
> >>>> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> >>>> 
> >>>>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >>>>>> 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>
> >>>>>> ---
> >>>>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >>>>>> 1 files changed, 35 insertions(+), 0 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >>>>>> index fdb9401..1c49008 100644
> >>>>>> --- a/hw/unin_pci.c
> >>>>>> +++ b/hw/unin_pci.c
> >>>>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >>>>>> {
> >>>>>> }
> >>>>>> 
> >>>>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >>>>>> +{
> >>>>>> +    uint32_t retval;
> >>>>>> +    uint32_t reg = s->config_reg;
> >>>>>> +
> >>>>>> +    if (reg & (1u << 31)) {
> >>>>>> +        /* XXX OpenBIOS compatibility hack */
> >>>>>> +        retval = reg;
> >>>>>> +        addr |= reg & 7;
> >>>>>> +    } else if (reg & 1) {
> >>>>>> +        /* Set upper valid bit and remove lower one */
> >>>>>> +        retval = (reg & ~3u) | (1u << 31);
> >>>>>> +    } else {
> >>>>>> +        uint32_t slot, func;
> >>>>>> +        uint32_t devfn;
> >>>>>> +
> >>>>>> +        /* Grab CFA0 style values */
> >>>>>> +        slot = ffs(reg & 0xfffff800) - 1;
> >>>>>> +        func = (reg >> 8) & 7;
> >>>>>> +        devfn = PCI_DEVFN(slot, func);
> >>>>>> +
> >>>>>> +        /* ... and then convert them to x86 format */
> >>>>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> >>>>> 
> >>>>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> >>>>> number?  This way this encoding can be changed down the road.
> >>>> 
> >>>> I don't think I understand this comment? :-)
> >>> 
> >>> This puts reg+dev+fn in a format that pci_host can the understand
> >>> correct? So it would make sense to have an inline function
> >>> in pci host that gets 3 parameters and does the encoding.
> >> 
> >> We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it.
> >> 
> >> We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.
> >> 
> >> I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).
> >> 
> >> Alex
> > 
> > So what the comment above suggests, is adding helper routine
> > that gets register device, function and creates 32 bit value
> > in "default format".
> 
> Oh, so you mean that instead of returning a uint32_t that magically gets converted inside the conversion function, we'd create another function like this:
> 
> uint32_t pci_host_config_address(int bus, int dev, int fn)
> {
>     return (1u << 31) | (bus << 11) | (dev << 3) | fn;
> }
> 
> which then would be called at the end of the conversion function?
> 
> 
> Alex

Yes.

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
> >> makes a qemu understandable one out of it.
> >> 
> >> 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>
> > 
> > Thanks!
> > 
> > It always bothered me that the code in pci_host uses x86 encoding and
> > other architectures are converted to x86.  As long as we are adding
> > redirection, maybe we should get rid of this, and make get_config_reg
> > return register and devfn separately, so we don't need to encode/decode
> > back and forth?
> 
> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
> 
> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
> 
> > OTOH if we are after a quick fix, won't it be simpler to have
> > unin_pci simply use its own routines instead of pci_host_conf_register_mmio
> > etc?
> 
> Hm, I figured this is less work :-).

Let me explain the idea: we have:

	static void pci_host_config_writel_ioport(void *opaque,
						  uint32_t addr, uint32_t val)
	{
	    PCIHostState *s = opaque;

	    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
	val);
	    s->config_reg = val;
	}

	static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
	addr)
	{
	    PCIHostState *s = opaque;
	    uint32_t val = s->config_reg;

	    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
	val);
	    return val;
	}

	void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
	{
	    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
	s);
	    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
	}

If instead of a simple config_reg = val we translate to pc format
here, the rest will work. No?


> > 
> >> ---
> >> hw/apb_pci.c           |    1 +
> >> hw/grackle_pci.c       |    1 +
> >> hw/gt64xxx.c           |    1 +
> >> hw/pci_host.c          |   11 +++++++++++
> >> hw/pci_host.h          |    5 +++++
> >> hw/pci_host_template.h |   30 ++++++++++++++++++------------
> >> hw/piix_pci.c          |    1 +
> >> hw/ppc4xx_pci.c        |    1 +
> >> hw/ppce500_pci.c       |    1 +
> >> hw/unin_pci.c          |    1 +
> >> 10 files changed, 41 insertions(+), 12 deletions(-)
> >> 
> >> 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_host.c b/hw/pci_host.c
> >> index eeb8dee..2d12887 100644
> >> --- a/hw/pci_host.c
> >> +++ b/hw/pci_host.c
> >> @@ -228,3 +228,14 @@ 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 the config register */
> >> +static uint32_t pci_host_get_config_reg(PCIHostState *s, uint32_t addr)
> >> +{
> >> +    return s->config_reg | (addr & 3);
> >> +}
> >> +
> >> +void pci_host_init(PCIHostState *s)
> >> +{
> >> +    s->get_config_reg = pci_host_get_config_reg;
> >> +}
> > 
> > So config_reg is always set but only used for PC now?
> > This is not very pretty ...
> 
> config_reg is used by the template.h helpers. So everyone should use it. get_config_reg is always set. It's set to pci_host_get_config_reg as default and can be overridden by a host bus if it knows it's using a different layout.
> 
> 
> Alex

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>>> makes a qemu understandable one out of it.
>>>> 
>>>> 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>
>>> 
>>> Thanks!
>>> 
>>> It always bothered me that the code in pci_host uses x86 encoding and
>>> other architectures are converted to x86.  As long as we are adding
>>> redirection, maybe we should get rid of this, and make get_config_reg
>>> return register and devfn separately, so we don't need to encode/decode
>>> back and forth?
>> 
>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>> 
>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>> 
>>> OTOH if we are after a quick fix, won't it be simpler to have
>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>> etc?
>> 
>> Hm, I figured this is less work :-).
> 
> Let me explain the idea: we have:
> 
> 	static void pci_host_config_writel_ioport(void *opaque,
> 						  uint32_t addr, uint32_t val)
> 	{
> 	    PCIHostState *s = opaque;
> 
> 	    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
> 	val);
> 	    s->config_reg = val;
> 	}
> 
> 	static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
> 	addr)
> 	{
> 	    PCIHostState *s = opaque;
> 	    uint32_t val = s->config_reg;
> 
> 	    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
> 	val);
> 	    return val;
> 	}
> 
> 	void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
> 	{
> 	    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
> 	s);
> 	    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
> 	}
> 
> If instead of a simple config_reg = val we translate to pc format
> here, the rest will work. No?

Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...

Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.

Alex

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
> >> 
> >> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
> >> 
> >>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
> >>>> makes a qemu understandable one out of it.
> >>>> 
> >>>> 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>
> >>> 
> >>> Thanks!
> >>> 
> >>> It always bothered me that the code in pci_host uses x86 encoding and
> >>> other architectures are converted to x86.  As long as we are adding
> >>> redirection, maybe we should get rid of this, and make get_config_reg
> >>> return register and devfn separately, so we don't need to encode/decode
> >>> back and forth?
> >> 
> >> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
> >> 
> >> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
> >> 
> >>> OTOH if we are after a quick fix, won't it be simpler to have
> >>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
> >>> etc?
> >> 
> >> Hm, I figured this is less work :-).
> > 
> > Let me explain the idea: we have:
> > 
> > 	static void pci_host_config_writel_ioport(void *opaque,
> > 						  uint32_t addr, uint32_t val)
> > 	{
> > 	    PCIHostState *s = opaque;
> > 
> > 	    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
> > 	val);
> > 	    s->config_reg = val;
> > 	}
> > 
> > 	static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
> > 	addr)
> > 	{
> > 	    PCIHostState *s = opaque;
> > 	    uint32_t val = s->config_reg;
> > 
> > 	    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
> > 	val);
> > 	    return val;
> > 	}
> > 
> > 	void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
> > 	{
> > 	    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
> > 	s);
> > 	    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
> > 	}
> > 
> > If instead of a simple config_reg = val we translate to pc format
> > here, the rest will work. No?
> 
> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...

Right.

> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
> 
> Alex

There's already ugliness with swap/noswap versions in there.  Anything
that claims to be a proper framework would need to address that as well
IMO.

-- 
MST

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>> 
>>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>>>>> makes a qemu understandable one out of it.
>>>>>> 
>>>>>> 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>
>>>>> 
>>>>> Thanks!
>>>>> 
>>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>>>> other architectures are converted to x86.  As long as we are adding
>>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>>>> return register and devfn separately, so we don't need to encode/decode
>>>>> back and forth?
>>>> 
>>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>>>> 
>>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>>>> 
>>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>>>> etc?
>>>> 
>>>> Hm, I figured this is less work :-).
>>> 
>>> Let me explain the idea: we have:
>>> 
>>> 	static void pci_host_config_writel_ioport(void *opaque,
>>> 						  uint32_t addr, uint32_t val)
>>> 	{
>>> 	    PCIHostState *s = opaque;
>>> 
>>> 	    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>> 	val);
>>> 	    s->config_reg = val;
>>> 	}
>>> 
>>> 	static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>> 	addr)
>>> 	{
>>> 	    PCIHostState *s = opaque;
>>> 	    uint32_t val = s->config_reg;
>>> 
>>> 	    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>> 	val);
>>> 	    return val;
>>> 	}
>>> 
>>> 	void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>> 	{
>>> 	    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>> 	s);
>>> 	    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>> 	}
>>> 
>>> If instead of a simple config_reg = val we translate to pc format
>>> here, the rest will work. No?
>> 
>> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
> 
> Right.
> 
>> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>> 
>> Alex
> 
> There's already ugliness with swap/noswap versions in there.  Anything
> that claims to be a proper framework would need to address that as well
> IMO.

Hm, so you'd rather wait for 5 years to have an all-in-one rework than incrementally improving the existing code? I can do the hacky version then :-).


Alex

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
> >> 
> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
> >> 
> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
> >>>> 
> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
> >>>> 
> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
> >>>>>> makes a qemu understandable one out of it.
> >>>>>> 
> >>>>>> 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>
> >>>>> 
> >>>>> Thanks!
> >>>>> 
> >>>>> It always bothered me that the code in pci_host uses x86 encoding and
> >>>>> other architectures are converted to x86.  As long as we are adding
> >>>>> redirection, maybe we should get rid of this, and make get_config_reg
> >>>>> return register and devfn separately, so we don't need to encode/decode
> >>>>> back and forth?
> >>>> 
> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
> >>>> 
> >>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
> >>>> 
> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
> >>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
> >>>>> etc?
> >>>> 
> >>>> Hm, I figured this is less work :-).
> >>> 
> >>> Let me explain the idea: we have:
> >>> 
> >>> 	static void pci_host_config_writel_ioport(void *opaque,
> >>> 						  uint32_t addr, uint32_t val)
> >>> 	{
> >>> 	    PCIHostState *s = opaque;
> >>> 
> >>> 	    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
> >>> 	val);
> >>> 	    s->config_reg = val;
> >>> 	}
> >>> 
> >>> 	static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
> >>> 	addr)
> >>> 	{
> >>> 	    PCIHostState *s = opaque;
> >>> 	    uint32_t val = s->config_reg;
> >>> 
> >>> 	    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
> >>> 	val);
> >>> 	    return val;
> >>> 	}
> >>> 
> >>> 	void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
> >>> 	{
> >>> 	    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
> >>> 	s);
> >>> 	    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
> >>> 	}
> >>> 
> >>> If instead of a simple config_reg = val we translate to pc format
> >>> here, the rest will work. No?
> >> 
> >> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
> > 
> > Right.
> > 
> >> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
> >> 
> >> Alex
> > 
> > There's already ugliness with swap/noswap versions in there.  Anything
> > that claims to be a proper framework would need to address that as well
> > IMO.
> 
> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
> incrementally improving the existing code?

Not really, incremental improvements are good.  But what you posted does
not seem to clean up most code, what it really does is fixes ppc
unin_pci.  My feeling was since there's only one user for now maybe it
is better to just have device specific code rather than complicate
generic code. Makes sense?

We need to see several users before we add yet another level of
indirection.  If there is a level of indirection that solves the
swap/noswap ugliness as well that would show this is a good abstraction.
I think I even know what a good abstraction would be: decode address on
write and keep it in decoded form.

> I can do the hacky version then :-).
> 
> 
> Alex


I haven't seen the hacky version one so it's hard to evaluate which way
of fixing unin_pci is better. What is your opinion?

-- 
MST

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Blue Swirl]

On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>
>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>
>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>> >>
>> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>> >>
>> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>> >>>>
>> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>> >>>>
>> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>> >>>>>> makes a qemu understandable one out of it.
>> >>>>>>
>> >>>>>> 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>
>> >>>>>
>> >>>>> Thanks!
>> >>>>>
>> >>>>> It always bothered me that the code in pci_host uses x86 encoding and
>> >>>>> other architectures are converted to x86.  As long as we are adding
>> >>>>> redirection, maybe we should get rid of this, and make get_config_reg
>> >>>>> return register and devfn separately, so we don't need to encode/decode
>> >>>>> back and forth?
>> >>>>
>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>> >>>>
>> >>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>> >>>>
>> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
>> >>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>> >>>>> etc?
>> >>>>
>> >>>> Hm, I figured this is less work :-).
>> >>>
>> >>> Let me explain the idea: we have:
>> >>>
>> >>>   static void pci_host_config_writel_ioport(void *opaque,
>> >>>                                             uint32_t addr, uint32_t val)
>> >>>   {
>> >>>       PCIHostState *s = opaque;
>> >>>
>> >>>       PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>> >>>   val);
>> >>>       s->config_reg = val;
>> >>>   }
>> >>>
>> >>>   static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>> >>>   addr)
>> >>>   {
>> >>>       PCIHostState *s = opaque;
>> >>>       uint32_t val = s->config_reg;
>> >>>
>> >>>       PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>> >>>   val);
>> >>>       return val;
>> >>>   }
>> >>>
>> >>>   void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>> >>>   {
>> >>>       register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>> >>>   s);
>> >>>       register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>> >>>   }
>> >>>
>> >>> If instead of a simple config_reg = val we translate to pc format
>> >>> here, the rest will work. No?
>> >>
>> >> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
>> >
>> > Right.
>> >
>> >> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>> >>
>> >> Alex
>> >
>> > There's already ugliness with swap/noswap versions in there.  Anything
>> > that claims to be a proper framework would need to address that as well
>> > IMO.
>>
>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>> incrementally improving the existing code?
>
> Not really, incremental improvements are good.  But what you posted does
> not seem to clean up most code, what it really does is fixes ppc
> unin_pci.  My feeling was since there's only one user for now maybe it
> is better to just have device specific code rather than complicate
> generic code. Makes sense?
>
> We need to see several users before we add yet another level of
> indirection.  If there is a level of indirection that solves the
> swap/noswap ugliness as well that would show this is a good abstraction.
> I think I even know what a good abstraction would be: decode address on
> write and keep it in decoded form.

I think Sparc64 PBM configuration cycles are also a bit different.
It's described in UltraSPARC User's Manual 805-0087, p.325.

Currently both QEMU and OpenBIOS just use something common, but as
soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
that.

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 03.01.2010, at 19:06, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>>>> 
>>>>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>>>> 
>>>>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>>>> 
>>>>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>>>>>>> makes a qemu understandable one out of it.
>>>>>>>> 
>>>>>>>> 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>
>>>>>>> 
>>>>>>> Thanks!
>>>>>>> 
>>>>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>>>>>> other architectures are converted to x86.  As long as we are adding
>>>>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>>>>>> return register and devfn separately, so we don't need to encode/decode
>>>>>>> back and forth?
>>>>>> 
>>>>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>>>>>> 
>>>>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>>>>>> 
>>>>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>>>>>> etc?
>>>>>> 
>>>>>> Hm, I figured this is less work :-).
>>>>> 
>>>>> Let me explain the idea: we have:
>>>>> 
>>>>> 	static void pci_host_config_writel_ioport(void *opaque,
>>>>> 						  uint32_t addr, uint32_t val)
>>>>> 	{
>>>>> 	    PCIHostState *s = opaque;
>>>>> 
>>>>> 	    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>>>> 	val);
>>>>> 	    s->config_reg = val;
>>>>> 	}
>>>>> 
>>>>> 	static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>>>> 	addr)
>>>>> 	{
>>>>> 	    PCIHostState *s = opaque;
>>>>> 	    uint32_t val = s->config_reg;
>>>>> 
>>>>> 	    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>>>> 	val);
>>>>> 	    return val;
>>>>> 	}
>>>>> 
>>>>> 	void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>>>> 	{
>>>>> 	    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>>>> 	s);
>>>>> 	    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>>>> 	}
>>>>> 
>>>>> If instead of a simple config_reg = val we translate to pc format
>>>>> here, the rest will work. No?
>>>> 
>>>> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
>>> 
>>> Right.
>>> 
>>>> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>>>> 
>>>> Alex
>>> 
>>> There's already ugliness with swap/noswap versions in there.  Anything
>>> that claims to be a proper framework would need to address that as well
>>> IMO.
>> 
>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>> incrementally improving the existing code?
> 
> Not really, incremental improvements are good.  But what you posted does
> not seem to clean up most code, what it really does is fixes ppc
> unin_pci.  My feeling was since there's only one user for now maybe it
> is better to just have device specific code rather than complicate
> generic code. Makes sense?
> 
> We need to see several users before we add yet another level of
> indirection.  If there is a level of indirection that solves the
> swap/noswap ugliness as well that would show this is a good abstraction.
> I think I even know what a good abstraction would be: decode address on
> write and keep it in decoded form.

Sounds like a good idea.

> 
>> I can do the hacky version then :-).
>> 
>> 
>> Alex
> 
> 
> I haven't seen the hacky version one so it's hard to evaluate which way
> of fixing unin_pci is better. What is your opinion?

I think if unin_pci is the only user, it'd be better to do it hacky inside unin_pci.c. But if there's a chance there's another user, it'd be better to make it generic.

Since this is the first time I ever stumbled across type 0 and type 1 PCI config space addresses, I simply don't know if there are any. Blue Swirls comment indicated that there are. Ben also sounded as if it's rather common to not use the x86 format. On the other hand, it looks like nobody in qemu needed it so far - and we're implementing ARM, MIPS and all other sorts of platforms.

So if anyone with knowledge in PCI could shed some light here, please do so.

Alex

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Benjamin Herrenschmidt]

On Sun, 2010-01-03 at 21:27 +0100, Alexander Graf wrote:

> I think if unin_pci is the only user, it'd be better to do it hacky
> inside unin_pci.c. But if there's a chance there's another user, it'd
> be better to make it generic.
> 
> Since this is the first time I ever stumbled across type 0 and type 1
> PCI config space addresses, I simply don't know if there are any. Blue
> Swirls comment indicated that there are. Ben also sounded as if it's
> rather common to not use the x86 format. On the other hand, it looks
> like nobody in qemu needed it so far - and we're implementing ARM,
> MIPS and all other sorts of platforms.
> 
> So if anyone with knowledge in PCI could shed some light here, please
> do so.

My feeling is that what you're better off doing is to have the qemu core
take an abstract struct to identify a device config space location, that
consists of separate fields for:

 - The PCI domain (which is what host bridge it hangs off since bus
numbers are not unique between domains)

 - The bus number

 - The device number (aka slot ID)

 - The function number

Now, you can then provide a "helper" that translate the standard x86
type 0 and type 1 cf8/cfc accesses into the above for most bridges to
use, and uninorth or other non-x86 that use different scheme can do
their own decoding.

The reason you want the above is to be more future proof, ie, you'll
probably want to deal with PCIe extended function numbers for example,
or implement different methods of MMCONFIG etc... and it's better to
disconnect the low level decoding of the config space access from the
internal representation in qemu.

Cheers,
Ben.

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 03.01.2010, at 21:50, Benjamin Herrenschmidt wrote:

> On Sun, 2010-01-03 at 21:27 +0100, Alexander Graf wrote:
> 
>> I think if unin_pci is the only user, it'd be better to do it hacky
>> inside unin_pci.c. But if there's a chance there's another user, it'd
>> be better to make it generic.
>> 
>> Since this is the first time I ever stumbled across type 0 and type 1
>> PCI config space addresses, I simply don't know if there are any. Blue
>> Swirls comment indicated that there are. Ben also sounded as if it's
>> rather common to not use the x86 format. On the other hand, it looks
>> like nobody in qemu needed it so far - and we're implementing ARM,
>> MIPS and all other sorts of platforms.
>> 
>> So if anyone with knowledge in PCI could shed some light here, please
>> do so.
> 
> My feeling is that what you're better off doing is to have the qemu core
> take an abstract struct to identify a device config space location, that
> consists of separate fields for:
> 
> - The PCI domain (which is what host bridge it hangs off since bus
> numbers are not unique between domains)
> 
> - The bus number

Hm, I think it'd make more sense to just store a PCIBus pointer in there. We could then fetch the bus and domain id from there.

I'll write something up :-).


Alex

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Isaku Yamahata]

On Mon, Jan 04, 2010 at 04:26:46AM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 21:50, Benjamin Herrenschmidt wrote:
> 
> > On Sun, 2010-01-03 at 21:27 +0100, Alexander Graf wrote:
> > 
> >> I think if unin_pci is the only user, it'd be better to do it hacky
> >> inside unin_pci.c. But if there's a chance there's another user, it'd
> >> be better to make it generic.
> >> 
> >> Since this is the first time I ever stumbled across type 0 and type 1
> >> PCI config space addresses, I simply don't know if there are any. Blue
> >> Swirls comment indicated that there are. Ben also sounded as if it's
> >> rather common to not use the x86 format. On the other hand, it looks
> >> like nobody in qemu needed it so far - and we're implementing ARM,
> >> MIPS and all other sorts of platforms.
> >> 
> >> So if anyone with knowledge in PCI could shed some light here, please
> >> do so.
> > 
> > My feeling is that what you're better off doing is to have the qemu core
> > take an abstract struct to identify a device config space location, that
> > consists of separate fields for:
> > 
> > - The PCI domain (which is what host bridge it hangs off since bus
> > numbers are not unique between domains)
> > 
> > - The bus number
> 
> Hm, I think it'd make more sense to just store a PCIBus pointer in there. We could then fetch the bus and domain id from there.
> 
> I'll write something up :-).
> 
> 
> Alex
> 

Does the following patch help?
I did only compile test though.

>From 9c62b4846f95ebe84e182f76295016e1fe980699 Mon Sep 17 00:00:00 2001
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Mon, 4 Jan 2010 19:39:36 +0900
Subject: [PATCH] pci: pcihost clean up.

remove some codes by introduce callback to calculate pci device and offset
in configuration space.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/gt64xxx.c           |    9 +---
 hw/pci_host.c          |  115 ++++++++++++++++++++++++++++--------------------
 hw/pci_host.h          |   17 ++++++-
 hw/pci_host_template.h |   21 +++------
 hw/prep_pci.c          |   68 +++--------------------------
 hw/sh_pci.c            |   92 +++++++++++++++++---------------------
 hw/versatile_pci.c     |   74 +++++--------------------------
 7 files changed, 150 insertions(+), 246 deletions(-)

diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index fb7f5bd..b399f0c 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -530,8 +530,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
     case GT_PCI0_CFGDATA:
         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_data_write(s->pci, 0, val, 4);
         break;
 
     /* Interrupts */
@@ -768,10 +767,7 @@ static uint32_t gt64120_readl (void *opaque,
         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);
+        val = pci_data_read(s->pci, 0, 4);
         if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
             val = bswap32(val);
         break;
@@ -1119,6 +1115,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
 
     s = qemu_mallocz(sizeof(GT64120State));
     s->pci = qemu_mallocz(sizeof(GT64120PCIState));
+    pci_host_init(s->pci, NULL, NULL);
 
     s->pci->bus = pci_register_bus(NULL, "pci",
                                    pci_gt64120_set_irq, pci_gt64120_map_irq,
diff --git a/hw/pci_host.c b/hw/pci_host.c
index eeb8dee..6677175 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -40,6 +40,49 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
  */
 
 /* the helper functio to get a PCIDeice* for a given pci address */
+PCIDevice *pci_host_find_dev(PCIBus *bus, uint32_t 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));
+}
+
+uint32_t pci_host_config_offset(uint32_t addr)
+{
+    return addr & (PCI_CONFIG_SPACE_SIZE - 1);
+}
+
+static uint32_t pci_host_pci_addr(PCIHostState *s, uint32_t addr)
+{
+    return s->config_reg | (addr & 3);
+}
+static PCIDevice *pci_host_find_dev_active(PCIBus *bus, uint32_t pci_addr)
+{
+    if (!(pci_addr & (1u << 31)))
+        return NULL;
+
+    return pci_host_find_dev(bus, pci_addr);
+}
+
+static PCIDevice *pci_host_dev_find_fn_noswap(PCIHostState *s, uint32_t addr)
+{
+    return pci_host_find_dev_active(s->bus, pci_host_pci_addr(s, addr));
+}
+
+static PCIDevice *pci_host_dev_find_fn(PCIHostState *s, uint32_t addr)
+{
+#ifdef TARGET_WORDS_BIGENDIAN
+    addr = bswap32(addr);
+#endif
+    return pci_host_find_dev_active(s->bus, pci_host_pci_addr(s, addr));
+}
+
+static uint32_t pci_host_config_offset_fn(PCIHostState *s, uint32_t addr)
+{
+    return pci_host_config_offset(pci_host_pci_addr(s, addr));
+}
+
 static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 {
     uint8_t bus_num = addr >> 16;
@@ -48,10 +91,10 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
     return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
-void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
+void pci_data_write(PCIHostState *s, 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 = s->dev_find(s, addr);
+    uint32_t config_addr = s->config_offset(s, addr);
 
     if (!pci_dev)
         return;
@@ -61,10 +104,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(PCIHostState *s, 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 = s->dev_find(s, addr);
+    uint32_t config_addr = s->config_offset(s, addr);
     uint32_t val;
 
     assert(len == 1 || len == 2 || len == 4);
@@ -79,14 +122,24 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
     return val;
 }
 
+void pci_host_init(PCIHostState *s,
+                   pci_dev_find_fn dev_find,
+                   pci_config_offset_fn config_offset)
+{
+    if (!dev_find)
+        dev_find = pci_host_dev_find_fn;
+    if (!config_offset)
+        config_offset = pci_host_config_offset_fn;
+
+    s->dev_find = dev_find;
+    s->config_offset = config_offset;
+}
+
 static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
                                    uint32_t val)
 {
     PCIHostState *s = opaque;
 
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
     PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
                 __func__, addr, val);
     s->config_reg = val;
@@ -97,9 +150,6 @@ static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
     PCIHostState *s = opaque;
     uint32_t val = s->config_reg;
 
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
     PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
                 __func__, addr, val);
     return val;
@@ -119,48 +169,16 @@ static CPUReadMemoryFunc * const pci_host_config_read[] = {
 
 int pci_host_conf_register_mmio(PCIHostState *s)
 {
+    pci_host_init(s, NULL, NULL);
     return cpu_register_io_memory(pci_host_config_read,
                                   pci_host_config_write, s);
 }
 
-static void pci_host_config_writel_noswap(void *opaque,
-                                          target_phys_addr_t addr,
-                                          uint32_t val)
-{
-    PCIHostState *s = opaque;
-
-    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
-                __func__, addr, val);
-    s->config_reg = val;
-}
-
-static uint32_t pci_host_config_readl_noswap(void *opaque,
-                                             target_phys_addr_t addr)
-{
-    PCIHostState *s = opaque;
-    uint32_t val = s->config_reg;
-
-    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
-                __func__, addr, val);
-    return val;
-}
-
-static CPUWriteMemoryFunc * const pci_host_config_write_noswap[] = {
-    &pci_host_config_writel_noswap,
-    &pci_host_config_writel_noswap,
-    &pci_host_config_writel_noswap,
-};
-
-static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
-    &pci_host_config_readl_noswap,
-    &pci_host_config_readl_noswap,
-    &pci_host_config_readl_noswap,
-};
-
 int pci_host_conf_register_mmio_noswap(PCIHostState *s)
 {
-    return cpu_register_io_memory(pci_host_config_read_noswap,
-                                  pci_host_config_write_noswap, s);
+    pci_host_init(s, pci_host_dev_find_fn_noswap, NULL);
+    return cpu_register_io_memory(pci_host_config_read,
+                                  pci_host_config_write, s);
 }
 
 static void pci_host_config_writel_ioport(void *opaque,
@@ -183,6 +201,7 @@ static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
 
 void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
 {
+    pci_host_init(s, NULL, NULL);
     register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport, s);
     register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
 }
diff --git a/hw/pci_host.h b/hw/pci_host.h
index a006687..47c339a 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -30,14 +30,27 @@
 
 #include "sysbus.h"
 
+typedef PCIDevice *(*pci_dev_find_fn)(PCIHostState *s, uint32_t addr);
+typedef uint32_t (*pci_config_offset_fn)(PCIHostState *s, uint32_t addr);
+
 struct PCIHostState {
     SysBusDevice busdev;
     uint32_t config_reg;
     PCIBus *bus;
+
+    pci_dev_find_fn dev_find;
+    pci_config_offset_fn config_offset;
 };
 
-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,
+                   pci_dev_find_fn dev_find,
+                   pci_config_offset_fn config_offset);
+
+PCIDevice *pci_host_find_dev(PCIBus *bus, uint32_t addr);
+uint32_t pci_host_config_offset(uint32_t addr);
+
+void pci_data_write(PCIHostState *s, uint32_t addr, uint32_t val, int len);
+uint32_t pci_data_read(PCIHostState *s, 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 11e6c88..24a03e0 100644
--- a/hw/pci_host_template.h
+++ b/hw/pci_host_template.h
@@ -32,8 +32,7 @@ static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)(
 
     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);
+    pci_data_write(s, addr, val, 1);
 }
 
 static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
@@ -45,8 +44,7 @@ static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
 #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);
+    pci_data_write(s, addr, val, 2);
 }
 
 static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
@@ -58,8 +56,7 @@ static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
 #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);
+    pci_data_write(s, addr, val, 4);
 }
 
 static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
@@ -68,9 +65,7 @@ static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
     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);
+    val = pci_data_read(s, addr, 1);
     PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
                 (target_phys_addr_t)addr, val);
     return val;
@@ -81,9 +76,7 @@ static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)(
 {
     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);
+    val = pci_data_read(s, addr, 2);
     PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
                 (target_phys_addr_t)addr, val);
 #ifdef TARGET_WORDS_BIGENDIAN
@@ -97,9 +90,7 @@ static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)(
 {
     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);
+    val = pci_data_read(s, addr, 4);
     PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
                 (target_phys_addr_t)addr, val);
 #ifdef TARGET_WORDS_BIGENDIAN
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 19f028c..8640b2e 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -40,72 +40,16 @@ static inline uint32_t PPC_PCIIO_config(target_phys_addr_t addr)
     return (addr & 0x7ff) |  (i << 11);
 }
 
-static void PPC_PCIIO_writeb (void *opaque, target_phys_addr_t addr, uint32_t val)
+static PCIDevice *PPC_dev_find_fn(PCIHostState *s, uint32_t addr)
 {
-    PREPPCIState *s = opaque;
-    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 1);
+    return pci_host_find_dev(s->bus, PPC_PCIIO_config(addr));
 }
 
-static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr, uint32_t val)
+static uint32_t PPC_config_offset_fn(PCIHostState *s, uint32_t addr)
 {
-    PREPPCIState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 2);
+    return pci_host_config_offset(PPC_PCIIO_config(addr));
 }
 
-static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr, uint32_t val)
-{
-    PREPPCIState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    pci_data_write(s->bus, PPC_PCIIO_config(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);
-    return val;
-}
-
-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);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    return val;
-}
-
-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);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    return val;
-}
-
-static CPUWriteMemoryFunc * const PPC_PCIIO_write[] = {
-    &PPC_PCIIO_writeb,
-    &PPC_PCIIO_writew,
-    &PPC_PCIIO_writel,
-};
-
-static CPUReadMemoryFunc * const PPC_PCIIO_read[] = {
-    &PPC_PCIIO_readb,
-    &PPC_PCIIO_readw,
-    &PPC_PCIIO_readl,
-};
-
 static int prep_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     return (irq_num + (pci_dev->devfn >> 3)) & 1;
@@ -132,8 +76,8 @@ PCIBus *pci_prep_init(qemu_irq *pic)
 
     pci_host_data_register_ioport(0xcfc, s);
 
-    PPC_io_memory = cpu_register_io_memory(PPC_PCIIO_read,
-                                           PPC_PCIIO_write, s);
+    pci_host_init(s, PPC_dev_find_fn, PPC_config_offset_fn);
+    PPC_io_memory = pci_host_data_register_mmio(s);
     cpu_register_physical_memory(0x80800000, 0x00400000, PPC_io_memory);
 
     /* PCI host bridge */
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index abe4c75..209b92e 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -29,13 +29,37 @@
 #include "bswap.h"
 
 typedef struct {
-    PCIBus *bus;
+    PCIHostState pci_state;
     PCIDevice *dev;
-    uint32_t par;
     uint32_t mbr;
     uint32_t iobr;
+    uint32_t par;
 } SHPCIC;
 
+static void sh_pci_data_write(SHPCIC *p, uint32_t val)
+{
+    uint32_t addr = p->par;
+    PCIDevice *dev = pci_host_find_dev(p->pci_state.bus, addr);
+    uint32_t config_offset = pci_host_config_offset(addr);
+
+    if (!dev) {
+        return;
+    }
+    dev->config_write(dev, config_offset, val, 4);
+}
+
+static uint32_t sh_pci_data_read(SHPCIC *p)
+{
+    uint32_t addr = p->par;
+    PCIDevice *dev = pci_host_find_dev(p->pci_state.bus, addr);
+    uint32_t config_offset = pci_host_config_offset(addr);
+
+    if (!dev) {
+        return ~(uint32_t)0;
+    }
+    return dev->config_read(dev, config_offset, 4);
+}
+
 static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
 {
     SHPCIC *pcic = p;
@@ -53,7 +77,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);
+        sh_pci_data_write(pcic, val);
         break;
     }
 }
@@ -67,51 +91,21 @@ 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 sh_pci_data_read(pcic);
     }
     return 0;
 }
 
-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);
-}
-
-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);
-}
-
-static void sh_pci_writeb (void *p, target_phys_addr_t addr, uint32_t val)
-{
-    sh_pci_data_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);
-}
-
-static void sh_pci_writel (void *p, target_phys_addr_t addr, uint32_t val)
+static PCIDevice *sh_pci_dev_find_fn(PCIHostState *s, uint32_t addr)
 {
-    sh_pci_data_write(p, addr, val, 4);
+    SHPCIC *pcic = DO_UPCAST(SHPCIC, pci_state, s);
+    return pci_host_find_dev(pcic->pci_state.bus, addr + pcic->mbr);
 }
 
-static uint32_t sh_pci_readb (void *p, target_phys_addr_t addr)
+static uint32_t sh_pci_config_offset_fn(PCIHostState *s, uint32_t addr)
 {
-    return sh_pci_mem_read(p, addr, 1);
-}
-
-static uint32_t sh_pci_readw (void *p, target_phys_addr_t addr)
-{
-    return sh_pci_mem_read(p, addr, 2);
-}
-
-static uint32_t sh_pci_readl (void *p, target_phys_addr_t addr)
-{
-    return sh_pci_mem_read(p, addr, 4);
+    SHPCIC *pcic = DO_UPCAST(SHPCIC, pci_state, s);
+    return pci_host_config_offset(addr + pcic->mbr);
 }
 
 static int sh_pci_addr2port(SHPCIC *pcic, target_phys_addr_t addr)
@@ -159,11 +153,6 @@ static MemOp sh_pci_reg = {
     { NULL, NULL, sh_pci_reg_write },
 };
 
-static MemOp sh_pci_mem = {
-    { sh_pci_readb, sh_pci_readw, sh_pci_readl },
-    { sh_pci_writeb, sh_pci_writew, sh_pci_writel },
-};
-
 static MemOp sh_pci_iop = {
     { sh_pci_inb, sh_pci_inw, sh_pci_inl },
     { sh_pci_outb, sh_pci_outw, sh_pci_outl },
@@ -176,14 +165,15 @@ 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->pci_state.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),
-                                 -1, NULL, NULL);
+    p->dev = pci_register_device(p->pci_state.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);
-    mem = cpu_register_io_memory(sh_pci_mem.r, sh_pci_mem.w, p);
+    pci_host_init(&p->pci_state, sh_pci_dev_find_fn, sh_pci_config_offset_fn);
+    mem = pci_host_data_register_mmio(&p->pci_state);
     cpu_register_physical_memory(0x1e200000, 0x224, reg);
     cpu_register_physical_memory(0x1e240000, 0x40000, iop);
     cpu_register_physical_memory(0x1d000000, 0x1000000, mem);
@@ -198,5 +188,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->pci_state.bus;
 }
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 153c651..31bc1cd 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_state;
     qemu_irq irq[4];
     int realview;
     int mem_config;
@@ -23,69 +23,16 @@ static inline uint32_t vpb_pci_config_addr(target_phys_addr_t addr)
     return addr & 0xffffff;
 }
 
-static void pci_vpb_config_writeb (void *opaque, target_phys_addr_t addr,
-                                   uint32_t val)
+static PCIDevice *pci_vpb_dev_find_fn(PCIHostState *s, uint32_t addr)
 {
-    pci_data_write(opaque, vpb_pci_config_addr (addr), val, 1);
+    return pci_host_find_dev(s->bus, vpb_pci_config_addr(addr));
 }
 
-static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
-                                   uint32_t val)
+static uint32_t pci_vpb_config_offset_fn(PCIHostState *s, uint32_t addr)
 {
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    pci_data_write(opaque, vpb_pci_config_addr (addr), val, 2);
+    return pci_host_config_offset(vpb_pci_config_addr(addr));
 }
 
-static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
-                                   uint32_t val)
-{
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    pci_data_write(opaque, vpb_pci_config_addr (addr), val, 4);
-}
-
-static uint32_t pci_vpb_config_readb (void *opaque, target_phys_addr_t addr)
-{
-    uint32_t val;
-    val = pci_data_read(opaque, vpb_pci_config_addr (addr), 1);
-    return val;
-}
-
-static uint32_t pci_vpb_config_readw (void *opaque, target_phys_addr_t addr)
-{
-    uint32_t val;
-    val = pci_data_read(opaque, vpb_pci_config_addr (addr), 2);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    return val;
-}
-
-static uint32_t pci_vpb_config_readl (void *opaque, target_phys_addr_t addr)
-{
-    uint32_t val;
-    val = pci_data_read(opaque, vpb_pci_config_addr (addr), 4);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    return val;
-}
-
-static CPUWriteMemoryFunc * const pci_vpb_config_write[] = {
-    &pci_vpb_config_writeb,
-    &pci_vpb_config_writew,
-    &pci_vpb_config_writel,
-};
-
-static CPUReadMemoryFunc * const pci_vpb_config_read[] = {
-    &pci_vpb_config_readb,
-    &pci_vpb_config_readw,
-    &pci_vpb_config_readl,
-};
-
 static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
 {
     return irq_num;
@@ -114,7 +61,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_state = FROM_SYSBUS(PCIHostState, dev);
+    PCIVPBState *s = DO_UPCAST(PCIVPBState, pci_state, pci_state);
     PCIBus *bus;
     int i;
 
@@ -127,8 +75,9 @@ static int pci_vpb_init(SysBusDevice *dev)
 
     /* ??? Register memory space.  */
 
-    s->mem_config = cpu_register_io_memory(pci_vpb_config_read,
-                                           pci_vpb_config_write, bus);
+    pci_host_init(&s->pci_state,
+                  pci_vpb_dev_find_fn, pci_vpb_config_offset_fn);
+    s->mem_config = pci_host_data_register_mmio(&s->pci_state);
     sysbus_init_mmio_cb(dev, 0x04000000, pci_vpb_map);
 
     pci_create_simple(bus, -1, "versatile_pci_host");
@@ -137,7 +86,8 @@ static int pci_vpb_init(SysBusDevice *dev)
 
 static int pci_realview_init(SysBusDevice *dev)
 {
-    PCIVPBState *s = FROM_SYSBUS(PCIVPBState, dev);
+    PCIHostState *pci_state = FROM_SYSBUS(PCIHostState, dev);
+    PCIVPBState *s = DO_UPCAST(PCIVPBState, pci_state, pci_state);
     s->realview = 1;
     return pci_vpb_init(dev);
 }
-- 
1.6.5.4



-- 
yamahata

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 04.01.2010, at 11:45, Isaku Yamahata wrote:

> On Mon, Jan 04, 2010 at 04:26:46AM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 21:50, Benjamin Herrenschmidt wrote:
>> 
>>> On Sun, 2010-01-03 at 21:27 +0100, Alexander Graf wrote:
>>> 
>>>> I think if unin_pci is the only user, it'd be better to do it hacky
>>>> inside unin_pci.c. But if there's a chance there's another user, it'd
>>>> be better to make it generic.
>>>> 
>>>> Since this is the first time I ever stumbled across type 0 and type 1
>>>> PCI config space addresses, I simply don't know if there are any. Blue
>>>> Swirls comment indicated that there are. Ben also sounded as if it's
>>>> rather common to not use the x86 format. On the other hand, it looks
>>>> like nobody in qemu needed it so far - and we're implementing ARM,
>>>> MIPS and all other sorts of platforms.
>>>> 
>>>> So if anyone with knowledge in PCI could shed some light here, please
>>>> do so.
>>> 
>>> My feeling is that what you're better off doing is to have the qemu core
>>> take an abstract struct to identify a device config space location, that
>>> consists of separate fields for:
>>> 
>>> - The PCI domain (which is what host bridge it hangs off since bus
>>> numbers are not unique between domains)
>>> 
>>> - The bus number
>> 
>> Hm, I think it'd make more sense to just store a PCIBus pointer in there. We could then fetch the bus and domain id from there.
>> 
>> I'll write something up :-).
>> 
>> 
>> Alex
>> 
> 
> Does the following patch help?
> I did only compile test though.

I sent out v2 already, which contains a complete resolution framework. It also allows for incremental cleanup of the users, as I'd rather like to see everyone use pci_host instead of hacky homegrown functions. But I don't think changing all at once is going to fly wrt reviewablity.

I'd be really glad if you could take a glimpse at my version. You're definitely more knowledgable in the PCI areas than me :-). I verified that it fixes Uninorth and x86 still works.


Alex

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Mon, Jan 04, 2010 at 07:45:16PM +0900, Isaku Yamahata wrote:
> +static PCIDevice *pci_host_dev_find_fn_noswap(PCIHostState *s, uint32_t addr)
> +{
> +    return pci_host_find_dev_active(s->bus, pci_host_pci_addr(s, addr));
> +}
> +
> +static PCIDevice *pci_host_dev_find_fn(PCIHostState *s, uint32_t addr)
> +{
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    addr = bswap32(addr);
> +#endif
> +    return pci_host_find_dev_active(s->bus, pci_host_pci_addr(s, addr));
> +}
> +

BTW, I think we really should think about the right way to address the
swap/noswap issue without using a preprocessor. Maybe make pci host
bridge explicitly specify whether to swap bytes?  How about adding a
field in PCIHostState to make it do this?

-- 
MST

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Isaku Yamahata]

On Mon, Jan 04, 2010 at 11:55:10AM +0100, Alexander Graf wrote:
> 
> On 04.01.2010, at 11:45, Isaku Yamahata wrote:
> 
> > On Mon, Jan 04, 2010 at 04:26:46AM +0100, Alexander Graf wrote:
> >> 
> >> On 03.01.2010, at 21:50, Benjamin Herrenschmidt wrote:
> >> 
> >>> On Sun, 2010-01-03 at 21:27 +0100, Alexander Graf wrote:
> >>> 
> >>>> I think if unin_pci is the only user, it'd be better to do it hacky
> >>>> inside unin_pci.c. But if there's a chance there's another user, it'd
> >>>> be better to make it generic.
> >>>> 
> >>>> Since this is the first time I ever stumbled across type 0 and type 1
> >>>> PCI config space addresses, I simply don't know if there are any. Blue
> >>>> Swirls comment indicated that there are. Ben also sounded as if it's
> >>>> rather common to not use the x86 format. On the other hand, it looks
> >>>> like nobody in qemu needed it so far - and we're implementing ARM,
> >>>> MIPS and all other sorts of platforms.
> >>>> 
> >>>> So if anyone with knowledge in PCI could shed some light here, please
> >>>> do so.
> >>> 
> >>> My feeling is that what you're better off doing is to have the qemu core
> >>> take an abstract struct to identify a device config space location, that
> >>> consists of separate fields for:
> >>> 
> >>> - The PCI domain (which is what host bridge it hangs off since bus
> >>> numbers are not unique between domains)
> >>> 
> >>> - The bus number
> >> 
> >> Hm, I think it'd make more sense to just store a PCIBus pointer in there. We could then fetch the bus and domain id from there.
> >> 
> >> I'll write something up :-).
> >> 
> >> 
> >> Alex
> >> 
> > 
> > Does the following patch help?
> > I did only compile test though.
> 
> I sent out v2 already, which contains a complete resolution framework. It also allows for incremental cleanup of the users, as I'd rather like to see everyone use pci_host instead of hacky homegrown functions. But I don't think changing all at once is going to fly wrt reviewablity.

Agreed. Anyway that patch is just for discussion, not for commit.
If wanted, I'm willing to split it up into a series of patches or
rebase it on top of your patches. (Or throw it away)


> I'd be really glad if you could take a glimpse at my version. You're definitely more knowledgable in the PCI areas than me :-). I verified that it fixes Uninorth and x86 still works.

I'll have a look at it tomorrow.

-- 
yamahata

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 04.01.2010, at 12:07, Michael S. Tsirkin wrote:

> On Mon, Jan 04, 2010 at 07:45:16PM +0900, Isaku Yamahata wrote:
>> +static PCIDevice *pci_host_dev_find_fn_noswap(PCIHostState *s, uint32_t addr)
>> +{
>> +    return pci_host_find_dev_active(s->bus, pci_host_pci_addr(s, addr));
>> +}
>> +
>> +static PCIDevice *pci_host_dev_find_fn(PCIHostState *s, uint32_t addr)
>> +{
>> +#ifdef TARGET_WORDS_BIGENDIAN
>> +    addr = bswap32(addr);
>> +#endif
>> +    return pci_host_find_dev_active(s->bus, pci_host_pci_addr(s, addr));
>> +}
>> +
> 
> BTW, I think we really should think about the right way to address the
> swap/noswap issue without using a preprocessor. Maybe make pci host
> bridge explicitly specify whether to swap bytes?  How about adding a
> field in PCIHostState to make it do this?

Sounds reasonable. But let's take baby steps here. I don't want to end up with a 10000 lines patch :-).


Alex

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Benjamin Herrenschmidt]

On Mon, 2010-01-04 at 13:07 +0200, Michael S. Tsirkin wrote:
> BTW, I think we really should think about the right way to address the
> swap/noswap issue without using a preprocessor. Maybe make pci host
> bridge explicitly specify whether to swap bytes?  How about adding a
> field in PCIHostState to make it do this? 

No, this is a non issue if you get your design right. Just abstract out
the reference to a device in a struct like Alex is proposing and have
the host bridge specific code fill that up appropriately. I don't see
why there would be any need for swapping and in any case, this should go
away once the host bridge code knows how to interpret the write to
whatever config access registers it exposes.

Ben.

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Tue, Jan 05, 2010 at 07:10:58AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2010-01-04 at 13:07 +0200, Michael S. Tsirkin wrote:
> > BTW, I think we really should think about the right way to address the
> > swap/noswap issue without using a preprocessor. Maybe make pci host
> > bridge explicitly specify whether to swap bytes?  How about adding a
> > field in PCIHostState to make it do this? 
> 
> No, this is a non issue if you get your design right. Just abstract out
> the reference to a device in a struct like Alex is proposing and have
> the host bridge specific code fill that up appropriately. I don't see
> why there would be any need for swapping and in any case, this should go
> away once the host bridge code knows how to interpret the write to
> whatever config access registers it exposes.
> 
> Ben.

Well, the main issue if I understand correcttly is that basically the
same hardware bridge can be connected to host in different ways. Yes, we
can say "if it's connected differently it's a different device" but this
is slightly ugly, device should not have to know how it's connected. It
would be cleaner to have a "connector" device in the middle that swaps
bytes.  Even though yes, what you describe would be less ugly than using
proprocessor as we do now.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Benjamin Herrenschmidt]

On Mon, 2010-01-04 at 23:12 +0200, Michael S. Tsirkin wrote:
> Well, the main issue if I understand correcttly is that basically the
> same hardware bridge can be connected to host in different ways. Yes, we
> can say "if it's connected differently it's a different device" but this
> is slightly ugly, device should not have to know how it's connected. It
> would be cleaner to have a "connector" device in the middle that swaps
> bytes.  Even though yes, what you describe would be less ugly than using
> proprocessor as we do now.

Well, the thing is... PCI is always little endian. I'm not 100% familiar
on how emulation of devices works in qemu, but it's possible that the
problem here is simply not how a standard PCI host bridge is connected
to the processor changing but rather whether it's connected to an x86
host or a ppc host should make the byte order appear different. IE. a
PPC operating system will byteswap accesses. If qemu just passes on
accesses -as-is- instead of doing natural byteswapping then indeed you
will need that added swap there too.

I still think though that this should be buried in the accessors for the
host bridge themselves, eventually controlled by a flag set when
instanciating the host bridge in case it can indeed be wired in
different ways.

Cheers,
Ben.
 

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Tue, Jan 05, 2010 at 08:25:30AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2010-01-04 at 23:12 +0200, Michael S. Tsirkin wrote:
> > Well, the main issue if I understand correcttly is that basically the
> > same hardware bridge can be connected to host in different ways. Yes, we
> > can say "if it's connected differently it's a different device" but this
> > is slightly ugly, device should not have to know how it's connected. It
> > would be cleaner to have a "connector" device in the middle that swaps
> > bytes.  Even though yes, what you describe would be less ugly than using
> > proprocessor as we do now.
> 
> Well, the thing is... PCI is always little endian.
> I'm not 100% familiar
> on how emulation of devices works in qemu, but it's possible that the
> problem here is simply not how a standard PCI host bridge is connected
> to the processor changing but rather whether it's connected to an x86
> host or a ppc host should make the byte order appear different. IE. a
> PPC operating system will byteswap accesses. If qemu just passes on
> accesses -as-is- instead of doing natural byteswapping then indeed you
> will need that added swap there too.
> 

Yes, but I think how you program your host to pci bridge is platform specific,
the standard (mostly) applies to what happens below the bridge.  There's
no real standard for how PCI host bridge is connected to processor
AFAIK, it's by luck we can share code there at all.

> I still think though that this should be buried in the accessors for the
> host bridge themselves, eventually controlled by a flag set when
> instanciating the host bridge in case it can indeed be wired in
> different ways.
> 
> Cheers,
> Ben.
>  

buried sounds scary, but generally yes, a flag in host bridge code
is the idea.

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Benjamin Herrenschmidt]

> Yes, but I think how you program your host to pci bridge is platform specific,
> the standard (mostly) applies to what happens below the bridge.  There's
> no real standard for how PCI host bridge is connected to processor
> AFAIK, it's by luck we can share code there at all.

Well, yes and no ... there's a standard on how a PCI host bridge is
connected in the sense that how normal MMIO accesses go through in term
of endianness is well defined.

How you actually issue config space cycles is a property of a given
bridge. How you issue IO cycles as well in fact.

Cheers,
Ben.

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Tue, Jan 05, 2010 at 08:53:52AM +1100, Benjamin Herrenschmidt wrote:
> 
> > Yes, but I think how you program your host to pci bridge is platform specific,
> > the standard (mostly) applies to what happens below the bridge.  There's
> > no real standard for how PCI host bridge is connected to processor
> > AFAIK, it's by luck we can share code there at all.
> 
> Well, yes and no ... there's a standard on how a PCI host bridge is
> connected in the sense that how normal MMIO accesses go through in term
> of endianness is well defined.
> 

Go through where? From processor to PCI?
Which spec do you refer to?

> How you actually issue config space cycles is a property of a given
> bridge. How you issue IO cycles as well in fact.
> 
> Cheers,
> Ben.

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Benjamin Herrenschmidt]

On Tue, 2010-01-05 at 00:25 +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 05, 2010 at 08:53:52AM +1100, Benjamin Herrenschmidt wrote:
> > 
> > > Yes, but I think how you program your host to pci bridge is platform specific,
> > > the standard (mostly) applies to what happens below the bridge.  There's
> > > no real standard for how PCI host bridge is connected to processor
> > > AFAIK, it's by luck we can share code there at all.
> > 
> > Well, yes and no ... there's a standard on how a PCI host bridge is
> > connected in the sense that how normal MMIO accesses go through in term
> > of endianness is well defined.
> > 
> 
> Go through where? From processor to PCI?
> Which spec do you refer to?

The PCI spec from memory :-) Byte swizzling for MMIO between a processor
and PCI bus is well defined.

Now of course, since issuing config space cycles tend to be host-bridge
chip specific, everything is possible there :-) In -most- cases though,
they use a mechanism similar to x86 using the cf8/cfc ports or
equivalent mapped wherever the PIO region is mapped in MMIO space for
non-x86 processors, and thus end up with the exact same byte swizzling.

In fact, this is true of accesses to PCI devices as well. Take for
example, a device that has a 32-bit MMIO register. This register is
meant to appear as little endian (well, unless the device itself plays
tricks but most don't) whatever the host processor is. Thus an x86 host
will need no byteswap but a powerpc host (assuming the ppc is running in
BE mode) will.

This is why for example the base readl and writel function in Linux do
byteswap on powerpc.

This is important to understand that this is a property of how the PCI
bridge is connected to the host processor, such that the PCI "native"
byte order is preserved along with address invariance for sub-32-bit
quantities.

The endianness of the host bridge config space access register is thus
most of the time just a natural side effect of said register being part
of the bridge PIO space and thus getting the natural byteswap explained
above for a 32-bit LE register on PCI.

Cheers,
Ben.

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Tue, Jan 05, 2010 at 09:51:48AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2010-01-05 at 00:25 +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 05, 2010 at 08:53:52AM +1100, Benjamin Herrenschmidt wrote:
> > > 
> > > > Yes, but I think how you program your host to pci bridge is platform specific,
> > > > the standard (mostly) applies to what happens below the bridge.  There's
> > > > no real standard for how PCI host bridge is connected to processor
> > > > AFAIK, it's by luck we can share code there at all.
> > > 
> > > Well, yes and no ... there's a standard on how a PCI host bridge is
> > > connected in the sense that how normal MMIO accesses go through in term
> > > of endianness is well defined.
> > > 
> > 
> > Go through where? From processor to PCI?
> > Which spec do you refer to?
> 
> The PCI spec from memory :-) Byte swizzling for MMIO between a processor
> and PCI bus is well defined.

Couldn't find it in spec.

> Now of course, since issuing config space cycles tend to be host-bridge
> chip specific, everything is possible there :-) In -most- cases though,
> they use a mechanism similar to x86 using the cf8/cfc ports or
> equivalent mapped wherever the PIO region is mapped in MMIO space for
> non-x86 processors, and thus end up with the exact same byte swizzling.
> 
> In fact, this is true of accesses to PCI devices as well. Take for
> example, a device that has a 32-bit MMIO register. This register is
> meant to appear as little endian (well, unless the device itself plays
> tricks but most don't) whatever the host processor is. Thus an x86 host
> will need no byteswap but a powerpc host (assuming the ppc is running in
> BE mode) will.
> 
> This is why for example the base readl and writel function in Linux do
> byteswap on powerpc.
> 
> This is important to understand that this is a property of how the PCI
> bridge is connected to the host processor, such that the PCI "native"
> byte order is preserved along with address invariance for sub-32-bit
> quantities.
> 
> The endianness of the host bridge config space access register is thus
> most of the time just a natural side effect of said register being part
> of the bridge PIO space and thus getting the natural byteswap explained
> above for a 32-bit LE register on PCI.
> 
> Cheers,
> Ben.

So, it appears that this is not the case for many platforms: bridge
itself does a byteswap to make devices behind it work according to spec,
but this does not apply to programming bridge itself.

This seems common on BE platforms, this is why qemu has
ifdef TARGET_WORDS_BIGENDIAN there IIUC.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 04.01.2010, at 23:59, Michael S. Tsirkin wrote:

> On Tue, Jan 05, 2010 at 09:51:48AM +1100, Benjamin Herrenschmidt wrote:
>> On Tue, 2010-01-05 at 00:25 +0200, Michael S. Tsirkin wrote:
>>> On Tue, Jan 05, 2010 at 08:53:52AM +1100, Benjamin Herrenschmidt wrote:
>>>> 
>>>>> Yes, but I think how you program your host to pci bridge is platform specific,
>>>>> the standard (mostly) applies to what happens below the bridge.  There's
>>>>> no real standard for how PCI host bridge is connected to processor
>>>>> AFAIK, it's by luck we can share code there at all.
>>>> 
>>>> Well, yes and no ... there's a standard on how a PCI host bridge is
>>>> connected in the sense that how normal MMIO accesses go through in term
>>>> of endianness is well defined.
>>>> 
>>> 
>>> Go through where? From processor to PCI?
>>> Which spec do you refer to?
>> 
>> The PCI spec from memory :-) Byte swizzling for MMIO between a processor
>> and PCI bus is well defined.
> 
> Couldn't find it in spec.
> 
>> Now of course, since issuing config space cycles tend to be host-bridge
>> chip specific, everything is possible there :-) In -most- cases though,
>> they use a mechanism similar to x86 using the cf8/cfc ports or
>> equivalent mapped wherever the PIO region is mapped in MMIO space for
>> non-x86 processors, and thus end up with the exact same byte swizzling.
>> 
>> In fact, this is true of accesses to PCI devices as well. Take for
>> example, a device that has a 32-bit MMIO register. This register is
>> meant to appear as little endian (well, unless the device itself plays
>> tricks but most don't) whatever the host processor is. Thus an x86 host
>> will need no byteswap but a powerpc host (assuming the ppc is running in
>> BE mode) will.
>> 
>> This is why for example the base readl and writel function in Linux do
>> byteswap on powerpc.
>> 
>> This is important to understand that this is a property of how the PCI
>> bridge is connected to the host processor, such that the PCI "native"
>> byte order is preserved along with address invariance for sub-32-bit
>> quantities.
>> 
>> The endianness of the host bridge config space access register is thus
>> most of the time just a natural side effect of said register being part
>> of the bridge PIO space and thus getting the natural byteswap explained
>> above for a 32-bit LE register on PCI.
>> 
>> Cheers,
>> Ben.
> 
> So, it appears that this is not the case for many platforms: bridge
> itself does a byteswap to make devices behind it work according to spec,
> but this does not apply to programming bridge itself.
> 
> This seems common on BE platforms, this is why qemu has
> ifdef TARGET_WORDS_BIGENDIAN there IIUC.

IIRC qemu's mmio functions just pass the register value the guest had at that moment to the mmio function.

While that is pretty simple, it means that we behave different from real hardware. Real hardware has 2 components:

1) CPU
2) Device

The CPU sends an MMIO request in local byte order to the device. The device also has a native endianness - either BE or LE.
So what the byte swap here does is simply converting from wrong LE (what Linux thought the device needs) to a proper variable.

I'm not sure what the correct solution to this is. I'm inclined to suggest that mmio callbacks should get called with tswap'ed values. But then again the code as is works in quite a lot of cases and I don't want to spend months getting it back to where it is just because of a cosmetic change.

Alex

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

On Tue, Jan 05, 2010 at 12:08:19AM +0100, Alexander Graf wrote:
> 
> On 04.01.2010, at 23:59, Michael S. Tsirkin wrote:
> 
> > On Tue, Jan 05, 2010 at 09:51:48AM +1100, Benjamin Herrenschmidt wrote:
> >> On Tue, 2010-01-05 at 00:25 +0200, Michael S. Tsirkin wrote:
> >>> On Tue, Jan 05, 2010 at 08:53:52AM +1100, Benjamin Herrenschmidt wrote:
> >>>> 
> >>>>> Yes, but I think how you program your host to pci bridge is platform specific,
> >>>>> the standard (mostly) applies to what happens below the bridge.  There's
> >>>>> no real standard for how PCI host bridge is connected to processor
> >>>>> AFAIK, it's by luck we can share code there at all.
> >>>> 
> >>>> Well, yes and no ... there's a standard on how a PCI host bridge is
> >>>> connected in the sense that how normal MMIO accesses go through in term
> >>>> of endianness is well defined.
> >>>> 
> >>> 
> >>> Go through where? From processor to PCI?
> >>> Which spec do you refer to?
> >> 
> >> The PCI spec from memory :-) Byte swizzling for MMIO between a processor
> >> and PCI bus is well defined.
> > 
> > Couldn't find it in spec.
> > 
> >> Now of course, since issuing config space cycles tend to be host-bridge
> >> chip specific, everything is possible there :-) In -most- cases though,
> >> they use a mechanism similar to x86 using the cf8/cfc ports or
> >> equivalent mapped wherever the PIO region is mapped in MMIO space for
> >> non-x86 processors, and thus end up with the exact same byte swizzling.
> >> 
> >> In fact, this is true of accesses to PCI devices as well. Take for
> >> example, a device that has a 32-bit MMIO register. This register is
> >> meant to appear as little endian (well, unless the device itself plays
> >> tricks but most don't) whatever the host processor is. Thus an x86 host
> >> will need no byteswap but a powerpc host (assuming the ppc is running in
> >> BE mode) will.
> >> 
> >> This is why for example the base readl and writel function in Linux do
> >> byteswap on powerpc.
> >> 
> >> This is important to understand that this is a property of how the PCI
> >> bridge is connected to the host processor, such that the PCI "native"
> >> byte order is preserved along with address invariance for sub-32-bit
> >> quantities.
> >> 
> >> The endianness of the host bridge config space access register is thus
> >> most of the time just a natural side effect of said register being part
> >> of the bridge PIO space and thus getting the natural byteswap explained
> >> above for a 32-bit LE register on PCI.
> >> 
> >> Cheers,
> >> Ben.
> > 
> > So, it appears that this is not the case for many platforms: bridge
> > itself does a byteswap to make devices behind it work according to spec,
> > but this does not apply to programming bridge itself.
> > 
> > This seems common on BE platforms, this is why qemu has
> > ifdef TARGET_WORDS_BIGENDIAN there IIUC.
> 
> IIRC qemu's mmio functions just pass the register value the guest had at that moment to the mmio function.
> 
> While that is pretty simple, it means that we behave different from real hardware. Real hardware has 2 components:
> 
> 1) CPU
> 2) Device
> 
> The CPU sends an MMIO request in local byte order to the device. The device also has a native endianness - either BE or LE.
> So what the byte swap here does is simply converting from wrong LE (what Linux thought the device needs) to a proper variable.
> 
> I'm not sure what the correct solution to this is. I'm inclined to suggest that mmio callbacks should get called with tswap'ed values. But then again the code as is works in quite a lot of cases and I don't want to spend months getting it back to where it is just because of a cosmetic change.
> 
> Alex

If I understand correctly this is just a byteswap to emulate how host
bridge is connected, unrelated to local byte order.  By anyway, I agree:
let's not spend months on this.

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Benjamin Herrenschmidt]

> So, it appears that this is not the case for many platforms: bridge
> itself does a byteswap to make devices behind it work according to spec,
> but this does not apply to programming bridge itself.
> 
> This seems common on BE platforms, this is why qemu has
> ifdef TARGET_WORDS_BIGENDIAN there IIUC.

Sadly, it is quite often that misleaded HW designers thing they are
doing SW a service by making something BE instead of LE on a BE
platform, but in the end just forcing everybody to special case :-)

It's hard to say what is the most common. All powerpc chrp have it LE
afaik, FSL tried to be smart (FAIL) and got their SoC config space
access reg BE instead. Apple stuff is just "special", etc...

In any case, it doesn't matter much as long as qemu gets normal MMIO
emulation right. Host bridge config space is chipset specific and as
such platform specific.

Cheers,
Ben.

Re: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Benjamin Herrenschmidt]

On Tue, 2010-01-05 at 00:08 +0100, Alexander Graf wrote:
> 
> IIRC qemu's mmio functions just pass the register value the guest had
> at that moment to the mmio function.

That means that qemu HW emulation needs, for each device, to add a layer
of byteswap depending on whether the CPU is LE or BE which sucks :-)

> While that is pretty simple, it means that we behave different from
> real hardware. Real hardware has 2 components:
> 
> 1) CPU
> 2) Device
> 
> The CPU sends an MMIO request in local byte order to the device. The
> device also has a native endianness - either BE or LE.
> So what the byte swap here does is simply converting from wrong LE
> (what Linux thought the device needs) to a proper variable.

Not quite. The CPU sends an MMIO request to a PCI host bridge. For
devices (and I'm not talking about the host bridge registers themselves,
those are on the CPU bus and could be wired in any fancy way), but for
PCI devices behind that bridge, you expect things to be wired in such a
way that the BE CPU MSB is wired to the PCI LSB.

IE. If a piece of C code writes 0xaabbccdd to a PCI device 32-bit
register using a native aligned access from the CPU of the form
*(unsigned int *)addr, the device should see 0xaabbccdd with a LE CPU
and 0xddccbbaa with a BE CPU :-) Really ! This is why writel() in linux
will byteswap on BE CPUs.

Ideally, QEMU should do the same swapping for all accesses over PCI in
order to avoid to CPU conditional byteswap tricks in the device
emulation proper.

Now, regarding the host bridge own registers, as I said above, if they
are legacy IO cf8/cfc (which is the case of most CHRPs for example) they
will be LE just the same way as above. Some SoCs like FSL do try to be
smart and get them BE though. But yeah, that is all specific to a given
host bridge.

> I'm not sure what the correct solution to this is. I'm inclined to
> suggest that mmio callbacks should get called with tswap'ed values.
> But then again the code as is works in quite a lot of cases and I
> don't want to spend months getting it back to where it is just because
> of a cosmetic change.

Well, I'd say that MMIO callbacks should continue being native for
anything on the CPU bus, -but- when crossing a bridge, they may need to
get some swapping done depending on what that bridge does. For example,
I would expect a powerpc/amba bridge to do a similar swapping as above
for a powerpc/pci bridge.

Cheers,
Ben.

> Alex 

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Blue Swirl]

On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>>
>>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>>
>>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>>> >>
>>> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>>> >>
>>> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>> >>>>
>>> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>> >>>>
>>> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>> >>>>>> makes a qemu understandable one out of it.
>>> >>>>>>
>>> >>>>>> 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>
>>> >>>>>
>>> >>>>> Thanks!
>>> >>>>>
>>> >>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>> >>>>> other architectures are converted to x86.  As long as we are adding
>>> >>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>> >>>>> return register and devfn separately, so we don't need to encode/decode
>>> >>>>> back and forth?
>>> >>>>
>>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>>> >>>>
>>> >>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>>> >>>>
>>> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>> >>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>> >>>>> etc?
>>> >>>>
>>> >>>> Hm, I figured this is less work :-).
>>> >>>
>>> >>> Let me explain the idea: we have:
>>> >>>
>>> >>>   static void pci_host_config_writel_ioport(void *opaque,
>>> >>>                                             uint32_t addr, uint32_t val)
>>> >>>   {
>>> >>>       PCIHostState *s = opaque;
>>> >>>
>>> >>>       PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>> >>>   val);
>>> >>>       s->config_reg = val;
>>> >>>   }
>>> >>>
>>> >>>   static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>> >>>   addr)
>>> >>>   {
>>> >>>       PCIHostState *s = opaque;
>>> >>>       uint32_t val = s->config_reg;
>>> >>>
>>> >>>       PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>> >>>   val);
>>> >>>       return val;
>>> >>>   }
>>> >>>
>>> >>>   void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>> >>>   {
>>> >>>       register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>> >>>   s);
>>> >>>       register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>> >>>   }
>>> >>>
>>> >>> If instead of a simple config_reg = val we translate to pc format
>>> >>> here, the rest will work. No?
>>> >>
>>> >> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
>>> >
>>> > Right.
>>> >
>>> >> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>>> >>
>>> >> Alex
>>> >
>>> > There's already ugliness with swap/noswap versions in there.  Anything
>>> > that claims to be a proper framework would need to address that as well
>>> > IMO.
>>>
>>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>>> incrementally improving the existing code?
>>
>> Not really, incremental improvements are good.  But what you posted does
>> not seem to clean up most code, what it really does is fixes ppc
>> unin_pci.  My feeling was since there's only one user for now maybe it
>> is better to just have device specific code rather than complicate
>> generic code. Makes sense?
>>
>> We need to see several users before we add yet another level of
>> indirection.  If there is a level of indirection that solves the
>> swap/noswap ugliness as well that would show this is a good abstraction.
>> I think I even know what a good abstraction would be: decode address on
>> write and keep it in decoded form.
>
> I think Sparc64 PBM configuration cycles are also a bit different.
> It's described in UltraSPARC User's Manual 805-0087, p.325.
>
> Currently both QEMU and OpenBIOS just use something common, but as
> soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
> that.

That time is very close, with latest QEMU and OpenBIOS, PCI probe
starts (with GDB tricks for calibrate_delay, which won't be needed
after %tick_cmpr is fixed):

[sparc64] Kernel already loaded

PROMLIB: Sun IEEE Boot Prom 'OBP 3.10.24 1999/01/01 01:01'
PROMLIB: Root node compatible: sun4u
Linux version 2.6.32 (test@host) (gcc version 4.2.4) #3 Sat Jan 9
20:36:42 UTC 2010
bootconsole [earlyprom0] enabled
ARCH: SUN4U
Ethernet address: 52:54:00:12:34:56
Kernel: Using 1 locked TLB entries for main kernel image.
Remapping the kernel... done.
OF stdout device is: /pci@1fe,0/pci@1/pci@1,1/ebus@3/su@1fe
PROM: Built device tree with 32165 bytes of memory.
Top of RAM: 0x7e80000, Total RAM: 0x7e80000
Memory hole size: 0MB
Zone PFN ranges:
  Normal   0x00000000 -> 0x00003f40
Movable zone start PFN for each node
early_node_map[1] active PFN ranges
    0: 0x00000000 -> 0x00003f40
On node 0 totalpages: 16192
  Normal zone: 127 pages used for memmap
  Normal zone: 0 pages reserved
  Normal zone: 16065 pages, LIFO batch:3
Booting Linux...
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16065
Kernel command line: root=/dev/hdc1 console=ttyS0 -p debug
PID hash table entries: 512 (order: -1, 4096 bytes)
Dentry cache hash table entries: 16384 (order: 4, 131072 bytes)
Inode-cache hash table entries: 8192 (order: 3, 65536 bytes)
Memory: 118480k available (1472k kernel code, 488k data, 104k init)
[fffff80000000000,0000000007e80000]
SLUB: Genslabs=14, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
Hierarchical RCU implementation.
NR_IRQS:255
clocksource: mult[a0000] shift[16]
clockevent: mult[19999999] shift[32]
Console: colour dummy device 80x25
Calibrating delay loop (skipped) preset value.. 5000.00 BogoMIPS (lpj=10000000)
Mount-cache hash table entries: 512
/pci: PCI IO[1fe02000000] MEM[1ff00000000]
/pci: SABRE PCI Bus Module ver[0:0]
PCI: Scanning PBM /pci

There it hangs because the probed address is bad:
(gdb)
0x000000000043a1a8      75              __asm__ __volatile__("membar #Sync\n\t"
1: x/i $pc
0x43a1a8 <pci_config_read16+40>:        lduha  [ %o0 ] (29), %o4
(gdb) p $o0
$12 = 0x1fe01000806

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Blue Swirl]

On Sun, Jan 10, 2010 at 6:41 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>>>
>>>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>>>
>>>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>>>> >>
>>>> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>>>> >>
>>>> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>> >>>>
>>>> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>> >>>>
>>>> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>>> >>>>>> makes a qemu understandable one out of it.
>>>> >>>>>>
>>>> >>>>>> 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>
>>>> >>>>>
>>>> >>>>> Thanks!
>>>> >>>>>
>>>> >>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>>> >>>>> other architectures are converted to x86.  As long as we are adding
>>>> >>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>>> >>>>> return register and devfn separately, so we don't need to encode/decode
>>>> >>>>> back and forth?
>>>> >>>>
>>>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>>>> >>>>
>>>> >>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>>>> >>>>
>>>> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>> >>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>>> >>>>> etc?
>>>> >>>>
>>>> >>>> Hm, I figured this is less work :-).
>>>> >>>
>>>> >>> Let me explain the idea: we have:
>>>> >>>
>>>> >>>   static void pci_host_config_writel_ioport(void *opaque,
>>>> >>>                                             uint32_t addr, uint32_t val)
>>>> >>>   {
>>>> >>>       PCIHostState *s = opaque;
>>>> >>>
>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>>> >>>   val);
>>>> >>>       s->config_reg = val;
>>>> >>>   }
>>>> >>>
>>>> >>>   static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>>> >>>   addr)
>>>> >>>   {
>>>> >>>       PCIHostState *s = opaque;
>>>> >>>       uint32_t val = s->config_reg;
>>>> >>>
>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>>> >>>   val);
>>>> >>>       return val;
>>>> >>>   }
>>>> >>>
>>>> >>>   void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>>> >>>   {
>>>> >>>       register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>>> >>>   s);
>>>> >>>       register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>>> >>>   }
>>>> >>>
>>>> >>> If instead of a simple config_reg = val we translate to pc format
>>>> >>> here, the rest will work. No?
>>>> >>
>>>> >> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
>>>> >
>>>> > Right.
>>>> >
>>>> >> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>>>> >>
>>>> >> Alex
>>>> >
>>>> > There's already ugliness with swap/noswap versions in there.  Anything
>>>> > that claims to be a proper framework would need to address that as well
>>>> > IMO.
>>>>
>>>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>>>> incrementally improving the existing code?
>>>
>>> Not really, incremental improvements are good.  But what you posted does
>>> not seem to clean up most code, what it really does is fixes ppc
>>> unin_pci.  My feeling was since there's only one user for now maybe it
>>> is better to just have device specific code rather than complicate
>>> generic code. Makes sense?
>>>
>>> We need to see several users before we add yet another level of
>>> indirection.  If there is a level of indirection that solves the
>>> swap/noswap ugliness as well that would show this is a good abstraction.
>>> I think I even know what a good abstraction would be: decode address on
>>> write and keep it in decoded form.
>>
>> I think Sparc64 PBM configuration cycles are also a bit different.
>> It's described in UltraSPARC User's Manual 805-0087, p.325.
>>
>> Currently both QEMU and OpenBIOS just use something common, but as
>> soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
>> that.
>
> That time is very close, with latest QEMU and OpenBIOS, PCI probe
> starts (with GDB tricks for calibrate_delay, which won't be needed
> after %tick_cmpr is fixed):

Now I get this:
[sparc64] Kernel already loaded

PROMLIB: Sun IEEE Boot Prom 'OBP 3.10.24 1999/01/01 01:01'
PROMLIB: Root node compatible: sun4u
Linux version 2.6.32 (test@host) (gcc version 4.2.4) #3 Sat Jan 9
20:36:42 UTC 2010
bootconsole [earlyprom0] enabled
ARCH: SUN4U
Ethernet address: 52:54:00:12:34:56
Kernel: Using 1 locked TLB entries for main kernel image.
Remapping the kernel... done.
OF stdout device is: /pci@1fe,0/pci@1/pci@1,1/ebus@3/su@1fe
PROM: Built device tree with 32176 bytes of memory.
Top of RAM: 0x7e80000, Total RAM: 0x7e80000
Memory hole size: 0MB
Zone PFN ranges:
  Normal   0x00000000 -> 0x00003f40
Movable zone start PFN for each node
early_node_map[1] active PFN ranges
    0: 0x00000000 -> 0x00003f40
On node 0 totalpages: 16192
  Normal zone: 127 pages used for memmap
  Normal zone: 0 pages reserved
  Normal zone: 16065 pages, LIFO batch:3
Booting Linux...
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16065
Kernel command line: root=/dev/hda1 console=ttyS0 -p debug lpj=100000
PID hash table entries: 512 (order: -1, 4096 bytes)
Dentry cache hash table entries: 16384 (order: 4, 131072 bytes)
Inode-cache hash table entries: 8192 (order: 3, 65536 bytes)
Memory: 118480k available (1472k kernel code, 488k data, 104k init)
[fffff80000000000,0000000007e80000]
SLUB: Genslabs=14, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
Hierarchical RCU implementation.
NR_IRQS:255
clocksource: mult[a0000] shift[16]
clockevent: mult[19999999] shift[32]
Console: colour dummy device 80x25
Calibrating delay loop (skipped) preset value.. 50.00 BogoMIPS (lpj=100000)
Mount-cache hash table entries: 512
/pci: PCI IO[1fe02000000] MEM[1ff00000000]
/pci: SABRE PCI Bus Module ver[0:0]
PCI: Scanning PBM /pci
------------[ cut here ]------------
WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
Call Trace:
 [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
 [00000000004e92ec] sysfs_add_one+0x8c/0xc0
 [00000000004ea480] sysfs_do_create_link+0x80/0x140
 [000000000053fc88] device_add+0x3c8/0x500
 [0000000000513f34] pci_bus_add_child+0x14/0x80
 [0000000000514144] pci_bus_add_devices+0x144/0x200
 [00000000005140ec] pci_bus_add_devices+0xec/0x200
 [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
 [000000000056d6f0] sabre_probe+0x2d0/0x5a0
 [0000000000569d9c] of_platform_device_probe+0x3c/0x80
 [00000000005427b0] driver_probe_device+0x70/0x160
 [0000000000542918] __driver_attach+0x78/0xa0
 [0000000000541bcc] bus_for_each_dev+0x4c/0x80
 [00000000005421dc] bus_add_driver+0x9c/0x240
 [0000000000542c10] driver_register+0x50/0x160
 [0000000000426a98] do_one_initcall+0x18/0x160
---[ end trace 139ce121c98e96c9 ]---
pci 0000:00:01.1: Error adding bus, continuing
------------[ cut here ]------------
WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
Call Trace:
 [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
 [00000000004e92ec] sysfs_add_one+0x8c/0xc0
 [00000000004ea480] sysfs_do_create_link+0x80/0x140
 [000000000053fc88] device_add+0x3c8/0x500
 [0000000000513f34] pci_bus_add_child+0x14/0x80
 [0000000000514144] pci_bus_add_devices+0x144/0x200
 [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
 [000000000056d6f0] sabre_probe+0x2d0/0x5a0
 [0000000000569d9c] of_platform_device_probe+0x3c/0x80
 [00000000005427b0] driver_probe_device+0x70/0x160
 [0000000000542918] __driver_attach+0x78/0xa0
 [0000000000541bcc] bus_for_each_dev+0x4c/0x80
 [00000000005421dc] bus_add_driver+0x9c/0x240
 [0000000000542c10] driver_register+0x50/0x160
 [0000000000426a98] do_one_initcall+0x18/0x160
 [00000000005f4274] kernel_init+0xd4/0x140
---[ end trace 139ce121c98e96ca ]---
pci 0000:00:01.0: Error adding bus, continuing
bio: create slab <bio-0> at 0
vgaarb: loaded
Switching to clocksource tick
io scheduler noop registered (default)
------------[ cut here ]------------
WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
proc_dir_entry 'pci/0000:00' already registered
Call Trace:
 [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
 [00000000004e2948] proc_register+0xe8/0x1c0
 [00000000004e2c30] proc_mkdir_mode+0x30/0x60
 [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
 [00000000006027fc] pci_proc_init+0x5c/0xa0
 [0000000000426a98] do_one_initcall+0x18/0x160
 [00000000005f4274] kernel_init+0xd4/0x140
 [000000000042b130] kernel_thread+0x30/0x60
 [000000000056a8f8] rest_init+0x18/0x80
---[ end trace 139ce121c98e96cb ]---
------------[ cut here ]------------
WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
proc_dir_entry 'pci/0000:00' already registered
Call Trace:
 [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
 [00000000004e2948] proc_register+0xe8/0x1c0
 [00000000004e2c30] proc_mkdir_mode+0x30/0x60
 [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
 [00000000006027fc] pci_proc_init+0x5c/0xa0
 [0000000000426a98] do_one_initcall+0x18/0x160
 [00000000005f4274] kernel_init+0xd4/0x140
 [000000000042b130] kernel_thread+0x30/0x60
 [000000000056a8f8] rest_init+0x18/0x80
---[ end trace 139ce121c98e96cc ]---
Uniform Multi-Platform E-IDE driver
cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
PCI: Enabling device: (0000:00:05.0), cmd 301
cmd64x 0000:00:05.0: unable to enable IDE controller
cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
CMD64x_IDE 0000:00:05.0: BAR 0: can't reserve I/O region
[0x1fe02000500-0x1fe02000507]
cmd64x 0000:00:05.0: can't reserve resources
CMD64x_IDE: probe of 0000:00:05.0 failed with error -16
ide-gd driver 1.18
ide-cd driver 5.00
mice: PS/2 mouse device common for all mice
turn off boot console earlyprom0

So PCI probe seems to work. The errors show some bugs with the APB bridges.

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Igor Kovalenko]

On Tue, Jan 12, 2010 at 12:29 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jan 10, 2010 at 6:41 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>>>>
>>>>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>>>>
>>>>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>>>>> >>
>>>>> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>>>>> >>
>>>>> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>>> >>>>
>>>>> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>>> >>>>
>>>>> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>>>> >>>>>> makes a qemu understandable one out of it.
>>>>> >>>>>>
>>>>> >>>>>> 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>
>>>>> >>>>>
>>>>> >>>>> Thanks!
>>>>> >>>>>
>>>>> >>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>>>> >>>>> other architectures are converted to x86.  As long as we are adding
>>>>> >>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>>>> >>>>> return register and devfn separately, so we don't need to encode/decode
>>>>> >>>>> back and forth?
>>>>> >>>>
>>>>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>>>>> >>>>
>>>>> >>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>>>>> >>>>
>>>>> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>>> >>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>>>> >>>>> etc?
>>>>> >>>>
>>>>> >>>> Hm, I figured this is less work :-).
>>>>> >>>
>>>>> >>> Let me explain the idea: we have:
>>>>> >>>
>>>>> >>>   static void pci_host_config_writel_ioport(void *opaque,
>>>>> >>>                                             uint32_t addr, uint32_t val)
>>>>> >>>   {
>>>>> >>>       PCIHostState *s = opaque;
>>>>> >>>
>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>>>> >>>   val);
>>>>> >>>       s->config_reg = val;
>>>>> >>>   }
>>>>> >>>
>>>>> >>>   static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>>>> >>>   addr)
>>>>> >>>   {
>>>>> >>>       PCIHostState *s = opaque;
>>>>> >>>       uint32_t val = s->config_reg;
>>>>> >>>
>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>>>> >>>   val);
>>>>> >>>       return val;
>>>>> >>>   }
>>>>> >>>
>>>>> >>>   void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>>>> >>>   {
>>>>> >>>       register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>>>> >>>   s);
>>>>> >>>       register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>>>> >>>   }
>>>>> >>>
>>>>> >>> If instead of a simple config_reg = val we translate to pc format
>>>>> >>> here, the rest will work. No?
>>>>> >>
>>>>> >> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
>>>>> >
>>>>> > Right.
>>>>> >
>>>>> >> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>>>>> >>
>>>>> >> Alex
>>>>> >
>>>>> > There's already ugliness with swap/noswap versions in there.  Anything
>>>>> > that claims to be a proper framework would need to address that as well
>>>>> > IMO.
>>>>>
>>>>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>>>>> incrementally improving the existing code?
>>>>
>>>> Not really, incremental improvements are good.  But what you posted does
>>>> not seem to clean up most code, what it really does is fixes ppc
>>>> unin_pci.  My feeling was since there's only one user for now maybe it
>>>> is better to just have device specific code rather than complicate
>>>> generic code. Makes sense?
>>>>
>>>> We need to see several users before we add yet another level of
>>>> indirection.  If there is a level of indirection that solves the
>>>> swap/noswap ugliness as well that would show this is a good abstraction.
>>>> I think I even know what a good abstraction would be: decode address on
>>>> write and keep it in decoded form.
>>>
>>> I think Sparc64 PBM configuration cycles are also a bit different.
>>> It's described in UltraSPARC User's Manual 805-0087, p.325.
>>>
>>> Currently both QEMU and OpenBIOS just use something common, but as
>>> soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
>>> that.
>>
>> That time is very close, with latest QEMU and OpenBIOS, PCI probe
>> starts (with GDB tricks for calibrate_delay, which won't be needed
>> after %tick_cmpr is fixed):
>
> Now I get this:
> [sparc64] Kernel already loaded
>
> PROMLIB: Sun IEEE Boot Prom 'OBP 3.10.24 1999/01/01 01:01'
> PROMLIB: Root node compatible: sun4u
> Linux version 2.6.32 (test@host) (gcc version 4.2.4) #3 Sat Jan 9
> 20:36:42 UTC 2010
> bootconsole [earlyprom0] enabled
> ARCH: SUN4U
> Ethernet address: 52:54:00:12:34:56
> Kernel: Using 1 locked TLB entries for main kernel image.
> Remapping the kernel... done.
> OF stdout device is: /pci@1fe,0/pci@1/pci@1,1/ebus@3/su@1fe
> PROM: Built device tree with 32176 bytes of memory.
> Top of RAM: 0x7e80000, Total RAM: 0x7e80000
> Memory hole size: 0MB
> Zone PFN ranges:
>  Normal   0x00000000 -> 0x00003f40
> Movable zone start PFN for each node
> early_node_map[1] active PFN ranges
>    0: 0x00000000 -> 0x00003f40
> On node 0 totalpages: 16192
>  Normal zone: 127 pages used for memmap
>  Normal zone: 0 pages reserved
>  Normal zone: 16065 pages, LIFO batch:3
> Booting Linux...
> Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16065
> Kernel command line: root=/dev/hda1 console=ttyS0 -p debug lpj=100000
> PID hash table entries: 512 (order: -1, 4096 bytes)
> Dentry cache hash table entries: 16384 (order: 4, 131072 bytes)
> Inode-cache hash table entries: 8192 (order: 3, 65536 bytes)
> Memory: 118480k available (1472k kernel code, 488k data, 104k init)
> [fffff80000000000,0000000007e80000]
> SLUB: Genslabs=14, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
> Hierarchical RCU implementation.
> NR_IRQS:255
> clocksource: mult[a0000] shift[16]
> clockevent: mult[19999999] shift[32]
> Console: colour dummy device 80x25
> Calibrating delay loop (skipped) preset value.. 50.00 BogoMIPS (lpj=100000)
> Mount-cache hash table entries: 512
> /pci: PCI IO[1fe02000000] MEM[1ff00000000]
> /pci: SABRE PCI Bus Module ver[0:0]
> PCI: Scanning PBM /pci
> ------------[ cut here ]------------
> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
> Call Trace:
>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>  [000000000053fc88] device_add+0x3c8/0x500
>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>  [00000000005140ec] pci_bus_add_devices+0xec/0x200
>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>  [00000000005427b0] driver_probe_device+0x70/0x160
>  [0000000000542918] __driver_attach+0x78/0xa0
>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>  [00000000005421dc] bus_add_driver+0x9c/0x240
>  [0000000000542c10] driver_register+0x50/0x160
>  [0000000000426a98] do_one_initcall+0x18/0x160
> ---[ end trace 139ce121c98e96c9 ]---
> pci 0000:00:01.1: Error adding bus, continuing
> ------------[ cut here ]------------
> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
> Call Trace:
>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>  [000000000053fc88] device_add+0x3c8/0x500
>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>  [00000000005427b0] driver_probe_device+0x70/0x160
>  [0000000000542918] __driver_attach+0x78/0xa0
>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>  [00000000005421dc] bus_add_driver+0x9c/0x240
>  [0000000000542c10] driver_register+0x50/0x160
>  [0000000000426a98] do_one_initcall+0x18/0x160
>  [00000000005f4274] kernel_init+0xd4/0x140
> ---[ end trace 139ce121c98e96ca ]---
> pci 0000:00:01.0: Error adding bus, continuing
> bio: create slab <bio-0> at 0
> vgaarb: loaded
> Switching to clocksource tick
> io scheduler noop registered (default)
> ------------[ cut here ]------------
> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
> proc_dir_entry 'pci/0000:00' already registered
> Call Trace:
>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>  [00000000004e2948] proc_register+0xe8/0x1c0
>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>  [0000000000426a98] do_one_initcall+0x18/0x160
>  [00000000005f4274] kernel_init+0xd4/0x140
>  [000000000042b130] kernel_thread+0x30/0x60
>  [000000000056a8f8] rest_init+0x18/0x80
> ---[ end trace 139ce121c98e96cb ]---
> ------------[ cut here ]------------
> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
> proc_dir_entry 'pci/0000:00' already registered
> Call Trace:
>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>  [00000000004e2948] proc_register+0xe8/0x1c0
>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>  [0000000000426a98] do_one_initcall+0x18/0x160
>  [00000000005f4274] kernel_init+0xd4/0x140
>  [000000000042b130] kernel_thread+0x30/0x60
>  [000000000056a8f8] rest_init+0x18/0x80
> ---[ end trace 139ce121c98e96cc ]---
> Uniform Multi-Platform E-IDE driver
> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
> PCI: Enabling device: (0000:00:05.0), cmd 301
> cmd64x 0000:00:05.0: unable to enable IDE controller
> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
> CMD64x_IDE 0000:00:05.0: BAR 0: can't reserve I/O region
> [0x1fe02000500-0x1fe02000507]
> cmd64x 0000:00:05.0: can't reserve resources
> CMD64x_IDE: probe of 0000:00:05.0 failed with error -16
> ide-gd driver 1.18
> ide-cd driver 5.00
> mice: PS/2 mouse device common for all mice
> turn off boot console earlyprom0
>
> So PCI probe seems to work. The errors show some bugs with the APB bridges.

You have just committed a bug, writes and reads should list methods in
{b,w,l} order.
See cpu_register_io_memory_fixed where we do map byte access at index 0 etc..
Not shure if it helps solving current issue, but the code should look
like the following:

 static CPUWriteMemoryFunc * const apb_pci_config_writes[] = {
-    &apb_pci_config_writel,
-    &apb_pci_config_writew,
     &apb_pci_config_writeb,
+    &apb_pci_config_writew,
+    &apb_pci_config_writel,
 };

 static CPUReadMemoryFunc * const apb_pci_config_reads[] = {
-    &apb_pci_config_readl,
-    &apb_pci_config_readw,
     &apb_pci_config_readb,
+    &apb_pci_config_readw,
+    &apb_pci_config_readl,
 };

-- 
Kind regards,
Igor V. Kovalenko

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Blue Swirl]

On Mon, Jan 11, 2010 at 10:33 PM, Igor Kovalenko
<igor.v.kovalenko@gmail.com> wrote:
> On Tue, Jan 12, 2010 at 12:29 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Jan 10, 2010 at 6:41 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>>>>>
>>>>>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>>>>>
>>>>>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>>>>>> >>
>>>>>> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>>>>>> >>
>>>>>> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>>>> >>>>
>>>>>> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>>>> >>>>
>>>>>> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>>>>> >>>>>> makes a qemu understandable one out of it.
>>>>>> >>>>>>
>>>>>> >>>>>> 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>
>>>>>> >>>>>
>>>>>> >>>>> Thanks!
>>>>>> >>>>>
>>>>>> >>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>>>>> >>>>> other architectures are converted to x86.  As long as we are adding
>>>>>> >>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>>>>> >>>>> return register and devfn separately, so we don't need to encode/decode
>>>>>> >>>>> back and forth?
>>>>>> >>>>
>>>>>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>>>>>> >>>>
>>>>>> >>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>>>>>> >>>>
>>>>>> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>>>> >>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>>>>> >>>>> etc?
>>>>>> >>>>
>>>>>> >>>> Hm, I figured this is less work :-).
>>>>>> >>>
>>>>>> >>> Let me explain the idea: we have:
>>>>>> >>>
>>>>>> >>>   static void pci_host_config_writel_ioport(void *opaque,
>>>>>> >>>                                             uint32_t addr, uint32_t val)
>>>>>> >>>   {
>>>>>> >>>       PCIHostState *s = opaque;
>>>>>> >>>
>>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>>>>> >>>   val);
>>>>>> >>>       s->config_reg = val;
>>>>>> >>>   }
>>>>>> >>>
>>>>>> >>>   static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>>>>> >>>   addr)
>>>>>> >>>   {
>>>>>> >>>       PCIHostState *s = opaque;
>>>>>> >>>       uint32_t val = s->config_reg;
>>>>>> >>>
>>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>>>>> >>>   val);
>>>>>> >>>       return val;
>>>>>> >>>   }
>>>>>> >>>
>>>>>> >>>   void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>>>>> >>>   {
>>>>>> >>>       register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>>>>> >>>   s);
>>>>>> >>>       register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>>>>> >>>   }
>>>>>> >>>
>>>>>> >>> If instead of a simple config_reg = val we translate to pc format
>>>>>> >>> here, the rest will work. No?
>>>>>> >>
>>>>>> >> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
>>>>>> >
>>>>>> > Right.
>>>>>> >
>>>>>> >> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>>>>>> >>
>>>>>> >> Alex
>>>>>> >
>>>>>> > There's already ugliness with swap/noswap versions in there.  Anything
>>>>>> > that claims to be a proper framework would need to address that as well
>>>>>> > IMO.
>>>>>>
>>>>>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>>>>>> incrementally improving the existing code?
>>>>>
>>>>> Not really, incremental improvements are good.  But what you posted does
>>>>> not seem to clean up most code, what it really does is fixes ppc
>>>>> unin_pci.  My feeling was since there's only one user for now maybe it
>>>>> is better to just have device specific code rather than complicate
>>>>> generic code. Makes sense?
>>>>>
>>>>> We need to see several users before we add yet another level of
>>>>> indirection.  If there is a level of indirection that solves the
>>>>> swap/noswap ugliness as well that would show this is a good abstraction.
>>>>> I think I even know what a good abstraction would be: decode address on
>>>>> write and keep it in decoded form.
>>>>
>>>> I think Sparc64 PBM configuration cycles are also a bit different.
>>>> It's described in UltraSPARC User's Manual 805-0087, p.325.
>>>>
>>>> Currently both QEMU and OpenBIOS just use something common, but as
>>>> soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
>>>> that.
>>>
>>> That time is very close, with latest QEMU and OpenBIOS, PCI probe
>>> starts (with GDB tricks for calibrate_delay, which won't be needed
>>> after %tick_cmpr is fixed):
>>
>> Now I get this:
>> [sparc64] Kernel already loaded
>>
>> PROMLIB: Sun IEEE Boot Prom 'OBP 3.10.24 1999/01/01 01:01'
>> PROMLIB: Root node compatible: sun4u
>> Linux version 2.6.32 (test@host) (gcc version 4.2.4) #3 Sat Jan 9
>> 20:36:42 UTC 2010
>> bootconsole [earlyprom0] enabled
>> ARCH: SUN4U
>> Ethernet address: 52:54:00:12:34:56
>> Kernel: Using 1 locked TLB entries for main kernel image.
>> Remapping the kernel... done.
>> OF stdout device is: /pci@1fe,0/pci@1/pci@1,1/ebus@3/su@1fe
>> PROM: Built device tree with 32176 bytes of memory.
>> Top of RAM: 0x7e80000, Total RAM: 0x7e80000
>> Memory hole size: 0MB
>> Zone PFN ranges:
>>  Normal   0x00000000 -> 0x00003f40
>> Movable zone start PFN for each node
>> early_node_map[1] active PFN ranges
>>    0: 0x00000000 -> 0x00003f40
>> On node 0 totalpages: 16192
>>  Normal zone: 127 pages used for memmap
>>  Normal zone: 0 pages reserved
>>  Normal zone: 16065 pages, LIFO batch:3
>> Booting Linux...
>> Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16065
>> Kernel command line: root=/dev/hda1 console=ttyS0 -p debug lpj=100000
>> PID hash table entries: 512 (order: -1, 4096 bytes)
>> Dentry cache hash table entries: 16384 (order: 4, 131072 bytes)
>> Inode-cache hash table entries: 8192 (order: 3, 65536 bytes)
>> Memory: 118480k available (1472k kernel code, 488k data, 104k init)
>> [fffff80000000000,0000000007e80000]
>> SLUB: Genslabs=14, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
>> Hierarchical RCU implementation.
>> NR_IRQS:255
>> clocksource: mult[a0000] shift[16]
>> clockevent: mult[19999999] shift[32]
>> Console: colour dummy device 80x25
>> Calibrating delay loop (skipped) preset value.. 50.00 BogoMIPS (lpj=100000)
>> Mount-cache hash table entries: 512
>> /pci: PCI IO[1fe02000000] MEM[1ff00000000]
>> /pci: SABRE PCI Bus Module ver[0:0]
>> PCI: Scanning PBM /pci
>> ------------[ cut here ]------------
>> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
>> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
>> Call Trace:
>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>>  [000000000053fc88] device_add+0x3c8/0x500
>>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>>  [00000000005140ec] pci_bus_add_devices+0xec/0x200
>>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>>  [00000000005427b0] driver_probe_device+0x70/0x160
>>  [0000000000542918] __driver_attach+0x78/0xa0
>>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>>  [00000000005421dc] bus_add_driver+0x9c/0x240
>>  [0000000000542c10] driver_register+0x50/0x160
>>  [0000000000426a98] do_one_initcall+0x18/0x160
>> ---[ end trace 139ce121c98e96c9 ]---
>> pci 0000:00:01.1: Error adding bus, continuing
>> ------------[ cut here ]------------
>> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
>> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
>> Call Trace:
>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>>  [000000000053fc88] device_add+0x3c8/0x500
>>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>>  [00000000005427b0] driver_probe_device+0x70/0x160
>>  [0000000000542918] __driver_attach+0x78/0xa0
>>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>>  [00000000005421dc] bus_add_driver+0x9c/0x240
>>  [0000000000542c10] driver_register+0x50/0x160
>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>  [00000000005f4274] kernel_init+0xd4/0x140
>> ---[ end trace 139ce121c98e96ca ]---
>> pci 0000:00:01.0: Error adding bus, continuing
>> bio: create slab <bio-0> at 0
>> vgaarb: loaded
>> Switching to clocksource tick
>> io scheduler noop registered (default)
>> ------------[ cut here ]------------
>> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
>> proc_dir_entry 'pci/0000:00' already registered
>> Call Trace:
>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>  [00000000004e2948] proc_register+0xe8/0x1c0
>>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>  [00000000005f4274] kernel_init+0xd4/0x140
>>  [000000000042b130] kernel_thread+0x30/0x60
>>  [000000000056a8f8] rest_init+0x18/0x80
>> ---[ end trace 139ce121c98e96cb ]---
>> ------------[ cut here ]------------
>> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
>> proc_dir_entry 'pci/0000:00' already registered
>> Call Trace:
>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>  [00000000004e2948] proc_register+0xe8/0x1c0
>>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>  [00000000005f4274] kernel_init+0xd4/0x140
>>  [000000000042b130] kernel_thread+0x30/0x60
>>  [000000000056a8f8] rest_init+0x18/0x80
>> ---[ end trace 139ce121c98e96cc ]---
>> Uniform Multi-Platform E-IDE driver
>> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
>> PCI: Enabling device: (0000:00:05.0), cmd 301
>> cmd64x 0000:00:05.0: unable to enable IDE controller
>> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
>> CMD64x_IDE 0000:00:05.0: BAR 0: can't reserve I/O region
>> [0x1fe02000500-0x1fe02000507]
>> cmd64x 0000:00:05.0: can't reserve resources
>> CMD64x_IDE: probe of 0000:00:05.0 failed with error -16
>> ide-gd driver 1.18
>> ide-cd driver 5.00
>> mice: PS/2 mouse device common for all mice
>> turn off boot console earlyprom0
>>
>> So PCI probe seems to work. The errors show some bugs with the APB bridges.
>
> You have just committed a bug, writes and reads should list methods in
> {b,w,l} order.
> See cpu_register_io_memory_fixed where we do map byte access at index 0 etc..
> Not shure if it helps solving current issue, but the code should look
> like the following:
>
>  static CPUWriteMemoryFunc * const apb_pci_config_writes[] = {
> -    &apb_pci_config_writel,
> -    &apb_pci_config_writew,
>     &apb_pci_config_writeb,
> +    &apb_pci_config_writew,
> +    &apb_pci_config_writel,
>  };
>
>  static CPUReadMemoryFunc * const apb_pci_config_reads[] = {
> -    &apb_pci_config_readl,
> -    &apb_pci_config_readw,
>     &apb_pci_config_readb,
> +    &apb_pci_config_readw,
> +    &apb_pci_config_readl,
>  };

Good catch. I actually did try the correct order, but maybe I edited
the wrong place and it didn't work then.

With your change, CMD646 revision is shown correctly:
cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)
PCI: Enabling device: (0000:00:05.0), cmd 301
cmd64x 0000:00:05.0: unable to enable IDE controller
cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)

[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Blue Swirl]

On Tue, Jan 12, 2010 at 7:29 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Jan 11, 2010 at 10:33 PM, Igor Kovalenko
> <igor.v.kovalenko@gmail.com> wrote:
>> On Tue, Jan 12, 2010 at 12:29 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sun, Jan 10, 2010 at 6:41 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>>>>>>
>>>>>>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>>>>>>
>>>>>>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>>>>>>> >>
>>>>>>> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>>>>>>> >>
>>>>>>> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>>>>> >>>>
>>>>>>> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>>>>> >>>>
>>>>>>> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +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 converter function that takes whatever accessor we have and
>>>>>>> >>>>>> makes a qemu understandable one out of it.
>>>>>>> >>>>>>
>>>>>>> >>>>>> 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>
>>>>>>> >>>>>
>>>>>>> >>>>> Thanks!
>>>>>>> >>>>>
>>>>>>> >>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>>>>>> >>>>> other architectures are converted to x86.  As long as we are adding
>>>>>>> >>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>>>>>> >>>>> return register and devfn separately, so we don't need to encode/decode
>>>>>>> >>>>> back and forth?
>>>>>>> >>>>
>>>>>>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to build on stuff that is there already. That's exactly what happened here.
>>>>>>> >>>>
>>>>>>> >>>> I'm personally not against defining the x86 format as qemu default. That way everyone knows what to deal with. I'm not sure if PCI defines anything that couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. So it seems like a pretty good fit, especially given that all the other code is already in place.
>>>>>>> >>>>
>>>>>>> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>>>>> >>>>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio
>>>>>>> >>>>> etc?
>>>>>>> >>>>
>>>>>>> >>>> Hm, I figured this is less work :-).
>>>>>>> >>>
>>>>>>> >>> Let me explain the idea: we have:
>>>>>>> >>>
>>>>>>> >>>   static void pci_host_config_writel_ioport(void *opaque,
>>>>>>> >>>                                             uint32_t addr, uint32_t val)
>>>>>>> >>>   {
>>>>>>> >>>       PCIHostState *s = opaque;
>>>>>>> >>>
>>>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>>>>>> >>>   val);
>>>>>>> >>>       s->config_reg = val;
>>>>>>> >>>   }
>>>>>>> >>>
>>>>>>> >>>   static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>>>>>> >>>   addr)
>>>>>>> >>>   {
>>>>>>> >>>       PCIHostState *s = opaque;
>>>>>>> >>>       uint32_t val = s->config_reg;
>>>>>>> >>>
>>>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>>>>>> >>>   val);
>>>>>>> >>>       return val;
>>>>>>> >>>   }
>>>>>>> >>>
>>>>>>> >>>   void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>>>>>> >>>   {
>>>>>>> >>>       register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>>>>>> >>>   s);
>>>>>>> >>>       register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>>>>>> >>>   }
>>>>>>> >>>
>>>>>>> >>> If instead of a simple config_reg = val we translate to pc format
>>>>>>> >>> here, the rest will work. No?
>>>>>>> >>
>>>>>>> >> Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ...
>>>>>>> >
>>>>>>> > Right.
>>>>>>> >
>>>>>>> >> Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar.
>>>>>>> >>
>>>>>>> >> Alex
>>>>>>> >
>>>>>>> > There's already ugliness with swap/noswap versions in there.  Anything
>>>>>>> > that claims to be a proper framework would need to address that as well
>>>>>>> > IMO.
>>>>>>>
>>>>>>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>>>>>>> incrementally improving the existing code?
>>>>>>
>>>>>> Not really, incremental improvements are good.  But what you posted does
>>>>>> not seem to clean up most code, what it really does is fixes ppc
>>>>>> unin_pci.  My feeling was since there's only one user for now maybe it
>>>>>> is better to just have device specific code rather than complicate
>>>>>> generic code. Makes sense?
>>>>>>
>>>>>> We need to see several users before we add yet another level of
>>>>>> indirection.  If there is a level of indirection that solves the
>>>>>> swap/noswap ugliness as well that would show this is a good abstraction.
>>>>>> I think I even know what a good abstraction would be: decode address on
>>>>>> write and keep it in decoded form.
>>>>>
>>>>> I think Sparc64 PBM configuration cycles are also a bit different.
>>>>> It's described in UltraSPARC User's Manual 805-0087, p.325.
>>>>>
>>>>> Currently both QEMU and OpenBIOS just use something common, but as
>>>>> soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
>>>>> that.
>>>>
>>>> That time is very close, with latest QEMU and OpenBIOS, PCI probe
>>>> starts (with GDB tricks for calibrate_delay, which won't be needed
>>>> after %tick_cmpr is fixed):
>>>
>>> Now I get this:
>>> [sparc64] Kernel already loaded
>>>
>>> PROMLIB: Sun IEEE Boot Prom 'OBP 3.10.24 1999/01/01 01:01'
>>> PROMLIB: Root node compatible: sun4u
>>> Linux version 2.6.32 (test@host) (gcc version 4.2.4) #3 Sat Jan 9
>>> 20:36:42 UTC 2010
>>> bootconsole [earlyprom0] enabled
>>> ARCH: SUN4U
>>> Ethernet address: 52:54:00:12:34:56
>>> Kernel: Using 1 locked TLB entries for main kernel image.
>>> Remapping the kernel... done.
>>> OF stdout device is: /pci@1fe,0/pci@1/pci@1,1/ebus@3/su@1fe
>>> PROM: Built device tree with 32176 bytes of memory.
>>> Top of RAM: 0x7e80000, Total RAM: 0x7e80000
>>> Memory hole size: 0MB
>>> Zone PFN ranges:
>>>  Normal   0x00000000 -> 0x00003f40
>>> Movable zone start PFN for each node
>>> early_node_map[1] active PFN ranges
>>>    0: 0x00000000 -> 0x00003f40
>>> On node 0 totalpages: 16192
>>>  Normal zone: 127 pages used for memmap
>>>  Normal zone: 0 pages reserved
>>>  Normal zone: 16065 pages, LIFO batch:3
>>> Booting Linux...
>>> Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16065
>>> Kernel command line: root=/dev/hda1 console=ttyS0 -p debug lpj=100000
>>> PID hash table entries: 512 (order: -1, 4096 bytes)
>>> Dentry cache hash table entries: 16384 (order: 4, 131072 bytes)
>>> Inode-cache hash table entries: 8192 (order: 3, 65536 bytes)
>>> Memory: 118480k available (1472k kernel code, 488k data, 104k init)
>>> [fffff80000000000,0000000007e80000]
>>> SLUB: Genslabs=14, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
>>> Hierarchical RCU implementation.
>>> NR_IRQS:255
>>> clocksource: mult[a0000] shift[16]
>>> clockevent: mult[19999999] shift[32]
>>> Console: colour dummy device 80x25
>>> Calibrating delay loop (skipped) preset value.. 50.00 BogoMIPS (lpj=100000)
>>> Mount-cache hash table entries: 512
>>> /pci: PCI IO[1fe02000000] MEM[1ff00000000]
>>> /pci: SABRE PCI Bus Module ver[0:0]
>>> PCI: Scanning PBM /pci
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
>>> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>>>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>>>  [000000000053fc88] device_add+0x3c8/0x500
>>>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>>>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>>>  [00000000005140ec] pci_bus_add_devices+0xec/0x200
>>>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>>>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>>>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>>>  [00000000005427b0] driver_probe_device+0x70/0x160
>>>  [0000000000542918] __driver_attach+0x78/0xa0
>>>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>>>  [00000000005421dc] bus_add_driver+0x9c/0x240
>>>  [0000000000542c10] driver_register+0x50/0x160
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>> ---[ end trace 139ce121c98e96c9 ]---
>>> pci 0000:00:01.1: Error adding bus, continuing
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
>>> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>>>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>>>  [000000000053fc88] device_add+0x3c8/0x500
>>>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>>>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>>>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>>>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>>>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>>>  [00000000005427b0] driver_probe_device+0x70/0x160
>>>  [0000000000542918] __driver_attach+0x78/0xa0
>>>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>>>  [00000000005421dc] bus_add_driver+0x9c/0x240
>>>  [0000000000542c10] driver_register+0x50/0x160
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>>  [00000000005f4274] kernel_init+0xd4/0x140
>>> ---[ end trace 139ce121c98e96ca ]---
>>> pci 0000:00:01.0: Error adding bus, continuing
>>> bio: create slab <bio-0> at 0
>>> vgaarb: loaded
>>> Switching to clocksource tick
>>> io scheduler noop registered (default)
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
>>> proc_dir_entry 'pci/0000:00' already registered
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e2948] proc_register+0xe8/0x1c0
>>>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>>>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>>>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>>  [00000000005f4274] kernel_init+0xd4/0x140
>>>  [000000000042b130] kernel_thread+0x30/0x60
>>>  [000000000056a8f8] rest_init+0x18/0x80
>>> ---[ end trace 139ce121c98e96cb ]---
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
>>> proc_dir_entry 'pci/0000:00' already registered
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e2948] proc_register+0xe8/0x1c0
>>>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>>>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>>>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>>  [00000000005f4274] kernel_init+0xd4/0x140
>>>  [000000000042b130] kernel_thread+0x30/0x60
>>>  [000000000056a8f8] rest_init+0x18/0x80
>>> ---[ end trace 139ce121c98e96cc ]---
>>> Uniform Multi-Platform E-IDE driver
>>> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
>>> PCI: Enabling device: (0000:00:05.0), cmd 301
>>> cmd64x 0000:00:05.0: unable to enable IDE controller
>>> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
>>> CMD64x_IDE 0000:00:05.0: BAR 0: can't reserve I/O region
>>> [0x1fe02000500-0x1fe02000507]
>>> cmd64x 0000:00:05.0: can't reserve resources
>>> CMD64x_IDE: probe of 0000:00:05.0 failed with error -16
>>> ide-gd driver 1.18
>>> ide-cd driver 5.00
>>> mice: PS/2 mouse device common for all mice
>>> turn off boot console earlyprom0
>>>
>>> So PCI probe seems to work. The errors show some bugs with the APB bridges.
>>
>> You have just committed a bug, writes and reads should list methods in
>> {b,w,l} order.
>> See cpu_register_io_memory_fixed where we do map byte access at index 0 etc..
>> Not shure if it helps solving current issue, but the code should look
>> like the following:
>>
>>  static CPUWriteMemoryFunc * const apb_pci_config_writes[] = {
>> -    &apb_pci_config_writel,
>> -    &apb_pci_config_writew,
>>     &apb_pci_config_writeb,
>> +    &apb_pci_config_writew,
>> +    &apb_pci_config_writel,
>>  };
>>
>>  static CPUReadMemoryFunc * const apb_pci_config_reads[] = {
>> -    &apb_pci_config_readl,
>> -    &apb_pci_config_readw,
>>     &apb_pci_config_readb,
>> +    &apb_pci_config_readw,
>> +    &apb_pci_config_readl,
>>  };
>
> Good catch. I actually did try the correct order, but maybe I edited
> the wrong place and it didn't work then.
>
> With your change, CMD646 revision is shown correctly:
> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)
> PCI: Enabling device: (0000:00:05.0), cmd 301
> cmd64x 0000:00:05.0: unable to enable IDE controller
> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)

Maybe there is some other bug with the revision field. Just for fun, I
enabled virtio for Sparc64 but Linux sees the zero revision field as 1
(used as ABI version):

cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)
CMD64x_IDE 0000:00:05.0: BAR 0: can't reserve I/O region
[0x1fe02000500-0x1fe02000507]
cmd64x 0000:00:05.0: can't reserve resources
CMD64x_IDE: probe of 0000:00:05.0 failed with error -16
ide-gd driver 1.18
ne2k-pci.c:v1.03 9/22/2003 D. Becker/P. Gortmaker
PCI: Enabling device: (0000:00:04.0), cmd 301
eth0: RealTek RTL-8029 found at 0x1fe02000400, IRQ 1, 52:54:00:12:34:56.
pcnet32.c:v1.35 21.Apr.2008 tsbogend@alpha.franken.de
mice: PS/2 mouse device common for all mice
virtio_pci: expected ABI version 0, got 1

However, RTL-8029 gets the MAC address read out correctly.