Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Moger, Babu]

> -----Original Message-----
> From: address@hidden [mailto:address@hidden
> On Behalf Of Eduardo Habkost
> Sent: Monday, June 11, 2018 4:10 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden; Jiri
> Denemark <address@hidden>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Mon, Jun 11, 2018 at 05:50:30PM -0300, Eduardo Habkost wrote:
> [...]
> > > +        /* TOPOEXT feature requires 0x8000001E */
> > > +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)
> {
> > > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
> 0x8000001E);
> > > +        }
> >
> > I suggest moving this hunk to a separate patch.  I'm not 100%
> > sure yet if this will require compat_props code to disable
> > auto-xlevel-increase on older machine-types.
> 
> The problem here is that:
>   $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
> currently results in xlevel=0x8000001A, since QEMU 1.3.
> 
> (The same applies to all machine-types between 1.3 and 2.12)
> 
> I was hoping that we could declare topoext as non-migration-safe,
> but I believe libvirt will already include "topoext" when using
> "host-model" if the host CPU supports TOPOEXT.  Jiri, can you
> confirm that?
> 
> We can address that with a "x-topoext-auto-xlevel" property, set
> to true on all CPU models by default, and disabled by
> PC_COMPAT_2_12.
> 
> The code would become:
> 
>     if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT) {
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>     }
> 
> Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
> never increase xlevel automatically (on any machine-type), and
> change the code above to:
> 
>     if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT) {
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>     }

I was going to do this.  But there is one problem.  We don't set the 
CPUID_EXT3_TOPOEXT in CPU model table. So this won't work.
One more thing I noticed that feature setting should happen much before 
x86_cpu_realizefn.

Couple of options. 
First option.
1. Set both feature and xlevel here(in x86_cpu_expand_features).
 if (cpu->x_auto_topoext {
        env->features[FEAT_8000_0001_ECX]  |= CPUID_EXT3_TOPOEXT;
        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
 }
2. And remove feature setting in x86_cpu_realizefn.

Or 

Second option
1.Set the feature bit in CPU model table.
2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext
3. And remove feature setting in x86_cpu_realizefn.
 
I  prefer the second option. 

> 
> --
> Eduardo