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

[Ingo Molnar]

* Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:

> Ingo Molnar <mingo@xxxxxxxxxx> writes:
> 
> > * Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>    Date:   Sat Aug 1 16:08:09 2015 -0700
> >> 
> >>        Drivers: hv: vmbus: add special crash handler
> >> 
> >> 
> >> Signed-off-by: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>
> >> 
> >> 
> >> --- 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/