Patchwork [v2,11/17] kvm: x86: Reset paravirtual MSRs

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 3, 2011, 8:33 a.m.
Message ID <54192ab9004ed6b528de0846d6a83df432addcd5.1294043582.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/77228/
State New
Headers show

Comments

Jan Kiszka - Jan. 3, 2011, 8:33 a.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

Make sure to clear MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, and
MSR_KVM_ASYNC_PF_EN so that a freshly booted guest cannot be disturbed
by old values.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
CC: Glauber Costa <glommer@redhat.com>
---
 target-i386/kvm.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
Glauber Costa - Jan. 3, 2011, 4:40 p.m.
On Mon, 2011-01-03 at 09:33 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Make sure to clear MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, and
> MSR_KVM_ASYNC_PF_EN so that a freshly booted guest cannot be disturbed
> by old values.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> CC: Glauber Costa <glommer@redhat.com>
> ---
>  target-i386/kvm.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index d8f26bf..664a4a0 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -453,6 +453,9 @@ void kvm_arch_reset_vcpu(CPUState *env)
>      env->nmi_injected = 0;
>      env->nmi_pending = 0;
>      env->xcr0 = 1;
> +    env->system_time_msr = 0;
> +    env->wall_clock_msr = 0;
> +    env->async_pf_en_msr = 0;

Have you seen this happening? I'd expect CPUState to be zeroed out over
init. And if it is not, I guess we should...
Jan Kiszka - Jan. 3, 2011, 4:46 p.m.
Am 03.01.2011 17:40, Glauber Costa wrote:
> On Mon, 2011-01-03 at 09:33 +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Make sure to clear MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, and
>> MSR_KVM_ASYNC_PF_EN so that a freshly booted guest cannot be disturbed
>> by old values.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> CC: Glauber Costa <glommer@redhat.com>
>> ---
>>  target-i386/kvm.c |   10 ++++++++++
>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index d8f26bf..664a4a0 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -453,6 +453,9 @@ void kvm_arch_reset_vcpu(CPUState *env)
>>      env->nmi_injected = 0;
>>      env->nmi_pending = 0;
>>      env->xcr0 = 1;
>> +    env->system_time_msr = 0;
>> +    env->wall_clock_msr = 0;
>> +    env->async_pf_en_msr = 0;
> 
> Have you seen this happening? I'd expect CPUState to be zeroed out over
> init. And if it is not, I guess we should...

Ah, true, those three are part of the section that is zeroed. Will drop
that hunk on repost.

Guess we should rather move some other variables in that region too and
avoid clearing them manually like above...

Jan
Glauber Costa - Jan. 3, 2011, 4:51 p.m.
On Mon, 2011-01-03 at 17:46 +0100, Jan Kiszka wrote:
> Am 03.01.2011 17:40, Glauber Costa wrote:
> > On Mon, 2011-01-03 at 09:33 +0100, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Make sure to clear MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, and
> >> MSR_KVM_ASYNC_PF_EN so that a freshly booted guest cannot be disturbed
> >> by old values.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> CC: Glauber Costa <glommer@redhat.com>
> >> ---
> >>  target-i386/kvm.c |   10 ++++++++++
> >>  1 files changed, 10 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index d8f26bf..664a4a0 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -453,6 +453,9 @@ void kvm_arch_reset_vcpu(CPUState *env)
> >>      env->nmi_injected = 0;
> >>      env->nmi_pending = 0;
> >>      env->xcr0 = 1;
> >> +    env->system_time_msr = 0;
> >> +    env->wall_clock_msr = 0;
> >> +    env->async_pf_en_msr = 0;
> > 
> > Have you seen this happening? I'd expect CPUState to be zeroed out over
> > init. And if it is not, I guess we should...
> 
> Ah, true, those three are part of the section that is zeroed. Will drop
> that hunk on repost.
> 
> Guess we should rather move some other variables in that region too and
> avoid clearing them manually like above...
> 
> Jan
> 
Agreed.

Patch

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index d8f26bf..664a4a0 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -453,6 +453,9 @@  void kvm_arch_reset_vcpu(CPUState *env)
     env->nmi_injected = 0;
     env->nmi_pending = 0;
     env->xcr0 = 1;
+    env->system_time_msr = 0;
+    env->wall_clock_msr = 0;
+    env->async_pf_en_msr = 0;
     if (kvm_irqchip_in_kernel()) {
         env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
                                           KVM_MP_STATE_UNINITIALIZED;
@@ -845,6 +848,13 @@  static int kvm_put_msrs(CPUState *env, int level)
         if (smp_cpus == 1 || env->tsc != 0) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }
+    }
+    /*
+     * The following paravirtual MSRs have side effects on the guest or are
+     * too heavy for normal writeback. Limit them to reset or full state
+     * updates.
+     */
+    if (level >= KVM_PUT_RESET_STATE) {
         kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME,
                           env->system_time_msr);
         kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr);