[PATCH v9 0/2] x86: vmclear vmcss on all cpus when doing kdump if necessary

[PATCH v9 0/2] x86: vmclear vmcss on all cpus when doing kdump if necessary

[Zhang Yanfei]

Currently, kdump just makes all the logical processors leave VMX operation by
executing VMXOFF instruction, so any VMCSs active on the logical processors may
be corrupted. But, sometimes, we need the VMCSs to debug guest images contained
in the host vmcore. To prevent the corruption, we should VMCLEAR the VMCSs before
executing the VMXOFF instruction.

The patch set provides a way to VMCLEAR vmcss related to guests on all cpus before
executing the VMXOFF when doing kdump. This is used to ensure the VMCSs in the
vmcore updated and non-corrupted.

Changelog from v8 to v9:
1. KEXEC: use a callback function instead of a notifier.
2. KVM-INTEL: use a new vmclear function instead of just calling 
   vmclear_local_loaded_vmcss to make sure we just do the core vmclear
   operation in kdump.

Changelog from v7 to v8:
1. KEXEC: regression for using name crash_notifier_list
   and remove comments related to KVM
   and just call function atomic_notifier_call_chain directly.

Changelog from v6 to v7:
1. KVM-INTEL: in hardware_disable, we needn't disable the
   vmclear, so remove it.

Changelog from v5 to v6:
1. KEXEC: the atomic notifier list renamed:
   crash_notifier_list --> vmclear_notifier_list
2. KVM-INTEL: provide empty functions if CONFIG_KEXEC is
   not defined and remove unnecessary #ifdef's.

Changelog from v4 to v5:
1. use an atomic notifier instead of function call, so
   have all the vmclear codes in vmx.c.

Changelog from v3 to v4:
1. add a new percpu variable vmclear_skipped to skip
   vmclear in kdump in some conditions.

Changelog from v2 to v3:
1. remove unnecessary conditions in function
   cpu_emergency_clear_loaded_vmcss as Marcelo suggested.

Changelog from v1 to v2:
1. remove the sysctl and clear VMCSs unconditionally.

Zhang Yanfei (2):
  x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary
  KVM-INTEL: provide the vmclear function and a bitmap to support
    VMCLEAR in kdump

 arch/x86/include/asm/kexec.h |    2 +
 arch/x86/kernel/crash.c      |   25 ++++++++++++++++
 arch/x86/kvm/vmx.c           |   65 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 0 deletions(-)

[PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary

[Zhang Yanfei]

This patch provides a way to VMCLEAR VMCSs related to guests
on all cpus before executing the VMXOFF when doing kdump. This
is used to ensure the VMCSs in the vmcore updated and
non-corrupted.

Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 arch/x86/include/asm/kexec.h |    2 ++
 arch/x86/kernel/crash.c      |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
index 317ff17..28feeba 100644
--- a/arch/x86/include/asm/kexec.h
+++ b/arch/x86/include/asm/kexec.h
@@ -163,6 +163,8 @@ struct kimage_arch {
 };
 #endif
 
+extern void (*crash_vmclear_loaded_vmcss)(void);
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_X86_KEXEC_H */
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 13ad899..4a2a12f 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>
@@ -29,6 +30,20 @@
 #include <asm/virtext.h>
 
 int in_crash_kexec;
+ 
+/*
+ * This is used to VMCLEAR all VMCSs loaded on the
+ * processor. And when loading kvm_intel module, the
+ * callback function pointer will be assigned.
+ */
+void (*crash_vmclear_loaded_vmcss)(void) = NULL;
+EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss);
+
+static inline void cpu_emergency_vmclear_loaded_vmcss(void)
+{
+	if (crash_vmclear_loaded_vmcss)
+		crash_vmclear_loaded_vmcss();
+}
 
 #if defined(CONFIG_SMP) && defined(CONFIG_X86_LOCAL_APIC)
 
@@ -46,6 +61,11 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
 #endif
 	crash_save_cpu(regs, cpu);
 
+	/*
+	 * VMCLEAR VMCSs loaded on all cpus if needed.
+	 */
+	cpu_emergency_vmclear_loaded_vmcss();
+
 	/* Disable VMX or SVM if needed.
 	 *
 	 * We need to disable virtualization on all CPUs.
@@ -88,6 +108,11 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
 
 	kdump_nmi_shootdown_cpus();
 
+	/*
+	 * VMCLEAR VMCSs loaded on this cpu if needed.
+	 */
+	cpu_emergency_vmclear_loaded_vmcss();
+
 	/* Booting kdump kernel with VMX or SVM enabled won't work,
 	 * because (among other limitations) we can't disable paging
 	 * with the virt flags.
-- 
1.7.1

[PATCH v9 2/2] KVM-INTEL: provide the vmclear function and a bitmap to support VMCLEAR in kdump

[Zhang Yanfei]

The vmclear function will be assigned to the callback function pointer
when loading kvm-intel module. And the bitmap indicates whether we
should do VMCLEAR operation in kdump. The bits in the bitmap are
set/unset according to different conditions.

Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 arch/x86/kvm/vmx.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4ff0ab9..561d3b6 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -41,6 +41,7 @@
 #include <asm/i387.h>
 #include <asm/xcr.h>
 #include <asm/perf_event.h>
+#include <asm/kexec.h>
 
 #include "trace.h"
 
@@ -963,6 +964,46 @@ static void vmcs_load(struct vmcs *vmcs)
 		       vmcs, phys_addr);
 }
 
+#ifdef CONFIG_KEXEC
+/*
+ * This bitmap is used to indicate whether the vmclear
+ * operation is enabled on all cpus. All disabled by
+ * default.
+ */
+static cpumask_t crash_vmclear_enabled_bitmap = CPU_MASK_NONE;
+
+static inline void crash_enable_local_vmclear(int cpu)
+{
+	cpumask_set_cpu(cpu, &crash_vmclear_enabled_bitmap);
+}
+
+static inline void crash_disable_local_vmclear(int cpu)
+{
+	cpumask_clear_cpu(cpu, &crash_vmclear_enabled_bitmap);
+}
+
+static inline int crash_local_vmclear_enabled(int cpu)
+{
+	return cpumask_test_cpu(cpu, &crash_vmclear_enabled_bitmap);
+}
+
+static void crash_vmclear_local_loaded_vmcss(void)
+{
+	int cpu = raw_smp_processor_id();
+	struct loaded_vmcs *v;
+
+	if (!crash_local_vmclear_enabled(cpu))
+		return;
+
+	list_for_each_entry(v, &per_cpu(loaded_vmcss_on_cpu, cpu),
+			    loaded_vmcss_on_cpu_link)
+		vmcs_clear(v->vmcs);
+}
+#else
+static inline void crash_enable_local_vmclear(int cpu) { }
+static inline void crash_disable_local_vmclear(int cpu) { }
+#endif /* CONFIG_KEXEC */
+
 static void __loaded_vmcs_clear(void *arg)
 {
 	struct loaded_vmcs *loaded_vmcs = arg;
@@ -972,8 +1013,10 @@ static void __loaded_vmcs_clear(void *arg)
 		return; /* vcpu migration can race with cpu offline */
 	if (per_cpu(current_vmcs, cpu) == loaded_vmcs->vmcs)
 		per_cpu(current_vmcs, cpu) = NULL;
+	crash_disable_local_vmclear(cpu);
 	list_del(&loaded_vmcs->loaded_vmcss_on_cpu_link);
 	loaded_vmcs_init(loaded_vmcs);
+	crash_enable_local_vmclear(cpu);
 }
 
 static void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs)
@@ -1491,8 +1534,10 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		local_irq_disable();
+		crash_disable_local_vmclear(cpu);
 		list_add(&vmx->loaded_vmcs->loaded_vmcss_on_cpu_link,
 			 &per_cpu(loaded_vmcss_on_cpu, cpu));
+		crash_enable_local_vmclear(cpu);
 		local_irq_enable();
 
 		/*
@@ -2302,6 +2347,18 @@ static int hardware_enable(void *garbage)
 		return -EBUSY;
 
 	INIT_LIST_HEAD(&per_cpu(loaded_vmcss_on_cpu, cpu));
+
+	/*
+	 * Now we can enable the vmclear operation in kdump
+	 * since the loaded_vmcss_on_cpu list on this cpu
+	 * has been initialized.
+	 *
+	 * Though the cpu is not in VMX operation now, there
+	 * is no problem to enable the vmclear operation
+	 * for the loaded_vmcss_on_cpu list is empty!
+	 */
+	crash_enable_local_vmclear(cpu);
+
 	rdmsrl(MSR_IA32_FEATURE_CONTROL, old);
 
 	test_bits = FEATURE_CONTROL_LOCKED;
@@ -7230,6 +7287,10 @@ static int __init vmx_init(void)
 	if (r)
 		goto out3;
 
+#ifdef CONFIG_KEXEC
+	crash_vmclear_loaded_vmcss = crash_vmclear_local_loaded_vmcss;
+#endif
+
 	vmx_disable_intercept_for_msr(MSR_FS_BASE, false);
 	vmx_disable_intercept_for_msr(MSR_GS_BASE, false);
 	vmx_disable_intercept_for_msr(MSR_KERNEL_GS_BASE, true);
@@ -7265,6 +7326,10 @@ static void __exit vmx_exit(void)
 	free_page((unsigned long)vmx_io_bitmap_b);
 	free_page((unsigned long)vmx_io_bitmap_a);
 
+#ifdef CONFIG_KEXEC
+	crash_vmclear_loaded_vmcss = NULL;
+#endif
+
 	kvm_exit();
 }
 
-- 
1.7.1

Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary

[Gleb Natapov]

Eric, can you ACK it?

On Tue, Nov 27, 2012 at 11:26:02AM +0800, Zhang Yanfei wrote:
> This patch provides a way to VMCLEAR VMCSs related to guests
> on all cpus before executing the VMXOFF when doing kdump. This
> is used to ensure the VMCSs in the vmcore updated and
> non-corrupted.
> 
> Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> ---
>  arch/x86/include/asm/kexec.h |    2 ++
>  arch/x86/kernel/crash.c      |   25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index 317ff17..28feeba 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -163,6 +163,8 @@ struct kimage_arch {
>  };
>  #endif
>  
> +extern void (*crash_vmclear_loaded_vmcss)(void);
> +
>  #endif /* __ASSEMBLY__ */
>  
>  #endif /* _ASM_X86_KEXEC_H */
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 13ad899..4a2a12f 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>
> @@ -29,6 +30,20 @@
>  #include <asm/virtext.h>
>  
>  int in_crash_kexec;
> + 
> +/*
> + * This is used to VMCLEAR all VMCSs loaded on the
> + * processor. And when loading kvm_intel module, the
> + * callback function pointer will be assigned.
> + */
> +void (*crash_vmclear_loaded_vmcss)(void) = NULL;
> +EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss);
> +
> +static inline void cpu_emergency_vmclear_loaded_vmcss(void)
> +{
> +	if (crash_vmclear_loaded_vmcss)
> +		crash_vmclear_loaded_vmcss();
> +}
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_X86_LOCAL_APIC)
>  
> @@ -46,6 +61,11 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
>  #endif
>  	crash_save_cpu(regs, cpu);
>  
> +	/*
> +	 * VMCLEAR VMCSs loaded on all cpus if needed.
> +	 */
> +	cpu_emergency_vmclear_loaded_vmcss();
> +
>  	/* Disable VMX or SVM if needed.
>  	 *
>  	 * We need to disable virtualization on all CPUs.
> @@ -88,6 +108,11 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
>  
>  	kdump_nmi_shootdown_cpus();
>  
> +	/*
> +	 * VMCLEAR VMCSs loaded on this cpu if needed.
> +	 */
> +	cpu_emergency_vmclear_loaded_vmcss();
> +
>  	/* Booting kdump kernel with VMX or SVM enabled won't work,
>  	 * because (among other limitations) we can't disable paging
>  	 * with the virt flags.
> -- 
> 1.7.1

--
			Gleb.

Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary

[Gleb Natapov]

On Tue, Nov 27, 2012 at 02:18:47PM +0200, Gleb Natapov wrote:
> Eric, can you ACK it?
> 
Eric, ping.

> On Tue, Nov 27, 2012 at 11:26:02AM +0800, Zhang Yanfei wrote:
> > This patch provides a way to VMCLEAR VMCSs related to guests
> > on all cpus before executing the VMXOFF when doing kdump. This
> > is used to ensure the VMCSs in the vmcore updated and
> > non-corrupted.
> > 
> > Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> > ---
> >  arch/x86/include/asm/kexec.h |    2 ++
> >  arch/x86/kernel/crash.c      |   25 +++++++++++++++++++++++++
> >  2 files changed, 27 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> > index 317ff17..28feeba 100644
> > --- a/arch/x86/include/asm/kexec.h
> > +++ b/arch/x86/include/asm/kexec.h
> > @@ -163,6 +163,8 @@ struct kimage_arch {
> >  };
> >  #endif
> >  
> > +extern void (*crash_vmclear_loaded_vmcss)(void);
> > +
> >  #endif /* __ASSEMBLY__ */
> >  
> >  #endif /* _ASM_X86_KEXEC_H */
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index 13ad899..4a2a12f 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>
> > @@ -29,6 +30,20 @@
> >  #include <asm/virtext.h>
> >  
> >  int in_crash_kexec;
> > + 
> > +/*
> > + * This is used to VMCLEAR all VMCSs loaded on the
> > + * processor. And when loading kvm_intel module, the
> > + * callback function pointer will be assigned.
> > + */
> > +void (*crash_vmclear_loaded_vmcss)(void) = NULL;
> > +EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss);
> > +
> > +static inline void cpu_emergency_vmclear_loaded_vmcss(void)
> > +{
> > +	if (crash_vmclear_loaded_vmcss)
> > +		crash_vmclear_loaded_vmcss();
> > +}
> >  
> >  #if defined(CONFIG_SMP) && defined(CONFIG_X86_LOCAL_APIC)
> >  
> > @@ -46,6 +61,11 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
> >  #endif
> >  	crash_save_cpu(regs, cpu);
> >  
> > +	/*
> > +	 * VMCLEAR VMCSs loaded on all cpus if needed.
> > +	 */
> > +	cpu_emergency_vmclear_loaded_vmcss();
> > +
> >  	/* Disable VMX or SVM if needed.
> >  	 *
> >  	 * We need to disable virtualization on all CPUs.
> > @@ -88,6 +108,11 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
> >  
> >  	kdump_nmi_shootdown_cpus();
> >  
> > +	/*
> > +	 * VMCLEAR VMCSs loaded on this cpu if needed.
> > +	 */
> > +	cpu_emergency_vmclear_loaded_vmcss();
> > +
> >  	/* Booting kdump kernel with VMX or SVM enabled won't work,
> >  	 * because (among other limitations) we can't disable paging
> >  	 * with the virt flags.
> > -- 
> > 1.7.1
> 
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary

[Eric W. Biederman]

Zhang Yanfei <zhangyanfei@cn.fujitsu.com> writes:

> This patch provides a way to VMCLEAR VMCSs related to guests
> on all cpus before executing the VMXOFF when doing kdump. This
> is used to ensure the VMCSs in the vmcore updated and
> non-corrupted.

Apologies for the delay I have been travelling, and I wanted
to at least read through the code.

Overall I think this is good but I have one nit, and I see one real
problem with this code.

> +/*
> + * This is used to VMCLEAR all VMCSs loaded on the
> + * processor. And when loading kvm_intel module, the
> + * callback function pointer will be assigned.
> + */
> +void (*crash_vmclear_loaded_vmcss)(void) = NULL;
> +EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss);
> +
> +static inline void cpu_emergency_vmclear_loaded_vmcss(void)
> +{
> +	if (crash_vmclear_loaded_vmcss)
> +		crash_vmclear_loaded_vmcss();
> +}

The nit is the use of emergency instead of crash in the name.

The problem is that this is potentially a NULL pointer dereference if
kvm-intel is removed.  The easist fix would be in your second patch to
just make it impossible to unload the kvm-intel module.  Otherwise
there the deference of crash_vmclear_loaded_vmcss needs to be rcu
protected, with a syncrhonize_rcu after the pointer is set to NULL in
the unload path.

Otherwise I have no objections to this code.

Eric

Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary

[Zhang Yanfei]

于 2012年12月05日 04:14, Eric W. Biederman 写道:
> Zhang Yanfei <zhangyanfei@cn.fujitsu.com> writes:
> 
>> This patch provides a way to VMCLEAR VMCSs related to guests
>> on all cpus before executing the VMXOFF when doing kdump. This
>> is used to ensure the VMCSs in the vmcore updated and
>> non-corrupted.
> 
> Apologies for the delay I have been travelling, and I wanted
> to at least read through the code.
> 
> Overall I think this is good but I have one nit, and I see one real
> problem with this code.
> 
>> +/*
>> + * This is used to VMCLEAR all VMCSs loaded on the
>> + * processor. And when loading kvm_intel module, the
>> + * callback function pointer will be assigned.
>> + */
>> +void (*crash_vmclear_loaded_vmcss)(void) = NULL;
>> +EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss);
>> +
>> +static inline void cpu_emergency_vmclear_loaded_vmcss(void)
>> +{
>> +	if (crash_vmclear_loaded_vmcss)
>> +		crash_vmclear_loaded_vmcss();
>> +}
> 
> The nit is the use of emergency instead of crash in the name.

ok, emergency -> crash

> 
> The problem is that this is potentially a NULL pointer dereference if
> kvm-intel is removed.  The easist fix would be in your second patch to
> just make it impossible to unload the kvm-intel module.  Otherwise
> there the deference of crash_vmclear_loaded_vmcss needs to be rcu
> protected, with a syncrhonize_rcu after the pointer is set to NULL in
> the unload path.

Ah, thanks for this comment.

I think I will use the rcu machanism to solve the problem.

> 
> Otherwise I have no objections to this code.

Thanks for your review. I will update the patch and resend it.

Thanks
Zhang Yanfei