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

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

[Avi Kivity]

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 "kvm: exiting hardware virtualization\n");
 		kvm_rebooting = true;
 		on_each_cpu(hardware_disable, NULL, 1);
+		break;
+	default:
+		break;
 	}
 	return NOTIFY_OK;
 }
-- 
1.6.0

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

[Ingo Molnar]

* Avi Kivity <avi@qumranet.com> wrote:

> 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.

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

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

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi@qumranet.com> wrote:
>
>   
>> 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.
>>     
>
> looks good to me - i was bitten by that problem on a testbox.
>   

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).

The ACPI spec says:

> 4.7.3.6 Reset Register
>
> The optional ACPI reset mechanism specifies a standard mechanism that 
> provides a complete system reset.
> When implemented, this mechanism must reset the entire system. This 
> includes processors, core logic, all
> buses, and all peripherals. From an OSPM perspective, asserting the 
> reset mechanism is the logical
> equivalent to power cycling the machine. Upon gaining control after a 
> reset, OSPM will perform actions in
> like manner to a cold boot.
> The reset mechanism is implemented via an 8-bit register described by 
> RESET_REG in the FADT (always
> accessed via the natural alignment and size described in RESET_REG). 
> To reset the machine, software will
> write a value (indicated in RESET_VALUE in FADT) to the reset 
> register. The RESET_REG field in the
> FADT indicates the location of the reset register.
> The reset register may exist only in I/O space, Memory space, or in 
> PCI Configuration space on a function
> in bus 0. Therefore, the Address_Space_ID value in RESET_REG must be 
> set to I/O space, Memory space,
> or PCI Configuration space (with a bus number of 0). As the register 
> is only 8 bits, Register_Bit_Width
> must be 8 and Register_Bit_Offset must be 0.
> The system must reset immediately following the write to this 
> register. OSPM assumes that the processor
> will not execute beyond the write instruction. OSPM should execute 
> spin loops on the CPUs in the system
> following a write to this register.

Which seems to be what we want? Maybe we should just try acpi_reboot() 
before the other stuff.

>   Acked-by: Ingo Molnar <mingo@elte.hu>
>
> Seems best to merge this via the KVM tree, right?
>
>   

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

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

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

[Ingo Molnar]

* Avi Kivity <avi@qumranet.com> wrote:

> Ingo Molnar wrote:
>> * Avi Kivity <avi@qumranet.com> wrote:
>>
>>   
>>> 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.
>>>     
>>
>> looks good to me - i was bitten by that problem on a testbox.
>>   
>
> 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).

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 
approaches we try.

> Which seems to be what we want? Maybe we should just try acpi_reboot() 
> before the other stuff.

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

	Ingo

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

[Avi Kivity]

Ingo Molnar wrote:

  

>> 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).
>>     
>
> 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 
> approaches we try.
>
>   

It worked on my host.  Since it will fall back to keyboard reset and 
triple fault, it seems fairly safe.

>> Which seems to be what we want? Maybe we should just try acpi_reboot() 
>> before the other stuff.
>>     
>
> perhaps in a separate commit, for v2.6.28 at the earliest.
>   

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

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

[Ingo Molnar]

* Avi Kivity <avi@qumranet.com> wrote:

>> 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 
>> approaches we try.
>
> It worked on my host.  Since it will fall back to keyboard reset and 
> triple fault, it seems fairly safe.

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

	Ingo

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

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi@qumranet.com> wrote:
>
>   
>>> 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 
>>> approaches we try.
>>>       
>> It worked on my host.  Since it will fall back to keyboard reset and 
>> triple fault, it seems fairly safe.
>>     
>
> ... except if it hangs in ACPI/SMM code for whatever reason.
>   

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

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

[Eric W. Biederman]

Ingo Molnar <mingo@elte.hu> writes:

> * Avi Kivity <avi@qumranet.com> wrote:
>
>> 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.

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

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

[Avi Kivity]

Eric W. Biederman wrote:
> Ingo Molnar <mingo@elte.hu> writes:
>
>   
>> * Avi Kivity <avi@qumranet.com> wrote:
>>
>>     
>>> 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.
>>>       
>
> 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?
>
>   

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

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

[Eric W. Biederman]

Ingo Molnar <mingo@elte.hu> writes:

> * Avi Kivity <avi@qumranet.com> wrote:
>
>> 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.

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