[Qemu-devel] [RFC] target-i386: register a class for each CPU model

[Eduardo Habkost]

This creates the following class hierarchy:

- TYPE_X86_CPU ("<arch>-cpu")
 - TYPE_X86_DEFCPU "<arch>-cpu-predefined": abstract base class for the
   predefined CPU models
   - "<arch>-cpu-model-<model>": a class for each predefined CPU model
 - TYPE_X86_HOST_CPU ("<arch>-cpu-model-host"): class for the "-cpu host" CPU
   model

On TARGET_X86_64, "<arch>" is "x86_64", on TARGET_I386, "<arch>" is
"i386".

The trick here is to replace exactly what's implemented in the
cpu_x86_find_cpudef() logic and nothing else. So, the only difference in
relation to the previous code is that instead of looking at the CPU model table
on cpu_x86_find_cpudef(), we just use the right CPU class, that will fill the
X86CPUDefinition struct on a ->init_cpudef() method.

The init_cpudef() method was added jut to not require us to kill the
X86CPUDefinition array in the same patch. But the X86CPUDefinition
struct may eventually go away, and all the default-value initialization
for each CPU model can become just different defaults set on
instance_init() or class_init().

Signed-off-by: Eduardo Habkost <address@hidden>
---
I am using those class names because I don't want the CPU model names to live
in the same namespace as all device names. I want to avoid doing the same
mistake that was done in the arm code, that registers a QOM class named "any".
Some CPU model names, like "qemu64", don't make any sense unless you already
know it is a CPU model name.

As an alternative to the complex naming above, I was considering simply using
"cpu-<model>" as the class name. I don't know what others think.

This patch depends on the "[PATCH 00/17] target-i386: CPU init cleanup for CPU
classes/properties" series I have just sent.

Changes v1 -> v2:
 - Rebase on top of CPU properties series
 - Use a static type (with a different class init function) for the
   "-cpu host" class

Changes v2 -> v3:
 - Fix the "device not found" error to use the CPU model name, not
   the full -cpu string
 - kill cpudef_init() and cpudef_setup(), as we don't need a
   cpudef-specific initialization function, anymore
 - Instead of keeping a copy of a X86CPUDefinition struct inside
   X86CPUClass, keep only a pointer
 - Add a init_cpudef() class method, used to fill the X86CPUDefinition
   struct for the CPU
 - Use separate subclass for "-cpu host", that uses cpu_x86_fill_host()
   on init_cpudef()

Changes v3 -> v4:
 - Rebase on top of qemu.git master, without the CPU properties series
 - Rename CPU classes to "<arch>-cpu-model<model>"
---
 arch_init.c           |   7 --
 arch_init.h           |   1 -
 bsd-user/main.c       |   3 -
 linux-user/main.c     |   3 -
 target-i386/cpu-qom.h |  22 +++++-
 target-i386/cpu.c     | 208 +++++++++++++++++++++++++++++++++++++++++---------
 target-i386/cpu.h     |   2 -
 vl.c                  |   7 --
 8 files changed, 192 insertions(+), 61 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index e6effe8..bea1b9c 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1124,13 +1124,6 @@ void do_smbios_option(const char *optarg)
 #endif
 }
 
-void cpudef_init(void)
-{
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file */
-#endif
-}
-
 int audio_available(void)
 {
 #ifdef HAS_AUDIO
diff --git a/arch_init.h b/arch_init.h
index 5fc780c..84a7f9a 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -27,7 +27,6 @@ extern const uint32_t arch_type;
 void select_soundhw(const char *optarg);
 void do_acpitable_option(const char *optarg);
 void do_smbios_option(const char *optarg);
-void cpudef_init(void);
 int audio_available(void);
 void audio_init(ISABus *isa_bus, PCIBus *pci_bus);
 int tcg_available(void);
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 095ae8e..cf0a345 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -762,9 +762,6 @@ int main(int argc, char **argv)
     }
 
     cpu_model = NULL;
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file (TBD) */
-#endif
 
     optind = 1;
     for(;;) {
diff --git a/linux-user/main.c b/linux-user/main.c
index 25e35cd..e881fcf 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3426,9 +3426,6 @@ int main(int argc, char **argv, char **envp)
     }
 
     cpu_model = NULL;
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file (TBD) */
-#endif
 
     /* init debug */
     cpu_set_log_filename(log_file);
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 5901140..01ea9de 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -37,19 +37,37 @@
 #define X86_CPU_GET_CLASS(obj) \
     OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
 
+
+struct X86CPUDefinition;
+typedef struct X86CPUDefinition X86CPUDefinition;
+
+struct X86CPUClass;
+typedef struct X86CPUClass X86CPUClass;
+
 /**
  * X86CPUClass:
  * @parent_reset: The parent class' reset handler.
+ * @init_cpudef: initialize X86CPUDefinition struct for CPU class
  *
  * An x86 CPU model or family.
  */
-typedef struct X86CPUClass {
+struct X86CPUClass {
     /*< private >*/
     CPUClass parent_class;
     /*< public >*/
 
     void (*parent_reset)(CPUState *cpu);
-} X86CPUClass;
+
+    /* Init a X86CPUDefinition struct.
+     * This method should eventually go away when we make all info from
+     * X86CPUDefinition be settable using properties inside instance_init().
+     *
+     * We can't just store a X86CPUDefinition pointer on the X86CPUClass
+     * struct because the "-cpu host" class needs KVM to be initialized,
+     * to probe host CPU/KVM capabilities.
+     */
+    int (*init_cpudef)(X86CPUClass *xcc, X86CPUDefinition *def, Error **errp);
+};
 
 /**
  * X86CPU:
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5f2ce7d..7b13987 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -271,7 +271,11 @@ static void add_flagname_to_bitmaps(const char *flagname, 
uint32_t *features,
             fprintf(stderr, "CPU feature %s not found\n", flagname);
 }
 
-typedef struct X86CPUDefinition {
+/* This struct should eventually go away and just become multiple series of
+ * properties and values to set by default on CPU objects, one for each CPU
+ * class.
+ */
+struct X86CPUDefinition {
     struct X86CPUDefinition *next;
     const char *name;
     uint32_t level;
@@ -290,7 +294,7 @@ typedef struct X86CPUDefinition {
     uint32_t xlevel2;
     /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
     uint32_t cpuid_7_0_ebx_features;
-} X86CPUDefinition;
+};
 
 #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
@@ -1146,33 +1150,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor 
*v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-/* Find a CPU model definition and put the result on a X86CPUDefinition struct
- */
-static int cpu_x86_find_cpudef(const char *name,
-                               X86CPUDefinition *result,
-                               Error **errp)
-{
-    X86CPUDefinition *def;
-
-    for (def = x86_defs; def; def = def->next)
-        if (name && !strcmp(name, def->name))
-            break;
-    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
-        kvm_cpu_fill_host(result);
-    } else if (!def) {
-        goto error;
-    } else {
-        memcpy(result, def, sizeof(*def));
-    }
-    return 0;
-
-error:
-    if (!error_is_set(errp)) {
-        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
-    }
-    return -1;
-}
-
 static int cpu_x86_parse_featurestr(X86CPUDefinition *x86_cpu_def,
                                     char *features, Error **errp)
 {
@@ -1503,29 +1480,54 @@ static int cpudef_2_x86_cpu(X86CPU *cpu, 
X86CPUDefinition *def, Error **errp)
     return 0;
 }
 
+/* Build class name for specific CPU model:
+ *
+ * The CPU class names are in the form <arch>-cpu-model-<model>.
+ */
+
+/* For statically-defined class names (e.g. TYPE_X86_CPU_HOST) */
+#define STATIC_CPU_CLASS_NAME(model) (TYPE_X86_CPU "-model-" model)
+
+/* For dynamically-defined class names (built from the cpudef array) */
+static char *build_cpu_class_name(const char *model)
+{
+    return g_strdup_printf("%s-model-%s", TYPE_X86_CPU, model);
+}
+
 X86CPU *cpu_x86_init(const char *cpu_string)
 {
     X86CPU *cpu = NULL;
+    ObjectClass *obj_class;
+    X86CPUClass *cpu_class;
     CPUX86State *env;
     Error *error = NULL;
     X86CPUDefinition def;
-    char *name, *features;
+    char *model_name, *features;
     gchar **model_pieces;
+    char *class_name = NULL;
 
     model_pieces = g_strsplit(cpu_string, ",", 2);
     if (!model_pieces[0]) {
         goto error;
     }
-    name = model_pieces[0];
+    model_name = model_pieces[0];
     features = model_pieces[1];
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    class_name = build_cpu_class_name(model_name);
+    obj_class = object_class_by_name(class_name);
+    if (!obj_class) {
+        error_set(&error, QERR_DEVICE_NOT_FOUND, model_name);
+        goto error;
+    }
+
+    cpu = X86_CPU(object_new(class_name));
     env = &cpu->env;
     env->cpu_model_str = cpu_string;
 
     memset(&def, 0, sizeof(def));
 
-    if (cpu_x86_find_cpudef(name, &def, &error) < 0) {
+    cpu_class = X86_CPU_GET_CLASS(cpu);
+    if (cpu_class->init_cpudef(cpu_class, &def, &error) < 0) {
         goto error;
     }
 
@@ -1543,9 +1545,11 @@ X86CPU *cpu_x86_init(const char *cpu_string)
     }
 
     g_strfreev(model_pieces);
+    g_free(class_name);
     return cpu;
 error:
     g_strfreev(model_pieces);
+    g_free(class_name);
     if (cpu) {
         object_delete(OBJECT(cpu));
     }
@@ -1567,7 +1571,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
 
 /* Initialize list of CPU models, filling some non-static fields if necessary
  */
-void x86_cpudef_setup(void)
+static void x86_cpudef_setup(void)
 {
     int i, j;
     static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" 
};
@@ -2123,15 +2127,147 @@ static const TypeInfo x86_cpu_type_info = {
     .name = TYPE_X86_CPU,
     .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,
 };
 
+/* TYPE_X86_DEFCPU: abstract class for predefined CPU models */
+
+#define TYPE_X86_DEFCPU (TYPE_X86_CPU "-predefined")
+
+#define X86_DEFCPU_CLASS(klass) \
+    OBJECT_CLASS_CHECK(X86DEFCPUClass, (klass), TYPE_X86_DEFCPU)
+#define X86_DEFCPU(obj) \
+    OBJECT_CHECK(X86DEFCPU, (obj), TYPE_X86_DEFCPU)
+#define X86_DEFCPU_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(X86DEFCPUClass, (obj), TYPE_X86_DEFCPU)
+
+/**
+ * X86DEFCPUClass:
+ * @cpudef: pointer to X86CPUDefinition for CPU model
+ *
+ * A predefined CPU model, pointing to a X86CPUDefinition struct.
+ */
+typedef struct X86DEFCPUClass {
+    /*< private >*/
+    X86CPUClass parent_class;
+
+    /*< public >*/
+    X86CPUDefinition *cpudef;
+} X86DEFCPUClass;
+
+/* The init_cpudef() method for children of TYPE_X86_DEFCPU */
+static int x86_predef_cpu_init_cpudef(X86CPUClass *xcc, X86CPUDefinition *def,
+                                       Error **errp)
+{
+    X86DEFCPUClass *defclass = X86_DEFCPU_CLASS(xcc);
+    memcpy(def, defclass->cpudef, sizeof(X86CPUDefinition));
+    return 0;
+}
+
+/* The class_init() method for children of TYPE_X86_DEFCPU
+ *
+ * Note that this is the class_init() method for _children_ of TYPE_X86_DEFCPU,
+ * not for TYPE_X86_DEFCPU itself, because we need the class-specific
+ * class_data pointer.
+ */
+static void x86_predef_cpu_class_init(ObjectClass *oc, void *data)
+{
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
+    X86DEFCPUClass *xdcc = X86_DEFCPU_CLASS(xcc);
+    X86CPUDefinition *def = data;
+
+    xcc->init_cpudef = x86_predef_cpu_init_cpudef;
+    xdcc->cpudef = def;
+}
+
+static TypeInfo x86_predef_cpu_type_info = {
+    .name = TYPE_X86_DEFCPU,
+    .parent = TYPE_X86_CPU,
+    .abstract = true,
+    .class_size = sizeof(X86DEFCPUClass),
+};
+
+/* Register a CPU class for a specific X86CPUDefinintion
+ *
+ * The class will be a child of TYPE_X86_DEFCPU.
+ */
+static void x86_cpu_register_class(const char *name, X86CPUDefinition *def)
+{
+    char *class_name = build_cpu_class_name(name);
+    TypeInfo type = {
+        .name = class_name,
+        .parent = TYPE_X86_DEFCPU,
+        .instance_size = sizeof(X86CPU),
+        .instance_init = x86_cpu_initfn,
+        .class_size = sizeof(X86DEFCPUClass),
+        .class_init = x86_predef_cpu_class_init,
+        .class_data = def,
+    };
+    type_register(&type);
+    g_free(class_name);
+}
+
+
+/* TYPE_X86_HOST_CPU:
+ *
+ * X86CPU subclass for "-cpu host"
+ */
+
+#define TYPE_X86_HOST_CPU STATIC_CPU_CLASS_NAME("host")
+
+/* The init_cpudef() method for TYPE_X86_HOST_CPU */
+static int x86_host_cpu_init_cpudef(X86CPUClass *xcc, X86CPUDefinition *def,
+                                     Error **errp)
+{
+    if (!kvm_enabled()) {
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                  "-cpu host is available only when using KVM");
+        return -1;
+    }
+    memset(def, 0, sizeof(*def));
+    kvm_cpu_fill_host(def);
+    return 0;
+}
+
+/* The class_init() method for TYPE_X86_HOST_CPU */
+static void x86_cpu_host_class_init(ObjectClass *oc, void *data)
+{
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
+
+    xcc->init_cpudef = x86_host_cpu_init_cpudef;
+}
+
+static TypeInfo x86_cpu_host_type_info = {
+    .name = TYPE_X86_HOST_CPU,
+    .parent = TYPE_X86_CPU,
+    .instance_size = sizeof(X86CPU),
+    .instance_init = x86_cpu_initfn,
+    .class_size = sizeof(X86CPUClass),
+    .class_init = x86_cpu_host_class_init,
+};
+
 static void x86_cpu_register_types(void)
 {
+    X86CPUDefinition *def;
+
+    x86_cpudef_setup();
+
+    /* Abstract X86CPU class */
     type_register_static(&x86_cpu_type_info);
+
+    /* -cpu host class */
+    type_register_static(&x86_cpu_host_type_info);
+
+    /* Abstract class for predefined CPU models */
+    type_register_static(&x86_predef_cpu_type_info);
+
+    /* One class for each CPU model: */
+    for (def = x86_defs; def; def = def->next) {
+        x86_cpu_register_class(def->name, def);
+    }
+
 }
 
 type_init(x86_cpu_register_types)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 4d5510e..dba4f66 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -841,7 +841,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);
@@ -1022,7 +1021,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/vl.c b/vl.c
index 4f03a72..997d92a 100644
--- a/vl.c
+++ b/vl.c
@@ -3508,13 +3508,6 @@ int main(int argc, char **argv, char **envp)
         qemu_set_version(machine->hw_version);
     }
 
-    /* Init CPU def lists, based on config
-     * - Must be called after all the qemu_read_config_file() calls
-     * - Must be called before list_cpus()
-     * - Must be called before machine->init()
-     */
-    cpudef_init();
-
     if (cpu_model && is_help_option(cpu_model)) {
         list_cpus(stdout, &fprintf, cpu_model);
         exit(0);
-- 
1.7.11.7