[Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Alistair Francis]

This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
first does a check to make sure no other CPUs exist and if
they do the Cortex-A9 won't be added. This is implemented to
maintain compatibility and can be removed once all machines
have been updated

This patch also allows the midr and reset-property to be set

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
There comments in the code explaining the reason that the CPU
is initiated in the realize function. This is because it relies
on the num_cpu property, which isn't yet set in the initfn
Is this an acceptable compromise?

 hw/cpu/a9mpcore.c         |   43 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/cpu/a9mpcore.h |    4 ++++
 2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index c09358c..1159044 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
 {
     A9MPPrivState *s = A9MPCORE_PRIV(obj);
 
+    /* Ideally would init the CPUs here, but the num_cpu property has not been
+     * set yet. So that only works if assuming a single CPU
+     * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_ARM_CPU);
+     * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
+     */
+
     memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
 
@@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     Error *err = NULL;
     int i;
 
+    /* Just a temporary measure to not break machines that init the CPU
+     * seperatly */
+    if (!first_cpu) {
+        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);
+        for (i = 0; i < s->num_cpu; i++) {
+            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),
+                              "cortex-a9-" TYPE_ARM_CPU);
+
+            if (s->midr) {
+                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
+                                        "midr", &err);
+                if (err) {
+                    error_propagate(errp, err);
+                    exit(1);
+                }
+            }
+            if (s->reset_cbar) {
+                object_property_set_int(OBJECT((s->cpu + i)), s->reset_cbar,
+                                        "reset-cbar", &err);
+                if (err) {
+                    error_propagate(errp, err);
+                    exit(1);
+                }
+            }
+            object_property_set_bool(OBJECT((s->cpu + i)), true,
+                                     "realized", &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
+        }
+        g_free(s->cpu);
+    }
+
     scudev = DEVICE(&s->scu);
     qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
     object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
@@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
      * Other boards may differ and should set this property appropriately.
      */
     DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
+    /* Properties for the A9 CPU */
+    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
+    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
index 5d67ca2..8e395a4 100644
--- a/include/hw/cpu/a9mpcore.h
+++ b/include/hw/cpu/a9mpcore.h
@@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
     MemoryRegion container;
     uint32_t num_irq;
 
+    ARMCPU *cpu;
+    uint32_t midr;
+    uint64_t reset_cbar;
+
     A9SCUState scu;
     GICState gic;
     A9GTimerState gtimer;
-- 
1.7.1

[Qemu-devel] [RFC v1 2/2] zynq: Update Zynq to init the CPU in the a9mpcore device

[Alistair Francis]

This patch removes the initialisation of the ARM Cortex-A9
in Zynq and instead allows the a9mpcore device to init the
CPU. This also updates components that rely on the CPU
and GIC, as they are now initialised in a slightly different
way

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
All other Cortex-A9 machines can be updated a similar way

This patch breaks the AArch64 make check tests. I get a:
'Warning: "-global dynamic-prop-type-bad.prop3=103" not used'
followed by a broken pipe and failure.
Any hints on what would be causing this?

 hw/arm/xilinx_zynq.c |   63 +++++++++++++++++++++++--------------------------
 1 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index ba5aa82..5a4ce5c 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -26,6 +26,7 @@
 #include "hw/loader.h"
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
+#include "hw/cpu/a9mpcore.h"
 
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
@@ -104,12 +105,10 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
 static void zynq_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
-    const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
-    ObjectClass *cpu_oc;
-    ARMCPU *cpu;
+    A9MPPrivState *mpcore;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
@@ -119,30 +118,6 @@ static void zynq_init(MachineState *machine)
     Error *err = NULL;
     int n;
 
-    if (!cpu_model) {
-        cpu_model = "cortex-a9";
-    }
-    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
-
-    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
-
-    object_property_set_int(OBJECT(cpu), ZYNQ_BOARD_MIDR, "midr", &err);
-    if (err) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
-
-    object_property_set_int(OBJECT(cpu), MPCORE_PERIPHBASE, "reset-cbar", &err);
-    if (err) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
-    if (err) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
-
     /* max 2GB ram */
     if (ram_size > 0x80000000) {
         ram_size = 0x80000000;
@@ -171,16 +146,38 @@ static void zynq_init(MachineState *machine)
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
 
-    dev = qdev_create(NULL, "a9mpcore_priv");
-    qdev_prop_set_uint32(dev, "num-cpu", 1);
-    qdev_init_nofail(dev);
-    busdev = SYS_BUS_DEVICE(dev);
+    mpcore = A9MPCORE_PRIV(object_new("a9mpcore_priv"));
+    object_property_set_int(OBJECT(mpcore), 1, "num-cpu",
+                            &err);
+    if (err) {
+        error_report("%s", error_get_pretty(err));
+        exit(1);
+    }
+    object_property_set_int(OBJECT(mpcore), ZYNQ_BOARD_MIDR, "midr",
+                            &err);
+    if (err) {
+        error_report("%s", error_get_pretty(err));
+        exit(1);
+    }
+    object_property_set_int(OBJECT(mpcore), MPCORE_PERIPHBASE,
+                            "reset-cbar", &err);
+    if (err) {
+        error_report("%s", error_get_pretty(err));
+        exit(1);
+    }
+    object_property_set_bool(OBJECT(mpcore), true, "realized", &err);
+    if (err != NULL) {
+        error_report("Couldn't realize the Zynq A9MPCore: %s",
+                     error_get_pretty(err));
+        exit(1);
+    }
+    busdev = SYS_BUS_DEVICE(DEVICE(mpcore));
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
     sysbus_connect_irq(busdev, 0,
-                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
+                       qdev_get_gpio_in(DEVICE(mpcore->cpu), ARM_CPU_IRQ));
 
     for (n = 0; n < 64; n++) {
-        pic[n] = qdev_get_gpio_in(dev, n);
+        pic[n] = qdev_get_gpio_in(DEVICE(mpcore), n);
     }
 
     zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
-- 
1.7.1

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Alistair Francis]

Ping

On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
> first does a check to make sure no other CPUs exist and if
> they do the Cortex-A9 won't be added. This is implemented to
> maintain compatibility and can be removed once all machines
> have been updated
>
> This patch also allows the midr and reset-property to be set
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> There comments in the code explaining the reason that the CPU
> is initiated in the realize function. This is because it relies
> on the num_cpu property, which isn't yet set in the initfn
> Is this an acceptable compromise?
>
>  hw/cpu/a9mpcore.c         |   43 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/cpu/a9mpcore.h |    4 ++++
>  2 files changed, 47 insertions(+), 0 deletions(-)
>
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index c09358c..1159044 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
>  {
>      A9MPPrivState *s = A9MPCORE_PRIV(obj);
>
> +    /* Ideally would init the CPUs here, but the num_cpu property has not been
> +     * set yet. So that only works if assuming a single CPU
> +     * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_ARM_CPU);
> +     * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> +     */
> +
>      memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
>
> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>      Error *err = NULL;
>      int i;
>
> +    /* Just a temporary measure to not break machines that init the CPU
> +     * seperatly */
> +    if (!first_cpu) {
> +        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);
> +        for (i = 0; i < s->num_cpu; i++) {
> +            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),
> +                              "cortex-a9-" TYPE_ARM_CPU);
> +
> +            if (s->midr) {
> +                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
> +                                        "midr", &err);
> +                if (err) {
> +                    error_propagate(errp, err);
> +                    exit(1);
> +                }
> +            }
> +            if (s->reset_cbar) {
> +                object_property_set_int(OBJECT((s->cpu + i)), s->reset_cbar,
> +                                        "reset-cbar", &err);
> +                if (err) {
> +                    error_propagate(errp, err);
> +                    exit(1);
> +                }
> +            }
> +            object_property_set_bool(OBJECT((s->cpu + i)), true,
> +                                     "realized", &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
> +            }
> +        }
> +        g_free(s->cpu);
> +    }
> +
>      scudev = DEVICE(&s->scu);
>      qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
>      object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
>       * Other boards may differ and should set this property appropriately.
>       */
>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
> +    /* Properties for the A9 CPU */
> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
> index 5d67ca2..8e395a4 100644
> --- a/include/hw/cpu/a9mpcore.h
> +++ b/include/hw/cpu/a9mpcore.h
> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
>      MemoryRegion container;
>      uint32_t num_irq;
>
> +    ARMCPU *cpu;
> +    uint32_t midr;
> +    uint64_t reset_cbar;
> +
>      A9SCUState scu;
>      GICState gic;
>      A9GTimerState gtimer;
> --
> 1.7.1
>

Re: [Qemu-devel] [RFC v1 2/2] zynq: Update Zynq to init the CPU in the a9mpcore device

[Peter Crosthwaite]

On Tue, Jun 10, 2014 at 11:33 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch removes the initialisation of the ARM Cortex-A9
> in Zynq and instead allows the a9mpcore device to init the
> CPU. This also updates components that rely on the CPU
> and GIC, as they are now initialised in a slightly different
> way
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> All other Cortex-A9 machines can be updated a similar way
>
> This patch breaks the AArch64 make check tests. I get a:
> 'Warning: "-global dynamic-prop-type-bad.prop3=103" not used'
> followed by a broken pipe and failure.
> Any hints on what would be causing this?
>
>  hw/arm/xilinx_zynq.c |   63 +++++++++++++++++++++++--------------------------
>  1 files changed, 30 insertions(+), 33 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index ba5aa82..5a4ce5c 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -26,6 +26,7 @@
>  #include "hw/loader.h"
>  #include "hw/ssi.h"
>  #include "qemu/error-report.h"
> +#include "hw/cpu/a9mpcore.h"
>
>  #define NUM_SPI_FLASHES 4
>  #define NUM_QSPI_FLASHES 2
> @@ -104,12 +105,10 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>  static void zynq_init(MachineState *machine)
>  {
>      ram_addr_t ram_size = machine->ram_size;
> -    const char *cpu_model = machine->cpu_model;
>      const char *kernel_filename = machine->kernel_filename;
>      const char *kernel_cmdline = machine->kernel_cmdline;
>      const char *initrd_filename = machine->initrd_filename;
> -    ObjectClass *cpu_oc;
> -    ARMCPU *cpu;
> +    A9MPPrivState *mpcore;
>      MemoryRegion *address_space_mem = get_system_memory();
>      MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
>      MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
> @@ -119,30 +118,6 @@ static void zynq_init(MachineState *machine)
>      Error *err = NULL;
>      int n;
>
> -    if (!cpu_model) {
> -        cpu_model = "cortex-a9";
> -    }

So this defeatures the cpu_model override. That's a good thing, but
it's worthwhile to leave a check behind explaining to the user that
the feature no longer exists:

if (machine->cpu_model) {
    error_report("Zynq does not support CPU model override!\n";
    exit(1);
}

> -    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
> -
> -    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
> -
> -    object_property_set_int(OBJECT(cpu), ZYNQ_BOARD_MIDR, "midr", &err);
> -    if (err) {
> -        error_report("%s", error_get_pretty(err));
> -        exit(1);
> -    }
> -
> -    object_property_set_int(OBJECT(cpu), MPCORE_PERIPHBASE, "reset-cbar", &err);
> -    if (err) {
> -        error_report("%s", error_get_pretty(err));
> -        exit(1);
> -    }
> -    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> -    if (err) {
> -        error_report("%s", error_get_pretty(err));
> -        exit(1);
> -    }
> -
>      /* max 2GB ram */
>      if (ram_size > 0x80000000) {
>          ram_size = 0x80000000;
> @@ -171,16 +146,38 @@ static void zynq_init(MachineState *machine)
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
>
> -    dev = qdev_create(NULL, "a9mpcore_priv");
> -    qdev_prop_set_uint32(dev, "num-cpu", 1);
> -    qdev_init_nofail(dev);
> -    busdev = SYS_BUS_DEVICE(dev);
> +    mpcore = A9MPCORE_PRIV(object_new("a9mpcore_priv"));
> +    object_property_set_int(OBJECT(mpcore), 1, "num-cpu",
> +                            &err);
> +    if (err) {
> +        error_report("%s", error_get_pretty(err));
> +        exit(1);
> +    }
> +    object_property_set_int(OBJECT(mpcore), ZYNQ_BOARD_MIDR, "midr",
> +                            &err);
> +    if (err) {
> +        error_report("%s", error_get_pretty(err));
> +        exit(1);
> +    }
> +    object_property_set_int(OBJECT(mpcore), MPCORE_PERIPHBASE,
> +                            "reset-cbar", &err);
> +    if (err) {
> +        error_report("%s", error_get_pretty(err));
> +        exit(1);
> +    }
> +    object_property_set_bool(OBJECT(mpcore), true, "realized", &err);
> +    if (err != NULL) {
> +        error_report("Couldn't realize the Zynq A9MPCore: %s",
> +                     error_get_pretty(err));
> +        exit(1);
> +    }

Can we just use the qdev_prop setters to cut down on the error boilerplate?

> +    busdev = SYS_BUS_DEVICE(DEVICE(mpcore));
>      sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
>      sysbus_connect_irq(busdev, 0,
> -                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
> +                       qdev_get_gpio_in(DEVICE(mpcore->cpu), ARM_CPU_IRQ));
>

Mpcore should now be responsible for connecting GIC to CPU. This
should go away for board that use MPCore driven CPU instantiation.

Regards,
Peter

>      for (n = 0; n < 64; n++) {
> -        pic[n] = qdev_get_gpio_in(dev, n);
> +        pic[n] = qdev_get_gpio_in(DEVICE(mpcore), n);
>      }
>
>      zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
> --
> 1.7.1
>
>

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Crosthwaite]

On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
> first does a check to make sure no other CPUs exist and if
> they do the Cortex-A9 won't be added. This is implemented to
> maintain compatibility and can be removed once all machines
> have been updated
>
> This patch also allows the midr and reset-property to be set
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> There comments in the code explaining the reason that the CPU
> is initiated in the realize function. This is because it relies
> on the num_cpu property, which isn't yet set in the initfn
> Is this an acceptable compromise?
>
>  hw/cpu/a9mpcore.c         |   43 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/cpu/a9mpcore.h |    4 ++++
>  2 files changed, 47 insertions(+), 0 deletions(-)
>
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index c09358c..1159044 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
>  {
>      A9MPPrivState *s = A9MPCORE_PRIV(obj);
>
> +    /* Ideally would init the CPUs here, but the num_cpu property has not been
> +     * set yet. So that only works if assuming a single CPU
> +     * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_ARM_CPU);
> +     * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> +     */
> +

So you could add an integer property listener to init them earlier (or
even do dynamic extending/freeing or the allocated CPUs). I'm not sure
exactly what we are really supposed to do though, when the number of
child object depends on a prop like this? Andreas?

>      memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
>
> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>      Error *err = NULL;
>      int i;
>
> +    /* Just a temporary measure to not break machines that init the CPU
> +     * seperatly */

"separately"

> +    if (!first_cpu) {
> +        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);

g_new should be use to allocate arrays.

> +        for (i = 0; i < s->num_cpu; i++) {
> +            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),

&s->cpu[i] is more common and easier to read.

sizeof(*s->cpu) is fine.

> +                              "cortex-a9-" TYPE_ARM_CPU);

Use cpu_class_by_name logic like in some of the boards, rather than
the string concatenation. The specifics of the concatenation system is
(supposed to be) private to target-arm code.

> +
> +            if (s->midr) {
> +                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
> +                                        "midr", &err);
> +                if (err) {
> +                    error_propagate(errp, err);
> +                    exit(1);
> +                }
> +            }
> +            if (s->reset_cbar) {
> +                object_property_set_int(OBJECT((s->cpu + i)), s->reset_cbar,
> +                                        "reset-cbar", &err);
> +                if (err) {
> +                    error_propagate(errp, err);
> +                    exit(1);
> +                }
> +            }
> +            object_property_set_bool(OBJECT((s->cpu + i)), true,
> +                                     "realized", &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
> +            }
> +        }
> +        g_free(s->cpu);

Why free the just-initialized CPUs?

> +    }
> +
>      scudev = DEVICE(&s->scu);
>      qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
>      object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
>       * Other boards may differ and should set this property appropriately.
>       */
>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
> +    /* Properties for the A9 CPU */
> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
> index 5d67ca2..8e395a4 100644
> --- a/include/hw/cpu/a9mpcore.h
> +++ b/include/hw/cpu/a9mpcore.h
> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
>      MemoryRegion container;
>      uint32_t num_irq;
>
> +    ARMCPU *cpu;
> +    uint32_t midr;

I'd preface this as "cpu_midr".

> +    uint64_t reset_cbar;

MPCores refer to this as PERIPHBASE in their documentation.

Regards,
Peter

> +
>      A9SCUState scu;
>      GICState gic;
>      A9GTimerState gtimer;
> --
> 1.7.1
>
>

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Alistair Francis]

On Mon, Jun 16, 2014 at 2:43 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
>> first does a check to make sure no other CPUs exist and if
>> they do the Cortex-A9 won't be added. This is implemented to
>> maintain compatibility and can be removed once all machines
>> have been updated
>>
>> This patch also allows the midr and reset-property to be set
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> There comments in the code explaining the reason that the CPU
>> is initiated in the realize function. This is because it relies
>> on the num_cpu property, which isn't yet set in the initfn
>> Is this an acceptable compromise?
>>
>>  hw/cpu/a9mpcore.c         |   43 +++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/cpu/a9mpcore.h |    4 ++++
>>  2 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
>> index c09358c..1159044 100644
>> --- a/hw/cpu/a9mpcore.c
>> +++ b/hw/cpu/a9mpcore.c
>> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
>>  {
>>      A9MPPrivState *s = A9MPCORE_PRIV(obj);
>>
>> +    /* Ideally would init the CPUs here, but the num_cpu property has not been
>> +     * set yet. So that only works if assuming a single CPU
>> +     * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_ARM_CPU);
>> +     * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>> +     */
>> +
>
> So you could add an integer property listener to init them earlier (or
> even do dynamic extending/freeing or the allocated CPUs). I'm not sure
> exactly what we are really supposed to do though, when the number of
> child object depends on a prop like this? Andreas?

I'm open for ideas/opinions. The method used here seemed to be the easiest
to implement (and actually the only reliable method that I could think of).

>
>>      memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
>>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
>>
>> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>>      Error *err = NULL;
>>      int i;
>>
>> +    /* Just a temporary measure to not break machines that init the CPU
>> +     * seperatly */
>
> "separately"
>
>> +    if (!first_cpu) {
>> +        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);
>
> g_new should be use to allocate arrays.
>
>> +        for (i = 0; i < s->num_cpu; i++) {
>> +            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),
>
> &s->cpu[i] is more common and easier to read.
>
> sizeof(*s->cpu) is fine.
>
>> +                              "cortex-a9-" TYPE_ARM_CPU);
>
> Use cpu_class_by_name logic like in some of the boards, rather than
> the string concatenation. The specifics of the concatenation system is
> (supposed to be) private to target-arm code.
>
>> +
>> +            if (s->midr) {
>> +                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
>> +                                        "midr", &err);
>> +                if (err) {
>> +                    error_propagate(errp, err);
>> +                    exit(1);
>> +                }
>> +            }
>> +            if (s->reset_cbar) {
>> +                object_property_set_int(OBJECT((s->cpu + i)), s->reset_cbar,
>> +                                        "reset-cbar", &err);
>> +                if (err) {
>> +                    error_propagate(errp, err);
>> +                    exit(1);
>> +                }
>> +            }
>> +            object_property_set_bool(OBJECT((s->cpu + i)), true,
>> +                                     "realized", &err);
>> +            if (err) {
>> +                error_propagate(errp, err);
>> +                return;
>> +            }
>> +        }
>> +        g_free(s->cpu);
>
> Why free the just-initialized CPUs?

I shouldn't have done that, I don't know how that slipped through

>
>> +    }
>> +
>>      scudev = DEVICE(&s->scu);
>>      qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
>>      object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
>> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
>>       * Other boards may differ and should set this property appropriately.
>>       */
>>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
>> +    /* Properties for the A9 CPU */
>> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
>> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
>> index 5d67ca2..8e395a4 100644
>> --- a/include/hw/cpu/a9mpcore.h
>> +++ b/include/hw/cpu/a9mpcore.h
>> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
>>      MemoryRegion container;
>>      uint32_t num_irq;
>>
>> +    ARMCPU *cpu;
>> +    uint32_t midr;
>
> I'd preface this as "cpu_midr".
>
>> +    uint64_t reset_cbar;
>
> MPCores refer to this as PERIPHBASE in their documentation.
>
> Regards,
> Peter

All comments were changed. I'll give it a few days and see if anyone else
has any comments, otherwise I might release a patch following the same
style as this RFC

>
>> +
>>      A9SCUState scu;
>>      GICState gic;
>>      A9GTimerState gtimer;
>> --
>> 1.7.1
>>
>>
>

Re: [Qemu-devel] [RFC v1 2/2] zynq: Update Zynq to init the CPU in the a9mpcore device

[Alistair Francis]

On Mon, Jun 16, 2014 at 2:42 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 10, 2014 at 11:33 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> This patch removes the initialisation of the ARM Cortex-A9
>> in Zynq and instead allows the a9mpcore device to init the
>> CPU. This also updates components that rely on the CPU
>> and GIC, as they are now initialised in a slightly different
>> way
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> All other Cortex-A9 machines can be updated a similar way
>>
>> This patch breaks the AArch64 make check tests. I get a:
>> 'Warning: "-global dynamic-prop-type-bad.prop3=103" not used'
>> followed by a broken pipe and failure.
>> Any hints on what would be causing this?
>>
>>  hw/arm/xilinx_zynq.c |   63 +++++++++++++++++++++++--------------------------
>>  1 files changed, 30 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> index ba5aa82..5a4ce5c 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -26,6 +26,7 @@
>>  #include "hw/loader.h"
>>  #include "hw/ssi.h"
>>  #include "qemu/error-report.h"
>> +#include "hw/cpu/a9mpcore.h"
>>
>>  #define NUM_SPI_FLASHES 4
>>  #define NUM_QSPI_FLASHES 2
>> @@ -104,12 +105,10 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>>  static void zynq_init(MachineState *machine)
>>  {
>>      ram_addr_t ram_size = machine->ram_size;
>> -    const char *cpu_model = machine->cpu_model;
>>      const char *kernel_filename = machine->kernel_filename;
>>      const char *kernel_cmdline = machine->kernel_cmdline;
>>      const char *initrd_filename = machine->initrd_filename;
>> -    ObjectClass *cpu_oc;
>> -    ARMCPU *cpu;
>> +    A9MPPrivState *mpcore;
>>      MemoryRegion *address_space_mem = get_system_memory();
>>      MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
>>      MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
>> @@ -119,30 +118,6 @@ static void zynq_init(MachineState *machine)
>>      Error *err = NULL;
>>      int n;
>>
>> -    if (!cpu_model) {
>> -        cpu_model = "cortex-a9";
>> -    }
>
> So this defeatures the cpu_model override. That's a good thing, but
> it's worthwhile to leave a check behind explaining to the user that
> the feature no longer exists:
>
> if (machine->cpu_model) {
>     error_report("Zynq does not support CPU model override!\n";
>     exit(1);
> }
>

Good idea, added!

>> -    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
>> -
>> -    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
>> -
>> -    object_property_set_int(OBJECT(cpu), ZYNQ_BOARD_MIDR, "midr", &err);
>> -    if (err) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> -    }
>> -
>> -    object_property_set_int(OBJECT(cpu), MPCORE_PERIPHBASE, "reset-cbar", &err);
>> -    if (err) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> -    }
>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>> -    if (err) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> -    }
>> -
>>      /* max 2GB ram */
>>      if (ram_size > 0x80000000) {
>>          ram_size = 0x80000000;
>> @@ -171,16 +146,38 @@ static void zynq_init(MachineState *machine)
>>      qdev_init_nofail(dev);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
>>
>> -    dev = qdev_create(NULL, "a9mpcore_priv");
>> -    qdev_prop_set_uint32(dev, "num-cpu", 1);
>> -    qdev_init_nofail(dev);
>> -    busdev = SYS_BUS_DEVICE(dev);
>> +    mpcore = A9MPCORE_PRIV(object_new("a9mpcore_priv"));
>> +    object_property_set_int(OBJECT(mpcore), 1, "num-cpu",
>> +                            &err);
>> +    if (err) {
>> +        error_report("%s", error_get_pretty(err));
>> +        exit(1);
>> +    }
>> +    object_property_set_int(OBJECT(mpcore), ZYNQ_BOARD_MIDR, "midr",
>> +                            &err);
>> +    if (err) {
>> +        error_report("%s", error_get_pretty(err));
>> +        exit(1);
>> +    }
>> +    object_property_set_int(OBJECT(mpcore), MPCORE_PERIPHBASE,
>> +                            "reset-cbar", &err);
>> +    if (err) {
>> +        error_report("%s", error_get_pretty(err));
>> +        exit(1);
>> +    }
>> +    object_property_set_bool(OBJECT(mpcore), true, "realized", &err);
>> +    if (err != NULL) {
>> +        error_report("Couldn't realize the Zynq A9MPCore: %s",
>> +                     error_get_pretty(err));
>> +        exit(1);
>> +    }
>
> Can we just use the qdev_prop setters to cut down on the error boilerplate?
>

Yep, fixed

>> +    busdev = SYS_BUS_DEVICE(DEVICE(mpcore));
>>      sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
>>      sysbus_connect_irq(busdev, 0,
>> -                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
>> +                       qdev_get_gpio_in(DEVICE(mpcore->cpu), ARM_CPU_IRQ));
>>
>
> Mpcore should now be responsible for connecting GIC to CPU. This
> should go away for board that use MPCore driven CPU instantiation.
>

Ok, done

Also, something here causes check-qtest-aarch64 to fail. Any ideas what
that would be/how to fix it?

> Regards,
> Peter
>
>>      for (n = 0; n < 64; n++) {
>> -        pic[n] = qdev_get_gpio_in(dev, n);
>> +        pic[n] = qdev_get_gpio_in(DEVICE(mpcore), n);
>>      }
>>
>>      zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
>> --
>> 1.7.1
>>
>>
>

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Maydell]

On 10 June 2014 02:32, Alistair Francis <alistair.francis@xilinx.com> wrote:
> This patch adds the Cortex-A9 ARM CPU to the A9MPCore.

I think this is in general the right way to go.

> +    /* Properties for the A9 CPU */
> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),

It seems a bit error-prone if every container object has to manually
mirror the properties of the individual CPUs out like this; maybe
there's a better way we could come up with?

Paolo, weren't you looking at passthrough properties for the
virtio devices?

thanks
-- PMM

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Crosthwaite]

On Mon, Jun 16, 2014 at 5:40 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 June 2014 02:32, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> This patch adds the Cortex-A9 ARM CPU to the A9MPCore.
>
> I think this is in general the right way to go.
>
>> +    /* Properties for the A9 CPU */
>> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
>> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>
> It seems a bit error-prone if every container object has to manually
> mirror the properties of the individual CPUs out like this; maybe
> there's a better way we could come up with?
>
> Paolo, weren't you looking at passthrough properties for the
> virtio devices?
>

Stefan added support for QDEV property aliasing.

http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg06489.html

The bigger issue though is how do you do an N:1 mapping. The container
should only have 1 "midr" prop, but it should mirror to all contained
CPUs. Should we add multiplicity to the aliasing feature?

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Andreas Färber]

Am 16.06.2014 06:43, schrieb Peter Crosthwaite:
> On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
>> first does a check to make sure no other CPUs exist and if
>> they do the Cortex-A9 won't be added. This is implemented to
>> maintain compatibility and can be removed once all machines
>> have been updated
>>
>> This patch also allows the midr and reset-property to be set
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> There comments in the code explaining the reason that the CPU
>> is initiated in the realize function. This is because it relies
>> on the num_cpu property, which isn't yet set in the initfn
>> Is this an acceptable compromise?

No, this is not OK as is. The CPUs need to be initialized before
realizing them through the parent, otherwise the CPUs can't have any
additional properties set by user/management.

>>
>>  hw/cpu/a9mpcore.c         |   43 +++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/cpu/a9mpcore.h |    4 ++++
>>  2 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
>> index c09358c..1159044 100644
>> --- a/hw/cpu/a9mpcore.c
>> +++ b/hw/cpu/a9mpcore.c
>> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
>>  {
>>      A9MPPrivState *s = A9MPCORE_PRIV(obj);
>>
>> +    /* Ideally would init the CPUs here, but the num_cpu property has not been
>> +     * set yet. So that only works if assuming a single CPU
>> +     * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_ARM_CPU);
>> +     * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>> +     */
>> +
> 
> So you could add an integer property listener to init them earlier (or
> even do dynamic extending/freeing or the allocated CPUs). I'm not sure
> exactly what we are really supposed to do though, when the number of
> child object depends on a prop like this? Andreas?

PMM had added or looked into a property creating an array of properties.
However a more fundamental issue that PMM was unsure about is whether
the CPUs should be child<> of MPCore as done here or a sibling of the
MPCore container.

>>      memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
>>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
>>
>> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>>      Error *err = NULL;
>>      int i;
>>
>> +    /* Just a temporary measure to not break machines that init the CPU
>> +     * seperatly */
> 
> "separately"
> 
>> +    if (!first_cpu) {
>> +        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);
> 
> g_new should be use to allocate arrays.

Whether malloc or new, more important is to 0-initialize the memory
before running object_initialize() on it! :)

> 
>> +        for (i = 0; i < s->num_cpu; i++) {
>> +            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),
> 
> &s->cpu[i] is more common and easier to read.
> 
> sizeof(*s->cpu) is fine.
> 
>> +                              "cortex-a9-" TYPE_ARM_CPU);
> 
> Use cpu_class_by_name logic like in some of the boards, rather than
> the string concatenation. The specifics of the concatenation system is
> (supposed to be) private to target-arm code.

+1

> 
>> +
>> +            if (s->midr) {
>> +                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
>> +                                        "midr", &err);
>> +                if (err) {
>> +                    error_propagate(errp, err);
>> +                    exit(1);
>> +                }
>> +            }
>> +            if (s->reset_cbar) {
>> +                object_property_set_int(OBJECT((s->cpu + i)), s->reset_cbar,
>> +                                        "reset-cbar", &err);
>> +                if (err) {
>> +                    error_propagate(errp, err);
>> +                    exit(1);
>> +                }
>> +            }
>> +            object_property_set_bool(OBJECT((s->cpu + i)), true,
>> +                                     "realized", &err);
>> +            if (err) {
>> +                error_propagate(errp, err);
>> +                return;
>> +            }
>> +        }
>> +        g_free(s->cpu);
> 
> Why free the just-initialized CPUs?
> 
>> +    }
>> +
>>      scudev = DEVICE(&s->scu);
>>      qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
>>      object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
>> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
>>       * Other boards may differ and should set this property appropriately.
>>       */
>>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
>> +    /* Properties for the A9 CPU */
>> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
>> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
>> index 5d67ca2..8e395a4 100644
>> --- a/include/hw/cpu/a9mpcore.h
>> +++ b/include/hw/cpu/a9mpcore.h
>> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
>>      MemoryRegion container;
>>      uint32_t num_irq;
>>
>> +    ARMCPU *cpu;
>> +    uint32_t midr;
> 
> I'd preface this as "cpu_midr".
> 
>> +    uint64_t reset_cbar;
> 
> MPCores refer to this as PERIPHBASE in their documentation.

Why have this as separate state at all? Can't those properties be passed
through to the CPUs once created earlier? When the CPUs are not
available, setter can return an Error.

Regards,
Andreas

>> +
>>      A9SCUState scu;
>>      GICState gic;
>>      A9GTimerState gtimer;
>> --
>> 1.7.1

-- 
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] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Andreas Färber]

Am 16.06.2014 08:04, schrieb Alistair Francis:
> On Mon, Jun 16, 2014 at 2:43 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
>>> first does a check to make sure no other CPUs exist and if
>>> they do the Cortex-A9 won't be added. This is implemented to
>>> maintain compatibility and can be removed once all machines
>>> have been updated
>>>
>>> This patch also allows the midr and reset-property to be set
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>> There comments in the code explaining the reason that the CPU
>>> is initiated in the realize function. This is because it relies
>>> on the num_cpu property, which isn't yet set in the initfn
>>> Is this an acceptable compromise?
>>>
>>>  hw/cpu/a9mpcore.c         |   43 +++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/cpu/a9mpcore.h |    4 ++++
>>>  2 files changed, 47 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
>>> index c09358c..1159044 100644
>>> --- a/hw/cpu/a9mpcore.c
>>> +++ b/hw/cpu/a9mpcore.c
>>> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
>>>  {
>>>      A9MPPrivState *s = A9MPCORE_PRIV(obj);
>>>
>>> +    /* Ideally would init the CPUs here, but the num_cpu property has not been
>>> +     * set yet. So that only works if assuming a single CPU
>>> +     * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_ARM_CPU);
>>> +     * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>>> +     */
>>> +
>>
>> So you could add an integer property listener to init them earlier (or
>> even do dynamic extending/freeing or the allocated CPUs). I'm not sure
>> exactly what we are really supposed to do though, when the number of
>> child object depends on a prop like this? Andreas?
> 
> I'm open for ideas/opinions. The method used here seemed to be the easiest
> to implement (and actually the only reliable method that I could think of).

The point is that it needs to happen before realize, so that the object
can be inspected and modified before it's too late, and allocations may
fail so should be able to return an error to the caller. If there is
nothing that suits your needs, you can either implement a new type of
static qdev property or, easiest, implement a dynamic QOM property that
takes actions once the number-of-CPUs value is set. An example using
dynamic properties may be found around the sPAPR interrupt controllers.

Cheers,
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] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Crosthwaite]

On Mon, Jun 16, 2014 at 8:19 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 16.06.2014 06:43, schrieb Peter Crosthwaite:
>> On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
>>> first does a check to make sure no other CPUs exist and if
>>> they do the Cortex-A9 won't be added. This is implemented to
>>> maintain compatibility and can be removed once all machines
>>> have been updated
>>>
>>> This patch also allows the midr and reset-property to be set
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>> There comments in the code explaining the reason that the CPU
>>> is initiated in the realize function. This is because it relies
>>> on the num_cpu property, which isn't yet set in the initfn
>>> Is this an acceptable compromise?
>
> No, this is not OK as is. The CPUs need to be initialized before
> realizing them through the parent, otherwise the CPUs can't have any
> additional properties set by user/management.
>
>>>
>>>  hw/cpu/a9mpcore.c         |   43 +++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/cpu/a9mpcore.h |    4 ++++
>>>  2 files changed, 47 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
>>> index c09358c..1159044 100644
>>> --- a/hw/cpu/a9mpcore.c
>>> +++ b/hw/cpu/a9mpcore.c
>>> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
>>>  {
>>>      A9MPPrivState *s = A9MPCORE_PRIV(obj);
>>>
>>> +    /* Ideally would init the CPUs here, but the num_cpu property has not been
>>> +     * set yet. So that only works if assuming a single CPU
>>> +     * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_ARM_CPU);
>>> +     * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>>> +     */
>>> +
>>
>> So you could add an integer property listener to init them earlier (or
>> even do dynamic extending/freeing or the allocated CPUs). I'm not sure
>> exactly what we are really supposed to do though, when the number of
>> child object depends on a prop like this? Andreas?
>
> PMM had added or looked into a property creating an array of properties.

I guess it's only a small step to a property that init's an array of
child objects.

> However a more fundamental issue that PMM was unsure about is whether
> the CPUs should be child<> of MPCore as done here or a sibling of the
> MPCore container.
>

I'll go with child. The CPU does not exist outside the MPCore. They
are a heirachy, not-peers and the qom-composition should reflect that.

>>>      memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
>>>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
>>>
>>> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>>>      Error *err = NULL;
>>>      int i;
>>>
>>> +    /* Just a temporary measure to not break machines that init the CPU
>>> +     * seperatly */
>>
>> "separately"
>>
>>> +    if (!first_cpu) {
>>> +        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);
>>
>> g_new should be use to allocate arrays.
>
> Whether malloc or new, more important is to 0-initialize the memory
> before running object_initialize() on it! :)
>
>>
>>> +        for (i = 0; i < s->num_cpu; i++) {
>>> +            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),
>>
>> &s->cpu[i] is more common and easier to read.
>>
>> sizeof(*s->cpu) is fine.
>>
>>> +                              "cortex-a9-" TYPE_ARM_CPU);
>>
>> Use cpu_class_by_name logic like in some of the boards, rather than
>> the string concatenation. The specifics of the concatenation system is
>> (supposed to be) private to target-arm code.
>
> +1
>
>>
>>> +
>>> +            if (s->midr) {
>>> +                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
>>> +                                        "midr", &err);
>>> +                if (err) {
>>> +                    error_propagate(errp, err);
>>> +                    exit(1);
>>> +                }
>>> +            }
>>> +            if (s->reset_cbar) {
>>> +                object_property_set_int(OBJECT((s->cpu + i)), s->reset_cbar,
>>> +                                        "reset-cbar", &err);
>>> +                if (err) {
>>> +                    error_propagate(errp, err);
>>> +                    exit(1);
>>> +                }
>>> +            }
>>> +            object_property_set_bool(OBJECT((s->cpu + i)), true,
>>> +                                     "realized", &err);
>>> +            if (err) {
>>> +                error_propagate(errp, err);
>>> +                return;
>>> +            }
>>> +        }
>>> +        g_free(s->cpu);
>>
>> Why free the just-initialized CPUs?
>>
>>> +    }
>>> +
>>>      scudev = DEVICE(&s->scu);
>>>      qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
>>>      object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
>>> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
>>>       * Other boards may differ and should set this property appropriately.
>>>       */
>>>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
>>> +    /* Properties for the A9 CPU */
>>> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
>>> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
>>> index 5d67ca2..8e395a4 100644
>>> --- a/include/hw/cpu/a9mpcore.h
>>> +++ b/include/hw/cpu/a9mpcore.h
>>> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
>>>      MemoryRegion container;
>>>      uint32_t num_irq;
>>>
>>> +    ARMCPU *cpu;
>>> +    uint32_t midr;
>>
>> I'd preface this as "cpu_midr".
>>
>>> +    uint64_t reset_cbar;
>>
>> MPCores refer to this as PERIPHBASE in their documentation.
>
> Why have this as separate state at all? Can't those properties be passed
> through to the CPUs once created earlier? When the CPUs are not
> available, setter can return an Error.
>

This should evaporate if the alias system is used somehow. No need for
state or a setter fn.

But either way, the alias should do the rename. That way both MPCore
and ARMCPU match their documentation without one have to compromise to
the others naming scheme.

Regards,
Peter

> Regards,
> Andreas
>
>>> +
>>>      A9SCUState scu;
>>>      GICState gic;
>>>      A9GTimerState gtimer;
>>> --
>>> 1.7.1
>
> --
> 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] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Maydell]

On 16 June 2014 11:19, Andreas Färber <afaerber@suse.de> wrote:
> Am 16.06.2014 06:43, schrieb Peter Crosthwaite:
>> So you could add an integer property listener to init them earlier (or
>> even do dynamic extending/freeing or the allocated CPUs). I'm not sure
>> exactly what we are really supposed to do though, when the number of
>> child object depends on a prop like this? Andreas?
>
> PMM had added or looked into a property creating an array of properties.
> However a more fundamental issue that PMM was unsure about is whether
> the CPUs should be child<> of MPCore as done here or a sibling of the
> MPCore container.

Was I? I can't currently see any reason you'd want them to be
siblings of the container rather than children...

I think property-listeners was the mechanism we talked
about for when you need to do something before realize
but it depends on some property, yes.

>>>      memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
>>>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
>>>
>>> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>>>      Error *err = NULL;
>>>      int i;
>>>
>>> +    /* Just a temporary measure to not break machines that init the CPU
>>> +     * seperatly */
>>
>> "separately"
>>
>>> +    if (!first_cpu) {
>>> +        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);
>>
>> g_new should be use to allocate arrays.
>
> Whether malloc or new, more important is to 0-initialize the memory
> before running object_initialize() on it! :)
>
>>
>>> +        for (i = 0; i < s->num_cpu; i++) {
>>> +            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),
>>
>> &s->cpu[i] is more common and easier to read.
>>
>> sizeof(*s->cpu) is fine.
>>
>>> +                              "cortex-a9-" TYPE_ARM_CPU);
>>
>> Use cpu_class_by_name logic like in some of the boards, rather than
>> the string concatenation. The specifics of the concatenation system is
>> (supposed to be) private to target-arm code.
>
> +1
>
>>
>>> +
>>> +            if (s->midr) {
>>> +                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
>>> +                                        "midr", &err);
>>> +                if (err) {
>>> +                    error_propagate(errp, err);
>>> +                    exit(1);
>>> +                }
>>> +            }
>>> +            if (s->reset_cbar) {
>>> +                object_property_set_int(OBJECT((s->cpu + i)), s->reset_cbar,
>>> +                                        "reset-cbar", &err);
>>> +                if (err) {
>>> +                    error_propagate(errp, err);
>>> +                    exit(1);
>>> +                }
>>> +            }
>>> +            object_property_set_bool(OBJECT((s->cpu + i)), true,
>>> +                                     "realized", &err);
>>> +            if (err) {
>>> +                error_propagate(errp, err);
>>> +                return;
>>> +            }
>>> +        }
>>> +        g_free(s->cpu);
>>
>> Why free the just-initialized CPUs?
>>
>>> +    }
>>> +
>>>      scudev = DEVICE(&s->scu);
>>>      qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
>>>      object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
>>> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
>>>       * Other boards may differ and should set this property appropriately.
>>>       */
>>>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
>>> +    /* Properties for the A9 CPU */
>>> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
>>> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
>>> index 5d67ca2..8e395a4 100644
>>> --- a/include/hw/cpu/a9mpcore.h
>>> +++ b/include/hw/cpu/a9mpcore.h
>>> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
>>>      MemoryRegion container;
>>>      uint32_t num_irq;
>>>
>>> +    ARMCPU *cpu;
>>> +    uint32_t midr;
>>
>> I'd preface this as "cpu_midr".
>>
>>> +    uint64_t reset_cbar;
>>
>> MPCores refer to this as PERIPHBASE in their documentation.

Well, generally the external config signal is PERIPHBASE
and the register it affects is CBAR. For QEMU I think
we seem to be generally using CBAR in all contexts.

thanks
-- PMM

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Andreas Färber]

Am 16.06.2014 12:34, schrieb Peter Crosthwaite:
> On Mon, Jun 16, 2014 at 8:19 PM, Andreas Färber <afaerber@suse.de> wrote:
>> However a more fundamental issue that PMM was unsure about is whether
>> the CPUs should be child<> of MPCore as done here or a sibling of the
>> MPCore container.
>>
> 
> I'll go with child. The CPU does not exist outside the MPCore. They
> are a heirachy, not-peers and the qom-composition should reflect that.

Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if
existant by now) should also be refactored alongside, as proof of
concept - can you really create num_cpu cortex-a15 CPUs on the MPCore
for a big.LITTLE configuration? I'd be really surprised if there were
separate MPCore devices per cluster. That would then indicate that the
homogeneity assumption among CPUs within an MPCore is wrong and we need
to let its parent create the CPUs rather than an MPCore property.

Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to
mind, so we should be aware that ARMCPU child<>s on the MPCore will lead
to asymmetry between SoCs. But that shouldn't stop proper Cortex-A9/-A15
modeling, just like Quark and Baytrail SoCs will inevitably lead to
modeling differences in the PC world.

Regards,
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] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Maydell]

On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote:
> Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if
> existant by now) should also be refactored alongside, as proof of
> concept - can you really create num_cpu cortex-a15 CPUs on the MPCore
> for a big.LITTLE configuration? I'd be really surprised if there were
> separate MPCore devices per cluster. That would then indicate that the
> homogeneity assumption among CPUs within an MPCore is wrong and we need
> to let its parent create the CPUs rather than an MPCore property.

Not sure what the relevance of big.LITTLE is here -- QEMU
simply doesn't support heterogenous CPU configurations so
we can't model big.LITTLE at all. If we did, it would be
via having a SoC with one a15mpcore and one a7mpcore.
(This is how the hardware does it -- there are two
multicore clusters, plus some cache coherency interconnect
magic.)

> Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to
> mind, so we should be aware that ARMCPU child<>s on the MPCore will lead
> to asymmetry between SoCs.

For the A8 and A5 the SoC object would just instantiate them
directly -- there's no equivalent in the hardware of the
"n CPUs and their private devices" layer. So I think
any asymmetry between SoCs in QEMU is just a reflection
of the differences in the hardware.

thanks
-- PMM

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Andreas Färber]

Am 16.06.2014 13:11, schrieb Peter Maydell:
> On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote:
>> Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if
>> existant by now) should also be refactored alongside, as proof of
>> concept - can you really create num_cpu cortex-a15 CPUs on the MPCore
>> for a big.LITTLE configuration? I'd be really surprised if there were
>> separate MPCore devices per cluster. That would then indicate that the
>> homogeneity assumption among CPUs within an MPCore is wrong and we need
>> to let its parent create the CPUs rather than an MPCore property.
> 
> Not sure what the relevance of big.LITTLE is here -- QEMU
> simply doesn't support heterogenous CPU configurations so
> we can't model big.LITTLE at all.

Not today, but neither can the user fiddle with properties before
realize today. So better not put blockers to known future features, in
particular since I've been working into that direction. Two Cortex-As
with identical software features and just different cache sizes like
A53/A57 should be the easiest inhomogeneous configuration, compared to
my Vybrid A5+M4 work.

> If we did, it would be
> via having a SoC with one a15mpcore and one a7mpcore.
> (This is how the hardware does it -- there are two
> multicore clusters, plus some cache coherency interconnect
> magic.)

Then I'm surprised and we have one issue less to worry about. :)

Thanks,
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] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Andreas Färber]

Am 16.06.2014 12:44, schrieb Peter Maydell:
> On 16 June 2014 11:19, Andreas Färber <afaerber@suse.de> wrote:
>> Am 16.06.2014 06:43, schrieb Peter Crosthwaite:
>>> So you could add an integer property listener to init them earlier (or
>>> even do dynamic extending/freeing or the allocated CPUs). I'm not sure
>>> exactly what we are really supposed to do though, when the number of
>>> child object depends on a prop like this? Andreas?
>>
>> PMM had added or looked into a property creating an array of properties.
>> However a more fundamental issue that PMM was unsure about is whether
>> the CPUs should be child<> of MPCore as done here or a sibling of the
>> MPCore container.
> 
> Was I? I can't currently see any reason you'd want them to be
> siblings of the container rather than children...
> 
> I think property-listeners was the mechanism we talked
> about for when you need to do something before realize
> but it depends on some property, yes.

Got a pointer to that? Waiting for review or merged in my absence?

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] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Maydell]

On 16 June 2014 12:18, Andreas Färber <afaerber@suse.de> wrote:
> Am 16.06.2014 12:44, schrieb Peter Maydell:
>> I think property-listeners was the mechanism we talked
>> about for when you need to do something before realize
>> but it depends on some property, yes.
>
> Got a pointer to that? Waiting for review or merged in my absence?

No, I think it was just a mailing list discussion where we
said "this is how we would do it if we need to", in not
much more detail than we have in this thread :-)

-- PMM

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Crosthwaite]

On Mon, Jun 16, 2014 at 9:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote:
>> Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if
>> existant by now) should also be refactored alongside, as proof of
>> concept - can you really create num_cpu cortex-a15 CPUs on the MPCore
>> for a big.LITTLE configuration? I'd be really surprised if there were
>> separate MPCore devices per cluster. That would then indicate that the
>> homogeneity assumption among CPUs within an MPCore is wrong and we need
>> to let its parent create the CPUs rather than an MPCore property.
>
> Not sure what the relevance of big.LITTLE is here -- QEMU
> simply doesn't support heterogenous CPU configurations so
> we can't model big.LITTLE at all. If we did, it would be
> via having a SoC with one a15mpcore and one a7mpcore.
> (This is how the hardware does it -- there are two
> multicore clusters, plus some cache coherency interconnect
> magic.)
>
>> Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to
>> mind, so we should be aware that ARMCPU child<>s on the MPCore will lead
>> to asymmetry between SoCs.
>
> For the A8 and A5 the SoC object would just instantiate them
> directly -- there's no equivalent in the hardware of the
> "n CPUs and their private devices" layer. So I think
> any asymmetry between SoCs in QEMU is just a reflection
> of the differences in the hardware.
>

+1. Either you have an MPCore package as part of your soc or
standalone CPU. Both are valid instantiables on the SoC level.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Andreas Färber]

Am 16.06.2014 13:22, schrieb Peter Crosthwaite:
> On Mon, Jun 16, 2014 at 9:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote:
>>> Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to
>>> mind, so we should be aware that ARMCPU child<>s on the MPCore will lead
>>> to asymmetry between SoCs.
>>
>> For the A8 and A5 the SoC object would just instantiate them
>> directly -- there's no equivalent in the hardware of the
>> "n CPUs and their private devices" layer. So I think
>> any asymmetry between SoCs in QEMU is just a reflection
>> of the differences in the hardware.
>>
> 
> +1. Either you have an MPCore package as part of your soc or
> standalone CPU. Both are valid instantiables on the SoC level.

That's exactly what I said and PMM cut, so no argument here.

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] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Mon, Jun 16, 2014 at 05:46:24PM +1000, Peter Crosthwaite wrote:
> On Mon, Jun 16, 2014 at 5:40 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 10 June 2014 02:32, Alistair Francis <alistair.francis@xilinx.com> wrote:
> >> This patch adds the Cortex-A9 ARM CPU to the A9MPCore.
> >
> > I think this is in general the right way to go.
> >
> >> +    /* Properties for the A9 CPU */
> >> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
> >> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
> >
> > It seems a bit error-prone if every container object has to manually
> > mirror the properties of the individual CPUs out like this; maybe
> > there's a better way we could come up with?
> >
> > Paolo, weren't you looking at passthrough properties for the
> > virtio devices?
> >
> 
> Stefan added support for QDEV property aliasing.
> 
> http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg06489.html
> 
> The bigger issue though is how do you do an N:1 mapping. The container
> should only have 1 "midr" prop, but it should mirror to all contained
> CPUs. Should we add multiplicity to the aliasing feature?

If we'll need 1:N alias properties in other places too.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Paolo Bonzini]

Il 17/06/2014 09:16, Stefan Hajnoczi ha scritto:
> > The bigger issue though is how do you do an N:1 mapping. The container
> > should only have 1 "midr" prop, but it should mirror to all contained
> > CPUs. Should we add multiplicity to the aliasing feature?
>
> If we'll need 1:N alias properties in other places too.

I think in the case it's the contained object that should look for a 
midr property in the parent, and possibly create an alias to the 
parent's midr property.

Paolo

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Crosthwaite]

On Tue, Jun 17, 2014 at 6:05 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/06/2014 09:16, Stefan Hajnoczi ha scritto:
>
>> > The bigger issue though is how do you do an N:1 mapping. The container
>> > should only have 1 "midr" prop, but it should mirror to all contained
>> > CPUs. Should we add multiplicity to the aliasing feature?
>>
>> If we'll need 1:N alias properties in other places too.

Well A15 MPCore is the obvious one. Any homogeneous n-way container
that wants to forward props could make use of this.

>
>
> I think in the case it's the contained object that should look for a midr
> property in the parent, and possibly create an alias to the parent's midr
> property.
>

This sounds too difficult to me. How do you define whether a contained
object is allowed to go looking into a parent for suitable aliasing
oppurtunities without container level knowledge?

The contained object should also be able to create it's property
regularly in cases where the parent does not provide an aliasable
which adds some ugly iffery to the contained's init fn.

Ultimately I think the container should remain in full control and do
the 1:N aliasing in a loop when appropriate.

And some further food for thought, if we throw Andreas' heterogenous
MPCore idea (one of the tangent topics on this thread) in the mix, a
container will want to potentially alias the same property on two
different children to two different props on the child. The child
simply cannot handle this without knowing too much about it's
container.

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Alistair Francis]

On Tue, Jun 17, 2014 at 8:12 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 17, 2014 at 6:05 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 17/06/2014 09:16, Stefan Hajnoczi ha scritto:
>>
>>> > The bigger issue though is how do you do an N:1 mapping. The container
>>> > should only have 1 "midr" prop, but it should mirror to all contained
>>> > CPUs. Should we add multiplicity to the aliasing feature?
>>>
>>> If we'll need 1:N alias properties in other places too.
>
> Well A15 MPCore is the obvious one. Any homogeneous n-way container
> that wants to forward props could make use of this.
>
>>
>>
>> I think in the case it's the contained object that should look for a midr
>> property in the parent, and possibly create an alias to the parent's midr
>> property.
>>
>
> This sounds too difficult to me. How do you define whether a contained
> object is allowed to go looking into a parent for suitable aliasing
> oppurtunities without container level knowledge?
>
> The contained object should also be able to create it's property
> regularly in cases where the parent does not provide an aliasable
> which adds some ugly iffery to the contained's init fn.
>
> Ultimately I think the container should remain in full control and do
> the 1:N aliasing in a loop when appropriate.

I agree, the container should have the responsibility of passing
properties through.

I submitted a v2 of the RFC which does just that (the MPcore container passes
through the midr and reset-cbar properties)

>
> And some further food for thought, if we throw Andreas' heterogenous
> MPCore idea (one of the tangent topics on this thread) in the mix, a
> container will want to potentially alias the same property on two
> different children to two different props on the child. The child
> simply cannot handle this without knowing too much about it's
> container.
>
> Regards,
> Peter
>
>> Paolo
>>
>