Multi TCG mandates the CPU topology to be dimensioned to the actual
number of CPUs, depending on the number of chips the user asked for.
That is, '-machine num-chips=N' should always have a '-smp' companion
with a topology that meats the resulting number of CPUs, typically
'-smp sockets=N'.

Simplify the command line for these setups by computing the default
number of chips based on the CPU topology, ie. no need to explicitely
set "num-chips" anymore. This must be done at machine init because
smp_parse() is called after instance init.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index f8cf2b6d760f..9b777b7084a0 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -768,6 +768,19 @@ static void pnv_init(MachineState *machine)
         exit(1);
     }
 
+    if (!pnv->num_chips) {
+        uint32_t num_chips =
+            machine->smp.max_cpus / (machine->smp.cores * machine->smp.threads);
+        Error *local_err = NULL;
+
+        object_property_set_uint(OBJECT(pnv), num_chips, "num-chips",
+                                 &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            exit(1);
+        }
+    }
+
     pnv->chips = g_new0(PnvChip *, pnv->num_chips);
     for (i = 0; i < pnv->num_chips; i++) {
         char chip_name[32];
@@ -1722,6 +1735,9 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
      */
     if (!is_power_of_2(num_chips) || num_chips > 4) {
         error_setg(errp, "invalid number of chips: '%d'", num_chips);
+        error_append_hint(errp,
+                          "Set 'num-chips' implicitely with '-smp sockets=N'. "
+                          "Valid values are : 1, 2 or 4.\n");
         return;
     }
 
@@ -1735,12 +1751,6 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
     pnv->num_chips = num_chips;
 }
 
-static void pnv_machine_instance_init(Object *obj)
-{
-    PnvMachineState *pnv = PNV_MACHINE(obj);
-    pnv->num_chips = 1;
-}
-
 static void pnv_machine_class_props_init(ObjectClass *oc)
 {
     object_class_property_add(oc, "num-chips", "uint32",
@@ -1874,7 +1884,6 @@ static const TypeInfo types[] = {
         .parent        = TYPE_MACHINE,
         .abstract       = true,
         .instance_size = sizeof(PnvMachineState),
-        .instance_init = pnv_machine_instance_init,
         .class_init    = pnv_machine_class_init,
         .class_size    = sizeof(PnvMachineClass),
         .interfaces = (InterfaceInfo[]) {

On Fri, Dec 20, 2019 at 05:51:48PM +0100, Greg Kurz wrote:
> Multi TCG mandates the CPU topology to be dimensioned to the actual
> number of CPUs, depending on the number of chips the user asked for.
> That is, '-machine num-chips=N' should always have a '-smp' companion
> with a topology that meats the resulting number of CPUs, typically
> '-smp sockets=N'.
> 
> Simplify the command line for these setups by computing the default
> number of chips based on the CPU topology, ie. no need to explicitely
> set "num-chips" anymore. This must be done at machine init because
> smp_parse() is called after instance init.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Is there actually any reason to retain num-chips at all?  Or could we
just set the number of chips equal to the number of sockets, which
seems to make sense to me.

> ---
>  hw/ppc/pnv.c |   23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index f8cf2b6d760f..9b777b7084a0 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -768,6 +768,19 @@ static void pnv_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    if (!pnv->num_chips) {
> +        uint32_t num_chips =
> +            machine->smp.max_cpus / (machine->smp.cores * machine->smp.threads);
> +        Error *local_err = NULL;
> +
> +        object_property_set_uint(OBJECT(pnv), num_chips, "num-chips",
> +                                 &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            exit(1);
> +        }
> +    }
> +
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
> @@ -1722,6 +1735,9 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
>       */
>      if (!is_power_of_2(num_chips) || num_chips > 4) {
>          error_setg(errp, "invalid number of chips: '%d'", num_chips);
> +        error_append_hint(errp,
> +                          "Set 'num-chips' implicitely with '-smp sockets=N'. "
> +                          "Valid values are : 1, 2 or 4.\n");
>          return;
>      }
>  
> @@ -1735,12 +1751,6 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
>      pnv->num_chips = num_chips;
>  }
>  
> -static void pnv_machine_instance_init(Object *obj)
> -{
> -    PnvMachineState *pnv = PNV_MACHINE(obj);
> -    pnv->num_chips = 1;
> -}
> -
>  static void pnv_machine_class_props_init(ObjectClass *oc)
>  {
>      object_class_property_add(oc, "num-chips", "uint32",
> @@ -1874,7 +1884,6 @@ static const TypeInfo types[] = {
>          .parent        = TYPE_MACHINE,
>          .abstract       = true,
>          .instance_size = sizeof(PnvMachineState),
> -        .instance_init = pnv_machine_instance_init,
>          .class_init    = pnv_machine_class_init,
>          .class_size    = sizeof(PnvMachineClass),
>          .interfaces = (InterfaceInfo[]) {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Sat, 21 Dec 2019 11:39:06 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Dec 20, 2019 at 05:51:48PM +0100, Greg Kurz wrote:
> > Multi TCG mandates the CPU topology to be dimensioned to the actual
> > number of CPUs, depending on the number of chips the user asked for.
> > That is, '-machine num-chips=N' should always have a '-smp' companion
> > with a topology that meats the resulting number of CPUs, typically
> > '-smp sockets=N'.
> > 
> > Simplify the command line for these setups by computing the default
> > number of chips based on the CPU topology, ie. no need to explicitely
> > set "num-chips" anymore. This must be done at machine init because
> > smp_parse() is called after instance init.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Is there actually any reason to retain num-chips at all?  Or could we
> just set the number of chips equal to the number of sockets, which
> seems to make sense to me.
> 

I don't quite know why "num-chips" was introduced in the first place... so
yes, if it turns out it isn't needed, I'll gladly drop the property.

> > ---
> >  hw/ppc/pnv.c |   23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index f8cf2b6d760f..9b777b7084a0 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -768,6 +768,19 @@ static void pnv_init(MachineState *machine)
> >          exit(1);
> >      }
> >  
> > +    if (!pnv->num_chips) {
> > +        uint32_t num_chips =
> > +            machine->smp.max_cpus / (machine->smp.cores * machine->smp.threads);
> > +        Error *local_err = NULL;
> > +
> > +        object_property_set_uint(OBJECT(pnv), num_chips, "num-chips",
> > +                                 &local_err);
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            exit(1);
> > +        }
> > +    }
> > +
> >      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
> >      for (i = 0; i < pnv->num_chips; i++) {
> >          char chip_name[32];
> > @@ -1722,6 +1735,9 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
> >       */
> >      if (!is_power_of_2(num_chips) || num_chips > 4) {
> >          error_setg(errp, "invalid number of chips: '%d'", num_chips);
> > +        error_append_hint(errp,
> > +                          "Set 'num-chips' implicitely with '-smp sockets=N'. "
> > +                          "Valid values are : 1, 2 or 4.\n");
> >          return;
> >      }
> >  
> > @@ -1735,12 +1751,6 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
> >      pnv->num_chips = num_chips;
> >  }
> >  
> > -static void pnv_machine_instance_init(Object *obj)
> > -{
> > -    PnvMachineState *pnv = PNV_MACHINE(obj);
> > -    pnv->num_chips = 1;
> > -}
> > -
> >  static void pnv_machine_class_props_init(ObjectClass *oc)
> >  {
> >      object_class_property_add(oc, "num-chips", "uint32",
> > @@ -1874,7 +1884,6 @@ static const TypeInfo types[] = {
> >          .parent        = TYPE_MACHINE,
> >          .abstract       = true,
> >          .instance_size = sizeof(PnvMachineState),
> > -        .instance_init = pnv_machine_instance_init,
> >          .class_init    = pnv_machine_class_init,
> >          .class_size    = sizeof(PnvMachineClass),
> >          .interfaces = (InterfaceInfo[]) {
> > 
> 

On 12/21/19 11:28 AM, Greg Kurz wrote:
> On Sat, 21 Dec 2019 11:39:06 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Fri, Dec 20, 2019 at 05:51:48PM +0100, Greg Kurz wrote:
>>> Multi TCG mandates the CPU topology to be dimensioned to the actual
>>> number of CPUs, depending on the number of chips the user asked for.
>>> That is, '-machine num-chips=N' should always have a '-smp' companion
>>> with a topology that meats the resulting number of CPUs, typically
>>> '-smp sockets=N'.
>>>
>>> Simplify the command line for these setups by computing the default
>>> number of chips based on the CPU topology, ie. no need to explicitely
>>> set "num-chips" anymore. This must be done at machine init because
>>> smp_parse() is called after instance init.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>
>> Is there actually any reason to retain num-chips at all?  Or could we
>> just set the number of chips equal to the number of sockets, which
>> seems to make sense to me.
>>
> 
> I don't quite know why "num-chips" was introduced in the first place... so
> yes, if it turns out it isn't needed, I'll gladly drop the property.

I concur. We have some freedom on the PowerNV machine options. 
Let's replace "num-chips" with "sockets".

Thanks,

C.