Re: [Qemu-devel] [PATCH v4 02/18] target-i386: Simplify reporting of unavailable features

Re: [Qemu-devel] [PATCH v4 02/18] target-i386: Simplify reporting of unavailable features

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> Instead of checking and calling unavailable_host_feature() once for each
> bit, simply call the function (now renamed to
> report_unavailable_features()) once for each feature word.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  * Rebase to latest qom-cpu (commit 90c5d39c)
> Changes v2 -> v3:
>  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
> ---
>  target-i386/cpu.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7ec706f..9cd0039 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1236,11 +1236,11 @@ static const TypeInfo host_x86_cpu_type_info = {
>  
>  #endif
>  
> -static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
> +static int report_unavailable_features(FeatureWordInfo *f, uint32_t mask)
>  {
>      int i;
>  
> -    for (i = 0; i < 32; ++i)
> +    for (i = 0; i < 32; ++i) {
>          if (1 << i & mask) {
>              const char *reg = get_register_name_32(f->cpuid_reg);
>              assert(reg);
> @@ -1249,8 +1249,8 @@ static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
>                  f->cpuid_eax, reg,
>                  f->feat_names[i] ? "." : "",
>                  f->feat_names[i] ? f->feat_names[i] : "", i);
> -            break;
>          }
> +    }
>      return 0;
>  }
>  
> @@ -1274,12 +1274,10 @@ static int kvm_check_features_against_host(KVMState *s, X86CPU *cpu)
>          uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
>                                                               wi->cpuid_ecx,
>                                                               wi->cpuid_reg);
> -        uint32_t mask;
> -        for (mask = 1; mask; mask <<= 1) {
> -            if (guest_feat & mask && !(host_feat & mask)) {
> -                unavailable_host_feature(wi, mask);
> -                rv = 1;
> -            }
> +        uint32_t unavailable_features = guest_feat & ~host_feat;
> +        if (unavailable_features) {
> +            report_unavailable_features(wi, unavailable_features);
> +            rv = 1;
>          }
>      }
>      return rv;

Both before and after your patch the return value is never checked.
Shall we/I change it to void?

Regards,
Andreas

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3c4f327..d095c7d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1236,7 +1236,7 @@ static const TypeInfo host_x86_cpu_type_info = {

 #endif

-static int report_unavailable_features(FeatureWordInfo *f, uint32_t mask)
+static void report_unavailable_features(FeatureWordInfo *f, uint32_t mask)
 {
     int i;

@@ -1251,7 +1251,6 @@ static int
report_unavailable_features(FeatureWordInfo *f, uint32_t mask)
                 f->feat_names[i] ? f->feat_names[i] : "", i);
         }
     }
-    return 0;
 }

 /* Check if all requested cpu flags are making their way to the guest

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 02/18] target-i386: Simplify reporting of unavailable features

[Eduardo Habkost]

On Thu, May 15, 2014 at 02:21:12PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > Instead of checking and calling unavailable_host_feature() once for each
> > bit, simply call the function (now renamed to
> > report_unavailable_features()) once for each feature word.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> >  * Rebase to latest qom-cpu (commit 90c5d39c)
> > Changes v2 -> v3:
> >  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
> > ---
> >  target-i386/cpu.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7ec706f..9cd0039 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1236,11 +1236,11 @@ static const TypeInfo host_x86_cpu_type_info = {
> >  
> >  #endif
> >  
> > -static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
> > +static int report_unavailable_features(FeatureWordInfo *f, uint32_t mask)
> >  {
> >      int i;
> >  
> > -    for (i = 0; i < 32; ++i)
> > +    for (i = 0; i < 32; ++i) {
> >          if (1 << i & mask) {
> >              const char *reg = get_register_name_32(f->cpuid_reg);
> >              assert(reg);
> > @@ -1249,8 +1249,8 @@ static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
> >                  f->cpuid_eax, reg,
> >                  f->feat_names[i] ? "." : "",
> >                  f->feat_names[i] ? f->feat_names[i] : "", i);
> > -            break;
> >          }
> > +    }
> >      return 0;
> >  }
> >  
> > @@ -1274,12 +1274,10 @@ static int kvm_check_features_against_host(KVMState *s, X86CPU *cpu)
> >          uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
> >                                                               wi->cpuid_ecx,
> >                                                               wi->cpuid_reg);
> > -        uint32_t mask;
> > -        for (mask = 1; mask; mask <<= 1) {
> > -            if (guest_feat & mask && !(host_feat & mask)) {
> > -                unavailable_host_feature(wi, mask);
> > -                rv = 1;
> > -            }
> > +        uint32_t unavailable_features = guest_feat & ~host_feat;
> > +        if (unavailable_features) {
> > +            report_unavailable_features(wi, unavailable_features);
> > +            rv = 1;
> >          }
> >      }
> >      return rv;
> 
> Both before and after your patch the return value is never checked.
> Shall we/I change it to void?

I will change this on my tree, but feel free to change my patch before
applying, if you prefer.

Thanks,

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 02/18] target-i386: Simplify reporting of unavailable features

[Andreas Färber]

Am 15.05.2014 15:39, schrieb Eduardo Habkost:
> On Thu, May 15, 2014 at 02:21:12PM +0200, Andreas Färber wrote:
>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>>> Instead of checking and calling unavailable_host_feature() once for each
>>> bit, simply call the function (now renamed to
>>> report_unavailable_features()) once for each feature word.
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>> Changes v1 -> v2:
>>>  * Rebase to latest qom-cpu (commit 90c5d39c)
>>> Changes v2 -> v3:
>>>  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
>>> ---
>>>  target-i386/cpu.c | 16 +++++++---------
>>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index 7ec706f..9cd0039 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -1236,11 +1236,11 @@ static const TypeInfo host_x86_cpu_type_info = {
>>>  
>>>  #endif
>>>  
>>> -static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
>>> +static int report_unavailable_features(FeatureWordInfo *f, uint32_t mask)
>>>  {
>>>      int i;
>>>  
>>> -    for (i = 0; i < 32; ++i)
>>> +    for (i = 0; i < 32; ++i) {
>>>          if (1 << i & mask) {
>>>              const char *reg = get_register_name_32(f->cpuid_reg);
>>>              assert(reg);
>>> @@ -1249,8 +1249,8 @@ static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
>>>                  f->cpuid_eax, reg,
>>>                  f->feat_names[i] ? "." : "",
>>>                  f->feat_names[i] ? f->feat_names[i] : "", i);
>>> -            break;
>>>          }
>>> +    }
>>>      return 0;
>>>  }
>>>  
>>> @@ -1274,12 +1274,10 @@ static int kvm_check_features_against_host(KVMState *s, X86CPU *cpu)
>>>          uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
>>>                                                               wi->cpuid_ecx,
>>>                                                               wi->cpuid_reg);
>>> -        uint32_t mask;
>>> -        for (mask = 1; mask; mask <<= 1) {
>>> -            if (guest_feat & mask && !(host_feat & mask)) {
>>> -                unavailable_host_feature(wi, mask);
>>> -                rv = 1;
>>> -            }
>>> +        uint32_t unavailable_features = guest_feat & ~host_feat;
>>> +        if (unavailable_features) {
>>> +            report_unavailable_features(wi, unavailable_features);
>>> +            rv = 1;
>>>          }
>>>      }
>>>      return rv;
>>
>> Both before and after your patch the return value is never checked.
>> Shall we/I change it to void?
> 
> I will change this on my tree, but feel free to change my patch before
> applying, if you prefer.

I had inserted a patch and will squash it then, no need to resend.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 03/18] target-i386: Merge feature filtering/checking functions

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> Merge filter_features_for_kvm() and kvm_check_features_against_host().
> 
> Both functions made exactly the same calculations, the only difference
> was that filter_features_for_kvm() changed the bits on cpu->features[],
> and kvm_check_features_against_host() did error reporting.
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Applying minor style tweak towards gtk-doc:

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6302a5c..f022ef3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1819,11 +1819,12 @@ CpuDefinitionInfoList
*arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }

-/* Filters CPU feature words based on host availability of each feature
- *
- * Returns 0 if all flags are supported by the host, non-zero otherwise.
+/*
+ * Filters CPU feature words based on host availability of each feature.
  *
  * This function may be called only if KVM is enabled.
+ *
+ * Returns: 0 if all flags are supported by the host, non-zero otherwise.
  */
 static int filter_features_for_kvm(X86CPU *cpu)
 {

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 07/18] target-i386: Filter FEAT_7_0_EBX TCG features too

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> The TCG_7_0_EBX_FEATURES macro was defined but never used (it even had a
> typo that was never noticed). Make the existing TCG feature filtering
> code use it.
> 
> Change the TCG feature flag filtering code to use it.

Sentence seems duplicate - which one to keep?

Should we CC this commit for -stable? (Affects -cpu Haswell probably?)
If not, should we make this conditional on the machine version?

One off-topic question below...

> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
>    (Reviewed-by line kept)
> Changes v2 -> v3:
>  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
>    (Reviewed-by line kept)
> ---
>  target-i386/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index bbac5fc..714d121 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -588,7 +588,7 @@ struct X86CPUDefinition {
>  #define TCG_EXT3_FEATURES (CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM | \
>            CPUID_EXT3_CR8LEG | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A)
>  #define TCG_SVM_FEATURES 0
> -#define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP \
> +#define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP | \
>            CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX)
>            /* missing:
>            CPUID_7_0_EBX_FSGSBASE, CPUID_7_0_EBX_HLE, CPUID_7_0_EBX_AVX2,
> @@ -2596,6 +2596,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (!kvm_enabled()) {

Is there a patch or should I follow-up with one to make TCG filtering
conditional to if (tcg_enabled())? (Xen, QTest)

Regards,
Andreas

>          env->features[FEAT_1_EDX] &= TCG_FEATURES;
>          env->features[FEAT_1_ECX] &= TCG_EXT_FEATURES;
> +        env->features[FEAT_7_0_EBX] &= TCG_7_0_EBX_FEATURES;
>          env->features[FEAT_8000_0001_EDX] &= TCG_EXT2_FEATURES;
>          env->features[FEAT_8000_0001_ECX] &= TCG_EXT3_FEATURES;
>          env->features[FEAT_SVM] &= TCG_SVM_FEATURES;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 07/18] target-i386: Filter FEAT_7_0_EBX TCG features too

[Eduardo Habkost]

On Thu, May 15, 2014 at 08:10:16PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > The TCG_7_0_EBX_FEATURES macro was defined but never used (it even had a
> > typo that was never noticed). Make the existing TCG feature filtering
> > code use it.
> > 
> > Change the TCG feature flag filtering code to use it.
> 
> Sentence seems duplicate - which one to keep?

Oops. Please removed the second paragraph (I removed it on the v4
resend).

> 
> Should we CC this commit for -stable? (Affects -cpu Haswell probably?)

It affects Haswell in a guest-visible way, yes. I don't know how well
guests behave when very recent CPU models run in TCG mode (having lots
of features removed), so I don't know if it makes sense for -stable or
not.

> If not, should we make this conditional on the machine version?

There would be no point, as there's no ABI stability guarantee in TCG
mode at all.

One day somebody may want to implement it, and it should be possible now
that we are making the enforce/check/filtered-features code work
properly with TCG. But it doesn't exist today.

> 
> One off-topic question below...
> 
> > 
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> >  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
> >    (Reviewed-by line kept)
> > Changes v2 -> v3:
> >  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
> >    (Reviewed-by line kept)
> > ---
> >  target-i386/cpu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index bbac5fc..714d121 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -588,7 +588,7 @@ struct X86CPUDefinition {
> >  #define TCG_EXT3_FEATURES (CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM | \
> >            CPUID_EXT3_CR8LEG | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A)
> >  #define TCG_SVM_FEATURES 0
> > -#define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP \
> > +#define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP | \
> >            CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX)
> >            /* missing:
> >            CPUID_7_0_EBX_FSGSBASE, CPUID_7_0_EBX_HLE, CPUID_7_0_EBX_AVX2,
> > @@ -2596,6 +2596,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >      if (!kvm_enabled()) {
> 
> Is there a patch or should I follow-up with one to make TCG filtering
> conditional to if (tcg_enabled())? (Xen, QTest)

There's no patch for that yet. I am not sure what would be appropriate
to do on those cases. Does it even make sense to set up any CPUID data
with Xen or QTest?

> 
> Regards,
> Andreas
> 
> >          env->features[FEAT_1_EDX] &= TCG_FEATURES;
> >          env->features[FEAT_1_ECX] &= TCG_EXT_FEATURES;
> > +        env->features[FEAT_7_0_EBX] &= TCG_7_0_EBX_FEATURES;
> >          env->features[FEAT_8000_0001_EDX] &= TCG_EXT2_FEATURES;
> >          env->features[FEAT_8000_0001_ECX] &= TCG_EXT3_FEATURES;
> >          env->features[FEAT_SVM] &= TCG_SVM_FEATURES;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 12/18] target-i386: Support check/enforce flags in TCG mode, too

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> If enforce/check is specified in TCG mode, QEMU will ensure all CPU
> features are supported by TCG, so no CPU feature is silently disabled.
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
>    (Reviewed-by line kept)
> Changes v2 -> v3:
>  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
>    (Reviewed-by line kept)
> ---
>  target-i386/cpu.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b2e30ca..53b5038 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1265,8 +1265,9 @@ static int report_unavailable_features(FeatureWord w, uint32_t mask)
>          if (1 << i & mask) {
>              const char *reg = get_register_name_32(f->cpuid_reg);
>              assert(reg);
> -            fprintf(stderr, "warning: host doesn't support requested feature: "
> +            fprintf(stderr, "warning: %s doesn't support requested feature: "
>                  "CPUID.%02XH:%s%s%s [bit %d]\n",
> +                kvm_enabled() ? "host" : "TCG",
>                  f->cpuid_eax, reg,
>                  f->feat_names[i] ? "." : "",
>                  f->feat_names[i] ? f->feat_names[i] : "", i);
> @@ -1826,17 +1827,18 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w)
>  {
>      FeatureWordInfo *wi = &feature_word_info[w];
> -    assert(kvm_enabled());
> -    return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> -                                                   wi->cpuid_ecx,
> -                                                   wi->cpuid_reg);
> +    if (kvm_enabled()) {
> +        return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> +                                                       wi->cpuid_ecx,
> +                                                       wi->cpuid_reg);
> +    } else {
> +        return wi->tcg_features;
> +    }
>  }

This function is called unconditionally now, so apply the following?

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 48ba1d8..112b437 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1839,8 +1839,10 @@ static uint32_t
x86_cpu_get_supported_feature_word(FeatureWord w)
         return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
                                                        wi->cpuid_ecx,
                                                        wi->cpuid_reg);
-    } else {
+    } else if (tcg_enabled()) {
         return wi->tcg_features;
+    } else {
+        return UINT32_MAX;
     }
 }


Not sure what to do about the warning message. It wouldn't occur though
due to the suggested mask, so we could just ignore it for now.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 12/18] target-i386: Support check/enforce flags in TCG mode, too

[Eduardo Habkost]

On Thu, May 15, 2014 at 08:54:15PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > If enforce/check is specified in TCG mode, QEMU will ensure all CPU
> > features are supported by TCG, so no CPU feature is silently disabled.
> > 
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> >  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
> >    (Reviewed-by line kept)
> > Changes v2 -> v3:
> >  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
> >    (Reviewed-by line kept)
> > ---
> >  target-i386/cpu.c | 34 ++++++++++++++++------------------
> >  1 file changed, 16 insertions(+), 18 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b2e30ca..53b5038 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1265,8 +1265,9 @@ static int report_unavailable_features(FeatureWord w, uint32_t mask)
> >          if (1 << i & mask) {
> >              const char *reg = get_register_name_32(f->cpuid_reg);
> >              assert(reg);
> > -            fprintf(stderr, "warning: host doesn't support requested feature: "
> > +            fprintf(stderr, "warning: %s doesn't support requested feature: "
> >                  "CPUID.%02XH:%s%s%s [bit %d]\n",
> > +                kvm_enabled() ? "host" : "TCG",
> >                  f->cpuid_eax, reg,
> >                  f->feat_names[i] ? "." : "",
> >                  f->feat_names[i] ? f->feat_names[i] : "", i);
> > @@ -1826,17 +1827,18 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
> >  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w)
> >  {
> >      FeatureWordInfo *wi = &feature_word_info[w];
> > -    assert(kvm_enabled());
> > -    return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> > -                                                   wi->cpuid_ecx,
> > -                                                   wi->cpuid_reg);
> > +    if (kvm_enabled()) {
> > +        return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> > +                                                       wi->cpuid_ecx,
> > +                                                       wi->cpuid_reg);
> > +    } else {
> > +        return wi->tcg_features;
> > +    }
> >  }
> 
> This function is called unconditionally now, so apply the following?
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 48ba1d8..112b437 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1839,8 +1839,10 @@ static uint32_t
> x86_cpu_get_supported_feature_word(FeatureWord w)
>          return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>                                                         wi->cpuid_ecx,
>                                                         wi->cpuid_reg);
> -    } else {
> +    } else if (tcg_enabled()) {
>          return wi->tcg_features;
> +    } else {
> +        return UINT32_MAX;

Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX.

>      }
>  }
> 
> 
> Not sure what to do about the warning message. It wouldn't occur though
> due to the suggested mask, so we could just ignore it for now.

One day we may be able to simply ask the machine object for the current
accelerator name. In the meantime, we could use:

  "warning: host (%s) doesn't support requested feature [...]",
  kvm_enabled() ? "KVM" : (tcg_enabled() ? "TCG" : "QEMU")

(But I won't object if you prefer to keep the warning message I
originally sent.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 12/18] target-i386: Support check/enforce flags in TCG mode, too

[Andreas Färber]

Am 15.05.2014 21:12, schrieb Eduardo Habkost:
> On Thu, May 15, 2014 at 08:54:15PM +0200, Andreas Färber wrote:
>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>>> If enforce/check is specified in TCG mode, QEMU will ensure all CPU
>>> features are supported by TCG, so no CPU feature is silently disabled.
>>>
>>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>> Changes v1 -> v2:
>>>  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
>>>    (Reviewed-by line kept)
>>> Changes v2 -> v3:
>>>  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
>>>    (Reviewed-by line kept)
>>> ---
>>>  target-i386/cpu.c | 34 ++++++++++++++++------------------
>>>  1 file changed, 16 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index b2e30ca..53b5038 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -1265,8 +1265,9 @@ static int report_unavailable_features(FeatureWord w, uint32_t mask)
>>>          if (1 << i & mask) {
>>>              const char *reg = get_register_name_32(f->cpuid_reg);
>>>              assert(reg);
>>> -            fprintf(stderr, "warning: host doesn't support requested feature: "
>>> +            fprintf(stderr, "warning: %s doesn't support requested feature: "
>>>                  "CPUID.%02XH:%s%s%s [bit %d]\n",
>>> +                kvm_enabled() ? "host" : "TCG",
>>>                  f->cpuid_eax, reg,
>>>                  f->feat_names[i] ? "." : "",
>>>                  f->feat_names[i] ? f->feat_names[i] : "", i);
>>> @@ -1826,17 +1827,18 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>>>  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w)
>>>  {
>>>      FeatureWordInfo *wi = &feature_word_info[w];
>>> -    assert(kvm_enabled());
>>> -    return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>>> -                                                   wi->cpuid_ecx,
>>> -                                                   wi->cpuid_reg);
>>> +    if (kvm_enabled()) {
>>> +        return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>>> +                                                       wi->cpuid_ecx,
>>> +                                                       wi->cpuid_reg);
>>> +    } else {
>>> +        return wi->tcg_features;
>>> +    }
>>>  }
>>
>> This function is called unconditionally now, so apply the following?
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 48ba1d8..112b437 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -1839,8 +1839,10 @@ static uint32_t
>> x86_cpu_get_supported_feature_word(FeatureWord w)
>>          return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>>                                                         wi->cpuid_ecx,
>>                                                         wi->cpuid_reg);
>> -    } else {
>> +    } else if (tcg_enabled()) {
>>          return wi->tcg_features;
>> +    } else {
>> +        return UINT32_MAX;
> 
> Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX.

FTR done as ~0u to avoid any signedness issues.

> 
>>      }
>>  }
>>
>>
>> Not sure what to do about the warning message. It wouldn't occur though
>> due to the suggested mask, so we could just ignore it for now.
> 
> One day we may be able to simply ask the machine object for the current
> accelerator name. In the meantime, we could use:
> 
>   "warning: host (%s) doesn't support requested feature [...]",
>   kvm_enabled() ? "KVM" : (tcg_enabled() ? "TCG" : "QEMU")
> 
> (But I won't object if you prefer to keep the warning message I
> originally sent.)

I think I did the latter, yes...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 12/18] target-i386: Support check/enforce flags in TCG mode, too

[Paolo Bonzini]

Il 18/06/2014 17:50, Andreas Färber ha scritto:
>>> >> +    } else if (tcg_enabled()) {
>>> >>          return wi->tcg_features;
>>> >> +    } else {
>>> >> +        return UINT32_MAX;
>> >
>> > Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX.
> FTR done as ~0u to avoid any signedness issues.
>

FWIW, I find ~0u worse than ~0, because the former expands to 0xffffffff 
if x86_cpu_get_supported_feature_word is ever changed to return 
uint64_t.  The latter instead returns all-ones as desired.

Paolo

Re: [Qemu-devel] [PATCH v4 13/18] target-i386: Support "-cpu host" in TCG mode

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> As "-cpu host" simply means "enable every bit that can be enabled on
> this host", we can emulate similar behavior even if KVM is not enabled.
> We just need to set all feature bits supported by TCG, accordingly.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Did you consider that this TCG -cpu host is only available when built
with KVM support? There is some #ifdef CONFIG_KVM in addition to these
kvm_enabled() checks.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 13/18] target-i386: Support "-cpu host" in TCG mode

[Eduardo Habkost]

On Thu, May 15, 2014 at 09:12:44PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > As "-cpu host" simply means "enable every bit that can be enabled on
> > this host", we can emulate similar behavior even if KVM is not enabled.
> > We just need to set all feature bits supported by TCG, accordingly.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Did you consider that this TCG -cpu host is only available when built
> with KVM support? There is some #ifdef CONFIG_KVM in addition to these
> kvm_enabled() checks.

Oops, I intended to remove all the CONFIG_KVM #ifdefs but forgot to do
it.

Feel free to squash this into the patch:

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2ef702c..3e9e2da 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1187,8 +1187,6 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
     }
 }
 
-#ifdef CONFIG_KVM
-
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1269,8 +1267,6 @@ static const TypeInfo host_x86_cpu_type_info = {
     .class_init = host_x86_cpu_class_init,
 };
 
-#endif
-
 static void report_unavailable_features(FeatureWord w, uint32_t mask)
 {
     FeatureWordInfo *f = &feature_word_info[w];
@@ -1808,11 +1804,8 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
         snprintf(buf, sizeof(buf), "%s", def->name);
         (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
     }
-#ifdef CONFIG_KVM
     (*cpu_fprintf)(f, "x86 %16s  %-48s\n", "host",
-                   "KVM processor with all supported host features "
-                   "(only available in KVM mode)");
-#endif
+                   "CPU with all features supported by the host");
 
     (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
     for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -2857,9 +2850,7 @@ static void x86_cpu_register_types(void)
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
         x86_register_cpudef_type(&builtin_x86_defs[i]);
     }
-#ifdef CONFIG_KVM
     type_register_static(&host_x86_cpu_type_info);
-#endif
 }
 
 type_init(x86_cpu_register_types)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 14/18] target-i386: Add "migratable" property to "host" CPU model

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> This flag will allow the user to choose between two modes:
>  * All flags that can be enabled on the host, even if unmigratable
>    (migratable=no);
>  * All flags that can be enabled on the host, known to QEMU,
>    and migratable (migratable=yes).
> 
> The default is still migratable=false, to keep current behavior, but
> this will be changed to migratable=true by another patch.
> 
> My plan was to support the "migratable" flag on all CPU classes, but
> have the default to "false" on all CPU models except "host". However,
> DeviceClass has no mechanism to allow a child class to have a different
> property default from the parent class yet, so by now only the "host"
> CPU model will support the "migratable" flag.

Just set the new default in the derived type's instance_init?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu-qom.h |  5 +++++
>  target-i386/cpu.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index e9b3d57..016f90d 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -87,6 +87,11 @@ typedef struct X86CPU {
>      bool hyperv_time;
>      bool check_cpuid;
>      bool enforce_cpuid;
> +    /* If set, only migratable flags will be accepted when "enforce" mode is
> +     * used, and only migratable flags will be included in the "host"
> +     * CPU model.
> +     */

This belongs in the documentation comment above:

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index e9b3d57..aa63d3c 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -71,6 +71,9 @@ typedef struct X86CPUClass {
 /**
  * X86CPU:
  * @env: #CPUX86State
+ * @migratable: If set, only migratable flags will be accepted when
"enforce"
+ * mode is used, and only migratable flags will be included in the "host"
+ * CPU model.
  *
  * An x86 CPU.
  */

> +    bool migratable;
>  
>      /* if true the CPUID code directly forward host cache leaves to the guest */
>      bool cache_info_passthrough;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a357056..9c30957 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -338,6 +338,7 @@ typedef struct FeatureWordInfo {
>      uint32_t cpuid_ecx;   /* Input ECX value for CPUID */
>      int cpuid_reg;        /* output register (R_* constant) */
>      uint32_t tcg_features; /* Feature flags supported by TCG */
> +    uint32_t unmigratable_flags; /* Feature flags known to be unmigratable */

(Here it's in .c only, so would not affect generated documentation.)

>  } FeatureWordInfo;
>  
>  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> @@ -461,6 +462,30 @@ void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features)
>      kvm_default_features[w] &= ~features;
>  }
>  
> +/* Returns the set of feature flags that are supported and migratable by
> + * QEMU, for a given FeatureWord
> + */
> +static uint32_t x86_cpu_get_migratable_flags(FeatureWord w)
> +{
> +    uint32_t r = 0;
> +    int i;
> +
> +    FeatureWordInfo *wi = &feature_word_info[w];
> +    for (i = 0; i < 32; i++) {
> +        uint32_t f = 1U << i;
> +        /* If the feature name is unknown, it is not supported by QEMU yet */
> +        if (!wi->feat_names[i]) {
> +            continue;
> +        }
> +        /* Skip features known to QEMU, but explicitly marked as unmigratable */
> +        if (wi->unmigratable_flags & f) {
> +            continue;
> +        }
> +        r |= f;
> +    }
> +    return r;
> +}
> +
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>  {
> @@ -1206,6 +1231,11 @@ static int cpu_x86_fill_model_id(char *str)
>  
>  static X86CPUDefinition host_cpudef;
>  
> +static Property x86_host_cpu_properties[] = {

host_x86_cpu_... for consistency please (specific to abstract).

> +    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, false),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>  /* class_init for the "host" CPU model
>   *
>   * This function may be called before KVM is initialized.
> @@ -1213,6 +1243,7 @@ static X86CPUDefinition host_cpudef;
>  static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);

Ordered by type hierarchy please.

>      uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
>  
>      host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> @@ -1228,12 +1259,14 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
>      xcc->cpu_def = &host_cpudef;
>      host_cpudef.cache_info_passthrough = true;
>  
> +    dc->props = x86_host_cpu_properties;
>      /* level, xlevel, xlevel2, and the feature words are initialized on
>       * instance_init, because they require KVM to be initialized.
>       */

I'll swap these, as [x]level* are logically still xcc.

>  }
>  
> -static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w);
> +static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> +                                                   bool migratable_only);
>  
>  static void host_x86_cpu_initfn(Object *obj)
>  {
> @@ -1257,7 +1290,7 @@ static void host_x86_cpu_initfn(Object *obj)
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          env->features[w] =
> -            x86_cpu_get_supported_feature_word(w);
> +            x86_cpu_get_supported_feature_word(w, cpu->migratable);
>      }
>      object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
>  }
> @@ -1839,16 +1872,22 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>      return cpu_list;
>  }
>  
> -static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w)
> +static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> +                                                   bool migratable_only)
>  {
>      FeatureWordInfo *wi = &feature_word_info[w];
> +    uint32_t r;
>      if (kvm_enabled()) {
> -        return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> +        r =  kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>                                                         wi->cpuid_ecx,
>                                                         wi->cpuid_reg);

Reindented.

>      } else {
> -        return wi->tcg_features;
> +        r =  wi->tcg_features;
> +    }
> +    if (migratable_only) {
> +        r &= x86_cpu_get_migratable_flags(w);
>      }
> +    return r;
>  }
>  
>  /* Filters CPU feature words based on host availability of each feature
> @@ -1862,7 +1901,8 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>      int rv = 0;
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
> -        uint32_t host_feat = x86_cpu_get_supported_feature_word(w);
> +        uint32_t host_feat =
> +            x86_cpu_get_supported_feature_word(w, cpu->migratable);
>          uint32_t requested_features = env->features[w];
>          env->features[w] &= host_feat;
>          cpu->filtered_features[w] = requested_features & ~env->features[w];

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 14/18] target-i386: Add "migratable" property to "host" CPU model

[Eduardo Habkost]

On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > This flag will allow the user to choose between two modes:
> >  * All flags that can be enabled on the host, even if unmigratable
> >    (migratable=no);
> >  * All flags that can be enabled on the host, known to QEMU,
> >    and migratable (migratable=yes).
> > 
> > The default is still migratable=false, to keep current behavior, but
> > this will be changed to migratable=true by another patch.
> > 
> > My plan was to support the "migratable" flag on all CPU classes, but
> > have the default to "false" on all CPU models except "host". However,
> > DeviceClass has no mechanism to allow a child class to have a different
> > property default from the parent class yet, so by now only the "host"
> > CPU model will support the "migratable" flag.
> 
> Just set the new default in the derived type's instance_init?

That would work. I am still assuming that one day we will allow
management to query for class property defaults without instantiating
objects. But even if we do it, "host" is already an exception (because
the defaults depend on KVM initialization), so in this case it will be
OK.

So, this patch can be dropped because it will be replaced. I will also
implement the other changes you requested for this patch.

Thanks,

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 14/18] target-i386: Add "migratable" property to "host" CPU model

[Andreas Färber]

Am 15.05.2014 22:26, schrieb Eduardo Habkost:
> On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>>> This flag will allow the user to choose between two modes:
>>>  * All flags that can be enabled on the host, even if unmigratable
>>>    (migratable=no);
>>>  * All flags that can be enabled on the host, known to QEMU,
>>>    and migratable (migratable=yes).
>>>
>>> The default is still migratable=false, to keep current behavior, but
>>> this will be changed to migratable=true by another patch.
>>>
>>> My plan was to support the "migratable" flag on all CPU classes, but
>>> have the default to "false" on all CPU models except "host". However,
>>> DeviceClass has no mechanism to allow a child class to have a different
>>> property default from the parent class yet, so by now only the "host"
>>> CPU model will support the "migratable" flag.
>>
>> Just set the new default in the derived type's instance_init?
> 
> That would work. I am still assuming that one day we will allow
> management to query for class property defaults without instantiating
> objects. But even if we do it, "host" is already an exception (because
> the defaults depend on KVM initialization), so in this case it will be
> OK.
> 
> So, this patch can be dropped because it will be replaced. I will also
> implement the other changes you requested for this patch.

Before you make yourself too much work, have a peek at qom-cpu. :)
I should have all except 15 and 18, with some cleanups TBD.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 14/18] target-i386: Add "migratable" property to "host" CPU model

[Eduardo Habkost]

On Fri, May 16, 2014 at 12:12:18AM +0200, Andreas Färber wrote:
> Am 15.05.2014 22:26, schrieb Eduardo Habkost:
> > On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
> >> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> >>> This flag will allow the user to choose between two modes:
> >>>  * All flags that can be enabled on the host, even if unmigratable
> >>>    (migratable=no);
> >>>  * All flags that can be enabled on the host, known to QEMU,
> >>>    and migratable (migratable=yes).
> >>>
> >>> The default is still migratable=false, to keep current behavior, but
> >>> this will be changed to migratable=true by another patch.
> >>>
> >>> My plan was to support the "migratable" flag on all CPU classes, but
> >>> have the default to "false" on all CPU models except "host". However,
> >>> DeviceClass has no mechanism to allow a child class to have a different
> >>> property default from the parent class yet, so by now only the "host"
> >>> CPU model will support the "migratable" flag.
> >>
> >> Just set the new default in the derived type's instance_init?
> > 
> > That would work. I am still assuming that one day we will allow
> > management to query for class property defaults without instantiating
> > objects. But even if we do it, "host" is already an exception (because
> > the defaults depend on KVM initialization), so in this case it will be
> > OK.
> > 
> > So, this patch can be dropped because it will be replaced. I will also
> > implement the other changes you requested for this patch.
> 
> Before you make yourself too much work, have a peek at qom-cpu. :)
> I should have all except 15 and 18, with some cleanups TBD.

Thsnk! But I see two problems on current qom-cpu:

 * The "migratable" flag is now not affecting the results of "-cpu host"
   (host_x86_cpu_initfn()), which was the whole point of adding the
   property.
 * Without setting migratable=yes by default, we are going to break
   existing setups after applying 'support "invariant tsc" flag' and
   "block migration and savevm if invariant tsc is exposed" (See
   http://marc.info/?l=qemu-devel&m=139838802220184&w=2 ).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 14/18] target-i386: Add "migratable" property to "host" CPU model

[Andreas Färber]

Am 16.05.2014 18:13, schrieb Eduardo Habkost:
> On Fri, May 16, 2014 at 12:12:18AM +0200, Andreas Färber wrote:
>> Am 15.05.2014 22:26, schrieb Eduardo Habkost:
>>> On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
>>>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>>>>> This flag will allow the user to choose between two modes:
>>>>>  * All flags that can be enabled on the host, even if unmigratable
>>>>>    (migratable=no);
>>>>>  * All flags that can be enabled on the host, known to QEMU,
>>>>>    and migratable (migratable=yes).
>>>>>
>>>>> The default is still migratable=false, to keep current behavior, but
>>>>> this will be changed to migratable=true by another patch.
>>>>>
>>>>> My plan was to support the "migratable" flag on all CPU classes, but
>>>>> have the default to "false" on all CPU models except "host". However,
>>>>> DeviceClass has no mechanism to allow a child class to have a different
>>>>> property default from the parent class yet, so by now only the "host"
>>>>> CPU model will support the "migratable" flag.
>>>>
>>>> Just set the new default in the derived type's instance_init?
>>>
>>> That would work. I am still assuming that one day we will allow
>>> management to query for class property defaults without instantiating
>>> objects. But even if we do it, "host" is already an exception (because
>>> the defaults depend on KVM initialization), so in this case it will be
>>> OK.
>>>
>>> So, this patch can be dropped because it will be replaced. I will also
>>> implement the other changes you requested for this patch.
>>
>> Before you make yourself too much work, have a peek at qom-cpu. :)
>> I should have all except 15 and 18, with some cleanups TBD.
> 
> Thsnk! But I see two problems on current qom-cpu:
> 
>  * The "migratable" flag is now not affecting the results of "-cpu host"
>    (host_x86_cpu_initfn()), which was the whole point of adding the
>    property.

Where did I break that? Renaming the variable and reordering it with a
comment shouldn't be a functional change... Note that some patches
needed to be applied with patch -p1 due to rebased qom-next, so maybe
there's a mismerge somewhere?

OTOH maybe we should start writing qtests for the CPU? I've been meaning
to write one for cpu-add but didn't get to it yet.

Andreas

>  * Without setting migratable=yes by default, we are going to break
>    existing setups after applying 'support "invariant tsc" flag' and
>    "block migration and savevm if invariant tsc is exposed" (See
>    http://marc.info/?l=qemu-devel&m=139838802220184&w=2 ).

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 14/18] target-i386: Add "migratable" property to "host" CPU model

[Eduardo Habkost]

On Fri, May 16, 2014 at 06:29:36PM +0200, Andreas Färber wrote:
> Am 16.05.2014 18:13, schrieb Eduardo Habkost:
> > On Fri, May 16, 2014 at 12:12:18AM +0200, Andreas Färber wrote:
> >> Am 15.05.2014 22:26, schrieb Eduardo Habkost:
> >>> On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
> >>>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> >>>>> This flag will allow the user to choose between two modes:
> >>>>>  * All flags that can be enabled on the host, even if unmigratable
> >>>>>    (migratable=no);
> >>>>>  * All flags that can be enabled on the host, known to QEMU,
> >>>>>    and migratable (migratable=yes).
> >>>>>
> >>>>> The default is still migratable=false, to keep current behavior, but
> >>>>> this will be changed to migratable=true by another patch.
> >>>>>
> >>>>> My plan was to support the "migratable" flag on all CPU classes, but
> >>>>> have the default to "false" on all CPU models except "host". However,
> >>>>> DeviceClass has no mechanism to allow a child class to have a different
> >>>>> property default from the parent class yet, so by now only the "host"
> >>>>> CPU model will support the "migratable" flag.
> >>>>
> >>>> Just set the new default in the derived type's instance_init?
> >>>
> >>> That would work. I am still assuming that one day we will allow
> >>> management to query for class property defaults without instantiating
> >>> objects. But even if we do it, "host" is already an exception (because
> >>> the defaults depend on KVM initialization), so in this case it will be
> >>> OK.
> >>>
> >>> So, this patch can be dropped because it will be replaced. I will also
> >>> implement the other changes you requested for this patch.
> >>
> >> Before you make yourself too much work, have a peek at qom-cpu. :)
> >> I should have all except 15 and 18, with some cleanups TBD.
> > 
> > Thsnk! But I see two problems on current qom-cpu:
> > 
> >  * The "migratable" flag is now not affecting the results of "-cpu host"
> >    (host_x86_cpu_initfn()), which was the whole point of adding the
> >    property.
> 
> Where did I break that? Renaming the variable and reordering it with a
> comment shouldn't be a functional change... Note that some patches
> needed to be applied with patch -p1 due to rebased qom-next, so maybe
> there's a mismerge somewhere?

Oh, the problem is not on this specific patch, but the fact that the
patch doesn't make sense without applying "Support '-cpu host' in TCG
mode" first (which changes host_x86_cpu_initfn() to use
x86_cpu_get_supported_feature_word() instead of
kvm_arch_get_supported_cpuid() directly).

> 
> OTOH maybe we should start writing qtests for the CPU? I've been meaning
> to write one for cpu-add but didn't get to it yet.

Main difficulty with writing tests for those host/check/enforce changes
is that the results depend on the GET_SUPPORTED_CPUID values returned by
the kernel. This may be addressed by writing a configurable fake
kvm_arch_get_supported_cpuid().

Another difficulty is that the new code is about handling unknown flags,
and any unknown flag may become a known one in future QEMU versions
(which would then break the test). This may be addressed by adding a
hack to clear an arbitrary item on a *_feature_name[] array during
testing.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 15/18] target-i386: Set migratable=yes by default

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> Having only migratable flags reported by default on the "host" CPU model
> is safer for the following reasons:
> 
>  * Existing users may expect "-cpu host" to be migration-safe, if they
>    take care of always using compatible host CPUs, host kernels, and
>    QEMU versions.
>  * Users who don't care aboug migration and want to enable all features

"about"

>    supported by the host kernel can simply change their setup to use
>    migratable=no.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

I see no Reviewed-by or Acked-by for this change...

Shouldn't we at least add .compat_props if we change this behavior?

(NB at this point there are no unmigratable flags yet, but in 18/18.)

Regards,
Andreas

> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9c30957..9ef27fc 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1232,7 +1232,7 @@ static int cpu_x86_fill_model_id(char *str)
>  static X86CPUDefinition host_cpudef;
>  
>  static Property x86_host_cpu_properties[] = {
> -    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, false),
> +    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, true),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 15/18] target-i386: Set migratable=yes by default

[Eduardo Habkost]

On Thu, May 15, 2014 at 10:07:12PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > Having only migratable flags reported by default on the "host" CPU model
> > is safer for the following reasons:
> > 
> >  * Existing users may expect "-cpu host" to be migration-safe, if they
> >    take care of always using compatible host CPUs, host kernels, and
> >    QEMU versions.
> >  * Users who don't care aboug migration and want to enable all features
> 
> "about"

Oops.

> 
> >    supported by the host kernel can simply change their setup to use
> >    migratable=no.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> I see no Reviewed-by or Acked-by for this change...

Paolo? Marcelo? Can you help here?

> 
> Shouldn't we at least add .compat_props if we change this behavior?

I don't believe we need it. Machine-types are about ABI stability, and
"-cpu host" has no ABI stability unless you have exactly the same host
CPU, same QEMU version and the same kernel version.

> 
> (NB at this point there are no unmigratable flags yet, but in 18/18.)

Actually, there are unmigratable flags, already: the ones supported by
the KVM kernel module but are still unknown to QEMU.

> 
> Regards,
> Andreas
> 
> > ---
> >  target-i386/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 9c30957..9ef27fc 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1232,7 +1232,7 @@ static int cpu_x86_fill_model_id(char *str)
> >  static X86CPUDefinition host_cpudef;
> >  
> >  static Property x86_host_cpu_properties[] = {
> > -    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, false),
> > +    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, true),
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >  
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 15/18] target-i386: Set migratable=yes by default

[Marcelo Tosatti]

On Wed, Apr 30, 2014 at 01:48:42PM -0300, Eduardo Habkost wrote:
> Having only migratable flags reported by default on the "host" CPU model
> is safer for the following reasons:
> 
>  * Existing users may expect "-cpu host" to be migration-safe, if they
>    take care of always using compatible host CPUs, host kernels, and
>    QEMU versions.
>  * Users who don't care aboug migration and want to enable all features
>    supported by the host kernel can simply change their setup to use
>    migratable=no.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9c30957..9ef27fc 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1232,7 +1232,7 @@ static int cpu_x86_fill_model_id(char *str)
>  static X86CPUDefinition host_cpudef;
>  
>  static Property x86_host_cpu_properties[] = {
> -    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, false),
> +    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, true),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> -- 
> 1.9.0

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

Re: [Qemu-devel] [PATCH v4 17/18] target-i386: block migration and savevm if invariant tsc is exposed

[Andreas Färber]

Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> From: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Invariant TSC documentation mentions that "invariant TSC will run at a
> constant rate in all ACPI P-, C-. and T-states".
> 
> This is not the case if migration to a host with different TSC frequency
> is allowed, or if savevm is performed. So block migration/savevm.
> 
> Also do not expose invariant tsc flag by default.
> 
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu-qom.h |  2 +-
>  target-i386/kvm.c     | 13 +++++++++++++
>  target-i386/machine.c |  2 +-
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 016f90d..473d803 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -121,7 +121,7 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
>  #define ENV_OFFSET offsetof(X86CPU, env)
>  
>  #ifndef CONFIG_USER_ONLY
> -extern const struct VMStateDescription vmstate_x86_cpu;
> +extern struct VMStateDescription vmstate_x86_cpu;
>  #endif
>  
>  /**
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 4389959..99cc7e3 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -33,6 +33,8 @@
>  #include "exec/ioport.h"
>  #include <asm/hyperv.h>
>  #include "hw/pci/pci.h"
> +#include "migration/migration.h"
> +#include "qapi/qmp/qerror.h"
>  
>  //#define DEBUG_KVM
>  
> @@ -447,6 +449,8 @@ static bool hyperv_enabled(X86CPU *cpu)
>              cpu->hyperv_relaxed_timing);
>  }
>  
> +Error *invtsc_mig_blocker;

This should be static, even if no zero-initialization is needed below.

> +
>  #define KVM_MAX_CPUID_ENTRIES  100
>  
>  int kvm_arch_init_vcpu(CPUState *cs)
> @@ -702,6 +706,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                                    !!(c->ecx & CPUID_EXT_SMX);
>      }
>  
> +    c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> +    if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
> +        /* for migration */
> +        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED, "cpu");

This does not compile for me. error_setg()? With what text?

Regards,
Andreas

> +        migrate_add_blocker(invtsc_mig_blocker);
> +        /* for savevm */
> +        vmstate_x86_cpu.unmigratable = 1;
> +    }
> +
>      cpuid_data.cpuid.padding = 0;
>      r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);
>      if (r) {
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 168cab6..4d4c023 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -613,7 +613,7 @@ static const VMStateDescription vmstate_msr_hyperv_time = {
>      }
>  };
>  
> -const VMStateDescription vmstate_x86_cpu = {
> +VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
>      .minimum_version_id = 3,
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 17/18] target-i386: block migration and savevm if invariant tsc is exposed

[Andreas Färber]

Am 15.05.2014 22:22, schrieb Andreas Färber:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>> From: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> Invariant TSC documentation mentions that "invariant TSC will run at a
>> constant rate in all ACPI P-, C-. and T-states".
>>
>> This is not the case if migration to a host with different TSC frequency
>> is allowed, or if savevm is performed. So block migration/savevm.
>>
>> Also do not expose invariant tsc flag by default.
>>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  target-i386/cpu-qom.h |  2 +-
>>  target-i386/kvm.c     | 13 +++++++++++++
>>  target-i386/machine.c |  2 +-
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
>> index 016f90d..473d803 100644
>> --- a/target-i386/cpu-qom.h
>> +++ b/target-i386/cpu-qom.h
>> @@ -121,7 +121,7 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
>>  #define ENV_OFFSET offsetof(X86CPU, env)
>>  
>>  #ifndef CONFIG_USER_ONLY
>> -extern const struct VMStateDescription vmstate_x86_cpu;
>> +extern struct VMStateDescription vmstate_x86_cpu;
>>  #endif
>>  
>>  /**
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 4389959..99cc7e3 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -33,6 +33,8 @@
>>  #include "exec/ioport.h"
>>  #include <asm/hyperv.h>
>>  #include "hw/pci/pci.h"
>> +#include "migration/migration.h"
>> +#include "qapi/qmp/qerror.h"
>>  
>>  //#define DEBUG_KVM
>>  
>> @@ -447,6 +449,8 @@ static bool hyperv_enabled(X86CPU *cpu)
>>              cpu->hyperv_relaxed_timing);
>>  }
>>  
>> +Error *invtsc_mig_blocker;
> 
> This should be static, even if no zero-initialization is needed below.
> 
>> +
>>  #define KVM_MAX_CPUID_ENTRIES  100
>>  
>>  int kvm_arch_init_vcpu(CPUState *cs)
>> @@ -702,6 +706,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>                                    !!(c->ecx & CPUID_EXT_SMX);
>>      }
>>  
>> +    c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>> +    if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
>> +        /* for migration */
>> +        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED, "cpu");
> 
> This does not compile for me. error_setg()? With what text?

http://git.qemu-project.org/?p=qemu.git;a=blobdiff;f=include/qapi/qmp/qerror.h;h=01d1d0661c607ace1c5d3831e5c79eeab851f6b7;hp=a72bbc98503fe261bb8d2c407220252b1e6a85a4;hb=f231b88db14f13ee9a41599896f57f3594c1ca8b;hpb=d73f0beadb57f885e678bffc362864f4401262e0

-#define QERR_MIGRATION_NOT_SUPPORTED \
-    ERROR_CLASS_GENERIC_ERROR, "State blocked by non-migratable device
'%s'"

Suggesting something nicer than "device 'cpu'":

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 1fe8512..9b09a1a 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -710,7 +710,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
     c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
     if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
         /* for migration */
-        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED,
"cpu");
+        error_setg(&invtsc_mig_blocker,
+                   "State blocked by non-migratable CPU device");
         migrate_add_blocker(invtsc_mig_blocker);
         /* for savevm */
         vmstate_x86_cpu.unmigratable = 1;

> 
> Regards,
> Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 17/18] target-i386: block migration and savevm if invariant tsc is exposed

[Luiz Capitulino]

On Fri, 16 May 2014 11:05:28 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 15.05.2014 22:22, schrieb Andreas Färber:
> > Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> >> From: Marcelo Tosatti <mtosatti@redhat.com>
> >>
> >> Invariant TSC documentation mentions that "invariant TSC will run at a
> >> constant rate in all ACPI P-, C-. and T-states".
> >>
> >> This is not the case if migration to a host with different TSC frequency
> >> is allowed, or if savevm is performed. So block migration/savevm.
> >>
> >> Also do not expose invariant tsc flag by default.
> >>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >>  target-i386/cpu-qom.h |  2 +-
> >>  target-i386/kvm.c     | 13 +++++++++++++
> >>  target-i386/machine.c |  2 +-
> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> >> index 016f90d..473d803 100644
> >> --- a/target-i386/cpu-qom.h
> >> +++ b/target-i386/cpu-qom.h
> >> @@ -121,7 +121,7 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> >>  #define ENV_OFFSET offsetof(X86CPU, env)
> >>  
> >>  #ifndef CONFIG_USER_ONLY
> >> -extern const struct VMStateDescription vmstate_x86_cpu;
> >> +extern struct VMStateDescription vmstate_x86_cpu;
> >>  #endif
> >>  
> >>  /**
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index 4389959..99cc7e3 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -33,6 +33,8 @@
> >>  #include "exec/ioport.h"
> >>  #include <asm/hyperv.h>
> >>  #include "hw/pci/pci.h"
> >> +#include "migration/migration.h"
> >> +#include "qapi/qmp/qerror.h"
> >>  
> >>  //#define DEBUG_KVM
> >>  
> >> @@ -447,6 +449,8 @@ static bool hyperv_enabled(X86CPU *cpu)
> >>              cpu->hyperv_relaxed_timing);
> >>  }
> >>  
> >> +Error *invtsc_mig_blocker;
> > 
> > This should be static, even if no zero-initialization is needed below.
> > 
> >> +
> >>  #define KVM_MAX_CPUID_ENTRIES  100
> >>  
> >>  int kvm_arch_init_vcpu(CPUState *cs)
> >> @@ -702,6 +706,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >>                                    !!(c->ecx & CPUID_EXT_SMX);
> >>      }
> >>  
> >> +    c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> >> +    if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
> >> +        /* for migration */
> >> +        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED, "cpu");
> > 
> > This does not compile for me. error_setg()? With what text?
> 
> http://git.qemu-project.org/?p=qemu.git;a=blobdiff;f=include/qapi/qmp/qerror.h;h=01d1d0661c607ace1c5d3831e5c79eeab851f6b7;hp=a72bbc98503fe261bb8d2c407220252b1e6a85a4;hb=f231b88db14f13ee9a41599896f57f3594c1ca8b;hpb=d73f0beadb57f885e678bffc362864f4401262e0
> 
> -#define QERR_MIGRATION_NOT_SUPPORTED \
> -    ERROR_CLASS_GENERIC_ERROR, "State blocked by non-migratable device
> '%s'"
> 
> Suggesting something nicer than "device 'cpu'":
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 1fe8512..9b09a1a 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -710,7 +710,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>      if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
>          /* for migration */
> -        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED,
> "cpu");
> +        error_setg(&invtsc_mig_blocker,
> +                   "State blocked by non-migratable CPU device");

That seems correct to me.

>          migrate_add_blocker(invtsc_mig_blocker);
>          /* for savevm */
>          vmstate_x86_cpu.unmigratable = 1;
> 
> > 
> > Regards,
> > Andreas
> 

Re: [Qemu-devel] [PATCH v4 17/18] target-i386: block migration and savevm if invariant tsc is exposed

[Eduardo Habkost]

On Fri, May 16, 2014 at 11:05:28AM +0200, Andreas Färber wrote:
> Am 15.05.2014 22:22, schrieb Andreas Färber:
> > Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> >> From: Marcelo Tosatti <mtosatti@redhat.com>
> >>
> >> Invariant TSC documentation mentions that "invariant TSC will run at a
> >> constant rate in all ACPI P-, C-. and T-states".
> >>
> >> This is not the case if migration to a host with different TSC frequency
> >> is allowed, or if savevm is performed. So block migration/savevm.
> >>
> >> Also do not expose invariant tsc flag by default.
> >>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >>  target-i386/cpu-qom.h |  2 +-
> >>  target-i386/kvm.c     | 13 +++++++++++++
> >>  target-i386/machine.c |  2 +-
> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> >> index 016f90d..473d803 100644
> >> --- a/target-i386/cpu-qom.h
> >> +++ b/target-i386/cpu-qom.h
> >> @@ -121,7 +121,7 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> >>  #define ENV_OFFSET offsetof(X86CPU, env)
> >>  
> >>  #ifndef CONFIG_USER_ONLY
> >> -extern const struct VMStateDescription vmstate_x86_cpu;
> >> +extern struct VMStateDescription vmstate_x86_cpu;
> >>  #endif
> >>  
> >>  /**
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index 4389959..99cc7e3 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -33,6 +33,8 @@
> >>  #include "exec/ioport.h"
> >>  #include <asm/hyperv.h>
> >>  #include "hw/pci/pci.h"
> >> +#include "migration/migration.h"
> >> +#include "qapi/qmp/qerror.h"
> >>  
> >>  //#define DEBUG_KVM
> >>  
> >> @@ -447,6 +449,8 @@ static bool hyperv_enabled(X86CPU *cpu)
> >>              cpu->hyperv_relaxed_timing);
> >>  }
> >>  
> >> +Error *invtsc_mig_blocker;
> > 
> > This should be static, even if no zero-initialization is needed below.
> > 
> >> +
> >>  #define KVM_MAX_CPUID_ENTRIES  100
> >>  
> >>  int kvm_arch_init_vcpu(CPUState *cs)
> >> @@ -702,6 +706,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >>                                    !!(c->ecx & CPUID_EXT_SMX);
> >>      }
> >>  
> >> +    c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> >> +    if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
> >> +        /* for migration */
> >> +        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED, "cpu");
> > 
> > This does not compile for me. error_setg()? With what text?
> 
> http://git.qemu-project.org/?p=qemu.git;a=blobdiff;f=include/qapi/qmp/qerror.h;h=01d1d0661c607ace1c5d3831e5c79eeab851f6b7;hp=a72bbc98503fe261bb8d2c407220252b1e6a85a4;hb=f231b88db14f13ee9a41599896f57f3594c1ca8b;hpb=d73f0beadb57f885e678bffc362864f4401262e0
> 
> -#define QERR_MIGRATION_NOT_SUPPORTED \
> -    ERROR_CLASS_GENERIC_ERROR, "State blocked by non-migratable device
> '%s'"
> 
> Suggesting something nicer than "device 'cpu'":
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 1fe8512..9b09a1a 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -710,7 +710,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>      if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
>          /* for migration */
> -        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED,
> "cpu");
> +        error_setg(&invtsc_mig_blocker,
> +                   "State blocked by non-migratable CPU device");
>          migrate_add_blocker(invtsc_mig_blocker);
>          /* for savevm */
>          vmstate_x86_cpu.unmigratable = 1;

v3 sent by Marcelo had a different error message, but my queue had v2. I
suggest this:

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f9ffa4b..7099a51 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -710,8 +710,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
     c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
     if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
         /* for migration */
-        error_setg(&invtsc_mig_blocker,
-                   "State blocked by non-migratable CPU device");
+        error_set(&invtsc_mig_blocker,
+                  QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "invtsc", "cpu");
         migrate_add_blocker(invtsc_mig_blocker);
         /* for savevm */
         vmstate_x86_cpu.unmigratable = 1;

Requiring the user to look at QEMU source code to find out which feature
is blocking migration would just cause confusion.

BTW, applying this patch without applying "Set migratable=yes by
default" will break existing setups where users expect migration to
work. See the discussion at:
http://marc.info/?l=qemu-devel&m=139838802220184&w=2

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 17/18] target-i386: block migration and savevm if invariant tsc is exposed

[Eduardo Habkost]

On Fri, May 16, 2014 at 12:36:49PM -0300, Eduardo Habkost wrote:
[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index f9ffa4b..7099a51 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -710,8 +710,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>      if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
>          /* for migration */
> -        error_setg(&invtsc_mig_blocker,
> -                   "State blocked by non-migratable CPU device");
> +        error_set(&invtsc_mig_blocker,
> +                  QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "invtsc", "cpu");
>          migrate_add_blocker(invtsc_mig_blocker);
>          /* for savevm */
>          vmstate_x86_cpu.unmigratable = 1;

Nevermind, QERR_DEVICE_FEATURE_BLOCKS_MIGRATION was removed recently,
too.

-- 
Eduardo