lkml.org 
[lkml]   [2018]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Debug: Add cpu registers dump in case of kernel panic
On Mon, 9 Jul 2018, Tian, Baofeng wrote:

> From: "Tian, Baofeng" <baofeng.tian@intel.com>
> Subject: [PATCH] Debug: Add cpu registers dump in case of kernel panic
>
> When kernel panic, the value of cpu registers are needed for analyzing, through whole ramdump to restore the crash point.
>

Please use proper line breaks at around 78 characters.

> We store CPU registers to RAM before cpu stopped, then get them with whole ram dump after warm reset.Those registers are used in analysis of dumped RAM image.
>

This is explaining what the patch does but not really why. And it does not
explain the scope of what is stored and why that would be helpful.

> Signed-off-by: Baofeng, Tian <baofeng.tian@intel.com>
> Signed-off-by: Emmanuel Berthier <emmanuel.berthier@intel.com>

This SOB chain is wrong. How is Emmmanuel involved here? He's even cc'ed on this.

> ---
> arch/x86/kernel/smp.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c index 5c574df..484831d 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -117,6 +117,20 @@
> static atomic_t stopping_cpu = ATOMIC_INIT(-1); static bool smp_no_nmi_ipi = false;
>
> +static DEFINE_PER_CPU(struct pt_regs, cpu_regs);
> +
> +/* Store regs of this CPU for RAM dump decoding help */ static inline
> +void store_regs(struct pt_regs *regs) {

That's surely not kernel coding style.

> + struct pt_regs *print_regs;

print_regs ? There is nothing printed here.

> +
> + print_regs = &get_cpu_var(cpu_regs);
> + crash_setup_regs(print_regs, regs);
> +
> + /* Flush CPU cache */
> + wbinvd();
> +}
> +
> /*
> * this function sends a 'reschedule' IPI to another CPU.
> * it goes straight through and wastes no time serializing @@ -163,6 +177,7 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
> if (raw_smp_processor_id() == atomic_read(&stopping_cpu))
> return NMI_HANDLED;
>
> + store_regs(regs);
> cpu_emergency_vmxoff();
> stop_this_cpu(NULL);
>
> @@ -173,9 +188,10 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
> * this function calls the 'stop' function on all other CPUs in the system.
> */
>
> -asmlinkage __visible void smp_reboot_interrupt(void)
> +__visible void smp_reboot_interrupt(struct pt_regs *regs)
> {
> ipi_entering_ack_irq();
> + store_regs(regs);
> cpu_emergency_vmxoff();
> stop_this_cpu(NULL);
> irq_exit();
> @@ -247,6 +263,7 @@ static void native_stop_other_cpus(int wait)
> }
>
> finish:
> + store_regs(NULL);

How is storing regs _AFTER_ doing a lots of work helpful?

> local_irq_save(flags);
> disable_local_APIC();
> mcheck_cpu_clear(this_cpu_ptr(&cpu_info));

I'm still not seeing how all that is helpful, so I'm waiting for a new
version with proper explanations.

Thanks,

tglx

\
 
 \ /
  Last update: 2018-07-19 17:12    [W:0.049 / U:5.380 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site