Re: [PATCH] ppc/pnv: Drop "num-chips" machine property

[Cédric Le Goater]

On 1/6/20 11:29 AM, Greg Kurz wrote:
> The number of CPU chips of the powernv machine is configurable through a
> "num-chips" property. This doesn't fit well with the CPU topology, eg.
> some configurations can come up with more CPUs than the maximum of CPUs
> set in the toplogy. This causes assertion to be hit with mttcg:
> 
>    -machine powernv,num-chips=2 -smp cores=2 -accel tcg,thread=multi
> 
> ERROR:
> tcg/tcg.c:789:tcg_register_thread: assertion failed: (n < ms->smp.max_cpus)
> Aborted (core dumped)
> 
> Mttcg 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'.
> 
> It thus seems that "num-chips" doesn't bring anything but forcing the user
> to specify the requested number of chips on the command line twice. Simplify
> the command line by computing the number of chips based on the CPU topology
> exclusively. The powernv machine isn't a production thing ; it is mostly
> used by developpers to prepare the bringup of real HW. Because of this and
> for simplicity, this deliberately ignores the official deprecation process
> and dumps "num-chips" right away : '-smp sockets=N' is now the only way to
> control the number of CPU chips.
> 
> This is done at machine init because smp_parse() is called after instance
> init.
> 
> Signed-off-by: Greg Kurz <address@hidden>

Reviewed-by: Cédric Le Goater <address@hidden>

Thanks,

C.

> ---
>  hw/ppc/pnv.c |   62 
> +++++++++++-----------------------------------------------
>  1 file changed, 12 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index f77e7ca84ede..b225ffbb2c41 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -768,6 +768,18 @@ static void pnv_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    pnv->num_chips =
> +        machine->smp.max_cpus / (machine->smp.cores * machine->smp.threads);
> +    /*
> +     * TODO: should we decide on how many chips we can create based
> +     * on #cores and Venice vs. Murano vs. Naples chip type etc...,
> +     */
> +    if (!is_power_of_2(pnv->num_chips) || pnv->num_chips > 4) {
> +        error_report("invalid number of chips: '%d'", pnv->num_chips);
> +        error_printf("Try '-smp sockets=N'. Valid values are : 1, 2 or 
> 4.\n");
> +        exit(1);
> +    }
> +
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
> @@ -1696,53 +1708,6 @@ PnvChip *pnv_get_chip(uint32_t chip_id)
>      return NULL;
>  }
>  
> -static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    visit_type_uint32(v, name, &PNV_MACHINE(obj)->num_chips, errp);
> -}
> -
> -static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    PnvMachineState *pnv = PNV_MACHINE(obj);
> -    uint32_t num_chips;
> -    Error *local_err = NULL;
> -
> -    visit_type_uint32(v, name, &num_chips, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
> -    /*
> -     * TODO: should we decide on how many chips we can create based
> -     * on #cores and Venice vs. Murano vs. Naples chip type etc...,
> -     */
> -    if (!is_power_of_2(num_chips) || num_chips > 4) {
> -        error_setg(errp, "invalid number of chips: '%d'", num_chips);
> -        return;
> -    }
> -
> -    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",
> -                              pnv_get_num_chips, pnv_set_num_chips,
> -                              NULL, NULL, NULL);
> -    object_class_property_set_description(oc, "num-chips",
> -                              "Specifies the number of processor chips",
> -                              NULL);
> -}
> -
>  static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1812,8 +1777,6 @@ static void pnv_machine_class_init(ObjectClass *oc, 
> void *data)
>       */
>      mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
>      ispc->print_info = pnv_pic_print_info;
> -
> -    pnv_machine_class_props_init(oc);
>  }
>  
>  #define DEFINE_PNV8_CHIP_TYPE(type, class_initfn) \
> @@ -1866,7 +1829,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[]) {
>