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