| Message ID | 157686070815.97184.16561911770080665569.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/pnv: Improve command line experience with multi-chip | expand |
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.
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[]) {
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(-)