Re: [PATCH v3 1/6] MIPS: Make play_dead() work for kexec

[Dengcheng Zhu <dzhu@wavecomp.com>]

Hi Paul,


On 08/30/2018 04:32 PM, Paul Burton wrote:
> Hi Dengcheng,
>
> On Mon, Jul 30, 2018 at 11:50:38AM -0700, Dengcheng Zhu wrote:
>> On 07/24/2018 04:23 PM, Paul Burton wrote:
>>> On Mon, Jul 23, 2018 at 07:48:14AM -0700, Dengcheng Zhu wrote:
>>>> Extract play_dead() from CONFIG_HOTPLUG_CPU and share with CONFIG_KEXEC.
>>>> Also, add one parameter to it to avoid doing unnecessary things in the case
>>>> of kexec.
>>> I'd prefer that we use a separate function to play_dead() for this, for
>>> example we could provide an implementation of crash_smp_send_stop() much
>>> like ARM's which invokes a machine_crash_nonpanic_core() function on all
>>> CPUs other than the crash CPU.
>> %
>>> This would prevent the kexec/kdump functionality from depending on the
>>> board/platform specific play_dead(), and wouldn't need these changes to
>>> all of the implementations of play_dead().
>> The revised play_dead() is JUST to make sure we are turning off CPUs cleanly.
>> This function itself already hides board/platform details. So it seems a good
>> candidate for turning off CPUs for the target platform.
>>
>> This function is called only in the newly created kexec_smp_reboot(), before
>> which cpu states have been saved.
> I can see the appeal, but please see my reply from just now (which is
> accidentally in response to v2, but still applies to v3) about cleaning
> up the changes to play_dead() a little. I think that would help it feel
> "nicer" to me.
>
>>> We should also be calling crash_save_cpu() on each CPU, which is a
>>> further difference from play_dead().
>> crash_save_cpu() is already called in machine_crash_shutdown(), which is
>> prior to machine_kexec().
> But that only happens on one CPU, right? See the comment in
> crash_kexec(). So aren't we missing the register state for all the other
> CPUs?

No, we are not missing the state for other CPUs. They do crash_save_cpu() in
crash_shutdown_secondary().


Thanks,

Dengcheng

---------------------------------------------------------------------------

*From:* Paul Burton <mailto:paul.burton@mips.com>
*Sent:* Thursday, August 30, 2018 4:32PM
*To:* Dengcheng Zhu <mailto:dzhu@wavecomp.com>
*Cc:* Pburton <mailto:pburton@wavecomp.com>, Ralf 
<mailto:ralf@linux-mips.org>, Linux-mips 
<mailto:linux-mips@linux-mips.org>, Rachel.mozes 
<mailto:rachel.mozes@intel.com>
*Subject:* Re: [PATCH v3 1/6] MIPS: Make play_dead() work for kexec

Hi Dengcheng,

On Mon, Jul 30, 2018 at 11:50:38AM -0700, Dengcheng Zhu wrote:

> On 07/24/2018 04:23 PM, Paul Burton wrote:
>> On Mon, Jul 23, 2018 at 07:48:14AM -0700, Dengcheng Zhu wrote:
>>> Extract play_dead() from CONFIG_HOTPLUG_CPU and share with CONFIG_KEXEC.
>>> Also, add one parameter to it to avoid doing unnecessary things in the case
>>> of kexec.
>> I'd prefer that we use a separate function to play_dead() for this, for
>> example we could provide an implementation of crash_smp_send_stop() much
>> like ARM's which invokes a machine_crash_nonpanic_core() function on all
>> CPUs other than the crash CPU.
> %
>> This would prevent the kexec/kdump functionality from depending on the
>> board/platform specific play_dead(), and wouldn't need these changes to
>> all of the implementations of play_dead().
> The revised play_dead() is JUST to make sure we are turning off CPUs cleanly.
> This function itself already hides board/platform details. So it seems a good
> candidate for turning off CPUs for the target platform.
>
> This function is called only in the newly created kexec_smp_reboot(), before
> which cpu states have been saved.

I can see the appeal, but please see my reply from just now (which is
accidentally in response to v2, but still applies to v3) about cleaning
up the changes to play_dead() a little. I think that would help it feel
"nicer" to me.

>> We should also be calling crash_save_cpu() on each CPU, which is a
>> further difference from play_dead().
> crash_save_cpu() is already called in machine_crash_shutdown(), which is
> prior to machine_kexec().

But that only happens on one CPU, right? See the comment in
crash_kexec(). So aren't we missing the register state for all the other
CPUs?

Thanks,
     Paul