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
<address@hidden> 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 <address@hidden>
> ---
> 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
>
>