Re: [PATCH RFC 5/7] xen: arm: rewrite start of day page table and cpu bring up

[Andre Przywara <andre.przywara@linaro.org>]

On 09/17/2013 06:55 PM, Julien Grall wrote:
> On 09/17/2013 05:36 PM, Ian Campbell wrote:
>> On Tue, 2013-09-17 at 17:18 +0100, Julien Grall wrote:
>>>> +        /* Now we can install the fixmap and dtb mappings, since we
>>>> +         * don't need the 1:1 map any more */
>>>> +        dsb   sy
>>>> +        ldr   r1, =boot_second
>>>> +#if defined(EARLY_PRINTK)
>>>> +        /* xen_fixmap pagetable */
>>>
>>> Can you add a comment to explain why we don't need to map the fixmap
>>> when early printk is not enabled?
>>
>> It's covered by the overall description of the boot tables layout which
>> is in mm.c and referenced elsewhere in this file. Is the suficient?
>
> I just saw the comment, I'm fine with it. It took me several minutes to
> find where :).
>
>> [..]
>>>> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
>>>> index f460e9c..f616807 100644
>>>> --- a/xen/include/asm-arm/platform.h
>>>> +++ b/xen/include/asm-arm/platform.h
>>>> @@ -14,6 +14,11 @@ struct platform_desc {
>>>>       /* Platform initialization */
>>>>       int (*init)(void);
>>>>       int (*init_time)(void);
>>>> +#ifdef CONFIG_ARM_32
>>>> +    /* SMP */
>>>> +    int (*cpu_init)(int cpu);
>>>
>>> I don't think a cpu_init callback is usefull. An smp_init callback would
>>> be better.
>>>
>>> This will allow you to move the sys_flags check for the versatile
>>> express in smp_init.
>>
>> I wondered if there might be platforms with differeing mbox addresses
>> for different CPU. e.g. the armv8 stuff (which doesn't use this path)
>> makes provisions for this.
>
> I believe, it's the case on midway. Andre, can you confirm?

Yes, it is: 0x40 + cpunr * 0x10
This only matters if you use the native way of SMP booting, but actually 
on Midway you should be using PSCI (which is already supported by the 
firmware).
So if PSCI support is just around the corner, this should be OK for the 
time being.

Regards,
Andre.

> But, I think this code can be merge in cpu_up.
>
>>
>> But I'll make the suggested change -- we can also refactor or add a
>> second callback if such a platform shows up.
>>
>> You didn't trim your quotes, I hope I didn't miss any comments (I
>> trimmed a bunch of the s/xen_/boot_/ ones... Will do a thorough
>> sweep...)
>
> You didn't miss any comments, next time I will trim my quotes.
>