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

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