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

["Andreas Färber" <afaerber@suse.de>]

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