[2/2] ppc/pnv: Use the CPU topology to compute the default number of chips

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(-)

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[]) {
>

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.