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
