Re: [Qemu-devel] [PATCH v8 2/8] ARM: Samsung exynos4210-based boards emulation

[Peter Maydell]

On 19 January 2012 13:15, Evgeny Voevodin <address@hidden> wrote:
> On 01/19/2012 04:19 PM, Peter Maydell wrote:
>>
>> On 19 January 2012 08:31, Evgeny Voevodin<address@hidden>  wrote:
>>
>>> +    /*
>>> +     * Secondary CPU startup code will be placed here.
>>> +     */
>>> +    memory_region_init_ram(&s->hack_mem, "exynos4210.hack", 0x1000);
>>> +    memory_region_add_subregion(system_mem, EXYNOS4210_SMP_BOOT_ADDR,
>>> +&s->hack_mem);
>>
>> I've been thinking about this 'hack' memory region, because I figured
>> out that we didn't actually need it on vexpress (or realview, though
>> I haven't submitted a patch to fix that yet). Basically the idea is that
>> we need to put the secondary CPU startup code somewhere where Linux
>> won't trash it before the secondary cores get started properly. However
>> this requirement exists also for the real hardware. So the right thing
>> to do here is identify where the real hardware's boot ROM puts the
>> secondary CPU holding pen code, and use the same thing. (On vexpress
>> this is in an area of SDRAM). What does the Exynos4 hardware/bootrom do
>> here?
>>
> In our case it is not a hack. Secondary CPU boot loader resides in ROM
> memory.
> And we have mapped a region in the ROM to place our boot-loader code there.
> Here all is correct.

If this is ROM then (a) don't model it as RAM (b) don't call it "hack"
if it's actually justifiable (c) justify it in a comment.

>> If this was modelled as an actual register in a device model with a reset
>> function we wouldn't have needed the code in arm_boot.c:do_cpu_reset()
>> that clears smp_bootreg_addr. I think that's an argument for implementing
>> this as an actual qdev device, however minimal (one that implements
>> exactly one register would do) rather than as a bit of RAM.
>>
> Boot reg could be in the RAM on other platforms and code which is monitoring
> this address is awaiting a non-zero value here. If we would not clear this
> region, wrong boot address for secondary CPU will be read. So, code to clear
> this region should be at reset function anyway.

On *what* other platforms? On realview/vexpress the smp_bootreg_addr is
a device register so is cleared at reset. Highbank has its own complicated
thing (which is going to be implemented via some hook code). Basically
having code in do_cpu_reset() is doing something hardware doesn't do.
On hardware we will either (if the location is a device) clear the value
as part of that device's reset, or (if it is actually RAM) clear the value
as part of the code executed by one of the CPUs as it boots.

> I think that it is a matter of current board how to manage boot address -
> map a region or implement a device.

Mapping a RAM region in the model when the hardware implements the
device is drifting away from modelling what the hardware actually
does. When that drift means we end up needing workarounds in
generic code like arm_boot.c that's a cause for concern and a
reason to avoid that drift.

-- PMM