[Qemu-devel] [PATCH 0/3] e500: creating CCSR region and registering bar0

[Qemu-devel] [PATCH 0/3] e500: creating CCSR region and registering bar0

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

First Patch Fixes the wrong array offset issue with serial initialization.

The CCSR memory region is exported to pci device. The MSI interrupt
generation is the main reason to export the CCSR region to PCI device.
This put the requirement to move mpic under CCSR region, but logically
all devices should be under CCSR.
  
So second patch creates the CCSR region and places all emulated devices
under ccsr region.
 
PCI Root complex have TYPE-1 configuration header while PCI endpoint
have type-0 configuration header. The type-1 configuration header have
a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci
address space to CCSR address space.

The third patch maps the BAR0 to ccsr region.

*** BLURB HERE ***

Bharat Bhushan (3):
  e500: Fix serial initialization
  e500: Adding CCSR memory region
  Adding BAR0 for e500 PCI controller

 hw/ppc/e500-ccsr.h |   13 +++++++
 hw/ppc/e500.c      |   97 ++++++++++++++++++++++++++++++++++++++-------------
 hw/ppce500_pci.c   |   22 +++++++++++-
 3 files changed, 106 insertions(+), 26 deletions(-)
 create mode 100644 hw/ppc/e500-ccsr.h

[Qemu-devel] [PATCH 1/3] e500: Fix serial initialization

[Bharat Bhushan]

it was wrongly using serial_hds[0] instead of serial_hds[1]

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 hw/ppc/e500.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index d23f9b2..1949c81 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -496,7 +496,7 @@ void ppce500_init(PPCE500Params *params)
     if (serial_hds[1]) {
         serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
                        0, mpic[12+26], 399193,
-                       serial_hds[0], DEVICE_BIG_ENDIAN);
+                       serial_hds[1], DEVICE_BIG_ENDIAN);
     }
 
     /* General Utility device */
-- 
1.7.0.4

[Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Bharat Bhushan]

All devices are also placed under CCSR memory region.
The CCSR memory region is exported to pci device. The MSI interrupt
generation is the main reason to export the CCSR region to PCI device.
This put the requirement to move mpic under CCSR region, but logically
all devices should be under CCSR. So this patch places all emulated
devices under ccsr region.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 hw/ppc/e500.c |   61 +++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 1949c81..b3e6a1e 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -46,14 +46,16 @@
 /* TODO: parameterize */
 #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
 #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
-#define MPC8544_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + 0x40000ULL)
-#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4500ULL)
-#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4600ULL)
-#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + 0x8000ULL)
+#define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
+#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
+#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
+#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
+#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
+                                    MPC8544_PCI_REGS_OFFSET)
 #define MPC8544_PCI_REGS_SIZE      0x1000ULL
 #define MPC8544_PCI_IO             0xE1000000ULL
 #define MPC8544_PCI_IOLEN          0x10000ULL
-#define MPC8544_UTIL_BASE          (MPC8544_CCSRBAR_BASE + 0xe0000ULL)
+#define MPC8544_UTIL_OFFSET        0xe0000ULL
 #define MPC8544_SPIN_BASE          0xEF000000ULL
 
 struct boot_info
@@ -268,13 +270,12 @@ static int ppce500_load_device_tree(CPUPPCState *env,
     /* XXX should contain a reasonable value */
     qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
 
-    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc,
-             MPC8544_MPIC_REGS_BASE - MPC8544_CCSRBAR_BASE);
+    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
     qemu_devtree_add_subnode(fdt, mpic);
     qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
     qemu_devtree_setprop_string(fdt, mpic, "compatible", "chrp,open-pic");
-    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_BASE -
-                               MPC8544_CCSRBAR_BASE, 0x40000);
+    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
+                               0x40000);
     qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
     qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
     mpic_ph = qemu_devtree_alloc_phandle(fdt);
@@ -287,17 +288,16 @@ static int ppce500_load_device_tree(CPUPPCState *env,
      * device it finds in the dt as serial output device. And we generate
      * devices in reverse order to the dt.
      */
-    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE,
+    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
                      soc, mpic, "serial1", 1, false);
-    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_BASE - MPC8544_CCSRBAR_BASE,
+    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
                      soc, mpic, "serial0", 0, true);
 
     snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,
-             MPC8544_UTIL_BASE - MPC8544_CCSRBAR_BASE);
+             MPC8544_UTIL_OFFSET);
     qemu_devtree_add_subnode(fdt, gutil);
     qemu_devtree_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
-    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_BASE -
-                               MPC8544_CCSRBAR_BASE, 0x1000);
+    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
     qemu_devtree_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
 
     snprintf(pci, sizeof(pci), "/pci@%llx", MPC8544_PCI_REGS_BASE);
@@ -423,6 +423,8 @@ void ppce500_init(PPCE500Params *params)
     qemu_irq **irqs, *mpic;
     DeviceState *dev;
     CPUPPCState *firstenv = NULL;
+    MemoryRegion *ccsr;
+    SysBusDevice *s;
 
     /* Setup CPUs */
     if (params->cpu_model == NULL) {
@@ -451,7 +453,8 @@ void ppce500_init(PPCE500Params *params)
         irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
         irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
         env->spr[SPR_BOOKE_PIR] = env->cpu_index = i;
-        env->mpic_cpu_base = MPC8544_MPIC_REGS_BASE + 0x20000;
+        env->mpic_cpu_base = MPC8544_CCSRBAR_BASE +
+                              MPC8544_MPIC_REGS_OFFSET + 0x20000;
 
         ppc_booke_timers_init(env, 400000000, PPC_TIMER_E500);
 
@@ -478,8 +481,12 @@ void ppce500_init(PPCE500Params *params)
     vmstate_register_ram_global(ram);
     memory_region_add_subregion(address_space_mem, 0, ram);
 
+    ccsr = g_malloc0(sizeof(MemoryRegion));
+    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
+    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
+
     /* MPIC */
-    mpic = mpic_init(address_space_mem, MPC8544_MPIC_REGS_BASE,
+    mpic = mpic_init(ccsr, MPC8544_MPIC_REGS_OFFSET,
                      smp_cpus, irqs, NULL);
 
     if (!mpic) {
@@ -488,25 +495,33 @@ void ppce500_init(PPCE500Params *params)
 
     /* Serial */
     if (serial_hds[0]) {
-        serial_mm_init(address_space_mem, MPC8544_SERIAL0_REGS_BASE,
+        serial_mm_init(ccsr, MPC8544_SERIAL0_REGS_OFFSET,
                        0, mpic[12+26], 399193,
                        serial_hds[0], DEVICE_BIG_ENDIAN);
     }
 
     if (serial_hds[1]) {
-        serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
+        serial_mm_init(ccsr, MPC8544_SERIAL1_REGS_OFFSET,
                        0, mpic[12+26], 399193,
                        serial_hds[1], DEVICE_BIG_ENDIAN);
     }
 
     /* General Utility device */
-    sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
+    dev = qdev_create(NULL, "mpc8544-guts");
+    qdev_init_nofail(dev);
+    s = sysbus_from_qdev(dev);
+    memory_region_add_subregion(ccsr, MPC8544_UTIL_OFFSET, s->mmio[0].memory);
 
     /* PCI */
-    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
-                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
-                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
-                                NULL);
+    dev = qdev_create(NULL, "e500-pcihost");
+    qdev_init_nofail(dev);
+    s = sysbus_from_qdev(dev);
+    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
+    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
+    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
+    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
+    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
+
     pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
     if (!pci_bus)
         printf("couldn't create PCI controller!\n");
-- 
1.7.0.4

[Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller

[Bharat Bhushan]

PCI Root complex have TYPE-1 configuration header while PCI endpoint
have type-0 configuration header. The type-1 configuration header have
a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci
address space to CCSR address space. This can used for 2 purposes: 1)
for MSI interrupt generation 2) Allow CCSR registers access when configured
as PCI endpoint, which I am not sure is a use case with QEMU-KVM guest.

What I observed is that when guest read the size of BAR0 of host controller
configuration header (TYPE1 header) then it always reads it as 0. When
looking into the QEMU hw/ppce500_pci.c, I do not find the PCI controller
device registering BAR0. I do not find any other controller also doing so
may they do not use BAR0.

There are two issues when BAR0 is not there (which I can think of):
1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) and
when reading the size of BAR0, it should give size as per real h/w.

2) Do we need this BAR0 inbound address translation?
        When BAR0 is of non-zero size then it will be configured for PCI
address space to local address(CCSR) space translation on inbound access.
The primary use case is for MSI interrupt generation. The device is
configured with a address offsets in PCI address space, which will be
translated to MSI interrupt generation MPIC registers. Currently I do
not understand the MSI interrupt generation mechanism in QEMU and also
IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines.
But this BAR0 will be used when using MSI on e500.

I can see one more issue, There are ATMUs emulated in hw/ppce500_pci.c,
but i do not see these being used for address translation.
So far that works because pci address space and local address space are 1:1
mapped. BAR0 inbound translation + ATMU translation will complete the address
translation of inbound traffic.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 hw/ppc/e500-ccsr.h |   13 +++++++++++++
 hw/ppc/e500.c      |   38 +++++++++++++++++++++++++++++++++++---
 hw/ppce500_pci.c   |   22 +++++++++++++++++++++-
 3 files changed, 69 insertions(+), 4 deletions(-)
 create mode 100644 hw/ppc/e500-ccsr.h

diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
new file mode 100644
index 0000000..867bdb0
--- /dev/null
+++ b/hw/ppc/e500-ccsr.h
@@ -0,0 +1,13 @@
+#ifndef E500_CCSR_H
+#define E500_CCSR_H 
+
+#include "../sysbus.h"
+
+typedef struct PPCE500CCSRState {
+    SysBusDevice parent;
+    MemoryRegion ccsr_space;
+} PPCE500CCSRState;
+
+#define TYPE_CCSR "e500-ccsr"
+#define CCSR(obj) OBJECT_CHECK(PPCE500CCSRState, (obj), TYPE_CCSR)
+#endif
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b3e6a1e..ffcacd5 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -17,6 +17,7 @@
 #include "config.h"
 #include "qemu-common.h"
 #include "e500.h"
+#include "e500-ccsr.h"
 #include "net.h"
 #include "hw/hw.h"
 #include "hw/pc.h"
@@ -36,7 +37,7 @@
 
 #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
 #define UIMAGE_LOAD_BASE           0
-#define DTC_LOAD_PAD               0x1800000
+#define DTC_LOAD_PAD               0x500000
 #define DTC_PAD_MASK               0xFFFFF
 #define INITRD_LOAD_PAD            0x2000000
 #define INITRD_PAD_MASK            0xFFFFFF
@@ -424,6 +425,7 @@ void ppce500_init(PPCE500Params *params)
     DeviceState *dev;
     CPUPPCState *firstenv = NULL;
     MemoryRegion *ccsr;
+    PPCE500CCSRState *pci_ccsr;
     SysBusDevice *s;
 
     /* Setup CPUs */
@@ -481,8 +483,13 @@ void ppce500_init(PPCE500Params *params)
     vmstate_register_ram_global(ram);
     memory_region_add_subregion(address_space_mem, 0, ram);
 
-    ccsr = g_malloc0(sizeof(MemoryRegion));
-    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
+    dev = qdev_create(NULL, "e500-ccsr");
+    object_property_add_child(qdev_get_machine(), "e500-ccsr",
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+    pci_ccsr = CCSR(dev);
+    ccsr = &pci_ccsr->ccsr_space;
+    memory_region_init(ccsr, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
     memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
 
     /* MPIC */
@@ -594,3 +601,28 @@ void ppce500_init(PPCE500Params *params)
         kvmppc_init();
     }
 }
+
+static int e500_ccsr_initfn(SysBusDevice *dev)
+{
+    return 0;
+}
+
+static void e500_ccsr_class_init(ObjectClass *klass, void *data)
+{
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    k->init = e500_ccsr_initfn;
+}
+
+static const TypeInfo e500_ccsr_info = {
+    .name          = TYPE_CCSR,
+    .parent        =  TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PPCE500CCSRState),
+    .class_init    = e500_ccsr_class_init,
+};
+
+static void e500_ccsr_register_types(void)
+{
+    type_register_static(&e500_ccsr_info);
+}
+
+type_init(e500_ccsr_register_types)
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 92b1dc0..869ed92 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -15,6 +15,7 @@
  */
 
 #include "hw.h"
+#include "hw/ppc/e500-ccsr.h"
 #include "pci.h"
 #include "pci_host.h"
 #include "bswap.h"
@@ -89,6 +90,12 @@ struct PPCE500PCIState {
     MemoryRegion iomem;
 };
 
+struct PPCE500PCI_Bridge_State {
+    PCIDevice p;
+    MemoryRegion bar0;
+};
+
+typedef struct PPCE500PCI_Bridge_State PPCE500PCI_Bridge_State;
 typedef struct PPCE500PCIState PPCE500PCIState;
 
 static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
@@ -307,6 +314,18 @@ static const VMStateDescription vmstate_ppce500_pci = {
 
 #include "exec-memory.h"
 
+static int e500_pcihost_bridge_initfn(PCIDevice *d)
+{
+    PPCE500PCI_Bridge_State *b = DO_UPCAST(PPCE500PCI_Bridge_State, p, d);
+    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(), "/e500-ccsr"));
+
+    b->bar0 = ccsr->ccsr_space;
+    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
+    memory_region_init_alias(&b->bar0, "e500-pci-bar0", &ccsr->ccsr_space,
+                             0, int128_get64(ccsr->ccsr_space.size));
+    return 0;
+}
+
 static int e500_pcihost_initfn(SysBusDevice *dev)
 {
     PCIHostState *h;
@@ -350,6 +369,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
+    k->init = e500_pcihost_bridge_initfn;
     k->vendor_id = PCI_VENDOR_ID_FREESCALE;
     k->device_id = PCI_DEVICE_ID_MPC8533E;
     k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
@@ -359,7 +379,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
 static const TypeInfo e500_host_bridge_info = {
     .name          = "e500-host-bridge",
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PCIDevice),
+    .instance_size = sizeof(PPCE500PCI_Bridge_State),
     .class_init    = e500_host_bridge_class_init,
 };
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 1/3] e500: Fix serial initialization

[Andreas Färber]

Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> it was wrongly using serial_hds[0] instead of serial_hds[1]
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Andreas Färber]

Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> All devices are also placed under CCSR memory region.
> The CCSR memory region is exported to pci device. The MSI interrupt
> generation is the main reason to export the CCSR region to PCI device.
> This put the requirement to move mpic under CCSR region, but logically
> all devices should be under CCSR. So this patch places all emulated
> devices under ccsr region.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>  hw/ppc/e500.c |   61 +++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 1949c81..b3e6a1e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -46,14 +46,16 @@
>  /* TODO: parameterize */
>  #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>  #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> -#define MPC8544_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + 0x40000ULL)
> -#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4500ULL)
> -#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4600ULL)
> -#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + 0x8000ULL)
> +#define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
> +#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
> +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
> +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
> +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_PCI_REGS_OFFSET)
>  #define MPC8544_PCI_REGS_SIZE      0x1000ULL
>  #define MPC8544_PCI_IO             0xE1000000ULL
>  #define MPC8544_PCI_IOLEN          0x10000ULL
> -#define MPC8544_UTIL_BASE          (MPC8544_CCSRBAR_BASE + 0xe0000ULL)
> +#define MPC8544_UTIL_OFFSET        0xe0000ULL
>  #define MPC8544_SPIN_BASE          0xEF000000ULL
>  
>  struct boot_info
> @@ -268,13 +270,12 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>      /* XXX should contain a reasonable value */
>      qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
>  
> -    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc,
> -             MPC8544_MPIC_REGS_BASE - MPC8544_CCSRBAR_BASE);
> +    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
>      qemu_devtree_add_subnode(fdt, mpic);
>      qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
>      qemu_devtree_setprop_string(fdt, mpic, "compatible", "chrp,open-pic");
> -    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_BASE -
> -                               MPC8544_CCSRBAR_BASE, 0x40000);
> +    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
> +                               0x40000);
>      qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
>      qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
>      mpic_ph = qemu_devtree_alloc_phandle(fdt);
> @@ -287,17 +288,16 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>       * device it finds in the dt as serial output device. And we generate
>       * devices in reverse order to the dt.
>       */
> -    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE,
> +    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
>                       soc, mpic, "serial1", 1, false);
> -    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_BASE - MPC8544_CCSRBAR_BASE,
> +    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
>                       soc, mpic, "serial0", 0, true);
>  
>      snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,
> -             MPC8544_UTIL_BASE - MPC8544_CCSRBAR_BASE);
> +             MPC8544_UTIL_OFFSET);
>      qemu_devtree_add_subnode(fdt, gutil);
>      qemu_devtree_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
> -    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_BASE -
> -                               MPC8544_CCSRBAR_BASE, 0x1000);
> +    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
>      qemu_devtree_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
>  
>      snprintf(pci, sizeof(pci), "/pci@%llx", MPC8544_PCI_REGS_BASE);
> @@ -423,6 +423,8 @@ void ppce500_init(PPCE500Params *params)
>      qemu_irq **irqs, *mpic;
>      DeviceState *dev;
>      CPUPPCState *firstenv = NULL;
> +    MemoryRegion *ccsr;
> +    SysBusDevice *s;
>  
>      /* Setup CPUs */
>      if (params->cpu_model == NULL) {
> @@ -451,7 +453,8 @@ void ppce500_init(PPCE500Params *params)
>          irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>          irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>          env->spr[SPR_BOOKE_PIR] = env->cpu_index = i;
> -        env->mpic_cpu_base = MPC8544_MPIC_REGS_BASE + 0x20000;
> +        env->mpic_cpu_base = MPC8544_CCSRBAR_BASE +
> +                              MPC8544_MPIC_REGS_OFFSET + 0x20000;
>  
>          ppc_booke_timers_init(env, 400000000, PPC_TIMER_E500);
>  
> @@ -478,8 +481,12 @@ void ppce500_init(PPCE500Params *params)
>      vmstate_register_ram_global(ram);
>      memory_region_add_subregion(address_space_mem, 0, ram);
>  
> +    ccsr = g_malloc0(sizeof(MemoryRegion));
> +    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
> +    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
> +
>      /* MPIC */
> -    mpic = mpic_init(address_space_mem, MPC8544_MPIC_REGS_BASE,
> +    mpic = mpic_init(ccsr, MPC8544_MPIC_REGS_OFFSET,
>                       smp_cpus, irqs, NULL);
>  
>      if (!mpic) {
> @@ -488,25 +495,33 @@ void ppce500_init(PPCE500Params *params)
>  
>      /* Serial */
>      if (serial_hds[0]) {
> -        serial_mm_init(address_space_mem, MPC8544_SERIAL0_REGS_BASE,
> +        serial_mm_init(ccsr, MPC8544_SERIAL0_REGS_OFFSET,
>                         0, mpic[12+26], 399193,
>                         serial_hds[0], DEVICE_BIG_ENDIAN);
>      }
>  
>      if (serial_hds[1]) {
> -        serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
> +        serial_mm_init(ccsr, MPC8544_SERIAL1_REGS_OFFSET,
>                         0, mpic[12+26], 399193,
>                         serial_hds[1], DEVICE_BIG_ENDIAN);
>      }
>  
>      /* General Utility device */
> -    sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
> +    dev = qdev_create(NULL, "mpc8544-guts");
> +    qdev_init_nofail(dev);
> +    s = sysbus_from_qdev(dev);

s = SYS_BUS_DEVICE(dev);

> +    memory_region_add_subregion(ccsr, MPC8544_UTIL_OFFSET, s->mmio[0].memory);

Hmm ...

>  
>      /* PCI */
> -    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
> -                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
> -                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
> -                                NULL);
> +    dev = qdev_create(NULL, "e500-pcihost");
> +    qdev_init_nofail(dev);
> +    s = sysbus_from_qdev(dev);

(dito)

> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);

... I wonder if fiddling with SysBus MMIO is a good idea.
s->mmio[0].addr is not getting assigned this way, which is checked as
condition for deleting the subregion. But sysbus_mmio_map() only adds to
/ deletes from get_system_memory().
The alternative would be using a custom field rather than the
SysBus-internal one. Avi/Alex?

> +
>      pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>      if (!pci_bus)
>          printf("couldn't create PCI controller!\n");
> 

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Andreas Färber]

Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 1949c81..b3e6a1e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
[...]
> @@ -478,8 +481,12 @@ void ppce500_init(PPCE500Params *params)
>      vmstate_register_ram_global(ram);
>      memory_region_add_subregion(address_space_mem, 0, ram);
>  
> +    ccsr = g_malloc0(sizeof(MemoryRegion));
> +    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);

"e500-ccsr" :) Got fixed in 3/3 only.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller

[Andreas Färber]

Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> PCI Root complex have TYPE-1 configuration header while PCI endpoint
> have type-0 configuration header. The type-1 configuration header have
> a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci
> address space to CCSR address space. This can used for 2 purposes: 1)
> for MSI interrupt generation 2) Allow CCSR registers access when configured
> as PCI endpoint, which I am not sure is a use case with QEMU-KVM guest.
> 
> What I observed is that when guest read the size of BAR0 of host controller
> configuration header (TYPE1 header) then it always reads it as 0. When
> looking into the QEMU hw/ppce500_pci.c, I do not find the PCI controller
> device registering BAR0. I do not find any other controller also doing so
> may they do not use BAR0.
> 
> There are two issues when BAR0 is not there (which I can think of):
> 1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) and
> when reading the size of BAR0, it should give size as per real h/w.
> 
> 2) Do we need this BAR0 inbound address translation?
>         When BAR0 is of non-zero size then it will be configured for PCI
> address space to local address(CCSR) space translation on inbound access.
> The primary use case is for MSI interrupt generation. The device is
> configured with a address offsets in PCI address space, which will be
> translated to MSI interrupt generation MPIC registers. Currently I do
> not understand the MSI interrupt generation mechanism in QEMU and also
> IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines.
> But this BAR0 will be used when using MSI on e500.
> 
> I can see one more issue, There are ATMUs emulated in hw/ppce500_pci.c,
> but i do not see these being used for address translation.
> So far that works because pci address space and local address space are 1:1
> mapped. BAR0 inbound translation + ATMU translation will complete the address
> translation of inbound traffic.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

This is starting to look really good modeling-wise. :) Some minor issues
below.

> ---
>  hw/ppc/e500-ccsr.h |   13 +++++++++++++
>  hw/ppc/e500.c      |   38 +++++++++++++++++++++++++++++++++++---
>  hw/ppce500_pci.c   |   22 +++++++++++++++++++++-
>  3 files changed, 69 insertions(+), 4 deletions(-)
>  create mode 100644 hw/ppc/e500-ccsr.h
> 
> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
> new file mode 100644
> index 0000000..867bdb0
> --- /dev/null
> +++ b/hw/ppc/e500-ccsr.h
> @@ -0,0 +1,13 @@
> +#ifndef E500_CCSR_H
> +#define E500_CCSR_H 
> +
> +#include "../sysbus.h"
> +
> +typedef struct PPCE500CCSRState {
> +    SysBusDevice parent;

I would suggest to insert a while line here to separate the parent from
the other field(s).

> +    MemoryRegion ccsr_space;
> +} PPCE500CCSRState;
> +
> +#define TYPE_CCSR "e500-ccsr"
> +#define CCSR(obj) OBJECT_CHECK(PPCE500CCSRState, (obj), TYPE_CCSR)

While line please, since #endif corresponds to whole file.

> +#endif
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index b3e6a1e..ffcacd5 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -17,6 +17,7 @@
>  #include "config.h"
>  #include "qemu-common.h"
>  #include "e500.h"
> +#include "e500-ccsr.h"
>  #include "net.h"
>  #include "hw/hw.h"
>  #include "hw/pc.h"

> @@ -36,7 +37,7 @@
>  
>  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
>  #define UIMAGE_LOAD_BASE           0
> -#define DTC_LOAD_PAD               0x1800000
> +#define DTC_LOAD_PAD               0x500000
>  #define DTC_PAD_MASK               0xFFFFF
>  #define INITRD_LOAD_PAD            0x2000000
>  #define INITRD_PAD_MASK            0xFFFFFF

Was this change intentional? I don't see it being used here, and commit
message doesn't seem to mention it.

> @@ -424,6 +425,7 @@ void ppce500_init(PPCE500Params *params)
>      DeviceState *dev;
>      CPUPPCState *firstenv = NULL;
>      MemoryRegion *ccsr;
> +    PPCE500CCSRState *pci_ccsr;
>      SysBusDevice *s;
>  
>      /* Setup CPUs */
> @@ -481,8 +483,13 @@ void ppce500_init(PPCE500Params *params)
>      vmstate_register_ram_global(ram);
>      memory_region_add_subregion(address_space_mem, 0, ram);
>  
> -    ccsr = g_malloc0(sizeof(MemoryRegion));
> -    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
> +    dev = qdev_create(NULL, "e500-ccsr");
> +    object_property_add_child(qdev_get_machine(), "e500-ccsr",
> +                              OBJECT(dev), NULL);

Note: According to our QOM ABI guarantees this means that we cannot
place any differently typed property of that name onto that machine, but
I believe this is okay with that naming.

(My plan for the future would be an "e500" SoC object in e500.c either
with a "ccsr" child or exposing the MemoryRegion directly through its
struct.)

> +    qdev_init_nofail(dev);
> +    pci_ccsr = CCSR(dev);
> +    ccsr = &pci_ccsr->ccsr_space;
> +    memory_region_init(ccsr, "e500-ccsr", MPC8544_CCSRBAR_SIZE);

Since ccsr_space is in the SysBusDevice now, it would nice to move the
initialization into the new initfn below, e.g.:

>      memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
>  
>      /* MPIC */
> @@ -594,3 +601,28 @@ void ppce500_init(PPCE500Params *params)
>          kvmppc_init();
>      }
>  }
> +
> +static int e500_ccsr_initfn(SysBusDevice *dev)
> +{

PPCE500CCSRState *ccsr = CCSR(dev);
memory_region_init(&ccsr->ccsr_space, "e500-ccsr", MPC8544_CCSRBAR_SIZE);

> +    return 0;
> +}
> +
> +static void e500_ccsr_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    k->init = e500_ccsr_initfn;
> +}
> +
> +static const TypeInfo e500_ccsr_info = {
> +    .name          = TYPE_CCSR,
> +    .parent        =  TYPE_SYS_BUS_DEVICE,

Duplicate white space after assignment.

> +    .instance_size = sizeof(PPCE500CCSRState),
> +    .class_init    = e500_ccsr_class_init,
> +};
> +
> +static void e500_ccsr_register_types(void)
> +{
> +    type_register_static(&e500_ccsr_info);
> +}
> +
> +type_init(e500_ccsr_register_types)

Usually the naming follows the file name, i.e. e500_register_types. Then
we can nicely add non-CCSR types there later.

> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..869ed92 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include "hw.h"
> +#include "hw/ppc/e500-ccsr.h"
>  #include "pci.h"
>  #include "pci_host.h"
>  #include "bswap.h"
> @@ -89,6 +90,12 @@ struct PPCE500PCIState {
>      MemoryRegion iomem;
>  };
>  
> +struct PPCE500PCI_Bridge_State {
> +    PCIDevice p;

Suggest readable "parent" and optionally separating white line.

> +    MemoryRegion bar0;
> +};
> +
> +typedef struct PPCE500PCI_Bridge_State PPCE500PCI_Bridge_State;

Coding Style asks for CamelCase in both cases, i.e. PPCE500PCIBridgeState.

>  typedef struct PPCE500PCIState PPCE500PCIState;
>  
>  static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
> @@ -307,6 +314,18 @@ static const VMStateDescription vmstate_ppce500_pci = {
>  
>  #include "exec-memory.h"
>  
> +static int e500_pcihost_bridge_initfn(PCIDevice *d)
> +{
> +    PPCE500PCI_Bridge_State *b = DO_UPCAST(PPCE500PCI_Bridge_State, p, d);

Please introduce your own QOM cast macro for this type, like you did
CCSR(). Then the field names become irrelevant.

> +    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(), "/e500-ccsr"));
> +
> +    b->bar0 = ccsr->ccsr_space;
> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
> +    memory_region_init_alias(&b->bar0, "e500-pci-bar0", &ccsr->ccsr_space,
> +                             0, int128_get64(ccsr->ccsr_space.size));

This looks fishy: First you value-assign the CCSR MemoryRegion to bar0,
then you newly init it as an alias.
Not sure how that fits with bar registration, whether that may need to
be reordered to after the initialization and the copy-initialization
dropped?

Regards,
Andreas

> +    return 0;
> +}
> +
>  static int e500_pcihost_initfn(SysBusDevice *dev)
>  {
>      PCIHostState *h;
> @@ -350,6 +369,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> +    k->init = e500_pcihost_bridge_initfn;
>      k->vendor_id = PCI_VENDOR_ID_FREESCALE;
>      k->device_id = PCI_DEVICE_ID_MPC8533E;
>      k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
> @@ -359,7 +379,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
>  static const TypeInfo e500_host_bridge_info = {
>      .name          = "e500-host-bridge",
>      .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PCIDevice),
> +    .instance_size = sizeof(PPCE500PCI_Bridge_State),
>      .class_init    = e500_host_bridge_class_init,
>  };
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] e500: Fix serial initialization

[Alexander Graf]

On 08.10.2012, at 18:46, Bharat Bhushan wrote:

> it was wrongly using serial_hds[0] instead of serial_hds[1]
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

Thanks, applied to ppc-next.

Alex

> ---
> hw/ppc/e500.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index d23f9b2..1949c81 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -496,7 +496,7 @@ void ppce500_init(PPCE500Params *params)
>     if (serial_hds[1]) {
>         serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
>                        0, mpic[12+26], 399193,
> -                       serial_hds[0], DEVICE_BIG_ENDIAN);
> +                       serial_hds[1], DEVICE_BIG_ENDIAN);
>     }
> 
>     /* General Utility device */
> -- 
> 1.7.0.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Alexander Graf]

On 08.10.2012, at 19:21, Andreas Färber wrote:

> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>> All devices are also placed under CCSR memory region.
>> The CCSR memory region is exported to pci device. The MSI interrupt
>> generation is the main reason to export the CCSR region to PCI device.
>> This put the requirement to move mpic under CCSR region, but logically
>> all devices should be under CCSR. So this patch places all emulated
>> devices under ccsr region.
>> 
>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>> ---
>> hw/ppc/e500.c |   61 +++++++++++++++++++++++++++++++++++---------------------
>> 1 files changed, 38 insertions(+), 23 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 1949c81..b3e6a1e 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -46,14 +46,16 @@
>> /* TODO: parameterize */
>> #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>> #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> -#define MPC8544_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + 0x40000ULL)
>> -#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4500ULL)
>> -#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4600ULL)
>> -#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + 0x8000ULL)
>> +#define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
>> +#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
>> +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
>> +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
>> +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
>> +                                    MPC8544_PCI_REGS_OFFSET)
>> #define MPC8544_PCI_REGS_SIZE      0x1000ULL
>> #define MPC8544_PCI_IO             0xE1000000ULL
>> #define MPC8544_PCI_IOLEN          0x10000ULL
>> -#define MPC8544_UTIL_BASE          (MPC8544_CCSRBAR_BASE + 0xe0000ULL)
>> +#define MPC8544_UTIL_OFFSET        0xe0000ULL
>> #define MPC8544_SPIN_BASE          0xEF000000ULL
>> 
>> struct boot_info
>> @@ -268,13 +270,12 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>>     /* XXX should contain a reasonable value */
>>     qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
>> 
>> -    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc,
>> -             MPC8544_MPIC_REGS_BASE - MPC8544_CCSRBAR_BASE);
>> +    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
>>     qemu_devtree_add_subnode(fdt, mpic);
>>     qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
>>     qemu_devtree_setprop_string(fdt, mpic, "compatible", "chrp,open-pic");
>> -    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_BASE -
>> -                               MPC8544_CCSRBAR_BASE, 0x40000);
>> +    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
>> +                               0x40000);
>>     qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
>>     qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
>>     mpic_ph = qemu_devtree_alloc_phandle(fdt);
>> @@ -287,17 +288,16 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>>      * device it finds in the dt as serial output device. And we generate
>>      * devices in reverse order to the dt.
>>      */
>> -    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE,
>> +    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
>>                      soc, mpic, "serial1", 1, false);
>> -    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_BASE - MPC8544_CCSRBAR_BASE,
>> +    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
>>                      soc, mpic, "serial0", 0, true);
>> 
>>     snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,
>> -             MPC8544_UTIL_BASE - MPC8544_CCSRBAR_BASE);
>> +             MPC8544_UTIL_OFFSET);
>>     qemu_devtree_add_subnode(fdt, gutil);
>>     qemu_devtree_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
>> -    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_BASE -
>> -                               MPC8544_CCSRBAR_BASE, 0x1000);
>> +    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
>>     qemu_devtree_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
>> 
>>     snprintf(pci, sizeof(pci), "/pci@%llx", MPC8544_PCI_REGS_BASE);
>> @@ -423,6 +423,8 @@ void ppce500_init(PPCE500Params *params)
>>     qemu_irq **irqs, *mpic;
>>     DeviceState *dev;
>>     CPUPPCState *firstenv = NULL;
>> +    MemoryRegion *ccsr;
>> +    SysBusDevice *s;
>> 
>>     /* Setup CPUs */
>>     if (params->cpu_model == NULL) {
>> @@ -451,7 +453,8 @@ void ppce500_init(PPCE500Params *params)
>>         irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>>         irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>>         env->spr[SPR_BOOKE_PIR] = env->cpu_index = i;
>> -        env->mpic_cpu_base = MPC8544_MPIC_REGS_BASE + 0x20000;
>> +        env->mpic_cpu_base = MPC8544_CCSRBAR_BASE +
>> +                              MPC8544_MPIC_REGS_OFFSET + 0x20000;
>> 
>>         ppc_booke_timers_init(env, 400000000, PPC_TIMER_E500);
>> 
>> @@ -478,8 +481,12 @@ void ppce500_init(PPCE500Params *params)
>>     vmstate_register_ram_global(ram);
>>     memory_region_add_subregion(address_space_mem, 0, ram);
>> 
>> +    ccsr = g_malloc0(sizeof(MemoryRegion));
>> +    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
>> +    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
>> +
>>     /* MPIC */
>> -    mpic = mpic_init(address_space_mem, MPC8544_MPIC_REGS_BASE,
>> +    mpic = mpic_init(ccsr, MPC8544_MPIC_REGS_OFFSET,
>>                      smp_cpus, irqs, NULL);
>> 
>>     if (!mpic) {
>> @@ -488,25 +495,33 @@ void ppce500_init(PPCE500Params *params)
>> 
>>     /* Serial */
>>     if (serial_hds[0]) {
>> -        serial_mm_init(address_space_mem, MPC8544_SERIAL0_REGS_BASE,
>> +        serial_mm_init(ccsr, MPC8544_SERIAL0_REGS_OFFSET,
>>                        0, mpic[12+26], 399193,
>>                        serial_hds[0], DEVICE_BIG_ENDIAN);
>>     }
>> 
>>     if (serial_hds[1]) {
>> -        serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
>> +        serial_mm_init(ccsr, MPC8544_SERIAL1_REGS_OFFSET,
>>                        0, mpic[12+26], 399193,
>>                        serial_hds[1], DEVICE_BIG_ENDIAN);
>>     }
>> 
>>     /* General Utility device */
>> -    sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
>> +    dev = qdev_create(NULL, "mpc8544-guts");
>> +    qdev_init_nofail(dev);
>> +    s = sysbus_from_qdev(dev);
> 
> s = SYS_BUS_DEVICE(dev);

Mind to explain why? The rest of the code uses the above helper :).

> 
>> +    memory_region_add_subregion(ccsr, MPC8544_UTIL_OFFSET, s->mmio[0].memory);
> 
> Hmm ...
> 
>> 
>>     /* PCI */
>> -    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
>> -                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
>> -                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
>> -                                NULL);
>> +    dev = qdev_create(NULL, "e500-pcihost");
>> +    qdev_init_nofail(dev);
>> +    s = sysbus_from_qdev(dev);
> 
> (dito)
> 
>> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
>> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
>> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
>> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
>> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
> 
> ... I wonder if fiddling with SysBus MMIO is a good idea.
> s->mmio[0].addr is not getting assigned this way, which is checked as
> condition for deleting the subregion. But sysbus_mmio_map() only adds to
> / deletes from get_system_memory().
> The alternative would be using a custom field rather than the
> SysBus-internal one. Avi/Alex?

I honestly don't mind either way. We could

  a) add a new sysbus helper for maps in memory region containers
  b) not use sysbus at all
  c) do it this way

Anything else wouldn't improve the situation. I really don't care which way we go. To me, c) is fine.


Alex

Re: [Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller

[Alexander Graf]

On 08.10.2012, at 20:00, Andreas Färber wrote:

> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>> PCI Root complex have TYPE-1 configuration header while PCI endpoint
>> have type-0 configuration header. The type-1 configuration header have
>> a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci
>> address space to CCSR address space. This can used for 2 purposes: 1)
>> for MSI interrupt generation 2) Allow CCSR registers access when configured
>> as PCI endpoint, which I am not sure is a use case with QEMU-KVM guest.
>> 
>> What I observed is that when guest read the size of BAR0 of host controller
>> configuration header (TYPE1 header) then it always reads it as 0. When
>> looking into the QEMU hw/ppce500_pci.c, I do not find the PCI controller
>> device registering BAR0. I do not find any other controller also doing so
>> may they do not use BAR0.
>> 
>> There are two issues when BAR0 is not there (which I can think of):
>> 1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) and
>> when reading the size of BAR0, it should give size as per real h/w.
>> 
>> 2) Do we need this BAR0 inbound address translation?
>>        When BAR0 is of non-zero size then it will be configured for PCI
>> address space to local address(CCSR) space translation on inbound access.
>> The primary use case is for MSI interrupt generation. The device is
>> configured with a address offsets in PCI address space, which will be
>> translated to MSI interrupt generation MPIC registers. Currently I do
>> not understand the MSI interrupt generation mechanism in QEMU and also
>> IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines.
>> But this BAR0 will be used when using MSI on e500.
>> 
>> I can see one more issue, There are ATMUs emulated in hw/ppce500_pci.c,
>> but i do not see these being used for address translation.
>> So far that works because pci address space and local address space are 1:1
>> mapped. BAR0 inbound translation + ATMU translation will complete the address
>> translation of inbound traffic.
>> 
>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> 
> This is starting to look really good modeling-wise. :) Some minor issues
> below.
> 
>> ---
>> hw/ppc/e500-ccsr.h |   13 +++++++++++++
>> hw/ppc/e500.c      |   38 +++++++++++++++++++++++++++++++++++---
>> hw/ppce500_pci.c   |   22 +++++++++++++++++++++-
>> 3 files changed, 69 insertions(+), 4 deletions(-)
>> create mode 100644 hw/ppc/e500-ccsr.h
>> 
>> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
>> new file mode 100644
>> index 0000000..867bdb0
>> --- /dev/null
>> +++ b/hw/ppc/e500-ccsr.h
>> @@ -0,0 +1,13 @@
>> +#ifndef E500_CCSR_H
>> +#define E500_CCSR_H 
>> +
>> +#include "../sysbus.h"
>> +
>> +typedef struct PPCE500CCSRState {
>> +    SysBusDevice parent;
> 
> I would suggest to insert a while line here to separate the parent from
> the other field(s).

Blank line? White line?

> 
>> +    MemoryRegion ccsr_space;
>> +} PPCE500CCSRState;
>> +
>> +#define TYPE_CCSR "e500-ccsr"
>> +#define CCSR(obj) OBJECT_CHECK(PPCE500CCSRState, (obj), TYPE_CCSR)
> 
> While line please, since #endif corresponds to whole file.
> 
>> +#endif
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index b3e6a1e..ffcacd5 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -17,6 +17,7 @@
>> #include "config.h"
>> #include "qemu-common.h"
>> #include "e500.h"
>> +#include "e500-ccsr.h"
>> #include "net.h"
>> #include "hw/hw.h"
>> #include "hw/pc.h"
> 
>> @@ -36,7 +37,7 @@
>> 
>> #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
>> #define UIMAGE_LOAD_BASE           0
>> -#define DTC_LOAD_PAD               0x1800000
>> +#define DTC_LOAD_PAD               0x500000
>> #define DTC_PAD_MASK               0xFFFFF
>> #define INITRD_LOAD_PAD            0x2000000
>> #define INITRD_PAD_MASK            0xFFFFFF
> 
> Was this change intentional? I don't see it being used here, and commit
> message doesn't seem to mention it.

I'd assume he tried to work around a bug I fixed in between. But this change definitely is not intentional.


Alex

Re: [Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller

[Scott Wood]

On 10/08/2012 02:04:43 PM, Alexander Graf wrote:
> 
> On 08.10.2012, at 20:00, Andreas Färber wrote:
> 
> > Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> >> #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> >> #define UIMAGE_LOAD_BASE           0
> >> -#define DTC_LOAD_PAD               0x1800000
> >> +#define DTC_LOAD_PAD               0x500000
> >> #define DTC_PAD_MASK               0xFFFFF
> >> #define INITRD_LOAD_PAD            0x2000000
> >> #define INITRD_PAD_MASK            0xFFFFFF
> >
> > Was this change intentional? I don't see it being used here, and  
> commit
> > message doesn't seem to mention it.
> 
> I'd assume he tried to work around a bug I fixed in between. But this  
> change definitely is not intentional.

It looks like an accidental revert of  
http://patchwork.ozlabs.org/patch/179475/

-Scott

Re: [Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, October 09, 2012 1:54 AM
> To: Alexander Graf
> Cc: Andreas Färber; Bhushan Bharat-R65777; Avi Kivity; qemu-ppc@nongnu.org;
> qemu-devel@nongnu.org; Bhushan Bharat-R65777
> Subject: Re: [Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller
> 
> On 10/08/2012 02:04:43 PM, Alexander Graf wrote:
> >
> > On 08.10.2012, at 20:00, Andreas Färber wrote:
> >
> > > Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> > >> #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> > >> #define UIMAGE_LOAD_BASE           0
> > >> -#define DTC_LOAD_PAD               0x1800000
> > >> +#define DTC_LOAD_PAD               0x500000
> > >> #define DTC_PAD_MASK               0xFFFFF
> > >> #define INITRD_LOAD_PAD            0x2000000
> > >> #define INITRD_PAD_MASK            0xFFFFFF
> > >
> > > Was this change intentional? I don't see it being used here, and
> > commit
> > > message doesn't seem to mention it.
> >
> > I'd assume he tried to work around a bug I fixed in between. But this
> > change definitely is not intentional.
> 
> It looks like an accidental revert of
> http://patchwork.ozlabs.org/patch/179475/

Oops, it was not intended to be sent as patch. It was not accidental because without this revert I was not able to boot my guest.

Thanks
-Bharat

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Avi Kivity]

On 10/08/2012 07:21 PM, Andreas Färber wrote:
> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>> All devices are also placed under CCSR memory region.
>> The CCSR memory region is exported to pci device. The MSI interrupt
>> generation is the main reason to export the CCSR region to PCI device.
>> This put the requirement to move mpic under CCSR region, but logically
>> all devices should be under CCSR. So this patch places all emulated
>> devices under ccsr region.
>> 
>> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
>> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
>> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
>> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
>> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
> 
> ... I wonder if fiddling with SysBus MMIO is a good idea.
> s->mmio[0].addr is not getting assigned this way, which is checked as
> condition for deleting the subregion. But sysbus_mmio_map() only adds to
> / deletes from get_system_memory().
> The alternative would be using a custom field rather than the
> SysBus-internal one. Avi/Alex?

IMO yes.  Or not use sysbus at all.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Tuesday, October 09, 2012 2:35 PM
> To: Andreas Färber
> Cc: Bhushan Bharat-R65777; agraf@suse.de; qemu-ppc@nongnu.org; qemu-
> devel@nongnu.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> 
> On 10/08/2012 07:21 PM, Andreas Färber wrote:
> > Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> >> All devices are also placed under CCSR memory region.
> >> The CCSR memory region is exported to pci device. The MSI interrupt
> >> generation is the main reason to export the CCSR region to PCI device.
> >> This put the requirement to move mpic under CCSR region, but
> >> logically all devices should be under CCSR. So this patch places all
> >> emulated devices under ccsr region.
> >>
> >> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> >> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
> >> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
> >> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
> >> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET,
> >> + s->mmio[0].memory);
> >
> > ... I wonder if fiddling with SysBus MMIO is a good idea.
> > s->mmio[0].addr is not getting assigned this way, which is checked as
> > condition for deleting the subregion. But sysbus_mmio_map() only adds
> > to / deletes from get_system_memory().
> > The alternative would be using a custom field rather than the
> > SysBus-internal one. Avi/Alex?
> 
> IMO yes.  Or not use sysbus at all.

What about adding a API:
void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
                           MemoryRegion *mr)
{
    assert(n >= 0 && n < dev->num_mmio);

    if (dev->mmio[n].addr == addr) {
        /* ??? region already mapped here.  */
        return;
    }
    if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
        /* Unregister previous mapping.  */
        memory_region_del_subregion(mr, dev->mmio[n].memory);
    }
    dev->mmio[n].addr = addr;
    memory_region_add_subregion(mr, addr, dev->mmio[n].memory);
}

Thanks
-Bharat

> 
> 
> --
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Avi Kivity]

On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
> 
> What about adding a API:
> void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
>                            MemoryRegion *mr)
> {
>     assert(n >= 0 && n < dev->num_mmio);
> 
>     if (dev->mmio[n].addr == addr) {
>         /* ??? region already mapped here.  */
>         return;
>     }
>     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
>         /* Unregister previous mapping.  */
>         memory_region_del_subregion(mr, dev->mmio[n].memory);
>     }
>     dev->mmio[n].addr = addr;
>     memory_region_add_subregion(mr, addr, dev->mmio[n].memory);
> }
> 

I think you can just use sysbus_mmio_get_region().  There are plenty of
other users, so there's precedent.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Tuesday, October 09, 2012 10:24 PM
> To: Bhushan Bharat-R65777
> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> 
> On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
> >
> > What about adding a API:
> > void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
> >                            MemoryRegion *mr) {
> >     assert(n >= 0 && n < dev->num_mmio);
> >
> >     if (dev->mmio[n].addr == addr) {
> >         /* ??? region already mapped here.  */
> >         return;
> >     }
> >     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
> >         /* Unregister previous mapping.  */
> >         memory_region_del_subregion(mr, dev->mmio[n].memory);
> >     }
> >     dev->mmio[n].addr = addr;
> >     memory_region_add_subregion(mr, addr, dev->mmio[n].memory); }
> >
> 
> I think you can just use sysbus_mmio_get_region().  There are plenty of other
> users, so there's precedent.

You mean something like this : memory_region_add_subregion(mr, addr, sysbus_mmio_get_region(dev, 0);

Ok, but this will still not resolve the issue of not setting the "dev->mmio[n].addr", no ?

Thanks
-Bharat

> 
> 
> --
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Avi Kivity]

On 10/09/2012 06:57 PM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Tuesday, October 09, 2012 10:24 PM
>> To: Bhushan Bharat-R65777
>> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
>> 
>> On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
>> >
>> > What about adding a API:
>> > void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
>> >                            MemoryRegion *mr) {
>> >     assert(n >= 0 && n < dev->num_mmio);
>> >
>> >     if (dev->mmio[n].addr == addr) {
>> >         /* ??? region already mapped here.  */
>> >         return;
>> >     }
>> >     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
>> >         /* Unregister previous mapping.  */
>> >         memory_region_del_subregion(mr, dev->mmio[n].memory);
>> >     }
>> >     dev->mmio[n].addr = addr;
>> >     memory_region_add_subregion(mr, addr, dev->mmio[n].memory); }
>> >
>> 
>> I think you can just use sysbus_mmio_get_region().  There are plenty of other
>> users, so there's precedent.
> 
> You mean something like this : memory_region_add_subregion(mr, addr, sysbus_mmio_get_region(dev, 0);
> 
> Ok, but this will still not resolve the issue of not setting the "dev->mmio[n].addr", no ?

Correct.  But there are 20 uses already, so it can't matter much.  If
someone wants to fix them, they can write a new API and do a sweep.

But really, sysbus just needs to go away.  It's pointless to give it
more and more features.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Tuesday, October 09, 2012 10:31 PM
> To: Bhushan Bharat-R65777
> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> 
> On 10/09/2012 06:57 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Avi Kivity [mailto:avi@redhat.com]
> >> Sent: Tuesday, October 09, 2012 10:24 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org;
> >> qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> >>
> >> On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
> >> >
> >> > What about adding a API:
> >> > void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t
> addr,
> >> >                            MemoryRegion *mr) {
> >> >     assert(n >= 0 && n < dev->num_mmio);
> >> >
> >> >     if (dev->mmio[n].addr == addr) {
> >> >         /* ??? region already mapped here.  */
> >> >         return;
> >> >     }
> >> >     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
> >> >         /* Unregister previous mapping.  */
> >> >         memory_region_del_subregion(mr, dev->mmio[n].memory);
> >> >     }
> >> >     dev->mmio[n].addr = addr;
> >> >     memory_region_add_subregion(mr, addr, dev->mmio[n].memory); }
> >> >
> >>
> >> I think you can just use sysbus_mmio_get_region().  There are plenty
> >> of other users, so there's precedent.
> >
> > You mean something like this : memory_region_add_subregion(mr, addr,
> > sysbus_mmio_get_region(dev, 0);
> >
> > Ok, but this will still not resolve the issue of not setting the "dev-
> >mmio[n].addr", no ?
> 
> Correct.  But there are 20 uses already, so it can't matter much.  If someone
> wants to fix them, they can write a new API and do a sweep.
> 
> But really, sysbus just needs to go away.  It's pointless to give it more and
> more features.

Ok

Thanks
-Bharat

> 
> 
> --
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Andreas Färber]

Am 08.10.2012 20:58, schrieb Alexander Graf:
> 
> On 08.10.2012, at 19:21, Andreas Färber wrote:
> 
>> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>>> +    s = sysbus_from_qdev(dev);
>>
>> s = SYS_BUS_DEVICE(dev);
> 
> Mind to explain why? The rest of the code uses the above helper :).

SYS_BUS_DEVICE() is the QOM cast macro, relying on binary struct layout.

sysbus_from_qdev() is an old qdev helper, relying on container_of() and
field naming.

I'm lobbying for using the new constructs in new code. You can argue of
course that SysBus will go away some day so it doesn't matter much here.
But we had issues with PCIHostState not adhering to QOM layout
requirements (e.g., pSeries), so using the cast consistently helps to
avoid/detect such issues.

>>> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
>>
>> ... I wonder if fiddling with SysBus MMIO is a good idea.
>> s->mmio[0].addr is not getting assigned this way, which is checked as
>> condition for deleting the subregion. But sysbus_mmio_map() only adds to
>> / deletes from get_system_memory().
>> The alternative would be using a custom field rather than the
>> SysBus-internal one. Avi/Alex?
> 
> I honestly don't mind either way. We could
> 
>   a) add a new sysbus helper for maps in memory region containers
>   b) not use sysbus at all
>   c) do it this way
> 
> Anything else wouldn't improve the situation. I really don't care which way we go. To me, c) is fine.

b) is not that easily possible for the PHB, since we still haven't seen
a new round of i440fx patches (PCIHostState is based on SysBusDevice, so
all PHBs would need to get converted at once).

I'd also be in favor of c) with Avi's suggestion of using the accessor
function to obtain the MemoryRegion* (data caging).

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg