Hi Dengcheng,
On Thu, Sep 06, 2018 at 03:23:13PM -0700, Dengcheng Zhu wrote:
> Regarding cache flushing in machine_kexec(), I recommend simply removing
> __flush_cache_all() as CPU0 will do local icache flush before jumping to
> reboot_code_buffer, and other CPUs don't jump at all.
Unfortunately that doesn't work - what if CPU 1 runs machine_kexec() &
writes to reboot_code_buffer, then CPU 0 fetches stale/garbage code from
L2 into its icache?
The icache flush I added in kexec_this_cpu() isn't enough in all systems
unless the CPU that wrote the code writes it back far enough for a
remote CPU to observe. The existing __flush_cache_all() is one
heavy-handed way to achieve that.
> Re: marking CPU offline in kexec_this_cpu(), it's probably good NOT doing
> it either, as the system is going to reboot from CPU0 soon.
...but the new kernel will have no knowledge of whether the old kernel
had CPUs marked online or offline, so I don't follow the argument?
Marking CPUs offline does have the advantage that we won't try to IPI
them. This just makes perfect sense to me, and note that both arch/arm &
arch/x86 offline CPUs during kexec too (I *really* like the way arch/arm
just uses disable_nonboot_cpus() to go through the usual hotplug
sequence in the non-crash case).
> HALT is good enough, no need to gate core power.
> As to whether it's safe running play_dead() in parallel, it shouldn't
> be a problem, as the loop is based
> on cpu online mask (which we don't mark offline as mentioned above) -- The
> CPU will simply halt itself. For non-MT CPUs, play_dead() does make sense
> as well, as it's supposed to stop this CPU's execution, getting ready to
> reboot from CPU0.
That feels like relying on play_dead() accidentally working rather than
doing something it's designed for though, and it would come at the
expense of not marking CPUs offline & the associated fragility of
needing to avoid anything that might IPI them (like some cache flush
functions, as we've seen). I'd much rather we figure out a way to do
this without all that.
One option might be to add something like arch/x86's stop_other_cpus &
crash_stop_other_cpus callbacks in struct plat_smp_ops, which we can
then implement as appropriate. We'd hopefully still reuse some of the
code from play_dead() implementations, but have the separation to allow
them to function differently where needed (eg. the new callbacks could
halt all threads in MT systems without caring about cpu_online_mask).
This would also give us a natural way to check whether a system actually
supports kexec properly, as we could just return with an error from
machine_kexec_prepare() if the stop_other_cpus callback isn't
implemented for the system (ie. is NULL).
Thanks,
Paul
