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

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

[Bharat Bhushan]

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

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 First 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.i

The second patch maps the BAR0 to ccsr region.

Bharat Bhushan (2):
  e500: Adding CCSR memory region
  Adding BAR0 for e500 PCI controller

 hw/ppc/e500.c    |   52 ++++++++++++++++++++++++++++++++++++++--------------
 hw/ppce500_pci.c |   13 +++++++++++++
 2 files changed, 51 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/2] 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 |   51 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 5bab340..197411d 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -46,14 +46,23 @@
 /* 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_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + \
+                                    MPC8544_MPIC_REGS_OFFSET)
+#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
+#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
+                                    MPC8544_SERIAL0_REGS_OFFSET)
+#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
+#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
+                                    MPC8544_SERIAL1_REGS_OFFSET)
+#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_UTIL_BASE          (MPC8544_CCSRBAR_BASE + MPC8544_UTIL_OFFSET)
 #define MPC8544_SPIN_BASE          0xEF000000ULL
 
 struct boot_info
@@ -419,6 +428,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) {
@@ -474,8 +485,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) {
@@ -484,25 +499,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[0], DEVICE_BIG_ENDIAN);
+                       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 2/2] 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.

This patch solves this problem.

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.c    |    1 +
 hw/ppce500_pci.c |   13 +++++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 197411d..c7ae2b6 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params)
 
     /* PCI */
     dev = qdev_create(NULL, "e500-pcihost");
+    qdev_prop_set_ptr(dev, "bar0_region", ccsr);
     qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 92b1dc0..16e4af2 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -87,6 +87,7 @@ struct PPCE500PCIState {
     /* mmio maps */
     MemoryRegion container;
     MemoryRegion iomem;
+    void *bar0;
 };
 
 typedef struct PPCE500PCIState PPCE500PCIState;
@@ -315,6 +316,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     int i;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *address_space_io = get_system_io();
+    PCIDevice *pdev;
+    MemoryRegion bar0;
 
     h = PCI_HOST_BRIDGE(dev);
     s = PPC_E500_PCI_HOST_BRIDGE(dev);
@@ -342,6 +345,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
     sysbus_init_mmio(dev, &s->container);
 
+    bar0 = *(MemoryRegion *)s->bar0;
+    pdev = pci_find_device(b, 0, 0);
+    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0);
+
     return 0;
 }
 
@@ -363,6 +370,11 @@ static const TypeInfo e500_host_bridge_info = {
     .class_init    = e500_host_bridge_class_init,
 };
 
+static Property pci_host_dev_info[] = {
+    DEFINE_PROP_PTR("bar0_region", PPCE500PCIState, bar0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void e500_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -370,6 +382,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
 
     k->init = e500_pcihost_initfn;
     dc->vmsd = &vmstate_ppce500_pci;
+    dc->props = pci_host_dev_info;
 }
 
 static const TypeInfo e500_pcihost_info = {
-- 
1.7.0.4

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

[Alexander Graf]

On 03.10.2012, at 13:49, Bharat Bhushan wrote:

> 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 |   51 +++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 5bab340..197411d 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -46,14 +46,23 @@
> /* 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_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_MPIC_REGS_OFFSET)
> +#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
> +#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_SERIAL0_REGS_OFFSET)
> +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
> +#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_SERIAL1_REGS_OFFSET)
> +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
> +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_PCI_REGS_OFFSET)

You don't use any of the bases anymore, right? Please remove the respective #define's.

The rest of the patch looks very nice.


Alex

> #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_UTIL_BASE          (MPC8544_CCSRBAR_BASE + MPC8544_UTIL_OFFSET)
> #define MPC8544_SPIN_BASE          0xEF000000ULL
> 
> struct boot_info
> @@ -419,6 +428,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) {
> @@ -474,8 +485,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) {
> @@ -484,25 +499,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[0], DEVICE_BIG_ENDIAN);
> +                       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
> 
> 

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

[Alexander Graf]

On 03.10.2012, at 13:50, Bharat Bhushan wrote:

> 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.
> 
> This patch solves this problem.
> 
> 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.c    |    1 +
> hw/ppce500_pci.c |   13 +++++++++++++
> 2 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 197411d..c7ae2b6 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params)
> 
>     /* PCI */
>     dev = qdev_create(NULL, "e500-pcihost");
> +    qdev_prop_set_ptr(dev, "bar0_region", ccsr);
>     qdev_init_nofail(dev);
>     s = sysbus_from_qdev(dev);
>     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..16e4af2 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>     /* mmio maps */
>     MemoryRegion container;
>     MemoryRegion iomem;
> +    void *bar0;
> };
> 
> typedef struct PPCE500PCIState PPCE500PCIState;
> @@ -315,6 +316,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     int i;
>     MemoryRegion *address_space_mem = get_system_memory();
>     MemoryRegion *address_space_io = get_system_io();
> +    PCIDevice *pdev;
> +    MemoryRegion bar0;
> 
>     h = PCI_HOST_BRIDGE(dev);
>     s = PPC_E500_PCI_HOST_BRIDGE(dev);
> @@ -342,6 +345,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
>     sysbus_init_mmio(dev, &s->container);
> 
> +    bar0 = *(MemoryRegion *)s->bar0;
> +    pdev = pci_find_device(b, 0, 0);
> +    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0);

Any reason you can't just pass in s->bar0 here? Though I'm not sure whether we have to do something special to get a memory region alias.

Avi, any ideas? We want the same memory region mapped twice. Once at a fixed address, once as BAR0 of the PCI host controller.


Alex

> +
>     return 0;
> }
> 
> @@ -363,6 +370,11 @@ static const TypeInfo e500_host_bridge_info = {
>     .class_init    = e500_host_bridge_class_init,
> };
> 
> +static Property pci_host_dev_info[] = {
> +    DEFINE_PROP_PTR("bar0_region", PPCE500PCIState, bar0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void e500_pcihost_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -370,6 +382,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
> 
>     k->init = e500_pcihost_initfn;
>     dc->vmsd = &vmstate_ppce500_pci;
> +    dc->props = pci_host_dev_info;
> }
> 
> static const TypeInfo e500_pcihost_info = {
> -- 
> 1.7.0.4
> 
> 

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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, October 03, 2012 5:39 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/2] e500: Adding CCSR memory region
> 
> 
> On 03.10.2012, at 13:49, Bharat Bhushan wrote:
> 
> > 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 |   51 +++++++++++++++++++++++++++++++++++++--------------
> > 1 files changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 5bab340..197411d
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -46,14 +46,23 @@
> > /* 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_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + \
> > +                                    MPC8544_MPIC_REGS_OFFSET) #define
> > +MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL #define
> > +MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
> > +                                    MPC8544_SERIAL0_REGS_OFFSET)
> > +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL #define
> > +MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
> > +                                    MPC8544_SERIAL1_REGS_OFFSET)
> > +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
> > +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
> > +                                    MPC8544_PCI_REGS_OFFSET)
> 
> You don't use any of the bases anymore, right? Please remove the respective
> #define's.

Alex, some of these bases are used in device tree creation code.

Thanks
-Bharat
> 
> The rest of the patch looks very nice.
> 
> 
> Alex
> 
> > #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_UTIL_BASE          (MPC8544_CCSRBAR_BASE +
> MPC8544_UTIL_OFFSET)
> > #define MPC8544_SPIN_BASE          0xEF000000ULL
> >
> > struct boot_info
> > @@ -419,6 +428,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) {
> > @@ -474,8 +485,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) {
> > @@ -484,25 +499,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[0], DEVICE_BIG_ENDIAN);
> > +                       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
> >
> >
> 

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

[Alexander Graf]

On 03.10.2012, at 14:16, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, October 03, 2012 5:39 PM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; Bhushan Bharat-R65777
>> Subject: Re: [PATCH 1/2] e500: Adding CCSR memory region
>> 
>> 
>> On 03.10.2012, at 13:49, Bharat Bhushan wrote:
>> 
>>> 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 |   51 +++++++++++++++++++++++++++++++++++++--------------
>>> 1 files changed, 37 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 5bab340..197411d
>>> 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -46,14 +46,23 @@
>>> /* 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_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + \
>>> +                                    MPC8544_MPIC_REGS_OFFSET) #define
>>> +MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL #define
>>> +MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
>>> +                                    MPC8544_SERIAL0_REGS_OFFSET)
>>> +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL #define
>>> +MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
>>> +                                    MPC8544_SERIAL1_REGS_OFFSET)
>>> +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
>>> +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
>>> +                                    MPC8544_PCI_REGS_OFFSET)
>> 
>> You don't use any of the bases anymore, right? Please remove the respective
>> #define's.
> 
> Alex, some of these bases are used in device tree creation code.

But they're used by subtracting the ccsr base from them again, no? :) We should just use the offsets there directly.


Alex

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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, October 03, 2012 5:41 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel qemu-devel; qemu-ppc@nongnu.org List; Bhushan Bharat-R65777; Avi
> Kivity
> Subject: Re: [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> 
> On 03.10.2012, at 13:50, Bharat Bhushan wrote:
> 
> > 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.
> >
> > This patch solves this problem.
> >
> > 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.c    |    1 +
> > hw/ppce500_pci.c |   13 +++++++++++++
> > 2 files changed, 14 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 197411d..c7ae2b6
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params)
> >
> >     /* PCI */
> >     dev = qdev_create(NULL, "e500-pcihost");
> > +    qdev_prop_set_ptr(dev, "bar0_region", ccsr);
> >     qdev_init_nofail(dev);
> >     s = sysbus_from_qdev(dev);
> >     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
> > a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2 100644
> > --- a/hw/ppce500_pci.c
> > +++ b/hw/ppce500_pci.c
> > @@ -87,6 +87,7 @@ struct PPCE500PCIState {
> >     /* mmio maps */
> >     MemoryRegion container;
> >     MemoryRegion iomem;
> > +    void *bar0;
> > };
> >
> > typedef struct PPCE500PCIState PPCE500PCIState; @@ -315,6 +316,8 @@
> > static int e500_pcihost_initfn(SysBusDevice *dev)
> >     int i;
> >     MemoryRegion *address_space_mem = get_system_memory();
> >     MemoryRegion *address_space_io = get_system_io();
> > +    PCIDevice *pdev;
> > +    MemoryRegion bar0;
> >
> >     h = PCI_HOST_BRIDGE(dev);
> >     s = PPC_E500_PCI_HOST_BRIDGE(dev); @@ -342,6 +345,10 @@ static int
> > e500_pcihost_initfn(SysBusDevice *dev)
> >     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
> >     sysbus_init_mmio(dev, &s->container);
> >
> > +    bar0 = *(MemoryRegion *)s->bar0;
> > +    pdev = pci_find_device(b, 0, 0);
> > +    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0);
> 
> Any reason you can't just pass in s->bar0 here? Though I'm not sure whether we
> have to do something special to get a memory region alias.

Yes I think this should work:
         pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)s->bar0);

Thanks
-Bharat

> 
> Avi, any ideas? We want the same memory region mapped twice. Once at a fixed
> address, once as BAR0 of the PCI host controller.
> 
> 
> Alex
> 
> > +
> >     return 0;
> > }
> >
> > @@ -363,6 +370,11 @@ static const TypeInfo e500_host_bridge_info = {
> >     .class_init    = e500_host_bridge_class_init,
> > };
> >
> > +static Property pci_host_dev_info[] = {
> > +    DEFINE_PROP_PTR("bar0_region", PPCE500PCIState, bar0),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > static void e500_pcihost_class_init(ObjectClass *klass, void *data) {
> >     DeviceClass *dc = DEVICE_CLASS(klass); @@ -370,6 +382,7 @@ static
> > void e500_pcihost_class_init(ObjectClass *klass, void *data)
> >
> >     k->init = e500_pcihost_initfn;
> >     dc->vmsd = &vmstate_ppce500_pci;
> > +    dc->props = pci_host_dev_info;
> > }
> >
> > static const TypeInfo e500_pcihost_info = {
> > --
> > 1.7.0.4
> >
> >
> 

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

[Avi Kivity]

On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
>      sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..16e4af2 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>      /* mmio maps */
>      MemoryRegion container;
>      MemoryRegion iomem;
> +    void *bar0;
>  };

void *?  Why?

>  
>  typedef struct PPCE500PCIState PPCE500PCIState;
> @@ -315,6 +316,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>      int i;
>      MemoryRegion *address_space_mem = get_system_memory();
>      MemoryRegion *address_space_io = get_system_io();
> +    PCIDevice *pdev;
> +    MemoryRegion bar0;
>  
>      h = PCI_HOST_BRIDGE(dev);
>      s = PPC_E500_PCI_HOST_BRIDGE(dev);
> @@ -342,6 +345,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>      memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
>      sysbus_init_mmio(dev, &s->container);
>  
> +    bar0 = *(MemoryRegion *)s->bar0;
> +    pdev = pci_find_device(b, 0, 0);
> +    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0);
> +

This is broken, you're registering an object on the stack which will be
freed as soon as the function returns.

Just pass &s->bar0 as Alex suggests.

However this is best done from the pci device's initialization function,
not here (the MemoryRegion should be a member of the pci device as well).

>      return 0;
>  }
>  


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

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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Thursday, October 04, 2012 6:02 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan Bharat-
> R65777
> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
> >      sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
> > a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2 100644
> > --- a/hw/ppce500_pci.c
> > +++ b/hw/ppce500_pci.c
> > @@ -87,6 +87,7 @@ struct PPCE500PCIState {
> >      /* mmio maps */
> >      MemoryRegion container;
> >      MemoryRegion iomem;
> > +    void *bar0;
> >  };
> 
> void *?  Why?

Passing parameter via qdev is either possible by value or by void *. That's why I used void *.

> 
> >
> >  typedef struct PPCE500PCIState PPCE500PCIState; @@ -315,6 +316,8 @@
> > static int e500_pcihost_initfn(SysBusDevice *dev)
> >      int i;
> >      MemoryRegion *address_space_mem = get_system_memory();
> >      MemoryRegion *address_space_io = get_system_io();
> > +    PCIDevice *pdev;
> > +    MemoryRegion bar0;
> >
> >      h = PCI_HOST_BRIDGE(dev);
> >      s = PPC_E500_PCI_HOST_BRIDGE(dev); @@ -342,6 +345,10 @@ static
> > int e500_pcihost_initfn(SysBusDevice *dev)
> >      memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
> >      sysbus_init_mmio(dev, &s->container);
> >
> > +    bar0 = *(MemoryRegion *)s->bar0;
> > +    pdev = pci_find_device(b, 0, 0);
> > +    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0);
> > +
> 
> This is broken, you're registering an object on the stack which will be freed as
> soon as the function returns.
> 
> Just pass &s->bar0 as Alex suggests.

Ok.

> 
> However this is best done from the pci device's initialization function, not
> here (the MemoryRegion should be a member of the pci device as well).

We want to set BAR0 of PCI controller so we did this here. But yes, we also want that all devices under the PCI controller have this mapping, so when they access the MPIC register to send MSI then they can do that.

Which device's initialization function you are talking about?

Thanks
-Bharat

> 
> >      return 0;
> >  }
> >
> 
> 
> --
> error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Thursday, October 04, 2012 6:02 PM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan Bharat-
>> R65777
>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
>> 
>> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
>> >      sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
>> > a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2 100644
>> > --- a/hw/ppce500_pci.c
>> > +++ b/hw/ppce500_pci.c
>> > @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>> >      /* mmio maps */
>> >      MemoryRegion container;
>> >      MemoryRegion iomem;
>> > +    void *bar0;
>> >  };
>> 
>> void *?  Why?
> 
> Passing parameter via qdev is either possible by value or by void *. That's why I used void *.

Then you shouldn't be using qdev for this.  But if you make the changes
below, it will likely not be necessary.

>> 
>> However this is best done from the pci device's initialization function, not
>> here (the MemoryRegion should be a member of the pci device as well).
> 
> We want to set BAR0 of PCI controller so we did this here. But yes, we also want that all devices under the PCI controller have this mapping, so when they access the MPIC register to send MSI then they can do that.

Please elaborate.  One way to describe what you want is to issue an
'info mtree' and show what changes you want.

> 
> Which device's initialization function you are talking about?

static const TypeInfo e500_host_bridge_info = {
    .name          = "e500-host-bridge",
    .parent        = TYPE_PCI_DEVICE,
    .instance_size = sizeof(PCIDevice),
    .class_init    = e500_host_bridge_class_init,
};

This needs to describe a derived class of PCIDevice, hold the BAR as a
data member, and register the BAR in the init function (which doesn't
exist yet because you haven't subclassed it).  This way the BAR lives in
the device which exposes it, like BARs everywhere.

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

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

[Andreas Färber]

Am 03.10.2012 13:50, schrieb Bharat Bhushan:
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 197411d..c7ae2b6 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params)
>  
>      /* PCI */
>      dev = qdev_create(NULL, "e500-pcihost");
> +    qdev_prop_set_ptr(dev, "bar0_region", ccsr);
>      qdev_init_nofail(dev);
>      s = sysbus_from_qdev(dev);
>      sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);

Please...

> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..16e4af2 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>      /* mmio maps */
>      MemoryRegion container;
>      MemoryRegion iomem;
> +    void *bar0;
>  };
>  
>  typedef struct PPCE500PCIState PPCE500PCIState;

...do not do this. qdev_prop_set_ptr() is considered deprecated and we
had long discussions how to solve this differently.

Was there anything wrong with using a SysBusDevice for the CCSR to
encapsulate the MemoryRegion?

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/2] Adding BAR0 for e500 PCI controller

[Alexander Graf]

On 04.10.2012, at 17:54, Andreas Färber wrote:

> Am 03.10.2012 13:50, schrieb Bharat Bhushan:
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 197411d..c7ae2b6 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params)
>> 
>>     /* PCI */
>>     dev = qdev_create(NULL, "e500-pcihost");
>> +    qdev_prop_set_ptr(dev, "bar0_region", ccsr);
>>     qdev_init_nofail(dev);
>>     s = sysbus_from_qdev(dev);
>>     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> 
> Please...
> 
>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>> index 92b1dc0..16e4af2 100644
>> --- a/hw/ppce500_pci.c
>> +++ b/hw/ppce500_pci.c
>> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>>     /* mmio maps */
>>     MemoryRegion container;
>>     MemoryRegion iomem;
>> +    void *bar0;
>> };
>> 
>> typedef struct PPCE500PCIState PPCE500PCIState;
> 
> ...do not do this. qdev_prop_set_ptr() is considered deprecated and we
> had long discussions how to solve this differently.
> 
> Was there anything wrong with using a SysBusDevice for the CCSR to
> encapsulate the MemoryRegion?

Not at all. This was meant as a first shot, so we can slowly move towards the CCSR-as-device model.
In fact, now that we have this code as far as it is, we can go over to tackle it that way.

Bharat, mind to model a new CCSR device now that contains all the CCSR devices?

That one creates a memory region then. The PCI host controller gets a reference to the CCSR device and from there can call a CCSR specific function to receive the memory region pointer. And suddenly we have all of this solved :).


Alex

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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Thursday, October 04, 2012 8:28 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Avi Kivity [mailto:avi@redhat.com]
> >> Sent: Thursday, October 04, 2012 6:02 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
> >> Bhushan Bharat-
> >> R65777
> >> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
> >> controller
> >>
> >> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
> >> >      sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
> >> > a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2 100644
> >> > --- a/hw/ppce500_pci.c
> >> > +++ b/hw/ppce500_pci.c
> >> > @@ -87,6 +87,7 @@ struct PPCE500PCIState {
> >> >      /* mmio maps */
> >> >      MemoryRegion container;
> >> >      MemoryRegion iomem;
> >> > +    void *bar0;
> >> >  };
> >>
> >> void *?  Why?
> >
> > Passing parameter via qdev is either possible by value or by void *. That's
> why I used void *.
> 
> Then you shouldn't be using qdev for this.  But if you make the changes below,
> it will likely not be necessary.
> 
> >>
> >> However this is best done from the pci device's initialization
> >> function, not here (the MemoryRegion should be a member of the pci device as
> well).
> >
> > We want to set BAR0 of PCI controller so we did this here. But yes, we also
> want that all devices under the PCI controller have this mapping, so when they
> access the MPIC register to send MSI then they can do that.
> 
> Please elaborate.  One way to describe what you want is to issue an 'info mtree'
> and show what changes you want.

Setup is something like this:

  |-------------|
  | PCI device  |
  |             |
  ---------------
        |
        |
        |
        |
 |-------------------|
 |                   |
 | PCI controller    |
 |                   |
 |   --------------  |
 |   |  BAR0 in   |  |
 |   |   TYPE1    |  |
 |   | Config HDR |  |
 |   --------------  |
 |                   |
  -------------------

BAR0: it is an inbound window, and on e500 devices this maps the pci bus address to CCSR address.

My requirement are:
 1) when guest read pci controller BAR0, it is able to get proper size ( Size of CCSR space)
 2) Guest can program this to any address in pci address space. When PCI device access this address range then that address should be mapped to CCSR address space.

Though this patch only met the requirement number (1). 

> 
> >
> > Which device's initialization function you are talking about?
> 
> static const TypeInfo e500_host_bridge_info = {
>     .name          = "e500-host-bridge",
>     .parent        = TYPE_PCI_DEVICE,
>     .instance_size = sizeof(PCIDevice),
>     .class_init    = e500_host_bridge_class_init,
> };
> 
> This needs to describe a derived class of PCIDevice, hold the BAR as a data
> member, and register the BAR in the init function (which doesn't exist yet
> because you haven't subclassed it).  This way the BAR lives in the device which
> exposes it, like BARs everywhere.

Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add this) function of "e500-host-bridge"

This way I should be able to met both of my requirements.

Thanks
-Bharat

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

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

[Alexander Graf]

On 04.10.2012, at 18:03, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Thursday, October 04, 2012 8:28 PM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
>> 
>> On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Avi Kivity [mailto:avi@redhat.com]
>>>> Sent: Thursday, October 04, 2012 6:02 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
>>>> Bhushan Bharat-
>>>> R65777
>>>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
>>>> controller
>>>> 
>>>> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
>>>>>     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
>>>>> a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2 100644
>>>>> --- a/hw/ppce500_pci.c
>>>>> +++ b/hw/ppce500_pci.c
>>>>> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>>>>>     /* mmio maps */
>>>>>     MemoryRegion container;
>>>>>     MemoryRegion iomem;
>>>>> +    void *bar0;
>>>>> };
>>>> 
>>>> void *?  Why?
>>> 
>>> Passing parameter via qdev is either possible by value or by void *. That's
>> why I used void *.
>> 
>> Then you shouldn't be using qdev for this.  But if you make the changes below,
>> it will likely not be necessary.
>> 
>>>> 
>>>> However this is best done from the pci device's initialization
>>>> function, not here (the MemoryRegion should be a member of the pci device as
>> well).
>>> 
>>> We want to set BAR0 of PCI controller so we did this here. But yes, we also
>> want that all devices under the PCI controller have this mapping, so when they
>> access the MPIC register to send MSI then they can do that.
>> 
>> Please elaborate.  One way to describe what you want is to issue an 'info mtree'
>> and show what changes you want.
> 
> Setup is something like this:
> 
>  |-------------|
>  | PCI device  |
>  |             |
>  ---------------
>        |
>        |
>        |
>        |
> |-------------------|
> |                   |
> | PCI controller    |
> |                   |
> |   --------------  |
> |   |  BAR0 in   |  |
> |   |   TYPE1    |  |
> |   | Config HDR |  |
> |   --------------  |
> |                   |
>  -------------------
> 
> BAR0: it is an inbound window, and on e500 devices this maps the pci bus address to CCSR address.
> 
> My requirement are:
> 1) when guest read pci controller BAR0, it is able to get proper size ( Size of CCSR space)
> 2) Guest can program this to any address in pci address space. When PCI device access this address range then that address should be mapped to CCSR address space.
> 
> Though this patch only met the requirement number (1). 

No, it also meets (2). The PCI address space is identical to the CPU memory space in our mapping right now. So if the guest maps BAR0 somewhere, it automatically maps CCSR into CPU address space, which exposes it to PCI address space.

> 
>> 
>>> 
>>> Which device's initialization function you are talking about?
>> 
>> static const TypeInfo e500_host_bridge_info = {
>>    .name          = "e500-host-bridge",
>>    .parent        = TYPE_PCI_DEVICE,
>>    .instance_size = sizeof(PCIDevice),
>>    .class_init    = e500_host_bridge_class_init,
>> };
>> 
>> This needs to describe a derived class of PCIDevice, hold the BAR as a data
>> member, and register the BAR in the init function (which doesn't exist yet
>> because you haven't subclassed it).  This way the BAR lives in the device which
>> exposes it, like BARs everywhere.
> 
> Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add this) function of "e500-host-bridge"

No, he means that you create a new struct like this:

  struct foo {
    PCIDevice p;
    MemoryRegion bar0;
  };

Please check out any other random PCI device in QEMU. Almost all of them do this to store more information than their parent class can hold.


Alex

> 
> This way I should be able to met both of my requirements.
> 
> Thanks
> -Bharat
> 
>> 
>> --
>> error compiling committee.c: too many arguments to function
> 
> 

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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, October 04, 2012 9:37 PM
> To: Bhushan Bharat-R65777
> Cc: Avi Kivity; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> 
> On 04.10.2012, at 18:03, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Avi Kivity [mailto:avi@redhat.com]
> >> Sent: Thursday, October 04, 2012 8:28 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
> >> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
> >> controller
> >>
> >> On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Avi Kivity [mailto:avi@redhat.com]
> >>>> Sent: Thursday, October 04, 2012 6:02 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
> >>>> Bhushan Bharat-
> >>>> R65777
> >>>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
> >>>> controller
> >>>>
> >>>> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
> >>>>>     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
> >>>>> a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2
> >>>>> 100644
> >>>>> --- a/hw/ppce500_pci.c
> >>>>> +++ b/hw/ppce500_pci.c
> >>>>> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
> >>>>>     /* mmio maps */
> >>>>>     MemoryRegion container;
> >>>>>     MemoryRegion iomem;
> >>>>> +    void *bar0;
> >>>>> };
> >>>>
> >>>> void *?  Why?
> >>>
> >>> Passing parameter via qdev is either possible by value or by void *.
> >>> That's
> >> why I used void *.
> >>
> >> Then you shouldn't be using qdev for this.  But if you make the
> >> changes below, it will likely not be necessary.
> >>
> >>>>
> >>>> However this is best done from the pci device's initialization
> >>>> function, not here (the MemoryRegion should be a member of the pci
> >>>> device as
> >> well).
> >>>
> >>> We want to set BAR0 of PCI controller so we did this here. But yes,
> >>> we also
> >> want that all devices under the PCI controller have this mapping, so
> >> when they access the MPIC register to send MSI then they can do that.
> >>
> >> Please elaborate.  One way to describe what you want is to issue an 'info
> mtree'
> >> and show what changes you want.
> >
> > Setup is something like this:
> >
> >  |-------------|
> >  | PCI device  |
> >  |             |
> >  ---------------
> >        |
> >        |
> >        |
> >        |
> > |-------------------|
> > |                   |
> > | PCI controller    |
> > |                   |
> > |   --------------  |
> > |   |  BAR0 in   |  |
> > |   |   TYPE1    |  |
> > |   | Config HDR |  |
> > |   --------------  |
> > |                   |
> >  -------------------
> >
> > BAR0: it is an inbound window, and on e500 devices this maps the pci bus
> address to CCSR address.
> >
> > My requirement are:
> > 1) when guest read pci controller BAR0, it is able to get proper size
> > ( Size of CCSR space)
> > 2) Guest can program this to any address in pci address space. When PCI device
> access this address range then that address should be mapped to CCSR address
> space.
> >
> > Though this patch only met the requirement number (1).
> 
> No, it also meets (2). The PCI address space is identical to the CPU memory
> space in our mapping right now. So if the guest maps BAR0 somewhere, it
> automatically maps CCSR into CPU address space, which exposes it to PCI address
> space.

Really? I think on powerpc the pci address space is defined as: it maps the outbound window just below 0x1_0000_0000, then CCSR and then inbound window. So inbound window is 1:1 map if guest physical starts from 0x0. But I do not think CCSR is 1:1 map in pci address space and cpu address space. 

> 
> >
> >>
> >>>
> >>> Which device's initialization function you are talking about?
> >>
> >> static const TypeInfo e500_host_bridge_info = {
> >>    .name          = "e500-host-bridge",
> >>    .parent        = TYPE_PCI_DEVICE,
> >>    .instance_size = sizeof(PCIDevice),
> >>    .class_init    = e500_host_bridge_class_init,
> >> };
> >>
> >> This needs to describe a derived class of PCIDevice, hold the BAR as
> >> a data member, and register the BAR in the init function (which
> >> doesn't exist yet because you haven't subclassed it).  This way the
> >> BAR lives in the device which exposes it, like BARs everywhere.
> >
> > Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the
> same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add
> this) function of "e500-host-bridge"
> 
> No, he means that you create a new struct like this:
> 
>   struct foo {
>     PCIDevice p;
>     MemoryRegion bar0;
>   };
> 
> Please check out any other random PCI device in QEMU. Almost all of them do this
> to store more information than their parent class can hold.

Ok,

Thanks
-Bharat

> 
> 
> Alex
> 
> >
> > This way I should be able to met both of my requirements.
> >
> > Thanks
> > -Bharat
> >
> >>
> >> --
> >> error compiling committee.c: too many arguments to function
> >
> >
> 

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

[Alexander Graf]

On 04.10.2012, at 18:48, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, October 04, 2012 9:37 PM
>> To: Bhushan Bharat-R65777
>> Cc: Avi Kivity; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
>> 
>> 
>> On 04.10.2012, at 18:03, Bhushan Bharat-R65777 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Avi Kivity [mailto:avi@redhat.com]
>>>> Sent: Thursday, October 04, 2012 8:28 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
>>>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
>>>> controller
>>>> 
>>>> On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
>>>>> 
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Avi Kivity [mailto:avi@redhat.com]
>>>>>> Sent: Thursday, October 04, 2012 6:02 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
>>>>>> Bhushan Bharat-
>>>>>> R65777
>>>>>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
>>>>>> controller
>>>>>> 
>>>>>> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
>>>>>>>    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
>>>>>>> a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2
>>>>>>> 100644
>>>>>>> --- a/hw/ppce500_pci.c
>>>>>>> +++ b/hw/ppce500_pci.c
>>>>>>> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>>>>>>>    /* mmio maps */
>>>>>>>    MemoryRegion container;
>>>>>>>    MemoryRegion iomem;
>>>>>>> +    void *bar0;
>>>>>>> };
>>>>>> 
>>>>>> void *?  Why?
>>>>> 
>>>>> Passing parameter via qdev is either possible by value or by void *.
>>>>> That's
>>>> why I used void *.
>>>> 
>>>> Then you shouldn't be using qdev for this.  But if you make the
>>>> changes below, it will likely not be necessary.
>>>> 
>>>>>> 
>>>>>> However this is best done from the pci device's initialization
>>>>>> function, not here (the MemoryRegion should be a member of the pci
>>>>>> device as
>>>> well).
>>>>> 
>>>>> We want to set BAR0 of PCI controller so we did this here. But yes,
>>>>> we also
>>>> want that all devices under the PCI controller have this mapping, so
>>>> when they access the MPIC register to send MSI then they can do that.
>>>> 
>>>> Please elaborate.  One way to describe what you want is to issue an 'info
>> mtree'
>>>> and show what changes you want.
>>> 
>>> Setup is something like this:
>>> 
>>> |-------------|
>>> | PCI device  |
>>> |             |
>>> ---------------
>>>       |
>>>       |
>>>       |
>>>       |
>>> |-------------------|
>>> |                   |
>>> | PCI controller    |
>>> |                   |
>>> |   --------------  |
>>> |   |  BAR0 in   |  |
>>> |   |   TYPE1    |  |
>>> |   | Config HDR |  |
>>> |   --------------  |
>>> |                   |
>>> -------------------
>>> 
>>> BAR0: it is an inbound window, and on e500 devices this maps the pci bus
>> address to CCSR address.
>>> 
>>> My requirement are:
>>> 1) when guest read pci controller BAR0, it is able to get proper size
>>> ( Size of CCSR space)
>>> 2) Guest can program this to any address in pci address space. When PCI device
>> access this address range then that address should be mapped to CCSR address
>> space.
>>> 
>>> Though this patch only met the requirement number (1).
>> 
>> No, it also meets (2). The PCI address space is identical to the CPU memory
>> space in our mapping right now. So if the guest maps BAR0 somewhere, it
>> automatically maps CCSR into CPU address space, which exposes it to PCI address
>> space.
> 
> Really? I think on powerpc the pci address space is defined as: it maps the outbound window just below 0x1_0000_0000, then CCSR and then inbound window. So inbound window is 1:1 map if guest physical starts from 0x0. But I do not think CCSR is 1:1 map in pci address space and cpu address space. 

In QEMU, we map everything 1:1 today.


Alex

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

[Avi Kivity]

On 10/04/2012 06:50 PM, Alexander Graf wrote:
>>> 
>>> No, it also meets (2). The PCI address space is identical to the CPU memory
>>> space in our mapping right now. So if the guest maps BAR0 somewhere, it
>>> automatically maps CCSR into CPU address space, which exposes it to PCI address
>>> space.
>> 
>> Really? I think on powerpc the pci address space is defined as: it maps the outbound window just below 0x1_0000_0000, then CCSR and then inbound window. So inbound window is 1:1 map if guest physical starts from 0x0. But I do not think CCSR is 1:1 map in pci address space and cpu address space. 
> 
> In QEMU, we map everything 1:1 today.

An unmerged patch set entitled "Integrate DMA into the memory API"
changes that.  I'll be happy to work with you to make use of it to
emulate the hardware properly, it will give me a nice test case.

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

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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, October 04, 2012 9:37 PM
> To: Bhushan Bharat-R65777
> Cc: Avi Kivity; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> 
> On 04.10.2012, at 18:03, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Avi Kivity [mailto:avi@redhat.com]
> >> Sent: Thursday, October 04, 2012 8:28 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
> >> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
> >> controller
> >>
> >> On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Avi Kivity [mailto:avi@redhat.com]
> >>>> Sent: Thursday, October 04, 2012 6:02 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
> >>>> Bhushan Bharat-
> >>>> R65777
> >>>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
> >>>> controller
> >>>>
> >>>> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
> >>>>>     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
> >>>>> a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2
> >>>>> 100644
> >>>>> --- a/hw/ppce500_pci.c
> >>>>> +++ b/hw/ppce500_pci.c
> >>>>> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
> >>>>>     /* mmio maps */
> >>>>>     MemoryRegion container;
> >>>>>     MemoryRegion iomem;
> >>>>> +    void *bar0;
> >>>>> };
> >>>>
> >>>> void *?  Why?
> >>>
> >>> Passing parameter via qdev is either possible by value or by void *.
> >>> That's
> >> why I used void *.
> >>
> >> Then you shouldn't be using qdev for this.  But if you make the
> >> changes below, it will likely not be necessary.
> >>
> >>>>
> >>>> However this is best done from the pci device's initialization
> >>>> function, not here (the MemoryRegion should be a member of the pci
> >>>> device as
> >> well).
> >>>
> >>> We want to set BAR0 of PCI controller so we did this here. But yes,
> >>> we also
> >> want that all devices under the PCI controller have this mapping, so
> >> when they access the MPIC register to send MSI then they can do that.
> >>
> >> Please elaborate.  One way to describe what you want is to issue an 'info
> mtree'
> >> and show what changes you want.
> >
> > Setup is something like this:
> >
> >  |-------------|
> >  | PCI device  |
> >  |             |
> >  ---------------
> >        |
> >        |
> >        |
> >        |
> > |-------------------|
> > |                   |
> > | PCI controller    |
> > |                   |
> > |   --------------  |
> > |   |  BAR0 in   |  |
> > |   |   TYPE1    |  |
> > |   | Config HDR |  |
> > |   --------------  |
> > |                   |
> >  -------------------
> >
> > BAR0: it is an inbound window, and on e500 devices this maps the pci bus
> address to CCSR address.
> >
> > My requirement are:
> > 1) when guest read pci controller BAR0, it is able to get proper size
> > ( Size of CCSR space)
> > 2) Guest can program this to any address in pci address space. When PCI device
> access this address range then that address should be mapped to CCSR address
> space.
> >
> > Though this patch only met the requirement number (1).
> 
> No, it also meets (2). The PCI address space is identical to the CPU memory
> space in our mapping right now. So if the guest maps BAR0 somewhere, it
> automatically maps CCSR into CPU address space, which exposes it to PCI address
> space.
> 
> >
> >>
> >>>
> >>> Which device's initialization function you are talking about?
> >>
> >> static const TypeInfo e500_host_bridge_info = {
> >>    .name          = "e500-host-bridge",
> >>    .parent        = TYPE_PCI_DEVICE,
> >>    .instance_size = sizeof(PCIDevice),
> >>    .class_init    = e500_host_bridge_class_init,
> >> };
> >>
> >> This needs to describe a derived class of PCIDevice, hold the BAR as
> >> a data member, and register the BAR in the init function (which
> >> doesn't exist yet because you haven't subclassed it).  This way the
> >> BAR lives in the device which exposes it, like BARs everywhere.
> >
> > Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the
> same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add
> this) function of "e500-host-bridge"
> 
> No, he means that you create a new struct like this:
> 
>   struct foo {
>     PCIDevice p;
>     MemoryRegion bar0;
>   };
> 
> Please check out any other random PCI device in QEMU. Almost all of them do this
> to store more information than their parent class can hold.

Just want to be sure I understood you correctly: Do you mean something like this : ( I know I have to switch to QOM mechanism to share parameters)

diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 92b1dc0..a948bc6 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -89,6 +89,12 @@ struct PPCE500PCIState {
     MemoryRegion iomem;
 };
 
+struct BHARAT {
+    PCIDevice p;
+    void *bar0;
+};
+
+typedef struct BHARAT bharat;
 typedef struct PPCE500PCIState PPCE500PCIState;
 
 static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
@@ -307,6 +313,16 @@ static const VMStateDescription vmstate_ppce500_pci = {
 
 #include "exec-memory.h"
 
+static int e500_pcihost_bridge_initfn(PCIDevice *d)
+{
+    bharat *b = DO_UPCAST(bharat, p, d);
+
+    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr, (unsigned long long)int128_get64(((Me
+    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0);
+    return 0;
+}
+
+MemoryRegion ccsr;
 static int e500_pcihost_initfn(SysBusDevice *dev)
 {
     PCIHostState *h;
@@ -315,6 +331,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     int i;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *address_space_io = get_system_io();
+    PCIDevice *d;
 
     h = PCI_HOST_BRIDGE(dev);
     s = PPC_E500_PCI_HOST_BRIDGE(dev);
@@ -328,7 +345,12 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
                          address_space_io, PCI_DEVFN(0x11, 0), 4);
     h->bus = b;
 
-    pci_create_simple(b, 0, "e500-host-bridge");
+    d = pci_create(b, 0, "e500-host-bridge");
+    /* ccsr-> should be passed from hw/ppc/e500.c */
+    ccsr.addr = 0xE0000000;
+    ccsr.size = int128_make64(0x00100000);
+    qdev_prop_set_ptr(&d->qdev, "bar0_region", &ccsr);
+    qdev_init_nofail(&d->qdev);
 
     memory_region_init(&s->container, "pci-container", PCIE500_ALL_SIZE);
     memory_region_init_io(&h->conf_mem, &pci_host_conf_be_ops, h,
@@ -345,11 +367,18 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     return 0;
 }
 
+static Property pci_host_dev_info[] = {
+    DEFINE_PROP_PTR("bar0_region", bharat, bar0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 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;
+    dc->props = pci_host_dev_info;
     k->vendor_id = PCI_VENDOR_ID_FREESCALE;
     k->device_id = PCI_DEVICE_ID_MPC8533E;
     k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
@@ -359,10 +388,11 @@ 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(bharat),
     .class_init    = e500_host_bridge_class_init,
 };
 
 static void e500_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);

Thanks
-Bharat

> 
> 
> Alex
> 
> >
> > This way I should be able to met both of my requirements.
> >
> > Thanks
> > -Bharat
> >
> >>
> >> --
> >> error compiling committee.c: too many arguments to function
> >
> >
> 

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

[Alexander Graf]

On 05.10.2012, at 09:11, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, October 04, 2012 9:37 PM
>> To: Bhushan Bharat-R65777
>> Cc: Avi Kivity; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
>> 
>> 
>> On 04.10.2012, at 18:03, Bhushan Bharat-R65777 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Avi Kivity [mailto:avi@redhat.com]
>>>> Sent: Thursday, October 04, 2012 8:28 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
>>>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
>>>> controller
>>>> 
>>>> On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
>>>>> 
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Avi Kivity [mailto:avi@redhat.com]
>>>>>> Sent: Thursday, October 04, 2012 6:02 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
>>>>>> Bhushan Bharat-
>>>>>> R65777
>>>>>> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
>>>>>> controller
>>>>>> 
>>>>>> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
>>>>>>>    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); diff --git
>>>>>>> a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 92b1dc0..16e4af2
>>>>>>> 100644
>>>>>>> --- a/hw/ppce500_pci.c
>>>>>>> +++ b/hw/ppce500_pci.c
>>>>>>> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>>>>>>>    /* mmio maps */
>>>>>>>    MemoryRegion container;
>>>>>>>    MemoryRegion iomem;
>>>>>>> +    void *bar0;
>>>>>>> };
>>>>>> 
>>>>>> void *?  Why?
>>>>> 
>>>>> Passing parameter via qdev is either possible by value or by void *.
>>>>> That's
>>>> why I used void *.
>>>> 
>>>> Then you shouldn't be using qdev for this.  But if you make the
>>>> changes below, it will likely not be necessary.
>>>> 
>>>>>> 
>>>>>> However this is best done from the pci device's initialization
>>>>>> function, not here (the MemoryRegion should be a member of the pci
>>>>>> device as
>>>> well).
>>>>> 
>>>>> We want to set BAR0 of PCI controller so we did this here. But yes,
>>>>> we also
>>>> want that all devices under the PCI controller have this mapping, so
>>>> when they access the MPIC register to send MSI then they can do that.
>>>> 
>>>> Please elaborate.  One way to describe what you want is to issue an 'info
>> mtree'
>>>> and show what changes you want.
>>> 
>>> Setup is something like this:
>>> 
>>> |-------------|
>>> | PCI device  |
>>> |             |
>>> ---------------
>>>       |
>>>       |
>>>       |
>>>       |
>>> |-------------------|
>>> |                   |
>>> | PCI controller    |
>>> |                   |
>>> |   --------------  |
>>> |   |  BAR0 in   |  |
>>> |   |   TYPE1    |  |
>>> |   | Config HDR |  |
>>> |   --------------  |
>>> |                   |
>>> -------------------
>>> 
>>> BAR0: it is an inbound window, and on e500 devices this maps the pci bus
>> address to CCSR address.
>>> 
>>> My requirement are:
>>> 1) when guest read pci controller BAR0, it is able to get proper size
>>> ( Size of CCSR space)
>>> 2) Guest can program this to any address in pci address space. When PCI device
>> access this address range then that address should be mapped to CCSR address
>> space.
>>> 
>>> Though this patch only met the requirement number (1).
>> 
>> No, it also meets (2). The PCI address space is identical to the CPU memory
>> space in our mapping right now. So if the guest maps BAR0 somewhere, it
>> automatically maps CCSR into CPU address space, which exposes it to PCI address
>> space.
>> 
>>> 
>>>> 
>>>>> 
>>>>> Which device's initialization function you are talking about?
>>>> 
>>>> static const TypeInfo e500_host_bridge_info = {
>>>>   .name          = "e500-host-bridge",
>>>>   .parent        = TYPE_PCI_DEVICE,
>>>>   .instance_size = sizeof(PCIDevice),
>>>>   .class_init    = e500_host_bridge_class_init,
>>>> };
>>>> 
>>>> This needs to describe a derived class of PCIDevice, hold the BAR as
>>>> a data member, and register the BAR in the init function (which
>>>> doesn't exist yet because you haven't subclassed it).  This way the
>>>> BAR lives in the device which exposes it, like BARs everywhere.
>>> 
>>> Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the
>> same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add
>> this) function of "e500-host-bridge"
>> 
>> No, he means that you create a new struct like this:
>> 
>>  struct foo {
>>    PCIDevice p;
>>    MemoryRegion bar0;
>>  };
>> 
>> Please check out any other random PCI device in QEMU. Almost all of them do this
>> to store more information than their parent class can hold.
> 
> Just want to be sure I understood you correctly: Do you mean something like this : ( I know I have to switch to QOM mechanism to share parameters)
> 
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..a948bc6 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -89,6 +89,12 @@ struct PPCE500PCIState {
>     MemoryRegion iomem;
> };
> 
> +struct BHARAT {
> +    PCIDevice p;
> +    void *bar0;

MemoryRegion *bar0

> +};
> +
> +typedef struct BHARAT bharat;
> typedef struct PPCE500PCIState PPCE500PCIState;
> 
> static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
> @@ -307,6 +313,16 @@ static const VMStateDescription vmstate_ppce500_pci = {
> 
> #include "exec-memory.h"
> 
> +static int e500_pcihost_bridge_initfn(PCIDevice *d)
> +{
> +    bharat *b = DO_UPCAST(bharat, p, d);
> +
> +    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr, (unsigned long long)int128_get64(((Me
> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0);

That one still has to call its parent initfn, no?

> +    return 0;
> +}
> +
> +MemoryRegion ccsr;
> static int e500_pcihost_initfn(SysBusDevice *dev)
> {
>     PCIHostState *h;
> @@ -315,6 +331,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     int i;
>     MemoryRegion *address_space_mem = get_system_memory();
>     MemoryRegion *address_space_io = get_system_io();
> +    PCIDevice *d;
> 
>     h = PCI_HOST_BRIDGE(dev);
>     s = PPC_E500_PCI_HOST_BRIDGE(dev);
> @@ -328,7 +345,12 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>                          address_space_io, PCI_DEVFN(0x11, 0), 4);
>     h->bus = b;
> 
> -    pci_create_simple(b, 0, "e500-host-bridge");
> +    d = pci_create(b, 0, "e500-host-bridge");
> +    /* ccsr-> should be passed from hw/ppc/e500.c */
> +    ccsr.addr = 0xE0000000;
> +    ccsr.size = int128_make64(0x00100000);

What is this?


Alex

> +    qdev_prop_set_ptr(&d->qdev, "bar0_region", &ccsr);
> +    qdev_init_nofail(&d->qdev);
> 
>     memory_region_init(&s->container, "pci-container", PCIE500_ALL_SIZE);
>     memory_region_init_io(&h->conf_mem, &pci_host_conf_be_ops, h,
> @@ -345,11 +367,18 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     return 0;
> }
> 
> +static Property pci_host_dev_info[] = {
> +    DEFINE_PROP_PTR("bar0_region", bharat, bar0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> 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;
> +    dc->props = pci_host_dev_info;
>     k->vendor_id = PCI_VENDOR_ID_FREESCALE;
>     k->device_id = PCI_DEVICE_ID_MPC8533E;
>     k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
> @@ -359,10 +388,11 @@ 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(bharat),
>     .class_init    = e500_host_bridge_class_init,
> };
> 
> static void e500_pcihost_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> 
> Thanks
> -Bharat
> 
>> 
>> 
>> Alex
>> 
>>> 
>>> This way I should be able to met both of my requirements.
>>> 
>>> Thanks
>>> -Bharat
>>> 
>>>> 
>>>> --
>>>> error compiling committee.c: too many arguments to function
>>> 
>>> 
>> 
> 
> 

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

[Avi Kivity]

On 10/05/2012 01:59 PM, Alexander Graf wrote:
>>>> Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the
>>> same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add
>>> this) function of "e500-host-bridge"
>>> 
>>> No, he means that you create a new struct like this:
>>> 
>>>  struct foo {
>>>    PCIDevice p;
>>>    MemoryRegion bar0;
>>>  };
>>> 
>>> Please check out any other random PCI device in QEMU. Almost all of them do this
>>> to store more information than their parent class can hold.
>> 
>> Just want to be sure I understood you correctly: Do you mean something like this : ( I know I have to switch to QOM mechanism to share parameters)
>> 
>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>> index 92b1dc0..a948bc6 100644
>> --- a/hw/ppce500_pci.c
>> +++ b/hw/ppce500_pci.c
>> @@ -89,6 +89,12 @@ struct PPCE500PCIState {
>>     MemoryRegion iomem;
>> };
>> 
>> +struct BHARAT {
>> +    PCIDevice p;
>> +    void *bar0;
> 
> MemoryRegion *bar0

MemoryRegion bar0;

> 
>> +};
>> +
>> +typedef struct BHARAT bharat;
>> typedef struct PPCE500PCIState PPCE500PCIState;
>> 
>> static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
>> @@ -307,6 +313,16 @@ static const VMStateDescription vmstate_ppce500_pci = {
>> 
>> #include "exec-memory.h"
>> 
>> +static int e500_pcihost_bridge_initfn(PCIDevice *d)
>> +{
>> +    bharat *b = DO_UPCAST(bharat, p, d);
>> +
>> +    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr, (unsigned long long)int128_get64(((Me

memory_region_init_io(&d->bar0, ...);

>> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0);
> 
> That one still has to call its parent initfn, no?
> 

yes

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

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

[Alexander Graf]

On 07.10.2012, at 11:48, Avi Kivity <avi@redhat.com> wrote:

> On 10/05/2012 01:59 PM, Alexander Graf wrote:
>>>>> Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the
>>>> same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add
>>>> this) function of "e500-host-bridge"
>>>> 
>>>> No, he means that you create a new struct like this:
>>>> 
>>>> struct foo {
>>>>   PCIDevice p;
>>>>   MemoryRegion bar0;
>>>> };
>>>> 
>>>> Please check out any other random PCI device in QEMU. Almost all of them do this
>>>> to store more information than their parent class can hold.
>>> 
>>> Just want to be sure I understood you correctly: Do you mean something like this : ( I know I have to switch to QOM mechanism to share parameters)
>>> 
>>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>>> index 92b1dc0..a948bc6 100644
>>> --- a/hw/ppce500_pci.c
>>> +++ b/hw/ppce500_pci.c
>>> @@ -89,6 +89,12 @@ struct PPCE500PCIState {
>>>    MemoryRegion iomem;
>>> };
>>> 
>>> +struct BHARAT {
>>> +    PCIDevice p;
>>> +    void *bar0;
>> 
>> MemoryRegion *bar0
> 
> MemoryRegion bar0;

Why? We want the same region that is mapped outside of the pci device be available as BAR0 too.

Alex

> 
>> 
>>> +};
>>> +
>>> +typedef struct BHARAT bharat;
>>> typedef struct PPCE500PCIState PPCE500PCIState;
>>> 
>>> static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
>>> @@ -307,6 +313,16 @@ static const VMStateDescription vmstate_ppce500_pci = {
>>> 
>>> #include "exec-memory.h"
>>> 
>>> +static int e500_pcihost_bridge_initfn(PCIDevice *d)
>>> +{
>>> +    bharat *b = DO_UPCAST(bharat, p, d);
>>> +
>>> +    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr, (unsigned long long)int128_get64(((Me
> 
> memory_region_init_io(&d->bar0, ...);
> 
>>> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0);
>> 
>> That one still has to call its parent initfn, no?
>> 
> 
> yes
> 
> -- 
> error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 10/07/2012 01:57 PM, Alexander Graf wrote:
> 
> 
> On 07.10.2012, at 11:48, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 10/05/2012 01:59 PM, Alexander Graf wrote:
>>>>>> Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the
>>>>> same thing that I was doing in e500_pcihost_initfn() in the k->init() (will add
>>>>> this) function of "e500-host-bridge"
>>>>> 
>>>>> No, he means that you create a new struct like this:
>>>>> 
>>>>> struct foo {
>>>>>   PCIDevice p;
>>>>>   MemoryRegion bar0;
>>>>> };
>>>>> 
>>>>> Please check out any other random PCI device in QEMU. Almost all of them do this
>>>>> to store more information than their parent class can hold.
>>>> 
>>>> Just want to be sure I understood you correctly: Do you mean something like this : ( I know I have to switch to QOM mechanism to share parameters)
>>>> 
>>>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>>>> index 92b1dc0..a948bc6 100644
>>>> --- a/hw/ppce500_pci.c
>>>> +++ b/hw/ppce500_pci.c
>>>> @@ -89,6 +89,12 @@ struct PPCE500PCIState {
>>>>    MemoryRegion iomem;
>>>> };
>>>> 
>>>> +struct BHARAT {
>>>> +    PCIDevice p;
>>>> +    void *bar0;
>>> 
>>> MemoryRegion *bar0
>> 
>> MemoryRegion bar0;
> 
> Why? We want the same region that is mapped outside of the pci device be available as BAR0 too.

A MemoryRegion can only have one container.  If you want dual maps, use
memory_region_init_alias().

It is possible (not trivial though) to extend the memory API to support
m:n container:subregion relationships.  I don't think it's worthwhile
though.


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

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

[Bhushan Bharat-R65777]

> >>>>> Which device's initialization function you are talking about?
> >>>>
> >>>> static const TypeInfo e500_host_bridge_info = {
> >>>>   .name          = "e500-host-bridge",
> >>>>   .parent        = TYPE_PCI_DEVICE,
> >>>>   .instance_size = sizeof(PCIDevice),
> >>>>   .class_init    = e500_host_bridge_class_init,
> >>>> };
> >>>>
> >>>> This needs to describe a derived class of PCIDevice, hold the BAR
> >>>> as a data member, and register the BAR in the init function (which
> >>>> doesn't exist yet because you haven't subclassed it).  This way the
> >>>> BAR lives in the device which exposes it, like BARs everywhere.
> >>>
> >>> Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct.
> >>> Do the
> >> same thing that I was doing in e500_pcihost_initfn() in the k->init()
> >> (will add
> >> this) function of "e500-host-bridge"
> >>
> >> No, he means that you create a new struct like this:
> >>
> >>  struct foo {
> >>    PCIDevice p;
> >>    MemoryRegion bar0;
> >>  };
> >>
> >> Please check out any other random PCI device in QEMU. Almost all of
> >> them do this to store more information than their parent class can hold.
> >
> > Just want to be sure I understood you correctly: Do you mean something
> > like this : ( I know I have to switch to QOM mechanism to share
> > parameters)
> >
> > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c index
> > 92b1dc0..a948bc6 100644
> > --- a/hw/ppce500_pci.c
> > +++ b/hw/ppce500_pci.c
> > @@ -89,6 +89,12 @@ struct PPCE500PCIState {
> >     MemoryRegion iomem;
> > };
> >
> > +struct BHARAT {
> > +    PCIDevice p;
> > +    void *bar0;
> 
> MemoryRegion *bar0

If I uses " MemoryRegion *bar0"  or " MemoryRegion bar0" then how the size and address of bar0 will be populated?

As far as I can see, other PCI devices using this way to have extra information but they does not get MemoryRegion information from other object.

> 
> > +};
> > +
> > +typedef struct BHARAT bharat;
> > typedef struct PPCE500PCIState PPCE500PCIState;
> >
> > static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
> > @@ -307,6 +313,16 @@ static const VMStateDescription
> > vmstate_ppce500_pci = {
> >
> > #include "exec-memory.h"
> >
> > +static int e500_pcihost_bridge_initfn(PCIDevice *d) {
> > +    bharat *b = DO_UPCAST(bharat, p, d);
> > +
> > +    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr,
> (unsigned long long)int128_get64(((Me
> > +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
> > + (MemoryRegion *)b->bar0);
> 
> That one still has to call its parent initfn, no?

I am really sorry, but I did not get. The object says its parent is TYPE_PCI_DEVICE, so which function are you talking about?

> 
> > +    return 0;
> > +}
> > +
> > +MemoryRegion ccsr;
> > static int e500_pcihost_initfn(SysBusDevice *dev) {
> >     PCIHostState *h;
> > @@ -315,6 +331,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> >     int i;
> >     MemoryRegion *address_space_mem = get_system_memory();
> >     MemoryRegion *address_space_io = get_system_io();
> > +    PCIDevice *d;
> >
> >     h = PCI_HOST_BRIDGE(dev);
> >     s = PPC_E500_PCI_HOST_BRIDGE(dev); @@ -328,7 +345,12 @@ static int
> > e500_pcihost_initfn(SysBusDevice *dev)
> >                          address_space_io, PCI_DEVFN(0x11, 0), 4);
> >     h->bus = b;
> >
> > -    pci_create_simple(b, 0, "e500-host-bridge");
> > +    d = pci_create(b, 0, "e500-host-bridge");
> > +    /* ccsr-> should be passed from hw/ppc/e500.c */
> > +    ccsr.addr = 0xE0000000;
> > +    ccsr.size = int128_make64(0x00100000);
> 
> What is this?

I am trying to pass the CCSR information to "e500-host-bridge". This is currently hardcoded, but finally this will come from hw/ppc/e500.c.

Thanks
-Bharat

> 
> 
> Alex
> 
> > +    qdev_prop_set_ptr(&d->qdev, "bar0_region", &ccsr);
> > +    qdev_init_nofail(&d->qdev);
> >
> >     memory_region_init(&s->container, "pci-container", PCIE500_ALL_SIZE);
> >     memory_region_init_io(&h->conf_mem, &pci_host_conf_be_ops, h, @@
> > -345,11 +367,18 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> >     return 0;
> > }
> >
> > +static Property pci_host_dev_info[] = {
> > +    DEFINE_PROP_PTR("bar0_region", bharat, bar0),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > 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;
> > +    dc->props = pci_host_dev_info;
> >     k->vendor_id = PCI_VENDOR_ID_FREESCALE;
> >     k->device_id = PCI_DEVICE_ID_MPC8533E;
> >     k->class_id = PCI_CLASS_PROCESSOR_POWERPC; @@ -359,10 +388,11 @@
> > 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(bharat),
> >     .class_init    = e500_host_bridge_class_init,
> > };
> >
> > static void e500_pcihost_class_init(ObjectClass *klass, void *data) {
> >     DeviceClass *dc = DEVICE_CLASS(klass);
> >
> > Thanks
> > -Bharat
> >
> >>
> >>
> >> Alex
> >>
> >>>
> >>> This way I should be able to met both of my requirements.
> >>>
> >>> Thanks
> >>> -Bharat
> >>>
> >>>>
> >>>> --
> >>>> error compiling committee.c: too many arguments to function
> >>>
> >>>
> >>
> >
> >
> 

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

[Alexander Graf]

On 08.10.2012, at 10:23, Bhushan Bharat-R65777 wrote:

>>>>>>> Which device's initialization function you are talking about?
>>>>>> 
>>>>>> static const TypeInfo e500_host_bridge_info = {
>>>>>>  .name          = "e500-host-bridge",
>>>>>>  .parent        = TYPE_PCI_DEVICE,
>>>>>>  .instance_size = sizeof(PCIDevice),
>>>>>>  .class_init    = e500_host_bridge_class_init,
>>>>>> };
>>>>>> 
>>>>>> This needs to describe a derived class of PCIDevice, hold the BAR
>>>>>> as a data member, and register the BAR in the init function (which
>>>>>> doesn't exist yet because you haven't subclassed it).  This way the
>>>>>> BAR lives in the device which exposes it, like BARs everywhere.
>>>>> 
>>>>> Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct.
>>>>> Do the
>>>> same thing that I was doing in e500_pcihost_initfn() in the k->init()
>>>> (will add
>>>> this) function of "e500-host-bridge"
>>>> 
>>>> No, he means that you create a new struct like this:
>>>> 
>>>> struct foo {
>>>>   PCIDevice p;
>>>>   MemoryRegion bar0;
>>>> };
>>>> 
>>>> Please check out any other random PCI device in QEMU. Almost all of
>>>> them do this to store more information than their parent class can hold.
>>> 
>>> Just want to be sure I understood you correctly: Do you mean something
>>> like this : ( I know I have to switch to QOM mechanism to share
>>> parameters)
>>> 
>>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c index
>>> 92b1dc0..a948bc6 100644
>>> --- a/hw/ppce500_pci.c
>>> +++ b/hw/ppce500_pci.c
>>> @@ -89,6 +89,12 @@ struct PPCE500PCIState {
>>>    MemoryRegion iomem;
>>> };
>>> 
>>> +struct BHARAT {
>>> +    PCIDevice p;
>>> +    void *bar0;
>> 
>> MemoryRegion *bar0
> 
> If I uses " MemoryRegion *bar0"  or " MemoryRegion bar0" then how the size and address of bar0 will be populated?

If I understand Avi's comments correctly, by creating a memory region alias. Who would be responsible for creating that alias and how that alias would come to be in this struct is still puzzling myself too though :).

Avi?

> 
> As far as I can see, other PCI devices using this way to have extra information but they does not get MemoryRegion information from other object.
> 
>> 
>>> +};
>>> +
>>> +typedef struct BHARAT bharat;
>>> typedef struct PPCE500PCIState PPCE500PCIState;
>>> 
>>> static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
>>> @@ -307,6 +313,16 @@ static const VMStateDescription
>>> vmstate_ppce500_pci = {
>>> 
>>> #include "exec-memory.h"
>>> 
>>> +static int e500_pcihost_bridge_initfn(PCIDevice *d) {
>>> +    bharat *b = DO_UPCAST(bharat, p, d);
>>> +
>>> +    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr,
>> (unsigned long long)int128_get64(((Me
>>> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>> + (MemoryRegion *)b->bar0);
>> 
>> That one still has to call its parent initfn, no?
> 
> I am really sorry, but I did not get. The object says its parent is TYPE_PCI_DEVICE, so which function are you talking about?

In object oriented programming, every time you overload a class, your constructor should call the overloaded class's constructor. I don't see this happening for other PCI device's init functions though.

Andreas, shouldn't parent class init be called for pci subclass devices?

> 
>> 
>>> +    return 0;
>>> +}
>>> +
>>> +MemoryRegion ccsr;
>>> static int e500_pcihost_initfn(SysBusDevice *dev) {
>>>    PCIHostState *h;
>>> @@ -315,6 +331,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>>>    int i;
>>>    MemoryRegion *address_space_mem = get_system_memory();
>>>    MemoryRegion *address_space_io = get_system_io();
>>> +    PCIDevice *d;
>>> 
>>>    h = PCI_HOST_BRIDGE(dev);
>>>    s = PPC_E500_PCI_HOST_BRIDGE(dev); @@ -328,7 +345,12 @@ static int
>>> e500_pcihost_initfn(SysBusDevice *dev)
>>>                         address_space_io, PCI_DEVFN(0x11, 0), 4);
>>>    h->bus = b;
>>> 
>>> -    pci_create_simple(b, 0, "e500-host-bridge");
>>> +    d = pci_create(b, 0, "e500-host-bridge");
>>> +    /* ccsr-> should be passed from hw/ppc/e500.c */
>>> +    ccsr.addr = 0xE0000000;
>>> +    ccsr.size = int128_make64(0x00100000);
>> 
>> What is this?
> 
> I am trying to pass the CCSR information to "e500-host-bridge". This is currently hardcoded, but finally this will come from hw/ppc/e500.c.

hw/ppc/e500.c owns the CCSR memory region and maps the CCSR memory region. In Andreas' model, the whole CCSR region is a separate object. The PCI host device should only ever take the memory region it gets from the CCSR device (or hw/ppc/e500.c for now) and map that as BAR0. No need to know any details about it.


Alex

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

[Andreas Färber]

Am 08.10.2012 10:50, schrieb Alexander Graf:
> 
> On 08.10.2012, at 10:23, Bhushan Bharat-R65777 wrote:
>>>> @@ -307,6 +313,16 @@ static const VMStateDescription
>>>> vmstate_ppce500_pci = {
>>>>
>>>> #include "exec-memory.h"
>>>>
>>>> +static int e500_pcihost_bridge_initfn(PCIDevice *d) {
>>>> +    bharat *b = DO_UPCAST(bharat, p, d);
>>>> +
>>>> +    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr,
>>> (unsigned long long)int128_get64(((Me
>>>> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>>> + (MemoryRegion *)b->bar0);
>>>
>>> That one still has to call its parent initfn, no?
>>
>> I am really sorry, but I did not get. The object says its parent is TYPE_PCI_DEVICE, so which function are you talking about?
> 
> In object oriented programming, every time you overload a class, your constructor should call the overloaded class's constructor. I don't see this happening for other PCI device's init functions though.
> 
> Andreas, shouldn't parent class init be called for pci subclass devices?

.class_init and .instance_init are called iteratively through the
hierarchy, so no parent method needs to be called manually. This is
different for user-coded methods like CPUState::reset. qdev initfns
haven't changed in that respect to date - they are orthogonal to QOM.

Andreas

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