Re: [PATCH v12 12/16] arm64: kdump: implement machine_crash_shutdown()

[Marc Zyngier <marc.zyngier@arm.com>]

On 24/11/15 22:25, Geoff Levand wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> kdump calls machine_crash_shutdown() to shut down non-boot cpus and
> save registers' status in per-cpu ELF notes before starting the crash
> dump kernel. See kernel_kexec().
> 
> ipi_cpu_stop() is a bit modified and used to support this behavior.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  arch/arm64/include/asm/kexec.h    | 34 +++++++++++++++++++++++++++++++++-
>  arch/arm64/kernel/machine_kexec.c | 31 +++++++++++++++++++++++++++++--
>  arch/arm64/kernel/smp.c           | 16 ++++++++++++++--
>  3 files changed, 76 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> index 46d63cd..555a955 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -30,6 +30,8 @@
>  
>  #if !defined(__ASSEMBLY__)
>  
> +extern bool in_crash_kexec;
> +
>  /**
>   * crash_setup_regs() - save registers for the panic kernel
>   *
> @@ -40,7 +42,37 @@
>  static inline void crash_setup_regs(struct pt_regs *newregs,
>  				    struct pt_regs *oldregs)
>  {
> -	/* Empty routine needed to avoid build errors. */
> +	if (oldregs) {
> +		memcpy(newregs, oldregs, sizeof(*newregs));
> +	} else {
> +		__asm__ __volatile__ (
> +			"stp	 x0,   x1, [%3, #16 *  0]\n"
> +			"stp	 x2,   x3, [%3, #16 *  1]\n"
> +			"stp	 x4,   x5, [%3, #16 *  2]\n"
> +			"stp	 x6,   x7, [%3, #16 *  3]\n"
> +			"stp	 x8,   x9, [%3, #16 *  4]\n"
> +			"stp	x10,  x11, [%3, #16 *  5]\n"
> +			"stp	x12,  x13, [%3, #16 *  6]\n"
> +			"stp	x14,  x15, [%3, #16 *  7]\n"
> +			"stp	x16,  x17, [%3, #16 *  8]\n"
> +			"stp	x18,  x19, [%3, #16 *  9]\n"
> +			"stp	x20,  x21, [%3, #16 * 10]\n"
> +			"stp	x22,  x23, [%3, #16 * 11]\n"
> +			"stp	x24,  x25, [%3, #16 * 12]\n"
> +			"stp	x26,  x27, [%3, #16 * 13]\n"
> +			"stp	x28,  x29, [%3, #16 * 14]\n"
> +			"str	x30,	   [%3, #16 * 15]\n"
> +			"mov	%0, sp\n"
> +			"adr	%1, 1f\n"
> +			"mrs	%2, spsr_el1\n"
> +		"1:"
> +			: "=r" (newregs->sp),
> +			  "=r" (newregs->pc),
> +			  "=r" (newregs->pstate)
> +			: "r"  (&newregs->regs)
> +			: "memory"
> +		);

I wonder how useful this thing is, given that it starts by corrupting
whatever register is holding newregs->regs. Maybe this is not supposed
to be accurate anyway...


> +	}
>  }
>  
>  #endif /* !defined(__ASSEMBLY__) */
> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
> index da28a26..d2d7e90 100644
> --- a/arch/arm64/kernel/machine_kexec.c
> +++ b/arch/arm64/kernel/machine_kexec.c
> @@ -9,6 +9,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/kernel.h>
>  #include <linux/kexec.h>
>  #include <linux/of_fdt.h>
>  #include <linux/slab.h>
> @@ -23,6 +24,7 @@
>  extern const unsigned char arm64_relocate_new_kernel[];
>  extern const unsigned long arm64_relocate_new_kernel_size;
>  
> +bool in_crash_kexec;
>  static unsigned long kimage_start;
>  
>  /**
> @@ -203,13 +205,38 @@ void machine_kexec(struct kimage *kimage)
>  	 */
>  
>  	cpu_soft_restart(virt_to_phys(cpu_reset),
> -		is_hyp_mode_available(),
> +		in_crash_kexec ? 0 : is_hyp_mode_available(),
>  		reboot_code_buffer_phys, kimage->head, kimage_start);
>  
>  	BUG(); /* Should never get here. */
>  }
>  
> +/**
> + * machine_crash_shutdown - shutdown non-boot cpus and save registers
> + */
>  void machine_crash_shutdown(struct pt_regs *regs)
>  {
> -	/* Empty routine needed to avoid build errors. */
> +	struct pt_regs dummy_regs;
> +	int cpu;
> +
> +	local_irq_disable();
> +
> +	in_crash_kexec = true;
> +
> +	/*
> +	 * clear and initialize the per-cpu info. This is necessary
> +	 * because, otherwise, slots for offline cpus would never be
> +	 * filled up. See smp_send_stop().
> +	 */
> +	memset(&dummy_regs, 0, sizeof(dummy_regs));
> +	for_each_possible_cpu(cpu)
> +		crash_save_cpu(&dummy_regs, cpu);
> +
> +	/* shutdown non-boot cpus */
> +	smp_send_stop();
> +
> +	/* for boot cpu */
> +	crash_save_cpu(regs, smp_processor_id());
> +
> +	pr_info("Starting crashdump kernel...\n");
>  }
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index b1adc51..15aabef 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -37,6 +37,7 @@
>  #include <linux/completion.h>
>  #include <linux/of.h>
>  #include <linux/irq_work.h>
> +#include <linux/kexec.h>
>  
>  #include <asm/alternative.h>
>  #include <asm/atomic.h>
> @@ -54,6 +55,8 @@
>  #include <asm/ptrace.h>
>  #include <asm/virt.h>
>  
> +#include "cpu-reset.h"
> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/ipi.h>
>  
> @@ -683,8 +686,12 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
>  /*
>   * ipi_cpu_stop - handle IPI from smp_send_stop()
>   */
> -static void ipi_cpu_stop(unsigned int cpu)
> +static void ipi_cpu_stop(unsigned int cpu, struct pt_regs *regs)
>  {
> +#ifdef CONFIG_KEXEC
> +	/* printing messages may slow down the shutdown. */
> +	if (!in_crash_kexec)
> +#endif
>  	if (system_state == SYSTEM_BOOTING ||
>  	    system_state == SYSTEM_RUNNING) {
>  		raw_spin_lock(&stop_lock);

Irrespective of how useful this change is, how about having a predicate
instead? Something like:

static inline bool is_in_crash_kexec(void)
{
#ifdef CONFIG_KEXEC
	return in_crash_kexec;
#else
	return false;
#endif
}

located in machine_kexec.c (making the in_crash_kernel static), and then

	if (!is_in_crash_kexec() && (systen_state == ... || ...) {

It would certainly look better.

> @@ -697,6 +704,11 @@ static void ipi_cpu_stop(unsigned int cpu)
>  
>  	local_irq_disable();
>  
> +#ifdef CONFIG_KEXEC
> +	if (in_crash_kexec)
> +		crash_save_cpu(regs, cpu);
> +#endif /* CONFIG_KEXEC */
> +

Same here.

>  	while (1)
>  		cpu_relax();
>  }
> @@ -727,7 +739,7 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>  
>  	case IPI_CPU_STOP:
>  		irq_enter();
> -		ipi_cpu_stop(cpu);
> +		ipi_cpu_stop(cpu, regs);
>  		irq_exit();
>  		break;
>  
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec