[PATCH] ppc64/fadump: fix inaccurate CPU state info in vmcore generated with panic

[Hari Bathini]

On 11/11/21 6:50 pm, Nicholas Piggin wrote:
> Excerpts from Hari Bathini's message of November 11, 2021 10:06 pm:
>>
>>
>> On 11/11/21 11:44 am, Michael Ellerman wrote:
>>> Hari Bathini <hbathini at linux.ibm.com> writes:
>>>> In panic path, fadump is triggered via a panic notifier function.
>>>> Before calling panic notifier functions, smp_send_stop() gets called,
>>>> which stops all CPUs except the panic'ing CPU. Commit 8389b37dffdc
>>>> ("powerpc: stop_this_cpu: remove the cpu from the online map.") and
>>>> again commit bab26238bbd4 ("powerpc: Offline CPU in stop_this_cpu()")
>>>> started marking CPUs as offline while stopping them. So, if a kernel
>>>> has either of the above commits, vmcore captured with fadump via panic
>>>> path would show only the panic'ing CPU as online. Sample output of
>>>> crash-utility with such vmcore:
>>>>
>>>>     # crash vmlinux vmcore
>>>>     ...
>>>>           KERNEL: vmlinux
>>>>         DUMPFILE: vmcore  [PARTIAL DUMP]
>>>>             CPUS: 1
>>>>             DATE: Wed Nov 10 09:56:34 EST 2021
>>>>           UPTIME: 00:00:42
>>>>     LOAD AVERAGE: 2.27, 0.69, 0.24
>>>>            TASKS: 183
>>>>         NODENAME: XXXXXXXXX
>>>>          RELEASE: 5.15.0+
>>>>          VERSION: #974 SMP Wed Nov 10 04:18:19 CST 2021
>>>>          MACHINE: ppc64le  (2500 Mhz)
>>>>           MEMORY: 8 GB
>>>>            PANIC: "Kernel panic - not syncing: sysrq triggered crash"
>>>>              PID: 3394
>>>>          COMMAND: "bash"
>>>>             TASK: c0000000150a5f80  [THREAD_INFO: c0000000150a5f80]
>>>>              CPU: 1
>>>>            STATE: TASK_RUNNING (PANIC)
>>>>
>>>>     crash> p -x __cpu_online_mask
>>>>     __cpu_online_mask = $1 = {
>>>>       bits = {0x2, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}
>>>>     }
>>>>     crash>
>>>>     crash>
>>>>     crash> p -x __cpu_active_mask
>>>>     __cpu_active_mask = $2 = {
>>>>       bits = {0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}
>>>>     }
>>>>     crash>
>>>>
>>>> While this has been the case since fadump was introduced, the issue
>>>> was not identified for two probable reasons:
>>>>
>>>>     - In general, the bulk of the vmcores analyzed were from crash
>>>>       due to exception.
>>>>
>>>>     - The above did change since commit 8341f2f222d7 ("sysrq: Use
>>>>       panic() to force a crash") started using panic() instead of
>>>>       deferencing NULL pointer to force a kernel crash. But then
>>>>       commit de6e5d38417e ("powerpc: smp_send_stop do not offline
>>>>       stopped CPUs") stopped marking CPUs as offline till kernel
>>>>       commit bab26238bbd4 ("powerpc: Offline CPU in stop_this_cpu()")
>>>>       reverted that change.
>>>>
>>>> To avoid vmcore from showing only one CPU as online in panic path,
>>>> skip marking non panic'ing CPUs as offline while stopping them
>>>> during fadump crash.
>>>
>>> Is this really worth the added complexity/bug surface?
>>>
>>> Why does it matter if the vmcore shows only one CPU online?
>>
>> We lose out on backtrace/register data of non-crashing CPUs as they
>> are explicitly marked offline.
>>
>> Actually, the state of CPU resources is explicitly changed after the
>> panic though the aim of vmcore is to capture the system state at the
>> time of panic...
>>
>> Alternatively, how about moving crash_fadump() call from panic notifier
>> into panic() function explicitly, just like __crash_kexec() - before the
>> smp_send_stop() call, so as to remove dependency with smp_send_stop()
>> implementation altogether...
> 
> Does the crash dump code snapshot the CPUs with send_debuggr_break? I
> can't remember the exact flow. But if it sends NMI IPIs to all other
> CPUs then maybe it could be moved before smp_send_stop, good idea.

Except for crashing CPU, snapshot of register data for all other CPUs is
collected by f/w on ibm,os-term rtas call.


>>> The comment talks about printk_safe_flush_on_panic(), and this change
>>> would presumably break that. Except that printk_safe_flush_on_panic() no
>>> longer exists.
>>>
>>> So do we need to set the CPU online here at all?
>>>
>>> ie. could we revert bab26238bbd4 ("powerpc: Offline CPU in stop_this_cpu()")
>>> now that printk_safe_flush_on_panic() no longer exists?
>>
>> Yeah, sounds like the logical thing to do but I guess, Nick would be in
>> a better position to answer this..
> 
> Maybe we could look at reverting it, it would be nice. But I think it
> would be good to consider moving crash_fadump as well. There is at

OK, I will try moving crash_fadump() instead and post the patch..

Thanks
Hari