[Qemu-devel] [PATCH] properly save kvm system time msr registers

[Qemu-devel] [PATCH] properly save kvm system time msr registers

[Glauber Costa]

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

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()
     }
 };
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Glauber Costa]

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...

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Glauber Costa]

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.

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Glauber Costa]

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...

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Glauber Costa]

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?

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Glauber Costa]

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!

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Glauber Costa]

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.

Re: [Qemu-devel] [PATCH] properly save kvm system time msr registers

[Anthony Liguori]

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.

-- 
Regards,

Anthony Liguori