Re: [Xen-devel] [PATCH 5/7] xen/arm: Remove __initdata and __init to enable CPU hotplug
Hi Julien,
On Thu, Apr 12, 2018 at 2:56 PM, Julien Grall wrote:
> Hi,
>
> On 12/04/18 13:50, Mirela Simonovic wrote:
>>
>> Hi,
>>
>> On Thu, Apr 12, 2018 at 11:03 AM, Julien Grall
>> 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
Re: [Xen-devel] [PATCH 5/7] xen/arm: Remove __initdata and __init to enable CPU hotplug
Hi,
On 12/04/18 13:50, Mirela Simonovic wrote:
Hi,
On Thu, Apr 12, 2018 at 11:03 AM, Julien Grall 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.
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?
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/7] xen/arm: Remove __initdata and __init to enable CPU hotplug
Hi,
On Thu, Apr 12, 2018 at 11:03 AM, Julien Grall 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.
>
> 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.
Thanks,
Mirela
> Cheers,
>
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/7] xen/arm: Remove __initdata and __init to enable CPU hotplug
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?
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.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/7] xen/arm: Remove __initdata and __init to enable CPU hotplug
On Wed, 11 Apr 2018, Mirela Simonovic wrote:
> CPU up flow is currently used during the initial boot to start secondary
> CPUs. However, the same flow should be used for CPU hotplug, e.g. when
> hotplugging secondary CPUs within the resume procedure (resume from the
> suspend to RAM). Therefore, prefixes __initdata and __init had to be removed
> from few data structures and functions that are used within the cpu up flow.
>
> Signed-off-by: Mirela Simonovic
> ---
> xen/arch/arm/arm64/smpboot.c | 2 +-
> xen/arch/arm/irq.c | 2 +-
> xen/arch/arm/processor.c | 2 +-
> xen/arch/arm/smpboot.c | 4 ++--
> xen/include/asm-arm/procinfo.h | 4 ++--
> 5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 4fd0ac68b7..694fbf67e6 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -104,7 +104,7 @@ int __init arch_cpu_init(int cpu, struct dt_device_node
> *dn)
> return smp_psci_init(cpu);
> }
>
> -int __init arch_cpu_up(int cpu)
> +int arch_cpu_up(int cpu)
> {
> if ( !smp_enable_ops[cpu].prepare_cpu )
> return -ENODEV;
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index aa4e832cae..098281f8ab 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -65,7 +65,7 @@ irq_desc_t *__irq_to_desc(int irq)
> return &irq_desc[irq-NR_LOCAL_IRQS];
> }
>
> -int __init arch_init_one_irq_desc(struct irq_desc *desc)
> +int arch_init_one_irq_desc(struct irq_desc *desc)
> {
> desc->arch.type = IRQ_TYPE_INVALID;
> return 0;
> diff --git a/xen/arch/arm/processor.c b/xen/arch/arm/processor.c
> index ce4385064a..acad8b31d6 100644
> --- a/xen/arch/arm/processor.c
> +++ b/xen/arch/arm/processor.c
> @@ -20,7 +20,7 @@
>
> static DEFINE_PER_CPU(struct processor *, processor);
>
> -void __init processor_setup(void)
> +void processor_setup(void)
> {
> const struct proc_info_list *procinfo;
>
> 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?
> diff --git a/xen/include/asm-arm/procinfo.h b/xen/include/asm-arm/procinfo.h
> index 26306b35f8..02be56e348 100644
> --- a/xen/include/asm-arm/procinfo.h
> +++ b/xen/include/asm-arm/procinfo.h
> @@ -35,9 +35,9 @@ struct proc_info_list {
> struct processor*processor;
> };
>
> -const __init struct proc_info_list *lookup_processor_type(void);
> +const struct proc_info_list *lookup_processor_type(void);
>
> -void __init processor_setup(void);
> +void processor_setup(void);
> void processor_vcpu_initialise(struct vcpu *v);
>
> #endif
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
