Re: [Qemu-devel] [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be supported

[Moger, Babu]

> -----Original Message-----
> From: address@hidden [mailto:address@hidden
> On Behalf Of Moger, Babu
> Sent: Thursday, June 14, 2018 5:19 PM
> To: Eduardo Habkost <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> supported
> 
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:address@hidden
> > Sent: Thursday, June 14, 2018 2:13 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
> > Subject: Re: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> > supported
> >
> > On Wed, Jun 13, 2018 at 09:18:26PM -0400, Babu Moger wrote:
> > > Disable the TOPOEXT feature if it cannot be supported.
> > > We cannot support this feature with more than 2 nr_threads
> > > or more than 32 cores in a socket.
> > >
> > > Signed-off-by: Babu Moger <address@hidden>
> > > ---
> > >  target/i386/cpu.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 2eb26da..637d8eb 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -4765,7 +4765,7 @@ static void x86_cpu_realizefn(DeviceState *dev,
> > Error **errp)
> > >      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > >      CPUX86State *env = &cpu->env;
> > >      Error *local_err = NULL;
> > > -    static bool ht_warned;
> > > +    static bool ht_warned, topo_warned;
> > >
> > >      if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
> > >          char *name = x86_cpu_class_get_model_name(xcc);
> > > @@ -4779,6 +4779,21 @@ static void x86_cpu_realizefn(DeviceState
> *dev,
> > Error **errp)
> > >          return;
> > >      }
> > >
> > > +    /* Disable TOPOEXT if topology cannot be supported */
> > > +    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> > > +        if (!topology_supports_topoext(MAX_CORES_IN_NODE *
> > MAX_NODES_PER_SOCKET,
> > > +                                      2)) {
> >
> > I understand you stopped using cpu->nr_cores/cpu->nr_threads
> > because it was not filled yet.
> >
> > But why exactly do you need to do this before calling
> > x86_cpu_expand_features()?
> 
> We extend the xlevel in x86_cpu_expand_features based on the TOPOEXT
> feature.
> So, I thought it would be right to do that way.
> >
> > If you really need nr_cores and nr_threads to be available
> > earlier, we could simply move their initialization to
> > cpu_exec_initfn() instead of the solution you implemented in
> > patch 4/6.
> >
> > > +            env->features[FEAT_8000_0001_ECX] &=
> !CPUID_EXT3_TOPOEXT;
> >
> > !CPUID_EXT3_TOPOEXT is 0, this will clear all bits in
> > env->features[FEAT_8000_0001_ECX].  Did you mean
> > ~CPUID_EXT3_TOPOEXT?
> 
> Yes. That is correct.  Sorry.. I missed it.
> >
> >
> > > +            if (!topo_warned) {
> > > +                error_report("TOPOEXT feature cannot be supported with
> more"
> > > +                             " than %d cores or more than 2 threads per 
> > > socket."
> > > +                             " Disabling the feature.",
> > > +                             (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> > > +                topo_warned = true;
> >
> > This will print a warning for "-cpu EPYC -smp 64,cores=64".
> > We shouldn't.

If we support all the values, we may not need this. 
> >
> > I'm starting to believe we shouldn't add TOPOEXT to EPYC unless
> > we're ready to make the TOPOEXT CPUID leaves work for all valid
> > -smp configurations.  If the feature will work only on a few
> > specific cases, the feature should be enabled explicitly using
> > "-cpu ...,+topoext".
> >
> > Is it really impossible to make CPUID return reasonable topology
> > data for larger nr_cores and nr_threads values?  It would make
> > everything much simpler.
> 
> I am starting to think about this.  We tried to limit the configuration based 
> on
> the actual hardware configuration.
> If we leave that decision to the user then we might allow the config
> whatever the user wants.
> I need to make some changes in for topology(0x80000001e) information to
> make this work.
> 

One  more thought, we can allow all the configurations. If user creates 
supported configuration, it will work perfectly fine.
If user creates unsupported configuration(like more threads, more cores etc), 
we still create the topology, but it will not be ideal topology.
Reason for this is, I don't want to mess up the good configuration to support 
invalid config. That way we don't have to change anything in topology code now.
 
> >
> > --
> > Eduardo