Re: [Qemu-devel] [PATCH] ppc/pnv: fix cores per chip for multiple cpus

[Cédric Le Goater]

On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> writes:
> 
>>>>>
>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>>>>> this:
>>>>>
>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", 
>>>>> &error_fatal);
>>>>>
>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>>>>> was 1), which is wrong as per the command-line that was provided.
>>>>
>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>>>> thread.  If you can't supply 4 sockets you should error, but you
>>>> shouldn't go and change the number of cores per socket.
>>>
>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
>>> case. Now looking more into it, i see that powernv has something called
>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
>>
>> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
>> an internal variable, rather than using (smp_cpus / smp_cores /
>> smp_threads) everywhere.  But we shouldn't have it as a direct
>> user-settable property, instead setting it from the -smp command line
>> option.
> 
> Something like the below works till num_chips=2, after that guest does
> not boot up. This might be some limitation within the OS, Cedric might
> have some clue. Otherwise, I see that multiple chips are created with
> single core having single thread.
> 
>     ppc/pnv: Use num_chips for multiple sockets
>     
>     When the user does not provide the cpu topology, e.g. "-smp 4", machine 
> fails to
>     initialize 4 cpus. QEMU assumes smp_threads and smp_cores both as 1. Make 
> sure
>     that we initialize multiple chips for this.

-smp 4          would give a machine with 4 sockets with 1 core
-smp 4,cores=4  would give a machine with 1 socket  with 4 cores

correct ?

C.

>     Remove the user-settable property num_chips from machine command line 
> option
>     
>     Signed-off-by: Nikunj A Dadhania <address@hidden>
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 9724719..fa501f9 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -709,7 +709,17 @@ static void ppc_powernv_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    pnv->num_chips = smp_cpus / (smp_cores * smp_threads);
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
> +
> +    if (smp_cpus != (smp_threads * pnv->num_chips * smp_cores)) {
> +        error_report("cpu topology not balanced: "
> +                     "chips (%u) * cores (%u) * threads (%u) != "
> +                     "number of cpus (%u)",
> +                     pnv->num_chips, smp_cores, smp_threads, smp_cpus);
> +        exit(1);
> +    }
> +
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
>          Object *chip = object_new(chip_typename);
> @@ -1255,53 +1265,12 @@ static void pnv_pic_print_info(InterruptStatsProvider 
> *obj,
>      }
>  }
>  
> -static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    visit_type_uint32(v, name, &POWERNV_MACHINE(obj)->num_chips, errp);
> -}
> -
> -static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    PnvMachineState *pnv = POWERNV_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 powernv_machine_initfn(Object *obj)
>  {
>      PnvMachineState *pnv = POWERNV_MACHINE(obj);
>      pnv->num_chips = 1;
>  }
>  
> -static void powernv_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 powernv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1321,8 +1290,6 @@ static void powernv_machine_class_init(ObjectClass *oc, 
> void *data)
>      xic->ics_get = pnv_ics_get;
>      xic->ics_resend = pnv_ics_resend;
>      ispc->print_info = pnv_pic_print_info;
> -
> -    powernv_machine_class_props_init(oc);
>  }
>  
>  static const TypeInfo powernv_machine_info = {
>