diff mbox

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

Message ID 1474482404-15678-6-git-send-email-ehabkost@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eduardo Habkost Sept. 21, 2016, 6:26 p.m. UTC 
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(-)

Comments

Richard Henderson Sept. 21, 2016, 7:53 p.m. UTC | #1 
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~
Eduardo Habkost Sept. 21, 2016, 8:14 p.m. UTC | #2 
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.
Richard Henderson Sept. 21, 2016, 8:58 p.m. UTC | #3 
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~
Eduardo Habkost Sept. 22, 2016, 1:32 p.m. UTC | #4 
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);
diff mbox

Patch

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);