Re: [Qemu-devel] [PATCH] hw/arm/vexpress: Set reset-cbar property for CPUs

[Peter Crosthwaite]

On Fri, Feb 14, 2014 at 7:45 AM, Peter Maydell <address@hidden> wrote:
> On 13 February 2014 21:31, Rob Herring <address@hidden> wrote:
>> On Thu, Feb 13, 2014 at 8:26 AM, Peter Maydell <address@hidden> wrote:
>>> Newer versions of the Linux kernel (as of commit bc41b8724 in 3.12)
>>> now assume that if the CPU is a Cortex-A9 and the reset value of the
>>> PERIPHBASE/CBAR register is zero then the CPU is a specific buggy
>>> single core A9 SoC, and will not try to start other cores. Since we
>>> now have a CPU property for the reset value of the CBAR, we can
>>> just fix the vexpress board model to correctly set CBAR so SMP
>>> works again. To avoid duplicate boilerplate code in both the A9
>>> and A15 daughterboard init functions, we split out the CPU and
>>> private memory region init to its own function.
>>>
>>> Signed-off-by: Peter Maydell <address@hidden>
>>> Reported-by: Rob Herring <address@hidden>
>>> ---
>>> Thanks to Rob for tracking down this SMP boot issue and identifying
>>> the offending kernel change (which personally I think is a terrible
>>> hack, but it's in shipping kernels and our  models ought to be
>>> accurate for CBAR anyway).
>>
>> And i was working on the fix as well...
>
> Ah, sorry. I checked your git repo to see if there was a patch
> in it, but didn't find anything so I guessed you'd just done a
> quick "get it working" patch.
>
>>> +static void init_cpus(const char *cpu_model, const char *privdev,
>>> +                      hwaddr periphbase, qemu_irq *pic)
>>
>> There is nothing really vexpress specific about this function other
>> than number of irqs. This is really just expanding cpu_arm_init which
>> is the route I was going down.
>
> However cpu_arm_init() is in target-arm and has no
> business instantiating devices. I think the correct long
> term approach is going to involve the A9 and A15
> private peripheral devices instantiating the CPUs themselves.

Agreed. Most of that common code should go away with the migration of
CPU instantiation to MPCore, which should happen sooner or later. That
will end up naturally share all the code with Zynq, Highbank and
friends. Factoring this out to a global helper is backwards step to
the old style of qdev-init helpers.

Reviewed-by: Peter Crosthwaite <address@hidden>

I've been thinking about the CPU-mpcore problem, and perhaps the most
annoying part of it is propagating the user -cpu argument through to
change to CPU model. On several occasions however we have declared
this to be largely bogus for ARM. E.G.

qemu-system-arm -M zynq -cpu "cortex-a8"

doesn't really make any sense. So going into the release (which has a
major revision bump last I knew), can we defeature the -cpu syntax at
least for the MPCore boards, if not all ARM. If you want to BYO cpu,
then it should be done with regular -device style mechanisms (some
patches needed).

Regards,
Peter

> But I figured that two weeks before soft freeze was perhaps
> not the best time to open that can of worms, hence this patch
> which is localised to just the machine model code.
>
> thanks
> -- PMM
>