[PATCH] disable clock before rebooting.

[PATCH] disable clock before rebooting.

[Glauber Costa]

This patch writes 0 (actually, what really matters is that the
LSB is cleared) to the system time msr before rebooting/shutting down
the machine.

Without it, we can have a random memory location being written
when the guest comes back

Signed-off-by: Glauber Costa <gcosta@redhat.com>
---
 arch/x86/kernel/kvmclock.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index f654a12..5c9ff8d 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -21,6 +21,7 @@ #include <linux/kvm_para.h>
 #include <asm/arch_hooks.h>
 #include <asm/msr.h>
 #include <linux/percpu.h>
+#include <asm/reboot.h>
 
 #define KVM_SCALE 22
 
@@ -142,6 +143,32 @@ static void kvm_setup_secondary_clock(vo
 	setup_secondary_APIC_clock();
 }
 
+/*
+ * After the clock is registered, the host will keep writing to the
+ * registered memory location. If the guest happens to shutdown, or restart,
+ * this memory won't be valid. In cases like kexec, in which you install a new kernel,
+ * this will mean a random memory location will be kept being written. So before
+ * any kind of shutdown from our side, we unregister the clock by writting anything
+ * that does not have the 'enable' bit set in the msr
+ */ 
+static void kvm_restart(char *unused) {
+	native_write_msr_safe(MSR_KVM_SYSTEM_TIME, 0, 0);
+	native_machine_restart(unused);
+}
+
+/* Forgive me dear lord, for my laziness */
+#define kvm_reboot_fn(x) \
+static void kvm_##x(void) { \
+	native_write_msr_safe(MSR_KVM_SYSTEM_TIME, 0, 0); \
+	native_machine_##x(); \
+}
+
+kvm_reboot_fn(emergency_restart)
+kvm_reboot_fn(shutdown)
+kvm_reboot_fn(halt)
+kvm_reboot_fn(power_off)
+#undef kvm_reboot_fn
+
 void __init kvmclock_init(void)
 {
 	if (!kvm_para_available())
@@ -154,6 +181,11 @@ void __init kvmclock_init(void)
 		pv_time_ops.set_wallclock = kvm_set_wallclock;
 		pv_time_ops.sched_clock = kvm_clock_read;
 		pv_apic_ops.setup_secondary_clock = kvm_setup_secondary_clock;
+		machine_ops.emergency_restart = kvm_emergency_restart;
+		machine_ops.shutdown  = kvm_shutdown;
+		machine_ops.restart  = kvm_restart;
+		machine_ops.halt  = kvm_halt;
+		machine_ops.power_off  = kvm_power_off;
 		clocksource_register(&kvm_clock);
 	}
 }
-- 
1.4.2


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] disable clock before rebooting.

[Avi Kivity]

Glauber Costa wrote:
> This patch writes 0 (actually, what really matters is that the
> LSB is cleared) to the system time msr before rebooting/shutting down
> the machine.
>
> Without it, we can have a random memory location being written
> when the guest comes back
>
> Signed-off-by: Glauber Costa <gcosta@redhat.com>
> ---
>  arch/x86/kernel/kvmclock.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index f654a12..5c9ff8d 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -21,6 +21,7 @@ #include <linux/kvm_para.h>
>  #include <asm/arch_hooks.h>
>  #include <asm/msr.h>
>  #include <linux/percpu.h>
> +#include <asm/reboot.h>
>  
>  #define KVM_SCALE 22
>  
> @@ -142,6 +143,32 @@ static void kvm_setup_secondary_clock(vo
>  	setup_secondary_APIC_clock();
>  }
>  
> +/*
> + * After the clock is registered, the host will keep writing to the
> + * registered memory location. If the guest happens to shutdown, or restart,
> + * this memory won't be valid. In cases like kexec, in which you install a new kernel,
> + * this will mean a random memory location will be kept being written. So before
> + * any kind of shutdown from our side, we unregister the clock by writting anything
> + * that does not have the 'enable' bit set in the msr
> + */ 
> +static void kvm_restart(char *unused) {
>   

This looks like a struct, with the { sitting there on the end.

> +	native_write_msr_safe(MSR_KVM_SYSTEM_TIME, 0, 0);
> +	native_machine_restart(unused);
> +}
> +
> +/* Forgive me dear lord, for my laziness */
> +#define kvm_reboot_fn(x) \
> +static void kvm_##x(void) { \
> +	native_write_msr_safe(MSR_KVM_SYSTEM_TIME, 0, 0); \
> +	native_machine_##x(); \
> +}
> +
> +kvm_reboot_fn(emergency_restart)
> +kvm_reboot_fn(shutdown)
> +kvm_reboot_fn(halt)
> +kvm_reboot_fn(power_off)
> +#undef kvm_reboot_fn
> +
>   

Why not go all the way and to _restart the same way?

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] disable clock before rebooting.

[Glauber Costa]

Avi Kivity wrote:
> Glauber Costa wrote:
>> This patch writes 0 (actually, what really matters is that the
>> LSB is cleared) to the system time msr before rebooting/shutting down
>> the machine.
>>
>> Without it, we can have a random memory location being written
>> when the guest comes back
>>
>> Signed-off-by: Glauber Costa <gcosta@redhat.com>
>> ---
>>  arch/x86/kernel/kvmclock.c |   32 ++++++++++++++++++++++++++++++++
>>  1 files changed, 32 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
>> index f654a12..5c9ff8d 100644
>> --- a/arch/x86/kernel/kvmclock.c
>> +++ b/arch/x86/kernel/kvmclock.c
>> @@ -21,6 +21,7 @@ #include <linux/kvm_para.h>
>>  #include <asm/arch_hooks.h>
>>  #include <asm/msr.h>
>>  #include <linux/percpu.h>
>> +#include <asm/reboot.h>
>>  
>>  #define KVM_SCALE 22
>>  
>> @@ -142,6 +143,32 @@ static void kvm_setup_secondary_clock(vo
>>      setup_secondary_APIC_clock();
>>  }
>>  
>> +/*
>> + * After the clock is registered, the host will keep writing to the
>> + * registered memory location. If the guest happens to shutdown, or 
>> restart,
>> + * this memory won't be valid. In cases like kexec, in which you 
>> install a new kernel,
>> + * this will mean a random memory location will be kept being 
>> written. So before
>> + * any kind of shutdown from our side, we unregister the clock by 
>> writting anything
>> + * that does not have the 'enable' bit set in the msr
>> + */ +static void kvm_restart(char *unused) {
>>   
> 
> This looks like a struct, with the { sitting there on the end.
my bad.

>> +    native_write_msr_safe(MSR_KVM_SYSTEM_TIME, 0, 0);
>> +    native_machine_restart(unused);
>> +}
>> +
>> +/* Forgive me dear lord, for my laziness */
>> +#define kvm_reboot_fn(x) \
>> +static void kvm_##x(void) { \
>> +    native_write_msr_safe(MSR_KVM_SYSTEM_TIME, 0, 0); \
>> +    native_machine_##x(); \
>> +}
>> +
>> +kvm_reboot_fn(emergency_restart)
>> +kvm_reboot_fn(shutdown)
>> +kvm_reboot_fn(halt)
>> +kvm_reboot_fn(power_off)
>> +#undef kvm_reboot_fn
>> +
>>   
> 
> Why not go all the way and to _restart the same way?
> 
Because it got a parameter, and doing it in the same macro would make
my beautiful macros ugly.
Using another one, to pass the argument, didn't seem justifiable to me, 
since there were just one of its kind.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] disable clock before rebooting.

[Avi Kivity]

Glauber Costa wrote:
>>
>> Why not go all the way and to _restart the same way?
>>
> Because it got a parameter, and doing it in the same macro would make
> my beautiful macros ugly.
> Using another one, to pass the argument, didn't seem justifiable to 
> me, since there were just one of its kind.

Yes, of course.  My mistake.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] disable clock before rebooting.

[Avi Kivity]

Glauber Costa wrote:
> This patch writes 0 (actually, what really matters is that the
> LSB is cleared) to the system time msr before rebooting/shutting down
> the machine.
>
> Without it, we can have a random memory location being written
> when the guest comes back
>  	if (!kvm_para_available())
> @@ -154,6 +181,11 @@ void __init kvmclock_init(void)
>  		pv_time_ops.set_wallclock = kvm_set_wallclock;
>  		pv_time_ops.sched_clock = kvm_clock_read;
>  		pv_apic_ops.setup_secondary_clock = kvm_setup_secondary_clock;
> +		machine_ops.emergency_restart = kvm_emergency_restart;
> +		machine_ops.shutdown  = kvm_shutdown;
> +		machine_ops.restart  = kvm_restart;
> +		machine_ops.halt  = kvm_halt;
> +		machine_ops.power_off  = kvm_power_off;
>  		clocksource_register(&kvm_clock);
>  	}
>  }
>   

Oh, I think that these are all unnecessary.  You need to stop the clock 
only if the memory it uses will be reused.  Halt, shutdown and poweroff 
clearly don't.  Resets need to go through the host anyway, since they 
can be invoked without the guest knowing about it.

The only case I can think of where we need to stop the clock is kexec.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] disable clock before rebooting.

[Glauber Costa]

Avi Kivity wrote:
> Glauber Costa wrote:
>> This patch writes 0 (actually, what really matters is that the
>> LSB is cleared) to the system time msr before rebooting/shutting down
>> the machine.
>>
>> Without it, we can have a random memory location being written
>> when the guest comes back
>>      if (!kvm_para_available())
>> @@ -154,6 +181,11 @@ void __init kvmclock_init(void)
>>          pv_time_ops.set_wallclock = kvm_set_wallclock;
>>          pv_time_ops.sched_clock = kvm_clock_read;
>>          pv_apic_ops.setup_secondary_clock = kvm_setup_secondary_clock;
>> +        machine_ops.emergency_restart = kvm_emergency_restart;
>> +        machine_ops.shutdown  = kvm_shutdown;
>> +        machine_ops.restart  = kvm_restart;
>> +        machine_ops.halt  = kvm_halt;
>> +        machine_ops.power_off  = kvm_power_off;
>>          clocksource_register(&kvm_clock);
>>      }
>>  }
>>   
> 
> Oh, I think that these are all unnecessary.  You need to stop the clock 
> only if the memory it uses will be reused.  Halt, shutdown and poweroff 
> clearly don't.  Resets need to go through the host anyway, since they 
> can be invoked without the guest knowing about it.

power off, agreed.
halt, it doesn't really do anything anyway in reboot.c, and is here just 
for "future completeness".

> The only case I can think of where we need to stop the clock is kexec.
> 
as for kexec, it uses precisely the shutdown function, doesn't it?

Or is it crash_shutdown?
Humm, /me looks, and I think it's the later, right?

If this is true and resets goes through the host, then we don't even 
need the header exports. Ingo'll be happy.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] disable clock before rebooting.

[Avi Kivity]

Glauber Costa wrote:
> as for kexec, it uses precisely the shutdown function, doesn't it?
>
> Or is it crash_shutdown?
> Humm, /me looks, and I think it's the later, right?
>

Only on crash-triggered kexecs.  It can also happen via sys_reboot().  
Which, it appears, goes through machine_shutdown().

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] disable clock before rebooting.

[Glauber Costa]

Avi Kivity wrote:
> Glauber Costa wrote:
>> as for kexec, it uses precisely the shutdown function, doesn't it?
>>
>> Or is it crash_shutdown?
>> Humm, /me looks, and I think it's the later, right?
>>
> 
> Only on crash-triggered kexecs.  It can also happen via sys_reboot().  
> Which, it appears, goes through machine_shutdown().
> 
yeah, it's already addressed in my new patches

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/