Re: [PATCH] Fix emergency_restart (sysrq-b) with kvm loaded on Intel hosts

Previous thread: [PATCH] driver-core: use klist for class device list and implement iterator by Tejun Heo on Monday, August 25, 2008 - 2:06 am. (1 message)

Next thread: [PATCH 0/2] usb: musb bug fixing patches by Bryan Wu on Monday, August 25, 2008 - 2:13 am. (7 messages)
From: Avi Kivity
Date: Monday, August 25, 2008 - 2:11 am

Enabling Intel VT has the curious side effect whereby the INIT signal is
blocked.  Rather than comment on the wisdom of this side effect, this patch
adds an emergency restart reboot notifier, and modifies the kvm reboot
notifier to disable VT on emergency reboot.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 include/linux/notifier.h |    1 +
 kernel/sys.c             |    3 +++
 virt/kvm/kvm_main.c      |   10 ++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index da2698b..59123e4 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -203,6 +203,7 @@ static inline int notifier_to_errno(int ret)
 #define SYS_RESTART	SYS_DOWN
 #define SYS_HALT	0x0002	/* Notify of system halt */
 #define SYS_POWER_OFF	0x0003	/* Notify of system power off */
+#define SYS_EMERGENCY_RESTART 0x0004 /* sysrq-b; no locks taken */
 
 #define NETLINK_URELEASE	0x0001	/* Unicast netlink socket released */
 
diff --git a/kernel/sys.c b/kernel/sys.c
index 038a7bc..289dba3 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -270,6 +270,9 @@ out_unlock:
  */
 void emergency_restart(void)
 {
+	struct raw_notifier_head list = { .head = reboot_notifier_list.head };
+
+	raw_notifier_call_chain(&list, SYS_EMERGENCY_RESTART, NULL);
 	machine_emergency_restart();
 }
 EXPORT_SYMBOL_GPL(emergency_restart);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 0309571..125041f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1550,14 +1550,20 @@ EXPORT_SYMBOL_GPL(kvm_handle_fault_on_reboot);
 static int kvm_reboot(struct notifier_block *notifier, unsigned long val,
 		      void *v)
 {
-	if (val == SYS_RESTART) {
+	switch (val) {
+	case SYS_RESTART:
+		printk(KERN_INFO "kvm: exiting hardware virtualization\n");
+		/* coming through! */
+	case SYS_EMERGENCY_RESTART:
 		/*
 		 * Some (well, at least mine) BIOSes hang on reboot if
 		 * in vmx root mode.
 		 */
-		printk(KERN_INFO ...
From: Ingo Molnar
Date: Monday, August 25, 2008 - 2:15 am

looks good to me - i was bitten by that problem on a testbox.

  Acked-by: Ingo Molnar <mingo@elte.hu>

Seems best to merge this via the KVM tree, right?

	Ingo
--

From: Avi Kivity
Date: Monday, August 25, 2008 - 2:27 am

I'm a little worried about making emergency restart more complex.

Another thing that worries me is that emergency_restart() doesn't reset 
the box -- it sends INIT.  We could do better by using the ACPI FADT 
reset register (hopefully that's connected to RESET).


Which seems to be what we want? Maybe we should just try acpi_reboot() 

I'm happy to do that, if everyone feels the patch is fine.

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

--

From: Ingo Molnar
Date: Monday, August 25, 2008 - 2:30 am

reboot was always a bit fragile - i think we should only do that if we 
find a box where the FADT reset works better than the first-wave 

perhaps in a separate commit, for v2.6.28 at the earliest.

	Ingo
--

From: Avi Kivity
Date: Monday, August 25, 2008 - 3:03 am

It worked on my host.  Since it will fall back to keyboard reset and 

I'll send a patch.  I don't think my earlier patch is worthwhile as all 
machines with VT are acpi capable.

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

--

From: Ingo Molnar
Date: Monday, August 25, 2008 - 3:27 am

... except if it hangs in ACPI/SMM code for whatever reason.

	Ingo
--

From: Avi Kivity
Date: Monday, August 25, 2008 - 3:36 am

acpi reboot doesn't call into the aml interpreter.  It just bangs on a 
port that it reads from a static table.  See acpi_reboot().

It's true that SMM could be set up to intercept that port, but in that 
case, it is even more likely that it intercepts the keyboard controller 
port (to translate usb keyboards etc).

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

--

From: Eric W. Biederman
Date: Monday, August 25, 2008 - 6:12 am

Please no notifiers in emergency_restart.

First emergency_restart is not supposed to work reliably it is a best effort tickle
the hardware thing.

Second and more importantly whenever someone adds a notifier instead of a proper hook
to one a code path like this it seems like avoiding building a proper interface so
and I believe keeps us from getting all of the logic and the heuristics right.

Why not just add a disable intel_vt if it is enabled call?

Eric
--

From: Avi Kivity
Date: Monday, August 25, 2008 - 6:35 am

We need to do it across all cpus.

However, a reliable (and simpler) fix has emerged: reset via ACPI.  That 
causes a true reset which VT does not block.


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

--

From: Eric W. Biederman
Date: Monday, August 25, 2008 - 6:14 am

Please no notifiers in emergency_restart.

First emergency_restart is not supposed to work reliably it is a best effort tickle
the hardware thing.

Second and more importantly whenever someone adds a notifier instead of a proper hook
to one a code path like this it seems like avoiding building a proper interface so
and I believe keeps us from getting all of the logic and the heuristics right.

Why not just add a disable intel_vt if it is enabled call?

Eric
--

Previous thread: [PATCH] driver-core: use klist for class device list and implement iterator by Tejun Heo on Monday, August 25, 2008 - 2:06 am. (1 message)

Next thread: [PATCH 0/2] usb: musb bug fixing patches by Bryan Wu on Monday, August 25, 2008 - 2:13 am. (7 messages)