[Qemu-devel] [PATCH 0/7] x86 CPU subclasses, take 6

[Qemu-devel] [PATCH 0/7] x86 CPU subclasses, take 6

[Eduardo Habkost]

I want to try to get this in 1.8, because I have found one additional use-case
for the new subclasses:

libvirt needs to be able to query details about the existing CPU models, and it
can't do that today without restarting QEMU every time. Having separate classes
for each CPU model allows libvirt to create/destroy CPU objects in a loop just
to query the resulting properties (especially "feature-words") for each CPU
model.

This version is closer to the version sent by Andrea and then later resubmitted
by Igor as "[RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM
subclasses", than the previous version I have sent, as it doesn't create one new
class_init function for each subclass. One main difference is that this version
does not use KVM-specific subclasses, to keep things simpler.

Another difference is that instead of late registration of the "host" class, I
simply changed the "host" subclass to do KVM-dependent initialization steps on
instance_init instead of class_init. This way we won't require any
initialization-ordering tricks. In the past this was a problem, but today the
global properties are being set by qdev code on post_init, so we can safely
initialize property defaults on instance_init without having to worry that it
would break the setting of global properties.

This series needs to be applied on top of:
   Subject: [PATCH 0/8] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()
   Message-Id: <1385322940-27325-1-git-send-email-ehabkost@redhat.com>
   http://article.gmane.org/gmane.comp.emulators.qemu/243013


Eduardo Habkost (7):
  target-i386: Eliminate CONFIG_KVM #ifdefs
  target-i386: Don't change x86_def_t struct on cpu_x86_register()
  target-i386: Move KVM default-vendor hack to instance_init
  target-i386: Rename cpu_x86_register() to x86_cpu_load_def()
  target-i386: Call x86_cpu_load_def() earlier
  target-i386: Rename x86_def_t to X86CPUDefinition
  target-i386: CPU model subclasses

 target-i386/cpu-qom.h |  13 ++
 target-i386/cpu.c     | 407 ++++++++++++++++++++++++++++++--------------------
 target-i386/cpu.h     |   2 -
 3 files changed, 260 insertions(+), 162 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/7] target-i386: Eliminate CONFIG_KVM #ifdefs

[Eduardo Habkost]

The compiler is capable of eliminating the KVM-specific function calls
as long as the calling function has an assert(kvm_enabled()) line, so we
don't need to wrap all KVM-specific inside #ifdefs.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d747e04..b525592 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -373,7 +373,6 @@ void disable_kvm_pv_eoi(void)
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
-#if defined(CONFIG_KVM)
     uint32_t vec[4];
 
 #ifdef __x86_64__
@@ -401,7 +400,6 @@ void host_cpuid(uint32_t function, uint32_t count,
         *ecx = vec[2];
     if (edx)
         *edx = vec[3];
-#endif
 }
 
 #define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
@@ -1118,7 +1116,6 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
     }
 }
 
-#ifdef CONFIG_KVM
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1133,7 +1130,6 @@ static int cpu_x86_fill_model_id(char *str)
     }
     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
@@ -1142,7 +1138,6 @@ static int cpu_x86_fill_model_id(char *str)
  */
 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;
 
@@ -1172,8 +1167,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
             kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
                                          wi->cpuid_reg);
     }
-
-#endif /* CONFIG_KVM */
 }
 
 static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
@@ -1798,13 +1791,14 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
-#ifdef CONFIG_KVM
 static void filter_features_for_kvm(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
     KVMState *s = kvm_state;
     FeatureWord w;
 
+    assert(kvm_enabled());
+
     for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
         uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
@@ -1815,7 +1809,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
         cpu->filtered_features[w] = requested_features & ~env->features[w];
     }
 }
-#endif
 
 static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
 {
@@ -2525,9 +2518,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                        "Host's CPU doesn't support requested features");
             goto out;
         }
-#ifdef CONFIG_KVM
         filter_features_for_kvm(cpu);
-#endif
     }
 
 #ifndef CONFIG_USER_ONLY
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] target-i386: Don't change x86_def_t struct on cpu_x86_register()

[Eduardo Habkost]

As eventually the x86_def_t data is going to be provided by the CPU
class, it's better to not touch it, and handle the special cases on the
X86CPU object itself.

Current behavior of the code should stay exactly the same.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b525592..bfe2453 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1822,11 +1822,6 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
         return;
     }
 
-    if (kvm_enabled()) {
-        def->features[FEAT_KVM] |= kvm_default_features;
-    }
-    def->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
-
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
     object_property_set_int(OBJECT(cpu), def->level, "level", errp);
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
@@ -1845,6 +1840,12 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
     cpu->cache_info_passthrough = def->cache_info_passthrough;
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+
+    /* Special cases not set in the x86_def_t structs: */
+    if (kvm_enabled()) {
+        env->features[FEAT_KVM] |= kvm_default_features;
+    }
+    env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
 }
 
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] target-i386: Move KVM default-vendor hack to instance_init

[Eduardo Habkost]

As we will not have a cpu_x86_find_by_name() function anymore,
move the KVM default-vendor hack to instance_init.

Unfortunately we can't move that code to class_init because it depends
on KVM being initialized.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bfe2453..9bd6c20 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1560,18 +1560,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         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;
         }
     }
@@ -1822,7 +1810,6 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
         return;
     }
 
-    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
     object_property_set_int(OBJECT(cpu), def->level, "level", errp);
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
     object_property_set_int(OBJECT(cpu), def->model, "model", errp);
@@ -1846,6 +1833,25 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
         env->features[FEAT_KVM] |= kvm_default_features;
     }
     env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
+
+    /* 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
+     */
+    const char *vendor = def->vendor;
+    char host_vendor[CPUID_VENDOR_SZ + 1];
+    if (kvm_enabled()) {
+        uint32_t  ebx = 0, ecx = 0, edx = 0;
+        host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+        x86_cpu_vendor_words2str(host_vendor, ebx, edx, ecx);
+        vendor = host_vendor;
+    }
+
+    object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
+
 }
 
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] target-i386: Rename cpu_x86_register() to x86_cpu_load_def()

[Eduardo Habkost]

There isn't any kind of "registration" involved in cpu_x86_register()
anymore: it is simply looking up a CPU model name and loading the model
definition data into the X86CPU object. Rename it to x86_cpu_load_def()
to reflect what it does.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9bd6c20..f55caea 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1798,7 +1798,9 @@ static void filter_features_for_kvm(X86CPU *cpu)
     }
 }
 
-static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
+/* Load CPU definition for a given CPU model name
+ */
+static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
 {
     CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
@@ -1881,7 +1883,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     object_unref(OBJECT(cpu));
 #endif
 
-    cpu_x86_register(cpu, name, &error);
+    x86_cpu_load_def(cpu, name, &error);
     if (error) {
         goto out;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] target-i386: Call x86_cpu_load_def() earlier

[Eduardo Habkost]

As we will initialize the X86CPU fields on instance_init eventually,
move the code that initializes the X86CPU data based on the CPU model
name closer to the object_new() call.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f55caea..7f94a08 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1874,6 +1874,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     features = model_pieces[1];
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    x86_cpu_load_def(cpu, name, &error);
+    if (error) {
+        goto out;
+    }
+
 #ifndef CONFIG_USER_ONLY
     if (icc_bridge == NULL) {
         error_setg(&error, "Invalid icc-bridge value");
@@ -1883,11 +1888,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     object_unref(OBJECT(cpu));
 #endif
 
-    x86_cpu_load_def(cpu, name, &error);
-    if (error) {
-        goto out;
-    }
-
     /* Emulate per-model subclasses for global properties */
     typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
     qdev_prop_set_globals_for_type(DEVICE(cpu), typename, &error);
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/7] target-i386: Rename x86_def_t to X86CPUDefinition

[Eduardo Habkost]

As the new X86CPU subclass code is going to change lots of the code
invoving x86_def_t, let's rename the struct to match coding style first.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7f94a08..ab80081 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -481,7 +481,7 @@ static void add_flagname_to_bitmaps(const char *flagname,
     }
 }
 
-typedef struct x86_def_t {
+typedef struct X86CPUDefinition {
     const char *name;
     uint32_t level;
     uint32_t xlevel;
@@ -494,7 +494,7 @@ typedef struct x86_def_t {
     FeatureWordArray features;
     char model_id[48];
     bool cache_info_passthrough;
-} x86_def_t;
+} X86CPUDefinition;
 
 #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
@@ -546,7 +546,7 @@ typedef struct x86_def_t {
 
 /* built-in CPU model definitions
  */
-static x86_def_t builtin_x86_defs[] = {
+static X86CPUDefinition builtin_x86_defs[] = {
     {
         .name = "qemu64",
         .level = 4,
@@ -1105,7 +1105,7 @@ static x86_def_t builtin_x86_defs[] = {
 void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
                                  uint32_t feat_add, uint32_t feat_remove)
 {
-    x86_def_t *def;
+    X86CPUDefinition *def;
     int i;
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
         def = &builtin_x86_defs[i];
@@ -1131,12 +1131,12 @@ static int cpu_x86_fill_model_id(char *str)
     return 0;
 }
 
-/* Fill a x86_def_t struct with information about the host CPU, and
+/* Fill a X86CPUDefinition 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)
+static void kvm_cpu_fill_host(X86CPUDefinition *x86_cpu_def)
 {
     KVMState *s = kvm_state;
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1539,10 +1539,10 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque,
     error_propagate(errp, err);
 }
 
-static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
+static int cpu_x86_find_by_name(X86CPU *cpu, X86CPUDefinition *x86_cpu_def,
                                 const char *name)
 {
-    x86_def_t *def;
+    X86CPUDefinition *def;
     Error *err = NULL;
     int i;
 
@@ -1732,7 +1732,7 @@ static void listflags(char *buf, int bufsize, uint32_t fbits,
 /* generate CPU information. */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    x86_def_t *def;
+    X86CPUDefinition *def;
     char buf[256];
     int i;
 
@@ -1759,7 +1759,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
-    x86_def_t *def;
+    X86CPUDefinition *def;
     int i;
 
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
@@ -1803,7 +1803,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
 static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
 {
     CPUX86State *env = &cpu->env;
-    x86_def_t def1, *def = &def1;
+    X86CPUDefinition def1, *def = &def1;
 
     memset(def, 0, sizeof(*def));
 
@@ -1830,7 +1830,7 @@ static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
 
-    /* Special cases not set in the x86_def_t structs: */
+    /* Special cases not set in the X86CPUDefinition structs: */
     if (kvm_enabled()) {
         env->features[FEAT_KVM] |= kvm_default_features;
     }
@@ -1952,7 +1952,7 @@ void x86_cpudef_setup(void)
     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];
+        X86CPUDefinition *def = &builtin_x86_defs[i];
 
         /* Look for specific "cpudef" models that */
         /* have the QEMU version in .model_id */
-- 
1.8.3.1

[Qemu-devel] [RFC 7/7] target-i386: CPU model subclasses

[Eduardo Habkost]

Register separate QOM classes for each x86 CPU model.

This will allow management code to more easily probe what each CPU model
provides, by simply creating objects using the appropriate class name,
without having to restart QEMU.

This also allows us to eliminate the qdev_prop_set_globals_for_type()
hack to set CPU-model-specific global properties.

Instead of creating separate class_init functions for each class, I just
used class_dat to store a pointer to the X86CPUDefinition struct for
each CPU model. This should make the patch shorter and easier to review.
Later we can gradually convert each X86CPUDefinition field to lists of
per-class property defaults.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
This version is closer to the version sent by Andrea and then later
resubmitted by Igor as "[RFC v5] target-i386: Slim conversion to X86CPU
subclasses + KVM subclasses", as it doesn't create one new class_init
function for each subclass. One main difference is that this version
does not use KVM-specific subclasses, to keep things simpler.
---
 target-i386/cpu-qom.h |  13 ++
 target-i386/cpu.c     | 343 +++++++++++++++++++++++++++++++-------------------
 target-i386/cpu.h     |   2 -
 3 files changed, 228 insertions(+), 130 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index f4fab15..468fea7 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -37,6 +37,9 @@
 #define X86_CPU_GET_CLASS(obj) \
     OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
 
+
+typedef struct X86CPUDefinition X86CPUDefinition;
+
 /**
  * X86CPUClass:
  * @parent_realize: The parent class' realize handler.
@@ -49,6 +52,16 @@ typedef struct X86CPUClass {
     CPUClass parent_class;
     /*< public >*/
 
+    /* CPU model definition
+     * Should be eventually replaced by subclass-specific property defaults
+     */
+    X86CPUDefinition *cpu_def;
+    /* CPU model requires KVM to be enabled */
+    bool kvm_required;
+    /* Optional description of CPU model.
+     * If unavailable, cpu_def->model_id is used */
+    const char *model_description;
+
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
 } X86CPUClass;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ab80081..5898d52 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -481,7 +481,10 @@ static void add_flagname_to_bitmaps(const char *flagname,
     }
 }
 
-typedef struct X86CPUDefinition {
+/* CPU model definition data that was not converted to QOM per-subclass
+ * property defaults yet.
+ */
+struct X86CPUDefinition {
     const char *name;
     uint32_t level;
     uint32_t xlevel;
@@ -494,7 +497,7 @@ typedef struct X86CPUDefinition {
     FeatureWordArray features;
     char model_id[48];
     bool cache_info_passthrough;
-} X86CPUDefinition;
+};
 
 #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
@@ -544,8 +547,41 @@ typedef struct X86CPUDefinition {
           CPUID_7_0_EBX_ERMS, CPUID_7_0_EBX_INVPCID, CPUID_7_0_EBX_RTM,
           CPUID_7_0_EBX_RDSEED */
 
-/* built-in CPU model definitions
+/* CPU class name definitions: */
+
+#define X86_CPU_CLASS_SUFFIX "-" TYPE_X86_CPU
+#define CPU_CLASS_NAME(name) (name X86_CPU_CLASS_SUFFIX)
+
+
+static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
+{
+    const char *class_name = object_class_get_name(OBJECT_CLASS(cc));
+    if (g_str_has_suffix(class_name, X86_CPU_CLASS_SUFFIX)) {
+        return g_strndup(class_name,
+                         strlen(class_name) - strlen(X86_CPU_CLASS_SUFFIX));
+    } else {
+        return g_strdup(class_name);
+    }
+}
+
+/* Return class name for a given CPU model name
+ * Caller is responsible for freeing the returned string.
  */
+static char *x86_cpu_class_name(const char *model_name)
+{
+    return g_strdup_printf(CPU_CLASS_NAME("%s"), model_name);
+}
+
+/* Return X86CPUClass for a CPU model name */
+static X86CPUClass *x86_cpu_class_by_name(const char *name)
+{
+    X86CPUClass *cc;
+    char *class_name = x86_cpu_class_name(name);
+    cc =  X86_CPU_CLASS(object_class_by_name(class_name));
+    g_free(class_name);
+    return cc;
+}
+
 static X86CPUDefinition builtin_x86_defs[] = {
     {
         .name = "qemu64",
@@ -1090,6 +1126,33 @@ static X86CPUDefinition builtin_x86_defs[] = {
     },
 };
 
+static void x86_cpu_class_init_cpudef(ObjectClass *oc, void *data)
+{
+    X86CPUDefinition *cpudef = data;
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
+    xcc->cpu_def = cpudef;
+}
+
+static void x86_register_cpudef_classes(void)
+{
+    int i;
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
+        X86CPUDefinition *def = &builtin_x86_defs[i];
+        char *class_name = x86_cpu_class_name(def->name);
+        TypeInfo ti = {
+            .name = class_name,
+            .parent = TYPE_X86_CPU,
+            .instance_size = sizeof(X86CPU),
+            .abstract = false,
+            .class_size = sizeof(X86CPUClass),
+            .class_init = x86_cpu_class_init_cpudef,
+            .class_data = def,
+        };
+        type_register(&ti);
+        g_free(class_name);
+    }
+}
+
 /**
  * x86_cpu_compat_set_features:
  * @cpu_model: CPU model name to be changed. If NULL, all CPU models are changed
@@ -1131,44 +1194,77 @@ static int cpu_x86_fill_model_id(char *str)
     return 0;
 }
 
-/* Fill a X86CPUDefinition struct with information about the host CPU, and
- * the CPU features supported by the host hardware + host kernel
+static X86CPUDefinition host_cpudef;
+
+/* class_init for the "host" CPU model
  *
- * This function may be called only if KVM is enabled.
+ * This function may be called before KVM is initialized.
  */
-static void kvm_cpu_fill_host(X86CPUDefinition *x86_cpu_def)
+static void x86_cpu_class_init_host(ObjectClass *oc, void *data)
 {
-    KVMState *s = kvm_state;
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
-    assert(kvm_enabled());
+    xcc->kvm_required = true;
 
-    x86_cpu_def->name = "host";
-    x86_cpu_def->cache_info_passthrough = true;
     host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
+    x86_cpu_vendor_words2str(host_cpudef.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;
+    host_cpudef.family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
+    host_cpudef.model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
+    host_cpudef.stepping = eax & 0x0F;
 
-    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
-    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
-    x86_cpu_def->xlevel2 =
-        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    cpu_x86_fill_model_id(host_cpudef.model_id);
 
-    cpu_x86_fill_model_id(x86_cpu_def->model_id);
+    xcc->cpu_def = &host_cpudef;
+    xcc->model_description =
+        "KVM processor with all supported host features "
+        "(only available in KVM mode)";
+
+    host_cpudef.cache_info_passthrough = true;
+
+    /* level, xlevel, xlevel2, and the feature words are initialized on
+     * instance_init, because they require KVM to be initialized.
+     */
+}
+
+static void x86_cpu_instance_init_host(Object *obj)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    KVMState *s = kvm_state;
+    Error *err = NULL;
+
+    assert(kvm_enabled());
+
+    env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+    env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+    env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
 
     FeatureWord w;
     for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
-        x86_cpu_def->features[w] =
+        env->features[w] =
             kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
                                          wi->cpuid_reg);
     }
+    object_property_set_bool(OBJECT(cpu), true, "pmu", &err);
+
+    assert_no_error(err);
 }
 
+
+static const TypeInfo x86_cpu_host_type_info = {
+    .name = CPU_CLASS_NAME("host"),
+    .parent = TYPE_X86_CPU,
+    .instance_size = sizeof(X86CPU),
+    .instance_init = x86_cpu_instance_init_host,
+    .abstract = false,
+    .class_size = sizeof(X86CPUClass),
+    .class_init = x86_cpu_class_init_host,
+};
+
 static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
 {
     int i;
@@ -1539,34 +1635,6 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque,
     error_propagate(errp, err);
 }
 
-static int cpu_x86_find_by_name(X86CPU *cpu, X86CPUDefinition *x86_cpu_def,
-                                const char *name)
-{
-    X86CPUDefinition *def;
-    Error *err = NULL;
-    int i;
-
-    if (name == NULL) {
-        return -1;
-    }
-    if (kvm_enabled() && strcmp(name, "host") == 0) {
-        kvm_cpu_fill_host(x86_cpu_def);
-        object_property_set_bool(OBJECT(cpu), true, "pmu", &err);
-        assert_no_error(err);
-        return 0;
-    }
-
-    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));
-            return 0;
-        }
-    }
-
-    return -1;
-}
-
 /* Convert all '_' in a feature string option name to '-', to make feature
  * name conform to QOM property naming rule, which uses '-' instead of '_'.
  */
@@ -1729,23 +1797,63 @@ static void listflags(char *buf, int bufsize, uint32_t fbits,
         }
 }
 
-/* generate CPU information. */
+/* Sort alphabetically by type name, listing kvm_required models last. */
+static gint x86_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *class_a = (ObjectClass *)a;
+    ObjectClass *class_b = (ObjectClass *)b;
+    X86CPUClass *cc_a = X86_CPU_CLASS(class_a);
+    X86CPUClass *cc_b = X86_CPU_CLASS(class_b);
+    const char *name_a, *name_b;
+
+    if (cc_a->kvm_required != cc_b->kvm_required) {
+        /* kvm_required items go last */
+        return cc_a->kvm_required ? 1 : -1;
+    } else {
+        name_a = object_class_get_name(class_a);
+        name_b = object_class_get_name(class_b);
+        return strcmp(name_a, name_b);
+    }
+}
+
+static GSList *get_sorted_cpu_model_list(void)
+{
+    GSList *list = object_class_get_list(TYPE_X86_CPU, false);
+    list = g_slist_sort(list, x86_cpu_list_compare);
+    return list;
+}
+
+static void x86_cpu_list_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    X86CPUClass *cc = X86_CPU_CLASS(oc);
+    CPUListState *s = user_data;
+    char *name = x86_cpu_class_get_model_name(cc);
+    const char *desc = cc->model_description;
+    if (!desc) {
+        desc = cc->cpu_def->model_id;
+    }
+
+    (*s->cpu_fprintf)(s->file, "x86 %16s  %-48s\n",
+                      name, desc);
+    g_free(name);
+}
+
+/* list available CPU models and flags */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    X86CPUDefinition *def;
-    char buf[256];
     int i;
+    CPUListState s = {
+        .file = f,
+        .cpu_fprintf = cpu_fprintf,
+    };
+    GSList *list;
+    char buf[256];
 
-    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-        def = &builtin_x86_defs[i];
-        snprintf(buf, sizeof(buf), "%s", def->name);
-        (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
-    }
-#ifdef CONFIG_KVM
-    (*cpu_fprintf)(f, "x86 %16s  %-48s\n", "host",
-                   "KVM processor with all supported host features "
-                   "(only available in KVM mode)");
-#endif
+    (*cpu_fprintf)(f, "Available CPUs:\n");
+    list = get_sorted_cpu_model_list();
+    g_slist_foreach(list, x86_cpu_list_entry, &s);
+    g_slist_free(list);
 
     (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
     for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -1756,26 +1864,29 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     }
 }
 
+static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    X86CPUClass *cc = X86_CPU_CLASS(oc);
+    CpuDefinitionInfoList **cpu_list = user_data;
+    CpuDefinitionInfoList *entry;
+    CpuDefinitionInfo *info;
+
+    info = g_malloc0(sizeof(*info));
+    info->name = x86_cpu_class_get_model_name(cc);
+
+    entry = g_malloc0(sizeof(*entry));
+    entry->value = info;
+    entry->next = *cpu_list;
+    *cpu_list = entry;
+}
+
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
-    X86CPUDefinition *def;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-        CpuDefinitionInfoList *entry;
-        CpuDefinitionInfo *info;
-
-        def = &builtin_x86_defs[i];
-        info = g_malloc0(sizeof(*info));
-        info->name = g_strdup(def->name);
-
-        entry = g_malloc0(sizeof(*entry));
-        entry->value = info;
-        entry->next = cpu_list;
-        cpu_list = entry;
-    }
-
+    GSList *list = get_sorted_cpu_model_list();
+    g_slist_foreach(list, x86_cpu_definition_entry, &cpu_list);
+    g_slist_free(list);
     return cpu_list;
 }
 
@@ -1798,19 +1909,11 @@ static void filter_features_for_kvm(X86CPU *cpu)
     }
 }
 
-/* Load CPU definition for a given CPU model name
+/* Load data from X86CPUDefinition
  */
-static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
+static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
-    X86CPUDefinition def1, *def = &def1;
-
-    memset(def, 0, sizeof(*def));
-
-    if (cpu_x86_find_by_name(cpu, def, name) < 0) {
-        error_setg(errp, "Unable to find CPU definition: %s", name);
-        return;
-    }
 
     object_property_set_int(OBJECT(cpu), def->level, "level", errp);
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
@@ -1862,7 +1965,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     X86CPU *cpu = NULL;
     gchar **model_pieces;
     char *name, *features;
-    char *typename;
     Error *error = NULL;
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
@@ -1873,12 +1975,19 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     name = model_pieces[0];
     features = model_pieces[1];
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    x86_cpu_load_def(cpu, name, &error);
-    if (error) {
+    X86CPUClass *cc = x86_cpu_class_by_name(name);
+    if (!cc) {
+        error_setg(&error, "Unable to find CPU definition: %s", name);
+        goto out;
+     }
+
+    if (cc->kvm_required && !kvm_enabled()) {
+        error_setg(&error, "CPU model '%s' requires KVM", name);
         goto out;
     }
 
+    cpu = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(cc))));
+
 #ifndef CONFIG_USER_ONLY
     if (icc_bridge == NULL) {
         error_setg(&error, "Invalid icc-bridge value");
@@ -1888,14 +1997,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     object_unref(OBJECT(cpu));
 #endif
 
-    /* Emulate per-model subclasses for global properties */
-    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
-    qdev_prop_set_globals_for_type(DEVICE(cpu), typename, &error);
-    g_free(typename);
-    if (error) {
-        goto out;
-    }
-
     cpu_x86_parse_featurestr(cpu, features, &error);
     if (error) {
         goto out;
@@ -1904,8 +2005,10 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
 out:
     if (error != NULL) {
         error_propagate(errp, error);
-        object_unref(OBJECT(cpu));
-        cpu = NULL;
+        if (cpu) {
+            object_unref(OBJECT(cpu));
+            cpu = NULL;
+        }
     }
     g_strfreev(model_pieces);
     return cpu;
@@ -1944,30 +2047,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) {
-        X86CPUDefinition *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)
 {
@@ -2595,6 +2674,7 @@ static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
     X86CPU *cpu = X86_CPU(obj);
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
     CPUX86State *env = &cpu->env;
     static int inited;
 
@@ -2646,6 +2726,11 @@ static void x86_cpu_initfn(Object *obj)
         cpu_set_debug_excp_handler(breakpoint_handler);
 #endif
     }
+
+    X86CPUDefinition *def = xcc->cpu_def;
+    Error *err = NULL;
+    x86_cpu_load_def(cpu, def, &err);
+    assert_no_error(err);
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
@@ -2722,7 +2807,7 @@ 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,
 };
@@ -2730,6 +2815,8 @@ static const TypeInfo x86_cpu_type_info = {
 static void x86_cpu_register_types(void)
 {
     type_register_static(&x86_cpu_type_info);
+    x86_register_cpudef_classes();
+    type_register_static(&x86_cpu_host_type_info);
 }
 
 type_init(x86_cpu_register_types)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ea373e8..ea21ff3 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -925,7 +925,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
                        Error **errp);
 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);
@@ -1118,7 +1117,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
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/7] x86 CPU subclasses, take 6

[Eduardo Habkost]

Ping? No comments, even about the first few patches (that are fairly
simple)?


On Wed, Nov 27, 2013 at 05:34:02PM -0200, Eduardo Habkost wrote:
> I want to try to get this in 1.8, because I have found one additional use-case
> for the new subclasses:
> 
> libvirt needs to be able to query details about the existing CPU models, and it
> can't do that today without restarting QEMU every time. Having separate classes
> for each CPU model allows libvirt to create/destroy CPU objects in a loop just
> to query the resulting properties (especially "feature-words") for each CPU
> model.
> 
> This version is closer to the version sent by Andrea and then later resubmitted
> by Igor as "[RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM
> subclasses", than the previous version I have sent, as it doesn't create one new
> class_init function for each subclass. One main difference is that this version
> does not use KVM-specific subclasses, to keep things simpler.
> 
> Another difference is that instead of late registration of the "host" class, I
> simply changed the "host" subclass to do KVM-dependent initialization steps on
> instance_init instead of class_init. This way we won't require any
> initialization-ordering tricks. In the past this was a problem, but today the
> global properties are being set by qdev code on post_init, so we can safely
> initialize property defaults on instance_init without having to worry that it
> would break the setting of global properties.
> 
> This series needs to be applied on top of:
>    Subject: [PATCH 0/8] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()
>    Message-Id: <1385322940-27325-1-git-send-email-ehabkost@redhat.com>
>    http://article.gmane.org/gmane.comp.emulators.qemu/243013
> 
> 
> Eduardo Habkost (7):
>   target-i386: Eliminate CONFIG_KVM #ifdefs
>   target-i386: Don't change x86_def_t struct on cpu_x86_register()
>   target-i386: Move KVM default-vendor hack to instance_init
>   target-i386: Rename cpu_x86_register() to x86_cpu_load_def()
>   target-i386: Call x86_cpu_load_def() earlier
>   target-i386: Rename x86_def_t to X86CPUDefinition
>   target-i386: CPU model subclasses
> 
>  target-i386/cpu-qom.h |  13 ++
>  target-i386/cpu.c     | 407 ++++++++++++++++++++++++++++++--------------------
>  target-i386/cpu.h     |   2 -
>  3 files changed, 260 insertions(+), 162 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/7] target-i386: Eliminate CONFIG_KVM #ifdefs

[Paolo Bonzini]

Il 27/11/2013 20:34, Eduardo Habkost ha scritto:
> The compiler is capable of eliminating the KVM-specific function calls
> as long as the calling function has an assert(kvm_enabled()) line, so we
> don't need to wrap all KVM-specific inside #ifdefs.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d747e04..b525592 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -373,7 +373,6 @@ void disable_kvm_pv_eoi(void)
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>  {
> -#if defined(CONFIG_KVM)
>      uint32_t vec[4];
>  
>  #ifdef __x86_64__
> @@ -401,7 +400,6 @@ void host_cpuid(uint32_t function, uint32_t count,
>          *ecx = vec[2];
>      if (edx)
>          *edx = vec[3];
> -#endif
>  }

NACK for this one.  It includes an "asm", it is plausible that it fails
to parse before dead-code elimination.  Rather, change the #else to an
#elif, and add a new #else with an abort inside.

Otherwise looks good.

Paolo

>  #define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
> @@ -1118,7 +1116,6 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
>      }
>  }
>  
> -#ifdef CONFIG_KVM
>  static int cpu_x86_fill_model_id(char *str)
>  {
>      uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> @@ -1133,7 +1130,6 @@ static int cpu_x86_fill_model_id(char *str)
>      }
>      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
> @@ -1142,7 +1138,6 @@ static int cpu_x86_fill_model_id(char *str)
>   */
>  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;
>  
> @@ -1172,8 +1167,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>              kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
>                                           wi->cpuid_reg);
>      }
> -
> -#endif /* CONFIG_KVM */
>  }
>  
>  static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
> @@ -1798,13 +1791,14 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>      return cpu_list;
>  }
>  
> -#ifdef CONFIG_KVM
>  static void filter_features_for_kvm(X86CPU *cpu)
>  {
>      CPUX86State *env = &cpu->env;
>      KVMState *s = kvm_state;
>      FeatureWord w;
>  
> +    assert(kvm_enabled());
> +
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          FeatureWordInfo *wi = &feature_word_info[w];
>          uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
> @@ -1815,7 +1809,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
>          cpu->filtered_features[w] = requested_features & ~env->features[w];
>      }
>  }
> -#endif
>  
>  static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
>  {
> @@ -2525,9 +2518,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>                         "Host's CPU doesn't support requested features");
>              goto out;
>          }
> -#ifdef CONFIG_KVM
>          filter_features_for_kvm(cpu);
> -#endif
>      }
>  
>  #ifndef CONFIG_USER_ONLY
> 

Re: [Qemu-devel] [PATCH 0/7] x86 CPU subclasses, take 6

[Paolo Bonzini]

Il 09/12/2013 19:07, Eduardo Habkost ha scritto:
> 
> Ping? No comments, even about the first few patches (that are fairly
> simple)?

Just a small comment on patches 1-6.  I didn't look at 7 yet.

Paolo

[Qemu-devel] [PATCH 1/7 v2] target-i386: Eliminate CONFIG_KVM #ifdefs

[Eduardo Habkost]

The compiler is capable of eliminating the KVM-specific function calls
as long as the calling function has an assert(kvm_enabled()) line, so we
don't need to wrap all KVM-specific code inside #ifdefs.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 * Check for __i386__ on host_cpuid() so the code compiles properly
   on non-x86 hosts
---
 target-i386/cpu.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 783b3d0..211cdf1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -373,7 +373,6 @@ void disable_kvm_pv_eoi(void)
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
-#if defined(CONFIG_KVM)
     uint32_t vec[4];
 
 #ifdef __x86_64__
@@ -381,7 +380,7 @@ void host_cpuid(uint32_t function, uint32_t count,
                  : "=a"(vec[0]), "=b"(vec[1]),
                    "=c"(vec[2]), "=d"(vec[3])
                  : "0"(function), "c"(count) : "cc");
-#else
+#elif defined(__i386__)
     asm volatile("pusha \n\t"
                  "cpuid \n\t"
                  "mov %%eax, 0(%2) \n\t"
@@ -391,6 +390,8 @@ void host_cpuid(uint32_t function, uint32_t count,
                  "popa"
                  : : "a"(function), "c"(count), "S"(vec)
                  : "memory", "cc");
+#else
+    abort();
 #endif
 
     if (eax)
@@ -401,7 +402,6 @@ void host_cpuid(uint32_t function, uint32_t count,
         *ecx = vec[2];
     if (edx)
         *edx = vec[3];
-#endif
 }
 
 #define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
@@ -1118,7 +1118,6 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
     }
 }
 
-#ifdef CONFIG_KVM
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1133,7 +1132,6 @@ static int cpu_x86_fill_model_id(char *str)
     }
     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
@@ -1142,7 +1140,6 @@ static int cpu_x86_fill_model_id(char *str)
  */
 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;
 
@@ -1172,8 +1169,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
             kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
                                          wi->cpuid_reg);
     }
-
-#endif /* CONFIG_KVM */
 }
 
 static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
@@ -1798,13 +1793,14 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
-#ifdef CONFIG_KVM
 static void filter_features_for_kvm(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
     KVMState *s = kvm_state;
     FeatureWord w;
 
+    assert(kvm_enabled());
+
     for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
         uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
@@ -1815,7 +1811,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
         cpu->filtered_features[w] = requested_features & ~env->features[w];
     }
 }
-#endif
 
 static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
 {
@@ -2525,9 +2520,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                        "Host's CPU doesn't support requested features");
             goto out;
         }
-#ifdef CONFIG_KVM
         filter_features_for_kvm(cpu);
-#endif
     }
 
 #ifndef CONFIG_USER_ONLY
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/7 v2] target-i386: Eliminate CONFIG_KVM #ifdefs

[Richard Henderson]

On 12/10/2013 10:55 AM, Eduardo Habkost wrote:
> The compiler is capable of eliminating the KVM-specific function calls
> as long as the calling function has an assert(kvm_enabled()) line, so we
> don't need to wrap all KVM-specific code inside #ifdefs.

Really?  In tcg/tcg.h we force NDEBUG if not CONFIG_TCG_DEBUG, which makes
assert expand to nothing.  This statement may be true for some files, but
almost everything under target-i386 includes tcg.h.

Although I know we've talked within glibc and gcc the de-optimization of
missing out on assert info, and how we ought to use __builtin_gcc_unreachable
in order to retain that, we've still not done anything official with <assert.h>.

That said, I don't disagree with the changes, if they work with a forced
-DNDEBUG, i.e. unreachable code that still compiles.


r~

Re: [Qemu-devel] [PATCH 1/7 v2] target-i386: Eliminate CONFIG_KVM #ifdefs

[Eduardo Habkost]

On Tue, Dec 10, 2013 at 05:36:10PM -0800, Richard Henderson wrote:
> On 12/10/2013 10:55 AM, Eduardo Habkost wrote:
> > The compiler is capable of eliminating the KVM-specific function calls
> > as long as the calling function has an assert(kvm_enabled()) line, so we
> > don't need to wrap all KVM-specific code inside #ifdefs.
> 
> Really?  In tcg/tcg.h we force NDEBUG if not CONFIG_TCG_DEBUG, which makes
> assert expand to nothing.  This statement may be true for some files, but
> almost everything under target-i386 includes tcg.h.
> 
> Although I know we've talked within glibc and gcc the de-optimization of
> missing out on assert info, and how we ought to use __builtin_gcc_unreachable
> in order to retain that, we've still not done anything official with <assert.h>.
> 
> That said, I don't disagree with the changes, if they work with a forced
> -DNDEBUG, i.e. unreachable code that still compiles.

Oops, I didn't consider -DNDEBUG. You are right and assert() can't be
considered a sufficient hint to the compiler.

I remember making some tests where assert(kvm_enabled()) helped the
compiler, but I can't reproduce it anymore (maybe it was in an older
version of the code). I have just tested this patch using -DNEBUG
combined with: -O, -O0, -O1, -O2, and without -O. In all cases, QEMU was
built successfully with CONFIG_KVM disabled.

I will resubmit the patch without the assert() line and with a more
accurate commit message.

-- 
Eduardo

[Qemu-devel] [PATCH 1/7 v3] target-i386: Eliminate CONFIG_KVM #ifdefs

[Eduardo Habkost]

GCC is able to eliminate the kvm_arch_get_supported_cpuid() calls in
kvm_cpu_fill_host() and filter_features_for_kvm(), so we can eliminate
the CONFIG_KVM #ifdefs there.

Also, kvm_cpu_fill_host() and host_cpuid() don't need to check
CONFIG_KVM, as they don't have any KVM-specific function calls.

Tested to build successfully with CONFIG_KVM disabled, using the
following CFLAGS combinations: "-DNDEBUG", "-DNDEBUG -O', "-DNDEBUG
-O0", "-DNDEBUG -O1", "-DNDEBUG -O2".

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 * Check for __i386__ on host_cpuid() so the code compiles properly
   on non-x86 hosts.
   Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

Change v3:
 * Don't add assert(kvm_enabled()) line, which is not necessary to help
   the compiler (and wouldn't help it if using -DNDEBUG, anyway).
   Reported-by: Richard Henderson <rth@twiddle.net>
 * Commit message update
---
 target-i386/cpu.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 783b3d0..875740a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -373,7 +373,6 @@ void disable_kvm_pv_eoi(void)
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
-#if defined(CONFIG_KVM)
     uint32_t vec[4];
 
 #ifdef __x86_64__
@@ -381,7 +380,7 @@ void host_cpuid(uint32_t function, uint32_t count,
                  : "=a"(vec[0]), "=b"(vec[1]),
                    "=c"(vec[2]), "=d"(vec[3])
                  : "0"(function), "c"(count) : "cc");
-#else
+#elif defined(__i386__)
     asm volatile("pusha \n\t"
                  "cpuid \n\t"
                  "mov %%eax, 0(%2) \n\t"
@@ -391,6 +390,8 @@ void host_cpuid(uint32_t function, uint32_t count,
                  "popa"
                  : : "a"(function), "c"(count), "S"(vec)
                  : "memory", "cc");
+#else
+    abort();
 #endif
 
     if (eax)
@@ -401,7 +402,6 @@ void host_cpuid(uint32_t function, uint32_t count,
         *ecx = vec[2];
     if (edx)
         *edx = vec[3];
-#endif
 }
 
 #define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
@@ -1118,7 +1118,6 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
     }
 }
 
-#ifdef CONFIG_KVM
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1133,7 +1132,6 @@ static int cpu_x86_fill_model_id(char *str)
     }
     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
@@ -1142,7 +1140,6 @@ static int cpu_x86_fill_model_id(char *str)
  */
 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;
 
@@ -1172,8 +1169,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
             kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
                                          wi->cpuid_reg);
     }
-
-#endif /* CONFIG_KVM */
 }
 
 static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
@@ -1798,7 +1793,6 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
-#ifdef CONFIG_KVM
 static void filter_features_for_kvm(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
@@ -1815,7 +1809,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
         cpu->filtered_features[w] = requested_features & ~env->features[w];
     }
 }
-#endif
 
 static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
 {
@@ -2525,9 +2518,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                        "Host's CPU doesn't support requested features");
             goto out;
         }
-#ifdef CONFIG_KVM
         filter_features_for_kvm(cpu);
-#endif
     }
 
 #ifndef CONFIG_USER_ONLY
-- 
1.8.3.1