| Message ID | 1528939107-17193-6-git-send-email-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@amd.com> > --- > 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()? 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? > + 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.
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Thursday, June 14, 2018 2:13 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > 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@amd.com> > > --- > > 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
> -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > On Behalf Of Moger, Babu > Sent: Thursday, June 14, 2018 5:19 PM > To: Eduardo Habkost <ehabkost@redhat.com> > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be > supported > > > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Thursday, June 14, 2018 2:13 PM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > pbonzini@redhat.com; > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > 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@amd.com> > > > --- > > > 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
> -----Original Message----- > From: Moger, Babu > Sent: Thursday, June 14, 2018 6:09 PM > To: Moger, Babu <Babu.Moger@amd.com>; Eduardo Habkost > <ehabkost@redhat.com> > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be > supported > > > > -----Original Message----- > > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > > On Behalf Of Moger, Babu > > Sent: Thursday, June 14, 2018 5:19 PM > > To: Eduardo Habkost <ehabkost@redhat.com> > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > pbonzini@redhat.com; > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be > > supported > > > > > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Thursday, June 14, 2018 2:13 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > > pbonzini@redhat.com; > > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > > 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@amd.com> > > > > --- > > > > 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
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)) { + env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT; + 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; + } + } + } + x86_cpu_expand_features(cpu, &local_err); if (local_err) { goto out;
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@amd.com> --- target/i386/cpu.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)