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

[Moger, Babu]

> -----Original Message-----
> From: Moger, Babu
> Sent: Thursday, June 14, 2018 6:09 PM
> To: Moger, Babu <address@hidden>; 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: 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.
> 

I am working on changes to accommodate all the nr_cores and threads. 
Need to adjust the node_id in CPUID 8000001E to accommodate more nodes.
Will send updates later today.

> > >
> > > --
> > > Eduardo