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

[Moger, Babu]

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

> 
> --
> Eduardo