[PATCH 0/2] kvm: disable virtualization on kdump

[PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

The following two patches should make kdump work when the kvm-intel module
is loaded. We need to disable vmx mode before booting the kdump kernel,
so I've introduced a notifier interface where KVM can hook and disable
virtualization on all CPUs just before they are halted.

It has the same purpose of the KVM reboot notifier that gets executed
at kexec-time. But on the kdump case, things are not as simple because
the kernel has just crashed.

The notifier interface being introduced is x86-specific. I don't know
if an arch-independent interface would be more appropriate for this
case.

It was tested only using kvm-intel. Testing on different machines
is welcome.

-- 
Eduardo

[PATCH 1/2] kdump: crash-time CPU halt notifier interface

[Eduardo Habkost]

This patch adds an interface for notification of CPUs being
halted on a crash. The notifier will be called once on each CPU by
machine_crash_shutdown().

The notifiers will be used by KVM to disable virtualization before
halting the CPUs, otherwise the booting of the kdump kernel may hang
(it does, on my machine, when kvm-intel module is loaded).

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, and the new notifier call is hooked into
the CPU halting code that is on the crash shutdown NMI handler.

I am not sure if a non x86-specific interface is more recommended. Some
arches don't have code to halt the CPUs on machine_crash_shutdown(),
so the interface wouldn't make sense on those arches.

Signed-off-by: Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kernel/crash.c  |    5 ++++
 arch/x86/kernel/reboot.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++
 include/asm-x86/reboot.h |    4 +++
 3 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 2685538..3303e3c 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>
@@ -65,6 +66,8 @@ static int crash_nmi_callback(struct notifier_block *self,
 	}
 #endif
 	crash_save_cpu(regs, cpu);
+
+	emergency_halt_notify(cpu);
 	disable_local_APIC();
 	atomic_dec(&waiting_for_crash_ipi);
 	/* Assume hlt works */
@@ -134,6 +137,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();
+	emergency_halt_notify(crashing_cpu);
+
 	lapic_shutdown();
 #if defined(CONFIG_X86_IO_APIC)
 	disable_IO_APIC();
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index f4c93f1..976d0bf 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -518,3 +518,57 @@ void machine_crash_shutdown(struct pt_regs *regs)
 	machine_ops.crash_shutdown(regs);
 }
 #endif
+
+
+static RAW_NOTIFIER_HEAD(emergency_halt_chain);
+static DEFINE_SPINLOCK(emergency_halt_chain_lock);
+
+/**
+ *	register_emergency_halt_notifier - Register crash-time CPU halt notifier
+ *	@nb: Info about notifier function to be called
+ *
+ *	Registers a function to be called when CPUs are being halted at
+ *	machine_crash_shutdown().
+ *
+ *	The function is called once on each online CPU, possibly
+ *	from a NMI handler. Do the very least to allow the CPU
+ *	to be halted, as the kernel has just crashed.
+ */
+int register_emergency_halt_notifier(struct notifier_block *nb)
+{
+	int r;
+	spin_lock(&emergency_halt_chain_lock);
+	r = raw_notifier_chain_register(&emergency_halt_chain, nb);
+	spin_unlock(&emergency_halt_chain_lock);
+	return r;
+}
+EXPORT_SYMBOL(register_emergency_halt_notifier);
+
+/**
+ *	unregister_emergency_halt_notifier - Unregister a emergency_halt notifier
+ *	@nb: Notifier info previously registered
+ *
+ *	Unregister a CPU crash-time emergency halt notifier previously
+ *	registered.
+ *
+ *	Returns zero on success, or %-ENOENT on failure.
+ */
+int unregister_emergency_halt_notifier(struct notifier_block *nb)
+{
+	return raw_notifier_chain_unregister(&emergency_halt_chain, nb);
+}
+EXPORT_SYMBOL(unregister_emergency_halt_notifier);
+
+/* Notify that the CPU will be halted on crash
+ *
+ * Runs the notifiers registered by register_emergency_halt_notifier().
+ * Must be called on the CPU that is being halted.
+ */
+void emergency_halt_notify(unsigned int cpu)
+{
+	void *hcpu = (void *)(long)cpu;
+
+	/* Don't use any locking. We are crashing.
+	 */
+	raw_notifier_call_chain(&emergency_halt_chain, 0, hcpu);
+}
diff --git a/include/asm-x86/reboot.h b/include/asm-x86/reboot.h
index 1c2f0ce..49c9d7f 100644
--- a/include/asm-x86/reboot.h
+++ b/include/asm-x86/reboot.h
@@ -18,4 +18,8 @@ void native_machine_crash_shutdown(struct pt_regs *regs);
 void native_machine_shutdown(void);
 void machine_real_restart(const unsigned char *code, int length);
 
+int register_emergency_halt_notifier(struct notifier_block *nb);
+int unregister_emergency_halt_notifier(struct notifier_block *nb);
+void emergency_halt_notify(unsigned int cpu);
+
 #endif /* ASM_X86__REBOOT_H */
-- 
1.5.5.GIT

[PATCH 2/2] kvm: disable virtualization when halting CPUs on crash

[Eduardo Habkost]

Use the emergency_halt_notifier interface to disable virtualization
on all the CPUs on machine_crash_shutdown().

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5d29c50..17e2df1 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/reboot.h>
 
 #define MAX_IO_MSRS 256
 #define CR0_RESERVED_BITS						\
@@ -2583,6 +2584,18 @@ int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_pio_string);
 
+static int kvm_emergency_halt(struct notifier_block *notifier,
+			      unsigned long val, void *v)
+{
+	/* We are crashing. Do the very least */
+	kvm_arch_hardware_disable(NULL);
+	return NOTIFY_OK;
+}
+
+static struct notifier_block kvm_emergency_halt_notifier = {
+	.notifier_call = kvm_emergency_halt,
+};
+
 int kvm_arch_init(void *opaque)
 {
 	int r;
@@ -2605,10 +2618,14 @@ int kvm_arch_init(void *opaque)
 		goto out;
 	}
 
-	r = kvm_mmu_module_init();
+	r = register_emergency_halt_notifier(&kvm_emergency_halt_notifier);
 	if (r)
 		goto out;
 
+	r = kvm_mmu_module_init();
+	if (r)
+		goto out_unregister;
+
 	kvm_init_msr_list();
 
 	kvm_x86_ops = ops;
@@ -2618,6 +2635,8 @@ int kvm_arch_init(void *opaque)
 			PT_DIRTY_MASK, PT64_NX_MASK, 0, 0);
 	return 0;
 
+out_unregister:
+	unregister_emergency_halt_notifier(&kvm_emergency_halt_notifier);
 out:
 	return r;
 }
@@ -2626,6 +2645,7 @@ void kvm_arch_exit(void)
 {
 	kvm_x86_ops = NULL;
 	kvm_mmu_module_exit();
+	unregister_emergency_halt_notifier(&kvm_emergency_halt_notifier);
 }
 
 int kvm_emulate_halt(struct kvm_vcpu *vcpu)
-- 
1.5.5.GIT

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Simon Horman]

On Mon, Oct 20, 2008 at 01:01:32PM -0200, Eduardo Habkost wrote:
> The following two patches should make kdump work when the kvm-intel module
> is loaded. We need to disable vmx mode before booting the kdump kernel,
> so I've introduced a notifier interface where KVM can hook and disable
> virtualization on all CPUs just before they are halted.
> 
> It has the same purpose of the KVM reboot notifier that gets executed
> at kexec-time. But on the kdump case, things are not as simple because
> the kernel has just crashed.
> 
> The notifier interface being introduced is x86-specific. I don't know
> if an arch-independent interface would be more appropriate for this
> case.
> 
> It was tested only using kvm-intel. Testing on different machines
> is welcome.

These changes look fine to me from a kexec/kdump point of view.

Reviewed-by: Simon Horman <horms@verge.net.au>

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

On Thu, Oct 23, 2008 at 10:28:24AM +1100, Simon Horman wrote:
> On Mon, Oct 20, 2008 at 01:01:32PM -0200, Eduardo Habkost wrote:
> > The following two patches should make kdump work when the kvm-intel module
> > is loaded. We need to disable vmx mode before booting the kdump kernel,
> > so I've introduced a notifier interface where KVM can hook and disable
> > virtualization on all CPUs just before they are halted.
> > 
> > It has the same purpose of the KVM reboot notifier that gets executed
> > at kexec-time. But on the kdump case, things are not as simple because
> > the kernel has just crashed.
> > 
> > The notifier interface being introduced is x86-specific. I don't know
> > if an arch-independent interface would be more appropriate for this
> > case.
> > 
> > It was tested only using kvm-intel. Testing on different machines
> > is welcome.
> 
> These changes look fine to me from a kexec/kdump point of view.
> 
> Reviewed-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>

Thanks.

Considering they touch both KVM and kexec, which tree would be best way
to get them in?

(Avi: the patches were sent only to kexec and kvm mailing lists,
initially. If it's better to submit them to your address also so it gets
on your queue, please let me know)

-- 
Eduardo

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Simon Horman]

[ Added Andrew Morton, Eric Biederman, Vivek Goyal and Haren Myneni to CC ]

On Thu, Oct 23, 2008 at 05:41:29PM -0200, Eduardo Habkost wrote:
> On Thu, Oct 23, 2008 at 10:28:24AM +1100, Simon Horman wrote:
> > On Mon, Oct 20, 2008 at 01:01:32PM -0200, Eduardo Habkost wrote:
> > > The following two patches should make kdump work when the kvm-intel module
> > > is loaded. We need to disable vmx mode before booting the kdump kernel,
> > > so I've introduced a notifier interface where KVM can hook and disable
> > > virtualization on all CPUs just before they are halted.
> > > 
> > > It has the same purpose of the KVM reboot notifier that gets executed
> > > at kexec-time. But on the kdump case, things are not as simple because
> > > the kernel has just crashed.
> > > 
> > > The notifier interface being introduced is x86-specific. I don't know
> > > if an arch-independent interface would be more appropriate for this
> > > case.
> > > 
> > > It was tested only using kvm-intel. Testing on different machines
> > > is welcome.
> > 
> > These changes look fine to me from a kexec/kdump point of view.
> > 
> > Reviewed-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> 
> Thanks.
> 
> Considering they touch both KVM and kexec, which tree would be best way
> to get them in?

As I understand it, there is no kexec tree as such, rather
patches either get picked up by an arch tree or Andrew Morton.
I am happy to create and maintain a kexec tree if there is a need.
But in this case it seems that using the KVM tree would be best.

> (Avi: the patches were sent only to kexec and kvm mailing lists,
> initially. If it's better to submit them to your address also so it gets
> on your queue, please let me know)

I won't speak for Avi, but usually its good to CC the maintainer.

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Simon Horman <horms@verge.net.au> writes:

> [ Added Andrew Morton, Eric Biederman, Vivek Goyal and Haren Myneni to CC ]
>
> On Thu, Oct 23, 2008 at 05:41:29PM -0200, Eduardo Habkost wrote:
>> On Thu, Oct 23, 2008 at 10:28:24AM +1100, Simon Horman wrote:
>> > On Mon, Oct 20, 2008 at 01:01:32PM -0200, Eduardo Habkost wrote:
>> > > The following two patches should make kdump work when the kvm-intel module
>> > > is loaded. We need to disable vmx mode before booting the kdump kernel,
>> > > so I've introduced a notifier interface where KVM can hook and disable
>> > > virtualization on all CPUs just before they are halted.
>> > > 
>> > > It has the same purpose of the KVM reboot notifier that gets executed
>> > > at kexec-time. But on the kdump case, things are not as simple because
>> > > the kernel has just crashed.
>> > > 
>> > > The notifier interface being introduced is x86-specific. I don't know
>> > > if an arch-independent interface would be more appropriate for this
>> > > case.

My preference would be to have a magic function call that compiles out when
kvm isn't present.  This is a code path that is hard to audit and test, and
get right.  A notifier chain seems to make a proper audit all but impossible.

Why do we need to disable vmx mode before booting a normal linux kernel?
Is it possible to disable vmx mode before we enable interrrupts in the
kdump kernel?

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eric W. Biederman wrote:
> Why do we need to disable vmx mode before booting a normal linux kernel?
>   

vmx mode blocks INIT (even on the host; not just on the guests) so 
reboots don't work.  It also assigns some memory to the cpu; if the new 
kernel isn't aware of it, the cpu and the kernel would both think it 
belongs to them.  Finally, if vmx mode is enabled, you can't start kvm 
on the new kernel.

> Is it possible to disable vmx mode before we enable interrrupts in the
> kdump kernel?
>   

You need IPIs to disable vmx on smp.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Avi Kivity <avi@redhat.com> writes:

> Eric W. Biederman wrote:
>> Why do we need to disable vmx mode before booting a normal linux kernel?
>>
>
> vmx mode blocks INIT (even on the host; not just on the guests) 
*blink* broken hardware design there.

> so reboots don't
> work.  It also assigns some memory to the cpu; if the new kernel isn't aware of
> it, 

Not a problem for a kdump kernel, as it lives out of a reserved region
of memory.

> the cpu and the kernel would both think it belongs to them.  Finally, if vmx
> mode is enabled, you can't start kvm on the new kernel.

This isn't especially interesting in the crash dump scenario.

>> Is it possible to disable vmx mode before we enable interrrupts in the
>> kdump kernel?
>>
>
> You need IPIs to disable vmx on smp.

Thank you.  Reading your description and taking a quick look at
the code in hardware disable it does not appear that there is
anything needed (other than restricting ourselves it running
uniprocessor in the kdump kernel) that needs to happen.

Certainly it would be nice to have kvm disabled in hardware,
but if you are proposing using the existing hardware disable
I must say that the cure looks much worse than the disease.

It looks like the disable function is all of about 20 assembly
instructions so I would not have a problem if he had a
little inline function we could call that test to see if
vmx is enabled and disable it in the case of kexec on panic.

The normal polite shutdown.  That just looks like asking for trouble.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eric W. Biederman wrote:
>> so reboots don't
>> work.  It also assigns some memory to the cpu; if the new kernel isn't aware of
>> it, 
>>     
>
> Not a problem for a kdump kernel, as it lives out of a reserved region
> of memory.
>   

But it is a problem for general kexec.

>>> Is it possible to disable vmx mode before we enable interrrupts in the
>>> kdump kernel?
>>>
>>>       
>> You need IPIs to disable vmx on smp.
>>     
>
> Thank you.  Reading your description and taking a quick look at
> the code in hardware disable it does not appear that there is
> anything needed (other than restricting ourselves it running
> uniprocessor in the kdump kernel) that needs to happen.
>
> Certainly it would be nice to have kvm disabled in hardware,
> but if you are proposing using the existing hardware disable
> I must say that the cure looks much worse than the disease.
>   

Certainly you don't want to issue IPIs when kdump()ing.  It's not 
unlikely that the other cpus have interrupts permanently disabled.

(we can use NMI IPIs, but that will likely be messy)

> It looks like the disable function is all of about 20 assembly
> instructions so I would not have a problem if he had a
> little inline function we could call that test to see if
> vmx is enabled and disable it in the case of kexec on panic.
>
> The normal polite shutdown.  That just looks like asking for trouble.
>   

But what happens when the kdump kernel reboots?  If it is uniprocessor, 
it will never have a chance to disable vmx on other cpus.  Using acpi 
reset (now default) works around this on some machines, but not all.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

On Sun, Oct 26, 2008 at 05:07:45PM +0200, Avi Kivity wrote:
> Eric W. Biederman wrote:
<snip>
>>>> Is it possible to disable vmx mode before we enable interrrupts in the
>>>> kdump kernel?
>>>>
>>>>       
>>> You need IPIs to disable vmx on smp.
>>>     
>>
>> Thank you.  Reading your description and taking a quick look at
>> the code in hardware disable it does not appear that there is
>> anything needed (other than restricting ourselves it running
>> uniprocessor in the kdump kernel) that needs to happen.
>>
>> Certainly it would be nice to have kvm disabled in hardware,
>> but if you are proposing using the existing hardware disable
>> I must say that the cure looks much worse than the disease.
>>   
>
> Certainly you don't want to issue IPIs when kdump()ing.  It's not  
> unlikely that the other cpus have interrupts permanently disabled.
>
> (we can use NMI IPIs, but that will likely be messy)

NMI IPIs are already used on x86 native_machine_crash_shutdown(), so
it wouldn't get more messy that it is currently. We just need to add
another bit of code to the code that already runs on an NMI handler.

My question is: is a notifier chain too much complexity for a sensible
piece of code like that? If so, a compile-time hook on that point
would be safer, but then it wouldn't work when KVM is compiled as a
out-of-tree module.


>
>> It looks like the disable function is all of about 20 assembly
>> instructions so I would not have a problem if he had a
>> little inline function we could call that test to see if
>> vmx is enabled and disable it in the case of kexec on panic.
>>
>> The normal polite shutdown.  That just looks like asking for trouble.
>>   
>
> But what happens when the kdump kernel reboots?  If it is uniprocessor,  
> it will never have a chance to disable vmx on other cpus.  Using acpi  
> reset (now default) works around this on some machines, but not all.

Good point. My problem was a hang when booting the kdump kernel, but it
may also cause problems later, when the kdump kernel reboots.

-- 
Eduardo

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Sun, Oct 26, 2008 at 05:07:45PM +0200, Avi Kivity wrote:
>> Eric W. Biederman wrote:
> <snip>
>>>>> Is it possible to disable vmx mode before we enable interrrupts in the
>>>>> kdump kernel?
>>>>>
>>>>>       
>>>> You need IPIs to disable vmx on smp.
>>>>     
>>>
>>> Thank you.  Reading your description and taking a quick look at
>>> the code in hardware disable it does not appear that there is
>>> anything needed (other than restricting ourselves it running
>>> uniprocessor in the kdump kernel) that needs to happen.
>>>
>>> Certainly it would be nice to have kvm disabled in hardware,
>>> but if you are proposing using the existing hardware disable
>>> I must say that the cure looks much worse than the disease.
>>>   
>>
>> Certainly you don't want to issue IPIs when kdump()ing.  It's not  
>> unlikely that the other cpus have interrupts permanently disabled.
>>
>> (we can use NMI IPIs, but that will likely be messy)
>
> NMI IPIs are already used on x86 native_machine_crash_shutdown(), so
> it wouldn't get more messy that it is currently. We just need to add
> another bit of code to the code that already runs on an NMI handler.

Yes.  And handling of those NMIs is best effort.  Nothing fails if
they don't actually run.

> My question is: is a notifier chain too much complexity for a sensible
> piece of code like that? If so, a compile-time hook on that point
> would be safer, but then it wouldn't work when KVM is compiled as a
> out-of-tree module.

Well we could fairly easily have a non-modular function that does.
if (vmx_present && vmx_enabled) {
   turn_off_vmx();
}

Which at first skim looks like it is all of about 10-20 machine
instructions.

There are a few real places where we need code on the kdump
path because there it is not possible to do the work any
other way.  However we need to think long and hard about
that because placing the code anywhere besides in a broken
and failing kernel is going to be easier to maintain and
more reliable.

I oppose an atomic notifier because it makes the review
essentially impossible.  If any module can come in and register
a notifier we can't know what code is running on that code
path and we can't be certain the code is safe in an abnormal
case to run on that code path.

Right now we only need to support vmx on the kdump path because
of what appears to be a hardware design bug.  Enabling vmx
apparently disables standard functions like an INIT IPI.  Things
like this do happen but they should be rare.

> Good point. My problem was a hang when booting the kdump kernel, but it
> may also cause problems later, when the kdump kernel reboots.

What was the cause of the hang when booting the kdump kernel?

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eric W. Biederman wrote:
>> NMI IPIs are already used on x86 native_machine_crash_shutdown(), so
>> it wouldn't get more messy that it is currently. We just need to add
>> another bit of code to the code that already runs on an NMI handler.
>>     
>
> Yes.  And handling of those NMIs is best effort.  Nothing fails if
> they don't actually run.
>
>   

Unless someone can come up with another way to disable vmx remotely, 
that's going to change if you have vmx enabled.

> Well we could fairly easily have a non-modular function that does.
> if (vmx_present && vmx_enabled) {
>    turn_off_vmx();
> }
>
> Which at first skim looks like it is all of about 10-20 machine
> instructions.
>
>   

There's no way to query whether vmx is enabled or disabled, AFAICT.  So 
we have to execute vmxoff and ignore possible #UDs.

If we trust the exception handlers, there's no problem.  Otherwise we 
need to replace the current #UD handler with an iret (perhaps switching 
temporarily to another IDT).

> There are a few real places where we need code on the kdump
> path because there it is not possible to do the work any
> other way.  However we need to think long and hard about
> that because placing the code anywhere besides in a broken
> and failing kernel is going to be easier to maintain and
> more reliable.
>   

vmx blocking INITs makes it impossible to leave this to the new kernel.

> I oppose an atomic notifier because it makes the review
> essentially impossible.  If any module can come in and register
> a notifier we can't know what code is running on that code
> path and we can't be certain the code is safe in an abnormal
> case to run on that code path.
>   

What if it's a specialized notifier for kexec?  Or even kexec_crash?

That said, I have no issue with static code at the call site.

> Right now we only need to support vmx on the kdump path because
> of what appears to be a hardware design bug.  Enabling vmx
> apparently disables standard functions like an INIT IPI.  Things
> like this do happen but they should be rare.
>   

The general kexec path also wants this fixed.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

On Mon, Oct 27, 2008 at 11:13:41AM +0200, Avi Kivity wrote:
> Eric W. Biederman wrote:
>>> NMI IPIs are already used on x86 native_machine_crash_shutdown(), so
>>> it wouldn't get more messy that it is currently. We just need to add
>>> another bit of code to the code that already runs on an NMI handler.
>>>     
>>
>> Yes.  And handling of those NMIs is best effort.  Nothing fails if
>> they don't actually run.
>>
>>   
>
> Unless someone can come up with another way to disable vmx remotely,  
> that's going to change if you have vmx enabled.
>
>> Well we could fairly easily have a non-modular function that does.
>> if (vmx_present && vmx_enabled) {
>>    turn_off_vmx();
>> }
>>
>> Which at first skim looks like it is all of about 10-20 machine
>> instructions.
>>
>>   
>
> There's no way to query whether vmx is enabled or disabled, AFAICT.  So  
> we have to execute vmxoff and ignore possible #UDs.

Oops. This means the notifier my patches add would break, if vmx is
disabled on any CPU.

Can't we just set a flag when we are about to enable vmx, so we run vmxoff
only when know it's enabled? There will be a tiny window between setting
this flag and and actually running vmxon where things could go wrong,
but this doesn't look that bad.

Having to handle #UD would make things more messy, in my opinion.


BTW, is this problem vmx-specific? Do we need to do something similar
for svm?


>
> If we trust the exception handlers, there's no problem.  Otherwise we  
> need to replace the current #UD handler with an iret (perhaps switching  
> temporarily to another IDT).

I think we can't fully trust anything if we are on the crash dump path,
so the less code we depend on, the better.

>
>> There are a few real places where we need code on the kdump
>> path because there it is not possible to do the work any
>> other way.  However we need to think long and hard about
>> that because placing the code anywhere besides in a broken
>> and failing kernel is going to be easier to maintain and
>> more reliable.
>>   
>
> vmx blocking INITs makes it impossible to leave this to the new kernel.
>
>> I oppose an atomic notifier because it makes the review
>> essentially impossible.  If any module can come in and register
>> a notifier we can't know what code is running on that code
>> path and we can't be certain the code is safe in an abnormal
>> case to run on that code path.
>>   
>
> What if it's a specialized notifier for kexec?  Or even kexec_crash?

The patches I've sent to the kvm mailing list added a notifier interface
specific for kexec_crash, using raw_notifier_*().

IMO, if a notifier registration interface was acceptable, the raw
notifiers would be good enough for that. But Eric seems to think that
adding a notifier registration interface for the crash handler path
wouldn't be a good idea, and I am starting to agree with him.


>
> That said, I have no issue with static code at the call site.
>
>> Right now we only need to support vmx on the kdump path because
>> of what appears to be a hardware design bug.  Enabling vmx
>> apparently disables standard functions like an INIT IPI.  Things
>> like this do happen but they should be rare.
>>   
>
> The general kexec path also wants this fixed.

When I've tested it, kexec called the kvm reboot notifier, so
everything worked fine.

-- 
Eduardo

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eduardo Habkost wrote:
> Can't we just set a flag when we are about to enable vmx, so we run vmxoff
> only when know it's enabled? There will be a tiny window between setting
> this flag and and actually running vmxon where things could go wrong,
> but this doesn't look that bad.
>   

It makes more sense to have a vmxon api in the core; you call it, the 
kernel enables it and sets a flag; then either you or the core can 
disable it.

> Having to handle #UD would make things more messy, in my opinion.
>   

It's not too bad, either relying on exception handlers or hacking our own.

> BTW, is this problem vmx-specific? Do we need to do something similar
> for svm?
>
>   

svm needs it as well, since it shares some memory with the cpu.  It's 
less critical though, will likely work even without it.

>> If we trust the exception handlers, there's no problem.  Otherwise we  
>> need to replace the current #UD handler with an iret (perhaps switching  
>> temporarily to another IDT).
>>     
>
> I think we can't fully trust anything if we are on the crash dump path,
> so the less code we depend on, the better.
>   

So we can point #UD temporarily at an 'addq $3, (%rsp); iret' for the 
vmxoff instruction.  Or implement the 'enable virt extensions' API.

> The patches I've sent to the kvm mailing list added a notifier interface
> specific for kexec_crash, using raw_notifier_*().
>
> IMO, if a notifier registration interface was acceptable, the raw
> notifiers would be good enough for that. But Eric seems to think that
> adding a notifier registration interface for the crash handler path
> wouldn't be a good idea, and I am starting to agree with him.
>
>   

I wouldn't mind notifiers (with a nice comment explaining that you must 
know what you're doing, though that's the case with most kernel APIs).  
I'm fine with either approach.

>> The general kexec path also wants this fixed.
>>     
>
> When I've tested it, kexec called the kvm reboot notifier, so
> everything worked fine.
>   

Oh, okay.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Avi Kivity <avi@redhat.com> writes:

> Eduardo Habkost wrote:
>> Can't we just set a flag when we are about to enable vmx, so we run vmxoff
>> only when know it's enabled? There will be a tiny window between setting
>> this flag and and actually running vmxon where things could go wrong,
>> but this doesn't look that bad.
>>
>
> It makes more sense to have a vmxon api in the core; you call it, the kernel
> enables it and sets a flag; then either you or the core can disable it.

Yes.  That sounds like the most maintainable approach.

>> The patches I've sent to the kvm mailing list added a notifier interface
>> specific for kexec_crash, using raw_notifier_*().
>>
>> IMO, if a notifier registration interface was acceptable, the raw
>> notifiers would be good enough for that. But Eric seems to think that
>> adding a notifier registration interface for the crash handler path
>> wouldn't be a good idea, and I am starting to agree with him.
>>
>>
>
> I wouldn't mind notifiers (with a nice comment explaining that you must know
> what you're doing, though that's the case with most kernel APIs).  I'm fine with
> either approach.

This is the 3rd request I have seen for a notifier.  This is the first
request I have seen for code that must be executed in the kexec on
panic path.  So history suggest to me that notifiers make it
unreasonably easy to get code onto the kexec on panic code path.

Occasionally the kexec on panic code path is tested to see how
well it works in strange situations like being called from
a stack overflow etc.

The rest of the history is that previous attempts like lkcd
had very programmer friendly interfaces, that worked fine
in test environments giving beautiful core dumps, but when things
broke in the field they were essentially useless.  The kdump
approach is still not completely reliable but it does work
well enough that people get useful crash dumps sometimes.

I feel anything that makes the kexec on panic code path harder
to verify it will work when things are crazy broken, like
a notifier is something we should avoid.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

On Mon, Oct 27, 2008 at 10:32:43AM -0700, Eric W. Biederman wrote:
> Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
<snip>
> >
> > I wouldn't mind notifiers (with a nice comment explaining that you must know
> > what you're doing, though that's the case with most kernel APIs).  I'm fine with
> > either approach.
> 
> This is the 3rd request I have seen for a notifier.  This is the first
> request I have seen for code that must be executed in the kexec on
> panic path.  So history suggest to me that notifiers make it
> unreasonably easy to get code onto the kexec on panic code path.
> 
> Occasionally the kexec on panic code path is tested to see how
> well it works in strange situations like being called from
> a stack overflow etc.
> 
> The rest of the history is that previous attempts like lkcd
> had very programmer friendly interfaces, that worked fine
> in test environments giving beautiful core dumps, but when things
> broke in the field they were essentially useless.  The kdump
> approach is still not completely reliable but it does work
> well enough that people get useful crash dumps sometimes.
> 
> I feel anything that makes the kexec on panic code path harder
> to verify it will work when things are crazy broken, like
> a notifier is something we should avoid.

I am still wondering if a simple function pointer (instead of a full
notifier interface) would be good enough. It looks like a reasonable
tradeoff.

I think I will get flamed if I try to pull to the core a bunch of code
that always lived in the KVM module.  8)

And even if we pull those functions to the core, we will still have
a function pointer on the new code anyway, because we would need to
support vmx and svm.

-- 
Eduardo

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Eduardo Habkost <ehabkost@redhat.com> writes:

> I am still wondering if a simple function pointer (instead of a full
> notifier interface) would be good enough. It looks like a reasonable
> tradeoff.

Oh sorry.   As long as we do the whole rcu protected thing so it is safe
to call the function without taking locks it should work.  I'm not
thrilled about a function pointer but it should work.

> I think I will get flamed if I try to pull to the core a bunch of code
> that always lived in the KVM module.  8)

Why is KVM modular anyway?  That seems like some pretty core cpu functionality...

> And even if we pull those functions to the core, we will still have
> a function pointer on the new code anyway, because we would need to
> support vmx and svm.

Depending.  It doesn't sound like svm has the problem where init doesn't
work so svm really doesn't need to do this.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eric W. Biederman wrote:
>> I think I will get flamed if I try to pull to the core a bunch of code
>> that always lived in the KVM module.  8)
>>     
>
> Why is KVM modular anyway?  That seems like some pretty core cpu functionality...
>   

Many reasons.  Developers like the ability to rmmod and modprobe during 
development.  Distros like to keep their non-modular core small.  There 
is an external module distribution that allows users to graft a new kvm 
on an old kernel, which our testers and bleeding edge users like.  
Because it's there.

There's always CONFIG_KVM=y if you don't want it.

> Depending.  It doesn't sound like svm has the problem where init doesn't
> work so svm really doesn't need to do this.
>   

svm can writeback into memory at odd times if we don't do this, and the 
cost is small - clear a bit in EFER.  There's no reason to be lazy.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:

> Eric W. Biederman wrote:
>>> I think I will get flamed if I try to pull to the core a bunch of code
>>> that always lived in the KVM module.  8)
>>>
>>
>> Why is KVM modular anyway?  That seems like some pretty core cpu
> functionality...
>>
>
> Many reasons.  Developers like the ability to rmmod and modprobe during
> development.  Distros like to keep their non-modular core small.  There is an
> external module distribution that allows users to graft a new kvm on an old
> kernel, which our testers and bleeding edge users like.  Because it's there.

Most of the reason I was wondering is that the cpu hardware probing
largely seems to be a duplicate of what we have in the core for
probing cpu capabilities already, and could likely be made smaller
by building upon the existing codebase.


> svm can writeback into memory at odd times if we don't do this, and the cost is
> small - clear a bit in EFER.  There's no reason to be lazy.

Especially if we can clear that bit unconditionally (when
EFER is present) I'm all for it.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eric W. Biederman wrote:
> Most of the reason I was wondering is that the cpu hardware probing
> largely seems to be a duplicate of what we have in the core for
> probing cpu capabilities already, and could likely be made smaller
> by building upon the existing codebase.
>
>   

We use the core cpuid functions, or are you referring to something else?

>> svm can writeback into memory at odd times if we don't do this, and the cost is
>> small - clear a bit in EFER.  There's no reason to be lazy.
>>     
>
> Especially if we can clear that bit unconditionally (when
> EFER is present) I'm all for it.
>   

That is the case.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Avi Kivity <avi@redhat.com> writes:

> Eric W. Biederman wrote:
>> Most of the reason I was wondering is that the cpu hardware probing
>> largely seems to be a duplicate of what we have in the core for
>> probing cpu capabilities already, and could likely be made smaller
>> by building upon the existing codebase.
>>
>>
>
> We use the core cpuid functions, or are you referring to something else?

I was referring to the arch/x86/kernel/cpu/*
and arch/x86/include/asm/cpufeature.h

The core functions that are responsible for detecting all of the cpu features,
and disabling them if there are cpu errata.

The usual pattern is that code does all of the magic detection logic and
then the code that would use it would just need to test: cpu_has_vmx or cpu_has_svm.

At least in part that allows us to show the working cpu features in /proc/cpuinfo.

>>> svm can writeback into memory at odd times if we don't do this, and the cost
> is
>>> small - clear a bit in EFER.  There's no reason to be lazy.
>>>
>>
>> Especially if we can clear that bit unconditionally (when
>> EFER is present) I'm all for it.
>>
>
> That is the case.

Cool.  I forget if we have to test for EFER on 32bit, or if we can just wrmsr
and be done with it.  Regardless that sounds easy to do on the kexec path.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Chris Lalancette]

Eric W. Biederman wrote:
>>>> svm can writeback into memory at odd times if we don't do this, and the cost
>> is
>>>> small - clear a bit in EFER.  There's no reason to be lazy.
>>>>
>>> Especially if we can clear that bit unconditionally (when
>>> EFER is present) I'm all for it.
>>>
>> That is the case.
> 
> Cool.  I forget if we have to test for EFER on 32bit, or if we can just wrmsr
> and be done with it.  Regardless that sounds easy to do on the kexec path.

I'm pretty sure you have to test for it first; pre-64 bit x86 hardware doesn't
have the EFER register, so you'll fault on access.  On the other hand pre-64 bit
x86 hardware doesn't usually (ever?) have VT extensions either.

-- 
Chris Lalancette

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Chris Lalancette wrote:
> I'm pretty sure you have to test for it first; pre-64 bit x86 hardware doesn't
> have the EFER register, so you'll fault on access.  On the other hand pre-64 bit
> x86 hardware doesn't usually (ever?) have VT extensions either.
>   

All amd hardware that supports svm has an efer.  Some Intel hardware 
doesn't, but we don't care in that case.

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

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eric W. Biederman wrote:
> Avi Kivity <avi@redhat.com> writes:
>
>   
>> Eric W. Biederman wrote:
>>     
>>> Most of the reason I was wondering is that the cpu hardware probing
>>> largely seems to be a duplicate of what we have in the core for
>>> probing cpu capabilities already, and could likely be made smaller
>>> by building upon the existing codebase.
>>>
>>>
>>>       
>> We use the core cpuid functions, or are you referring to something else?
>>     
>
> I was referring to the arch/x86/kernel/cpu/*
> and arch/x86/include/asm/cpufeature.h
>
> The core functions that are responsible for detecting all of the cpu features,
> and disabling them if there are cpu errata.
>
> The usual pattern is that code does all of the magic detection logic and
> then the code that would use it would just need to test: cpu_has_vmx or cpu_has_svm.
>
>   

vmx is much more complicated than that, with some features define in 
read-only msrs.

> At least in part that allows us to show the working cpu features in /proc/cpuinfo.
>
>   

Yes that's a problem now; you can only tell if you have vmx or not, 
without any information as to the various vmx subfeatures.

> Cool.  I forget if we have to test for EFER on 32bit, or if we can just wrmsr
> and be done with it.  Regardless that sounds easy to do on the kexec path.
>   

if (cpu_has_svm())
    disable_svme();

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

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eduardo Habkost wrote:
> I think I will get flamed if I try to pull to the core a bunch of code
> that always lived in the KVM module.  8)
>   

Having a simple api to enter vmx or svm mode in core code should not be 
too bad.  The kvm hardware_enable() stuff could simply call that.  The 
only downsides I see are a very minor bloat to the core, and spreading 
kvm code around.

> And even if we pull those functions to the core, we will still have
> a function pointer on the new code anyway, because we would need to
> support vmx and svm.
>   

Or have an if () statement there.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:

> There's no way to query whether vmx is enabled or disabled, AFAICT.  So we have
> to execute vmxoff and ignore possible #UDs.
>
> If we trust the exception handlers, there's no problem.  Otherwise we need to
> replace the current #UD handler with an iret (perhaps switching temporarily to
> another IDT).

Ugh.  We already change the IDT on that code path so that
may be a way to go.

> The general kexec path also wants this fixed.

It looks like someone hooked the reboot notifier which should
be called on the normal kexec path.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

On Mon, Oct 27, 2008 at 08:05:50AM -0700, Eric W. Biederman wrote:
> Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> 
> > There's no way to query whether vmx is enabled or disabled, AFAICT.  So we have
> > to execute vmxoff and ignore possible #UDs.
> >
> > If we trust the exception handlers, there's no problem.  Otherwise we need to
> > replace the current #UD handler with an iret (perhaps switching temporarily to
> > another IDT).
> 
> Ugh.  We already change the IDT on that code path so that
> may be a way to go.

I don't see the IDT being changed on native_machine_shutdown(). It even
uses notifier chain registration (register_die_notifier()) to sneak a
NMI handler in.

-- 
Eduardo

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eduardo Habkost wrote:
>> (we can use NMI IPIs, but that will likely be messy)
>>     
>
> NMI IPIs are already used on x86 native_machine_crash_shutdown(), so
> it wouldn't get more messy that it is currently. We just need to add
> another bit of code to the code that already runs on an NMI handler.
>
>   

That looks like the easiest (and best) way out.

> My question is: is a notifier chain too much complexity for a sensible
> piece of code like that? If so, a compile-time hook on that point
> would be safer,

I think an unconditional vmx disable is wanted here, so kexec can work 
with other hypervisors.

>  but then it wouldn't work when KVM is compiled as a
> out-of-tree module.
>   

The external module can do without.  It's possible to hijack the nmi 
vector, but I don't think that's a good idea.  If someone wants 
kexec+vmx on an older kernel, they can patch that kernel.

>> But what happens when the kdump kernel reboots?  If it is uniprocessor,  
>> it will never have a chance to disable vmx on other cpus.  Using acpi  
>> reset (now default) works around this on some machines, but not all.
>>     
> Good point. My problem was a hang when booting the kdump kernel, but it
> may also cause problems later, when the kdump kernel reboots.
>   

The hang was likely caused by vmx blocking INIT.  Sigh.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Vivek Goyal]

On Mon, Oct 27, 2008 at 10:54:01AM +0200, Avi Kivity wrote:
> Eduardo Habkost wrote:
>>> (we can use NMI IPIs, but that will likely be messy)
>>>     
>>
>> NMI IPIs are already used on x86 native_machine_crash_shutdown(), so
>> it wouldn't get more messy that it is currently. We just need to add
>> another bit of code to the code that already runs on an NMI handler.
>>
>>   
>
> That looks like the easiest (and best) way out.
>
>> My question is: is a notifier chain too much complexity for a sensible
>> piece of code like that? If so, a compile-time hook on that point
>> would be safer,
>
> I think an unconditional vmx disable is wanted here, so kexec can work  
> with other hypervisors.
>
>>  but then it wouldn't work when KVM is compiled as a
>> out-of-tree module.
>>   
>
> The external module can do without.  It's possible to hijack the nmi  
> vector, but I don't think that's a good idea.  If someone wants  
> kexec+vmx on an older kernel, they can patch that kernel.
>
>>> But what happens when the kdump kernel reboots?  If it is 
>>> uniprocessor,  it will never have a chance to disable vmx on other 
>>> cpus.  Using acpi  reset (now default) works around this on some 
>>> machines, but not all.
>>>     
>> Good point. My problem was a hang when booting the kdump kernel, but it
>> may also cause problems later, when the kdump kernel reboots.
>>   
>
> The hang was likely caused by vmx blocking INIT.  Sigh.

Avi,

We boot kdump kernel with maxcpus=1. IIUC, in that code path we will not
be using INIT. So did you try booting kdump kernel with maxcpus=1 and did
it work for you? If not than problem could be something else.

Thanks
Vivek

>
> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Vivek Goyal wrote:

  

>> The hang was likely caused by vmx blocking INIT.  Sigh.
>>     
>
> Avi,
>
> We boot kdump kernel with maxcpus=1. IIUC, in that code path we will not
> be using INIT. So did you try booting kdump kernel with maxcpus=1 and did
> it work for you? If not than problem could be something else.
>   

It wasn't me who tried it, so I can't tell.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

On Mon, Oct 27, 2008 at 09:09:37AM -0400, Vivek Goyal wrote:
> On Mon, Oct 27, 2008 at 10:54:01AM +0200, Avi Kivity wrote:
<snip>
> >
> > The hang was likely caused by vmx blocking INIT.  Sigh.
> 
> Avi,
> 
> We boot kdump kernel with maxcpus=1. IIUC, in that code path we will not
> be using INIT. So did you try booting kdump kernel with maxcpus=1 and did
> it work for you? If not than problem could be something else.

Just checked it here: I am passing maxcpus=1 to the kdump kernel. My
full kexec commandline is this:

/sbin/kexec --console-serial -p '--command-line=ro root=/dev/vg0/rawhide rhgb  console=ttyS0,115200 ignore_loglevel irqpoll maxcpus=1 reset_devices earlyprintk=ttyS0,115200' --initrd=/boot/initrd-2.6.28-rc2kdump.img /boot/vmlinuz-2.6.28-rc2

Additional info: I don't get the "I'm in purgatory" message on serial
console, when the system hangs. I do get the message if I trigger the
crash dump after unloading the kvm-intel module, so the serial console
is working.

-- 
Eduardo

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eduardo Habkost wrote:
>> We boot kdump kernel with maxcpus=1. IIUC, in that code path we will not
>> be using INIT. So did you try booting kdump kernel with maxcpus=1 and did
>> it work for you? If not than problem could be something else.
>>     

vmx also disallows changing some bits in cr0 and cr4; for example you 
can't switch off paging.  This may interfere with the boot sequence.

The full set of pleasant surprises is detailed in section 19.8 of the 
SDM volume 3, "RESTRICTIONS ON VMX OPERATION".

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

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Vivek Goyal]

On Wed, Oct 29, 2008 at 06:10:06PM -0200, Eduardo Habkost wrote:
> On Mon, Oct 27, 2008 at 09:09:37AM -0400, Vivek Goyal wrote:
> > On Mon, Oct 27, 2008 at 10:54:01AM +0200, Avi Kivity wrote:
> <snip>
> > >
> > > The hang was likely caused by vmx blocking INIT.  Sigh.
> > 
> > Avi,
> > 
> > We boot kdump kernel with maxcpus=1. IIUC, in that code path we will not
> > be using INIT. So did you try booting kdump kernel with maxcpus=1 and did
> > it work for you? If not than problem could be something else.
> 
> Just checked it here: I am passing maxcpus=1 to the kdump kernel. My
> full kexec commandline is this:
> 
> /sbin/kexec --console-serial -p '--command-line=ro root=/dev/vg0/rawhide rhgb  console=ttyS0,115200 ignore_loglevel irqpoll maxcpus=1 reset_devices earlyprintk=ttyS0,115200' --initrd=/boot/initrd-2.6.28-rc2kdump.img /boot/vmlinuz-2.6.28-rc2
> 
> Additional info: I don't get the "I'm in purgatory" message on serial
> console, when the system hangs. I do get the message if I trigger the
> crash dump after unloading the kvm-intel module, so the serial console
> is working.
> 

That means we are not even beginning to boot second kernel and are getting
lost somewhere in the first kernel itself (most likely relocate_kernel_32/64.S).

And it could be because of any of the interesting restrictions put when
vmx is enabled (As Avi mentioned in another mail.).

To debug that code, I used to put some "outb" messages to output a
character to serial console and find out which instructions is creating
trouble. You can debug to find out exactly what operation is not allowed
when vmx is enabled or simply write the patches to disable vmx and it
should automatically lead to problem resolution.

Thanks
Vivek

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Vivek Goyal <vgoyal@redhat.com> writes:

> On Wed, Oct 29, 2008 at 06:10:06PM -0200, Eduardo Habkost wrote:
>> On Mon, Oct 27, 2008 at 09:09:37AM -0400, Vivek Goyal wrote:
>> > On Mon, Oct 27, 2008 at 10:54:01AM +0200, Avi Kivity wrote:
>> <snip>
>> > >
>> > > The hang was likely caused by vmx blocking INIT.  Sigh.
>> > 
>> > Avi,
>> > 
>> > We boot kdump kernel with maxcpus=1. IIUC, in that code path we will not
>> > be using INIT. So did you try booting kdump kernel with maxcpus=1 and did
>> > it work for you? If not than problem could be something else.
>> 
>> Just checked it here: I am passing maxcpus=1 to the kdump kernel. My
>> full kexec commandline is this:
>> 
>> /sbin/kexec --console-serial -p '--command-line=ro root=/dev/vg0/rawhide rhgb
> console=ttyS0,115200 ignore_loglevel irqpoll maxcpus=1 reset_devices
> earlyprintk=ttyS0,115200' --initrd=/boot/initrd-2.6.28-rc2kdump.img
> /boot/vmlinuz-2.6.28-rc2
>> 
>> Additional info: I don't get the "I'm in purgatory" message on serial
>> console, when the system hangs. I do get the message if I trigger the
>> crash dump after unloading the kvm-intel module, so the serial console
>> is working.
>> 
>
> That means we are not even beginning to boot second kernel and are getting
> lost somewhere in the first kernel itself (most likely relocate_kernel_32/64.S).
>
> And it could be because of any of the interesting restrictions put when
> vmx is enabled (As Avi mentioned in another mail.).
>
> To debug that code, I used to put some "outb" messages to output a
> character to serial console and find out which instructions is creating
> trouble. You can debug to find out exactly what operation is not allowed
> when vmx is enabled or simply write the patches to disable vmx and it
> should automatically lead to problem resolution.

The transfer between kernels happens with virtual addresses identity mapped
to physical addresses.  For 32bit mode we implement this by disabling paging.
Which apparently is one of the interesting restrictions placed of vmx.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Avi Kivity <avi@redhat.com> writes:

> Eric W. Biederman wrote:
>>> so reboots don't
>>> work.  It also assigns some memory to the cpu; if the new kernel isn't aware
> of
>>> it,
>>
>> Not a problem for a kdump kernel, as it lives out of a reserved region
>> of memory.
>>
>
> But it is a problem for general kexec.

Agreed.  We certainly want to cleanly disable vmx on a normal
kexec reboot.

>>>> Is it possible to disable vmx mode before we enable interrrupts in the
>>>> kdump kernel?
>>>>
>>>>
>>> You need IPIs to disable vmx on smp.
>>>
>>
>> Thank you.  Reading your description and taking a quick look at
>> the code in hardware disable it does not appear that there is
>> anything needed (other than restricting ourselves it running
>> uniprocessor in the kdump kernel) that needs to happen.
>>
>> Certainly it would be nice to have kvm disabled in hardware,
>> but if you are proposing using the existing hardware disable
>> I must say that the cure looks much worse than the disease.
>>
>
> Certainly you don't want to issue IPIs when kdump()ing.  It's not unlikely that
> the other cpus have interrupts permanently disabled.
>
> (we can use NMI IPIs, but that will likely be messy)
>
>> It looks like the disable function is all of about 20 assembly
>> instructions so I would not have a problem if he had a
>> little inline function we could call that test to see if
>> vmx is enabled and disable it in the case of kexec on panic.
>>
>> The normal polite shutdown.  That just looks like asking for trouble.
>>
>
> But what happens when the kdump kernel reboots?  If it is uniprocessor, it will
> never have a chance to disable vmx on other cpus.  Using acpi reset (now
> default) works around this on some machines, but not all.

Mostly likely any reboot path will trigger software to toggle the
reset line on the board.  At least that has been my experience, and
the reason we don't see many problems when we fail to properly
shutdown devices before reboot.  If vmx persists across that it would
seem to be very broken by design.

In any case if reboot fails after a kdump that is a minor thing.  Someone
can always push the power button or the equivalent.

My objections are:  on_each_cpu called from after a panic looks like
a good way to loose control of the box and never return.  Adding
a notifier looks like a good way too add functionality onto the
kdump path that never gets reviewed for robustness after a kernel
crash and thus a good way to remove the usefulness of the kexec
on panic kernel path.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eric W. Biederman wrote:
>> But what happens when the kdump kernel reboots?  If it is uniprocessor, it will
>> never have a chance to disable vmx on other cpus.  Using acpi reset (now
>> default) works around this on some machines, but not all.
>>     
>
> Mostly likely any reboot path will trigger software to toggle the
> reset line on the board.  At least that has been my experience, and
> the reason we don't see many problems when we fail to properly
> shutdown devices before reboot.  If vmx persists across that it would
> seem to be very broken by design.
>   

We've observed on some machines that keyboard controller reset didn't 
work, and that the fallback to triple-fault asserted INIT and not RESET 
(and that this INIT was blocked, hanging the machine).  Switching to 
acpi reset worked on some machines, but not all.  Either acpi reset was 
not implemented on the failing machines, or it was wired to INIT and not 
RESET.

> My objections are:  on_each_cpu called from after a panic looks like
> a good way to loose control of the box and never return.  Adding
> a notifier looks like a good way too add functionality onto the
> kdump path that never gets reviewed for robustness after a kernel
> crash and thus a good way to remove the usefulness of the kexec
> on panic kernel path.
>   

Since we already have NMI IPIs, we could disable vmx there.  If it is 
done unconditionally and without notifiers, there is no chance of having 
unreviewed surprises sneaking in.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eric W. Biederman]

Avi Kivity <avi@redhat.com> writes:

> Eric W. Biederman wrote:
>
> We've observed on some machines that keyboard controller reset didn't work, and
> that the fallback to triple-fault asserted INIT and not RESET (and that this
> INIT was blocked, hanging the machine).  Switching to acpi reset worked on some
> machines, but not all.  Either acpi reset was not implemented on the failing
> machines, or it was wired to INIT and not RESET.

Right.  INIT does not reset things like the MTRRs, I had forgotten
that distinction.

Frequently the firmware when it regains control at 0xffffffff0
asserts reset, but if we can't get there.  Ouch!


> Since we already have NMI IPIs, we could disable vmx there.  If it is done
> unconditionally and without notifiers, there is no chance of having unreviewed
> surprises sneaking in.

Sounds good to me.

Eric

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost]

On Mon, Oct 27, 2008 at 08:02:31AM -0700, Eric W. Biederman wrote:
> Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Eric W. Biederman wrote:
> >
> > We've observed on some machines that keyboard controller reset didn't work, and
> > that the fallback to triple-fault asserted INIT and not RESET (and that this
> > INIT was blocked, hanging the machine).  Switching to acpi reset worked on some
> > machines, but not all.  Either acpi reset was not implemented on the failing
> > machines, or it was wired to INIT and not RESET.
> 
> Right.  INIT does not reset things like the MTRRs, I had forgotten
> that distinction.
> 
> Frequently the firmware when it regains control at 0xffffffff0
> asserts reset, but if we can't get there.  Ouch!
> 
> 
> > Since we already have NMI IPIs, we could disable vmx there.  If it is done
> > unconditionally and without notifiers, there is no chance of having unreviewed
> > surprises sneaking in.
> 
> Sounds good to me.

The problem here is that we can't disable it unconditionally,
so we need to either hack into #UD,  or track on which CPUs it is
enabled. But tracking on which CPUs it is enabled is exactly what KVM
hardware_enable()/hardware_disable() do.

To avoid hacking into #UD only for that, I was being inclined to simply
add core code to track on which CPUs vmx is enabled. But just calling
back into KVM doesn't look too bad, as long as the callback does only
what is strictly necessary. It looks a bit better than moving kvm code
to the core, and looks simple enough to be better than hacking #UD.

Maybe we could have a simple virt-specific callback pointer instead of
a whole notifier chain?

-- 
Eduardo

Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Avi Kivity]

Eduardo Habkost wrote:
> Considering they touch both KVM and kexec, which tree would be best way
> to get them in?
>
>   

kvm.git will be happy to hold these patches, provided they are acked by 
the relevant maintainer.

> (Avi: the patches were sent only to kexec and kvm mailing lists,
> initially. If it's better to submit them to your address also so it gets
> on your queue, please let me know)
>   

Please do copy me.

-- 
error compiling committee.c: too many arguments to function