Re: [Qemu-devel] [PATCH 1/2] target-arm: make MPIDR a property

[Peter Maydell]

On 24 February 2014 23:13, Rob Herring <address@hidden> wrote:
> On Mon, Feb 24, 2014 at 4:28 PM, Peter Maydell <address@hidden> wrote:
>> On 24 February 2014 22:14, Rob Herring <address@hidden> wrote:
>>> From: Rob Herring <address@hidden>
>>> MPIDR register is a machine configurable option and current kernels require
>>> the value to match with DT cpu reg properties. So add a property for MPIDR
>>> value and allow platforms to override.
>
> [snip]
>
>>> @@ -1442,6 +1442,11 @@ static int mpidr_read(CPUARMState *env, const 
>>> ARMCPRegInfo *ri,
>>>  {
>>>      CPUState *cs = CPU(arm_env_get_cpu(env));
>>>      uint32_t mpidr = cs->cpu_index;
>>> +
>>> +    if (ri->resetvalue) {
>>> +      *value = ri->resetvalue;
>>> +      return 0;
>>> +    }
>>
>> This looks weird, because it's overriding the CPU index. (Also, you have
>> access to the CPU object here, you might as well just say "if (cpu->mpidr)"
>> rather than feeding it in through the ri->resetvalue when we don't actually
>> care about it at reset.)
>
> The cpu index has already been set by the platform, but yes I agree
> just using cpu->mpidr will be simpler.

Looking at your patch 2 I think makes it really clear we don't want
the platform to have to set the cpu index. It should just set the cluster ID.
(Eventually the CPU creation will disappear into the a15mpcore_priv
device, and then cluster ID will be a property on that device.)

>> Should the property actually be "cluster number" or something
>> similar? This is how the hardware does it, there are CLUSTERID
>> signals to set bits [11:8] of the MPIDR for A9/A15; A57 has
>> CLUSTERIDAFF1 and CLUSTERIDAFF2.
>
> The whole concept of cluster isn't architecturally defined beyond
> "affinity level" and is specific to those cores. I think it is better
> to leave the flexibility to override MPIDR to anything.
>
> Having been asked before, what the h/w folks tie these off to will be
> all over the map if left up to their own imaginations. :)

Yeah, but the h/w doesn't give the ability to tie off anything except
the cluster affinity fields -- you can't make the individual cores
report anything except 0..3 in the lowest part of the MPIDR,
and you can't prevent bit 31 being set.

>> It might be easier to leave as a single uint32_t property, but I'm
>> pretty sure we should be ORing it in with the CPU index and
>> bit 31.
>
> Some platform may want to do something like boot on cpu other than
> mpidr[7:0] == 0 and define a different mapping. There would probably
> be some other issues like GIC cpu interfaces before that would really
> work though.

If we ever need that I think we should implement it straightforwardly
(ie by supporting holding an arbitrary set of  CPUs in reset or powerdown
and booting on the first powered up core) and not by doing weird
rearrangements of the MPIDR to fake something we're not emulating
properly.

thanks
-- PMM