[Qemu-devel] [PATCH 0/6] target-i386: Increase CPUID level/xlevel/xlevel2 automatically

[Qemu-devel] [PATCH 0/6] target-i386: Increase CPUID level/xlevel/xlevel2 automatically

[Eduardo Habkost]

This series fixes the inconsistency between CPUID[7].EBX features
and all the rest of the configurable CPU features. This ensures
that level/xlevel/xlevel2 will be set to appropriate values
depending on the set of features enabled in a CPU model or in the
command-line.

Eduardo Habkost (6):
  target-i386: Remove unused X86CPUDefinition::xlevel2 field
  target-i386: Add a marker to end of the region zeroed on reset
  tests: Add test code for CPUID level/xlevel handling
  tests: Test CPUID level handling for old machines
  target-i386: Automatically set level/xlevel/xlevel2 when needed
  target-i386: Enable CPUID[0x8000000A] if SVM is enabled

 include/hw/i386/pc.h          |  27 ++++++++-
 target-i386/cpu.c             |  94 +++++++++++++++++++++++++++----
 target-i386/cpu.h             |  16 +++++-
 tests/Makefile.include        |   2 +
 tests/test-x86-cpuid-compat.c | 127 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 252 insertions(+), 14 deletions(-)
 create mode 100644 tests/test-x86-cpuid-compat.c

-- 
2.7.4

[Qemu-devel] [PATCH 1/6] target-i386: Remove unused X86CPUDefinition::xlevel2 field

[Eduardo Habkost]

No CPU model in builtin_x86_defs has xlevel2 set, so it is always
zero. Delete the field.

Note that this is not an user-visible change. It doesn't remove
the ability to set xlevel2 on the command-line, it just removes
an unused field in builtin_x86_defs.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index db12728..920b78f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -761,7 +761,6 @@ struct X86CPUDefinition {
     const char *name;
     uint32_t level;
     uint32_t xlevel;
-    uint32_t xlevel2;
     /* vendor is zero-terminated, 12 character ASCII string */
     char vendor[CPUID_VENDOR_SZ + 1];
     int family;
@@ -2214,7 +2213,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     object_property_set_int(OBJECT(cpu), def->model, "model", errp);
     object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
     object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
-    object_property_set_int(OBJECT(cpu), def->xlevel2, "xlevel2", errp);
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
     for (w = 0; w < FEATURE_WORDS; w++) {
         env->features[w] = def->features[w];
-- 
2.7.4

[Qemu-devel] [PATCH 2/6] target-i386: Add a marker to end of the region zeroed on reset

[Eduardo Habkost]

Instead of using cpuid_level, use an empty struct as a marker
(like we already did with {start,end}_init_save). This will avoid
accidentaly resetting the wrong fields if we change the field
ordering on CPUX86State.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 920b78f..26f0e59 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2714,7 +2714,7 @@ static void x86_cpu_reset(CPUState *s)
 
     xcc->parent_reset(s);
 
-    memset(env, 0, offsetof(CPUX86State, cpuid_level));
+    memset(env, 0, offsetof(CPUX86State, end_reset_fields));
 
     tlb_flush(s, 1);
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 27af9c3..604d591 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1108,6 +1108,7 @@ typedef struct CPUX86State {
     CPU_COMMON
 
     /* Fields from here on are preserved across CPU reset. */
+    struct {} end_reset_fields;
 
     /* processor features (e.g. for CPUID insn) */
     uint32_t cpuid_level;
-- 
2.7.4

[Qemu-devel] [PATCH 3/6] tests: Add test code for CPUID level/xlevel handling

[Eduardo Habkost]

Add test code that will check if the automatic CPUID level
changes are working as expected.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/Makefile.include        |  2 +
 tests/test-x86-cpuid-compat.c | 95 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)
 create mode 100644 tests/test-x86-cpuid-compat.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6052a38..5a98962 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -242,6 +242,7 @@ check-qtest-i386-y += tests/test-netfilter$(EXESUF)
 check-qtest-i386-y += tests/test-filter-mirror$(EXESUF)
 check-qtest-i386-y += tests/test-filter-redirector$(EXESUF)
 check-qtest-i386-y += tests/postcopy-test$(EXESUF)
+check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -656,6 +657,7 @@ tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-o
 tests/test-netfilter$(EXESUF): tests/test-netfilter.o $(qtest-obj-y)
 tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y)
 tests/test-filter-redirector$(EXESUF): tests/test-filter-redirector.o $(qtest-obj-y)
+tests/test-x86-cpuid-compat$(EXESUF): tests/test-x86-cpuid-compat.o $(qtest-obj-y)
 tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y)
 tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o
 
diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
new file mode 100644
index 0000000..9239a9a
--- /dev/null
+++ b/tests/test-x86-cpuid-compat.c
@@ -0,0 +1,95 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qint.h"
+#include "libqtest.h"
+
+static char *get_cpu0_qom_path(void)
+{
+    QDict *resp;
+    QList *ret;
+    QDict *cpu0;
+    char *path;
+
+    resp = qmp("{'execute': 'query-cpus', 'arguments': {}}");
+    g_assert(qdict_haskey(resp, "return"));
+    ret = qdict_get_qlist(resp, "return");
+
+    cpu0 = qobject_to_qdict(qlist_peek(ret));
+    path = g_strdup(qdict_get_str(cpu0, "qom_path"));
+    QDECREF(resp);
+    return path;
+}
+
+static QObject *qom_get(const char *path, const char *prop)
+{
+    QDict *resp = qmp("{ 'execute': 'qom-get',"
+                      "  'arguments': { 'path': %s,"
+                      "                 'property': %s } }",
+                      path, prop);
+    QObject *ret = qdict_get(resp, "return");
+    qobject_incref(ret);
+    QDECREF(resp);
+    return ret;
+}
+
+typedef struct CpuidTestArgs {
+    const char *cmdline;
+    const char *property;
+    int64_t expected_value;
+} CpuidTestArgs;
+
+static void test_cpuid_prop(const void *data)
+{
+    const CpuidTestArgs *args = data;
+    char *path;
+    QInt *value;
+
+    qtest_start(args->cmdline);
+    path = get_cpu0_qom_path();
+    value = qobject_to_qint(qom_get(path, args->property));
+    g_assert_cmpint(qint_get_int(value), ==, args->expected_value);
+    qtest_end();
+
+    QDECREF(value);
+    g_free(path);
+}
+
+static void add_cpuid_test(const char *name, const char *cmdline,
+                           const char *property, int64_t expected_value)
+{
+    CpuidTestArgs *args = g_new0(CpuidTestArgs, 1);
+    args->cmdline = cmdline;
+    args->property = property;
+    args->expected_value = expected_value;
+    qtest_add_data_func(name, args, test_cpuid_prop);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    /* Original level values for CPU models: */
+    add_cpuid_test("x86/cpuid/phenom/level", "-cpu phenom", "level", 5);
+    add_cpuid_test("x86/cpuid/Conroe/level", "-cpu Conroe", "level", 10);
+    add_cpuid_test("x86/cpuid/SandyBridge/level", "-cpu SandyBridge", "level", 0xd);
+    add_cpuid_test("x86/cpuid/486/xlevel", "-cpu 486", "xlevel", 0);
+    add_cpuid_test("x86/cpuid/core2duo/xlevel", "-cpu core2duo", "xlevel", 0x80000008);
+    add_cpuid_test("x86/cpuid/phenom/xlevel", "-cpu phenom", "xlevel", 0x8000001A);
+
+    /* If level is not large enough, it should increase automatically: */
+    /* CPUID[EAX=7,ECX=0].EBX: */
+    add_cpuid_test("x86/cpuid/auto-level/phenom/fsgsbase", "-cpu phenom,+fsgsbase", "level", 7);
+
+    /* If level is already large enough, it shouldn't change: */
+    add_cpuid_test("x86/cpuid/auto-level/SandyBridge/multiple", "-cpu SandyBridge,+arat,+fsgsbase,+avx512vbmi", "level", 0xd);
+
+    /* if xlevel is already large enough, it shouldn't change: */
+    add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);
+
+    /* if xlevel2 is already large enough, it shouldn't change: */
+    add_cpuid_test("x86/cpuid/auto-xlevel2/486/fixed", "-cpu 486,xlevel2=0xC0000002,+xstore", "xlevel2", 0xC0000002);
+
+    return g_test_run();
+}
-- 
2.7.4

[Qemu-devel] [PATCH 4/6] tests: Test CPUID level handling for old machines

[Eduardo Habkost]

We're going to change the way level/xlevel/xlevel2 are handled
when enabling features, but we need to keep the old behavior on
existing machine types. Add test cases for that.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/test-x86-cpuid-compat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
index 9239a9a..c65507f 100644
--- a/tests/test-x86-cpuid-compat.c
+++ b/tests/test-x86-cpuid-compat.c
@@ -91,5 +91,11 @@ int main(int argc, char **argv)
     /* if xlevel2 is already large enough, it shouldn't change: */
     add_cpuid_test("x86/cpuid/auto-xlevel2/486/fixed", "-cpu 486,xlevel2=0xC0000002,+xstore", "xlevel2", 0xC0000002);
 
+    /* Compatibility test for older machine-types that don't auto-increase level/xlevel/xlevel2: */
+
+    add_cpuid_test("x86/cpuid/auto-level/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+arat,+avx512vbmi,+xsaveopt", "level", 1);
+    add_cpuid_test("x86/cpuid/auto-xlevel/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0);
+    add_cpuid_test("x86/cpuid/auto-xlevel2/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+xstore", "xlevel2", 0);
+
     return g_test_run();
 }
-- 
2.7.4

[Qemu-devel] [PATCH 5/6] target-i386: Automatically set level/xlevel/xlevel2 when needed

[Eduardo Habkost]

Instead of requiring users and management software to be aware of
required CPUID level/xlevel/xlevel2 values for each feature,
automatically increase those values when features need them.

This was already done for CPUID[7].EBX, and is now made generic
for all CPUID feature flags. Unit test included, to make sure we
don't break ABI on older machine-types and don't mess with the
CPUID level values if they are explicitly set by the user.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/i386/pc.h          | 27 +++++++++++++-
 target-i386/cpu.c             | 84 +++++++++++++++++++++++++++++++++++++++----
 target-i386/cpu.h             | 15 ++++++--
 tests/test-x86-cpuid-compat.c | 23 ++++++++++++
 4 files changed, 138 insertions(+), 11 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ab8e319..18878d7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -376,7 +376,32 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_7 \
     PC_COMPAT_2_8 \
-    HW_COMPAT_2_7
+    HW_COMPAT_2_7 \
+    {\
+        .driver   = TYPE_X86_CPU,\
+        .property = "cpuid-auto-level-6",\
+        .value    = "off",\
+    },\
+    {\
+        .driver   = TYPE_X86_CPU,\
+        .property = "cpuid-auto-level-7-0-ecx",\
+        .value    = "off",\
+    },\
+    {\
+        .driver   = TYPE_X86_CPU,\
+        .property = "max-level",\
+        .value    = stringify(7),\
+    },\
+    {\
+        .driver   = TYPE_X86_CPU,\
+        .property = "max-xlevel",\
+        .value    = stringify(0),\
+    },\
+    {\
+        .driver   = TYPE_X86_CPU,\
+        .property = "max-xlevel2",\
+        .value    = stringify(0),\
+    },
 
 #define PC_COMPAT_2_6 \
     HW_COMPAT_2_6 \
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 26f0e59..1ebd7cc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1643,9 +1643,9 @@ static void host_x86_cpu_initfn(Object *obj)
 
     /* If KVM is disabled, x86_cpu_realizefn() will report an error later */
     if (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);
+        env->cpuid_min_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+        env->cpuid_min_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+        env->cpuid_min_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
 
         if (lmce_supported()) {
             object_property_set_bool(OBJECT(cpu), true, "lmce", &error_abort);
@@ -2208,11 +2208,13 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     char host_vendor[CPUID_VENDOR_SZ + 1];
     FeatureWord w;
 
-    object_property_set_int(OBJECT(cpu), def->level, "level", errp);
+    /* CPU models only set _minimum_ values for level/xlevel: */
+    object_property_set_int(OBJECT(cpu), def->level, "min-level", errp);
+    object_property_set_int(OBJECT(cpu), def->xlevel, "min-xlevel", errp);
+
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
     object_property_set_int(OBJECT(cpu), def->model, "model", errp);
     object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
-    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
     for (w = 0; w < FEATURE_WORDS; w++) {
         env->features[w] = def->features[w];
@@ -2951,6 +2953,42 @@ static uint32_t x86_host_phys_bits(void)
     return host_phys_bits;
 }
 
+static void x86_cpu_adjust_level(X86CPU *cpu, uint32_t *min, uint32_t max,
+                                uint32_t value)
+{
+    if (value <= max && *min < value) {
+        *min = value;
+    }
+}
+
+/* Increase cpuid_min_{level,xlevel,xlevel2} automatically, if appropriate */
+static void x86_cpu_adjust_feat_level(X86CPU *cpu, FeatureWord w)
+{
+    CPUX86State *env = &cpu->env;
+    FeatureWordInfo *fi = &feature_word_info[w];
+    uint32_t eax = fi->cpuid_eax;
+    uint32_t region = eax & 0xF0000000;
+
+    if (!env->features[w]) {
+        return;
+    }
+
+    switch (region) {
+        case 0x00000000:
+            x86_cpu_adjust_level(cpu, &env->cpuid_min_level,
+                                 env->cpuid_max_level, eax);
+        break;
+        case 0x80000000:
+            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
+                                 env->cpuid_max_xlevel, eax);
+        break;
+        case 0xC0000000:
+            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel2,
+                                 env->cpuid_max_xlevel2, eax);
+        break;
+    }
+}
+
 #define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
                            (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
                            (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
@@ -2996,8 +3034,32 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu->env.features[w] &= ~minus_features[w];
     }
 
-    if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
-        env->cpuid_level = 7;
+
+    x86_cpu_adjust_feat_level(cpu, FEAT_1_EDX);
+    x86_cpu_adjust_feat_level(cpu, FEAT_1_ECX);
+    if (cpu->cpuid_auto_level_6) {
+        x86_cpu_adjust_feat_level(cpu, FEAT_6_EAX);
+    }
+    x86_cpu_adjust_feat_level(cpu, FEAT_7_0_EBX);
+    if (cpu->cpuid_auto_level_7_0_ecx) {
+        x86_cpu_adjust_feat_level(cpu, FEAT_7_0_ECX);
+    }
+    x86_cpu_adjust_feat_level(cpu, FEAT_8000_0001_EDX);
+    x86_cpu_adjust_feat_level(cpu, FEAT_8000_0001_ECX);
+    x86_cpu_adjust_feat_level(cpu, FEAT_8000_0007_EDX);
+    x86_cpu_adjust_feat_level(cpu, FEAT_C000_0001_EDX);
+    x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
+    x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
+
+    /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
+    if (!env->cpuid_level) {
+        env->cpuid_level = env->cpuid_min_level;
+    }
+    if (!env->cpuid_xlevel) {
+        env->cpuid_xlevel = env->cpuid_min_xlevel;
+    }
+    if (!env->cpuid_xlevel2) {
+        env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
     }
 
     if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
@@ -3406,6 +3468,14 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
+    DEFINE_PROP_UINT32("min-level", X86CPU, env.cpuid_min_level, 0),
+    DEFINE_PROP_UINT32("min-xlevel", X86CPU, env.cpuid_min_xlevel, 0),
+    DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
+    DEFINE_PROP_UINT32("max-level", X86CPU, env.cpuid_max_level, UINT32_MAX),
+    DEFINE_PROP_UINT32("max-xlevel", X86CPU, env.cpuid_max_xlevel, UINT32_MAX),
+    DEFINE_PROP_UINT32("max-xlevel2", X86CPU, env.cpuid_max_xlevel2, UINT32_MAX),
+    DEFINE_PROP_BOOL("cpuid-auto-level-7-0-ecx", X86CPU, cpuid_auto_level_7_0_ecx, true),
+    DEFINE_PROP_BOOL("cpuid-auto-level-6", X86CPU, cpuid_auto_level_6, true),
     DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
     DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
     DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 604d591..3798d2a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1111,9 +1111,12 @@ typedef struct CPUX86State {
     struct {} end_reset_fields;
 
     /* processor features (e.g. for CPUID insn) */
-    uint32_t cpuid_level;
-    uint32_t cpuid_xlevel;
-    uint32_t cpuid_xlevel2;
+    /* Minimum level/xlevel/xlevel2, based on CPU model + features */
+    uint32_t cpuid_min_level, cpuid_min_xlevel, cpuid_min_xlevel2;
+    /* Maximum level/xlevel/xlevel2 value for auto-assignment: */
+    uint32_t cpuid_max_level, cpuid_max_xlevel, cpuid_max_xlevel2;
+    /* Actual level/xlevel/xlevel2 value: */
+    uint32_t cpuid_level, cpuid_xlevel, cpuid_xlevel2;
     uint32_t cpuid_vendor1;
     uint32_t cpuid_vendor2;
     uint32_t cpuid_vendor3;
@@ -1218,6 +1221,12 @@ struct X86CPU {
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
+    /* Enable auto level-increase for CPUID[7].ECX features */
+    bool cpuid_auto_level_7_0_ecx;
+
+    /* Enable auto level-increase for CPUID[6] features */
+    bool cpuid_auto_level_6;
+
     /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
     bool fill_mtrr_mask;
 
diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
index c65507f..7051825 100644
--- a/tests/test-x86-cpuid-compat.c
+++ b/tests/test-x86-cpuid-compat.c
@@ -79,14 +79,37 @@ int main(int argc, char **argv)
     add_cpuid_test("x86/cpuid/phenom/xlevel", "-cpu phenom", "xlevel", 0x8000001A);
 
     /* If level is not large enough, it should increase automatically: */
+    /* CPUID[6].EAX: */
+    add_cpuid_test("x86/cpuid/auto-level/phenom/arat", "-cpu 486,+arat", "level", 6);
     /* CPUID[EAX=7,ECX=0].EBX: */
     add_cpuid_test("x86/cpuid/auto-level/phenom/fsgsbase", "-cpu phenom,+fsgsbase", "level", 7);
+    /* CPUID[EAX=7,ECX=0].ECX: */
+    add_cpuid_test("x86/cpuid/auto-level/phenom/avx512vbmi", "-cpu phenom,+avx512vbmi", "level", 7);
+    /* CPUID[EAX=0xd,ECX=1].EAX: */
+    add_cpuid_test("x86/cpuid/auto-level/phenom/xsaveopt", "-cpu phenom,+xsaveopt", "level", 0xd);
+    /* CPUID[8000_0001].EDX: */
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/3dnow", "-cpu 486,+3dnow", "xlevel", 0x80000001);
+    /* CPUID[8000_0001].ECX: */
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/sse4a", "-cpu 486,+sse4a", "xlevel", 0x80000001);
+    /* CPUID[8000_0007].EDX: */
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/invtsc", "-cpu 486,+invtsc", "xlevel", 0x80000007);
+    /* CPUID[8000_000A].EDX: */
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/npt", "-cpu 486,+npt", "xlevel", 0x8000000A);
+    /* CPUID[C000_0001].EDX: */
+    add_cpuid_test("x86/cpuid/auto-xlevel2/phenom/xstore", "-cpu phenom,+xstore", "xlevel2", 0xC0000001);
+
 
     /* If level is already large enough, it shouldn't change: */
     add_cpuid_test("x86/cpuid/auto-level/SandyBridge/multiple", "-cpu SandyBridge,+arat,+fsgsbase,+avx512vbmi", "level", 0xd);
+    /* If level is explicitly set, it shouldn't change: */
+    add_cpuid_test("x86/cpuid/auto-level/486/fixed/0xF", "-cpu 486,level=0xF,+arat,+fsgsbase,+avx512vbmi,+xsaveopt", "level", 0xF);
+    add_cpuid_test("x86/cpuid/auto-level/486/fixed/2", "-cpu 486,level=2,+arat,+fsgsbase,+avx512vbmi,+xsaveopt", "level", 2);
 
     /* if xlevel is already large enough, it shouldn't change: */
     add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);
+    /* If xlevel is explicitly set, it shouldn't change: */
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002", "-cpu 486,xlevel=0x80000002,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x80000002);
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A", "-cpu 486,xlevel=0x8000001A,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);
 
     /* if xlevel2 is already large enough, it shouldn't change: */
     add_cpuid_test("x86/cpuid/auto-xlevel2/486/fixed", "-cpu 486,xlevel2=0xC0000002,+xstore", "xlevel2", 0xC0000002);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 5/6] target-i386: Automatically set level/xlevel/xlevel2 when needed

[Richard Henderson]

On 09/21/2016 11:26 AM, Eduardo Habkost wrote:
> +    /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
> +    if (!env->cpuid_level) {
> +        env->cpuid_level = env->cpuid_min_level;
> +    }
> +    if (!env->cpuid_xlevel) {
> +        env->cpuid_xlevel = env->cpuid_min_xlevel;
> +    }
> +    if (!env->cpuid_xlevel2) {
> +        env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>      }

Why are we not bounding them by MIN, if it's really a minimum?

> +    /* Enable auto level-increase for CPUID[7].ECX features */
> +    bool cpuid_auto_level_7_0_ecx;
> +
> +    /* Enable auto level-increase for CPUID[6] features */
> +    bool cpuid_auto_level_6;

Why two variables?  Seems like only one is needed for backward compatibility. 
You're not considering adding a new one when cpuid[8].ebx is invented are you?


r~

Re: [Qemu-devel] [PATCH 5/6] target-i386: Automatically set level/xlevel/xlevel2 when needed

[Eduardo Habkost]

On Wed, Sep 21, 2016 at 12:53:08PM -0700, Richard Henderson wrote:
> On 09/21/2016 11:26 AM, Eduardo Habkost wrote:
> > +    /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
> > +    if (!env->cpuid_level) {
> > +        env->cpuid_level = env->cpuid_min_level;
> > +    }
> > +    if (!env->cpuid_xlevel) {
> > +        env->cpuid_xlevel = env->cpuid_min_xlevel;
> > +    }
> > +    if (!env->cpuid_xlevel2) {
> > +        env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
> >      }
> 
> Why are we not bounding them by MIN, if it's really a minimum?

Not sure I understand what you mean. Do you mean silently
changing the value even if it was explicitly set by the user?

In those cases, I don't see what's the benefit of doing something
different from what the user explicitly asked for (considering
that it would require adding more compat code to keep the pc-2.7
behavior).

> 
> > +    /* Enable auto level-increase for CPUID[7].ECX features */
> > +    bool cpuid_auto_level_7_0_ecx;
> > +
> > +    /* Enable auto level-increase for CPUID[6] features */
> > +    bool cpuid_auto_level_6;
> 
> Why two variables?  Seems like only one is needed for backward
> compatibility.

It's true that we don't really need two variables to implement
pc-2.7 compatibility. I just implemented it this way because
having two separate variables looks clearer to me. I wouldn't
know how I would name the variable if it controlled both
CPUID[7].ECX and CPUID[6] at the same time (any suggestions?).

> You're not considering adding a new one when cpuid[8].ebx is
> invented are you?

I'm not. Those were added only to allow us to implement pc-2.7
compatibility.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 5/6] target-i386: Automatically set level/xlevel/xlevel2 when needed

[Richard Henderson]

On 09/21/2016 01:14 PM, Eduardo Habkost wrote:
> On Wed, Sep 21, 2016 at 12:53:08PM -0700, Richard Henderson wrote:
>> On 09/21/2016 11:26 AM, Eduardo Habkost wrote:
>>> +    /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
>>> +    if (!env->cpuid_level) {
>>> +        env->cpuid_level = env->cpuid_min_level;
>>> +    }
>>> +    if (!env->cpuid_xlevel) {
>>> +        env->cpuid_xlevel = env->cpuid_min_xlevel;
>>> +    }
>>> +    if (!env->cpuid_xlevel2) {
>>> +        env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>>>      }
>>
>> Why are we not bounding them by MIN, if it's really a minimum?
>
> Not sure I understand what you mean. Do you mean silently
> changing the value even if it was explicitly set by the user?

You're changing it if the user explicitly set the level to 0, aren't you?

Or is that merely an oversight and you really need the levels defaulted to some 
magic value like -1?

>>> +    /* Enable auto level-increase for CPUID[7].ECX features */
>>> +    bool cpuid_auto_level_7_0_ecx;
>>> +
>>> +    /* Enable auto level-increase for CPUID[6] features */
>>> +    bool cpuid_auto_level_6;
>>
>> Why two variables?  Seems like only one is needed for backward
>> compatibility.
>
> It's true that we don't really need two variables to implement
> pc-2.7 compatibility. I just implemented it this way because
> having two separate variables looks clearer to me. I wouldn't
> know how I would name the variable if it controlled both
> CPUID[7].ECX and CPUID[6] at the same time (any suggestions?).

cpuid_auto_level_compat(_27)?


r~

Re: [Qemu-devel] [PATCH 5/6] target-i386: Automatically set level/xlevel/xlevel2 when needed

[Eduardo Habkost]

On Wed, Sep 21, 2016 at 01:58:55PM -0700, Richard Henderson wrote:
> On 09/21/2016 01:14 PM, Eduardo Habkost wrote:
> > On Wed, Sep 21, 2016 at 12:53:08PM -0700, Richard Henderson wrote:
> > > On 09/21/2016 11:26 AM, Eduardo Habkost wrote:
> > > > +    /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
> > > > +    if (!env->cpuid_level) {
> > > > +        env->cpuid_level = env->cpuid_min_level;
> > > > +    }
> > > > +    if (!env->cpuid_xlevel) {
> > > > +        env->cpuid_xlevel = env->cpuid_min_xlevel;
> > > > +    }
> > > > +    if (!env->cpuid_xlevel2) {
> > > > +        env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
> > > >      }
> > > 
> > > Why are we not bounding them by MIN, if it's really a minimum?
> > 
> > Not sure I understand what you mean. Do you mean silently
> > changing the value even if it was explicitly set by the user?
> 
> You're changing it if the user explicitly set the level to 0, aren't you?
> 
> Or is that merely an oversight and you really need the levels defaulted to
> some magic value like -1?

Oversight: I planned 0 to be the magic value, as I assumed there
was no use case to set it explicitly to 0.

But setting it to 0 is as valid as setting it to other values, so
I will change the default/magic value to UINT32_MAX in the next
version. Thanks for spotting.

> 
> > > > +    /* Enable auto level-increase for CPUID[7].ECX features */
> > > > +    bool cpuid_auto_level_7_0_ecx;
> > > > +
> > > > +    /* Enable auto level-increase for CPUID[6] features */
> > > > +    bool cpuid_auto_level_6;
> > > 
> > > Why two variables?  Seems like only one is needed for backward
> > > compatibility.
> > 
> > It's true that we don't really need two variables to implement
> > pc-2.7 compatibility. I just implemented it this way because
> > having two separate variables looks clearer to me. I wouldn't
> > know how I would name the variable if it controlled both
> > CPUID[7].ECX and CPUID[6] at the same time (any suggestions?).
> 
> cpuid_auto_level_compat(_27)?

I don't like to reference machine type versions in device code.
The name doesn't describe the expected semantics, and it makes
downstream backports harder.

But, I think I found an alternative: I can add a single
force_auto_level_cpuid_7 flag, and make QEMU ignore max_level for
CPUID[7].EBX features in case it is set.

In other words:

    static void x86_cpu_adjust_feat_level(X86CPU *cpu, FeatureWord w, bool ignore_max_level);
    [...]
    x86_cpu_adjust_feat_level(cpu, FEAT_1_EDX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_1_ECX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_6_EAX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_7_0_EBX, env->force_auto_level_cpuid_7);
    x86_cpu_adjust_feat_level(cpu, FEAT_7_0_ECX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_8000_0001_EDX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_8000_0001_ECX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_8000_0007_EDX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_C000_0001_EDX, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_SVM, false);
    x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE, false);

-- 
Eduardo

[Qemu-devel] [PATCH 6/6] target-i386: Enable CPUID[0x8000000A] if SVM is enabled

[Eduardo Habkost]

SVM needs CPUID[0x8000000A] to be available. So if SVM is enabled
in a CPU model or explicitly in the command-line, adjust CPUID
xlevel to expose the CPUID[0x8000000A] leaf.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c             |  6 ++++++
 tests/test-x86-cpuid-compat.c | 11 +++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1ebd7cc..1953dda 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3051,6 +3051,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
     x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
 
+    /* SVM requires CPUID[0x8000000A] */
+    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
+        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, env->cpuid_max_xlevel,
+                             0x8000000A);
+    }
+
     /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
     if (!env->cpuid_level) {
         env->cpuid_level = env->cpuid_min_level;
diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
index 7051825..c287ead 100644
--- a/tests/test-x86-cpuid-compat.c
+++ b/tests/test-x86-cpuid-compat.c
@@ -77,6 +77,7 @@ int main(int argc, char **argv)
     add_cpuid_test("x86/cpuid/486/xlevel", "-cpu 486", "xlevel", 0);
     add_cpuid_test("x86/cpuid/core2duo/xlevel", "-cpu core2duo", "xlevel", 0x80000008);
     add_cpuid_test("x86/cpuid/phenom/xlevel", "-cpu phenom", "xlevel", 0x8000001A);
+    add_cpuid_test("x86/cpuid/athlon/xlevel", "-cpu athlon", "xlevel", 0x80000008);
 
     /* If level is not large enough, it should increase automatically: */
     /* CPUID[6].EAX: */
@@ -97,6 +98,8 @@ int main(int argc, char **argv)
     add_cpuid_test("x86/cpuid/auto-xlevel/486/npt", "-cpu 486,+npt", "xlevel", 0x8000000A);
     /* CPUID[C000_0001].EDX: */
     add_cpuid_test("x86/cpuid/auto-xlevel2/phenom/xstore", "-cpu phenom,+xstore", "xlevel2", 0xC0000001);
+    /* SVM needs CPUID[0x8000000A] */
+    add_cpuid_test("x86/cpuid/auto-xlevel/athlon/svm", "-cpu athlon,+svm", "xlevel", 0x8000000A);
 
 
     /* If level is already large enough, it shouldn't change: */
@@ -106,10 +109,10 @@ int main(int argc, char **argv)
     add_cpuid_test("x86/cpuid/auto-level/486/fixed/2", "-cpu 486,level=2,+arat,+fsgsbase,+avx512vbmi,+xsaveopt", "level", 2);
 
     /* if xlevel is already large enough, it shouldn't change: */
-    add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);
+    add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x8000001A);
     /* If xlevel is explicitly set, it shouldn't change: */
-    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002", "-cpu 486,xlevel=0x80000002,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x80000002);
-    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A", "-cpu 486,xlevel=0x8000001A,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002", "-cpu 486,xlevel=0x80000002,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x80000002);
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A", "-cpu 486,xlevel=0x8000001A,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x8000001A);
 
     /* if xlevel2 is already large enough, it shouldn't change: */
     add_cpuid_test("x86/cpuid/auto-xlevel2/486/fixed", "-cpu 486,xlevel2=0xC0000002,+xstore", "xlevel2", 0xC0000002);
@@ -117,7 +120,7 @@ int main(int argc, char **argv)
     /* Compatibility test for older machine-types that don't auto-increase level/xlevel/xlevel2: */
 
     add_cpuid_test("x86/cpuid/auto-level/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+arat,+avx512vbmi,+xsaveopt", "level", 1);
-    add_cpuid_test("x86/cpuid/auto-xlevel/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0);
+    add_cpuid_test("x86/cpuid/auto-xlevel/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0);
     add_cpuid_test("x86/cpuid/auto-xlevel2/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+xstore", "xlevel2", 0);
 
     return g_test_run();
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/6] target-i386: Increase CPUID level/xlevel/xlevel2 automatically

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1474482404-15678-1-git-send-email-ehabkost@redhat.com
Subject: [Qemu-devel] [PATCH 0/6] target-i386: Increase CPUID level/xlevel/xlevel2 automatically

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1470993574-11906-1-git-send-email-famz@redhat.com -> patchew/1470993574-11906-1-git-send-email-famz@redhat.com
 * [new tag]         patchew/1474482404-15678-1-git-send-email-ehabkost@redhat.com -> patchew/1474482404-15678-1-git-send-email-ehabkost@redhat.com
Switched to a new branch 'test'
cce6853 target-i386: Enable CPUID[0x8000000A] if SVM is enabled
b0097d2 target-i386: Automatically set level/xlevel/xlevel2 when needed
8b47896 tests: Test CPUID level handling for old machines
5fcfd1b tests: Add test code for CPUID level/xlevel handling
32d3954 target-i386: Add a marker to end of the region zeroed on reset
de76d02 target-i386: Remove unused X86CPUDefinition::xlevel2 field

=== OUTPUT BEGIN ===
Checking PATCH 1/6: target-i386: Remove unused X86CPUDefinition::xlevel2 field...
Checking PATCH 2/6: target-i386: Add a marker to end of the region zeroed on reset...
Checking PATCH 3/6: tests: Add test code for CPUID level/xlevel handling...
WARNING: line over 80 characters
#113: FILE: tests/test-x86-cpuid-compat.c:76:
+    add_cpuid_test("x86/cpuid/SandyBridge/level", "-cpu SandyBridge", "level", 0xd);

WARNING: line over 80 characters
#115: FILE: tests/test-x86-cpuid-compat.c:78:
+    add_cpuid_test("x86/cpuid/core2duo/xlevel", "-cpu core2duo", "xlevel", 0x80000008);

WARNING: line over 80 characters
#116: FILE: tests/test-x86-cpuid-compat.c:79:
+    add_cpuid_test("x86/cpuid/phenom/xlevel", "-cpu phenom", "xlevel", 0x8000001A);

ERROR: line over 90 characters
#120: FILE: tests/test-x86-cpuid-compat.c:83:
+    add_cpuid_test("x86/cpuid/auto-level/phenom/fsgsbase", "-cpu phenom,+fsgsbase", "level", 7);

ERROR: line over 90 characters
#123: FILE: tests/test-x86-cpuid-compat.c:86:
+    add_cpuid_test("x86/cpuid/auto-level/SandyBridge/multiple", "-cpu SandyBridge,+arat,+fsgsbase,+avx512vbmi", "level", 0xd);

ERROR: line over 90 characters
#126: FILE: tests/test-x86-cpuid-compat.c:89:
+    add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);

ERROR: line over 90 characters
#129: FILE: tests/test-x86-cpuid-compat.c:92:
+    add_cpuid_test("x86/cpuid/auto-xlevel2/486/fixed", "-cpu 486,xlevel2=0xC0000002,+xstore", "xlevel2", 0xC0000002);

total: 4 errors, 3 warnings, 109 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/6: tests: Test CPUID level handling for old machines...
ERROR: line over 90 characters
#21: FILE: tests/test-x86-cpuid-compat.c:94:
+    /* Compatibility test for older machine-types that don't auto-increase level/xlevel/xlevel2: */

ERROR: line over 90 characters
#23: FILE: tests/test-x86-cpuid-compat.c:96:
+    add_cpuid_test("x86/cpuid/auto-level/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+arat,+avx512vbmi,+xsaveopt", "level", 1);

ERROR: line over 90 characters
#24: FILE: tests/test-x86-cpuid-compat.c:97:
+    add_cpuid_test("x86/cpuid/auto-xlevel/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0);

ERROR: line over 90 characters
#25: FILE: tests/test-x86-cpuid-compat.c:98:
+    add_cpuid_test("x86/cpuid/auto-xlevel2/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+xstore", "xlevel2", 0);

total: 4 errors, 0 warnings, 11 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/6: target-i386: Automatically set level/xlevel/xlevel2 when needed...
WARNING: line over 80 characters
#69: FILE: target-i386/cpu.c:1647:
+        env->cpuid_min_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);

WARNING: line over 80 characters
#70: FILE: target-i386/cpu.c:1648:
+        env->cpuid_min_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);

ERROR: switch and case should be at the same indent
#114: FILE: target-i386/cpu.c:2976:
+    switch (region) {
+        case 0x00000000:
[...]
+        case 0x80000000:
[...]
+        case 0xC0000000:

WARNING: line over 80 characters
#177: FILE: target-i386/cpu.c:3476:
+    DEFINE_PROP_UINT32("max-xlevel2", X86CPU, env.cpuid_max_xlevel2, UINT32_MAX),

WARNING: line over 80 characters
#178: FILE: target-i386/cpu.c:3477:
+    DEFINE_PROP_BOOL("cpuid-auto-level-7-0-ecx", X86CPU, cpuid_auto_level_7_0_ecx, true),

WARNING: line over 80 characters
#225: FILE: tests/test-x86-cpuid-compat.c:83:
+    add_cpuid_test("x86/cpuid/auto-level/phenom/arat", "-cpu 486,+arat", "level", 6);

ERROR: line over 90 characters
#229: FILE: tests/test-x86-cpuid-compat.c:87:
+    add_cpuid_test("x86/cpuid/auto-level/phenom/avx512vbmi", "-cpu phenom,+avx512vbmi", "level", 7);

ERROR: line over 90 characters
#231: FILE: tests/test-x86-cpuid-compat.c:89:
+    add_cpuid_test("x86/cpuid/auto-level/phenom/xsaveopt", "-cpu phenom,+xsaveopt", "level", 0xd);

ERROR: line over 90 characters
#233: FILE: tests/test-x86-cpuid-compat.c:91:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/3dnow", "-cpu 486,+3dnow", "xlevel", 0x80000001);

ERROR: line over 90 characters
#235: FILE: tests/test-x86-cpuid-compat.c:93:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/sse4a", "-cpu 486,+sse4a", "xlevel", 0x80000001);

ERROR: line over 90 characters
#237: FILE: tests/test-x86-cpuid-compat.c:95:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/invtsc", "-cpu 486,+invtsc", "xlevel", 0x80000007);

ERROR: line over 90 characters
#239: FILE: tests/test-x86-cpuid-compat.c:97:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/npt", "-cpu 486,+npt", "xlevel", 0x8000000A);

ERROR: line over 90 characters
#241: FILE: tests/test-x86-cpuid-compat.c:99:
+    add_cpuid_test("x86/cpuid/auto-xlevel2/phenom/xstore", "-cpu phenom,+xstore", "xlevel2", 0xC0000001);

ERROR: line over 90 characters
#247: FILE: tests/test-x86-cpuid-compat.c:105:
+    add_cpuid_test("x86/cpuid/auto-level/486/fixed/0xF", "-cpu 486,level=0xF,+arat,+fsgsbase,+avx512vbmi,+xsaveopt", "level", 0xF);

ERROR: line over 90 characters
#248: FILE: tests/test-x86-cpuid-compat.c:106:
+    add_cpuid_test("x86/cpuid/auto-level/486/fixed/2", "-cpu 486,level=2,+arat,+fsgsbase,+avx512vbmi,+xsaveopt", "level", 2);

ERROR: line over 90 characters
#253: FILE: tests/test-x86-cpuid-compat.c:111:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002", "-cpu 486,xlevel=0x80000002,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x80000002);

ERROR: line over 90 characters
#254: FILE: tests/test-x86-cpuid-compat.c:112:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A", "-cpu 486,xlevel=0x8000001A,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);

total: 12 errors, 5 warnings, 214 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 6/6: target-i386: Enable CPUID[0x8000000A] if SVM is enabled...
WARNING: line over 80 characters
#38: FILE: tests/test-x86-cpuid-compat.c:80:
+    add_cpuid_test("x86/cpuid/athlon/xlevel", "-cpu athlon", "xlevel", 0x80000008);

ERROR: line over 90 characters
#47: FILE: tests/test-x86-cpuid-compat.c:102:
+    add_cpuid_test("x86/cpuid/auto-xlevel/athlon/svm", "-cpu athlon,+svm", "xlevel", 0x8000000A);

ERROR: line over 90 characters
#56: FILE: tests/test-x86-cpuid-compat.c:112:
+    add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x8000001A);

ERROR: line over 90 characters
#60: FILE: tests/test-x86-cpuid-compat.c:114:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002", "-cpu 486,xlevel=0x80000002,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x80000002);

ERROR: line over 90 characters
#61: FILE: tests/test-x86-cpuid-compat.c:115:
+    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A", "-cpu 486,xlevel=0x8000001A,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x8000001A);

ERROR: line over 90 characters
#70: FILE: tests/test-x86-cpuid-compat.c:123:
+    add_cpuid_test("x86/cpuid/auto-xlevel/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0);

total: 5 errors, 1 warnings, 48 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/6] target-i386: Increase CPUID level/xlevel/xlevel2 automatically

[Eduardo Habkost]

On Wed, Sep 21, 2016 at 11:49:04AM -0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 1474482404-15678-1-git-send-email-ehabkost@redhat.com
> Subject: [Qemu-devel] [PATCH 0/6] target-i386: Increase CPUID level/xlevel/xlevel2 automatically

Oops! I have fixed the problems below in my local branch. The only remaining
warnings are:

/tmp/tmp.JqvmKcw0pK/0004-tests-Test-CPUID-level-handling-for-old-machines.patch has no obvious style problems and is ready for submission.
WARNING: line over 80 characters
#188: FILE: target-i386/cpu.c:3479:
+    DEFINE_PROP_UINT32("max-xlevel2", X86CPU, env.cpuid_max_xlevel2, UINT32_MAX),

WARNING: line over 80 characters
#189: FILE: target-i386/cpu.c:3480:
+    DEFINE_PROP_BOOL("cpuid-auto-level-7-0-ecx", X86CPU, cpuid_auto_level_7_0_ecx, true),

total: 0 errors, 2 warnings, 238 lines checked


> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  - [tag update]      patchew/1470993574-11906-1-git-send-email-famz@redhat.com -> patchew/1470993574-11906-1-git-send-email-famz@redhat.com
>  * [new tag]         patchew/1474482404-15678-1-git-send-email-ehabkost@redhat.com -> patchew/1474482404-15678-1-git-send-email-ehabkost@redhat.com
> Switched to a new branch 'test'
> cce6853 target-i386: Enable CPUID[0x8000000A] if SVM is enabled
> b0097d2 target-i386: Automatically set level/xlevel/xlevel2 when needed
> 8b47896 tests: Test CPUID level handling for old machines
> 5fcfd1b tests: Add test code for CPUID level/xlevel handling
> 32d3954 target-i386: Add a marker to end of the region zeroed on reset
> de76d02 target-i386: Remove unused X86CPUDefinition::xlevel2 field
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/6: target-i386: Remove unused X86CPUDefinition::xlevel2 field...
> Checking PATCH 2/6: target-i386: Add a marker to end of the region zeroed on reset...
> Checking PATCH 3/6: tests: Add test code for CPUID level/xlevel handling...
> WARNING: line over 80 characters
> #113: FILE: tests/test-x86-cpuid-compat.c:76:
> +    add_cpuid_test("x86/cpuid/SandyBridge/level", "-cpu SandyBridge", "level", 0xd);
> 
> WARNING: line over 80 characters
> #115: FILE: tests/test-x86-cpuid-compat.c:78:
> +    add_cpuid_test("x86/cpuid/core2duo/xlevel", "-cpu core2duo", "xlevel", 0x80000008);
> 
> WARNING: line over 80 characters
> #116: FILE: tests/test-x86-cpuid-compat.c:79:
> +    add_cpuid_test("x86/cpuid/phenom/xlevel", "-cpu phenom", "xlevel", 0x8000001A);
> 
> ERROR: line over 90 characters
> #120: FILE: tests/test-x86-cpuid-compat.c:83:
> +    add_cpuid_test("x86/cpuid/auto-level/phenom/fsgsbase", "-cpu phenom,+fsgsbase", "level", 7);
> 
> ERROR: line over 90 characters
> #123: FILE: tests/test-x86-cpuid-compat.c:86:
> +    add_cpuid_test("x86/cpuid/auto-level/SandyBridge/multiple", "-cpu SandyBridge,+arat,+fsgsbase,+avx512vbmi", "level", 0xd);
> 
> ERROR: line over 90 characters
> #126: FILE: tests/test-x86-cpuid-compat.c:89:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);
> 
> ERROR: line over 90 characters
> #129: FILE: tests/test-x86-cpuid-compat.c:92:
> +    add_cpuid_test("x86/cpuid/auto-xlevel2/486/fixed", "-cpu 486,xlevel2=0xC0000002,+xstore", "xlevel2", 0xC0000002);
> 
> total: 4 errors, 3 warnings, 109 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 4/6: tests: Test CPUID level handling for old machines...
> ERROR: line over 90 characters
> #21: FILE: tests/test-x86-cpuid-compat.c:94:
> +    /* Compatibility test for older machine-types that don't auto-increase level/xlevel/xlevel2: */
> 
> ERROR: line over 90 characters
> #23: FILE: tests/test-x86-cpuid-compat.c:96:
> +    add_cpuid_test("x86/cpuid/auto-level/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+arat,+avx512vbmi,+xsaveopt", "level", 1);
> 
> ERROR: line over 90 characters
> #24: FILE: tests/test-x86-cpuid-compat.c:97:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0);
> 
> ERROR: line over 90 characters
> #25: FILE: tests/test-x86-cpuid-compat.c:98:
> +    add_cpuid_test("x86/cpuid/auto-xlevel2/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+xstore", "xlevel2", 0);
> 
> total: 4 errors, 0 warnings, 11 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 5/6: target-i386: Automatically set level/xlevel/xlevel2 when needed...
> WARNING: line over 80 characters
> #69: FILE: target-i386/cpu.c:1647:
> +        env->cpuid_min_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
> 
> WARNING: line over 80 characters
> #70: FILE: target-i386/cpu.c:1648:
> +        env->cpuid_min_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> 
> ERROR: switch and case should be at the same indent
> #114: FILE: target-i386/cpu.c:2976:
> +    switch (region) {
> +        case 0x00000000:
> [...]
> +        case 0x80000000:
> [...]
> +        case 0xC0000000:
> 
> WARNING: line over 80 characters
> #177: FILE: target-i386/cpu.c:3476:
> +    DEFINE_PROP_UINT32("max-xlevel2", X86CPU, env.cpuid_max_xlevel2, UINT32_MAX),
> 
> WARNING: line over 80 characters
> #178: FILE: target-i386/cpu.c:3477:
> +    DEFINE_PROP_BOOL("cpuid-auto-level-7-0-ecx", X86CPU, cpuid_auto_level_7_0_ecx, true),
> 
> WARNING: line over 80 characters
> #225: FILE: tests/test-x86-cpuid-compat.c:83:
> +    add_cpuid_test("x86/cpuid/auto-level/phenom/arat", "-cpu 486,+arat", "level", 6);
> 
> ERROR: line over 90 characters
> #229: FILE: tests/test-x86-cpuid-compat.c:87:
> +    add_cpuid_test("x86/cpuid/auto-level/phenom/avx512vbmi", "-cpu phenom,+avx512vbmi", "level", 7);
> 
> ERROR: line over 90 characters
> #231: FILE: tests/test-x86-cpuid-compat.c:89:
> +    add_cpuid_test("x86/cpuid/auto-level/phenom/xsaveopt", "-cpu phenom,+xsaveopt", "level", 0xd);
> 
> ERROR: line over 90 characters
> #233: FILE: tests/test-x86-cpuid-compat.c:91:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/3dnow", "-cpu 486,+3dnow", "xlevel", 0x80000001);
> 
> ERROR: line over 90 characters
> #235: FILE: tests/test-x86-cpuid-compat.c:93:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/sse4a", "-cpu 486,+sse4a", "xlevel", 0x80000001);
> 
> ERROR: line over 90 characters
> #237: FILE: tests/test-x86-cpuid-compat.c:95:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/invtsc", "-cpu 486,+invtsc", "xlevel", 0x80000007);
> 
> ERROR: line over 90 characters
> #239: FILE: tests/test-x86-cpuid-compat.c:97:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/npt", "-cpu 486,+npt", "xlevel", 0x8000000A);
> 
> ERROR: line over 90 characters
> #241: FILE: tests/test-x86-cpuid-compat.c:99:
> +    add_cpuid_test("x86/cpuid/auto-xlevel2/phenom/xstore", "-cpu phenom,+xstore", "xlevel2", 0xC0000001);
> 
> ERROR: line over 90 characters
> #247: FILE: tests/test-x86-cpuid-compat.c:105:
> +    add_cpuid_test("x86/cpuid/auto-level/486/fixed/0xF", "-cpu 486,level=0xF,+arat,+fsgsbase,+avx512vbmi,+xsaveopt", "level", 0xF);
> 
> ERROR: line over 90 characters
> #248: FILE: tests/test-x86-cpuid-compat.c:106:
> +    add_cpuid_test("x86/cpuid/auto-level/486/fixed/2", "-cpu 486,level=2,+arat,+fsgsbase,+avx512vbmi,+xsaveopt", "level", 2);
> 
> ERROR: line over 90 characters
> #253: FILE: tests/test-x86-cpuid-compat.c:111:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002", "-cpu 486,xlevel=0x80000002,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x80000002);
> 
> ERROR: line over 90 characters
> #254: FILE: tests/test-x86-cpuid-compat.c:112:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A", "-cpu 486,xlevel=0x8000001A,+3dnow,+sse4a,+invtsc,+npt", "xlevel", 0x8000001A);
> 
> total: 12 errors, 5 warnings, 214 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 6/6: target-i386: Enable CPUID[0x8000000A] if SVM is enabled...
> WARNING: line over 80 characters
> #38: FILE: tests/test-x86-cpuid-compat.c:80:
> +    add_cpuid_test("x86/cpuid/athlon/xlevel", "-cpu athlon", "xlevel", 0x80000008);
> 
> ERROR: line over 90 characters
> #47: FILE: tests/test-x86-cpuid-compat.c:102:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/athlon/svm", "-cpu athlon,+svm", "xlevel", 0x8000000A);
> 
> ERROR: line over 90 characters
> #56: FILE: tests/test-x86-cpuid-compat.c:112:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/phenom/3dnow", "-cpu phenom,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x8000001A);
> 
> ERROR: line over 90 characters
> #60: FILE: tests/test-x86-cpuid-compat.c:114:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/80000002", "-cpu 486,xlevel=0x80000002,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x80000002);
> 
> ERROR: line over 90 characters
> #61: FILE: tests/test-x86-cpuid-compat.c:115:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/486/fixed/8000001A", "-cpu 486,xlevel=0x8000001A,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0x8000001A);
> 
> ERROR: line over 90 characters
> #70: FILE: tests/test-x86-cpuid-compat.c:123:
> +    add_cpuid_test("x86/cpuid/auto-xlevel/pc-2.7", "-machine pc-i440fx-2.7 -cpu 486,+3dnow,+sse4a,+invtsc,+npt,+svm", "xlevel", 0);
> 
> total: 5 errors, 1 warnings, 48 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

-- 
Eduardo