make tsc stable over migration and machine start

If the machine is stopped, we should not record two different tsc values
upon a save operation. The same problem happens with kvmclock.

But kvmclock is taking a different diretion, being now seen as a separate
device. Since this is unlikely to happen with the tsc, I am taking the
approach here of simply registering a handler for state change, and
using a per-CPUState variable that prevents double updates for the TSC.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 target-i386/cpu.h |    1 +
 target-i386/kvm.c |   19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletions(-)

On 2011-02-01 20:17, Glauber Costa wrote:
> If the machine is stopped, we should not record two different tsc values
> upon a save operation. The same problem happens with kvmclock.
> 
> But kvmclock is taking a different diretion, being now seen as a separate
> device. Since this is unlikely to happen with the tsc, I am taking the
> approach here of simply registering a handler for state change, and
> using a per-CPUState variable that prevents double updates for the TSC.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  target-i386/cpu.h |    1 +
>  target-i386/kvm.c |   19 ++++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 6d619e8..7f1c4f8 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -732,6 +732,7 @@ typedef struct CPUX86State {
>      uint32_t sipi_vector;
>      uint32_t cpuid_kvm_features;
>      uint32_t cpuid_svm_features;
> +    uint8_t  update_tsc;

bool please.

>      
>      /* in order to simplify APIC support, we leave this pointer to the
>         user */
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ecb8405..c3925be 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -302,6 +302,16 @@ void kvm_inject_x86_mce(CPUState *cenv, int bank, uint64_t status,
>  
>  static int _kvm_arch_init_vcpu(CPUState *env);
>  
> +static void cpu_update_state(void *opaque, int running, int reason)
> +{
> +    CPUState *env = opaque;
> +
> +    if (!running) {
> +        env->update_tsc = 1;
> +    }
> +}
> +
> +

Additional blank line.

>  int kvm_arch_init_vcpu(CPUState *env)
>  {
>      int r;
> @@ -444,6 +454,8 @@ int kvm_arch_init_vcpu(CPUState *env)
>      }
>  #endif
>  
> +    qemu_add_vm_change_state_handler(cpu_update_state, env);
> +
>      return kvm_vcpu_ioctl(env, KVM_SET_CPUID2, &cpuid_data);
>  }
>  
> @@ -1093,7 +1105,12 @@ static int kvm_get_msrs(CPUState *env)
>  	msrs[n++].index = MSR_STAR;
>      if (kvm_has_msr_hsave_pa(env))
>          msrs[n++].index = MSR_VM_HSAVE_PA;
> -    msrs[n++].index = MSR_IA32_TSC;
> +
> +    if (env->update_tsc) {
> +        msrs[n++].index = MSR_IA32_TSC;
> +        env->update_tsc = 0;
> +    }
> +
>  #ifdef TARGET_X86_64
>      if (lm_capable_kernel) {
>          msrs[n++].index = MSR_CSTAR;

Not quite the logic I'm using for kvmclock:

cpu_update_state()
	if running
		tsc_valid = false;

kvm_get_msrs()
	...
	if (!tsc_valid)
		read_tsc
		tsc_valid = !vm_running;

That ensure we always read the tsc while the VM is running, and not only
after it was stopped (might otherwise be "surprising" when once
visualizing the MSRs).

Jan

On Tue, 2011-02-01 at 21:26 +0100, Jan Kiszka wrote:
> On 2011-02-01 20:17, Glauber Costa wrote:
> > If the machine is stopped, we should not record two different tsc values
> > upon a save operation. The same problem happens with kvmclock.
> > 
> > But kvmclock is taking a different diretion, being now seen as a separate
> > device. Since this is unlikely to happen with the tsc, I am taking the
> > approach here of simply registering a handler for state change, and
> > using a per-CPUState variable that prevents double updates for the TSC.
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  target-i386/cpu.h |    1 +
> >  target-i386/kvm.c |   19 ++++++++++++++++++-
> >  2 files changed, 19 insertions(+), 1 deletions(-)
> > 
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 6d619e8..7f1c4f8 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -732,6 +732,7 @@ typedef struct CPUX86State {
> >      uint32_t sipi_vector;
> >      uint32_t cpuid_kvm_features;
> >      uint32_t cpuid_svm_features;
> > +    uint8_t  update_tsc;
> 
> bool please.
> 
> >      
> >      /* in order to simplify APIC support, we leave this pointer to the
> >         user */
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index ecb8405..c3925be 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -302,6 +302,16 @@ void kvm_inject_x86_mce(CPUState *cenv, int bank, uint64_t status,
> >  
> >  static int _kvm_arch_init_vcpu(CPUState *env);
> >  
> > +static void cpu_update_state(void *opaque, int running, int reason)
> > +{
> > +    CPUState *env = opaque;
> > +
> > +    if (!running) {
> > +        env->update_tsc = 1;
> > +    }
> > +}
> > +
> > +
> 
> Additional blank line.
> 
> >  int kvm_arch_init_vcpu(CPUState *env)
> >  {
> >      int r;
> > @@ -444,6 +454,8 @@ int kvm_arch_init_vcpu(CPUState *env)
> >      }
> >  #endif
> >  
> > +    qemu_add_vm_change_state_handler(cpu_update_state, env);
> > +
> >      return kvm_vcpu_ioctl(env, KVM_SET_CPUID2, &cpuid_data);
> >  }
> >  
> > @@ -1093,7 +1105,12 @@ static int kvm_get_msrs(CPUState *env)
> >  	msrs[n++].index = MSR_STAR;
> >      if (kvm_has_msr_hsave_pa(env))
> >          msrs[n++].index = MSR_VM_HSAVE_PA;
> > -    msrs[n++].index = MSR_IA32_TSC;
> > +
> > +    if (env->update_tsc) {
> > +        msrs[n++].index = MSR_IA32_TSC;
> > +        env->update_tsc = 0;
> > +    }
> > +
> >  #ifdef TARGET_X86_64
> >      if (lm_capable_kernel) {
> >          msrs[n++].index = MSR_CSTAR;
> 
> Not quite the logic I'm using for kvmclock:

Ok. I have all the interest in keeping the same logic.
I will respin.

> cpu_update_state()
> 	if running
> 		tsc_valid = false;
> 
> kvm_get_msrs()
> 	...
> 	if (!tsc_valid)
> 		read_tsc
> 		tsc_valid = !vm_running;
> 
> That ensure we always read the tsc while the VM is running, and not only
> after it was stopped (might otherwise be "surprising" when once
> visualizing the MSRs).
> 
> Jan
>