[PATCH v3] x86/hyperv: Handle unknown NMIs on one CPU when unknown_nmi_panic

[PATCH v3] x86/hyperv: Handle unknown NMIs on one CPU when unknown_nmi_panic

[Vitaly Kuznetsov]

There is a feature in Hyper-V (Debug-VM --InjectNonMaskableInterrupt) which
injects NMI to the guest. Prior to WS2016 the NMI is injected to all CPUs
of the guest and WS2016 injects it to CPU0 only. When unknown_nmi_panic is
enabled and we'd like to do kdump we need to perform some minimal cleanup
so the kdump kernel will be able to initialize VMBus devices, this cleanup
includes sending CHANNELMSG_UNLOAD to the host waiting for
CHANNELMSG_UNLOAD_RESPONSE to arrive. WS2012R2 always sends the response
to the CPU which was used to send CHANNELMSG_REQUESTOFFERS on VMBus module
load and not to the CPU which is sending CHANNELMSG_UNLOAD. As we can't do
any cross-CPU work reliably on crash we have vmbus_wait_for_unload()
function which tries to read CHANNELMSG_UNLOAD_RESPONSE on all CPUs message
pages and this sometimes works. It was discovered that in case the host
wants to send more than one message to a secondary CPU (not the CPU running
vmbus_wait_for_unload()) we're unable to get it as after reading the first
message we're supposed to do EOMing by doing wrmsrl(HV_X64_MSR_EOM, 0) but
this is per-CPU. I have a feeling that this was working some time ago when
I implemented vmbus_wait_for_unload(), the host was re-trying to deliver a
message even without wrmsrl() but apparently this doesn't work any more.
Unfortunately there is not that much we can do when all CPUs get NMI as
all but the first one are getting blocked with interrupts disabled. What we
can do is limit processing unknown interrupts to the first CPU which gets
it in case we're about to crash.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: K. Y. Srinivasan <kys@microsoft.com>
---
Changes since v2:
- Supply NMI_UNKNOWN instead of NMI_LOCAL for register_nmi_handler()
- Retain KY's ACK (hopefully stands with the above mentioned change)

Changes since v1:
- Put everything under #ifdef CONFIG_X86_LOCAL_APIC as unknown_nmi_panic is
  otherwise undefined. [kbuild test robot]
- Add KY's Ack (hopefully stands with the above mentioned change)
---
 arch/x86/kernel/cpu/mshyperv.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 8f44c5a..f228f74 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -31,6 +31,7 @@
 #include <asm/apic.h>
 #include <asm/timer.h>
 #include <asm/reboot.h>
+#include <asm/nmi.h>
 
 struct ms_hyperv_info ms_hyperv;
 EXPORT_SYMBOL_GPL(ms_hyperv);
@@ -158,6 +159,26 @@ static unsigned char hv_get_nmi_reason(void)
 	return 0;
 }
 
+#ifdef CONFIG_X86_LOCAL_APIC
+/*
+ * Prior to WS2016 Debug-VM sends NMIs to all CPUs which makes
+ * it dificult to process CHANNELMSG_UNLOAD in case of crash. Handle
+ * unknown NMI on the first CPU which gets it.
+ */
+static int hv_nmi_unknown(unsigned int val, struct pt_regs *regs)
+{
+	static atomic_t nmi_cpu = ATOMIC_INIT(-1);
+
+	if (!unknown_nmi_panic)
+		return NMI_DONE;
+
+	if (atomic_cmpxchg(&nmi_cpu, -1, raw_smp_processor_id()) != -1)
+		return NMI_HANDLED;
+
+	return NMI_DONE;
+}
+#endif
+
 static void __init ms_hyperv_init_platform(void)
 {
 	/*
@@ -183,6 +204,9 @@ static void __init ms_hyperv_init_platform(void)
 		pr_info("HyperV: LAPIC Timer Frequency: %#x\n",
 			lapic_timer_frequency);
 	}
+
+	register_nmi_handler(NMI_UNKNOWN, hv_nmi_unknown, NMI_FLAG_FIRST,
+			     "hv_nmi_unknown");
 #endif
 
 	if (ms_hyperv.features & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE)
-- 
2.9.3

Re: [PATCH v3] x86/hyperv: Handle unknown NMIs on one CPU when unknown_nmi_panic

[Thomas Gleixner]

Vitaly,

On Thu, 1 Dec 2016, Vitaly Kuznetsov wrote:

> There is a feature in Hyper-V (Debug-VM --InjectNonMaskableInterrupt) which
> injects NMI to the guest. Prior to WS2016 the NMI is injected to all CPUs
> of the guest and WS2016 injects it to CPU0 only. When unknown_nmi_panic is
> enabled and we'd like to do kdump we need to perform some minimal cleanup
> so the kdump kernel will be able to initialize VMBus devices, this cleanup
> includes sending CHANNELMSG_UNLOAD to the host waiting for
> CHANNELMSG_UNLOAD_RESPONSE to arrive. WS2012R2 always sends the response
> to the CPU which was used to send CHANNELMSG_REQUESTOFFERS on VMBus module
> load and not to the CPU which is sending CHANNELMSG_UNLOAD. As we can't do
> any cross-CPU work reliably on crash we have vmbus_wait_for_unload()
> function which tries to read CHANNELMSG_UNLOAD_RESPONSE on all CPUs message
> pages and this sometimes works. It was discovered that in case the host
> wants to send more than one message to a secondary CPU (not the CPU running
> vmbus_wait_for_unload()) we're unable to get it as after reading the first
> message we're supposed to do EOMing by doing wrmsrl(HV_X64_MSR_EOM, 0) but
> this is per-CPU. I have a feeling that this was working some time ago when
> I implemented vmbus_wait_for_unload(), the host was re-trying to deliver a
> message even without wrmsrl() but apparently this doesn't work any more.
> Unfortunately there is not that much we can do when all CPUs get NMI as
> all but the first one are getting blocked with interrupts disabled. What we
> can do is limit processing unknown interrupts to the first CPU which gets
> it in case we're about to crash.

This is completely unreadable and I really tried hard to make sense of it.

Please structure it in a way that people who are not familiar with the
inner workings of hyperv can at least understand the problem you are trying
to solve and the concept of the solution w/o needing to figure out what all
the acronyms and constants actually mean.

Also visual structuring in paragraphs helps readability a lot.

AFAICT this tries to deal with different problems of different hypervisor
versions, but even that is unclear as you talk about version WS2016,
versions prior to WS2016 and then about WS2012R2 in particular.

Another issue I have with this is:

	".... I have a feeling that this was working ...."

Changes like this are not about feelings. We want to have changes based on
facts.

Thanks,

	tglx

Re: [PATCH v3] x86/hyperv: Handle unknown NMIs on one CPU when unknown_nmi_panic

[Vitaly Kuznetsov]

Thomas Gleixner <tglx@linutronix.de> writes:

> Vitaly,
>
> On Thu, 1 Dec 2016, Vitaly Kuznetsov wrote:
>
>> There is a feature in Hyper-V (Debug-VM --InjectNonMaskableInterrupt) which
>> injects NMI to the guest. Prior to WS2016 the NMI is injected to all CPUs
>> of the guest and WS2016 injects it to CPU0 only. When unknown_nmi_panic is
>> enabled and we'd like to do kdump we need to perform some minimal cleanup
>> so the kdump kernel will be able to initialize VMBus devices, this cleanup
>> includes sending CHANNELMSG_UNLOAD to the host waiting for
>> CHANNELMSG_UNLOAD_RESPONSE to arrive. WS2012R2 always sends the response
>> to the CPU which was used to send CHANNELMSG_REQUESTOFFERS on VMBus module
>> load and not to the CPU which is sending CHANNELMSG_UNLOAD. As we can't do
>> any cross-CPU work reliably on crash we have vmbus_wait_for_unload()
>> function which tries to read CHANNELMSG_UNLOAD_RESPONSE on all CPUs message
>> pages and this sometimes works. It was discovered that in case the host
>> wants to send more than one message to a secondary CPU (not the CPU running
>> vmbus_wait_for_unload()) we're unable to get it as after reading the first
>> message we're supposed to do EOMing by doing wrmsrl(HV_X64_MSR_EOM, 0) but
>> this is per-CPU. I have a feeling that this was working some time ago when
>> I implemented vmbus_wait_for_unload(), the host was re-trying to deliver a
>> message even without wrmsrl() but apparently this doesn't work any more.
>> Unfortunately there is not that much we can do when all CPUs get NMI as
>> all but the first one are getting blocked with interrupts disabled. What we
>> can do is limit processing unknown interrupts to the first CPU which gets
>> it in case we're about to crash.
>
> This is completely unreadable and I really tried hard to make sense of it.
>
> Please structure it in a way that people who are not familiar with the
> inner workings of hyperv can at least understand the problem you are trying
> to solve and the concept of the solution w/o needing to figure out what all
> the acronyms and constants actually mean.
>
> Also visual structuring in paragraphs helps readability a lot.
>

Got it,

I'll try to do my best to make it readable.

> AFAICT this tries to deal with different problems of different hypervisor
> versions, but even that is unclear as you talk about version WS2016,
> versions prior to WS2016 and then about WS2012R2 in particular.
>
> Another issue I have with this is:
>
> 	".... I have a feeling that this was working ...."
>
> Changes like this are not about feelings. We want to have changes based on
> facts.
>

The thing is that Hyper-V is a (proprietary) software which gets updates
and I don't remember which particular updates were installed when I was
imlementing vmbus_wait_for_unload() but as far as I remember it was
always working on WS2012R2. Now I observe a different behavior ... 

-- 
  Vitaly