Ingo Molnar <mi...@kernel.org> writes:
> * Vitaly Kuznetsov <vkuzn...@redhat.com> wrote:
>
>> Ingo Molnar <mi...@kernel.org> writes:
>>
>> > * Stephen Hemminger <step...@networkplumber.org> wrote:
>> >
>> >> Fixes regression 4.3 mergw window in my config
>> >> where hyperv is enable but CONFIG_KEXEC not enabled.
>> >>
>> >> arch/x86/kernel/cpu/mshyperv.c:112: undefined reference to
>> >> `native_machine_crash_shutdown'
>> >>
>> >> Introduced by:
>> >> commit b4370df2b1f5158de028e167974263c5757b34a6
>> >> Author: Vitaly Kuznetsov <vkuzn...@redhat.com>
>> >> Date: Sat Aug 1 16:08:09 2015 -0700
>> >>
>> >> Drivers: hv: vmbus: add special crash handler
>> >>
>> >>
>> >> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
>> >>
>> >>
>> >> --- a/arch/x86/kernel/cpu/mshyperv.c 2015-09-07 10:11:24.994885115
>> >> -0700
>> >> +++ b/arch/x86/kernel/cpu/mshyperv.c 2015-09-07 10:14:20.995698615
>> >> -0700
>> >> @@ -109,7 +109,9 @@ static void hv_machine_crash_shutdown(st
>> >> {
>> >> if (hv_crash_handler)
>> >> hv_crash_handler(regs);
>> >> +#ifdef CONFIG_KEXEC
>> >> native_machine_crash_shutdown(regs);
>> >> +#endif
>> >
>> > I think there's another related bug as well:
>> >
>> > machine_ops.crash_shutdown = hv_machine_crash_shutdown;
>> >
>> > that should be #ifdef CONFIG_KEXEC as well AFAICS.
>> >
>>
>> Why? [...]
>
> Because you are bloating the kernel.
>
> That's because machine_ops.crash_shutdown() does nothing outside of kexec and
> that's the existing pattern in the native and KVM code. (Xen does it
> inconsistently as well.)
>
> So you bloat the kernel at minimum, and also confuse the reader what it's all
> about.
>
>> [...] crash_shutdown is defined in machine_ops unconditionally, I don't see
>> why
>> we _need_ #ifdef here (and btw Greg insisted on removing them).
>
> So arguably the kexec interface should be cleaned up as well, into something
> like:
>
> kexec_crash_handler_set(hv_machine_crash_shutdown);
>
> ... which would compile to no code at all in the !KEXEC case, and then we
> could
> also make ::crash_shutdown #ifdef KEXEC.
>
> At least one #ifdef is unavoidable unless we make KCONFIG an always-enabled
> facility - or merge it more intelligently with the regular
> reboot/shutdown code.
Greg,
I don't personally have a strong opinion here but I'm leaning towards
Ingo's suggestion. That would mean I have to break your 'no #ifdefs in C
files' rule (ok, I can move everything to mshyperv.h to respect it but
that would feel like a fraud). Please let me know if you prefer such fix
to the previously posted one (https://lkml.org/lkml/2015/8/11/417).
Thanks,
>
> I.e. I don't think there should be kexec specific 'handlers' per se - there
> should
> be reboot/shutdown handlers that will also serve kexec just fine.
>
> But until that's fixed we've got to make the best of the existing kexec
> design.
>
> Thanks,
>
> Ingo
--
Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
