diff mbox

properly save kvm system time msr registers

Message ID 1256047441-9846-1-git-send-email-glommer@redhat.com
State New
Headers show

Commit Message

Glauber Costa Oct. 20, 2009, 2:04 p.m. UTC 
Currently, the msrs involved in setting up pvclock are not saved over
migration and/or save/restore. This patch puts their value in special
fields in our CPUState, and deal with them using vmstate.

kvm also has to account for it, by including them in the msr list
for the ioctls.

This is a backport from qemu-kvm.git

Signed-off-by: Glauber Costa <glommer@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
 target-i386/cpu.h     |    2 ++
 target-i386/kvm.c     |   13 +++++++++++++
 target-i386/machine.c |    3 +++
 3 files changed, 18 insertions(+), 0 deletions(-)

Comments

Gerd Hoffmann Oct. 21, 2009, 1:38 p.m. UTC | #1 
On 10/20/09 16:04, Glauber Costa wrote:
> Currently, the msrs involved in setting up pvclock are not saved over
> migration and/or save/restore. This patch puts their value in special
> fields in our CPUState, and deal with them using vmstate.
>
> kvm also has to account for it, by including them in the msr list
> for the ioctls.

Fails to build.  Fedora 11 machine with distro kernel.

http://repo.or.cz/w/qemu/pdb.git?a=blob;f=build.log;h=5b8a2e83116dc6a74e5045e30c3efe71fa7397ef;hb=0b4f913c89b89b45d3844dc1cf321e3d94871ecb

cheers,
   Gerd
Glauber Costa Oct. 21, 2009, 2:32 p.m. UTC | #2 
On Wed, Oct 21, 2009 at 03:38:31PM +0200, Gerd Hoffmann wrote:
> On 10/20/09 16:04, Glauber Costa wrote:
>> Currently, the msrs involved in setting up pvclock are not saved over
>> migration and/or save/restore. This patch puts their value in special
>> fields in our CPUState, and deal with them using vmstate.
>>
>> kvm also has to account for it, by including them in the msr list
>> for the ioctls.
>
> Fails to build.  Fedora 11 machine with distro kernel.
>
> http://repo.or.cz/w/qemu/pdb.git?a=blob;f=build.log;h=5b8a2e83116dc6a74e5045e30c3efe71fa7397ef;hb=0b4f913c89b89b45d3844dc1cf321e3d94871ecb
strange message. Do we need --kerneldir for that? kvm_para.h should be available...
Gerd Hoffmann Oct. 21, 2009, 2:52 p.m. UTC | #3 
On 10/21/09 16:32, Glauber Costa wrote:
> On Wed, Oct 21, 2009 at 03:38:31PM +0200, Gerd Hoffmann wrote:
>> On 10/20/09 16:04, Glauber Costa wrote:
>>> Currently, the msrs involved in setting up pvclock are not saved over
>>> migration and/or save/restore. This patch puts their value in special
>>> fields in our CPUState, and deal with them using vmstate.
>>>
>>> kvm also has to account for it, by including them in the msr list
>>> for the ioctls.
>>
>> Fails to build.  Fedora 11 machine with distro kernel.
>>
>> http://repo.or.cz/w/qemu/pdb.git?a=blob;f=build.log;h=5b8a2e83116dc6a74e5045e30c3efe71fa7397ef;hb=0b4f913c89b89b45d3844dc1cf321e3d94871ecb
> strange message. Do we need --kerneldir for that?

Requiring --kerneldir for a successful build isn't going to fly.

> kvm_para.h should be available...

Nope:

    $ rpm -ql kernel-headers | grep kvm
    /usr/include/asm/kvm.h
    /usr/include/linux/kvm.h

Did you ever build this?
You should improve your patch qa ...

cheers,
   Gerd
Glauber Costa Oct. 21, 2009, 2:57 p.m. UTC | #4 
On Wed, Oct 21, 2009 at 04:52:00PM +0200, Gerd Hoffmann wrote:
> On 10/21/09 16:32, Glauber Costa wrote:
>> On Wed, Oct 21, 2009 at 03:38:31PM +0200, Gerd Hoffmann wrote:
>>> On 10/20/09 16:04, Glauber Costa wrote:
>>>> Currently, the msrs involved in setting up pvclock are not saved over
>>>> migration and/or save/restore. This patch puts their value in special
>>>> fields in our CPUState, and deal with them using vmstate.
>>>>
>>>> kvm also has to account for it, by including them in the msr list
>>>> for the ioctls.
>>>
>>> Fails to build.  Fedora 11 machine with distro kernel.
>>>
>>> http://repo.or.cz/w/qemu/pdb.git?a=blob;f=build.log;h=5b8a2e83116dc6a74e5045e30c3efe71fa7397ef;hb=0b4f913c89b89b45d3844dc1cf321e3d94871ecb
>> strange message. Do we need --kerneldir for that?
>
> Requiring --kerneldir for a successful build isn't going to fly.
>
>> kvm_para.h should be available...
>
> Nope:
>
>    $ rpm -ql kernel-headers | grep kvm
>    /usr/include/asm/kvm.h
>    /usr/include/linux/kvm.h
>
> Did you ever build this?
> You should improve your patch qa ...
built and tested. I was using --kerneldir all along, however.

One option I see is to redefine the values of msrs in this file.
It seems it already happens for tsc anyway.
Glauber Costa Oct. 21, 2009, 7:39 p.m. UTC | #5 
On Wed, Oct 21, 2009 at 04:52:00PM +0200, Gerd Hoffmann wrote:
> On 10/21/09 16:32, Glauber Costa wrote:
>> On Wed, Oct 21, 2009 at 03:38:31PM +0200, Gerd Hoffmann wrote:
>>> On 10/20/09 16:04, Glauber Costa wrote:
>>>> Currently, the msrs involved in setting up pvclock are not saved over
>>>> migration and/or save/restore. This patch puts their value in special
>>>> fields in our CPUState, and deal with them using vmstate.
>>>>
>>>> kvm also has to account for it, by including them in the msr list
>>>> for the ioctls.
>>>
>>> Fails to build.  Fedora 11 machine with distro kernel.
>>>
>>> http://repo.or.cz/w/qemu/pdb.git?a=blob;f=build.log;h=5b8a2e83116dc6a74e5045e30c3efe71fa7397ef;hb=0b4f913c89b89b45d3844dc1cf321e3d94871ecb
>> strange message. Do we need --kerneldir for that?
>
> Requiring --kerneldir for a successful build isn't going to fly.
>
>> kvm_para.h should be available...
>
> Nope:
>
>    $ rpm -ql kernel-headers | grep kvm
>    /usr/include/asm/kvm.h
>    /usr/include/linux/kvm.h
>
> Did you ever build this?
> You should improve your patch qa ...
btw, it builds fine for me even without --kerneldir, just tested.

and the <linux/kvm_para.h> include is there since Feb08, a while now.

/me wonders what the heck is going on...
Gerd Hoffmann Oct. 22, 2009, 8 a.m. UTC | #6 
On 10/21/09 21:39, Glauber Costa wrote:
>>     $ rpm -ql kernel-headers | grep kvm
>>     /usr/include/asm/kvm.h
>>     /usr/include/linux/kvm.h
>>
>> Did you ever build this?
>> You should improve your patch qa ...
> btw, it builds fine for me even without --kerneldir, just tested.
>
> and the<linux/kvm_para.h>  include is there since Feb08, a while now.

Which doesn't imply "make headers" actually includes that file.  Even 
rawhide aka soon-to-be-F12 ships no kvm_para.h header file in 
kernel-headers.

Just defining the msrs instead of using kvm_para.h should be fine, the 
numbers will not change ...

cheers,
   Gerd
Glauber Costa Oct. 22, 2009, 11:23 a.m. UTC | #7 
On Thu, Oct 22, 2009 at 10:00:37AM +0200, Gerd Hoffmann wrote:
> On 10/21/09 21:39, Glauber Costa wrote:
>>>     $ rpm -ql kernel-headers | grep kvm
>>>     /usr/include/asm/kvm.h
>>>     /usr/include/linux/kvm.h
>>>
>>> Did you ever build this?
>>> You should improve your patch qa ...
>> btw, it builds fine for me even without --kerneldir, just tested.
>>
>> and the<linux/kvm_para.h>  include is there since Feb08, a while now.
>
> Which doesn't imply "make headers" actually includes that file.  Even  
> rawhide aka soon-to-be-F12 ships no kvm_para.h header file in  
> kernel-headers.
>
> Just defining the msrs instead of using kvm_para.h should be fine, the  
> numbers will not change ...
That's not the problem. If one removes kvm_para.h, a lot of other things
will not build. For example, the capabilities list.
And it has been like this for almost a year now!

Without that header, your system should have never successfully built a
qemu-kvm instance. This is what happens here, if I try to remove it:

 error: ‘KVM_FEATURE_CLOCKSOURCE’ undeclared here (not in a function)
 error: ‘KVM_FEATURE_NOP_IO_DELAY’ undeclared here (not in a function)
 error: ‘KVM_FEATURE_MMU_OP’ undeclared here (not in a function)

So again, wth?
Gerd Hoffmann Oct. 22, 2009, 12:15 p.m. UTC | #8 
>> Just defining the msrs instead of using kvm_para.h should be fine, the
>> numbers will not change ...
> That's not the problem. If one removes kvm_para.h, a lot of other things
> will not build. For example, the capabilities list.
> And it has been like this for almost a year now!
>
> Without that header, your system should have never successfully built a
> qemu-kvm instance. This is what happens here, if I try to remove it:

qemu != qemu-kvm.

qemu-kvm ships with a copy of the kvm kernel header files.
upstream qemu doesn't.

cheers,
   Gerd
Glauber Costa Oct. 22, 2009, 12:21 p.m. UTC | #9 
On Thu, Oct 22, 2009 at 02:15:06PM +0200, Gerd Hoffmann wrote:
>>> Just defining the msrs instead of using kvm_para.h should be fine, the
>>> numbers will not change ...
>> That's not the problem. If one removes kvm_para.h, a lot of other things
>> will not build. For example, the capabilities list.
>> And it has been like this for almost a year now!
>>
>> Without that header, your system should have never successfully built a
>> qemu-kvm instance. This is what happens here, if I try to remove it:
>
> qemu != qemu-kvm.
>
> qemu-kvm ships with a copy of the kvm kernel header files.
> upstream qemu doesn't.
ouch.

so this is the source of the confusion. My bad!
Glauber Costa Oct. 22, 2009, 12:22 p.m. UTC | #10 
On Thu, Oct 22, 2009 at 02:15:06PM +0200, Gerd Hoffmann wrote:
>>> Just defining the msrs instead of using kvm_para.h should be fine, the
>>> numbers will not change ...
>> That's not the problem. If one removes kvm_para.h, a lot of other things
>> will not build. For example, the capabilities list.
>> And it has been like this for almost a year now!
>>
>> Without that header, your system should have never successfully built a
>> qemu-kvm instance. This is what happens here, if I try to remove it:
>
> qemu != qemu-kvm.
>
> qemu-kvm ships with a copy of the kvm kernel header files.
> upstream qemu doesn't.
btw: I did tested qemu.git itself, but then I was using --kerneldir.
Anthony Liguori Oct. 22, 2009, 2:08 p.m. UTC | #11 
Glauber Costa wrote:
> That's not the problem. If one removes kvm_para.h, a lot of other things
> will not build. For example, the capabilities list.
> And it has been like this for almost a year now!
>
> Without that header, your system should have never successfully built a
> qemu-kvm instance. This is what happens here, if I try to remove it:
>
>  error: ‘KVM_FEATURE_CLOCKSOURCE’ undeclared here (not in a function)
>  error: ‘KVM_FEATURE_NOP_IO_DELAY’ undeclared here (not in a function)
>  error: ‘KVM_FEATURE_MMU_OP’ undeclared here (not in a function)
>
> So again, wth?
>   

FWIW, I always build with --kerneldir.  I'd like to pull in KVM headers 
but every time we discuss it it seems to rat hole.
diff mbox

Patch

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5929d28..f589044 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -651,6 +651,8 @@  typedef struct CPUX86State {
     target_ulong fmask;
     target_ulong kernelgsbase;
 #endif
+    uint64_t system_time_msr;
+    uint64_t wall_clock_msr;
 
     uint64_t tsc;
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0e69b57..8a6fa29 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -18,6 +18,7 @@ 
 #include <sys/utsname.h>
 
 #include <linux/kvm.h>
+#include <linux/kvm_para.h>
 
 #include "qemu-common.h"
 #include "sysemu.h"
@@ -500,6 +501,9 @@  static int kvm_put_msrs(CPUState *env)
         kvm_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar);
     }
 #endif
+    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);
+
     msr_data.info.nmsrs = n;
 
     return kvm_vcpu_ioctl(env, KVM_SET_MSRS, &msr_data);
@@ -634,6 +638,9 @@  static int kvm_get_msrs(CPUState *env)
         msrs[n++].index = MSR_LSTAR;
     }
 #endif
+    msrs[n++].index = MSR_KVM_SYSTEM_TIME;
+    msrs[n++].index = MSR_KVM_WALL_CLOCK;
+
     msr_data.info.nmsrs = n;
     ret = kvm_vcpu_ioctl(env, KVM_GET_MSRS, &msr_data);
     if (ret < 0)
@@ -670,6 +677,12 @@  static int kvm_get_msrs(CPUState *env)
         case MSR_IA32_TSC:
             env->tsc = msrs[i].data;
             break;
+        case MSR_KVM_SYSTEM_TIME:
+            env->system_time_msr = msrs[i].data;
+            break;
+        case MSR_KVM_WALL_CLOCK:
+            env->wall_clock_msr = msrs[i].data;
+            break;
         }
     }
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index b13eff4..f6fe216 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -475,6 +475,9 @@  const VMStateDescription vmstate_cpu = {
         VMSTATE_UINT64_ARRAY_V(mce_banks, CPUState, MCE_BANKS_DEF *4, 10),
         /* rdtscp */
         VMSTATE_UINT64_V(tsc_aux, CPUState, 11),
+        /* KVM pvclock msr */
+        VMSTATE_UINT64_V(system_time_msr, CPUState, 11),
+        VMSTATE_UINT64_V(wall_clock_msr, CPUState, 11),
         VMSTATE_END_OF_LIST()
     }
 };