Re: [PATCH 5/7] xen/arm: Remove __initdata and __init to enable CPU hotplug

[Mirela Simonovic <mirela.simonovic@aggios.com>]

Hi Julien,

On Thu, Apr 12, 2018 at 2:56 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
> On 12/04/18 13:50, Mirela Simonovic wrote:
>>
>> Hi,
>>
>> On Thu, Apr 12, 2018 at 11:03 AM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 12/04/18 01:07, Stefano Stabellini wrote:
>>>>
>>>>
>>>> On Wed, 11 Apr 2018, Mirela Simonovic wrote:
>>>>>
>>>>>
>>>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>>>> index 5666efcd3a..d15ea8df5e 100644
>>>>> --- a/xen/arch/arm/smpboot.c
>>>>> +++ b/xen/arch/arm/smpboot.c
>>>>> @@ -52,8 +52,8 @@ nodemask_t __read_mostly node_online_map = { { [0] =
>>>>> 1UL } };
>>>>>    static unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>>>>>           __attribute__((__aligned__(STACK_SIZE)));
>>>>>    -/* Initial boot cpu data */
>>>>> -struct init_info __initdata init_data =
>>>>> +/* Boot cpu data */
>>>>> +struct init_info init_data =
>>>>>    {
>>>>>        .stack = cpu0_boot_stack,
>>>>>    };
>>>>
>>>>
>>>>
>>>> Don't you also want to remove __initdata from cpu0_boot_stack?
>>>
>>>
>>
>> Somehow I didn't observe this as a problem... After taking a deeper
>> look now I understand that secondary CPUs reuse this stack to boot. So
>> I agree, __initdata from cpu0_boot_stack should be removed.
>
>
> No it should not be removed. cpu0_boot_stack is only used for Xen is booted
> (e.g CPU0 jumping at _start). In the suspend/resume case you are not going
> to use that patch for CPU0.
>

Thanks for clarification. I need to correct myself - I missed the fact
that the boot CPU updated init_data.stack for a secondary CPU to point
to its idle_vcpu's stack (in __cpu_up). So you're right,
cpu0_boot_stack will never be used again after the CPU0 boots and
therefore the __initdata should not be removed.

>>
>>>
>>> I am not sure about this. When you go idle, you could re-use the
>>> idle_vcpu[0]->arch.stack. So you save 12K in resident memory.
>>>
>>
>> I'm not sure I follow this, maybe Stefano can comment.
>
>
> Each CPU have an associated idle vCPU used for context switch and running
> idle mode. That idle vCPU contains the stack that is used for boot CPU.
>
> In the case of CPU0, you can not use idle vCPU stack when booting because it
> is not initialized. However during suspend/resume case, you will already
> have the idle_vcpu[0]->stack in hand. So there are no need to use
> cpu0_boot_stack.
>
> However, do you really need to setup the stack on resume?
>

There is no need to use cpu0_boot_stack for CPU0 on resume. The
idle_vcpu's stack is used and it has to be used if we want to resume
execution from where we suspended. We just save/restore SP on
suspend/resume.

Thanks,
Mirela

> Cheers,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel