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

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

David Gibson <david@gibson.dropbear.id.au> 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.
    
    Remove the user-settable property num_chips from machine command line option
    
    Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

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 = {