lkml.org 
[lkml]   [2015]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] hyperv: fix build if KEXEC not enabled
Date
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


\
 
 \ /
  Last update: 2015-09-09 00:01    [W:0.053 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site