[PATCH 0/6] kdump: disable virtualization extensions on crash (v2)

[PATCH 0/6] kdump: disable virtualization extensions on crash (v2)

[Eduardo Habkost]

Hi,

After some discussion, this is my second try to fix properly the kdump
hang when the kvm-intel module is loaded. I've kept the virtualization
code inside the KVM, and just used a function pointer that can be
registered by KVM to be called at crash time.


This series is for one of the alternatives for solving the problem. Other
alternatives that were discussed were:

a) Using a raw notifier chain (kernel/notifier.c) (the first version
   of this series)
   - This was dropped because the less code we have to rely on, the better,
     and it would make it too easy to break kdump
b) Always calling vmxoff, after hacking the #UD vector to ignore
   unknown opcode exceptions.
  - Too hackish in my opinion. And we don't need to hack #UD, anyway,
    if we can simply test CR4.VMXE (all our code that run vmxon/vmxoff
    toggle CR4.VMXE accordingly)
c) Moving the equivalent to the hardware_disable()/hardware_enable() API
   (and the vmx and svm implementations) from the KVM module to the kernel
   core, so the kernel core knows which CPUs have virtualization enabled,
   and disables virtualization without calling into the KVM module.
   - This wasn't completely discarded, but would move to the core a bunch
     of code that always lived in the KVM module.

-- 
Eduardo

[PATCH 1/6] kdump: crash-time virt disable function

[Eduardo Habkost]

This patch adds an interface to set a function to be called on crash time,
on each CPU. The function will be set by code that enables virtualization
extensions on the CPUs (i.e. KVM).  It will be called once on each CPU
by machine_crash_shutdown(), and should do the very least to disable
virt extensions on the CPU where it is being called.

The function will be used by KVM to disable virtualization before halting
the CPUs, otherwise the booting of the kdump kernel may hang. It does
hang, when vmx is enabled, and I wouldn't be surprised if having svm
enabled also causes problems.

The functions that set the function pointer uses RCU synchronization,
just in case the crash NMI is triggered while KVM is unloading.

We can't just use the same notifiers used at reboot time (that are used
by non-crash-dump kexec), because at crash time some CPUs may have IRQs
disabled, so we can't use IPIs. The crash shutdown code use NMIs to
tell the other CPUs to be halted, so the notifier call is hooked into
the CPU halting code that is on the crash shutdown NMI handler.

Signed-off-by: Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/include/asm/virtext.h |   24 +++++++++++++
 arch/x86/kernel/crash.c        |   71 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/include/asm/virtext.h

diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
new file mode 100644
index 0000000..773037c
--- /dev/null
+++ b/arch/x86/include/asm/virtext.h
@@ -0,0 +1,24 @@
+/* virtualization extensions handling code
+ */
+#ifndef _ASM_X86_VIRTEX_H
+#define _ASM_X86_VIRTEX_H
+
+#ifdef CONFIG_KEXEC
+
+int set_virt_disable_func(void (*fn)(unsigned int cpu));
+void clear_virt_disable_func(void);
+
+#else /* !CONFIG_KEXEC */
+
+inline int set_virt_disable_func(void (*fn)(unsigned int cpu))
+{
+	return 0;
+}
+
+inline void clear_virt_disable_func(void)
+{
+}
+
+#endif /* CONFIG_KEXEC */
+
+#endif /* _ASM_X86_VIRTEX_H */
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 2685538..6ed28e9 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -16,6 +16,7 @@
 #include <linux/delay.h>
 #include <linux/elf.h>
 #include <linux/elfcore.h>
+#include <linux/module.h>
 
 #include <asm/processor.h>
 #include <asm/hardirq.h>
@@ -32,6 +33,72 @@
 /* This keeps a track of which one is crashing cpu. */
 static int crashing_cpu;
 
+static DEFINE_SPINLOCK(virt_disable_lock);
+static void (*virt_disable_fn)(unsigned int cpu);
+
+/**	set function to be called to disable virtualization on crash
+ *
+ *	Registers a function to be called when CPUs are being halted at
+ *	machine_crash_shutdown().
+ *
+ *	There is only one function pointer, so the function
+ *	is reserved to be set by the KVM module at load time, before
+ *	enabling virtualization.
+ *
+ *	The function is called once on each online CPU, possibly
+ *	from a NMI handler. It should do the very least to allow the CPU
+ *	to be halted before booting the kdump kernel, as the kernel has
+ *	just crashed.
+ */
+int set_virt_disable_func(void (*fn)(unsigned int cpu))
+{
+	int r = 0;
+
+	spin_lock(&virt_disable_lock);
+	if (!virt_disable_fn)
+		rcu_assign_pointer(virt_disable_fn, fn);
+	else
+		r = -EEXIST;
+	spin_unlock(&virt_disable_lock);
+
+	return r;
+}
+EXPORT_SYMBOL(set_virt_disable_func);
+
+/** clear the virt_disable function set by set_virt_disable_func()
+ *
+ * You must call this function only if you sucessfully set
+ * the virt_disable function on a previous set_virt_disable_func()
+ * call.
+ *
+ * This function will use synchronize_sched() to wait until it's safe
+ * to free any data or code related to the previous existing virt_disable
+ * func, before returning.
+ */
+void clear_virt_disable_func(void)
+{
+	spin_lock(&virt_disable_lock);
+	rcu_assign_pointer(virt_disable_fn, NULL);
+	spin_unlock(&virt_disable_lock);
+
+	synchronize_sched();
+}
+EXPORT_SYMBOL(clear_virt_disable_func);
+
+/* Disable virtualization extensions if needed
+ *
+ * Runs thefunction set by set_virt_disable_func()
+ *
+ * Must be called on the CPU that is being halted.
+ */
+void virt_disable(unsigned int cpu)
+{
+	void (*fn)(unsigned int);
+	fn = rcu_dereference(virt_disable_fn);
+	if (fn)
+		fn(cpu);
+}
+
 #if defined(CONFIG_SMP) && defined(CONFIG_X86_LOCAL_APIC)
 static atomic_t waiting_for_crash_ipi;
 
@@ -65,6 +132,8 @@ static int crash_nmi_callback(struct notifier_block *self,
 	}
 #endif
 	crash_save_cpu(regs, cpu);
+
+	virt_disable(cpu);
 	disable_local_APIC();
 	atomic_dec(&waiting_for_crash_ipi);
 	/* Assume hlt works */
@@ -134,6 +203,8 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
 	/* Make a note of crashing cpu. Will be used in NMI callback.*/
 	crashing_cpu = safe_smp_processor_id();
 	nmi_shootdown_cpus();
+	virt_disable(crashing_cpu);
+
 	lapic_shutdown();
 #if defined(CONFIG_X86_IO_APIC)
 	disable_IO_APIC();
-- 
1.5.5.GIT

Re: [PATCH 1/6] kdump: crash-time virt disable function

[Avi Kivity]

Eduardo Habkost wrote:
> This patch adds an interface to set a function to be called on crash time,
> on each CPU. The function will be set by code that enables virtualization
> extensions on the CPUs (i.e. KVM).  It will be called once on each CPU
> by machine_crash_shutdown(), and should do the very least to disable
> virt extensions on the CPU where it is being called.
>
> The function will be used by KVM to disable virtualization before halting
> the CPUs, otherwise the booting of the kdump kernel may hang. It does
> hang, when vmx is enabled, and I wouldn't be surprised if having svm
> enabled also causes problems.
>
> The functions that set the function pointer uses RCU synchronization,
> just in case the crash NMI is triggered while KVM is unloading.
>
> We can't just use the same notifiers used at reboot time (that are used
> by non-crash-dump kexec), because at crash time some CPUs may have IRQs
> disabled, so we can't use IPIs. The crash shutdown code use NMIs to
> tell the other CPUs to be halted, so the notifier call is hooked into
> the CPU halting code that is on the crash shutdown NMI handler.
>
> +static void (*virt_disable_fn)(unsigned int cpu);
>   

Since you never use the cpu argument, I suggest dropping it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/6] kdump: crash-time virt disable function

[Eduardo Habkost]

On Thu, Oct 30, 2008 at 03:50:31PM +0200, Avi Kivity wrote:
<snip>
>>
>> +static void (*virt_disable_fn)(unsigned int cpu);
>>   
>
> Since you never use the cpu argument, I suggest dropping it.

It is a leftover from when I was using cpus_hardware_enabled to check
if vmx was enabled, before I've noticed I could just check CR4.VMXE. I
will drop it.

-- 
Eduardo

[PATCH 2/6] kvm_x86_ops: crash_hardware_disable() operation

[Eduardo Habkost]

The vmx hardware_disable() function does too much for the virt_disable
crash handler, so we will have a new operation for the crash-time
virt_disable case.

Signed-off-by: Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/include/asm/kvm_host.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 09e6c56..002023b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -426,6 +426,7 @@ struct kvm_x86_ops {
 	int (*disabled_by_bios)(void);             /* __init */
 	void (*hardware_enable)(void *dummy);      /* __init */
 	void (*hardware_disable)(void *dummy);
+	void (*crash_hardware_disable)(void *dummy);
 	void (*check_processor_compatibility)(void *rtn);
 	int (*hardware_setup)(void);               /* __init */
 	void (*hardware_unsetup)(void);            /* __exit */
-- 
1.5.5.GIT

[PATCH 3/6] kvm: svm: set crash_hardware_disable to svm_hardware_disable

[Eduardo Habkost]

We can use it as is, as it will work even if virtualization was already
disabled.

Signed-off-by: Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kvm/svm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f0ad4d4..6b3a660 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1942,6 +1942,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.check_processor_compatibility = svm_check_processor_compat,
 	.hardware_enable = svm_hardware_enable,
 	.hardware_disable = svm_hardware_disable,
+	.crash_hardware_disable = svm_hardware_disable,
 	.cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr,
 
 	.vcpu_create = svm_create_vcpu,
-- 
1.5.5.GIT

[PATCH 4/6] kvm: vmx: crash_hardware_disable function

[Eduardo Habkost]

We need to first check if virtualization was enabled. We do this by
checking CR4.VMXE. If it is set, run vmxoff and clear CR4.VMXE.

Signed-off-by: Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kvm/vmx.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ece9cb7..0f4e191 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1091,13 +1091,24 @@ static void vmclear_local_vcpus(void)
 		__vcpu_clear(vmx);
 }
 
-static void hardware_disable(void *garbage)
+static void __hardware_disable(void)
 {
-	vmclear_local_vcpus();
 	asm volatile (__ex(ASM_VMX_VMXOFF) : : : "cc");
 	write_cr4(read_cr4() & ~X86_CR4_VMXE);
 }
 
+static void hardware_disable(void *garbage)
+{
+	vmclear_local_vcpus();
+	__hardware_disable();
+}
+
+static void crash_hardware_disable(void *garbage)
+{
+	if (read_cr4() & X86_CR4_VMXE)
+		__hardware_disable();
+}
+
 static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
 				      u32 msr, u32 *result)
 {
@@ -3589,6 +3600,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.check_processor_compatibility = vmx_check_processor_compat,
 	.hardware_enable = hardware_enable,
 	.hardware_disable = hardware_disable,
+	.crash_hardware_disable = crash_hardware_disable,
 	.cpu_has_accelerated_tpr = cpu_has_vmx_virtualize_apic_accesses,
 
 	.vcpu_create = vmx_create_vcpu,
-- 
1.5.5.GIT

[PATCH 5/6] kvmx: x86: set kvm_x86_ops earlier on kvm_arch_init()

[Eduardo Habkost]

Small change that will be needed when we use set_virt_disable_func()
on kvm_arch_init().

Signed-off-by: Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kvm/x86.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 38f79b6..ff895e2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2603,19 +2603,22 @@ int kvm_arch_init(void *opaque)
 		goto out;
 	}
 
+	kvm_x86_ops = ops;
+
 	r = kvm_mmu_module_init();
 	if (r)
-		goto out;
+		goto out_clear_ops;
 
 	kvm_init_msr_list();
 
-	kvm_x86_ops = ops;
 	kvm_mmu_set_nonpresent_ptes(0ull, 0ull);
 	kvm_mmu_set_base_ptes(PT_PRESENT_MASK);
 	kvm_mmu_set_mask_ptes(PT_USER_MASK, PT_ACCESSED_MASK,
 			PT_DIRTY_MASK, PT64_NX_MASK, 0, 0);
 	return 0;
 
+out_clear_ops:
+	kvm_x86_ops = NULL;
 out:
 	return r;
 }
-- 
1.5.5.GIT

[PATCH 6/6] kvm: x86: set kdump virt_disable function on initialization

[Eduardo Habkost]

Finally implement the virt_disable function for kdump. It will call
kvm_x86_ops->crash_hardware_disable(), that will disable virtualization
extensions on the CPU if it is not disabled yet.

Signed-off-by: Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kvm/x86.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ff895e2..4f7a221 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -40,6 +40,7 @@
 #include <asm/msr.h>
 #include <asm/desc.h>
 #include <asm/mtrr.h>
+#include <asm/virtext.h>
 
 #define MAX_IO_MSRS 256
 #define CR0_RESERVED_BITS						\
@@ -2581,6 +2582,13 @@ int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_pio_string);
 
+/* Called at crash time, so we can disable virtualization if needed
+ */
+static void crash_hardware_disable(unsigned int cpu)
+{
+	kvm_x86_ops->crash_hardware_disable(NULL);
+}
+
 int kvm_arch_init(void *opaque)
 {
 	int r;
@@ -2605,9 +2613,15 @@ int kvm_arch_init(void *opaque)
 
 	kvm_x86_ops = ops;
 
+	r = set_virt_disable_func(crash_hardware_disable);
+	if (r) {
+		printk(KERN_ERR "kvm: virt_disable function already set?\n");
+		goto out_clear_ops;
+	}
+
 	r = kvm_mmu_module_init();
 	if (r)
-		goto out_clear_ops;
+		goto out_clear_crash;
 
 	kvm_init_msr_list();
 
@@ -2617,6 +2631,8 @@ int kvm_arch_init(void *opaque)
 			PT_DIRTY_MASK, PT64_NX_MASK, 0, 0);
 	return 0;
 
+out_clear_crash:
+	clear_virt_disable_func();
 out_clear_ops:
 	kvm_x86_ops = NULL;
 out:
@@ -2625,6 +2641,7 @@ out:
 
 void kvm_arch_exit(void)
 {
+	clear_virt_disable_func();
 	kvm_x86_ops = NULL;
 	kvm_mmu_module_exit();
 }
-- 
1.5.5.GIT

Re: [PATCH 0/6] kdump: disable virtualization extensions on crash (v2)

[Avi Kivity]

Eduardo Habkost wrote:
> After some discussion, this is my second try to fix properly the kdump
> hang when the kvm-intel module is loaded. I've kept the virtualization
> code inside the KVM, and just used a function pointer that can be
> registered by KVM to be called at crash time.
>
>   

Looks good.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.