| Message ID | 157830658266.533764.2214183961444213947.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/pnv: Drop "num-chips" machine property | expand |
On 1/6/20 11:29 AM, Greg Kurz wrote: > The number of CPU chips of the powernv machine is configurable through a > "num-chips" property. This doesn't fit well with the CPU topology, eg. > some configurations can come up with more CPUs than the maximum of CPUs > set in the toplogy. This causes assertion to be hit with mttcg: > > -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) > > Mttcg 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'. > > It thus seems that "num-chips" doesn't bring anything but forcing the user > to specify the requested number of chips on the command line twice. Simplify > the command line by computing the number of chips based on the CPU topology > exclusively. The powernv machine isn't a production thing ; it is mostly > used by developpers to prepare the bringup of real HW. Because of this and > for simplicity, this deliberately ignores the official deprecation process > and dumps "num-chips" right away : '-smp sockets=N' is now the only way to > control the number of CPU chips. > > This is done at machine init because smp_parse() is called after instance > init. > > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/ppc/pnv.c | 62 +++++++++++----------------------------------------------- > 1 file changed, 12 insertions(+), 50 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index f77e7ca84ede..b225ffbb2c41 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -768,6 +768,18 @@ static void pnv_init(MachineState *machine) > exit(1); > } > > + pnv->num_chips = > + machine->smp.max_cpus / (machine->smp.cores * machine->smp.threads); > + /* > + * 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(pnv->num_chips) || pnv->num_chips > 4) { > + error_report("invalid number of chips: '%d'", pnv->num_chips); > + error_printf("Try '-smp sockets=N'. Valid values are : 1, 2 or 4.\n"); > + exit(1); > + } > + > pnv->chips = g_new0(PnvChip *, pnv->num_chips); > for (i = 0; i < pnv->num_chips; i++) { > char chip_name[32]; > @@ -1696,53 +1708,6 @@ PnvChip *pnv_get_chip(uint32_t chip_id) > return NULL; > } > > -static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name, > - void *opaque, Error **errp) > -{ > - visit_type_uint32(v, name, &PNV_MACHINE(obj)->num_chips, errp); > -} > - > -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; > - 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 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", > - 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 pnv_machine_power8_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -1812,8 +1777,6 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data) > */ > mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE; > ispc->print_info = pnv_pic_print_info; > - > - pnv_machine_class_props_init(oc); > } > > #define DEFINE_PNV8_CHIP_TYPE(type, class_initfn) \ > @@ -1866,7 +1829,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 Mon, Jan 06, 2020 at 11:29:42AM +0100, Greg Kurz wrote: > The number of CPU chips of the powernv machine is configurable through a > "num-chips" property. This doesn't fit well with the CPU topology, eg. > some configurations can come up with more CPUs than the maximum of CPUs > set in the toplogy. This causes assertion to be hit with mttcg: > > -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) > > Mttcg 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'. > > It thus seems that "num-chips" doesn't bring anything but forcing the user > to specify the requested number of chips on the command line twice. Simplify > the command line by computing the number of chips based on the CPU topology > exclusively. The powernv machine isn't a production thing ; it is mostly > used by developpers to prepare the bringup of real HW. Because of this and > for simplicity, this deliberately ignores the official deprecation process > and dumps "num-chips" right away : '-smp sockets=N' is now the only way to > control the number of CPU chips. > > This is done at machine init because smp_parse() is called after instance > init. > > Signed-off-by: Greg Kurz <groug@kaod.org> Applied to ppc-for-5.0, thanks. > --- > hw/ppc/pnv.c | 62 +++++++++++----------------------------------------------- > 1 file changed, 12 insertions(+), 50 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index f77e7ca84ede..b225ffbb2c41 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -768,6 +768,18 @@ static void pnv_init(MachineState *machine) > exit(1); > } > > + pnv->num_chips = > + machine->smp.max_cpus / (machine->smp.cores * machine->smp.threads); > + /* > + * 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(pnv->num_chips) || pnv->num_chips > 4) { > + error_report("invalid number of chips: '%d'", pnv->num_chips); > + error_printf("Try '-smp sockets=N'. Valid values are : 1, 2 or 4.\n"); > + exit(1); > + } > + > pnv->chips = g_new0(PnvChip *, pnv->num_chips); > for (i = 0; i < pnv->num_chips; i++) { > char chip_name[32]; > @@ -1696,53 +1708,6 @@ PnvChip *pnv_get_chip(uint32_t chip_id) > return NULL; > } > > -static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name, > - void *opaque, Error **errp) > -{ > - visit_type_uint32(v, name, &PNV_MACHINE(obj)->num_chips, errp); > -} > - > -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; > - 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 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", > - 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 pnv_machine_power8_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -1812,8 +1777,6 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data) > */ > mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE; > ispc->print_info = pnv_pic_print_info; > - > - pnv_machine_class_props_init(oc); > } > > #define DEFINE_PNV8_CHIP_TYPE(type, class_initfn) \ > @@ -1866,7 +1829,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[]) { >
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index f77e7ca84ede..b225ffbb2c41 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -768,6 +768,18 @@ static void pnv_init(MachineState *machine) exit(1); } + pnv->num_chips = + machine->smp.max_cpus / (machine->smp.cores * machine->smp.threads); + /* + * 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(pnv->num_chips) || pnv->num_chips > 4) { + error_report("invalid number of chips: '%d'", pnv->num_chips); + error_printf("Try '-smp sockets=N'. Valid values are : 1, 2 or 4.\n"); + exit(1); + } + pnv->chips = g_new0(PnvChip *, pnv->num_chips); for (i = 0; i < pnv->num_chips; i++) { char chip_name[32]; @@ -1696,53 +1708,6 @@ PnvChip *pnv_get_chip(uint32_t chip_id) return NULL; } -static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name, - void *opaque, Error **errp) -{ - visit_type_uint32(v, name, &PNV_MACHINE(obj)->num_chips, errp); -} - -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; - 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 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", - 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 pnv_machine_power8_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -1812,8 +1777,6 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data) */ mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE; ispc->print_info = pnv_pic_print_info; - - pnv_machine_class_props_init(oc); } #define DEFINE_PNV8_CHIP_TYPE(type, class_initfn) \ @@ -1866,7 +1829,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[]) {
The number of CPU chips of the powernv machine is configurable through a "num-chips" property. This doesn't fit well with the CPU topology, eg. some configurations can come up with more CPUs than the maximum of CPUs set in the toplogy. This causes assertion to be hit with mttcg: -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) Mttcg 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'. It thus seems that "num-chips" doesn't bring anything but forcing the user to specify the requested number of chips on the command line twice. Simplify the command line by computing the number of chips based on the CPU topology exclusively. The powernv machine isn't a production thing ; it is mostly used by developpers to prepare the bringup of real HW. Because of this and for simplicity, this deliberately ignores the official deprecation process and dumps "num-chips" right away : '-smp sockets=N' is now the only way to control the number of CPU chips. This is done at machine init because smp_parse() is called after instance init. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/pnv.c | 62 +++++++++++----------------------------------------------- 1 file changed, 12 insertions(+), 50 deletions(-)