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~