> -----Original Message----- > From: Moger, Babu > Sent: Thursday, June 14, 2018 6:09 PM > To: Moger, Babu <Babu.Moger@xxxxxxx>; Eduardo Habkost > <ehabkost@xxxxxxxxxx> > Cc: mst@xxxxxxxxxx; marcel.apfelbaum@xxxxxxxxx; pbonzini@xxxxxxxxxx; > rth@xxxxxxxxxxx; mtosatti@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx; kash@xxxxxxxxxxxxxx; geoff@xxxxxxxxxxxxxxx > Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be > supported > > > > -----Original Message----- > > From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] > > On Behalf Of Moger, Babu > > Sent: Thursday, June 14, 2018 5:19 PM > > To: Eduardo Habkost <ehabkost@xxxxxxxxxx> > > Cc: mst@xxxxxxxxxx; marcel.apfelbaum@xxxxxxxxx; > pbonzini@xxxxxxxxxx; > > rth@xxxxxxxxxxx; mtosatti@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; > > kvm@xxxxxxxxxxxxxxx; kash@xxxxxxxxxxxxxx; geoff@xxxxxxxxxxxxxxx > > Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be > > supported > > > > > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@xxxxxxxxxx] > > > Sent: Thursday, June 14, 2018 2:13 PM > > > To: Moger, Babu <Babu.Moger@xxxxxxx> > > > Cc: mst@xxxxxxxxxx; marcel.apfelbaum@xxxxxxxxx; > > pbonzini@xxxxxxxxxx; > > > rth@xxxxxxxxxxx; mtosatti@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; > > > kvm@xxxxxxxxxxxxxxx; kash@xxxxxxxxxxxxxx; geoff@xxxxxxxxxxxxxxx > > > 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 <babu.moger@xxxxxxx> > > > > --- > > > > 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
