* [PATCH 0/2] ppc/pnv: Improve command line experience with multi-chip
@ 2019-12-20 16:51 Greg Kurz
2019-12-20 16:51 ` [PATCH 1/2] ppc/pnv: Exit gracefully if CPU topology doesn't match the machine capacities Greg Kurz
2019-12-20 16:51 ` [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips Greg Kurz
0 siblings, 2 replies; 6+ messages in thread
From: Greg Kurz @ 2019-12-20 16:51 UTC (permalink / raw)
To: David Gibson, Cédric Le Goater; +Cc: qemu-ppc, qemu-devel
Patch 1 adds a sanity check of the CPU topology to avoid an assertion in
the multi TCG code that can be easily triggered with the "num-chips" property
of the powernv machine.
This still leaves us with the fact that the number of chips must be specified
twice on the command line: once for the "num-chips" property and once for the
CPU topology with '-smp sockets'. Patch 2 tries to tackle that by changing the
default value of "num-chips" (currently 1) to the number of "sockets" of the
CPU topology, computed at startup time as:
smp.max_cpus / (smp.cores * smp.threads)
This allow to indirectly configure the number of chips and have a valid
CPU topology with a single '-smp sockets'.
--
Greg
---
Greg Kurz (2):
ppc/pnv: Exit gracefully if CPU topology doesn't match the machine capacities
ppc/pnv: Use the CPU topology to compute the default number of chips
hw/ppc/pnv.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] ppc/pnv: Exit gracefully if CPU topology doesn't match the machine capacities 2019-12-20 16:51 [PATCH 0/2] ppc/pnv: Improve command line experience with multi-chip Greg Kurz @ 2019-12-20 16:51 ` Greg Kurz 2019-12-20 16:51 ` [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips Greg Kurz 1 sibling, 0 replies; 6+ messages in thread From: Greg Kurz @ 2019-12-20 16:51 UTC (permalink / raw) To: David Gibson, Cédric Le Goater; +Cc: qemu-ppc, qemu-devel QEMU crashes when started with: -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) This happens because the powernv machine creates num-chips * smp.cores CPUs, which might exceed the maximum number of CPUs of the CPU topology as computed by smp_parse(). Check the CPU topology in pnv_set_num_chips(). Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/pnv.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index f77e7ca84ede..f8cf2b6d760f 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -1706,7 +1706,8 @@ 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; + MachineState *ms = MACHINE(pnv); + uint32_t num_chips, num_cpus; Error *local_err = NULL; visit_type_uint32(v, name, &num_chips, &local_err); @@ -1724,6 +1725,13 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name, return; } + num_cpus = num_chips * ms->smp.cores * ms->smp.threads; + if (num_cpus > ms->smp.max_cpus) { + error_setg(errp, "%d chips don't fit in the CPU topology", num_chips); + error_append_hint(errp, "Try -smp sockets=%d.\n", num_chips); + return; + } + pnv->num_chips = num_chips; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips 2019-12-20 16:51 [PATCH 0/2] ppc/pnv: Improve command line experience with multi-chip Greg Kurz 2019-12-20 16:51 ` [PATCH 1/2] ppc/pnv: Exit gracefully if CPU topology doesn't match the machine capacities Greg Kurz @ 2019-12-20 16:51 ` Greg Kurz 2019-12-21 0:39 ` David Gibson 1 sibling, 1 reply; 6+ messages in thread From: Greg Kurz @ 2019-12-20 16:51 UTC (permalink / raw) To: David Gibson, Cédric Le Goater; +Cc: qemu-ppc, qemu-devel 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(-) 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[]) { ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips 2019-12-20 16:51 ` [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips Greg Kurz @ 2019-12-21 0:39 ` David Gibson 2019-12-21 10:28 ` Greg Kurz 0 siblings, 1 reply; 6+ messages in thread From: David Gibson @ 2019-12-21 0:39 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3264 bytes --] 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[]) { > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips 2019-12-21 0:39 ` David Gibson @ 2019-12-21 10:28 ` Greg Kurz 2020-01-06 7:25 ` Cédric Le Goater 0 siblings, 1 reply; 6+ messages in thread From: Greg Kurz @ 2019-12-21 10:28 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3469 bytes --] 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[]) { > > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips 2019-12-21 10:28 ` Greg Kurz @ 2020-01-06 7:25 ` Cédric Le Goater 0 siblings, 0 replies; 6+ messages in thread From: Cédric Le Goater @ 2020-01-06 7:25 UTC (permalink / raw) To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-06 7:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-20 16:51 [PATCH 0/2] ppc/pnv: Improve command line experience with multi-chip Greg Kurz 2019-12-20 16:51 ` [PATCH 1/2] ppc/pnv: Exit gracefully if CPU topology doesn't match the machine capacities Greg Kurz 2019-12-20 16:51 ` [PATCH 2/2] ppc/pnv: Use the CPU topology to compute the default number of chips Greg Kurz 2019-12-21 0:39 ` David Gibson 2019-12-21 10:28 ` Greg Kurz 2020-01-06 7:25 ` Cédric Le Goater
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).