[Qemu-devel] [PATCH 0/3] target-i386: save/restore vcpu's TSC rate during migration

[Qemu-devel] [PATCH 0/3] target-i386: save/restore vcpu's TSC rate during migration

[Haozhong Zhang]

This patchset enables QEMU to save/restore vcpu's TSC rate during the
migration. When cooperating with KVM which supports TSC scaling, guest
programs can observe a consistent guest TSC rate even though they are
migrated among machines with different host TSC rates.

Haozhong Zhang (3):
  target-i386: add a subsection of vcpu's TSC rate in vmstate_x86_cpu
  target-i386: initialize vcpu's TSC rate to the value from KVM
  kvm-all: notice KVM of vcpu's TSC rate after migration

 kvm-all.c             | 13 +++++++++++++
 target-i386/kvm.c     |  7 +++++++
 target-i386/machine.c | 20 ++++++++++++++++++++
 3 files changed, 40 insertions(+)

--
2.4.8

[Qemu-devel] [PATCH 1/3] target-i386: add a subsection of vcpu's TSC rate in vmstate_x86_cpu

[Haozhong Zhang]

The newly added subsection 'vmstate_tsc_khz' in this patch results in
vcpu's TSC rate being saved on the source machine and loaded on the
target machine during the migration.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/machine.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 9fa0563..80108a3 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -752,6 +752,25 @@ static const VMStateDescription vmstate_xss = {
     }
 };
 
+static bool tsc_khz_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+
+    return env->tsc_khz != 0;
+}
+
+static const VMStateDescription vmstate_tsc_khz = {
+    .name = "cpu/tsc_khz",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = tsc_khz_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(env.tsc_khz, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -871,6 +890,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_crash,
         &vmstate_avx512,
         &vmstate_xss,
+        &vmstate_tsc_khz,
         NULL
     }
 };
-- 
2.4.8

Re: [Qemu-devel] [PATCH 1/3] target-i386: add a subsection of vcpu's TSC rate in vmstate_x86_cpu

[Dr. David Alan Gilbert]

* Haozhong Zhang (haozhong.zhang@intel.com) wrote:
> The newly added subsection 'vmstate_tsc_khz' in this patch results in
> vcpu's TSC rate being saved on the source machine and loaded on the
> target machine during the migration.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Hi,
  I'd appreciate it if you could tie this to only do it on newer
machine types; that way it won't break back migration.

Dave

> ---
>  target-i386/machine.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 9fa0563..80108a3 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -752,6 +752,25 @@ static const VMStateDescription vmstate_xss = {
>      }
>  };
>  
> +static bool tsc_khz_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    CPUX86State *env = &cpu->env;
> +
> +    return env->tsc_khz != 0;
> +}
> +
> +static const VMStateDescription vmstate_tsc_khz = {
> +    .name = "cpu/tsc_khz",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = tsc_khz_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
> @@ -871,6 +890,7 @@ VMStateDescription vmstate_x86_cpu = {
>          &vmstate_msr_hyperv_crash,
>          &vmstate_avx512,
>          &vmstate_xss,
> +        &vmstate_tsc_khz,
>          NULL
>      }
>  };
> -- 
> 2.4.8
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] target-i386: add a subsection of vcpu's TSC rate in vmstate_x86_cpu

[Haozhong Zhang]

On Tue, Sep 29, 2015 at 08:00:13PM +0100, Dr. David Alan Gilbert wrote:
> * Haozhong Zhang (haozhong.zhang@intel.com) wrote:
> > The newly added subsection 'vmstate_tsc_khz' in this patch results in
> > vcpu's TSC rate being saved on the source machine and loaded on the
> > target machine during the migration.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> Hi,
>   I'd appreciate it if you could tie this to only do it on newer
> machine types; that way it won't break back migration.
>

Does "back migration" mean migrating from QEMU w/ vmstate_tsc_khz
subsection to QEMU w/o that subsection?

- Haozhong

> Dave
> 
> > ---
> >  target-i386/machine.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index 9fa0563..80108a3 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -752,6 +752,25 @@ static const VMStateDescription vmstate_xss = {
> >      }
> >  };
> >  
> > +static bool tsc_khz_needed(void *opaque)
> > +{
> > +    X86CPU *cpu = opaque;
> > +    CPUX86State *env = &cpu->env;
> > +
> > +    return env->tsc_khz != 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_tsc_khz = {
> > +    .name = "cpu/tsc_khz",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = tsc_khz_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  VMStateDescription vmstate_x86_cpu = {
> >      .name = "cpu",
> >      .version_id = 12,
> > @@ -871,6 +890,7 @@ VMStateDescription vmstate_x86_cpu = {
> >          &vmstate_msr_hyperv_crash,
> >          &vmstate_avx512,
> >          &vmstate_xss,
> > +        &vmstate_tsc_khz,
> >          NULL
> >      }
> >  };
> > -- 
> > 2.4.8
> > 
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] target-i386: add a subsection of vcpu's TSC rate in vmstate_x86_cpu

[Dr. David Alan Gilbert]

* Haozhong Zhang (haozhong.zhang@intel.com) wrote:
> On Tue, Sep 29, 2015 at 08:00:13PM +0100, Dr. David Alan Gilbert wrote:
> > * Haozhong Zhang (haozhong.zhang@intel.com) wrote:
> > > The newly added subsection 'vmstate_tsc_khz' in this patch results in
> > > vcpu's TSC rate being saved on the source machine and loaded on the
> > > target machine during the migration.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > 
> > Hi,
> >   I'd appreciate it if you could tie this to only do it on newer
> > machine types; that way it won't break back migration.
> >
> 
> Does "back migration" mean migrating from QEMU w/ vmstate_tsc_khz
> subsection to QEMU w/o that subsection?

Yes; like if we migrate from a newer qemu to an older qemu but with
the same machine type.

Dave

> 
> - Haozhong
> 
> > Dave
> > 
> > > ---
> > >  target-i386/machine.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > > index 9fa0563..80108a3 100644
> > > --- a/target-i386/machine.c
> > > +++ b/target-i386/machine.c
> > > @@ -752,6 +752,25 @@ static const VMStateDescription vmstate_xss = {
> > >      }
> > >  };
> > >  
> > > +static bool tsc_khz_needed(void *opaque)
> > > +{
> > > +    X86CPU *cpu = opaque;
> > > +    CPUX86State *env = &cpu->env;
> > > +
> > > +    return env->tsc_khz != 0;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_tsc_khz = {
> > > +    .name = "cpu/tsc_khz",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = tsc_khz_needed,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > >  VMStateDescription vmstate_x86_cpu = {
> > >      .name = "cpu",
> > >      .version_id = 12,
> > > @@ -871,6 +890,7 @@ VMStateDescription vmstate_x86_cpu = {
> > >          &vmstate_msr_hyperv_crash,
> > >          &vmstate_avx512,
> > >          &vmstate_xss,
> > > +        &vmstate_tsc_khz,
> > >          NULL
> > >      }
> > >  };
> > > -- 
> > > 2.4.8
> > > 
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] target-i386: add a subsection of vcpu's TSC rate in vmstate_x86_cpu

[Haozhong Zhang]

On Wed, Sep 30, 2015 at 09:07:08AM +0100, Dr. David Alan Gilbert wrote:
> * Haozhong Zhang (haozhong.zhang@intel.com) wrote:
> > On Tue, Sep 29, 2015 at 08:00:13PM +0100, Dr. David Alan Gilbert wrote:
> > > * Haozhong Zhang (haozhong.zhang@intel.com) wrote:
> > > > The newly added subsection 'vmstate_tsc_khz' in this patch results in
> > > > vcpu's TSC rate being saved on the source machine and loaded on the
> > > > target machine during the migration.
> > > > 
> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > 
> > > Hi,
> > >   I'd appreciate it if you could tie this to only do it on newer
> > > machine types; that way it won't break back migration.
> > >
> > 
> > Does "back migration" mean migrating from QEMU w/ vmstate_tsc_khz
> > subsection to QEMU w/o that subsection?
> 
> Yes; like if we migrate from a newer qemu to an older qemu but with
> the same machine type.
>

This patch does break the back migration. I'll fix this in the next version.

- Haozhong

> Dave
> 
> > 
> > - Haozhong
> > 
> > > Dave
> > > 
> > > > ---
> > > >  target-i386/machine.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > > 
> > > > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > > > index 9fa0563..80108a3 100644
> > > > --- a/target-i386/machine.c
> > > > +++ b/target-i386/machine.c
> > > > @@ -752,6 +752,25 @@ static const VMStateDescription vmstate_xss = {
> > > >      }
> > > >  };
> > > >  
> > > > +static bool tsc_khz_needed(void *opaque)
> > > > +{
> > > > +    X86CPU *cpu = opaque;
> > > > +    CPUX86State *env = &cpu->env;
> > > > +
> > > > +    return env->tsc_khz != 0;
> > > > +}
> > > > +
> > > > +static const VMStateDescription vmstate_tsc_khz = {
> > > > +    .name = "cpu/tsc_khz",
> > > > +    .version_id = 1,
> > > > +    .minimum_version_id = 1,
> > > > +    .needed = tsc_khz_needed,
> > > > +    .fields = (VMStateField[]) {
> > > > +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> > > > +        VMSTATE_END_OF_LIST()
> > > > +    }
> > > > +};
> > > > +
> > > >  VMStateDescription vmstate_x86_cpu = {
> > > >      .name = "cpu",
> > > >      .version_id = 12,
> > > > @@ -871,6 +890,7 @@ VMStateDescription vmstate_x86_cpu = {
> > > >          &vmstate_msr_hyperv_crash,
> > > >          &vmstate_avx512,
> > > >          &vmstate_xss,
> > > > +        &vmstate_tsc_khz,
> > > >          NULL
> > > >      }
> > > >  };
> > > > -- 
> > > > 2.4.8
> > > > 
> > > > 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Qemu-devel] [PATCH 2/3] target-i386: initialize vcpu's TSC rate to the value from KVM

[Haozhong Zhang]

When creating a vcpu, we initialize its TSC rate to the value from
KVM (through ioctl KVM_GET_TSC_KHZ).

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/kvm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 7b0ba17..c2b161a 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -751,6 +751,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
         }
     }
 
+    r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
+    if (r < 0) {
+        fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
+        return r;
+    }
+    env->tsc_khz = r;
+
     if (kvm_has_xsave()) {
         env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
     }
-- 
2.4.8

Re: [Qemu-devel] [PATCH 2/3] target-i386: initialize vcpu's TSC rate to the value from KVM

[Eduardo Habkost]

On Mon, Sep 28, 2015 at 01:38:30PM +0800, Haozhong Zhang wrote:
> When creating a vcpu, we initialize its TSC rate to the value from
> KVM (through ioctl KVM_GET_TSC_KHZ).
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/kvm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 7b0ba17..c2b161a 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -751,6 +751,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          }
>      }
>  
> +    r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> +    if (r < 0) {
> +        fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> +        return r;
> +    }
> +    env->tsc_khz = r;

You are silently overwriting the tsc_khz value set by the user, why?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] target-i386: initialize vcpu's TSC rate to the value from KVM

[Haozhong Zhang]

On Mon, Sep 28, 2015 at 01:17:44PM -0300, Eduardo Habkost wrote:
> On Mon, Sep 28, 2015 at 01:38:30PM +0800, Haozhong Zhang wrote:
> > When creating a vcpu, we initialize its TSC rate to the value from
> > KVM (through ioctl KVM_GET_TSC_KHZ).
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/kvm.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 7b0ba17..c2b161a 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -751,6 +751,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          }
> >      }
> >  
> > +    r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > +    if (r < 0) {
> > +        fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > +        return r;
> > +    }
> > +    env->tsc_khz = r;
> 
> You are silently overwriting the tsc_khz value set by the user, why?
>

Oh, I need to check if user has provided tsc_khz, and if so then just
use the user-provided value. So I'll replace it with code like

if (env->tsc_khz) {
    kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
} else {
    r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
    if (r < 0) {
        fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
        return r;
    }
    env->tsc_khz = r;
}

- Haozhong

> -- 
> Eduardo

Re: [Qemu-devel] [PATCH 2/3] target-i386: initialize vcpu's TSC rate to the value from KVM

[Haozhong Zhang]

On Tue, Sep 29, 2015 at 09:23:39AM +0800, Haozhong Zhang wrote:
> On Mon, Sep 28, 2015 at 01:17:44PM -0300, Eduardo Habkost wrote:
> > On Mon, Sep 28, 2015 at 01:38:30PM +0800, Haozhong Zhang wrote:
> > > When creating a vcpu, we initialize its TSC rate to the value from
> > > KVM (through ioctl KVM_GET_TSC_KHZ).
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > ---
> > >  target-i386/kvm.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 7b0ba17..c2b161a 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -751,6 +751,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > >          }
> > >      }
> > >  
> > > +    r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > +    if (r < 0) {
> > > +        fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > > +        return r;
> > > +    }
> > > +    env->tsc_khz = r;
> > 
> > You are silently overwriting the tsc_khz value set by the user, why?
> >
> 
> Oh, I need to check if user has provided tsc_khz, and if so then just
> use the user-provided value. So I'll replace it with code like
> 
> if (env->tsc_khz) {
>     kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);

Just notice this line duplicates code several lines above. Only the
else branch is needed.

> } else {
>     r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
>     if (r < 0) {
>         fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
>         return r;
>     }
>     env->tsc_khz = r;
> }
> 
> - Haozhong
> 
> > -- 
> > Eduardo
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
- Haozhong Zhang

[Qemu-devel] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration

[Haozhong Zhang]

When a vcpu is created in KVM, its TSC rate is initially identical to
the host TSC rate. If its state is migrated to a vcpu on another
machine (target machine) which may uses a different host TSC rate, QEMU
on the target machine should notice KVM of the migrated vcpu's TSC
rate. In case that KVM on the target machine supports TSC scaling, guest
programs running on the migrated vcpu will observe the same TSC rate
before and after the migration.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 kvm-all.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/kvm-all.c b/kvm-all.c
index 0be4615..e8de038 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1769,6 +1769,19 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
 static void do_kvm_cpu_synchronize_post_init(void *arg)
 {
     CPUState *cpu = arg;
+    CPUX86State *env = &X86_CPU(cpu)->env;
+    int r;
+
+    /*
+     * XXX: KVM_SET_TSC_KHZ must be done before kvm_arch_put_registers().
+     */
+    r = kvm_check_extension(cpu->kvm_state, KVM_CAP_TSC_CONTROL);
+    if (r && env->tsc_khz) {
+        r = kvm_vcpu_ioctl(cpu, KVM_SET_TSC_KHZ, env->tsc_khz);
+        if (r < 0) {
+            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
+        }
+    }
 
     kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
     cpu->kvm_vcpu_dirty = false;
-- 
2.4.8

Re: [Qemu-devel] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration

[Eduardo Habkost]

On Mon, Sep 28, 2015 at 01:38:31PM +0800, Haozhong Zhang wrote:
> When a vcpu is created in KVM, its TSC rate is initially identical to
> the host TSC rate. If its state is migrated to a vcpu on another
> machine (target machine) which may uses a different host TSC rate, QEMU
> on the target machine should notice KVM of the migrated vcpu's TSC
> rate. In case that KVM on the target machine supports TSC scaling, guest
> programs running on the migrated vcpu will observe the same TSC rate
> before and after the migration.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  kvm-all.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 0be4615..e8de038 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1769,6 +1769,19 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
>  static void do_kvm_cpu_synchronize_post_init(void *arg)
>  {
>      CPUState *cpu = arg;
> +    CPUX86State *env = &X86_CPU(cpu)->env;
> +    int r;
> +
> +    /*
> +     * XXX: KVM_SET_TSC_KHZ must be done before kvm_arch_put_registers().

Could you explain where this requirement comes from?

> +     */
> +    r = kvm_check_extension(cpu->kvm_state, KVM_CAP_TSC_CONTROL);
> +    if (r && env->tsc_khz) {
> +        r = kvm_vcpu_ioctl(cpu, KVM_SET_TSC_KHZ, env->tsc_khz);
> +        if (r < 0) {
> +            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> +        }
> +    }

This is duplicating the existing KVM_SET_TSC_KHZ call at
kvm_arch_init_vcpu(). I wonder if there's a way to avoid this
duplication. Should we set TSC KHz only at
do_kvm_cpu_synchronize_post_init(), and remove the call from
kvm_arch_init_vcpu()?

Or maybe we shouldn't treat this as VM state, but as configuration, and
let management configure the TSC frequency explicitly if the user really
needs it to stay the same during migration.

(CCing libvir-list to see if they have feedback)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration

[Haozhong Zhang]

On Mon, Sep 28, 2015 at 01:37:34PM -0300, Eduardo Habkost wrote:
> On Mon, Sep 28, 2015 at 01:38:31PM +0800, Haozhong Zhang wrote:
> > When a vcpu is created in KVM, its TSC rate is initially identical to
> > the host TSC rate. If its state is migrated to a vcpu on another
> > machine (target machine) which may uses a different host TSC rate, QEMU
> > on the target machine should notice KVM of the migrated vcpu's TSC
> > rate. In case that KVM on the target machine supports TSC scaling, guest
> > programs running on the migrated vcpu will observe the same TSC rate
> > before and after the migration.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  kvm-all.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 0be4615..e8de038 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1769,6 +1769,19 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
> >  static void do_kvm_cpu_synchronize_post_init(void *arg)
> >  {
> >      CPUState *cpu = arg;
> > +    CPUX86State *env = &X86_CPU(cpu)->env;
> > +    int r;
> > +
> > +    /*
> > +     * XXX: KVM_SET_TSC_KHZ must be done before kvm_arch_put_registers().
> 
> Could you explain where this requirement comes from?
>

kvm_arch_put_registers() below will setup vcpu's MSR_IA32_TSC through
KVM ioctl KVM_SET_MSRS. KVM needs to know vcpu's TSC rate so as to
correctly scale the TSC value given by QEMU, especially when vcpu's
TSC rate is different than the host TSC rate (e.g. it's migrated from
another machine w/ different host TSC rate than the current one).

> > +     */
> > +    r = kvm_check_extension(cpu->kvm_state, KVM_CAP_TSC_CONTROL);
> > +    if (r && env->tsc_khz) {
> > +        r = kvm_vcpu_ioctl(cpu, KVM_SET_TSC_KHZ, env->tsc_khz);
> > +        if (r < 0) {
> > +            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > +        }
> > +    }
> 
> This is duplicating the existing KVM_SET_TSC_KHZ call at
> kvm_arch_init_vcpu(). I wonder if there's a way to avoid this
> duplication. Should we set TSC KHz only at
> do_kvm_cpu_synchronize_post_init(), and remove the call from
> kvm_arch_init_vcpu()?
>

I'll check if it's safe to remove the call from kvm_arch_init_vcpu().

> Or maybe we shouldn't treat this as VM state, but as configuration, and
> let management configure the TSC frequency explicitly if the user really
> needs it to stay the same during migration.
>
> (CCing libvir-list to see if they have feedback)
>

Thanks for CC. I'll consider to add a command line option to control
the configuration of guest TSC fequency.

> -- 
> Eduardo

Re: [Qemu-devel] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration

[Eduardo Habkost]

On Tue, Sep 29, 2015 at 11:43:34AM +0800, Haozhong Zhang wrote:
> On Mon, Sep 28, 2015 at 01:37:34PM -0300, Eduardo Habkost wrote:
> > On Mon, Sep 28, 2015 at 01:38:31PM +0800, Haozhong Zhang wrote:
[...]
> > >  static void do_kvm_cpu_synchronize_post_init(void *arg)
> > >  {
> > >      CPUState *cpu = arg;
> > > +    CPUX86State *env = &X86_CPU(cpu)->env;
> > > +    int r;
> > > +
> > > +    /*
> > > +     * XXX: KVM_SET_TSC_KHZ must be done before kvm_arch_put_registers().
> > 
> > Could you explain where this requirement comes from?
> >
> 
> kvm_arch_put_registers() below will setup vcpu's MSR_IA32_TSC through
> KVM ioctl KVM_SET_MSRS. KVM needs to know vcpu's TSC rate so as to
> correctly scale the TSC value given by QEMU, especially when vcpu's
> TSC rate is different than the host TSC rate (e.g. it's migrated from
> another machine w/ different host TSC rate than the current one).

Thanks. The comment above could contain a short version of this
explanation, e.g.: "KVM needs KVM_SET_TSC_KHZ to be done before
KVM_SET_MSRS sets MSR_IA32_TSC (done by kvm_arch_put_registers())".

> 
[...]
> > Or maybe we shouldn't treat this as VM state, but as configuration, and
> > let management configure the TSC frequency explicitly if the user really
> > needs it to stay the same during migration.
> >
> > (CCing libvir-list to see if they have feedback)
> >
> 
> Thanks for CC. I'll consider to add a command line option to control
> the configuration of guest TSC fequency.

It already exists, -cpu has a "tsc-freq" option.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration

[Haozhong Zhang]

On Tue, Sep 29, 2015 at 03:02:07PM -0300, Eduardo Habkost wrote:
> On Tue, Sep 29, 2015 at 11:43:34AM +0800, Haozhong Zhang wrote:
> > On Mon, Sep 28, 2015 at 01:37:34PM -0300, Eduardo Habkost wrote:
> > > On Mon, Sep 28, 2015 at 01:38:31PM +0800, Haozhong Zhang wrote:
> [...]
> > > >  static void do_kvm_cpu_synchronize_post_init(void *arg)
> > > >  {
> > > >      CPUState *cpu = arg;
> > > > +    CPUX86State *env = &X86_CPU(cpu)->env;
> > > > +    int r;
> > > > +
> > > > +    /*
> > > > +     * XXX: KVM_SET_TSC_KHZ must be done before kvm_arch_put_registers().
> > > 
> > > Could you explain where this requirement comes from?
> > >
> > 
> > kvm_arch_put_registers() below will setup vcpu's MSR_IA32_TSC through
> > KVM ioctl KVM_SET_MSRS. KVM needs to know vcpu's TSC rate so as to
> > correctly scale the TSC value given by QEMU, especially when vcpu's
> > TSC rate is different than the host TSC rate (e.g. it's migrated from
> > another machine w/ different host TSC rate than the current one).
> 
> Thanks. The comment above could contain a short version of this
> explanation, e.g.: "KVM needs KVM_SET_TSC_KHZ to be done before
> KVM_SET_MSRS sets MSR_IA32_TSC (done by kvm_arch_put_registers())".
>

will include this in the comment

> > 
> [...]
> > > Or maybe we shouldn't treat this as VM state, but as configuration, and
> > > let management configure the TSC frequency explicitly if the user really
> > > needs it to stay the same during migration.
> > >
> > > (CCing libvir-list to see if they have feedback)
> > >
> > 
> > Thanks for CC. I'll consider to add a command line option to control
> > the configuration of guest TSC fequency.
> 
> It already exists, -cpu has a "tsc-freq" option.
>

What I'm considering is to add a "-keep-tsc-freq" option to control
whether the TSC frequency should be migrated if "tsc-freq" is not
presented. Compared to "tsc-freq", "-keep-tsc-freq" can free users
from figuring out the guest TSC frequency manually in the migration.

- Haozhong

> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration

[Eduardo Habkost]

On Wed, Sep 30, 2015 at 08:32:26AM +0800, Haozhong Zhang wrote:
> > [...]
> > > > Or maybe we shouldn't treat this as VM state, but as configuration, and
> > > > let management configure the TSC frequency explicitly if the user really
> > > > needs it to stay the same during migration.
> > > >
> > > > (CCing libvir-list to see if they have feedback)
> > > >
> > > 
> > > Thanks for CC. I'll consider to add a command line option to control
> > > the configuration of guest TSC fequency.
> > 
> > It already exists, -cpu has a "tsc-freq" option.
> >
> 
> What I'm considering is to add a "-keep-tsc-freq" option to control
> whether the TSC frequency should be migrated if "tsc-freq" is not
> presented. Compared to "tsc-freq", "-keep-tsc-freq" can free users
> from figuring out the guest TSC frequency manually in the migration.

If you do that, please make it a property of the CPU object, so it will
can be set as a "-cpu" option.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration

[Haozhong Zhang]

On Wed, Sep 30, 2015 at 05:36:11PM -0300, Eduardo Habkost wrote:
> On Wed, Sep 30, 2015 at 08:32:26AM +0800, Haozhong Zhang wrote:
> > > [...]
> > > > > Or maybe we shouldn't treat this as VM state, but as configuration, and
> > > > > let management configure the TSC frequency explicitly if the user really
> > > > > needs it to stay the same during migration.
> > > > >
> > > > > (CCing libvir-list to see if they have feedback)
> > > > >
> > > > 
> > > > Thanks for CC. I'll consider to add a command line option to control
> > > > the configuration of guest TSC fequency.
> > > 
> > > It already exists, -cpu has a "tsc-freq" option.
> > >
> > 
> > What I'm considering is to add a "-keep-tsc-freq" option to control
> > whether the TSC frequency should be migrated if "tsc-freq" is not
> > presented. Compared to "tsc-freq", "-keep-tsc-freq" can free users
> > from figuring out the guest TSC frequency manually in the migration.
> 
> If you do that, please make it a property of the CPU object, so it will
> can be set as a "-cpu" option.
>

Yes, I'll do so.

- Haozhong

> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html