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

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Tuesday, June 12, 2018 12:40 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 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?

Yes. That works. We can go with this approach.

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

I was wrong here.  It should be done before x86_cpu_expand_features.

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

Now(setting feature before x86_cpu_expand_features), enabling TOPOEXT appears 
to work fine.

> --
> Eduardo