diff mbox

[v3,2/7] ppc: move smp_threads sanity checks to spapr

Message ID 146780720136.26232.7902445403479806617.stgit@bahia.lab.toulouse-stg.fr.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Greg Kurz July 6, 2016, 12:13 p.m. UTC
Only POWER5 and newer PowerPC cpus from IBM have SMT capabilities.
Since they are only supported by pseries, let's move the checks to
ppc_spapr_init().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c              |   12 ++++++++++++
 target-ppc/translate_init.c |   14 --------------
 2 files changed, 12 insertions(+), 14 deletions(-)

Comments

David Gibson July 7, 2016, 1:12 a.m. UTC | #1
On Wed, Jul 06, 2016 at 02:13:30PM +0200, Greg Kurz wrote:
> Only POWER5 and newer PowerPC cpus from IBM have SMT capabilities.
> Since they are only supported by pseries, let's move the checks to
> ppc_spapr_init().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I'm not comfortable with this.  SMT may only be used for pseries at
the moment, but the SMT possibilities are absolutely a constraint of
the CPU itself, not the machine type.  It seems more logical to me to
check this in the CPU code.

> ---
>  hw/ppc/spapr.c              |   12 ++++++++++++
>  target-ppc/translate_init.c |   14 --------------
>  2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d134eb2f338e..09600fee19b2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1739,6 +1739,18 @@ static void ppc_spapr_init(MachineState *machine)
>          }
>      }
>  
> +    if (smp_threads > smt) {
> +        error_report("Cannot support more than %d threads on PPC with %s",
> +                     smt, kvm_enabled() ? "KVM" : "TCG");
> +        exit(1);
> +    }
> +    if (!is_power_of_2(smp_threads)) {
> +        error_report("Cannot support %d threads on PPC with %s, "
> +                     "threads count must be a power of 2.",
> +                     smp_threads, kvm_enabled() ? "KVM" : "TCG");
> +        exit(1);
> +    }
> +
>      msi_nonbroken = true;
>  
>      QLIST_INIT(&spapr->phbs);
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 31120a5aaf33..775df72cf6c2 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9531,20 +9531,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>      int max_smt = kvmppc_smt_threads();
>  #endif
>  
> -#if !defined(CONFIG_USER_ONLY)
> -    if (smp_threads > max_smt) {
> -        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
> -                   max_smt, kvm_enabled() ? "KVM" : "TCG");
> -        return;
> -    }
> -    if (!is_power_of_2(smp_threads)) {
> -        error_setg(errp, "Cannot support %d threads on PPC with %s, "
> -                   "threads count must be a power of 2.",
> -                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
> -        return;
> -    }
> -#endif
> -
>      cpu_exec_init(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>
Benjamin Herrenschmidt July 7, 2016, 1:57 a.m. UTC | #2
On Thu, 2016-07-07 at 11:12 +1000, David Gibson wrote:
> I'm not comfortable with this.  SMT may only be used for pseries at
> the moment, but the SMT possibilities are absolutely a constraint of
> the CPU itself, not the machine type.  It seems more logical to me to
> check this in the CPU code.

Also powernv uses them too no ?

Ben.
David Gibson July 7, 2016, 2:05 a.m. UTC | #3
On Thu, Jul 07, 2016 at 11:57:32AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2016-07-07 at 11:12 +1000, David Gibson wrote:
> > I'm not comfortable with this.  SMT may only be used for pseries at
> > the moment, but the SMT possibilities are absolutely a constraint of
> > the CPU itself, not the machine type.  It seems more logical to me to
> > check this in the CPU code.
> 
> Also powernv uses them too no ?

Well, powernv isn't in yet, but yes it will.
Greg Kurz July 7, 2016, 6:49 a.m. UTC | #4
On Thu, 7 Jul 2016 11:12:42 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 06, 2016 at 02:13:30PM +0200, Greg Kurz wrote:
> > Only POWER5 and newer PowerPC cpus from IBM have SMT capabilities.
> > Since they are only supported by pseries, let's move the checks to
> > ppc_spapr_init().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> I'm not comfortable with this.  SMT may only be used for pseries at
> the moment, but the SMT possibilities are absolutely a constraint of
> the CPU itself, not the machine type.  It seems more logical to me to
> check this in the CPU code.
> 

Yes, you're right. Ans since this isn't a prerequisite to compute cpu_dt_id,
it can stay in ppc_cpu_realizefn().

I'll drop this patch in v4.

> > ---
> >  hw/ppc/spapr.c              |   12 ++++++++++++
> >  target-ppc/translate_init.c |   14 --------------
> >  2 files changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d134eb2f338e..09600fee19b2 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1739,6 +1739,18 @@ static void ppc_spapr_init(MachineState *machine)
> >          }
> >      }
> >  
> > +    if (smp_threads > smt) {
> > +        error_report("Cannot support more than %d threads on PPC with %s",
> > +                     smt, kvm_enabled() ? "KVM" : "TCG");
> > +        exit(1);
> > +    }
> > +    if (!is_power_of_2(smp_threads)) {
> > +        error_report("Cannot support %d threads on PPC with %s, "
> > +                     "threads count must be a power of 2.",
> > +                     smp_threads, kvm_enabled() ? "KVM" : "TCG");
> > +        exit(1);
> > +    }
> > +
> >      msi_nonbroken = true;
> >  
> >      QLIST_INIT(&spapr->phbs);
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index 31120a5aaf33..775df72cf6c2 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -9531,20 +9531,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
> >      int max_smt = kvmppc_smt_threads();
> >  #endif
> >  
> > -#if !defined(CONFIG_USER_ONLY)
> > -    if (smp_threads > max_smt) {
> > -        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
> > -                   max_smt, kvm_enabled() ? "KVM" : "TCG");
> > -        return;
> > -    }
> > -    if (!is_power_of_2(smp_threads)) {
> > -        error_setg(errp, "Cannot support %d threads on PPC with %s, "
> > -                   "threads count must be a power of 2.",
> > -                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
> > -        return;
> > -    }
> > -#endif
> > -
> >      cpu_exec_init(cs, &local_err);
> >      if (local_err != NULL) {
> >          error_propagate(errp, local_err);
> >   
>
Greg Kurz July 7, 2016, 7:15 a.m. UTC | #5
On Thu, 07 Jul 2016 11:57:32 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2016-07-07 at 11:12 +1000, David Gibson wrote:
> > I'm not comfortable with this.  SMT may only be used for pseries at
> > the moment, but the SMT possibilities are absolutely a constraint of
> > the CPU itself, not the machine type.  It seems more logical to me to
> > check this in the CPU code.  
> 
> Also powernv uses them too no ?
> 
> Ben.
> 

Hi Ben !

I've started to play with Cedric's tree on github. I'll work on adapting
powernv to this series.

Cheers.

--
Greg
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d134eb2f338e..09600fee19b2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1739,6 +1739,18 @@  static void ppc_spapr_init(MachineState *machine)
         }
     }
 
+    if (smp_threads > smt) {
+        error_report("Cannot support more than %d threads on PPC with %s",
+                     smt, kvm_enabled() ? "KVM" : "TCG");
+        exit(1);
+    }
+    if (!is_power_of_2(smp_threads)) {
+        error_report("Cannot support %d threads on PPC with %s, "
+                     "threads count must be a power of 2.",
+                     smp_threads, kvm_enabled() ? "KVM" : "TCG");
+        exit(1);
+    }
+
     msi_nonbroken = true;
 
     QLIST_INIT(&spapr->phbs);
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 31120a5aaf33..775df72cf6c2 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9531,20 +9531,6 @@  static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
     int max_smt = kvmppc_smt_threads();
 #endif
 
-#if !defined(CONFIG_USER_ONLY)
-    if (smp_threads > max_smt) {
-        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
-                   max_smt, kvm_enabled() ? "KVM" : "TCG");
-        return;
-    }
-    if (!is_power_of_2(smp_threads)) {
-        error_setg(errp, "Cannot support %d threads on PPC with %s, "
-                   "threads count must be a power of 2.",
-                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
-        return;
-    }
-#endif
-
     cpu_exec_init(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);