[PATCH 0/2] fix kvmclock bug - memory corruption

[PATCH 0/2] fix kvmclock bug - memory corruption

[Glauber Costa]

This patch series fixes I bug I just found with kvmclock,
when I booted into a kernel without kvmclock enabled.

Since I am setting msrs, I took the oportunity to
use yet another function from upstream qemu (patch 1).

Enjoy

Glauber Costa (2):
  replace set_msr_entry with kvm_msr_entry
  turn off kvmclock when resetting cpu

 qemu-kvm-x86.c    |   58 ++++++++++++++++++++++++++++++++---------------------
 target-i386/kvm.c |    3 ++
 2 files changed, 38 insertions(+), 23 deletions(-)

[PATCH 1/2] replace set_msr_entry with kvm_msr_entry

[Glauber Costa]

this is yet another function that upstream qemu implements,
so we can just use its implementation.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm-x86.c    |   39 ++++++++++++++++-----------------------
 target-i386/kvm.c |    3 +++
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 748ff69..439c31a 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -693,13 +693,6 @@ int kvm_arch_qemu_create_context(void)
     return 0;
 }
 
-static void set_msr_entry(struct kvm_msr_entry *entry, uint32_t index,
-                          uint64_t data)
-{
-    entry->index = index;
-    entry->data  = data;
-}
-
 /* returns 0 on success, non-0 on failure */
 static int get_msr_entry(struct kvm_msr_entry *entry, CPUState *env)
 {
@@ -960,19 +953,19 @@ void kvm_arch_load_regs(CPUState *env, int level)
     /* msrs */
     n = 0;
     /* Remember to increase msrs size if you add new registers below */
-    set_msr_entry(&msrs[n++], MSR_IA32_SYSENTER_CS,  env->sysenter_cs);
-    set_msr_entry(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
-    set_msr_entry(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip);
+    kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS,  env->sysenter_cs);
+    kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
+    kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip);
     if (kvm_has_msr_star)
-	set_msr_entry(&msrs[n++], MSR_STAR,              env->star);
+	kvm_msr_entry_set(&msrs[n++], MSR_STAR,              env->star);
     if (kvm_has_vm_hsave_pa)
-        set_msr_entry(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave);
+        kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave);
 #ifdef TARGET_X86_64
     if (lm_capable_kernel) {
-        set_msr_entry(&msrs[n++], MSR_CSTAR,             env->cstar);
-        set_msr_entry(&msrs[n++], MSR_KERNELGSBASE,      env->kernelgsbase);
-        set_msr_entry(&msrs[n++], MSR_FMASK,             env->fmask);
-        set_msr_entry(&msrs[n++], MSR_LSTAR  ,           env->lstar);
+        kvm_msr_entry_set(&msrs[n++], MSR_CSTAR,             env->cstar);
+        kvm_msr_entry_set(&msrs[n++], MSR_KERNELGSBASE,      env->kernelgsbase);
+        kvm_msr_entry_set(&msrs[n++], MSR_FMASK,             env->fmask);
+        kvm_msr_entry_set(&msrs[n++], MSR_LSTAR  ,           env->lstar);
     }
 #endif
     if (level == KVM_PUT_FULL_STATE) {
@@ -983,20 +976,20 @@ void kvm_arch_load_regs(CPUState *env, int level)
          * huge jump-backs that would occur without any writeback at all.
          */
         if (smp_cpus == 1 || env->tsc != 0) {
-            set_msr_entry(&msrs[n++], MSR_IA32_TSC, env->tsc);
+            kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }
-        set_msr_entry(&msrs[n++], MSR_KVM_SYSTEM_TIME, env->system_time_msr);
-        set_msr_entry(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
+        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);
     }
 #ifdef KVM_CAP_MCE
     if (env->mcg_cap) {
         if (level == KVM_PUT_RESET_STATE)
-            set_msr_entry(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
+            kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
         else if (level == KVM_PUT_FULL_STATE) {
-            set_msr_entry(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
-            set_msr_entry(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl);
+            kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
+            kvm_msr_entry_set(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl);
             for (i = 0; i < (env->mcg_cap & 0xff); i++)
-                set_msr_entry(&msrs[n++], MSR_MC0_CTL + i, env->mce_banks[i]);
+                kvm_msr_entry_set(&msrs[n++], MSR_MC0_CTL + i, env->mce_banks[i]);
         }
     }
 #endif
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5239eaf..56740bd 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -552,6 +552,8 @@ static int kvm_put_sregs(CPUState *env)
     return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
 }
 
+#endif 
+
 static void kvm_msr_entry_set(struct kvm_msr_entry *entry,
                               uint32_t index, uint64_t value)
 {
@@ -559,6 +561,7 @@ static void kvm_msr_entry_set(struct kvm_msr_entry *entry,
     entry->data = value;
 }
 
+#ifdef KVM_UPSTREAM
 static int kvm_put_msrs(CPUState *env, int level)
 {
     struct {
-- 
1.6.2.2

[PATCH 2/2] turn off kvmclock when resetting cpu

[Glauber Costa]

Currently, in the linux kernel, we reset kvmclock if we are rebooting
into a crash kernel through kexec. The rationale, is that a new kernel
won't follow the same memory addresses, and the memory where kvmclock is
located in the first kernel, will be something else in the second one.

We don't do it in normal reboots, because the second kernel ends up
registering kvmclock again, which has the effect of turning off the
first instance.

This is, however, totally wrong. This assumes we're booting into
a kernel that also has kvmclock enabled. If by some reason we reboot
into something that doesn't do kvmclock including but not limited to:
 * rebooting into an older kernel without kvmclock support,
 * rebooting with no-kvmclock,
 * rebootint into another O.S,

we'll simply have the hypervisor writing into a random memory position
into the guest. Neat, uh?

Moreover, I believe the fix belongs in qemu, since it is the entity
more prepared to detect all kinds of reboots (by means of a cpu_reset),
not to mention the presence of misbehaving guests, that can forget
to turn kvmclock off.

This patch fixes the issue for me.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm-x86.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 439c31a..4b94e04 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -1417,8 +1417,27 @@ void kvm_arch_push_nmi(void *opaque)
 }
 #endif /* KVM_CAP_USER_NMI */
 
+static int kvm_turn_off_clock(CPUState *env)
+{
+    struct {
+        struct kvm_msrs info;
+        struct kvm_msr_entry entries[100];
+    } msr_data;
+
+    struct kvm_msr_entry *msrs = msr_data.entries;
+    int n = 0;
+
+    kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME, 0);
+    kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, 0);
+    msr_data.info.nmsrs = n;
+
+    return kvm_vcpu_ioctl(env, KVM_SET_MSRS, &msr_data);
+}
+
+
 void kvm_arch_cpu_reset(CPUState *env)
 {
+    kvm_turn_off_clock(env);
     kvm_arch_reset_vcpu(env);
     kvm_reset_mpstate(env);
 }
-- 
1.6.2.2

Re: [PATCH 0/2] fix kvmclock bug - memory corruption

[Zachary Amsden]

On 05/04/2010 08:35 AM, Glauber Costa wrote:
> This patch series fixes I bug I just found with kvmclock,
> when I booted into a kernel without kvmclock enabled.
>
> Since I am setting msrs, I took the oportunity to
> use yet another function from upstream qemu (patch 1).
>
> Enjoy
>
> Glauber Costa (2):
>    replace set_msr_entry with kvm_msr_entry
>    turn off kvmclock when resetting cpu
>
>   qemu-kvm-x86.c    |   58 ++++++++++++++++++++++++++++++++---------------------
>   target-i386/kvm.c |    3 ++
>   2 files changed, 38 insertions(+), 23 deletions(-)
>
>    

Acked-by: Zachary Amsden <zamsden@redhat.com>

Re: [PATCH 2/2] turn off kvmclock when resetting cpu

[Avi Kivity]

On 05/04/2010 09:35 PM, Glauber Costa wrote:
> Currently, in the linux kernel, we reset kvmclock if we are rebooting
> into a crash kernel through kexec. The rationale, is that a new kernel
> won't follow the same memory addresses, and the memory where kvmclock is
> located in the first kernel, will be something else in the second one.
>
> We don't do it in normal reboots, because the second kernel ends up
> registering kvmclock again, which has the effect of turning off the
> first instance.
>
> This is, however, totally wrong. This assumes we're booting into
> a kernel that also has kvmclock enabled. If by some reason we reboot
> into something that doesn't do kvmclock including but not limited to:
>   * rebooting into an older kernel without kvmclock support,
>   * rebooting with no-kvmclock,
>   * rebootint into another O.S,
>
> we'll simply have the hypervisor writing into a random memory position
> into the guest. Neat, uh?
>
> Moreover, I believe the fix belongs in qemu, since it is the entity
> more prepared to detect all kinds of reboots (by means of a cpu_reset),
> not to mention the presence of misbehaving guests, that can forget
> to turn kvmclock off.
>
> This patch fixes the issue for me.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   qemu-kvm-x86.c |   19 +++++++++++++++++++
>   1 files changed, 19 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> index 439c31a..4b94e04 100644
> --- a/qemu-kvm-x86.c
> +++ b/qemu-kvm-x86.c
> @@ -1417,8 +1417,27 @@ void kvm_arch_push_nmi(void *opaque)
>   }
>   #endif /* KVM_CAP_USER_NMI */
>
> +static int kvm_turn_off_clock(CPUState *env)
> +{
> +    struct {
> +        struct kvm_msrs info;
> +        struct kvm_msr_entry entries[100];
> +    } msr_data;
> +
> +    struct kvm_msr_entry *msrs = msr_data.entries;
> +    int n = 0;
> +
> +    kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME, 0);
> +    kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, 0);
>    

This fails if the kernel doesn't support those MSRs.  Moreover, you need 
to use the new MSRs as well if we are ever to succeed in deprecating the 
old ones.

> +    msr_data.info.nmsrs = n;
> +
> +    return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
> +}
> +
> +
>    

How about a different approach?  Query the supported MSRs 
(KVM_GET_MSR_LIST or thereabout) and reset them (with special cases for 
the TSC, and the old clock MSRs when the new ones are present)?

Long term we need a kernel reset function, but this will do for now.

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

Re: [PATCH 1/2] replace set_msr_entry with kvm_msr_entry

[Avi Kivity]

On 05/04/2010 09:35 PM, Glauber Costa wrote:
> this is yet another function that upstream qemu implements,
> so we can just use its implementation.
>
>    

Applied, thanks.

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

Re: [PATCH 2/2] turn off kvmclock when resetting cpu

[Glauber Costa]

On Wed, May 05, 2010 at 10:26:43AM +0300, Avi Kivity wrote:
 
> >+    msr_data.info.nmsrs = n;
> >+
> >+    return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
> >+}
> >+
> >+
> 
> How about a different approach?  Query the supported MSRs
> (KVM_GET_MSR_LIST or thereabout) and reset them (with special cases
> for the TSC, and the old clock MSRs when the new ones are present)?
I didn't went that route because I was unsure that every one of them
would be resetable by writing 0 on it.

And if we are going to special case the most part of it, then there
is no point in doing it.

If you think it is doable to special case just the tsc, then I am fine.

Re: [PATCH 2/2] turn off kvmclock when resetting cpu

[Avi Kivity]

On 05/05/2010 06:24 PM, Glauber Costa wrote:
> On Wed, May 05, 2010 at 10:26:43AM +0300, Avi Kivity wrote:
>
>    
>>> +    msr_data.info.nmsrs = n;
>>> +
>>> +    return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
>>> +}
>>> +
>>> +
>>>        
>> How about a different approach?  Query the supported MSRs
>> (KVM_GET_MSR_LIST or thereabout) and reset them (with special cases
>> for the TSC, and the old clock MSRs when the new ones are present)?
>>      
> I didn't went that route because I was unsure that every one of them
> would be resetable by writing 0 on it.
>    

There are probably others.  We should reset them correctly anyway.

It's probably done by generic qemu code so it works.

> And if we are going to special case the most part of it, then there
> is no point in doing it.
>
> If you think it is doable to special case just the tsc, then I am fine.
>    

I think if we have the following sequence

   clear all msrs
   qemu reset
   kvm specific msr reset

Then we'd be fine.

Oh, and tsc needs to be reset to 0 as well - it isn't a special case.

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

Re: [PATCH 2/2] turn off kvmclock when resetting cpu

[Glauber Costa]

On Wed, May 05, 2010 at 06:34:22PM +0300, Avi Kivity wrote:
> On 05/05/2010 06:24 PM, Glauber Costa wrote:
> >On Wed, May 05, 2010 at 10:26:43AM +0300, Avi Kivity wrote:
> >
> >>>+    msr_data.info.nmsrs = n;
> >>>+
> >>>+    return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
> >>>+}
> >>>+
> >>>+
> >>How about a different approach?  Query the supported MSRs
> >>(KVM_GET_MSR_LIST or thereabout) and reset them (with special cases
> >>for the TSC, and the old clock MSRs when the new ones are present)?
> >I didn't went that route because I was unsure that every one of them
> >would be resetable by writing 0 on it.
> 
> There are probably others.  We should reset them correctly anyway.
> 
> It's probably done by generic qemu code so it works.
> 
> >And if we are going to special case the most part of it, then there
> >is no point in doing it.
> >
> >If you think it is doable to special case just the tsc, then I am fine.
> 
> I think if we have the following sequence
> 
>   clear all msrs
>   qemu reset
>   kvm specific msr reset
> 
> Then we'd be fine.
> 
> Oh, and tsc needs to be reset to 0 as well - it isn't a special case.
This means a guest running on a perfectly synchronized tsc host
will not see a sync tsc. Simply because we can't possible
reset all tscs at the same time.

Re: [PATCH 2/2] turn off kvmclock when resetting cpu

[Marcelo Tosatti]

On Tue, May 04, 2010 at 02:35:28PM -0400, Glauber Costa wrote:
> Currently, in the linux kernel, we reset kvmclock if we are rebooting
> into a crash kernel through kexec. The rationale, is that a new kernel
> won't follow the same memory addresses, and the memory where kvmclock is
> located in the first kernel, will be something else in the second one.
> 
> We don't do it in normal reboots, because the second kernel ends up
> registering kvmclock again, which has the effect of turning off the
> first instance.
> 
> This is, however, totally wrong. This assumes we're booting into
> a kernel that also has kvmclock enabled. If by some reason we reboot
> into something that doesn't do kvmclock including but not limited to:
>  * rebooting into an older kernel without kvmclock support,
>  * rebooting with no-kvmclock,
>  * rebootint into another O.S,
> 
> we'll simply have the hypervisor writing into a random memory position
> into the guest. Neat, uh?
> 
> Moreover, I believe the fix belongs in qemu, since it is the entity
> more prepared to detect all kinds of reboots (by means of a cpu_reset),
> not to mention the presence of misbehaving guests, that can forget
> to turn kvmclock off.
> 
> This patch fixes the issue for me.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  qemu-kvm-x86.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> index 439c31a..4b94e04 100644
> --- a/qemu-kvm-x86.c
> +++ b/qemu-kvm-x86.c
> @@ -1417,8 +1417,27 @@ void kvm_arch_push_nmi(void *opaque)
>  }
>  #endif /* KVM_CAP_USER_NMI */
>  
> +static int kvm_turn_off_clock(CPUState *env)
> +{
> +    struct {
> +        struct kvm_msrs info;
> +        struct kvm_msr_entry entries[100];
> +    } msr_data;
> +
> +    struct kvm_msr_entry *msrs = msr_data.entries;
> +    int n = 0;
> +
> +    kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME, 0);
> +    kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, 0);
> +    msr_data.info.nmsrs = n;
> +
> +    return kvm_vcpu_ioctl(env, KVM_SET_MSRS, &msr_data);
> +}
> +
> +
>  void kvm_arch_cpu_reset(CPUState *env)
>  {
> +    kvm_turn_off_clock(env);

Better to zero env->system_time_msr and wall_clock_msr here, and modify
kvm_arch_load_regs to write those MSRs on KVM_PUT_RESET_STATE (to be
conformant with the new writeback scheme).