[PATCH] hyperv: fix build if KEXEC not enabled

[PATCH] hyperv: fix build if KEXEC not enabled

[Stephen Hemminger]

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
 }
 
 

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

[Ingo Molnar]

* 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.

These bugs came upstream via the driver tree:

 b4370df2b1f5 ("Drivers: hv: vmbus: add special crash handler")
 2517281d63a2 ("Drivers: hv: vmbus: add special kexec handler")

Thanks,

	Ingo

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

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> 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
>  }

Greg in particular was against #ifdefs in C code and I sent the
following patch to fix the issue:

https://lkml.org/lkml/2015/8/11/417

-- 
  Vitaly

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

[Vitaly Kuznetsov]

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? crash_shutdown is defined in machine_ops unconditionally, I don't
see why we _need_ #ifdef here (and btw Greg insisted on removing them).

> These bugs came upstream via the driver tree:
>
>  b4370df2b1f5 ("Drivers: hv: vmbus: add special crash handler")
>  2517281d63a2 ("Drivers: hv: vmbus: add special kexec handler")
>
> Thanks,
>
> 	Ingo

-- 
  Vitaly

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

[Ingo Molnar]

* Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > 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
> >  }
> 
> Greg in particular was against #ifdefs in C code and I sent the
> following patch to fix the issue:
> 
> https://lkml.org/lkml/2015/8/11/417

Note that this is still somewhat buggy, the whole hv_machine_crash_shutdown() 
should be conditional - like in kvmclock.c.

Thanks,

	Ingo

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

[Ingo Molnar]

* 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.

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

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

[Vitaly Kuznetsov]

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