diff mbox series

[v14,5/6] i386: Disable TOPOEXT feature if it cannot be supported

Message ID 1528939107-17193-6-git-send-email-babu.moger@amd.com
State New
Headers show
Series i386: Enable TOPOEXT to support hyperthreading on AMD CPU | expand

Commit Message

Moger, Babu June 14, 2018, 1:18 a.m. UTC 
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(-)

Comments

Eduardo Habkost June 14, 2018, 7:13 p.m. UTC | #1 
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.
Moger, Babu June 14, 2018, 10:18 p.m. UTC | #2 
> -----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
Moger, Babu June 14, 2018, 11:09 p.m. UTC | #3 
> -----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
Moger, Babu June 15, 2018, 2:09 p.m. UTC | #4 
> -----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 mbox series

Patch

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;