Re: [PATCH v4 0/6] MIPS: kexec/kdump: Fix smp reboot and other issues

[Dengcheng Zhu <dzhu@wavecomp.com>]

Hi Paul,

On 09/05/2018 03:54 PM, Paul Burton wrote:
> Hi Dengcheng,
>
> On Wed, Sep 05, 2018 at 08:59:03AM -0700, Dengcheng Zhu wrote:
>> The issues are mentioned in patches 1/4/5/6. I will update kdump
>> documentation for MIPS if the series gets accepted. Testing has been done
>> on single core i6500/Boston with IOCU, dual core i6500 without IOCU, and
>> dual core interAptiv without IOCU.
>>
>> Changes:
>>
>> v4 - v3:
>> * In patch #1, idle_task_exit() is moved out from play_dead() to its sole
>>    caller arch_cpu_idle_dead(). So no interface change of play_dead().
>> * In patch #6, the kexec_prepare method for the Generic platform is defined
>>    as uhi_machine_kexec_prepare() for all platforms using UHI boot protocol.
> Thanks! I've applied patches 1,2,3,5 to a test branch with a few
> changes:
>
>      git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git test-kexec
>
>      https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/log/?h=test-kexec
>
> I didn't apply patch 4 because I'm not sure it's correct & I believe the
> changes in the branch above should take care of it - CPUs that reach
> kexec_this_cpu() are maked offline, so they shouldn't be IPI'd by
> __flush_cache_all().

I believe patch 4 is necessary. As mentioned in the code comment and patch
comment of that patch, machine_crash_shutdown() is called prior to
machine_kexec() in the kdump sequence. So other CPUs have disabled local
IRQs waiting for the reboot signal.

In fact, in kexec_this_cpu() [you renamed and modified kexec_smp_reboot()],
the added marking CPU offline will cause system hang (tested). This is
because it will change how play_dead() will work.

> The CPU that runs machine_kexec() should still
> flush its dcache (& the L2), and then CPU 0 invalidates its icache in
> kexec_this_cpu() prior to jumping into reboot_code_buffer.
>
> I'm also still not sure about patch 6 - since no platforms besides the
> arch/mips/generic/ make use of the UHI boot code yet I think it'd be
> best to leave as-is. If we do ever need to use it from another platform
> then we can deal with the problem then. If an out of tree platform needs
> to use it then for now it could copy generic_kexec_prepare() and deal
> with removing the duplication when it heads upstream.

Understood. It really depends on how this problem is viewed. If patch #6
is considered creating a framework for future UHI platforms, then it has
the following facts:

* It doesn't create code redundancy. I mean, it does not add unnecessary
   code to the kernel.

* Out of tree platforms will get access to this functionality by a simple
   "select UHI_BOOT". When the kernel developer of an out-of-tree platform
   wants to make kexec work, they will naturally look at machine_kexec.c,
   where they will find this UHI stuff, obviously telling them to do
   "select UHI_BOOT". Otherwise, unless they google onto this discussion
   thread, it's harder to know the solution to the kexec_args related
   problem hides in the code of another platform (Generic).

* It simplifies work if the out of tree platform wants to upstream.

>
> Could you take a look & let me know if you see any problems?
>
> Thanks,
>      Paul

Thanks,

Dengcheng

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

*From:* Paul Burton <mailto:paul.burton@mips.com>
*Sent:* Wednesday, September 05, 2018 3:54PM
*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 v4 0/6] MIPS: kexec/kdump: Fix smp reboot and other 
issues

Hi Dengcheng,

On Wed, Sep 05, 2018 at 08:59:03AM -0700, Dengcheng Zhu wrote:

> The issues are mentioned in patches 1/4/5/6. I will update kdump
> documentation for MIPS if the series gets accepted. Testing has been done
> on single core i6500/Boston with IOCU, dual core i6500 without IOCU, and
> dual core interAptiv without IOCU.
>
> Changes:
>
> v4 - v3:
> * In patch #1, idle_task_exit() is moved out from play_dead() to its sole
>    caller arch_cpu_idle_dead(). So no interface change of play_dead().
> * In patch #6, the kexec_prepare method for the Generic platform is defined
>    as uhi_machine_kexec_prepare() for all platforms using UHI boot protocol.

Thanks! I've applied patches 1,2,3,5 to a test branch with a few
changes:

     git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git test-kexec

     https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/log/?h=test-kexec

I didn't apply patch 4 because I'm not sure it's correct & I believe the
changes in the branch above should take care of it - CPUs that reach
kexec_this_cpu() are maked offline, so they shouldn't be IPI'd by
__flush_cache_all(). The CPU that runs machine_kexec() should still
flush its dcache (& the L2), and then CPU 0 invalidates its icache in
kexec_this_cpu() prior to jumping into reboot_code_buffer.

I'm also still not sure about patch 6 - since no platforms besides the
arch/mips/generic/ make use of the UHI boot code yet I think it'd be
best to leave as-is. If we do ever need to use it from another platform
then we can deal with the problem then. If an out of tree platform needs
to use it then for now it could copy generic_kexec_prepare() and deal
with removing the duplication when it heads upstream.

Could you take a look & let me know if you see any problems?

Thanks,
     Paul