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