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

Submitted by Jan Kiszka on Jan. 3, 2011, 8:33 a.m.

Details

Message ID 54192ab9004ed6b528de0846d6a83df432addcd5.1294043582.git.jan.kiszka@web.de
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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