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

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Jun 12, 2018 at 04:29:25PM +0000, Moger, Babu 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.

Won't this work if the auto_topoext handling is done before
x86_cpu_expand_features() is called?


> One more thing I noticed that feature setting should happen
> much before x86_cpu_realizefn.

Why?

> 
> 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.

This would make TOPOEXT be included in 'query-cpu-model-expansion
model=EPYC', which would be incorrect because TOPOEXT won't
always be enabled when using EPYC.


> 
> 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. 

Same here: TOPOEXT would be included in
'query-cpu-model-expansion model=EPYC', and this would be
incorrect.


I'm starting to think that enabling TOPOEXT automatically is
adding too much complexity and compatibility problems, and it's
better to leave this task to management software.

The main problem here is:

This works today with QEMU 2.12 + Linux <= 4.15:
  $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2"
and must keep working with QEMU 3.0 and Linux <= 4.15.

In addition to that, the results for:
  $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
must be deterministic and expose exactly the same CPUID data even
if host hardware or software changes, as long as the QEMU
command-line is the same.

Do you see a way to fulfill those two constraints while making
"-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?

-- 
Eduardo