* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.