Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1292082 > unrolled thread

Re: [RFC PATCH v3 4/8] arm64: Handle early CPU boot failures

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v3 4/8] arm64: Handle early CPU boot failures Will Deacon <will.deacon@arm.com> - 2015-12-15 13:00 +0100
    Re: [RFC PATCH v3 4/8] arm64: Handle early CPU boot failures "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2015-12-16 11:40 +0100
      Re: [RFC PATCH v3 4/8] arm64: Handle early CPU boot failures Will Deacon <will.deacon@arm.com> - 2015-12-16 12:30 +0100
        Re: [RFC PATCH v3 4/8] arm64: Handle early CPU boot failures Mark Rutland <mark.rutland@arm.com> - 2015-12-16 12:40 +0100

#1292082 — Re: [RFC PATCH v3 4/8] arm64: Handle early CPU boot failures

On Wed, Dec 09, 2015 at 09:57:15AM +0000, Suzuki K. Poulose wrote:
> A secondary CPU could fail to come online due to insufficient
> capabilities and could simply die or loop in the kernel.
> e.g, a CPU with no support for the selected kernel PAGE_SIZE
> loops in kernel with MMU turned off.
> or a hotplugged CPU which doesn't have one of the advertised
> system capability will die during the activation.
> 
> There is no way to synchronise the status of the failing CPU
> back to the master. This patch solves the issue by adding a
> field to the secondary_data which can be updated by the failing
> CPU.
> 
> Here are the possible states :
> 
>  -1. CPU_WAIT_STATUS - Initial value set by the master CPU.
> 
>  0. CPU_BOOT_SUCCESS - CPU has booted successfully.
> 
>  1. CPU_KILL_ME - CPU has invoked cpu_ops->die, indicating the
> master CPU to synchronise by issuing a cpu_ops->cpu_kill.
> 
>  2. CPU_STUCK_IN_KERNEL - CPU couldn't invoke die(), instead is
> looping in the kernel. This information could be used by say,
> kexec to check if it is really safe to do a kexec reboot.
> 
>  3. CPU_PANIC_KERNEL - CPU detected some serious issues which
> requires kernel to crash immediately. The secondary CPU cannot
> call panic() until it has initialised the GIC. This flag can
> be used to instruct the master to do so.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/smp.h    |   24 +++++++++++++++++++-
>  arch/arm64/kernel/asm-offsets.c |    3 +++
>  arch/arm64/kernel/head.S        |   19 ++++++++++++++--
>  arch/arm64/kernel/smp.c         |   48 ++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 90 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
> index 13ce01f..240ab3d 100644
> --- a/arch/arm64/include/asm/smp.h
> +++ b/arch/arm64/include/asm/smp.h
> @@ -16,6 +16,19 @@
>  #ifndef __ASM_SMP_H
>  #define __ASM_SMP_H
>  
> +/* Values for secondary_data.status */
> +
> +#define CPU_WAIT_RESULT		-1
> +#define CPU_BOOT_SUCCESS	0
> +/* The cpu invoked ops->cpu_die, synchronise it with cpu_kill */
> +#define CPU_KILL_ME		1
> +/* The cpu couldn't die gracefully and is looping in the kernel */
> +#define CPU_STUCK_IN_KERNEL	2
> +/* Fatal system error detected by secondary CPU, crash the system */
> +#define CPU_PANIC_KERNEL	3
> +
> +#ifndef __ASSEMBLY__
> +
>  #include <linux/threads.h>
>  #include <linux/cpumask.h>
>  #include <linux/thread_info.h>
> @@ -54,10 +67,15 @@ asmlinkage void secondary_start_kernel(void);
>  
>  /*
>   * Initial data for bringing up a secondary CPU.
> + * @stack  - sp for the secondary CPU
> + * @status - Result passed back from the secondary CPU to
> + *           indicate failure.
>   */
>  struct secondary_data {
>  	void *stack;
> -};
> +	unsigned long status;
> +} ____cacheline_aligned;

Why is this necessary?

> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index b225d34..e0a42dd 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -34,6 +34,7 @@
>  #include <asm/pgtable-hwdef.h>
>  #include <asm/pgtable.h>
>  #include <asm/page.h>
> +#include <asm/smp.h>
>  #include <asm/sysreg.h>
>  #include <asm/thread_info.h>
>  #include <asm/virt.h>
> @@ -605,7 +606,7 @@ ENTRY(secondary_startup)
>  ENDPROC(secondary_startup)
>  
>  ENTRY(__secondary_switched)
> -	ldr	x0, [x22]			// get secondary_data.stack
> +	ldr	x0, [x22, #CPU_BOOT_STACK]	// get secondary_data.stack
>  	mov	sp, x0
>  	mov	x29, #0
>  	b	secondary_start_kernel
> @@ -615,6 +616,7 @@ ENDPROC(__secondary_switched)
>   * Enable the MMU.
>   *
>   *  x0  = SCTLR_EL1 value for turning on the MMU.
> + *  x22 = __va(secondary_data)
>   *  x27 = *virtual* address to jump to upon completion
>   *
>   * Other registers depend on the function called upon completion.
> @@ -647,6 +649,19 @@ __enable_mmu:
>  ENDPROC(__enable_mmu)
>  
>  __no_granule_support:
> +	/* Indicate that this CPU can't boot and is stuck in the kernel */
> +	cmp	x22, 0				// Boot CPU doesn't update status
> +	b.eq	1f
> +	adrp	x1, secondary_data
> +	add	x1, x1, #:lo12:secondary_data	// x1 = __pa(secondary_data)

This feels a bit grotty. You end up using the virtual address of
secondary_data to figure out if you're the boot CPU or not, and then you
end up having to compute the physical address of the same variable anyway.

> +	mov	x0, #CPU_STUCK_IN_KERNEL
> +	str	x0, [x1, #CPU_BOOT_STATUS]	// update the secondary_data.status
> +	/* flush the data to PoC */

Can you call __inval_cache_range instead of open-coding this?

> +	isb

Why?

> +	dc	civac, x1
> +	dsb	sy
> +	isb

Why?

> +1:
>  	wfe

Curious, but do you know why we don't have a wfi here too (like we do in
the C code)?

> -	b __no_granule_support
> +	b 1b
>  ENDPROC(__no_granule_support)
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 607d876..708f4b1 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -63,6 +63,8 @@
>   * where to place its SVC stack
>   */
>  struct secondary_data secondary_data;
> +/* Number of CPUs which aren't online, but looping in kernel text. */
> +u32 cpus_stuck_in_kernel;
>  
>  enum ipi_msg_type {
>  	IPI_RESCHEDULE,
> @@ -72,6 +74,16 @@ enum ipi_msg_type {
>  	IPI_IRQ_WORK,
>  };
>  
> +#ifdef CONFIG_HOTPLUG_CPU
> +static int op_cpu_kill(unsigned int cpu);
> +#else
> +static inline int op_cpu_kill(unsigned int cpu)
> +{
> +	return -ENOSYS;
> +}
> +#endif
> +
> +
>  /*
>   * Boot a secondary CPU, and assign it the specified idle task.
>   * This also gives us the initial stack to use for this CPU.
> @@ -86,6 +98,12 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)
>  
>  static DECLARE_COMPLETION(cpu_running);
>  
> +void update_cpu_boot_status(unsigned long status)
> +{
> +	secondary_data.status = status;
> +	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
> +}
> +
>  int __cpu_up(unsigned int cpu, struct task_struct *idle)
>  {
>  	int ret;
> @@ -95,7 +113,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>  	 * page tables.
>  	 */
>  	secondary_data.stack = task_stack_page(idle) + THREAD_START_SP;
> -	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
> +	update_cpu_boot_status(CPU_WAIT_RESULT);
>  
>  	/*
>  	 * Now bring the CPU into our world.
> @@ -117,7 +135,31 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>  		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
>  	}
>  
> +	mb();

Why? (and why not smp_mb(), if the barrier is actually needed?).

> +
>  	secondary_data.stack = NULL;
> +	if (ret && secondary_data.status) {
> +		switch(secondary_data.status) {
> +		default:
> +			pr_err("CPU%u: failed in unknown state : 0x%lx\n",
> +					cpu, secondary_data.status);
> +			break;
> +		case CPU_KILL_ME:
> +			if (op_cpu_kill(cpu)) {
> +				pr_crit("CPU%u: may not have shut down cleanly\n",
> +							cpu);
> +				cpus_stuck_in_kernel++;

Maybe restructure this so the failed kill case is a fallthrough to the
CPU_STUCK_IN_KERNEL case?

> +			} else
> +				pr_crit("CPU%u: died during early boot\n", cpu);

Missing braces.

> +			break;
> +		case CPU_STUCK_IN_KERNEL:
> +			pr_crit("CPU%u: is stuck in kernel\n", cpu);
> +			cpus_stuck_in_kernel++;
> +			break;
> +		case CPU_PANIC_KERNEL:
> +			panic("CPU%u detected unsupported configuration\n", cpu);

It would nice to show what the configuration difference was, but maybe
that's work for later.

> +		}
> +	}
>  
>  	return ret;
>  }
> @@ -185,6 +227,7 @@ asmlinkage void secondary_start_kernel(void)
>  	 */
>  	pr_info("CPU%u: Booted secondary processor [%08x]\n",
>  					 cpu, read_cpuid_id());
> +	update_cpu_boot_status(CPU_BOOT_SUCCESS);

Why do we need to continue with the cacheflushing at this point?

>  	set_cpu_online(cpu, true);
>  	complete(&cpu_running);
>  
> @@ -327,10 +370,13 @@ void cpu_die_early(void)
>  	set_cpu_present(cpu, 0);
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> +	update_cpu_boot_status(CPU_KILL_ME);
>  	/* Check if we can park ourselves */
>  	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
>  		cpu_ops[cpu]->cpu_die(cpu);
>  #endif
> +	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
> +	__flush_dcache_area(&secondary_data, sizeof(secondary_data));

Extra flushing, but I don't see why you need it at all when you're
running in C on the CPU coming up.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1292882

On 15/12/15 11:55, Will Deacon wrote:
> On Wed, Dec 09, 2015 at 09:57:15AM +0000, Suzuki K. Poulose wrote:

>>   /*
>>    * Initial data for bringing up a secondary CPU.
>> + * @stack  - sp for the secondary CPU
>> + * @status - Result passed back from the secondary CPU to
>> + *           indicate failure.
>>    */
>>   struct secondary_data {
>>   	void *stack;
>> -};
>> +	unsigned long status;
>> +} ____cacheline_aligned;
>
> Why is this necessary?

That was based on a suggestion from Mark Rutland here:

   https://lkml.org/lkml/2015/12/1/580


>> @@ -615,6 +616,7 @@ ENDPROC(__secondary_switched)
>>    * Enable the MMU.
>>    *
>>    *  x0  = SCTLR_EL1 value for turning on the MMU.
>> + *  x22 = __va(secondary_data)
>>    *  x27 = *virtual* address to jump to upon completion

...

>>
>>   __no_granule_support:
>> +	/* Indicate that this CPU can't boot and is stuck in the kernel */
>> +	cmp	x22, 0				// Boot CPU doesn't update status
>> +	b.eq	1f
>> +	adrp	x1, secondary_data
>> +	add	x1, x1, #:lo12:secondary_data	// x1 = __pa(secondary_data)
>
> This feels a bit grotty. You end up using the virtual address of
> secondary_data to figure out if you're the boot CPU or not, and then you
> end up having to compute the physical address of the same variable anyway.
>

Yes, it looks a bit weird, but we pass down the va(secondary_data) to the secondary
CPU and that is used only after the MMU is turned on, to fetch the stack ptr.
We use that information to detect if we are secondary. When we fail before turning
the MMU on, we need to update our result back, hence the va-to-pa conversion.
Is there a better way to handle it ? Am I missing something ? Or we could add another
register which would indicate if it is secondary or not.

>> +	mov	x0, #CPU_STUCK_IN_KERNEL
>> +	str	x0, [x1, #CPU_BOOT_STATUS]	// update the secondary_data.status
>> +	/* flush the data to PoC */
>
> Can you call __inval_cache_range instead of open-coding this?

Yes, that sounds better, will use that.

>> +	isb
>
> Why?

...

>
>> +	dc	civac, x1
>> +	dsb	sy
>> +	isb
>
> Why?

I now realize how bad this code is. I wrote this up in a hurry, without proper
understanding of the dependencies of 'dc'. I was under the assumption that the isb
needed just like we do after updating the system control registers, which is not
true for dc.

>
>> +1:
>>   	wfe
>
> Curious, but do you know why we don't have a wfi here too (like we do in
> the C code)?

Yes we do, will add it.

>>   	/*
>>   	 * Now bring the CPU into our world.
>> @@ -117,7 +135,31 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>>   		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
>>   	}
>>
>> +	mb();
>
> Why? (and why not smp_mb(), if the barrier is actually needed?).

Actually, now when I think of it we don't need it as the cache would have been
invalidated by the secondary. All we need is making secondary_data.status volatile,
to make sure we don't use a stale copy of secondary_data.status cached in the
register.

I will rewrite this code.

>> +
>>   	secondary_data.stack = NULL;
>> +	if (ret && secondary_data.status) {
>> +		switch(secondary_data.status) {
>> +		default:
>> +			pr_err("CPU%u: failed in unknown state : 0x%lx\n",
>> +					cpu, secondary_data.status);
>> +			break;
>> +		case CPU_KILL_ME:
>> +			if (op_cpu_kill(cpu)) {
>> +				pr_crit("CPU%u: may not have shut down cleanly\n",
>> +							cpu);
>> +				cpus_stuck_in_kernel++;
>
> Maybe restructure this so the failed kill case is a fallthrough to the
> CPU_STUCK_IN_KERNEL case?

Yes, makes sense.

>
>> +			} else
>> +				pr_crit("CPU%u: died during early boot\n", cpu);
>
> Missing braces.

Will fix that.

>> +			break;
>> +		case CPU_STUCK_IN_KERNEL:
>> +			pr_crit("CPU%u: is stuck in kernel\n", cpu);
>> +			cpus_stuck_in_kernel++;
>> +			break;
>> +		case CPU_PANIC_KERNEL:
>> +			panic("CPU%u detected unsupported configuration\n", cpu);
>
> It would nice to show what the configuration difference was, but maybe
> that's work for later.

One way would be to assign code for each of the failing cases overload the
status with the reason (on the top half). But thats only needed for cases where
we couldn't write something to the console. Anyways, thats something for later.

>
>> +		}
>> +	}
>>
>>   	return ret;
>>   }
>> @@ -185,6 +227,7 @@ asmlinkage void secondary_start_kernel(void)
>>   	 */
>>   	pr_info("CPU%u: Booted secondary processor [%08x]\n",
>>   					 cpu, read_cpuid_id());
>> +	update_cpu_boot_status(CPU_BOOT_SUCCESS);
>
> Why do we need to continue with the cacheflushing at this point?

>> +	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
>> +	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
>
> Extra flushing, but I don't see why you need it at all when you're
> running in C on the CPU coming up.


You are right, we don't need to do this as we are already using the
va.

I will rewrite it properly. Thanks for the review and patience :)

Cheers
Suzuki
  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292908

On Wed, Dec 16, 2015 at 10:39:50AM +0000, Suzuki K. Poulose wrote:
> On 15/12/15 11:55, Will Deacon wrote:
> >On Wed, Dec 09, 2015 at 09:57:15AM +0000, Suzuki K. Poulose wrote:
> 
> >>  /*
> >>   * Initial data for bringing up a secondary CPU.
> >>+ * @stack  - sp for the secondary CPU
> >>+ * @status - Result passed back from the secondary CPU to
> >>+ *           indicate failure.
> >>   */
> >>  struct secondary_data {
> >>  	void *stack;
> >>-};
> >>+	unsigned long status;
> >>+} ____cacheline_aligned;
> >
> >Why is this necessary?
> 
> That was based on a suggestion from Mark Rutland here:
> 
>   https://lkml.org/lkml/2015/12/1/580

That thread is talking about the CWG, which is not the same thing as
____cacheline_aligned. Given that the architectural maximum for the CWG
is 2K, we can probably get away with allocating the status field amongst
the head.S text instead (which we know will be clean).

Since SMP boot is serialised, that should be sufficient, right?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292916

On Wed, Dec 16, 2015 at 11:29:15AM +0000, Will Deacon wrote:
> On Wed, Dec 16, 2015 at 10:39:50AM +0000, Suzuki K. Poulose wrote:
> > On 15/12/15 11:55, Will Deacon wrote:
> > >On Wed, Dec 09, 2015 at 09:57:15AM +0000, Suzuki K. Poulose wrote:
> > 
> > >>  /*
> > >>   * Initial data for bringing up a secondary CPU.
> > >>+ * @stack  - sp for the secondary CPU
> > >>+ * @status - Result passed back from the secondary CPU to
> > >>+ *           indicate failure.
> > >>   */
> > >>  struct secondary_data {
> > >>  	void *stack;
> > >>-};
> > >>+	unsigned long status;
> > >>+} ____cacheline_aligned;
> > >
> > >Why is this necessary?
> > 
> > That was based on a suggestion from Mark Rutland here:
> > 
> >   https://lkml.org/lkml/2015/12/1/580
> 
> That thread is talking about the CWG, which is not the same thing as
> ____cacheline_aligned. Given that the architectural maximum for the CWG
> is 2K, we can probably get away with allocating the status field amongst
> the head.S text instead (which we know will be clean).
> 
> Since SMP boot is serialised, that should be sufficient, right?

Assuming you mean in .head.text (rather than .text), that should work
given that we don't currently free .head.text.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web