Re: [PATCH] hyperv: fix build if KEXEC not enabled

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Ingo Molnar <mingo@kernel.org> writes:

> * Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Ingo Molnar <mingo@kernel.org> writes:
>> 
>> > * Stephen Hemminger <stephen@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 <vkuznets@redhat.com>
>> >>    Date:   Sat Aug 1 16:08:09 2015 -0700
>> >> 
>> >>        Drivers: hv: vmbus: add special crash handler
>> >> 
>> >> 
>> >> Signed-off-by: Stephen Hemminger <stephen@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