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
<address@hidden> wrote:
> 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?

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