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

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

[Glauber Costa]

Hi,

Avi, in this patch, I am resetting all msrs upon CPU reset.
Hope it is better. Patch 1 is yet another cleanup.

Glauber Costa (2):
  change header for kvm_get_msr_list
  turn off kvmclock when resetting cpu

 qemu-kvm-x86.c |   27 +++++++++++++++++++++++++--
 qemu-kvm.h     |    1 -
 2 files changed, 25 insertions(+), 3 deletions(-)

[PATCH v2 1/2] change header for kvm_get_msr_list

[Glauber Costa]

We don't use kvm_context anymore. So remove it.
Also, the function is not used anywhere else, so make it
static.

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

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 439c31a..5a2b552 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -393,7 +393,7 @@ void kvm_show_code(CPUState *env)
 /*
  * Returns available msr list.  User must free.
  */
-struct kvm_msr_list *kvm_get_msr_list(kvm_context_t kvm)
+static struct kvm_msr_list *kvm_get_msr_list(void)
 {
 	struct kvm_msr_list sizer, *msrs;
 	int r;
@@ -670,7 +670,7 @@ int kvm_arch_qemu_create_context(void)
     if (kvm_shadow_memory)
         kvm_set_shadow_pages(kvm_context, kvm_shadow_memory);
 
-    kvm_msr_list = kvm_get_msr_list(kvm_context);
+    kvm_msr_list = kvm_get_msr_list();
     if (!kvm_msr_list)
 		return -1;
     for (i = 0; i < kvm_msr_list->nmsrs; ++i) {
diff --git a/qemu-kvm.h b/qemu-kvm.h
index ba3808a..5eeed64 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -97,7 +97,6 @@ int handle_io_window(kvm_context_t kvm);
 int try_push_interrupts(kvm_context_t kvm);
 
 #if defined(__x86_64__) || defined(__i386__)
-struct kvm_msr_list *kvm_get_msr_list(kvm_context_t);
 int kvm_get_msrs(CPUState *env, struct kvm_msr_entry *msrs, int n);
 int kvm_set_msrs(CPUState *env, struct kvm_msr_entry *msrs, int n);
 int kvm_get_mce_cap_supported(kvm_context_t, uint64_t *mce_cap,
-- 
1.6.2.2

[PATCH v2 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 writting 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.

It is also necessary to reset other msrs, so this patch resets
everything that kvm exports through its MSR list.

This patch fixes the issue for me.

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

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 5a2b552..bd74316 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -1417,8 +1417,31 @@ void kvm_arch_push_nmi(void *opaque)
 }
 #endif /* KVM_CAP_USER_NMI */
 
+static int kvm_reset_msrs(CPUState *env)
+{
+    struct {
+        struct kvm_msrs info;
+        struct kvm_msr_entry entries[100];
+    } msr_data;
+    int n;
+    struct kvm_msr_entry *msrs = msr_data.entries;
+
+    if (!kvm_msr_list)
+        return -1;
+
+    for (n = 0; n < kvm_msr_list->nmsrs; n++) {
+        kvm_msr_entry_set(&msrs[n], kvm_msr_list->indices[n], 0);
+    }
+
+    msr_data.info.nmsrs = n;
+
+    return kvm_vcpu_ioctl(env, KVM_SET_MSRS, &msr_data);
+}
+
+
 void kvm_arch_cpu_reset(CPUState *env)
 {
+    kvm_reset_msrs(env);
     kvm_arch_reset_vcpu(env);
     kvm_reset_mpstate(env);
 }
-- 
1.6.2.2

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

[Zachary Amsden]

On 05/05/2010 09:19 AM, 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 writting 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.
>
> It is also necessary to reset other msrs, so this patch resets
> everything that kvm exports through its MSR list.
>    

Does that include the TSC?

Zach

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

[Glauber Costa]

On Wed, May 05, 2010 at 10:33:52AM -1000, Zachary Amsden wrote:
> On 05/05/2010 09:19 AM, 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 writting 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.
> >
> >It is also necessary to reset other msrs, so this patch resets
> >everything that kvm exports through its MSR list.
> 
> Does that include the TSC?
AFAIK, yes.

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

[Marcelo Tosatti]

On Wed, May 05, 2010 at 03:19:19PM -0400, Glauber Costa wrote:
> Hi,
> 
> Avi, in this patch, I am resetting all msrs upon CPU reset.
> Hope it is better. Patch 1 is yet another cleanup.
> 
> Glauber Costa (2):
>   change header for kvm_get_msr_list
>   turn off kvmclock when resetting cpu

Applied, thanks.

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

[Avi Kivity]

On 05/05/2010 10:19 PM, Glauber Costa wrote:
> Hi,
>
> Avi, in this patch, I am resetting all msrs upon CPU reset.
> Hope it is better. Patch 1 is yet another cleanup.
>    

Looks good.

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