[qom-cpu-next,v3,3/4] target-i386: Slim conversion to X86CPU subclasses

Commit Message

Andreas Färber Feb. 2, 2013, 12:37 a.m. UTC

Move x86_def_t definition to header and embed into X86CPUClass.
Register types per built-in model definition.

Move version initialization from x86_cpudef_setup() to class_init.

Inline cpu_x86_register() into the X86CPU initfn.
Since instance_init cannot reports errors, drop error handling.

Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
Move KVM host vendor override from cpu_x86_find_by_name() to the initfn.

Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs.
Make kvm_cpu_fill_host() a class_init and inline cpu_x86_fill_model_id().

Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
comparison.

Signed-off-by: Andreas Färber <[e-mail]>
---
 target-i386/cpu-qom.h |   24 ++++
 target-i386/cpu.c     |  324 +++++++++++++++++--------------------------------
 target-i386/cpu.h     |    2 -
 target-i386/kvm.c     |   93 ++++++++++++++
 4 Dateien geändert, 228 Zeilen hinzugefügt(+), 215 Zeilen entfernt(-)

Comments

Igor Mammedov Feb. 4, 2013, 11:08 a.m. UTC | #1

On Sat,  2 Feb 2013 01:37:07 +0100
Andreas Färber <[e-mail]> wrote:

> Move x86_def_t definition to header and embed into X86CPUClass.
> Register types per built-in model definition.
> 
> Move version initialization from x86_cpudef_setup() to class_init.
> 
> Inline cpu_x86_register() into the X86CPU initfn.
> Since instance_init cannot reports errors, drop error handling.
> 
> Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
> Move KVM host vendor override from cpu_x86_find_by_name() to the initfn.
> 
> Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs.
> Make kvm_cpu_fill_host() a class_init and inline cpu_x86_fill_model_id().
> 
> Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
> comparison.
> 
> Signed-off-by: Andreas Färber <[e-mail]>
> ---
>  target-i386/cpu-qom.h |   24 ++++
>  target-i386/cpu.c     |  324
> +++++++++++++++++-------------------------------- target-i386/cpu.h
> |    2 - target-i386/kvm.c     |   93 ++++++++++++++
>  4 Dateien geändert, 228 Zeilen hinzugefügt(+), 215 Zeilen entfernt(-)
> 
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 48e6b54..80bf72d 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -30,6 +30,27 @@
>  #define TYPE_X86_CPU "i386-cpu"
>  #endif
>  
> +#define TYPE_HOST_X86_CPU "host-" TYPE_X86_CPU
> +
> +typedef struct x86_def_t {
> +    const char *name;
> +    uint32_t level;
> +    /* vendor is zero-terminated, 12 character ASCII string */
> +    char vendor[CPUID_VENDOR_SZ + 1];
> +    int family;
> +    int model;
> +    int stepping;
> +    uint32_t features, ext_features, ext2_features, ext3_features;
> +    uint32_t kvm_features, svm_features;
> +    uint32_t xlevel;
> +    char model_id[48];
> +    /* Store the results of Centaur's CPUID instructions */
> +    uint32_t ext4_features;
> +    uint32_t xlevel2;
> +    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
> +    uint32_t cpuid_7_0_ebx_features;
> +} x86_def_t;
> +
>  #define X86_CPU_CLASS(klass) \
>      OBJECT_CLASS_CHECK(X86CPUClass, (klass), TYPE_X86_CPU)
>  #define X86_CPU(obj) \
> @@ -41,6 +62,7 @@
>   * X86CPUClass:
>   * @parent_realize: The parent class' realize handler.
>   * @parent_reset: The parent class' reset handler.
> + * @info: Model-specific data.
>   *
>   * An x86 CPU model or family.
>   */
> @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
>  
>      DeviceRealize parent_realize;
>      void (*parent_reset)(CPUState *cpu);
> +
> +    x86_def_t info;
Is it necessary to embed it here, could pointer to corresponding static array
be used here?
That way one could avoid extra memcpy in class_init().

>  } X86CPUClass;
>  
>  /**
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ee2fd6b..6c95740 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -346,25 +346,6 @@ static void add_flagname_to_bitmaps(const char
> *flagname, }
>  }
>  
> -typedef struct x86_def_t {
> -    const char *name;
> -    uint32_t level;
> -    /* vendor is zero-terminated, 12 character ASCII string */
> -    char vendor[CPUID_VENDOR_SZ + 1];
> -    int family;
> -    int model;
> -    int stepping;
> -    uint32_t features, ext_features, ext2_features, ext3_features;
> -    uint32_t kvm_features, svm_features;
> -    uint32_t xlevel;
> -    char model_id[48];
> -    /* Store the results of Centaur's CPUID instructions */
> -    uint32_t ext4_features;
> -    uint32_t xlevel2;
> -    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
> -    uint32_t cpuid_7_0_ebx_features;
> -} x86_def_t;
> -
>  #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
>  #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
>            CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
> @@ -868,86 +849,6 @@ static x86_def_t builtin_x86_defs[] = {
>      },
>  };
>  
> -#ifdef CONFIG_KVM
> -static int cpu_x86_fill_model_id(char *str)
> -{
> -    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> -    int i;
> -
> -    for (i = 0; i < 3; i++) {
> -        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
> -        memcpy(str + i * 16 +  0, &eax, 4);
> -        memcpy(str + i * 16 +  4, &ebx, 4);
> -        memcpy(str + i * 16 +  8, &ecx, 4);
> -        memcpy(str + i * 16 + 12, &edx, 4);
> -    }
> -    return 0;
> -}
> -#endif
> -
> -/* Fill a x86_def_t struct with information about the host CPU, and
> - * the CPU features supported by the host hardware + host kernel
> - *
> - * This function may be called only if KVM is enabled.
> - */
> -static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
> -{
> -#ifdef CONFIG_KVM
> -    KVMState *s = kvm_state;
> -    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> -
> -    assert(kvm_enabled());
> -
> -    x86_cpu_def->name = "host";
> -    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> -    x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> -
> -    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
> -    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
> -    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
> -    x86_cpu_def->stepping = eax & 0x0F;
> -
> -    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> -    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
> -    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
> R_ECX); -
> -    if (x86_cpu_def->level >= 7) {
> -        x86_cpu_def->cpuid_7_0_ebx_features =
> -                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
> -    } else {
> -        x86_cpu_def->cpuid_7_0_ebx_features = 0;
> -    }
> -
> -    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0,
> R_EAX);
> -    x86_cpu_def->ext2_features =
> -                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
> -    x86_cpu_def->ext3_features =
> -                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
> -
> -    cpu_x86_fill_model_id(x86_cpu_def->model_id);
> -
> -    /* Call Centaur's CPUID instruction. */
> -    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> -        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> -        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> -        if (eax >= 0xC0000001) {
> -            /* Support VIA max extended level */
> -            x86_cpu_def->xlevel2 = eax;
> -            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
> -            x86_cpu_def->ext4_features =
> -                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
> -        }
> -    }
> -
> -    /* Other KVM-specific feature fields: */
> -    x86_cpu_def->svm_features =
> -        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
> -    x86_cpu_def->kvm_features =
> -        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
> -
> -#endif /* CONFIG_KVM */
> -}
> -
>  static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
>  {
>      int i;
> @@ -975,31 +876,31 @@ static int unavailable_host_feature(FeatureWordInfo
> *f, uint32_t mask) static int kvm_check_features_against_host(X86CPU *cpu)
>  {
>      CPUX86State *env = &cpu->env;
> -    x86_def_t host_def;
> +    ObjectClass *host_oc = object_class_by_name(TYPE_HOST_X86_CPU);
> +    X86CPUClass *host_xcc = X86_CPU_CLASS(host_oc);
>      uint32_t mask;
>      int rv, i;
>      struct model_features_t ft[] = {
> -        {&env->cpuid_features, &host_def.features,
> +        {&env->cpuid_features, &host_xcc->info.features,
>              FEAT_1_EDX },
> -        {&env->cpuid_ext_features, &host_def.ext_features,
> +        {&env->cpuid_ext_features, &host_xcc->info.ext_features,
>              FEAT_1_ECX },
> -        {&env->cpuid_ext2_features, &host_def.ext2_features,
> +        {&env->cpuid_ext2_features, &host_xcc->info.ext2_features,
>              FEAT_8000_0001_EDX },
> -        {&env->cpuid_ext3_features, &host_def.ext3_features,
> +        {&env->cpuid_ext3_features, &host_xcc->info.ext3_features,
>              FEAT_8000_0001_ECX },
> -        {&env->cpuid_ext4_features, &host_def.ext4_features,
> +        {&env->cpuid_ext4_features, &host_xcc->info.ext4_features,
>              FEAT_C000_0001_EDX },
> -        {&env->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
> +        {&env->cpuid_7_0_ebx_features,
> &host_xcc->info.cpuid_7_0_ebx_features, FEAT_7_0_EBX },
> -        {&env->cpuid_svm_features, &host_def.svm_features,
> +        {&env->cpuid_svm_features, &host_xcc->info.svm_features,
>              FEAT_SVM },
> -        {&env->cpuid_kvm_features, &host_def.kvm_features,
> +        {&env->cpuid_kvm_features, &host_xcc->info.kvm_features,
>              FEAT_KVM },
>      };
>  
>      assert(kvm_enabled());
>  
> -    kvm_cpu_fill_host(&host_def);
>      for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) {
>          FeatureWord w = ft[i].feat_word;
>          FeatureWordInfo *wi = &feature_word_info[w];
> @@ -1261,40 +1162,30 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
> Visitor *v, void *opaque, cpu->env.tsc_khz = value / 1000;
>  }
>  
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
> +static ObjectClass *x86_cpu_class_by_name(const char *name)
>  {
> -    x86_def_t *def;
> -    int i;
> +    ObjectClass *oc;
> +    char *typename;
>  
>      if (name == NULL) {
> -        return -1;
> -    }
> -    if (kvm_enabled() && strcmp(name, "host") == 0) {
> -        kvm_cpu_fill_host(x86_cpu_def);
> -        return 0;
> +        return NULL;
>      }
>  
> -    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> -        def = &builtin_x86_defs[i];
> -        if (strcmp(name, def->name) == 0) {
> -            memcpy(x86_cpu_def, def, sizeof(*def));
> -            /* sysenter isn't supported in compatibility mode on AMD,
> -             * syscall isn't supported in compatibility mode on Intel.
> -             * Normally we advertise the actual CPU vendor, but you can
> -             * override this using the 'vendor' property if you want to use
> -             * KVM's sysenter/syscall emulation in compatibility mode and
> -             * when doing cross vendor migration
> -             */
> -            if (kvm_enabled()) {
> -                uint32_t  ebx = 0, ecx = 0, edx = 0;
> -                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> -                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx,
> ecx);
> -            }
> -            return 0;
> +    if (strcmp(name, "host") == 0) {
> +        if (kvm_enabled()) {
> +            return object_class_by_name(TYPE_HOST_X86_CPU);
>          }
> +        return NULL;
>      }
block is not necessary, the following block could yield the same result.

>  
> -    return -1;
> +    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
> +    oc = object_class_by_name(typename);
> +    g_free(typename);
> +    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_X86_CPU) ||
> +                       object_class_is_abstract(oc))) {
> +        oc = NULL;
> +    }
> +    return oc;
>  }
>  
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
> @@ -1516,60 +1407,13 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  }
>  #endif
>  
> -static int cpu_x86_register(X86CPU *cpu, const char *name)
> -{
> -    CPUX86State *env = &cpu->env;
> -    x86_def_t def1, *def = &def1;
> -    Error *error = NULL;
> -
> -    memset(def, 0, sizeof(*def));
> -
> -    if (cpu_x86_find_by_name(def, name) < 0) {
> -        error_setg(&error, "Unable to find CPU definition: %s", name);
> -        goto out;
> -    }
> -
> -    if (kvm_enabled()) {
> -        def->kvm_features |= kvm_default_features;
> -    }
> -    def->ext_features |= CPUID_EXT_HYPERVISOR;
> -
> -    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> -    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> -    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> -    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
> -    object_property_set_int(OBJECT(cpu), def->stepping, "stepping",
> &error);
> -    env->cpuid_features = def->features;
> -    env->cpuid_ext_features = def->ext_features;
> -    env->cpuid_ext2_features = def->ext2_features;
> -    env->cpuid_ext3_features = def->ext3_features;
> -    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
> -    env->cpuid_kvm_features = def->kvm_features;
> -    env->cpuid_svm_features = def->svm_features;
> -    env->cpuid_ext4_features = def->ext4_features;
> -    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> -    env->cpuid_xlevel2 = def->xlevel2;
> -
> -    object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
> &error);
> -    if (error) {
> -        goto out;
> -    }
> -
> -out:
> -    if (error) {
> -        fprintf(stderr, "%s\n", error_get_pretty(error));
> -        error_free(error);
> -        return -1;
> -    }
> -    return 0;
> -}
> -
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu = NULL;
>      CPUX86State *env;
>      gchar **model_pieces;
>      char *name, *features;
> +    ObjectClass *oc;
>      Error *error = NULL;
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
> @@ -1580,13 +1424,13 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>      name = model_pieces[0];
>      features = model_pieces[1];
>  
> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> -    env = &cpu->env;
> -    env->cpu_model_str = cpu_model;
> -
> -    if (cpu_x86_register(cpu, name) < 0) {
> +    oc = x86_cpu_class_by_name(name);
> +    if (oc == NULL) {
make sure error is reported when cpu is not found:
           error_setg(&error, "Can't find CPU: %s", name);

>          goto error;
>      }
> +    cpu = X86_CPU(object_new(object_class_get_name(oc)));
> +    env = &cpu->env;
> +    env->cpu_model_str = cpu_model;
>  
>      cpu_x86_parse_featurestr(cpu, features, &error);
>      if (error) {
> @@ -1621,30 +1465,6 @@ void cpu_clear_apic_feature(CPUX86State *env)
>  
>  #endif /* !CONFIG_USER_ONLY */
>  
> -/* Initialize list of CPU models, filling some non-static fields if
> necessary
> - */
> -void x86_cpudef_setup(void)
> -{
> -    int i, j;
> -    static const char *model_with_versions[] = { "qemu32", "qemu64",
> "athlon" }; -
> -    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
> -        x86_def_t *def = &builtin_x86_defs[i];
> -
> -        /* Look for specific "cpudef" models that */
> -        /* have the QEMU version in .model_id */
> -        for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) {
> -            if (strcmp(model_with_versions[j], def->name) == 0) {
> -                pstrcpy(def->model_id, sizeof(def->model_id),
> -                        "QEMU Virtual CPU version ");
> -                pstrcat(def->model_id, sizeof(def->model_id),
> -                        qemu_get_version());
> -                break;
> -            }
> -        }
> -    }
> -}
> -
>  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>                               uint32_t *ecx, uint32_t *edx)
>  {
> @@ -2198,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
> +    const x86_def_t *def = &xcc->info;
>      static int inited;
>  
>      cpu_exec_init(env);
> @@ -2227,6 +2049,41 @@ static void x86_cpu_initfn(Object *obj)
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
>  
> +    /* sysenter isn't supported in compatibility mode on AMD,
> +     * syscall isn't supported in compatibility mode on Intel.
> +     * Normally we advertise the actual CPU vendor, but you can
> +     * override this using the 'vendor' property if you want to use
> +     * KVM's sysenter/syscall emulation in compatibility mode and
> +     * when doing cross vendor migration
> +     */
> +    if (kvm_enabled()) {
> +        host_cpuid(0, 0, NULL, &env->cpuid_vendor1, &env->cpuid_vendor2,
> +                   &env->cpuid_vendor3);
This is not per instance specific, this override would be better done to
sub-classes in kvm_arch_init(). As bonus we would get actual 'vendor' when
doing class introspection provided it's done after kvm_init() is called.


> +    } else {
> +        object_property_set_str(OBJECT(cpu), def->vendor, "vendor", NULL);
> +    }
> +
> +    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
> +    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
> +    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", NULL);
> +    env->cpuid_features = def->features;
> +    env->cpuid_ext_features = def->ext_features;
> +    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
> +    env->cpuid_ext2_features = def->ext2_features;
> +    env->cpuid_ext3_features = def->ext3_features;
> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
> +    env->cpuid_kvm_features = def->kvm_features;
> +    if (kvm_enabled()) {
> +        env->cpuid_kvm_features |= kvm_default_features;
> +    }
> +    env->cpuid_svm_features = def->svm_features;
> +    env->cpuid_ext4_features = def->ext4_features;
> +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> +    env->cpuid_xlevel2 = def->xlevel2;
> +
> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", NULL);
All this feature initialization in initfn() is not compatible with
static/global properties because they are set in device_initfn(). But I can
remove properties/features from here as they are converted to static
properties and incrementally move their defaults initialization into
new x86_cpu_def_class_init().

> +
>      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>  
>      /* init various static tables used in TCG mode */
> @@ -2239,6 +2096,27 @@ static void x86_cpu_initfn(Object *obj)
>      }
>  }
>  
> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
> +{
> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> +    x86_def_t *def = data;
> +    int i;
> +    static const char *versioned_models[] = { "qemu32", "qemu64",
> "athlon" }; +
> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
perhaps sizeof(xcc->info) would be better?

> +
> +    /* Look for specific models that have the QEMU version in .model_id */
> +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
> +        if (strcmp(versioned_models[i], def->name) == 0) {
> +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
> +                    "QEMU Virtual CPU version ");
> +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
> +                    qemu_get_version());
> +            break;
> +        }
> +    }
> +}
> +
>  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>  {
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> @@ -2250,6 +2128,21 @@ static void x86_cpu_common_class_init(ObjectClass
> *oc, void *data) 
>      xcc->parent_reset = cc->reset;
>      cc->reset = x86_cpu_reset;
> +
> +    cc->class_by_name = x86_cpu_class_by_name;
> +}
> +
> +static void x86_register_cpu_type(const x86_def_t *def)
> +{
> +    TypeInfo type_info = {
> +        .parent = TYPE_X86_CPU,
> +        .class_init = x86_cpu_def_class_init,
> +        .class_data = (void *)def,
> +    };
> +
> +    type_info.name = g_strdup_printf("%s-" TYPE_X86_CPU, def->name);
> +    type_register(&type_info);
> +    g_free((void *)type_info.name);
>  }
>  
>  static const TypeInfo x86_cpu_type_info = {
> @@ -2257,14 +2150,19 @@ static const TypeInfo x86_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(X86CPU),
>      .instance_init = x86_cpu_initfn,
> -    .abstract = false,
> +    .abstract = true,
>      .class_size = sizeof(X86CPUClass),
>      .class_init = x86_cpu_common_class_init,
>  };
>  
>  static void x86_cpu_register_types(void)
>  {
> +    int i;
> +
>      type_register_static(&x86_cpu_type_info);
> +    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> +        x86_register_cpu_type(&builtin_x86_defs[i]);
> +    }
>  }
>  
>  type_init(x86_cpu_register_types)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 7577e4f..2aef7b7 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -887,7 +887,6 @@ typedef struct CPUX86State {
>  X86CPU *cpu_x86_init(const char *cpu_model);
>  int cpu_x86_exec(CPUX86State *s);
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> -void x86_cpudef_setup(void);
>  int cpu_x86_support_mca_broadcast(CPUX86State *env);
>  
>  int cpu_get_pic_interrupt(CPUX86State *s);
> @@ -1079,7 +1078,6 @@ static inline CPUX86State *cpu_init(const char
> *cpu_model) #define cpu_gen_code cpu_x86_gen_code
>  #define cpu_signal_handler cpu_x86_signal_handler
>  #define cpu_list x86_cpu_list
> -#define cpudef_setup	x86_cpudef_setup
>  
>  #define CPU_SAVE_VERSION 12
>  
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 9ebf181..dbfa670 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -735,6 +735,80 @@ static int kvm_get_supported_msrs(KVMState *s)
>      return ret;
>  }
>  
> +static void kvm_host_cpu_initfn(Object *obj)
> +{
> +    assert(kvm_enabled());
> +}
> +
> +static bool kvm_host_cpu_needs_class_init;
> +
> +static void kvm_host_cpu_class_init(ObjectClass *oc, void *data)
> +{
> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> +    x86_def_t *x86_cpu_def = &xcc->info;
> +    KVMState *s = kvm_state;
> +    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> +    int i;
> +
> +    if (!kvm_enabled()) {
> +        kvm_host_cpu_needs_class_init = true;
> +        return;
> +    }
> +
> +    xcc->info.name = "host";
> +
> +    /* Vendor will be set in initfn if kvm_enabled() */
> +
> +    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
> +    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
> +    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
> +    x86_cpu_def->stepping = eax & 0x0F;
> +
> +    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> +    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
> +    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
> R_ECX); +
> +    if (x86_cpu_def->level >= 7) {
> +        x86_cpu_def->cpuid_7_0_ebx_features =
> +                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
> +    } else {
> +        x86_cpu_def->cpuid_7_0_ebx_features = 0;
> +    }
> +
> +    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0,
> R_EAX);
> +    x86_cpu_def->ext2_features =
> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
> +    x86_cpu_def->ext3_features =
> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
> +
> +    for (i = 0; i < 3; i++) {
> +        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
> +        memcpy(xcc->info.model_id + i * 16 +  0, &eax, 4);
> +        memcpy(xcc->info.model_id + i * 16 +  4, &ebx, 4);
> +        memcpy(xcc->info.model_id + i * 16 +  8, &ecx, 4);
> +        memcpy(xcc->info.model_id + i * 16 + 12, &edx, 4);
> +    }
> +
> +    /* Call Centaur's CPUID instruction. */
> +    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> +        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> +        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> +        if (eax >= 0xC0000001) {
> +            /* Support VIA max extended level */
> +            x86_cpu_def->xlevel2 = eax;
> +            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
> +            x86_cpu_def->ext4_features =
> +                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
> +        }
> +    }
> +
> +    /* Other KVM-specific feature fields: */
> +    x86_cpu_def->svm_features =
> +        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
> +    x86_cpu_def->kvm_features =
> +        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
> +}
> +
>  int kvm_arch_init(KVMState *s)
>  {
>      QemuOptsList *list = qemu_find_opts("machine");
> @@ -797,6 +871,11 @@ int kvm_arch_init(KVMState *s)
>              }
>          }
>      }
> +
> +    if (kvm_host_cpu_needs_class_init) {
> +        kvm_host_cpu_class_init(object_class_by_name(TYPE_HOST_X86_CPU),
> NULL);
> +    }
kvm_host_cpu_needs_class_init is set to true in kvm_host_cpu_class_init()
so block is nop.
Why kvm_host_cpu_needs_class_init is needed anyway, why not just call
kvm_host_cpu_class_init() here?


> +
>      return 0;
>  }
>  
> @@ -2330,3 +2409,17 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t
> dev_id) return kvm_deassign_irq_internal(s, dev_id, KVM_DEV_IRQ_GUEST_MSIX |
>                                                  KVM_DEV_IRQ_HOST_MSIX);
>  }
> +
> +static const TypeInfo host_x86_cpu_type_info = {
> +    .name = TYPE_HOST_X86_CPU,
> +    .parent = TYPE_X86_CPU,
> +    .instance_init = kvm_host_cpu_initfn,
> +    .class_init = kvm_host_cpu_class_init,
> +};
> +
> +static void kvm_register_types(void)
> +{
> +    type_register_static(&host_x86_cpu_type_info);
> +}
> +
> +type_init(kvm_register_types)

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

Andreas Färber Feb. 4, 2013, 12:52 p.m. UTC | #2

Am 04.02.2013 12:08, schrieb Igor Mammedov:
> On Sat,  2 Feb 2013 01:37:07 +0100
> Andreas Färber <[e-mail]> wrote:
> 
>> Move x86_def_t definition to header and embed into X86CPUClass.
>> Register types per built-in model definition.
>>
>> Move version initialization from x86_cpudef_setup() to class_init.
>>
>> Inline cpu_x86_register() into the X86CPU initfn.
>> Since instance_init cannot reports errors, drop error handling.
>>
>> Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
>> Move KVM host vendor override from cpu_x86_find_by_name() to the initfn.
>>
>> Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs.
>> Make kvm_cpu_fill_host() a class_init and inline cpu_x86_fill_model_id().
>>
>> Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
>> comparison.
>>
>> Signed-off-by: Andreas Färber <[e-mail]>
>> ---
>>  target-i386/cpu-qom.h |   24 ++++
>>  target-i386/cpu.c     |  324
>> +++++++++++++++++-------------------------------- target-i386/cpu.h
>> |    2 - target-i386/kvm.c     |   93 ++++++++++++++
>>  4 Dateien geändert, 228 Zeilen hinzugefügt(+), 215 Zeilen entfernt(-)
>>
>> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
>> index 48e6b54..80bf72d 100644
>> --- a/target-i386/cpu-qom.h
>> +++ b/target-i386/cpu-qom.h
>> @@ -30,6 +30,27 @@
>>  #define TYPE_X86_CPU "i386-cpu"
>>  #endif
>>  
>> +#define TYPE_HOST_X86_CPU "host-" TYPE_X86_CPU
>> +
>> +typedef struct x86_def_t {
>> +    const char *name;
>> +    uint32_t level;
>> +    /* vendor is zero-terminated, 12 character ASCII string */
>> +    char vendor[CPUID_VENDOR_SZ + 1];
>> +    int family;
>> +    int model;
>> +    int stepping;
>> +    uint32_t features, ext_features, ext2_features, ext3_features;
>> +    uint32_t kvm_features, svm_features;
>> +    uint32_t xlevel;
>> +    char model_id[48];
>> +    /* Store the results of Centaur's CPUID instructions */
>> +    uint32_t ext4_features;
>> +    uint32_t xlevel2;
>> +    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
>> +    uint32_t cpuid_7_0_ebx_features;
>> +} x86_def_t;
>> +
>>  #define X86_CPU_CLASS(klass) \
>>      OBJECT_CLASS_CHECK(X86CPUClass, (klass), TYPE_X86_CPU)
>>  #define X86_CPU(obj) \
>> @@ -41,6 +62,7 @@
>>   * X86CPUClass:
>>   * @parent_realize: The parent class' realize handler.
>>   * @parent_reset: The parent class' reset handler.
>> + * @info: Model-specific data.
>>   *
>>   * An x86 CPU model or family.
>>   */
>> @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
>>  
>>      DeviceRealize parent_realize;
>>      void (*parent_reset)(CPUState *cpu);
>> +
>> +    x86_def_t info;
> Is it necessary to embed it here, could pointer to corresponding static array
> be used here?
> That way one could avoid extra memcpy in class_init().

That would require an allocation for -cpu host, which is undesired.
x86_def_t is supposed to die, forgot to add a TODO comment like on ppc.
Patches to do that are being being polished on qom-cpu-ppc.

>>  } X86CPUClass;
>>  
>>  /**
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index ee2fd6b..6c95740 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -346,25 +346,6 @@ static void add_flagname_to_bitmaps(const char
>> *flagname, }
>>  }
>>  
>> -typedef struct x86_def_t {
>> -    const char *name;
>> -    uint32_t level;
>> -    /* vendor is zero-terminated, 12 character ASCII string */
>> -    char vendor[CPUID_VENDOR_SZ + 1];
>> -    int family;
>> -    int model;
>> -    int stepping;
>> -    uint32_t features, ext_features, ext2_features, ext3_features;
>> -    uint32_t kvm_features, svm_features;
>> -    uint32_t xlevel;
>> -    char model_id[48];
>> -    /* Store the results of Centaur's CPUID instructions */
>> -    uint32_t ext4_features;
>> -    uint32_t xlevel2;
>> -    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
>> -    uint32_t cpuid_7_0_ebx_features;
>> -} x86_def_t;
>> -
>>  #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
>>  #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
>>            CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
>> @@ -868,86 +849,6 @@ static x86_def_t builtin_x86_defs[] = {
>>      },
>>  };
>>  
>> -#ifdef CONFIG_KVM
>> -static int cpu_x86_fill_model_id(char *str)
>> -{
>> -    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
>> -    int i;
>> -
>> -    for (i = 0; i < 3; i++) {
>> -        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
>> -        memcpy(str + i * 16 +  0, &eax, 4);
>> -        memcpy(str + i * 16 +  4, &ebx, 4);
>> -        memcpy(str + i * 16 +  8, &ecx, 4);
>> -        memcpy(str + i * 16 + 12, &edx, 4);
>> -    }
>> -    return 0;
>> -}
>> -#endif
>> -
>> -/* Fill a x86_def_t struct with information about the host CPU, and
>> - * the CPU features supported by the host hardware + host kernel
>> - *
>> - * This function may be called only if KVM is enabled.
>> - */
>> -static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>> -{
>> -#ifdef CONFIG_KVM
>> -    KVMState *s = kvm_state;
>> -    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
>> -
>> -    assert(kvm_enabled());
>> -
>> -    x86_cpu_def->name = "host";
>> -    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
>> -    x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
>> -
>> -    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
>> -    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
>> -    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
>> -    x86_cpu_def->stepping = eax & 0x0F;
>> -
>> -    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
>> -    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
>> -    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
>> R_ECX); -
>> -    if (x86_cpu_def->level >= 7) {
>> -        x86_cpu_def->cpuid_7_0_ebx_features =
>> -                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
>> -    } else {
>> -        x86_cpu_def->cpuid_7_0_ebx_features = 0;
>> -    }
>> -
>> -    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0,
>> R_EAX);
>> -    x86_cpu_def->ext2_features =
>> -                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
>> -    x86_cpu_def->ext3_features =
>> -                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
>> -
>> -    cpu_x86_fill_model_id(x86_cpu_def->model_id);
>> -
>> -    /* Call Centaur's CPUID instruction. */
>> -    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
>> -        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
>> -        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>> -        if (eax >= 0xC0000001) {
>> -            /* Support VIA max extended level */
>> -            x86_cpu_def->xlevel2 = eax;
>> -            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
>> -            x86_cpu_def->ext4_features =
>> -                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
>> -        }
>> -    }
>> -
>> -    /* Other KVM-specific feature fields: */
>> -    x86_cpu_def->svm_features =
>> -        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
>> -    x86_cpu_def->kvm_features =
>> -        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
>> -
>> -#endif /* CONFIG_KVM */
>> -}
>> -
>>  static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
>>  {
>>      int i;
>> @@ -975,31 +876,31 @@ static int unavailable_host_feature(FeatureWordInfo
>> *f, uint32_t mask) static int kvm_check_features_against_host(X86CPU *cpu)
>>  {
>>      CPUX86State *env = &cpu->env;
>> -    x86_def_t host_def;
>> +    ObjectClass *host_oc = object_class_by_name(TYPE_HOST_X86_CPU);
>> +    X86CPUClass *host_xcc = X86_CPU_CLASS(host_oc);
>>      uint32_t mask;
>>      int rv, i;
>>      struct model_features_t ft[] = {
>> -        {&env->cpuid_features, &host_def.features,
>> +        {&env->cpuid_features, &host_xcc->info.features,
>>              FEAT_1_EDX },
>> -        {&env->cpuid_ext_features, &host_def.ext_features,
>> +        {&env->cpuid_ext_features, &host_xcc->info.ext_features,
>>              FEAT_1_ECX },
>> -        {&env->cpuid_ext2_features, &host_def.ext2_features,
>> +        {&env->cpuid_ext2_features, &host_xcc->info.ext2_features,
>>              FEAT_8000_0001_EDX },
>> -        {&env->cpuid_ext3_features, &host_def.ext3_features,
>> +        {&env->cpuid_ext3_features, &host_xcc->info.ext3_features,
>>              FEAT_8000_0001_ECX },
>> -        {&env->cpuid_ext4_features, &host_def.ext4_features,
>> +        {&env->cpuid_ext4_features, &host_xcc->info.ext4_features,
>>              FEAT_C000_0001_EDX },
>> -        {&env->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
>> +        {&env->cpuid_7_0_ebx_features,
>> &host_xcc->info.cpuid_7_0_ebx_features, FEAT_7_0_EBX },
>> -        {&env->cpuid_svm_features, &host_def.svm_features,
>> +        {&env->cpuid_svm_features, &host_xcc->info.svm_features,
>>              FEAT_SVM },
>> -        {&env->cpuid_kvm_features, &host_def.kvm_features,
>> +        {&env->cpuid_kvm_features, &host_xcc->info.kvm_features,
>>              FEAT_KVM },
>>      };
>>  
>>      assert(kvm_enabled());
>>  
>> -    kvm_cpu_fill_host(&host_def);
>>      for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) {
>>          FeatureWord w = ft[i].feat_word;
>>          FeatureWordInfo *wi = &feature_word_info[w];
>> @@ -1261,40 +1162,30 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
>> Visitor *v, void *opaque, cpu->env.tsc_khz = value / 1000;
>>  }
>>  
>> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>> +static ObjectClass *x86_cpu_class_by_name(const char *name)
>>  {
>> -    x86_def_t *def;
>> -    int i;
>> +    ObjectClass *oc;
>> +    char *typename;
>>  
>>      if (name == NULL) {
>> -        return -1;
>> -    }
>> -    if (kvm_enabled() && strcmp(name, "host") == 0) {
>> -        kvm_cpu_fill_host(x86_cpu_def);
>> -        return 0;
>> +        return NULL;
>>      }
>>  
>> -    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
>> -        def = &builtin_x86_defs[i];
>> -        if (strcmp(name, def->name) == 0) {
>> -            memcpy(x86_cpu_def, def, sizeof(*def));
>> -            /* sysenter isn't supported in compatibility mode on AMD,
>> -             * syscall isn't supported in compatibility mode on Intel.
>> -             * Normally we advertise the actual CPU vendor, but you can
>> -             * override this using the 'vendor' property if you want to use
>> -             * KVM's sysenter/syscall emulation in compatibility mode and
>> -             * when doing cross vendor migration
>> -             */
>> -            if (kvm_enabled()) {
>> -                uint32_t  ebx = 0, ecx = 0, edx = 0;
>> -                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
>> -                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx,
>> ecx);
>> -            }
>> -            return 0;
>> +    if (strcmp(name, "host") == 0) {
>> +        if (kvm_enabled()) {
>> +            return object_class_by_name(TYPE_HOST_X86_CPU);
>>          }
>> +        return NULL;
>>      }
> block is not necessary, the following block could yield the same result.

No, it doesn't in the !kvm_enabled() case. I tripped over this when I
left the kvm_enabled() check where it was before, falling back to the
below lookup, leading to -cpu host being instantiated for TCG and
originally not asserting (that's why I added the -cpu host initfn).

Symptoms were x86_64 openSUSE .iso claiming no support for 486(?)
processors.

>>  
>> -    return -1;
>> +    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
>> +    oc = object_class_by_name(typename);
>> +    g_free(typename);
>> +    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_X86_CPU) ||
>> +                       object_class_is_abstract(oc))) {
>> +        oc = NULL;
>> +    }
>> +    return oc;
>>  }
>>  
>>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>> @@ -1516,60 +1407,13 @@ static void filter_features_for_kvm(X86CPU *cpu)
>>  }
>>  #endif
>>  
>> -static int cpu_x86_register(X86CPU *cpu, const char *name)
>> -{
>> -    CPUX86State *env = &cpu->env;
>> -    x86_def_t def1, *def = &def1;
>> -    Error *error = NULL;
>> -
>> -    memset(def, 0, sizeof(*def));
>> -
>> -    if (cpu_x86_find_by_name(def, name) < 0) {
>> -        error_setg(&error, "Unable to find CPU definition: %s", name);
>> -        goto out;
>> -    }
>> -
>> -    if (kvm_enabled()) {
>> -        def->kvm_features |= kvm_default_features;
>> -    }
>> -    def->ext_features |= CPUID_EXT_HYPERVISOR;
>> -
>> -    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
>> -    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
>> -    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
>> -    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
>> -    object_property_set_int(OBJECT(cpu), def->stepping, "stepping",
>> &error);
>> -    env->cpuid_features = def->features;
>> -    env->cpuid_ext_features = def->ext_features;
>> -    env->cpuid_ext2_features = def->ext2_features;
>> -    env->cpuid_ext3_features = def->ext3_features;
>> -    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
>> -    env->cpuid_kvm_features = def->kvm_features;
>> -    env->cpuid_svm_features = def->svm_features;
>> -    env->cpuid_ext4_features = def->ext4_features;
>> -    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
>> -    env->cpuid_xlevel2 = def->xlevel2;
>> -
>> -    object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
>> &error);
>> -    if (error) {
>> -        goto out;
>> -    }
>> -
>> -out:
>> -    if (error) {
>> -        fprintf(stderr, "%s\n", error_get_pretty(error));
>> -        error_free(error);
>> -        return -1;
>> -    }
>> -    return 0;
>> -}
>> -
>>  X86CPU *cpu_x86_init(const char *cpu_model)
>>  {
>>      X86CPU *cpu = NULL;
>>      CPUX86State *env;
>>      gchar **model_pieces;
>>      char *name, *features;
>> +    ObjectClass *oc;
>>      Error *error = NULL;
>>  
>>      model_pieces = g_strsplit(cpu_model, ",", 2);
>> @@ -1580,13 +1424,13 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>      name = model_pieces[0];
>>      features = model_pieces[1];
>>  
>> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
>> -    env = &cpu->env;
>> -    env->cpu_model_str = cpu_model;
>> -
>> -    if (cpu_x86_register(cpu, name) < 0) {
>> +    oc = x86_cpu_class_by_name(name);
>> +    if (oc == NULL) {
> make sure error is reported when cpu is not found:
>            error_setg(&error, "Can't find CPU: %s", name);

Why? The callers do that when NULL is returned.

>>          goto error;
>>      }
>> +    cpu = X86_CPU(object_new(object_class_get_name(oc)));
>> +    env = &cpu->env;
>> +    env->cpu_model_str = cpu_model;
>>  
>>      cpu_x86_parse_featurestr(cpu, features, &error);
>>      if (error) {
>> @@ -1621,30 +1465,6 @@ void cpu_clear_apic_feature(CPUX86State *env)
>>  
>>  #endif /* !CONFIG_USER_ONLY */
>>  
>> -/* Initialize list of CPU models, filling some non-static fields if
>> necessary
>> - */
>> -void x86_cpudef_setup(void)
>> -{
>> -    int i, j;
>> -    static const char *model_with_versions[] = { "qemu32", "qemu64",
>> "athlon" }; -
>> -    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
>> -        x86_def_t *def = &builtin_x86_defs[i];
>> -
>> -        /* Look for specific "cpudef" models that */
>> -        /* have the QEMU version in .model_id */
>> -        for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) {
>> -            if (strcmp(model_with_versions[j], def->name) == 0) {
>> -                pstrcpy(def->model_id, sizeof(def->model_id),
>> -                        "QEMU Virtual CPU version ");
>> -                pstrcat(def->model_id, sizeof(def->model_id),
>> -                        qemu_get_version());
>> -                break;
>> -            }
>> -        }
>> -    }
>> -}
>> -
>>  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>>                               uint32_t *ecx, uint32_t *edx)
>>  {
>> @@ -2198,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
>>      CPUState *cs = CPU(obj);
>>      X86CPU *cpu = X86_CPU(obj);
>>      CPUX86State *env = &cpu->env;
>> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
>> +    const x86_def_t *def = &xcc->info;
>>      static int inited;
>>  
>>      cpu_exec_init(env);
>> @@ -2227,6 +2049,41 @@ static void x86_cpu_initfn(Object *obj)
>>                          x86_cpuid_get_tsc_freq,
>>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
>>  
>> +    /* sysenter isn't supported in compatibility mode on AMD,
>> +     * syscall isn't supported in compatibility mode on Intel.
>> +     * Normally we advertise the actual CPU vendor, but you can
>> +     * override this using the 'vendor' property if you want to use
>> +     * KVM's sysenter/syscall emulation in compatibility mode and
>> +     * when doing cross vendor migration
>> +     */
>> +    if (kvm_enabled()) {
>> +        host_cpuid(0, 0, NULL, &env->cpuid_vendor1, &env->cpuid_vendor2,
>> +                   &env->cpuid_vendor3);
> This is not per instance specific, this override would be better done to
> sub-classes in kvm_arch_init().

Hmm... I think the issue I addresses this way was that kvm.c didn't have
access to your static vendor_words2str() helper. Writing the words into
the word fields is more direct than converting them to a string and
writing them back into the words.

> As bonus we would get actual 'vendor' when
> doing class introspection provided it's done after kvm_init() is called.

Doing that would mean that we force class_init of every CPU class to run
every time we start with KVM enabled. Is there any practical downside to
doing it at instance_init time? Properties are set afterwards, so it
doesn't override anything. Where would we want to inspect an X86CPUClass
to have an AMD model say Intel or vice versa? -cpu ? doesn't look at it
today, nor does QMP.

>> +    } else {
>> +        object_property_set_str(OBJECT(cpu), def->vendor, "vendor", NULL);
>> +    }
>> +
>> +    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
>> +    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
>> +    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
>> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", NULL);
>> +    env->cpuid_features = def->features;
>> +    env->cpuid_ext_features = def->ext_features;
>> +    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
>> +    env->cpuid_ext2_features = def->ext2_features;
>> +    env->cpuid_ext3_features = def->ext3_features;
>> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
>> +    env->cpuid_kvm_features = def->kvm_features;
>> +    if (kvm_enabled()) {
>> +        env->cpuid_kvm_features |= kvm_default_features;
>> +    }
>> +    env->cpuid_svm_features = def->svm_features;
>> +    env->cpuid_ext4_features = def->ext4_features;
>> +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
>> +    env->cpuid_xlevel2 = def->xlevel2;
>> +
>> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", NULL);
> All this feature initialization in initfn() is not compatible with
> static/global properties because they are set in device_initfn(). But I can
> remove properties/features from here as they are converted to static
> properties and incrementally move their defaults initialization into
> new x86_cpu_def_class_init().

Would it help to call the old registration function from here? Or what
exactly would you need on top?

>> +
>>      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>>  
>>      /* init various static tables used in TCG mode */
>> @@ -2239,6 +2096,27 @@ static void x86_cpu_initfn(Object *obj)
>>      }
>>  }
>>  
>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
>> +{
>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
>> +    x86_def_t *def = data;
>> +    int i;
>> +    static const char *versioned_models[] = { "qemu32", "qemu64",
>> "athlon" }; +
>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> perhaps sizeof(xcc->info) would be better?

OK.

>> +
>> +    /* Look for specific models that have the QEMU version in .model_id */
>> +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
>> +        if (strcmp(versioned_models[i], def->name) == 0) {
>> +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
>> +                    "QEMU Virtual CPU version ");
>> +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
>> +                    qemu_get_version());
>> +            break;
>> +        }
>> +    }
>> +}
>> +
>>  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>>  {
>>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
>> @@ -2250,6 +2128,21 @@ static void x86_cpu_common_class_init(ObjectClass
>> *oc, void *data) 
>>      xcc->parent_reset = cc->reset;
>>      cc->reset = x86_cpu_reset;
>> +
>> +    cc->class_by_name = x86_cpu_class_by_name;
>> +}
>> +
>> +static void x86_register_cpu_type(const x86_def_t *def)
>> +{
>> +    TypeInfo type_info = {
>> +        .parent = TYPE_X86_CPU,
>> +        .class_init = x86_cpu_def_class_init,
>> +        .class_data = (void *)def,
>> +    };
>> +
>> +    type_info.name = g_strdup_printf("%s-" TYPE_X86_CPU, def->name);
>> +    type_register(&type_info);
>> +    g_free((void *)type_info.name);
>>  }
>>  
>>  static const TypeInfo x86_cpu_type_info = {
>> @@ -2257,14 +2150,19 @@ static const TypeInfo x86_cpu_type_info = {
>>      .parent = TYPE_CPU,
>>      .instance_size = sizeof(X86CPU),
>>      .instance_init = x86_cpu_initfn,
>> -    .abstract = false,
>> +    .abstract = true,
>>      .class_size = sizeof(X86CPUClass),
>>      .class_init = x86_cpu_common_class_init,
>>  };
>>  
>>  static void x86_cpu_register_types(void)
>>  {
>> +    int i;
>> +
>>      type_register_static(&x86_cpu_type_info);
>> +    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
>> +        x86_register_cpu_type(&builtin_x86_defs[i]);
>> +    }
>>  }
>>  
>>  type_init(x86_cpu_register_types)
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 7577e4f..2aef7b7 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -887,7 +887,6 @@ typedef struct CPUX86State {
>>  X86CPU *cpu_x86_init(const char *cpu_model);
>>  int cpu_x86_exec(CPUX86State *s);
>>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>> -void x86_cpudef_setup(void);
>>  int cpu_x86_support_mca_broadcast(CPUX86State *env);
>>  
>>  int cpu_get_pic_interrupt(CPUX86State *s);
>> @@ -1079,7 +1078,6 @@ static inline CPUX86State *cpu_init(const char
>> *cpu_model) #define cpu_gen_code cpu_x86_gen_code
>>  #define cpu_signal_handler cpu_x86_signal_handler
>>  #define cpu_list x86_cpu_list
>> -#define cpudef_setup	x86_cpudef_setup
>>  
>>  #define CPU_SAVE_VERSION 12
>>  
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 9ebf181..dbfa670 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -735,6 +735,80 @@ static int kvm_get_supported_msrs(KVMState *s)
>>      return ret;
>>  }
>>  
>> +static void kvm_host_cpu_initfn(Object *obj)
>> +{
>> +    assert(kvm_enabled());
>> +}
>> +
>> +static bool kvm_host_cpu_needs_class_init;
>> +
>> +static void kvm_host_cpu_class_init(ObjectClass *oc, void *data)
>> +{
>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
>> +    x86_def_t *x86_cpu_def = &xcc->info;
>> +    KVMState *s = kvm_state;
>> +    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
>> +    int i;
>> +
>> +    if (!kvm_enabled()) {
>> +        kvm_host_cpu_needs_class_init = true;
>> +        return;
>> +    }
>> +
>> +    xcc->info.name = "host";
>> +
>> +    /* Vendor will be set in initfn if kvm_enabled() */
>> +
>> +    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
>> +    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
>> +    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
>> +    x86_cpu_def->stepping = eax & 0x0F;
>> +
>> +    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
>> +    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
>> +    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
>> R_ECX); +
>> +    if (x86_cpu_def->level >= 7) {
>> +        x86_cpu_def->cpuid_7_0_ebx_features =
>> +                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
>> +    } else {
>> +        x86_cpu_def->cpuid_7_0_ebx_features = 0;
>> +    }
>> +
>> +    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0,
>> R_EAX);
>> +    x86_cpu_def->ext2_features =
>> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
>> +    x86_cpu_def->ext3_features =
>> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
>> +
>> +    for (i = 0; i < 3; i++) {
>> +        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
>> +        memcpy(xcc->info.model_id + i * 16 +  0, &eax, 4);
>> +        memcpy(xcc->info.model_id + i * 16 +  4, &ebx, 4);
>> +        memcpy(xcc->info.model_id + i * 16 +  8, &ecx, 4);
>> +        memcpy(xcc->info.model_id + i * 16 + 12, &edx, 4);
>> +    }
>> +
>> +    /* Call Centaur's CPUID instruction. */
>> +    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
>> +        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
>> +        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>> +        if (eax >= 0xC0000001) {
>> +            /* Support VIA max extended level */
>> +            x86_cpu_def->xlevel2 = eax;
>> +            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
>> +            x86_cpu_def->ext4_features =
>> +                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
>> +        }
>> +    }
>> +
>> +    /* Other KVM-specific feature fields: */
>> +    x86_cpu_def->svm_features =
>> +        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
>> +    x86_cpu_def->kvm_features =
>> +        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
>> +}
>> +
>>  int kvm_arch_init(KVMState *s)
>>  {
>>      QemuOptsList *list = qemu_find_opts("machine");
>> @@ -797,6 +871,11 @@ int kvm_arch_init(KVMState *s)
>>              }
>>          }
>>      }
>> +
>> +    if (kvm_host_cpu_needs_class_init) {
>> +        kvm_host_cpu_class_init(object_class_by_name(TYPE_HOST_X86_CPU),
>> NULL);
>> +    }
> kvm_host_cpu_needs_class_init is set to true in kvm_host_cpu_class_init()
> so block is nop.

Nope. It's a question of initialization ordering:

> Why kvm_host_cpu_needs_class_init is needed anyway, why not just call
> kvm_host_cpu_class_init() here?

i) class_init may be called before kvm_enabled() evaluates to true, such
as for -cpu ?, so we need to handle that case gracefully.

ii) The regular case is that the class_init is run in response to
object_new() from pc.c, in which case kvm_enabled() evaluates to true
and the needs_class_init remains false.

Thus, both cases are used in practice.

Regards,
Andreas

>> +
>>      return 0;
>>  }
>>  
>> @@ -2330,3 +2409,17 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t
>> dev_id) return kvm_deassign_irq_internal(s, dev_id, KVM_DEV_IRQ_GUEST_MSIX |
>>                                                  KVM_DEV_IRQ_HOST_MSIX);
>>  }
>> +
>> +static const TypeInfo host_x86_cpu_type_info = {
>> +    .name = TYPE_HOST_X86_CPU,
>> +    .parent = TYPE_X86_CPU,
>> +    .instance_init = kvm_host_cpu_initfn,
>> +    .class_init = kvm_host_cpu_class_init,
>> +};
>> +
>> +static void kvm_register_types(void)
>> +{
>> +    type_register_static(&host_x86_cpu_type_info);
>> +}
>> +
>> +type_init(kvm_register_types)

Igor Mammedov Feb. 4, 2013, 4:05 p.m. UTC | #3

On Mon, 04 Feb 2013 13:52:32 +0100
Andreas Färber <[e-mail]> wrote:

> Am 04.02.2013 12:08, schrieb Igor Mammedov:
> > On Sat,  2 Feb 2013 01:37:07 +0100
> > Andreas Färber <[e-mail]> wrote:
> > 
> >> Move x86_def_t definition to header and embed into X86CPUClass.
> >> Register types per built-in model definition.
> >>
> >> Move version initialization from x86_cpudef_setup() to class_init.
> >>
> >> Inline cpu_x86_register() into the X86CPU initfn.
> >> Since instance_init cannot reports errors, drop error handling.
> >>
> >> Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
> >> Move KVM host vendor override from cpu_x86_find_by_name() to the initfn.
> >>
> >> Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs.
> >> Make kvm_cpu_fill_host() a class_init and inline cpu_x86_fill_model_id().
> >>
> >> Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
> >> comparison.
> >>
> >> Signed-off-by: Andreas Färber <[e-mail]>
> >> ---
> >>  target-i386/cpu-qom.h |   24 ++++
> >>  target-i386/cpu.c     |  324
> >> +++++++++++++++++-------------------------------- target-i386/cpu.h
> >> |    2 - target-i386/kvm.c     |   93 ++++++++++++++
> >>  4 Dateien geändert, 228 Zeilen hinzugefügt(+), 215 Zeilen entfernt(-)
> >>
> >> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> >> index 48e6b54..80bf72d 100644
> >> --- a/target-i386/cpu-qom.h
> >> +++ b/target-i386/cpu-qom.h
[...]

> >>   * An x86 CPU model or family.
> >>   */
> >> @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
> >>  
> >>      DeviceRealize parent_realize;
> >>      void (*parent_reset)(CPUState *cpu);
> >> +
> >> +    x86_def_t info;
> > Is it necessary to embed it here, could pointer to corresponding static
> > array be used here?
> > That way one could avoid extra memcpy in class_init().
> 
> That would require an allocation for -cpu host, which is undesired.
not necessary, it could be local static x86_def_t in target-i386/kvm.c in
host class_init(). Any way change is cosmetic and I do not insist on it.

> x86_def_t is supposed to die, forgot to add a TODO comment like on ppc.
> Patches to do that are being being polished on qom-cpu-ppc.
+1

> 
> >>  } X86CPUClass;
> >>  
> >>  /**
[...]

> >> *name) +static ObjectClass *x86_cpu_class_by_name(const char *name)
> >>  {
> >> -    x86_def_t *def;
> >> -    int i;
> >> +    ObjectClass *oc;
> >> +    char *typename;
> >>  
> >>      if (name == NULL) {
> >> -        return -1;
> >> -    }
> >> -    if (kvm_enabled() && strcmp(name, "host") == 0) {
> >> -        kvm_cpu_fill_host(x86_cpu_def);
> >> -        return 0;
> >> +        return NULL;
> >>      }
> >>  
> >> -    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> >> -        def = &builtin_x86_defs[i];
> >> -        if (strcmp(name, def->name) == 0) {
> >> -            memcpy(x86_cpu_def, def, sizeof(*def));
> >> -            /* sysenter isn't supported in compatibility mode on AMD,
> >> -             * syscall isn't supported in compatibility mode on Intel.
> >> -             * Normally we advertise the actual CPU vendor, but you can
> >> -             * override this using the 'vendor' property if you want to
> >> use
> >> -             * KVM's sysenter/syscall emulation in compatibility mode
> >> and
> >> -             * when doing cross vendor migration
> >> -             */
> >> -            if (kvm_enabled()) {
> >> -                uint32_t  ebx = 0, ecx = 0, edx = 0;
> >> -                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> >> -                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx,
> >> ecx);
> >> -            }
> >> -            return 0;
> >> +    if (strcmp(name, "host") == 0) {
> >> +        if (kvm_enabled()) {
> >> +            return object_class_by_name(TYPE_HOST_X86_CPU);
> >>          }
> >> +        return NULL;
> >>      }
> > block is not necessary, the following block could yield the same result.
> 
> No, it doesn't in the !kvm_enabled() case. I tripped over this when I
> left the kvm_enabled() check where it was before, falling back to the
> below lookup, leading to -cpu host being instantiated for TCG and
> originally not asserting (that's why I added the -cpu host initfn).
Maybe TYPE_HOST_X86_CPU shouldn't be registered at all if kvm_enabled() == 0.

> 
> Symptoms were x86_64 openSUSE .iso claiming no support for 486(?)
> processors.
> 
> >>  
> >> -    return -1;
> >> +    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
> >> +    oc = object_class_by_name(typename);
> >> +    g_free(typename);
> >> +    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_X86_CPU) ||
> >> +                       object_class_is_abstract(oc))) {
> >> +        oc = NULL;
> >> +    }
> >> +    return oc;
> >>  }
> >>  
[...]

> >> @@ -1580,13 +1424,13 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> >>      name = model_pieces[0];
> >>      features = model_pieces[1];
> >>  
> >> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> >> -    env = &cpu->env;
> >> -    env->cpu_model_str = cpu_model;
> >> -
> >> -    if (cpu_x86_register(cpu, name) < 0) {
> >> +    oc = x86_cpu_class_by_name(name);
> >> +    if (oc == NULL) {
> > make sure error is reported when cpu is not found:
> >            error_setg(&error, "Can't find CPU: %s", name);
> 
> Why? The callers do that when NULL is returned.
The callers do not exactly know why NULL is returned, wouldn't it better
to specify error message at the point of origin.

> 
> >>          goto error;
> >>      }
> >> +    cpu = X86_CPU(object_new(object_class_get_name(oc)));
> >> +    env = &cpu->env;
> >> +    env->cpu_model_str = cpu_model;
> >>  
> >>      cpu_x86_parse_featurestr(cpu, features, &error);
> >>      if (error) {
[...]

> >> @@ -2198,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
> >>      CPUState *cs = CPU(obj);
> >>      X86CPU *cpu = X86_CPU(obj);
> >>      CPUX86State *env = &cpu->env;
> >> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
> >> +    const x86_def_t *def = &xcc->info;
> >>      static int inited;
> >>  
> >>      cpu_exec_init(env);
> >> @@ -2227,6 +2049,41 @@ static void x86_cpu_initfn(Object *obj)
> >>                          x86_cpuid_get_tsc_freq,
> >>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> >>  
> >> +    /* sysenter isn't supported in compatibility mode on AMD,
> >> +     * syscall isn't supported in compatibility mode on Intel.
> >> +     * Normally we advertise the actual CPU vendor, but you can
> >> +     * override this using the 'vendor' property if you want to use
> >> +     * KVM's sysenter/syscall emulation in compatibility mode and
> >> +     * when doing cross vendor migration
> >> +     */
> >> +    if (kvm_enabled()) {
> >> +        host_cpuid(0, 0, NULL, &env->cpuid_vendor1, &env->cpuid_vendor2,
> >> +                   &env->cpuid_vendor3);
> > This is not per instance specific, this override would be better done to
> > sub-classes in kvm_arch_init().
> 
> Hmm... I think the issue I addresses this way was that kvm.c didn't have
> access to your static vendor_words2str() helper. Writing the words into
> the word fields is more direct than converting them to a string and
> writing them back into the words.
you have these correct vendor words ready after you called
kvm_host_cpu_class_init() in kvm_arch_init(), so just direct copy from host
class would do.

> 
> > As bonus we would get actual 'vendor' when
> > doing class introspection provided it's done after kvm_init() is called.
> 
> Doing that would mean that we force class_init of every CPU class to run
> every time we start with KVM enabled.
I'd ask if there is any particular downside in calling every class_init()?

> Is there any practical downside to
> doing it at instance_init time? Properties are set afterwards, so it
> doesn't override anything. Where would we want to inspect an X86CPUClass
> to have an AMD model say Intel or vice versa? -cpu ? doesn't look at it
> today, nor does QMP.
Presently nothing changes if we do it in kvm_init() or initfn() time since
none of users displays actual vendor value, but it looks more like a bug. 

Downside of doing it initfn() is that static properties defaults and global
properties are set by device_initfn() before cpu_initfn() is called, so
it renders all that was set via it overridden by a later x86_cpu_initfn().

So if there is no really big drawback in calling every x86 cpu class_init(),
I'd prefer to utilize features provided by static properties, than hacking
class wide vendor override per each cpu instance.

As alternative x86 cpu could have a hook that is called from
x86_cpu_class_init() that does kvm specific overrides and set by
kvm_arch_init(). Then there is no need to initialize all types in
kvm_arch_init(). The problem with it is if class is initialized before
kvm_init(), then override won't be applied ever.

> 
> >> +    } else {
> >> +        object_property_set_str(OBJECT(cpu), def->vendor, "vendor",
> >> NULL);
> >> +    }
> >> +
> >> +    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
> >> +    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
> >> +    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
> >> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping",
> >> NULL);
> >> +    env->cpuid_features = def->features;
> >> +    env->cpuid_ext_features = def->ext_features;
> >> +    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
> >> +    env->cpuid_ext2_features = def->ext2_features;
> >> +    env->cpuid_ext3_features = def->ext3_features;
> >> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
> >> +    env->cpuid_kvm_features = def->kvm_features;
> >> +    if (kvm_enabled()) {
> >> +        env->cpuid_kvm_features |= kvm_default_features;
> >> +    }
> >> +    env->cpuid_svm_features = def->svm_features;
> >> +    env->cpuid_ext4_features = def->ext4_features;
> >> +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> >> +    env->cpuid_xlevel2 = def->xlevel2;
> >> +
> >> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
> >> NULL);
> > All this feature initialization in initfn() is not compatible with
> > static/global properties because they are set in device_initfn(). But I
> > can remove properties/features from here as they are converted to static
> > properties and incrementally move their defaults initialization into
> > new x86_cpu_def_class_init().
> 
> Would it help to call the old registration function from here? Or what
> exactly would you need on top?
Nope, see previous answer to why setting defaults here is not desired.
After conversion, defaults will be set via static properties defaults and then
on top of it, global properties will be applied in device_initfn().
Eventually -cpu xxx,foo should set global properties for cpu type and cpu
type name for machine to use. no manual property setting between object_new()
and realize() unless it is instance specific.

[...]
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index 9ebf181..dbfa670 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -735,6 +735,80 @@ static int kvm_get_supported_msrs(KVMState *s)
> >>      return ret;
> >>  }
> >>  
> >> +static void kvm_host_cpu_initfn(Object *obj)
> >> +{
> >> +    assert(kvm_enabled());
> >> +}
> >> +
> >> +static bool kvm_host_cpu_needs_class_init;
> >> +
> >> +static void kvm_host_cpu_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> >> +    x86_def_t *x86_cpu_def = &xcc->info;
> >> +    KVMState *s = kvm_state;
> >> +    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> >> +    int i;
> >> +
> >> +    if (!kvm_enabled()) {
> >> +        kvm_host_cpu_needs_class_init = true;
> >> +        return;
> >> +    }
> >> +
> >> +    xcc->info.name = "host";
> >> +
> >> +    /* Vendor will be set in initfn if kvm_enabled() */
> >> +
> >> +    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
> >> +    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
> >> +    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
> >> +    x86_cpu_def->stepping = eax & 0x0F;
> >> +
> >> +    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> >> +    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
> >> R_EDX);
> >> +    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
> >> R_ECX); +
> >> +    if (x86_cpu_def->level >= 7) {
> >> +        x86_cpu_def->cpuid_7_0_ebx_features =
> >> +                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
> >> +    } else {
> >> +        x86_cpu_def->cpuid_7_0_ebx_features = 0;
> >> +    }
> >> +
> >> +    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0,
> >> R_EAX);
> >> +    x86_cpu_def->ext2_features =
> >> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
> >> +    x86_cpu_def->ext3_features =
> >> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
> >> +
> >> +    for (i = 0; i < 3; i++) {
> >> +        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
> >> +        memcpy(xcc->info.model_id + i * 16 +  0, &eax, 4);
> >> +        memcpy(xcc->info.model_id + i * 16 +  4, &ebx, 4);
> >> +        memcpy(xcc->info.model_id + i * 16 +  8, &ecx, 4);
> >> +        memcpy(xcc->info.model_id + i * 16 + 12, &edx, 4);
> >> +    }
> >> +
> >> +    /* Call Centaur's CPUID instruction. */
> >> +    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> >> +        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> >> +        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> >> +        if (eax >= 0xC0000001) {
> >> +            /* Support VIA max extended level */
> >> +            x86_cpu_def->xlevel2 = eax;
> >> +            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
> >> +            x86_cpu_def->ext4_features =
> >> +                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0,
> >> R_EDX);
> >> +        }
> >> +    }
> >> +
> >> +    /* Other KVM-specific feature fields: */
> >> +    x86_cpu_def->svm_features =
> >> +        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
> >> +    x86_cpu_def->kvm_features =
> >> +        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
> >> +}
> >> +
> >>  int kvm_arch_init(KVMState *s)
> >>  {
> >>      QemuOptsList *list = qemu_find_opts("machine");
> >> @@ -797,6 +871,11 @@ int kvm_arch_init(KVMState *s)
> >>              }
> >>          }
> >>      }
> >> +
> >> +    if (kvm_host_cpu_needs_class_init) {
> >> +        kvm_host_cpu_class_init(object_class_by_name(TYPE_HOST_X86_CPU),
> >> NULL);
> >> +    }
> > kvm_host_cpu_needs_class_init is set to true in kvm_host_cpu_class_init()
> > so block is nop.
> 
> Nope. It's a question of initialization ordering:
Yep, I see now, type_register_static(&host_x86_cpu_type_info) creates
fake(uninitialized) host class and then it is initialized when
kvm_arch_init() is called.

> 
> > Why kvm_host_cpu_needs_class_init is needed anyway, why not just call
> > kvm_host_cpu_class_init() here?
> 
> i) class_init may be called before kvm_enabled() evaluates to true, such
> as for -cpu ?, so we need to handle that case gracefully.
We do not print host cpu type at -cpu ? now, it's implied that host cpu is
available if started with --enable-kvm.

> 
> ii) The regular case is that the class_init is run in response to
> object_new() from pc.c, in which case kvm_enabled() evaluates to true
> and the needs_class_init remains false.
could we register host type in kvm_arch_init() time, and avoid calling
kvm_host_cpu_class_init() twice?
It also would allow to remove host look-up hack in x86_cpu_class_by_name().

> 
> Thus, both cases are used in practice.
> 
> Regards,
> Andreas
> 
> >> +
> >>      return 0;
> >>  }
> >>  
> >> @@ -2330,3 +2409,17 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t
> >> dev_id) return kvm_deassign_irq_internal(s, dev_id,
> >> KVM_DEV_IRQ_GUEST_MSIX | KVM_DEV_IRQ_HOST_MSIX);
> >>  }
> >> +
> >> +static const TypeInfo host_x86_cpu_type_info = {
> >> +    .name = TYPE_HOST_X86_CPU,
> >> +    .parent = TYPE_X86_CPU,
> >> +    .instance_init = kvm_host_cpu_initfn,
> >> +    .class_init = kvm_host_cpu_class_init,
> >> +};
> >> +
> >> +static void kvm_register_types(void)
> >> +{
> >> +    type_register_static(&host_x86_cpu_type_info);
> >> +}
> >> +
> >> +type_init(kvm_register_types)
> 

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

Igor Mammedov Feb. 4, 2013, 8:21 p.m. UTC | #4

On Mon, 4 Feb 2013 17:05:01 +0100
Igor Mammedov <[e-mail]> wrote:

> On Mon, 04 Feb 2013 13:52:32 +0100
> Andreas Färber <[e-mail]> wrote:
> 
> > Am 04.02.2013 12:08, schrieb Igor Mammedov:
> > > On Sat,  2 Feb 2013 01:37:07 +0100
> > > Andreas Färber <[e-mail]> wrote:
> > > 
[...]
> 
> > >> @@ -2198,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
> > >>      CPUState *cs = CPU(obj);
> > >>      X86CPU *cpu = X86_CPU(obj);
> > >>      CPUX86State *env = &cpu->env;
> > >> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
> > >> +    const x86_def_t *def = &xcc->info;
> > >>      static int inited;
> > >>  
> > >>      cpu_exec_init(env);
> > >> @@ -2227,6 +2049,41 @@ static void x86_cpu_initfn(Object *obj)
> > >>                          x86_cpuid_get_tsc_freq,
> > >>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > >>  
> > >> +    /* sysenter isn't supported in compatibility mode on AMD,
> > >> +     * syscall isn't supported in compatibility mode on Intel.
> > >> +     * Normally we advertise the actual CPU vendor, but you can
> > >> +     * override this using the 'vendor' property if you want to use
> > >> +     * KVM's sysenter/syscall emulation in compatibility mode and
> > >> +     * when doing cross vendor migration
> > >> +     */
> > >> +    if (kvm_enabled()) {
> > >> +        host_cpuid(0, 0, NULL, &env->cpuid_vendor1, &env->cpuid_vendor2,
> > >> +                   &env->cpuid_vendor3);
> > > This is not per instance specific, this override would be better done to
> > > sub-classes in kvm_arch_init().
> > 
> > Hmm... I think the issue I addresses this way was that kvm.c didn't have
> > access to your static vendor_words2str() helper. Writing the words into
> > the word fields is more direct than converting them to a string and
> > writing them back into the words.
> you have these correct vendor words ready after you called
> kvm_host_cpu_class_init() in kvm_arch_init(), so just direct copy from host
> class would do.
I was talking nonsense here.
Why not to make vendor_words2str() helper not static and use it in kvm.c
instead of doing partial init of host class and then reuse the value you got
to override vendor in other classes?

[...]

Patch

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 48e6b54..80bf72d 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -30,6 +30,27 @@ 
 #define TYPE_X86_CPU "i386-cpu"
 #endif
 
+#define TYPE_HOST_X86_CPU "host-" TYPE_X86_CPU
+
+typedef struct x86_def_t {
+    const char *name;
+    uint32_t level;
+    /* vendor is zero-terminated, 12 character ASCII string */
+    char vendor[CPUID_VENDOR_SZ + 1];
+    int family;
+    int model;
+    int stepping;
+    uint32_t features, ext_features, ext2_features, ext3_features;
+    uint32_t kvm_features, svm_features;
+    uint32_t xlevel;
+    char model_id[48];
+    /* Store the results of Centaur's CPUID instructions */
+    uint32_t ext4_features;
+    uint32_t xlevel2;
+    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
+    uint32_t cpuid_7_0_ebx_features;
+} x86_def_t;
+
 #define X86_CPU_CLASS(klass) \
     OBJECT_CLASS_CHECK(X86CPUClass, (klass), TYPE_X86_CPU)
 #define X86_CPU(obj) \
@@ -41,6 +62,7 @@ 
  * X86CPUClass:
  * @parent_realize: The parent class' realize handler.
  * @parent_reset: The parent class' reset handler.
+ * @info: Model-specific data.
  *
  * An x86 CPU model or family.
  */
@@ -51,6 +73,8 @@  typedef struct X86CPUClass {
 
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
+
+    x86_def_t info;
 } X86CPUClass;
 
 /**
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ee2fd6b..6c95740 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -346,25 +346,6 @@  static void add_flagname_to_bitmaps(const char *flagname,
     }
 }
 
-typedef struct x86_def_t {
-    const char *name;
-    uint32_t level;
-    /* vendor is zero-terminated, 12 character ASCII string */
-    char vendor[CPUID_VENDOR_SZ + 1];
-    int family;
-    int model;
-    int stepping;
-    uint32_t features, ext_features, ext2_features, ext3_features;
-    uint32_t kvm_features, svm_features;
-    uint32_t xlevel;
-    char model_id[48];
-    /* Store the results of Centaur's CPUID instructions */
-    uint32_t ext4_features;
-    uint32_t xlevel2;
-    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
-    uint32_t cpuid_7_0_ebx_features;
-} x86_def_t;
-
 #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
           CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
@@ -868,86 +849,6 @@  static x86_def_t builtin_x86_defs[] = {
     },
 };
 
-#ifdef CONFIG_KVM
-static int cpu_x86_fill_model_id(char *str)
-{
-    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
-    int i;
-
-    for (i = 0; i < 3; i++) {
-        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
-        memcpy(str + i * 16 +  0, &eax, 4);
-        memcpy(str + i * 16 +  4, &ebx, 4);
-        memcpy(str + i * 16 +  8, &ecx, 4);
-        memcpy(str + i * 16 + 12, &edx, 4);
-    }
-    return 0;
-}
-#endif
-
-/* Fill a x86_def_t struct with information about the host CPU, and
- * the CPU features supported by the host hardware + host kernel
- *
- * This function may be called only if KVM is enabled.
- */
-static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
-{
-#ifdef CONFIG_KVM
-    KVMState *s = kvm_state;
-    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
-
-    assert(kvm_enabled());
-
-    x86_cpu_def->name = "host";
-    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
-
-    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
-    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
-    x86_cpu_def->stepping = eax & 0x0F;
-
-    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
-    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
-    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
-
-    if (x86_cpu_def->level >= 7) {
-        x86_cpu_def->cpuid_7_0_ebx_features =
-                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
-    } else {
-        x86_cpu_def->cpuid_7_0_ebx_features = 0;
-    }
-
-    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
-    x86_cpu_def->ext2_features =
-                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
-    x86_cpu_def->ext3_features =
-                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
-
-    cpu_x86_fill_model_id(x86_cpu_def->model_id);
-
-    /* Call Centaur's CPUID instruction. */
-    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
-        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
-        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
-        if (eax >= 0xC0000001) {
-            /* Support VIA max extended level */
-            x86_cpu_def->xlevel2 = eax;
-            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
-            x86_cpu_def->ext4_features =
-                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-        }
-    }
-
-    /* Other KVM-specific feature fields: */
-    x86_cpu_def->svm_features =
-        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
-    x86_cpu_def->kvm_features =
-        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
-
-#endif /* CONFIG_KVM */
-}
-
 static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
 {
     int i;
@@ -975,31 +876,31 @@  static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
 static int kvm_check_features_against_host(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
-    x86_def_t host_def;
+    ObjectClass *host_oc = object_class_by_name(TYPE_HOST_X86_CPU);
+    X86CPUClass *host_xcc = X86_CPU_CLASS(host_oc);
     uint32_t mask;
     int rv, i;
     struct model_features_t ft[] = {
-        {&env->cpuid_features, &host_def.features,
+        {&env->cpuid_features, &host_xcc->info.features,
             FEAT_1_EDX },
-        {&env->cpuid_ext_features, &host_def.ext_features,
+        {&env->cpuid_ext_features, &host_xcc->info.ext_features,
             FEAT_1_ECX },
-        {&env->cpuid_ext2_features, &host_def.ext2_features,
+        {&env->cpuid_ext2_features, &host_xcc->info.ext2_features,
             FEAT_8000_0001_EDX },
-        {&env->cpuid_ext3_features, &host_def.ext3_features,
+        {&env->cpuid_ext3_features, &host_xcc->info.ext3_features,
             FEAT_8000_0001_ECX },
-        {&env->cpuid_ext4_features, &host_def.ext4_features,
+        {&env->cpuid_ext4_features, &host_xcc->info.ext4_features,
             FEAT_C000_0001_EDX },
-        {&env->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
+        {&env->cpuid_7_0_ebx_features, &host_xcc->info.cpuid_7_0_ebx_features,
             FEAT_7_0_EBX },
-        {&env->cpuid_svm_features, &host_def.svm_features,
+        {&env->cpuid_svm_features, &host_xcc->info.svm_features,
             FEAT_SVM },
-        {&env->cpuid_kvm_features, &host_def.kvm_features,
+        {&env->cpuid_kvm_features, &host_xcc->info.kvm_features,
             FEAT_KVM },
     };
 
     assert(kvm_enabled());
 
-    kvm_cpu_fill_host(&host_def);
     for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) {
         FeatureWord w = ft[i].feat_word;
         FeatureWordInfo *wi = &feature_word_info[w];
@@ -1261,40 +1162,30 @@  static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
+static ObjectClass *x86_cpu_class_by_name(const char *name)
 {
-    x86_def_t *def;
-    int i;
+    ObjectClass *oc;
+    char *typename;
 
     if (name == NULL) {
-        return -1;
-    }
-    if (kvm_enabled() && strcmp(name, "host") == 0) {
-        kvm_cpu_fill_host(x86_cpu_def);
-        return 0;
+        return NULL;
     }
 
-    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-        def = &builtin_x86_defs[i];
-        if (strcmp(name, def->name) == 0) {
-            memcpy(x86_cpu_def, def, sizeof(*def));
-            /* sysenter isn't supported in compatibility mode on AMD,
-             * syscall isn't supported in compatibility mode on Intel.
-             * Normally we advertise the actual CPU vendor, but you can
-             * override this using the 'vendor' property if you want to use
-             * KVM's sysenter/syscall emulation in compatibility mode and
-             * when doing cross vendor migration
-             */
-            if (kvm_enabled()) {
-                uint32_t  ebx = 0, ecx = 0, edx = 0;
-                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
-                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
-            }
-            return 0;
+    if (strcmp(name, "host") == 0) {
+        if (kvm_enabled()) {
+            return object_class_by_name(TYPE_HOST_X86_CPU);
         }
+        return NULL;
     }
 
-    return -1;
+    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
+    oc = object_class_by_name(typename);
+    g_free(typename);
+    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_X86_CPU) ||
+                       object_class_is_abstract(oc))) {
+        oc = NULL;
+    }
+    return oc;
 }
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
@@ -1516,60 +1407,13 @@  static void filter_features_for_kvm(X86CPU *cpu)
 }
 #endif
 
-static int cpu_x86_register(X86CPU *cpu, const char *name)
-{
-    CPUX86State *env = &cpu->env;
-    x86_def_t def1, *def = &def1;
-    Error *error = NULL;
-
-    memset(def, 0, sizeof(*def));
-
-    if (cpu_x86_find_by_name(def, name) < 0) {
-        error_setg(&error, "Unable to find CPU definition: %s", name);
-        goto out;
-    }
-
-    if (kvm_enabled()) {
-        def->kvm_features |= kvm_default_features;
-    }
-    def->ext_features |= CPUID_EXT_HYPERVISOR;
-
-    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
-    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
-    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
-    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
-    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
-    env->cpuid_features = def->features;
-    env->cpuid_ext_features = def->ext_features;
-    env->cpuid_ext2_features = def->ext2_features;
-    env->cpuid_ext3_features = def->ext3_features;
-    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
-    env->cpuid_kvm_features = def->kvm_features;
-    env->cpuid_svm_features = def->svm_features;
-    env->cpuid_ext4_features = def->ext4_features;
-    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
-    env->cpuid_xlevel2 = def->xlevel2;
-
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
-    if (error) {
-        goto out;
-    }
-
-out:
-    if (error) {
-        fprintf(stderr, "%s\n", error_get_pretty(error));
-        error_free(error);
-        return -1;
-    }
-    return 0;
-}
-
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu = NULL;
     CPUX86State *env;
     gchar **model_pieces;
     char *name, *features;
+    ObjectClass *oc;
     Error *error = NULL;
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
@@ -1580,13 +1424,13 @@  X86CPU *cpu_x86_init(const char *cpu_model)
     name = model_pieces[0];
     features = model_pieces[1];
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    env->cpu_model_str = cpu_model;
-
-    if (cpu_x86_register(cpu, name) < 0) {
+    oc = x86_cpu_class_by_name(name);
+    if (oc == NULL) {
         goto error;
     }
+    cpu = X86_CPU(object_new(object_class_get_name(oc)));
+    env = &cpu->env;
+    env->cpu_model_str = cpu_model;
 
     cpu_x86_parse_featurestr(cpu, features, &error);
     if (error) {
@@ -1621,30 +1465,6 @@  void cpu_clear_apic_feature(CPUX86State *env)
 
 #endif /* !CONFIG_USER_ONLY */
 
-/* Initialize list of CPU models, filling some non-static fields if necessary
- */
-void x86_cpudef_setup(void)
-{
-    int i, j;
-    static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
-
-    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
-        x86_def_t *def = &builtin_x86_defs[i];
-
-        /* Look for specific "cpudef" models that */
-        /* have the QEMU version in .model_id */
-        for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) {
-            if (strcmp(model_with_versions[j], def->name) == 0) {
-                pstrcpy(def->model_id, sizeof(def->model_id),
-                        "QEMU Virtual CPU version ");
-                pstrcat(def->model_id, sizeof(def->model_id),
-                        qemu_get_version());
-                break;
-            }
-        }
-    }
-}
-
 static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
                              uint32_t *ecx, uint32_t *edx)
 {
@@ -2198,6 +2018,8 @@  static void x86_cpu_initfn(Object *obj)
     CPUState *cs = CPU(obj);
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
+    const x86_def_t *def = &xcc->info;
     static int inited;
 
     cpu_exec_init(env);
@@ -2227,6 +2049,41 @@  static void x86_cpu_initfn(Object *obj)
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
 
+    /* sysenter isn't supported in compatibility mode on AMD,
+     * syscall isn't supported in compatibility mode on Intel.
+     * Normally we advertise the actual CPU vendor, but you can
+     * override this using the 'vendor' property if you want to use
+     * KVM's sysenter/syscall emulation in compatibility mode and
+     * when doing cross vendor migration
+     */
+    if (kvm_enabled()) {
+        host_cpuid(0, 0, NULL, &env->cpuid_vendor1, &env->cpuid_vendor2,
+                   &env->cpuid_vendor3);
+    } else {
+        object_property_set_str(OBJECT(cpu), def->vendor, "vendor", NULL);
+    }
+
+    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
+    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
+    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
+    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", NULL);
+    env->cpuid_features = def->features;
+    env->cpuid_ext_features = def->ext_features;
+    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
+    env->cpuid_ext2_features = def->ext2_features;
+    env->cpuid_ext3_features = def->ext3_features;
+    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
+    env->cpuid_kvm_features = def->kvm_features;
+    if (kvm_enabled()) {
+        env->cpuid_kvm_features |= kvm_default_features;
+    }
+    env->cpuid_svm_features = def->svm_features;
+    env->cpuid_ext4_features = def->ext4_features;
+    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
+    env->cpuid_xlevel2 = def->xlevel2;
+
+    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", NULL);
+
     env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
 
     /* init various static tables used in TCG mode */
@@ -2239,6 +2096,27 @@  static void x86_cpu_initfn(Object *obj)
     }
 }
 
+static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
+{
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
+    x86_def_t *def = data;
+    int i;
+    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" };
+
+    memcpy(&xcc->info, def, sizeof(x86_def_t));
+
+    /* Look for specific models that have the QEMU version in .model_id */
+    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
+        if (strcmp(versioned_models[i], def->name) == 0) {
+            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
+                    "QEMU Virtual CPU version ");
+            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
+                    qemu_get_version());
+            break;
+        }
+    }
+}
+
 static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 {
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
@@ -2250,6 +2128,21 @@  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_reset = cc->reset;
     cc->reset = x86_cpu_reset;
+
+    cc->class_by_name = x86_cpu_class_by_name;
+}
+
+static void x86_register_cpu_type(const x86_def_t *def)
+{
+    TypeInfo type_info = {
+        .parent = TYPE_X86_CPU,
+        .class_init = x86_cpu_def_class_init,
+        .class_data = (void *)def,
+    };
+
+    type_info.name = g_strdup_printf("%s-" TYPE_X86_CPU, def->name);
+    type_register(&type_info);
+    g_free((void *)type_info.name);
 }
 
 static const TypeInfo x86_cpu_type_info = {
@@ -2257,14 +2150,19 @@  static const TypeInfo x86_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(X86CPU),
     .instance_init = x86_cpu_initfn,
-    .abstract = false,
+    .abstract = true,
     .class_size = sizeof(X86CPUClass),
     .class_init = x86_cpu_common_class_init,
 };
 
 static void x86_cpu_register_types(void)
 {
+    int i;
+
     type_register_static(&x86_cpu_type_info);
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
+        x86_register_cpu_type(&builtin_x86_defs[i]);
+    }
 }
 
 type_init(x86_cpu_register_types)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 7577e4f..2aef7b7 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -887,7 +887,6 @@  typedef struct CPUX86State {
 X86CPU *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
-void x86_cpudef_setup(void);
 int cpu_x86_support_mca_broadcast(CPUX86State *env);
 
 int cpu_get_pic_interrupt(CPUX86State *s);
@@ -1079,7 +1078,6 @@  static inline CPUX86State *cpu_init(const char *cpu_model)
 #define cpu_gen_code cpu_x86_gen_code
 #define cpu_signal_handler cpu_x86_signal_handler
 #define cpu_list x86_cpu_list
-#define cpudef_setup	x86_cpudef_setup
 
 #define CPU_SAVE_VERSION 12
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 9ebf181..dbfa670 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -735,6 +735,80 @@  static int kvm_get_supported_msrs(KVMState *s)
     return ret;
 }
 
+static void kvm_host_cpu_initfn(Object *obj)
+{
+    assert(kvm_enabled());
+}
+
+static bool kvm_host_cpu_needs_class_init;
+
+static void kvm_host_cpu_class_init(ObjectClass *oc, void *data)
+{
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
+    x86_def_t *x86_cpu_def = &xcc->info;
+    KVMState *s = kvm_state;
+    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
+    int i;
+
+    if (!kvm_enabled()) {
+        kvm_host_cpu_needs_class_init = true;
+        return;
+    }
+
+    xcc->info.name = "host";
+
+    /* Vendor will be set in initfn if kvm_enabled() */
+
+    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
+    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
+    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
+    x86_cpu_def->stepping = eax & 0x0F;
+
+    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
+    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
+
+    if (x86_cpu_def->level >= 7) {
+        x86_cpu_def->cpuid_7_0_ebx_features =
+                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
+    } else {
+        x86_cpu_def->cpuid_7_0_ebx_features = 0;
+    }
+
+    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+    x86_cpu_def->ext2_features =
+                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
+    x86_cpu_def->ext3_features =
+                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
+
+    for (i = 0; i < 3; i++) {
+        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
+        memcpy(xcc->info.model_id + i * 16 +  0, &eax, 4);
+        memcpy(xcc->info.model_id + i * 16 +  4, &ebx, 4);
+        memcpy(xcc->info.model_id + i * 16 +  8, &ecx, 4);
+        memcpy(xcc->info.model_id + i * 16 + 12, &edx, 4);
+    }
+
+    /* Call Centaur's CPUID instruction. */
+    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
+        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
+        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+        if (eax >= 0xC0000001) {
+            /* Support VIA max extended level */
+            x86_cpu_def->xlevel2 = eax;
+            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
+            x86_cpu_def->ext4_features =
+                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
+        }
+    }
+
+    /* Other KVM-specific feature fields: */
+    x86_cpu_def->svm_features =
+        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
+    x86_cpu_def->kvm_features =
+        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
+}
+
 int kvm_arch_init(KVMState *s)
 {
     QemuOptsList *list = qemu_find_opts("machine");
@@ -797,6 +871,11 @@  int kvm_arch_init(KVMState *s)
             }
         }
     }
+
+    if (kvm_host_cpu_needs_class_init) {
+        kvm_host_cpu_class_init(object_class_by_name(TYPE_HOST_X86_CPU), NULL);
+    }
+
     return 0;
 }
 
@@ -2330,3 +2409,17 @@  int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id)
     return kvm_deassign_irq_internal(s, dev_id, KVM_DEV_IRQ_GUEST_MSIX |
                                                 KVM_DEV_IRQ_HOST_MSIX);
 }
+
+static const TypeInfo host_x86_cpu_type_info = {
+    .name = TYPE_HOST_X86_CPU,
+    .parent = TYPE_X86_CPU,
+    .instance_init = kvm_host_cpu_initfn,
+    .class_init = kvm_host_cpu_class_init,
+};
+
+static void kvm_register_types(void)
+{
+    type_register_static(&host_x86_cpu_type_info);
+}
+
+type_init(kvm_register_types)