Message ID | 1509734853-3014-1-git-send-email-cota@braap.org |
---|---|
State | New |
Headers | show |
Series | hw: add .min_cpus and .default_cpus fields to machine_class | expand |
On Fri, Nov 03, 2017 at 14:47:33 -0400, Emilio G. Cota wrote: > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > index e2d15a1..395d1b5 100644 > --- a/hw/arm/xlnx-zcu102.c > +++ b/hw/arm/xlnx-zcu102.c > @@ -185,6 +185,9 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_ep108_machine_init_typeinfo = { > @@ -241,6 +244,8 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } Should we update max_cpus to just NUM_APU_CPUS as well for these boards? -smp 5 or 6 (NUM_APU + NUM_RPU) still gets us 4 vCPUs. I see there's code for RPU cpus but it seems disabled at compile-time at xlnx-zynqmp.c:431: DEFINE_PROP_BOOL("has_rpu", XlnxZynqMPState, has_rpu, false) Or is there a run-time way to override this? Thanks, Emilio
On Fri, Nov 03, 2017 at 02:56:10PM -0400, Emilio G. Cota wrote: > On Fri, Nov 03, 2017 at 14:47:33 -0400, Emilio G. Cota wrote: > > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > > index e2d15a1..395d1b5 100644 > > --- a/hw/arm/xlnx-zcu102.c > > +++ b/hw/arm/xlnx-zcu102.c > > @@ -185,6 +185,9 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > > mc->block_default_type = IF_IDE; > > mc->units_per_default_bus = 1; > > mc->ignore_memory_transaction_failures = true; > > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > > } > > > > static const TypeInfo xlnx_ep108_machine_init_typeinfo = { > > @@ -241,6 +244,8 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > > mc->units_per_default_bus = 1; > > mc->ignore_memory_transaction_failures = true; > > mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > > } > > Should we update max_cpus to just NUM_APU_CPUS as well for these boards? > -smp 5 or 6 (NUM_APU + NUM_RPU) still gets us 4 vCPUs. > > I see there's code for RPU cpus but it seems disabled at compile-time > at xlnx-zynqmp.c:431: > DEFINE_PROP_BOOL("has_rpu", XlnxZynqMPState, has_rpu, false) > Or is there a run-time way to override this? Device properties can be overridden using -global, e.g.: -global driver=xlnx,,zynqmp,property=has_rpu,value=on (",," is how commas are escaped in QEMU options)
On Fri, Nov 03, 2017 at 21:02:33 +0100, Eduardo Habkost wrote: > On Fri, Nov 03, 2017 at 02:56:10PM -0400, Emilio G. Cota wrote: > > On Fri, Nov 03, 2017 at 14:47:33 -0400, Emilio G. Cota wrote: > > > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > > > index e2d15a1..395d1b5 100644 > > > --- a/hw/arm/xlnx-zcu102.c > > > +++ b/hw/arm/xlnx-zcu102.c (snip) > > > > Should we update max_cpus to just NUM_APU_CPUS as well for these boards? > > -smp 5 or 6 (NUM_APU + NUM_RPU) still gets us 4 vCPUs. > > > > I see there's code for RPU cpus but it seems disabled at compile-time > > at xlnx-zynqmp.c:431: > > DEFINE_PROP_BOOL("has_rpu", XlnxZynqMPState, has_rpu, false) > > Or is there a run-time way to override this? > > Device properties can be overridden using -global, e.g.: > > -global driver=xlnx,,zynqmp,property=has_rpu,value=on > > (",," is how commas are escaped in QEMU options) Very interesting! This raises two separate issues. 1. Using this feature breaks 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24). For instance: qemu-system-aarch64 -machine xlnx-zcu102 \ -global driver=xlnx,,zynqmp,property=has_rpu,value=on This will try to initialize TCG twice. The reason is that the second set of CPUs (the "RPUs") is of a different "object type name", which ends up as a different CPUClass. In xlnx-zynqmp.c: for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) { char *name; object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]), "cortex-r5-" TYPE_ARM_CPU); This hunk only runs when we use the -global override. This other hunk always runs. It initializes the "APUs": for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) { object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]), "cortex-a53-" TYPE_ARM_CPU); A trivial, ugly fix would be to either use the same "object name" for both sets of CPUs or (re)introduce a static variable in arm_translate_init. I'd prefer to be able to set tcg_initialized field directly for the RPU's CPUClass. Is that possible? I don't know much about qom/object code, so any good suggestion here would be appreciated. 2. Coming back to the original problem: given that we can get additional vCPUs, I think we need an additional flag to signal this. Otherwise we'll have to always do "max_cpus = mc.max_cpus", which for most machines would be a huge waste of TCG regions. See delta below. Thanks, Emilio diff --git a/include/hw/boards.h b/include/hw/boards.h index 62f160e..8c8ce51 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -103,6 +103,7 @@ typedef struct { /** * MachineClass: * @max_cpus: maximum number of CPUs supported. Default: 1 + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus * @min_cpus: minimum number of CPUs supported. Default: 1 * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 * @get_hotplug_handler: this function is called during bus-less @@ -181,7 +182,8 @@ struct MachineClass { no_sdcard:1, has_dynamic_sysbus:1, pci_allow_0_address:1, - legacy_fw_cfg_order:1; + legacy_fw_cfg_order:1, + force_max_cpus; int is_default; const char *default_machine_opts; const char *default_boot_order; diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c index 395d1b5..e406dc3 100644 --- a/hw/arm/xlnx-zcu102.c +++ b/hw/arm/xlnx-zcu102.c @@ -186,6 +186,7 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) mc->units_per_default_bus = 1; mc->ignore_memory_transaction_failures = true; mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; + mc->force_max_cpus = 1; mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; } @@ -244,6 +245,7 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) mc->units_per_default_bus = 1; mc->ignore_memory_transaction_failures = true; mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; + mc->force_max_cpus = 1; mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; } diff --git a/vl.c b/vl.c index 3ca5ee8..a21183d 100644 --- a/vl.c +++ b/vl.c @@ -4339,6 +4339,15 @@ int main(int argc, char **argv, char **envp) max_cpus = machine_class->default_cpus; } + /* + * Some boards can instantiate additional CPUs, e.g. by overriding + * device params via -global arguments, so they enforce the value + * that max_cpus should take. + */ + if (machine_class->force_max_cpus) { + max_cpus = machine_class->max_cpus; + } + /* sanity-check smp_cpus and max_cpus */ if (smp_cpus < machine_class->min_cpus) { error_report("Invalid SMP CPUs %d. The min CPUs "
On Fri, Nov 03, 2017 at 06:24:07PM -0400, Emilio G. Cota wrote: > On Fri, Nov 03, 2017 at 21:02:33 +0100, Eduardo Habkost wrote: > > On Fri, Nov 03, 2017 at 02:56:10PM -0400, Emilio G. Cota wrote: > > > On Fri, Nov 03, 2017 at 14:47:33 -0400, Emilio G. Cota wrote: > > > > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > > > > index e2d15a1..395d1b5 100644 > > > > --- a/hw/arm/xlnx-zcu102.c > > > > +++ b/hw/arm/xlnx-zcu102.c > (snip) > > > > > > Should we update max_cpus to just NUM_APU_CPUS as well for these boards? > > > -smp 5 or 6 (NUM_APU + NUM_RPU) still gets us 4 vCPUs. > > > > > > I see there's code for RPU cpus but it seems disabled at compile-time > > > at xlnx-zynqmp.c:431: > > > DEFINE_PROP_BOOL("has_rpu", XlnxZynqMPState, has_rpu, false) > > > Or is there a run-time way to override this? > > > > Device properties can be overridden using -global, e.g.: > > > > -global driver=xlnx,,zynqmp,property=has_rpu,value=on > > > > (",," is how commas are escaped in QEMU options) > > Very interesting! This raises two separate issues. > > 1. Using this feature breaks 55c3cee ("qom: Introduce CPUClass.tcg_initialize", > 2017-10-24). For instance: > qemu-system-aarch64 -machine xlnx-zcu102 \ > -global driver=xlnx,,zynqmp,property=has_rpu,value=on > This will try to initialize TCG twice. The reason is that the second > set of CPUs (the "RPUs") is of a different "object type name", which ends > up as a different CPUClass. In xlnx-zynqmp.c: > > for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) { > char *name; > > object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]), > "cortex-r5-" TYPE_ARM_CPU); > This hunk only runs when we use the -global override. > > This other hunk always runs. It initializes the "APUs": > for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) { > object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]), > "cortex-a53-" TYPE_ARM_CPU); > > A trivial, ugly fix would be to either use the same "object name" > for both sets of CPUs or (re)introduce a static variable in > arm_translate_init. > I'd prefer to be able to set tcg_initialized field directly for > the RPU's CPUClass. Is that possible? I don't know much about > qom/object code, so any good suggestion here would be appreciated. IMO, initialization state doesn't belong to CPUClass. We already have a single accelerator object in MachineState::accelerator, and tcg_initialized could be moved to a AccelState::initialized field. > > 2. Coming back to the original problem: given that we can get > additional vCPUs, I think we need an additional flag to signal > this. Otherwise we'll have to always do "max_cpus = mc.max_cpus", > which for most machines would be a huge waste of TCG regions. > See delta below. > > Thanks, > > Emilio > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 62f160e..8c8ce51 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -103,6 +103,7 @@ typedef struct { > /** > * MachineClass: > * @max_cpus: maximum number of CPUs supported. Default: 1 > + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus > * @min_cpus: minimum number of CPUs supported. Default: 1 > * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > * @get_hotplug_handler: this function is called during bus-less If we have a field containing the default value for smp_cpus (default_cpus), what about a default_max_cpus field for the default value of max_cpus? This could make the rules a bit easier to follow. (But I wonder if there's a way we can do this without introducing another MachineClass field and making the initialization code more complex.) The fact that MachineClass::max_cpus and the 'max_cpus' globals have completely different meanings makes this confusing enough. Maybe we should rename the 'max_cpus' global variable to 'max_hotplug_cpus'. > @@ -181,7 +182,8 @@ struct MachineClass { > no_sdcard:1, > has_dynamic_sysbus:1, > pci_allow_0_address:1, > - legacy_fw_cfg_order:1; > + legacy_fw_cfg_order:1, > + force_max_cpus; > int is_default; > const char *default_machine_opts; > const char *default_boot_order; > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > index 395d1b5..e406dc3 100644 > --- a/hw/arm/xlnx-zcu102.c > +++ b/hw/arm/xlnx-zcu102.c > @@ -186,6 +186,7 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->force_max_cpus = 1; > mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > @@ -244,6 +245,7 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->force_max_cpus = 1; > mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > diff --git a/vl.c b/vl.c > index 3ca5ee8..a21183d 100644 > --- a/vl.c > +++ b/vl.c > @@ -4339,6 +4339,15 @@ int main(int argc, char **argv, char **envp) > max_cpus = machine_class->default_cpus; > } > > + /* > + * Some boards can instantiate additional CPUs, e.g. by overriding > + * device params via -global arguments, so they enforce the value > + * that max_cpus should take. > + */ > + if (machine_class->force_max_cpus) { > + max_cpus = machine_class->max_cpus; > + } > + > /* sanity-check smp_cpus and max_cpus */ > if (smp_cpus < machine_class->min_cpus) { > error_report("Invalid SMP CPUs %d. The min CPUs "
On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: > IMO, initialization state doesn't belong to CPUClass. We already > have a single accelerator object in MachineState::accelerator, > and tcg_initialized could be moved to a AccelState::initialized > field. I don't know how to cleanly get AccelState from a CPUClass pointer (as I said I'm not familiar with object code / qom) -- suggestions welcome! The best I could come up in the limited time I have for this is to use a static bool, as shown below. ---8<--- Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24) introduces a per-CPUClass bool that we check so that the target CPU is initialized for TCG only once. This works well except when we end up creating more than one CPUClass, in which case we end up incorrectly initializing TCG more than once, i.e. once for each CPUClass. This can be replicated with: $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \ -global driver=xlnx,,zynqmp,property=has_rpu,value=on In this case the class name of the "RPUs" is prefixed by "cortex-r5-", whereas the "regular" CPUs are prefixed by "cortex-a53-". This results in two CPUClass instances being created. Fix it by introducing a static variable, so that only the first target CPU being initialized will initialize the target-dependent part of TCG, regardless of CPUClass instances. Fixes: 55c3ceef61fcf06fc98ddc752b7cce788ce7680b Signed-off-by: Emilio G. Cota <cota@braap.org> --- exec.c | 5 +++-- include/qom/cpu.h | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/exec.c b/exec.c index 97a24a8..8b579c0 100644 --- a/exec.c +++ b/exec.c @@ -792,11 +792,12 @@ void cpu_exec_initfn(CPUState *cpu) void cpu_exec_realizefn(CPUState *cpu, Error **errp) { CPUClass *cc = CPU_GET_CLASS(cpu); + static bool tcg_target_initialized; cpu_list_add(cpu); - if (tcg_enabled() && !cc->tcg_initialized) { - cc->tcg_initialized = true; + if (tcg_enabled() && !tcg_target_initialized) { + tcg_target_initialized = true; cc->tcg_initialize(); } diff --git a/include/qom/cpu.h b/include/qom/cpu.h index fa4b0c9..c2fa151 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -209,7 +209,6 @@ typedef struct CPUClass { /* Keep non-pointer data at the end to minimize holes. */ int gdb_num_core_regs; bool gdb_stop_before_watchpoint; - bool tcg_initialized; } CPUClass; #ifdef HOST_WORDS_BIGENDIAN
On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: > On Fri, Nov 03, 2017 at 06:24:07PM -0400, Emilio G. Cota wrote: > > /** > > * MachineClass: > > * @max_cpus: maximum number of CPUs supported. Default: 1 > > + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus > > * @min_cpus: minimum number of CPUs supported. Default: 1 > > * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > > * @get_hotplug_handler: this function is called during bus-less > > If we have a field containing the default value for smp_cpus > (default_cpus), what about a default_max_cpus field for the > default value of max_cpus? This could make the rules a bit > easier to follow. > > (But I wonder if there's a way we can do this without introducing > another MachineClass field and making the initialization code > more complex.) I don't think the rules are too hard to follow, since they're trivial for most machines because the defaults are reasonable (i.e. they only set max_cpus, and everything else is set to 1). Some machines though are tricky in what can and cannot support, e.g. the Xilinx ones we've been dealing with in this thread. wrt .default_max_cpus instead of .force_max_cpus: it lets users shoot themselves in the foot, so I wouldn't go this route. For instance, what do we do if the user passes maxcpus < default_max_cpus? max_cpus would be set to maxcpus, yet they could still pass the -global override that instantiates two extra CPUs, and at that point we'd fail (in TCG) without providing a meaningful error. We could increase maxcpus to default_max_cpus, but at that point default_max_cpus becomes the same thing as force_max_cpus (!). An alternative I've come up with is to have a field that specifies how many CPUs could be created in addition to those specified in max_cpus (in the Xilinx case these would be the RPUs). Patch below. > The fact that MachineClass::max_cpus and the 'max_cpus' globals > have completely different meanings makes this confusing enough. > Maybe we should rename the 'max_cpus' global variable to > 'max_hotplug_cpus'. I have no trouble with their different meanings, although I admit I am now quite well acquainted with this code so my bias is strong :-) Thanks, Emilio ---8<--- Subject: [PATCH] hw: add .{additional_max,min,default}_cpus fields to machine_class max_cpus needs to be an upper bound on the number of vCPUs initialized; otherwise TCG region initialization breaks. Some boards initialize a hard-coded number of vCPUs, which is not captured by the global max_cpus. Fix it by adding the .min_cpus field to machine_class. A subset of those boards can also instantiate additional vCPUs beyond what is specified in -smp, e.g. via -global overrides. To ensure that max_cpus remains an upper bound on the number of vCPUs that can be instantiated, we also add the .additional_max_cpus field to machine_class. This commit also changes some user-facing behaviour: we now die if -smp is below this hard-coded vCPU minimum instead of silently ignoring the passed -smp value (sometimes announcing this by printing a warning). However, the introduction of .default_cpus lessens the likelihood that users will notice this: if -smp isn't set, we now assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, if a user does not set -smp, they always get a correct number of vCPUs. This change fixes 3468b59 ("tcg: enable multiple TCG contexts in softmmu", 2017-10-24), which broke TCG initialization for some ARM boards. Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 Reported-by: Thomas Huth <thuth@redhat.com> Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Emilio G. Cota <cota@braap.org> --- hw/arm/exynos4_boards.c | 12 ++++-------- hw/arm/raspi.c | 2 ++ hw/arm/xlnx-zcu102.c | 9 ++++++++- include/hw/boards.h | 10 ++++++++++ vl.c | 38 +++++++++++++++++++++++++++++++++++--- 5 files changed, 59 insertions(+), 12 deletions(-) diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c index f1441ec..750162c 100644 --- a/hw/arm/exynos4_boards.c +++ b/hw/arm/exynos4_boards.c @@ -27,7 +27,6 @@ #include "qemu-common.h" #include "cpu.h" #include "sysemu/sysemu.h" -#include "sysemu/qtest.h" #include "hw/sysbus.h" #include "net/net.h" #include "hw/arm/arm.h" @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, Exynos4BoardType board_type) { Exynos4BoardState *s = g_new(Exynos4BoardState, 1); - MachineClass *mc = MACHINE_GET_CLASS(machine); - - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" - " value", - mc->name, EXYNOS4210_NCPUS); - } exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; exynos4_board_binfo.board_id = exynos4_board_id[board_type]; @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) mc->desc = "Samsung NURI board (Exynos4210)"; mc->init = nuri_init; mc->max_cpus = EXYNOS4210_NCPUS; + mc->min_cpus = EXYNOS4210_NCPUS; + mc->default_cpus = EXYNOS4210_NCPUS; mc->ignore_memory_transaction_failures = true; } @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) mc->desc = "Samsung SMDKC210 board (Exynos4210)"; mc->init = smdkc210_init; mc->max_cpus = EXYNOS4210_NCPUS; + mc->min_cpus = EXYNOS4210_NCPUS; + mc->default_cpus = EXYNOS4210_NCPUS; mc->ignore_memory_transaction_failures = true; } diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index 5941c9f..cd5fa8c 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) mc->no_floppy = 1; mc->no_cdrom = 1; mc->max_cpus = BCM2836_NCPUS; + mc->min_cpus = BCM2836_NCPUS; + mc->default_cpus = BCM2836_NCPUS; mc->default_ram_size = 1024 * 1024 * 1024; mc->ignore_memory_transaction_failures = true; }; diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c index e2d15a1..0f1837e 100644 --- a/hw/arm/xlnx-zcu102.c +++ b/hw/arm/xlnx-zcu102.c @@ -185,6 +185,10 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) mc->block_default_type = IF_IDE; mc->units_per_default_bus = 1; mc->ignore_memory_transaction_failures = true; + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; } static const TypeInfo xlnx_ep108_machine_init_typeinfo = { @@ -240,7 +244,10 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) mc->block_default_type = IF_IDE; mc->units_per_default_bus = 1; mc->ignore_memory_transaction_failures = true; - mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; } static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { diff --git a/include/hw/boards.h b/include/hw/boards.h index 191a5b3..be582dd 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -102,6 +102,13 @@ typedef struct { /** * MachineClass: + * @max_cpus: maximum number of CPUs supported. Default: 1 + * @max_additional_cpus: # of CPUs that can be instantiated in addition to those + * instantiated with -smp. For instance, if a machine has 4 main CPUs and 2 + * optional CPUs (e.g. instantiated via -global overrides), then @max_cpus + * and @max_additional_cpus will be set to 4 and 2, respectively. Default: 0 + * @min_cpus: minimum number of CPUs supported. Default: 1 + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 * @get_hotplug_handler: this function is called during bus-less * device hotplug. If defined it returns pointer to an instance * of HotplugHandler object, which handles hotplug operation @@ -167,6 +174,9 @@ struct MachineClass { BlockInterfaceType block_default_type; int units_per_default_bus; int max_cpus; + int max_additional_cpus; + int min_cpus; + int default_cpus; unsigned int no_serial:1, no_parallel:1, use_virtcon:1, diff --git a/vl.c b/vl.c index ec29909..e3ee36e 100644 --- a/vl.c +++ b/vl.c @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; Chardev *sclp_hds[MAX_SCLP_CONSOLES]; int win2k_install_hack = 0; int singlestep = 0; -int smp_cpus = 1; -unsigned int max_cpus = 1; +int smp_cpus; +unsigned int max_cpus; int smp_cores = 1; int smp_threads = 1; int acpi_enabled = 1; @@ -4330,12 +4330,44 @@ int main(int argc, char **argv, char **envp) smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ + machine_class->min_cpus = machine_class->min_cpus ?: 1; + machine_class->default_cpus = machine_class->default_cpus ?: 1; + + /* if -smp is not set, default to mc->default_cpus */ + if (!smp_cpus) { + smp_cpus = machine_class->default_cpus; + max_cpus = machine_class->default_cpus; + } + + /* sanity-check smp_cpus and max_cpus */ + if (smp_cpus < machine_class->min_cpus) { + error_report("Invalid SMP CPUs %d. The min CPUs " + "supported by machine '%s' is %d", smp_cpus, + machine_class->name, machine_class->min_cpus); + exit(1); + } if (max_cpus > machine_class->max_cpus) { - error_report("Invalid SMP CPUs %d. The max CPUs " + error_report("Invalid max SMP CPUs %d. The max CPUs " "supported by machine '%s' is %d", max_cpus, machine_class->name, machine_class->max_cpus); exit(1); } + if (max_cpus < machine_class->min_cpus) { + error_report("Invalid max SMP CPUs %d. The min CPUs " + "supported by machine '%s' is %d", max_cpus, + machine_class->name, machine_class->min_cpus); + exit(1); + } + + /* + * Some boards can instantiate additional CPUs, e.g. by overriding + * device params via -global arguments. Update max_cpus so that + * we are sure to have an upper bound on the maximum number of + * vCPUs that might be instantiated. + */ + if (machine_class->max_additional_cpus) { + max_cpus += machine_class->max_additional_cpus; + } /* * Get the default machine options from the machine if it is not already
On Mon, Nov 6, 2017 at 1:54 PM, Emilio G. Cota <cota@braap.org> wrote: > On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: >> On Fri, Nov 03, 2017 at 06:24:07PM -0400, Emilio G. Cota wrote: >> > /** >> > * MachineClass: >> > * @max_cpus: maximum number of CPUs supported. Default: 1 >> > + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus >> > * @min_cpus: minimum number of CPUs supported. Default: 1 >> > * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 >> > * @get_hotplug_handler: this function is called during bus-less >> >> If we have a field containing the default value for smp_cpus >> (default_cpus), what about a default_max_cpus field for the >> default value of max_cpus? This could make the rules a bit >> easier to follow. >> >> (But I wonder if there's a way we can do this without introducing >> another MachineClass field and making the initialization code >> more complex.) > > I don't think the rules are too hard to follow, since they're trivial > for most machines because the defaults are reasonable (i.e. they > only set max_cpus, and everything else is set to 1). Some machines > though are tricky in what can and cannot support, e.g. the Xilinx > ones we've been dealing with in this thread. > > wrt .default_max_cpus instead of .force_max_cpus: it lets users > shoot themselves in the foot, so I wouldn't go this route. > For instance, what do we do if the user passes maxcpus < default_max_cpus? > max_cpus would be set to maxcpus, yet they could still pass the > -global override that instantiates two extra CPUs, and at that point > we'd fail (in TCG) without providing a meaningful error. > > We could increase maxcpus to default_max_cpus, but at that point > default_max_cpus becomes the same thing as force_max_cpus (!). > > An alternative I've come up with is to have a field that specifies how > many CPUs could be created in addition to those specified in max_cpus > (in the Xilinx case these would be the RPUs). Patch below. > >> The fact that MachineClass::max_cpus and the 'max_cpus' globals >> have completely different meanings makes this confusing enough. >> Maybe we should rename the 'max_cpus' global variable to >> 'max_hotplug_cpus'. > > I have no trouble with their different meanings, although I admit > I am now quite well acquainted with this code so my bias is strong :-) > > Thanks, > > Emilio > > ---8<--- > > Subject: [PATCH] hw: add .{additional_max,min,default}_cpus fields to > machine_class > > max_cpus needs to be an upper bound on the number of vCPUs > initialized; otherwise TCG region initialization breaks. > > Some boards initialize a hard-coded number of vCPUs, which is not > captured by the global max_cpus. Fix it by adding the .min_cpus > field to machine_class. > > A subset of those boards can also instantiate additional vCPUs > beyond what is specified in -smp, e.g. via -global overrides. To > ensure that max_cpus remains an upper bound on the number of vCPUs > that can be instantiated, we also add the .additional_max_cpus field > to machine_class. > > This commit also changes some user-facing behaviour: we now die if > -smp is below this hard-coded vCPU minimum instead of silently > ignoring the passed -smp value (sometimes announcing this by printing > a warning). However, the introduction of .default_cpus lessens the > likelihood that users will notice this: if -smp isn't set, we now > assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, > if a user does not set -smp, they always get a correct number of vCPUs. > > This change fixes 3468b59 ("tcg: enable multiple TCG contexts in > softmmu", 2017-10-24), which broke TCG initialization for some > ARM boards. Sorry for the silence here, I noticed these were broken just before I went on holidays but didn't get a chance to fix anything. For the Xilinx case I was thinking of patching the machine code to sanely follow the -smp option. -smp 1 -> Only create 1 A53 -smp 4 -> Create 4 A53s -smp 6 -> Create all the CPUs I see a lot of advantages in not forcing the smallest number of CPUs to be 4 unless we really have to. I do see a nice advantage in being able to set the default smp option to something not 1 so the default closely matches hardware, but users can override that if they want to. So for the patch below I like the default_cpus option, but for Xilinx at least I would like to patch the logic to follow the -smp option instead of force a minimum. Thanks, Alistair > > Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 > Reported-by: Thomas Huth <thuth@redhat.com> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > hw/arm/exynos4_boards.c | 12 ++++-------- > hw/arm/raspi.c | 2 ++ > hw/arm/xlnx-zcu102.c | 9 ++++++++- > include/hw/boards.h | 10 ++++++++++ > vl.c | 38 +++++++++++++++++++++++++++++++++++--- > 5 files changed, 59 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c > index f1441ec..750162c 100644 > --- a/hw/arm/exynos4_boards.c > +++ b/hw/arm/exynos4_boards.c > @@ -27,7 +27,6 @@ > #include "qemu-common.h" > #include "cpu.h" > #include "sysemu/sysemu.h" > -#include "sysemu/qtest.h" > #include "hw/sysbus.h" > #include "net/net.h" > #include "hw/arm/arm.h" > @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, > Exynos4BoardType board_type) > { > Exynos4BoardState *s = g_new(Exynos4BoardState, 1); > - MachineClass *mc = MACHINE_GET_CLASS(machine); > - > - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { > - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" > - " value", > - mc->name, EXYNOS4210_NCPUS); > - } > > exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; > exynos4_board_binfo.board_id = exynos4_board_id[board_type]; > @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung NURI board (Exynos4210)"; > mc->init = nuri_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung SMDKC210 board (Exynos4210)"; > mc->init = smdkc210_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index 5941c9f..cd5fa8c 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) > mc->no_floppy = 1; > mc->no_cdrom = 1; > mc->max_cpus = BCM2836_NCPUS; > + mc->min_cpus = BCM2836_NCPUS; > + mc->default_cpus = BCM2836_NCPUS; > mc->default_ram_size = 1024 * 1024 * 1024; > mc->ignore_memory_transaction_failures = true; > }; > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > index e2d15a1..0f1837e 100644 > --- a/hw/arm/xlnx-zcu102.c > +++ b/hw/arm/xlnx-zcu102.c > @@ -185,6 +185,10 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_ep108_machine_init_typeinfo = { > @@ -240,7 +244,10 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > - mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 191a5b3..be582dd 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -102,6 +102,13 @@ typedef struct { > > /** > * MachineClass: > + * @max_cpus: maximum number of CPUs supported. Default: 1 > + * @max_additional_cpus: # of CPUs that can be instantiated in addition to those > + * instantiated with -smp. For instance, if a machine has 4 main CPUs and 2 > + * optional CPUs (e.g. instantiated via -global overrides), then @max_cpus > + * and @max_additional_cpus will be set to 4 and 2, respectively. Default: 0 > + * @min_cpus: minimum number of CPUs supported. Default: 1 > + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > * @get_hotplug_handler: this function is called during bus-less > * device hotplug. If defined it returns pointer to an instance > * of HotplugHandler object, which handles hotplug operation > @@ -167,6 +174,9 @@ struct MachineClass { > BlockInterfaceType block_default_type; > int units_per_default_bus; > int max_cpus; > + int max_additional_cpus; > + int min_cpus; > + int default_cpus; > unsigned int no_serial:1, > no_parallel:1, > use_virtcon:1, > diff --git a/vl.c b/vl.c > index ec29909..e3ee36e 100644 > --- a/vl.c > +++ b/vl.c > @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; > Chardev *sclp_hds[MAX_SCLP_CONSOLES]; > int win2k_install_hack = 0; > int singlestep = 0; > -int smp_cpus = 1; > -unsigned int max_cpus = 1; > +int smp_cpus; > +unsigned int max_cpus; > int smp_cores = 1; > int smp_threads = 1; > int acpi_enabled = 1; > @@ -4330,12 +4330,44 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > + machine_class->min_cpus = machine_class->min_cpus ?: 1; > + machine_class->default_cpus = machine_class->default_cpus ?: 1; > + > + /* if -smp is not set, default to mc->default_cpus */ > + if (!smp_cpus) { > + smp_cpus = machine_class->default_cpus; > + max_cpus = machine_class->default_cpus; > + } > + > + /* sanity-check smp_cpus and max_cpus */ > + if (smp_cpus < machine_class->min_cpus) { > + error_report("Invalid SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", smp_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > if (max_cpus > machine_class->max_cpus) { > - error_report("Invalid SMP CPUs %d. The max CPUs " > + error_report("Invalid max SMP CPUs %d. The max CPUs " > "supported by machine '%s' is %d", max_cpus, > machine_class->name, machine_class->max_cpus); > exit(1); > } > + if (max_cpus < machine_class->min_cpus) { > + error_report("Invalid max SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", max_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > + > + /* > + * Some boards can instantiate additional CPUs, e.g. by overriding > + * device params via -global arguments. Update max_cpus so that > + * we are sure to have an upper bound on the maximum number of > + * vCPUs that might be instantiated. > + */ > + if (machine_class->max_additional_cpus) { > + max_cpus += machine_class->max_additional_cpus; > + } > > /* > * Get the default machine options from the machine if it is not already > -- > 2.7.4 > >
On Mon, Nov 06, 2017 at 14:32:35 -0800, Alistair Francis wrote: > Sorry for the silence here, I noticed these were broken just before I > went on holidays but didn't get a chance to fix anything. > > For the Xilinx case I was thinking of patching the machine code to > sanely follow the -smp option. > > -smp 1 -> Only create 1 A53 > -smp 4 -> Create 4 A53s > -smp 6 -> Create all the CPUs > > I see a lot of advantages in not forcing the smallest number of CPUs > to be 4 unless we really have to. > > I do see a nice advantage in being able to set the default smp option > to something not 1 so the default closely matches hardware, but users > can override that if they want to. > > So for the patch below I like the default_cpus option, but for Xilinx > at least I would like to patch the logic to follow the -smp option > instead of force a minimum. Agreed, honouring -smp would be the right fix. Just note that since this is a regression we need the fix to be in for 2.11. I just took a look at the non-Xilinx boards. It seems simple enough to substitute the hard-coded value for smp_cpus, but yet again I see "Property" structs that I'm not sure what to do with. For instance, bcm2836.c:152: static Property bcm2836_props[] = { DEFINE_PROP_UINT32("enabled-cpus", BCM2836State, enabled_cpus, BCM2836_NCPUS), DEFINE_PROP_END_OF_LIST() }; What is the purpose here? To enable/disable CPUs with -global args, just like it's done for the Xilinx boards? Shouldn't we just use -smp for that? Also, note that I don't have a way to test these boards, which explains why I'm reluctant to change board code. But of course if board maintainers step in, I'm all for it :-) Thanks, Emilio
On Mon, Nov 6, 2017 at 3:21 PM, Emilio G. Cota <cota@braap.org> wrote: > On Mon, Nov 06, 2017 at 14:32:35 -0800, Alistair Francis wrote: >> Sorry for the silence here, I noticed these were broken just before I >> went on holidays but didn't get a chance to fix anything. >> >> For the Xilinx case I was thinking of patching the machine code to >> sanely follow the -smp option. >> >> -smp 1 -> Only create 1 A53 >> -smp 4 -> Create 4 A53s >> -smp 6 -> Create all the CPUs >> >> I see a lot of advantages in not forcing the smallest number of CPUs >> to be 4 unless we really have to. >> >> I do see a nice advantage in being able to set the default smp option >> to something not 1 so the default closely matches hardware, but users >> can override that if they want to. >> >> So for the patch below I like the default_cpus option, but for Xilinx >> at least I would like to patch the logic to follow the -smp option >> instead of force a minimum. > > Agreed, honouring -smp would be the right fix. > Just note that since this is a regression we need the fix to > be in for 2.11. Ok, I can spin up a patch for the Xilinx boards in the next day (maybe 2) > > I just took a look at the non-Xilinx boards. It seems simple enough to > substitute the hard-coded value for smp_cpus, but yet again > I see "Property" structs that I'm not sure what to do with. > For instance, bcm2836.c:152: > > static Property bcm2836_props[] = { > DEFINE_PROP_UINT32("enabled-cpus", BCM2836State, enabled_cpus, BCM2836_NCPUS), > DEFINE_PROP_END_OF_LIST() > }; > > What is the purpose here? To enable/disable CPUs with -global args, > just like it's done for the Xilinx boards? Shouldn't we just use > -smp for that? Hmm... > > Also, note that I don't have a way to test these boards, which > explains why I'm reluctant to change board code. But of > course if board maintainers step in, I'm all for it :-) Yeah, I see. What about if we set default_cpus to the -smp option that is expected. Then on some of the older boards (like the bcm2836) we print a warning in the machine init() if the -smp option doesn't match that? That way the default args work, we allow users to specify a overwrite but we warn them that it might not work and it ensures all boards follow a similar flow. Then if we can test the boards and know that -smp 1 works we can remove the warning. Thanks, Alistair > > Thanks, > > Emilio >
On Mon, Nov 6, 2017 at 12:13 PM, Emilio G. Cota <cota@braap.org> wrote: > On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: >> IMO, initialization state doesn't belong to CPUClass. We already >> have a single accelerator object in MachineState::accelerator, >> and tcg_initialized could be moved to a AccelState::initialized >> field. > > I don't know how to cleanly get AccelState from a CPUClass pointer > (as I said I'm not familiar with object code / qom) -- suggestions > welcome! The best I could come up in the limited time I have for > this is to use a static bool, as shown below. > > > ---8<--- > > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global > > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24) > introduces a per-CPUClass bool that we check so that the target CPU > is initialized for TCG only once. This works well except when > we end up creating more than one CPUClass, in which case we end > up incorrectly initializing TCG more than once, i.e. once for > each CPUClass. > > This can be replicated with: > $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \ > -global driver=xlnx,,zynqmp,property=has_rpu,value=on > In this case the class name of the "RPUs" is prefixed by "cortex-r5-", > whereas the "regular" CPUs are prefixed by "cortex-a53-". This > results in two CPUClass instances being created. > > Fix it by introducing a static variable, so that only the first > target CPU being initialized will initialize the target-dependent > part of TCG, regardless of CPUClass instances. > > Fixes: 55c3ceef61fcf06fc98ddc752b7cce788ce7680b > Signed-off-by: Emilio G. Cota <cota@braap.org> This works great for me, is this ok to be applied? Thanks, Alistair > --- > exec.c | 5 +++-- > include/qom/cpu.h | 1 - > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/exec.c b/exec.c > index 97a24a8..8b579c0 100644 > --- a/exec.c > +++ b/exec.c > @@ -792,11 +792,12 @@ void cpu_exec_initfn(CPUState *cpu) > void cpu_exec_realizefn(CPUState *cpu, Error **errp) > { > CPUClass *cc = CPU_GET_CLASS(cpu); > + static bool tcg_target_initialized; > > cpu_list_add(cpu); > > - if (tcg_enabled() && !cc->tcg_initialized) { > - cc->tcg_initialized = true; > + if (tcg_enabled() && !tcg_target_initialized) { > + tcg_target_initialized = true; > cc->tcg_initialize(); > } > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index fa4b0c9..c2fa151 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -209,7 +209,6 @@ typedef struct CPUClass { > /* Keep non-pointer data at the end to minimize holes. */ > int gdb_num_core_regs; > bool gdb_stop_before_watchpoint; > - bool tcg_initialized; > } CPUClass; > > #ifdef HOST_WORDS_BIGENDIAN > -- > 2.7.4 > >
On 11/06/2017 08:33 PM, Alistair Francis wrote: > On Mon, Nov 6, 2017 at 3:21 PM, Emilio G. Cota <cota@braap.org> wrote: >> On Mon, Nov 06, 2017 at 14:32:35 -0800, Alistair Francis wrote: >>> Sorry for the silence here, I noticed these were broken just before I >>> went on holidays but didn't get a chance to fix anything. >>> >>> For the Xilinx case I was thinking of patching the machine code to >>> sanely follow the -smp option. >>> >>> -smp 1 -> Only create 1 A53 >>> -smp 4 -> Create 4 A53s >>> -smp 6 -> Create all the CPUs >>> >>> I see a lot of advantages in not forcing the smallest number of CPUs >>> to be 4 unless we really have to. >>> >>> I do see a nice advantage in being able to set the default smp option >>> to something not 1 so the default closely matches hardware, but users >>> can override that if they want to. >>> >>> So for the patch below I like the default_cpus option, but for Xilinx >>> at least I would like to patch the logic to follow the -smp option >>> instead of force a minimum. >> >> Agreed, honouring -smp would be the right fix. >> Just note that since this is a regression we need the fix to >> be in for 2.11. Can we revert some patches to avoid the 2.11 regression and take time to see how to fix this correctly instead? The -smp help is: -smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,sockets=sockets] How would we start a Cortex-R52? default would be: -smp cores=4,lockstep so we can disable DCLS with: -smp cores=8 having Lockstep not being 2N cores but aliased as N, the only interest being injecting fault (or actually 2 cores, but 1 stopped, the guest not aware of that). The ZynqMP indeed has 6 CPUs, why not add apu/rpu options for ARM? 4+2=6 cores: -smp apus=4,rpus=2 4+1=5 cores -smp apus=4,rpus=2:lockstep There will always be 6 cores to this ZynqMP, why want to have less than 4 APUs? I'd rather have 4 APUs, all of them can be offlined / hotplugged, but they need to be instantiated and machine-initialized. > Ok, I can spin up a patch for the Xilinx boards in the next day (maybe 2) > >> >> I just took a look at the non-Xilinx boards. It seems simple enough to >> substitute the hard-coded value for smp_cpus, but yet again >> I see "Property" structs that I'm not sure what to do with. >> For instance, bcm2836.c:152: >> >> static Property bcm2836_props[] = { >> DEFINE_PROP_UINT32("enabled-cpus", BCM2836State, enabled_cpus, BCM2836_NCPUS), >> DEFINE_PROP_END_OF_LIST() >> }; >> >> What is the purpose here? To enable/disable CPUs with -global args, >> just like it's done for the Xilinx boards? Shouldn't we just use >> -smp for that? > > Hmm... > >> >> Also, note that I don't have a way to test these boards, which >> explains why I'm reluctant to change board code. But of >> course if board maintainers step in, I'm all for it :-) > > Yeah, I see. > > What about if we set default_cpus to the -smp option that is expected. > Then on some of the older boards (like the bcm2836) we print a warning > in the machine init() if the -smp option doesn't match that? > > That way the default args work, we allow users to specify a overwrite > but we warn them that it might not work and it ensures all boards > follow a similar flow. > > Then if we can test the boards and know that -smp 1 works we can > remove the warning. > > Thanks, > Alistair > >> >> Thanks, >> >> Emilio >> >
On Mon, Nov 6, 2017 at 4:54 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 11/06/2017 08:33 PM, Alistair Francis wrote: >> On Mon, Nov 6, 2017 at 3:21 PM, Emilio G. Cota <cota@braap.org> wrote: >>> On Mon, Nov 06, 2017 at 14:32:35 -0800, Alistair Francis wrote: >>>> Sorry for the silence here, I noticed these were broken just before I >>>> went on holidays but didn't get a chance to fix anything. >>>> >>>> For the Xilinx case I was thinking of patching the machine code to >>>> sanely follow the -smp option. >>>> >>>> -smp 1 -> Only create 1 A53 >>>> -smp 4 -> Create 4 A53s >>>> -smp 6 -> Create all the CPUs >>>> >>>> I see a lot of advantages in not forcing the smallest number of CPUs >>>> to be 4 unless we really have to. >>>> >>>> I do see a nice advantage in being able to set the default smp option >>>> to something not 1 so the default closely matches hardware, but users >>>> can override that if they want to. >>>> >>>> So for the patch below I like the default_cpus option, but for Xilinx >>>> at least I would like to patch the logic to follow the -smp option >>>> instead of force a minimum. >>> >>> Agreed, honouring -smp would be the right fix. >>> Just note that since this is a regression we need the fix to >>> be in for 2.11. > > Can we revert some patches to avoid the 2.11 regression and take time to > see how to fix this correctly instead? > > The -smp help is: > > -smp > [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,sockets=sockets] > > How would we start a Cortex-R52? I don't think it's unreasonable for the order to be A53s then R5s. So once you have all 4 A53s any other CPUs you add to the -smp option add the R5s. > > default would be: -smp cores=4,lockstep > so we can disable DCLS with: -smp cores=8 > > having Lockstep not being 2N cores but aliased as N, the only interest > being injecting fault (or actually 2 cores, but 1 stopped, the guest not > aware of that). We never model lockstep in QEMU, even in the Xilinx fork. We just disable one of the CPUs for lockstep (and fuz some of the other logic). > > The ZynqMP indeed has 6 CPUs, why not add apu/rpu options for ARM? > > 4+2=6 cores: > -smp apus=4,rpus=2 > > 4+1=5 cores > -smp apus=4,rpus=2:lockstep This results in a different command line interface for this machine, which I think is confusing and unnecessary. > > There will always be 6 cores to this ZynqMP, why want to have less than > 4 APUs? I'd rather have 4 APUs, all of them can be offlined / > hotplugged, but they need to be instantiated and machine-initialized. There are lots of reasons to not have all 6 CPUs. First of all adding the R5s creates issues (GDB debugging being the worst offender) that most of the time people don't want when they aren't using the R5s. Secondly there are a lot of uses where one a single CPU is running. We for example have baremetal test binaries that only run on a single CPU. We can spin up a large number of QEMU instances to multi-thread running different tests. We don't need the over head of 6 CPUs just to run a single A53 test. It's much easier and quicker for us to run a single CPU instance of QEMU. Thanks, Alistair > >> Ok, I can spin up a patch for the Xilinx boards in the next day (maybe 2) >> >>> >>> I just took a look at the non-Xilinx boards. It seems simple enough to >>> substitute the hard-coded value for smp_cpus, but yet again >>> I see "Property" structs that I'm not sure what to do with. >>> For instance, bcm2836.c:152: >>> >>> static Property bcm2836_props[] = { >>> DEFINE_PROP_UINT32("enabled-cpus", BCM2836State, enabled_cpus, BCM2836_NCPUS), >>> DEFINE_PROP_END_OF_LIST() >>> }; >>> >>> What is the purpose here? To enable/disable CPUs with -global args, >>> just like it's done for the Xilinx boards? Shouldn't we just use >>> -smp for that? >> >> Hmm... >> >>> >>> Also, note that I don't have a way to test these boards, which >>> explains why I'm reluctant to change board code. But of >>> course if board maintainers step in, I'm all for it :-) >> >> Yeah, I see. >> >> What about if we set default_cpus to the -smp option that is expected. >> Then on some of the older boards (like the bcm2836) we print a warning >> in the machine init() if the -smp option doesn't match that? >> >> That way the default args work, we allow users to specify a overwrite >> but we warn them that it might not work and it ensures all boards >> follow a similar flow. >> >> Then if we can test the boards and know that -smp 1 works we can >> remove the warning. >> >> Thanks, >> Alistair >> >>> >>> Thanks, >>> >>> Emilio >>> >> >
On Mon, Nov 06, 2017 at 04:43:34PM -0800, Alistair Francis wrote: > On Mon, Nov 6, 2017 at 12:13 PM, Emilio G. Cota <cota@braap.org> wrote: > > On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: > >> IMO, initialization state doesn't belong to CPUClass. We already > >> have a single accelerator object in MachineState::accelerator, > >> and tcg_initialized could be moved to a AccelState::initialized > >> field. > > > > I don't know how to cleanly get AccelState from a CPUClass pointer > > (as I said I'm not familiar with object code / qom) -- suggestions > > welcome! The best I could come up in the limited time I have for > > this is to use a static bool, as shown below. I don't believe a TYPE_ACCEL object is created in *-user. We can consider changing that, but not during 2.11 freeze. > > > > > > ---8<--- > > > > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global > > > > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24) > > introduces a per-CPUClass bool that we check so that the target CPU > > is initialized for TCG only once. This works well except when > > we end up creating more than one CPUClass, in which case we end > > up incorrectly initializing TCG more than once, i.e. once for > > each CPUClass. > > > > This can be replicated with: > > $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \ > > -global driver=xlnx,,zynqmp,property=has_rpu,value=on > > In this case the class name of the "RPUs" is prefixed by "cortex-r5-", > > whereas the "regular" CPUs are prefixed by "cortex-a53-". This > > results in two CPUClass instances being created. > > > > Fix it by introducing a static variable, so that only the first > > target CPU being initialized will initialize the target-dependent > > part of TCG, regardless of CPUClass instances. > > > > Fixes: 55c3ceef61fcf06fc98ddc752b7cce788ce7680b > > Signed-off-by: Emilio G. Cota <cota@braap.org> > > This works great for me, is this ok to be applied? It depends if we are already relying on initialization of CPUs of different types to be calling two different ->tcg_initialize() functions. I think that's the case today, so this looks reasonable as a quick fix for 2.11. I would wait for an Acked-by from Richard Henderson before applying it, though. Richard, what do you think? > > Thanks, > Alistair > > > --- > > exec.c | 5 +++-- > > include/qom/cpu.h | 1 - > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 97a24a8..8b579c0 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -792,11 +792,12 @@ void cpu_exec_initfn(CPUState *cpu) > > void cpu_exec_realizefn(CPUState *cpu, Error **errp) > > { > > CPUClass *cc = CPU_GET_CLASS(cpu); > > + static bool tcg_target_initialized; > > > > cpu_list_add(cpu); > > > > - if (tcg_enabled() && !cc->tcg_initialized) { > > - cc->tcg_initialized = true; > > + if (tcg_enabled() && !tcg_target_initialized) { > > + tcg_target_initialized = true; > > cc->tcg_initialize(); > > } > > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > index fa4b0c9..c2fa151 100644 > > --- a/include/qom/cpu.h > > +++ b/include/qom/cpu.h > > @@ -209,7 +209,6 @@ typedef struct CPUClass { > > /* Keep non-pointer data at the end to minimize holes. */ > > int gdb_num_core_regs; > > bool gdb_stop_before_watchpoint; > > - bool tcg_initialized; > > } CPUClass; > > > > #ifdef HOST_WORDS_BIGENDIAN > > -- > > 2.7.4 > > > >
Hi Emilio, On 11/06/2017 06:54 PM, Emilio G. Cota wrote: > On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: >> On Fri, Nov 03, 2017 at 06:24:07PM -0400, Emilio G. Cota wrote: >>> /** >>> * MachineClass: >>> * @max_cpus: maximum number of CPUs supported. Default: 1 >>> + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus >>> * @min_cpus: minimum number of CPUs supported. Default: 1 >>> * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 >>> * @get_hotplug_handler: this function is called during bus-less >> >> If we have a field containing the default value for smp_cpus >> (default_cpus), what about a default_max_cpus field for the >> default value of max_cpus? This could make the rules a bit >> easier to follow. >> >> (But I wonder if there's a way we can do this without introducing >> another MachineClass field and making the initialization code >> more complex.) > > I don't think the rules are too hard to follow, since they're trivial > for most machines because the defaults are reasonable (i.e. they > only set max_cpus, and everything else is set to 1). Some machines > though are tricky in what can and cannot support, e.g. the Xilinx > ones we've been dealing with in this thread. > > wrt .default_max_cpus instead of .force_max_cpus: it lets users > shoot themselves in the foot, so I wouldn't go this route. > For instance, what do we do if the user passes maxcpus < default_max_cpus? > max_cpus would be set to maxcpus, yet they could still pass the > -global override that instantiates two extra CPUs, and at that point > we'd fail (in TCG) without providing a meaningful error. > > We could increase maxcpus to default_max_cpus, but at that point > default_max_cpus becomes the same thing as force_max_cpus (!). > > An alternative I've come up with is to have a field that specifies how > many CPUs could be created in addition to those specified in max_cpus > (in the Xilinx case these would be the RPUs). Patch below. > >> The fact that MachineClass::max_cpus and the 'max_cpus' globals >> have completely different meanings makes this confusing enough. >> Maybe we should rename the 'max_cpus' global variable to >> 'max_hotplug_cpus'. > > I have no trouble with their different meanings, although I admit > I am now quite well acquainted with this code so my bias is strong :-) > > Thanks, > > Emilio > > ---8<--- > > Subject: [PATCH] hw: add .{additional_max,min,default}_cpus fields to > machine_class > > max_cpus needs to be an upper bound on the number of vCPUs > initialized; otherwise TCG region initialization breaks. > > Some boards initialize a hard-coded number of vCPUs, which is not > captured by the global max_cpus. Fix it by adding the .min_cpus > field to machine_class. > > A subset of those boards can also instantiate additional vCPUs > beyond what is specified in -smp, e.g. via -global overrides. To > ensure that max_cpus remains an upper bound on the number of vCPUs > that can be instantiated, we also add the .additional_max_cpus field > to machine_class. > > This commit also changes some user-facing behaviour: we now die if > -smp is below this hard-coded vCPU minimum instead of silently > ignoring the passed -smp value (sometimes announcing this by printing > a warning). However, the introduction of .default_cpus lessens the > likelihood that users will notice this: if -smp isn't set, we now > assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, > if a user does not set -smp, they always get a correct number of vCPUs. > > This change fixes 3468b59 ("tcg: enable multiple TCG contexts in > softmmu", 2017-10-24), which broke TCG initialization for some > ARM boards. > > Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 > Reported-by: Thomas Huth <thuth@redhat.com> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > hw/arm/exynos4_boards.c | 12 ++++-------- > hw/arm/raspi.c | 2 ++ > hw/arm/xlnx-zcu102.c | 9 ++++++++- > include/hw/boards.h | 10 ++++++++++ > vl.c | 38 +++++++++++++++++++++++++++++++++++--- > 5 files changed, 59 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c > index f1441ec..750162c 100644 > --- a/hw/arm/exynos4_boards.c > +++ b/hw/arm/exynos4_boards.c > @@ -27,7 +27,6 @@ > #include "qemu-common.h" > #include "cpu.h" > #include "sysemu/sysemu.h" > -#include "sysemu/qtest.h" > #include "hw/sysbus.h" > #include "net/net.h" > #include "hw/arm/arm.h" > @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, > Exynos4BoardType board_type) > { > Exynos4BoardState *s = g_new(Exynos4BoardState, 1); > - MachineClass *mc = MACHINE_GET_CLASS(machine); > - > - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { > - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" > - " value", > - mc->name, EXYNOS4210_NCPUS); > - } > > exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; > exynos4_board_binfo.board_id = exynos4_board_id[board_type]; > @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung NURI board (Exynos4210)"; > mc->init = nuri_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung SMDKC210 board (Exynos4210)"; > mc->init = smdkc210_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index 5941c9f..cd5fa8c 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) > mc->no_floppy = 1; > mc->no_cdrom = 1; > mc->max_cpus = BCM2836_NCPUS; > + mc->min_cpus = BCM2836_NCPUS; > + mc->default_cpus = BCM2836_NCPUS; > mc->default_ram_size = 1024 * 1024 * 1024; > mc->ignore_memory_transaction_failures = true; > }; > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > index e2d15a1..0f1837e 100644 > --- a/hw/arm/xlnx-zcu102.c > +++ b/hw/arm/xlnx-zcu102.c > @@ -185,6 +185,10 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_ep108_machine_init_typeinfo = { > @@ -240,7 +244,10 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > - mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 191a5b3..be582dd 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -102,6 +102,13 @@ typedef struct { > > /** > * MachineClass: > + * @max_cpus: maximum number of CPUs supported. Default: 1 > + * @max_additional_cpus: # of CPUs that can be instantiated in addition to those > + * instantiated with -smp. For instance, if a machine has 4 main CPUs and 2 > + * optional CPUs (e.g. instantiated via -global overrides), then @max_cpus > + * and @max_additional_cpus will be set to 4 and 2, respectively. Default: 0 > + * @min_cpus: minimum number of CPUs supported. Default: 1 > + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > * @get_hotplug_handler: this function is called during bus-less > * device hotplug. If defined it returns pointer to an instance > * of HotplugHandler object, which handles hotplug operation > @@ -167,6 +174,9 @@ struct MachineClass { > BlockInterfaceType block_default_type; > int units_per_default_bus; > int max_cpus; > + int max_additional_cpus; > + int min_cpus; > + int default_cpus; > unsigned int no_serial:1, > no_parallel:1, > use_virtcon:1, > diff --git a/vl.c b/vl.c > index ec29909..e3ee36e 100644 > --- a/vl.c > +++ b/vl.c > @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; > Chardev *sclp_hds[MAX_SCLP_CONSOLES]; > int win2k_install_hack = 0; > int singlestep = 0; > -int smp_cpus = 1; > -unsigned int max_cpus = 1; > +int smp_cpus; > +unsigned int max_cpus; > int smp_cores = 1; > int smp_threads = 1; > int acpi_enabled = 1; > @@ -4330,12 +4330,44 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > + machine_class->min_cpus = machine_class->min_cpus ?: 1; > + machine_class->default_cpus = machine_class->default_cpus ?: 1; > + > + /* if -smp is not set, default to mc->default_cpus */ > + if (!smp_cpus) { > + smp_cpus = machine_class->default_cpus; > + max_cpus = machine_class->default_cpus; > + } > + > + /* sanity-check smp_cpus and max_cpus */ > + if (smp_cpus < machine_class->min_cpus) { > + error_report("Invalid SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", smp_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > if (max_cpus > machine_class->max_cpus) { > - error_report("Invalid SMP CPUs %d. The max CPUs " > + error_report("Invalid max SMP CPUs %d. The max CPUs " > "supported by machine '%s' is %d", max_cpus, > machine_class->name, machine_class->max_cpus); > exit(1); > } > + if (max_cpus < machine_class->min_cpus) { > + error_report("Invalid max SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", max_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > + > + /* > + * Some boards can instantiate additional CPUs, e.g. by overriding > + * device params via -global arguments. Update max_cpus so that > + * we are sure to have an upper bound on the maximum number of > + * vCPUs that might be instantiated. > + */ > + if (machine_class->max_additional_cpus) { > + max_cpus += machine_class->max_additional_cpus; > + } This does fix 3468b59, so: Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> I'd rather think of another way, and wait for Richard review, but for the time frame left for next release: Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > /* > * Get the default machine options from the machine if it is not already >
On Mon, Nov 06, 2017 at 04:54:54PM -0500, Emilio G. Cota wrote: > On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: > > On Fri, Nov 03, 2017 at 06:24:07PM -0400, Emilio G. Cota wrote: > > > /** > > > * MachineClass: > > > * @max_cpus: maximum number of CPUs supported. Default: 1 > > > + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus > > > * @min_cpus: minimum number of CPUs supported. Default: 1 > > > * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > > > * @get_hotplug_handler: this function is called during bus-less > > > > If we have a field containing the default value for smp_cpus > > (default_cpus), what about a default_max_cpus field for the > > default value of max_cpus? This could make the rules a bit > > easier to follow. > > > > (But I wonder if there's a way we can do this without introducing > > another MachineClass field and making the initialization code > > more complex.) > > I don't think the rules are too hard to follow, since they're trivial > for most machines because the defaults are reasonable (i.e. they > only set max_cpus, and everything else is set to 1). Some machines > though are tricky in what can and cannot support, e.g. the Xilinx > ones we've been dealing with in this thread. > > wrt .default_max_cpus instead of .force_max_cpus: it lets users > shoot themselves in the foot, so I wouldn't go this route. > For instance, what do we do if the user passes maxcpus < default_max_cpus? > max_cpus would be set to maxcpus, yet they could still pass the > -global override that instantiates two extra CPUs, and at that point > we'd fail (in TCG) without providing a meaningful error. > > We could increase maxcpus to default_max_cpus, but at that point > default_max_cpus becomes the same thing as force_max_cpus (!). > > An alternative I've come up with is to have a field that specifies how > many CPUs could be created in addition to those specified in max_cpus > (in the Xilinx case these would be the RPUs). Patch below. > > > The fact that MachineClass::max_cpus and the 'max_cpus' globals > > have completely different meanings makes this confusing enough. > > Maybe we should rename the 'max_cpus' global variable to > > 'max_hotplug_cpus'. > > I have no trouble with their different meanings, although I admit > I am now quite well acquainted with this code so my bias is strong :-) > > Thanks, > > Emilio > > ---8<--- > > Subject: [PATCH] hw: add .{additional_max,min,default}_cpus fields to > machine_class > > max_cpus needs to be an upper bound on the number of vCPUs > initialized; otherwise TCG region initialization breaks. > > Some boards initialize a hard-coded number of vCPUs, which is not > captured by the global max_cpus. Fix it by adding the .min_cpus > field to machine_class. > > A subset of those boards can also instantiate additional vCPUs > beyond what is specified in -smp, e.g. via -global overrides. To > ensure that max_cpus remains an upper bound on the number of vCPUs > that can be instantiated, we also add the .additional_max_cpus field > to machine_class. Setting max_cpus > smp_cpus is supposed to be useful only for CPU hotplug, and this doesn't seem to be the case here. I would prefer to make the CPU creation code not care about max_cpus at all unless it is dealing with CPU hotplug, but this would require auditing existing code that could be allocating data structures based on max_cpus. In either case, it looks like the series sent by Alistair: Subject: [PATCH v1 0/2] Xilinx ZCU102 fixes for 2.11 makes max_additional_cpus unnecessary, doesn't it? > > This commit also changes some user-facing behaviour: we now die if > -smp is below this hard-coded vCPU minimum instead of silently > ignoring the passed -smp value (sometimes announcing this by printing > a warning). However, the introduction of .default_cpus lessens the > likelihood that users will notice this: if -smp isn't set, we now > assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, > if a user does not set -smp, they always get a correct number of vCPUs. > > This change fixes 3468b59 ("tcg: enable multiple TCG contexts in > softmmu", 2017-10-24), which broke TCG initialization for some > ARM boards. > > Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 > Reported-by: Thomas Huth <thuth@redhat.com> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > hw/arm/exynos4_boards.c | 12 ++++-------- > hw/arm/raspi.c | 2 ++ > hw/arm/xlnx-zcu102.c | 9 ++++++++- > include/hw/boards.h | 10 ++++++++++ > vl.c | 38 +++++++++++++++++++++++++++++++++++--- > 5 files changed, 59 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c > index f1441ec..750162c 100644 > --- a/hw/arm/exynos4_boards.c > +++ b/hw/arm/exynos4_boards.c > @@ -27,7 +27,6 @@ > #include "qemu-common.h" > #include "cpu.h" > #include "sysemu/sysemu.h" > -#include "sysemu/qtest.h" > #include "hw/sysbus.h" > #include "net/net.h" > #include "hw/arm/arm.h" > @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, > Exynos4BoardType board_type) > { > Exynos4BoardState *s = g_new(Exynos4BoardState, 1); > - MachineClass *mc = MACHINE_GET_CLASS(machine); > - > - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { > - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" > - " value", > - mc->name, EXYNOS4210_NCPUS); > - } > > exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; > exynos4_board_binfo.board_id = exynos4_board_id[board_type]; > @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung NURI board (Exynos4210)"; > mc->init = nuri_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung SMDKC210 board (Exynos4210)"; > mc->init = smdkc210_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index 5941c9f..cd5fa8c 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) > mc->no_floppy = 1; > mc->no_cdrom = 1; > mc->max_cpus = BCM2836_NCPUS; > + mc->min_cpus = BCM2836_NCPUS; > + mc->default_cpus = BCM2836_NCPUS; > mc->default_ram_size = 1024 * 1024 * 1024; > mc->ignore_memory_transaction_failures = true; > }; > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > index e2d15a1..0f1837e 100644 > --- a/hw/arm/xlnx-zcu102.c > +++ b/hw/arm/xlnx-zcu102.c > @@ -185,6 +185,10 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_ep108_machine_init_typeinfo = { > @@ -240,7 +244,10 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > - mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 191a5b3..be582dd 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -102,6 +102,13 @@ typedef struct { > > /** > * MachineClass: > + * @max_cpus: maximum number of CPUs supported. Default: 1 > + * @max_additional_cpus: # of CPUs that can be instantiated in addition to those > + * instantiated with -smp. For instance, if a machine has 4 main CPUs and 2 > + * optional CPUs (e.g. instantiated via -global overrides), then @max_cpus > + * and @max_additional_cpus will be set to 4 and 2, respectively. Default: 0 > + * @min_cpus: minimum number of CPUs supported. Default: 1 > + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > * @get_hotplug_handler: this function is called during bus-less > * device hotplug. If defined it returns pointer to an instance > * of HotplugHandler object, which handles hotplug operation > @@ -167,6 +174,9 @@ struct MachineClass { > BlockInterfaceType block_default_type; > int units_per_default_bus; > int max_cpus; > + int max_additional_cpus; > + int min_cpus; > + int default_cpus; > unsigned int no_serial:1, > no_parallel:1, > use_virtcon:1, > diff --git a/vl.c b/vl.c > index ec29909..e3ee36e 100644 > --- a/vl.c > +++ b/vl.c > @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; > Chardev *sclp_hds[MAX_SCLP_CONSOLES]; > int win2k_install_hack = 0; > int singlestep = 0; > -int smp_cpus = 1; > -unsigned int max_cpus = 1; > +int smp_cpus; > +unsigned int max_cpus; > int smp_cores = 1; > int smp_threads = 1; > int acpi_enabled = 1; > @@ -4330,12 +4330,44 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > + machine_class->min_cpus = machine_class->min_cpus ?: 1; > + machine_class->default_cpus = machine_class->default_cpus ?: 1; > + > + /* if -smp is not set, default to mc->default_cpus */ > + if (!smp_cpus) { > + smp_cpus = machine_class->default_cpus; > + max_cpus = machine_class->default_cpus; > + } > + > + /* sanity-check smp_cpus and max_cpus */ > + if (smp_cpus < machine_class->min_cpus) { > + error_report("Invalid SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", smp_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > if (max_cpus > machine_class->max_cpus) { > - error_report("Invalid SMP CPUs %d. The max CPUs " > + error_report("Invalid max SMP CPUs %d. The max CPUs " > "supported by machine '%s' is %d", max_cpus, > machine_class->name, machine_class->max_cpus); > exit(1); > } > + if (max_cpus < machine_class->min_cpus) { > + error_report("Invalid max SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", max_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > + > + /* > + * Some boards can instantiate additional CPUs, e.g. by overriding > + * device params via -global arguments. Update max_cpus so that > + * we are sure to have an upper bound on the maximum number of > + * vCPUs that might be instantiated. > + */ > + if (machine_class->max_additional_cpus) { > + max_cpus += machine_class->max_additional_cpus; > + } > > /* > * Get the default machine options from the machine if it is not already > -- > 2.7.4 >
On Tue, Nov 7, 2017 at 11:32 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Nov 06, 2017 at 04:54:54PM -0500, Emilio G. Cota wrote: >> On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: >> > On Fri, Nov 03, 2017 at 06:24:07PM -0400, Emilio G. Cota wrote: >> > > /** >> > > * MachineClass: >> > > * @max_cpus: maximum number of CPUs supported. Default: 1 >> > > + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus >> > > * @min_cpus: minimum number of CPUs supported. Default: 1 >> > > * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 >> > > * @get_hotplug_handler: this function is called during bus-less >> > >> > If we have a field containing the default value for smp_cpus >> > (default_cpus), what about a default_max_cpus field for the >> > default value of max_cpus? This could make the rules a bit >> > easier to follow. >> > >> > (But I wonder if there's a way we can do this without introducing >> > another MachineClass field and making the initialization code >> > more complex.) >> >> I don't think the rules are too hard to follow, since they're trivial >> for most machines because the defaults are reasonable (i.e. they >> only set max_cpus, and everything else is set to 1). Some machines >> though are tricky in what can and cannot support, e.g. the Xilinx >> ones we've been dealing with in this thread. >> >> wrt .default_max_cpus instead of .force_max_cpus: it lets users >> shoot themselves in the foot, so I wouldn't go this route. >> For instance, what do we do if the user passes maxcpus < default_max_cpus? >> max_cpus would be set to maxcpus, yet they could still pass the >> -global override that instantiates two extra CPUs, and at that point >> we'd fail (in TCG) without providing a meaningful error. >> >> We could increase maxcpus to default_max_cpus, but at that point >> default_max_cpus becomes the same thing as force_max_cpus (!). >> >> An alternative I've come up with is to have a field that specifies how >> many CPUs could be created in addition to those specified in max_cpus >> (in the Xilinx case these would be the RPUs). Patch below. >> >> > The fact that MachineClass::max_cpus and the 'max_cpus' globals >> > have completely different meanings makes this confusing enough. >> > Maybe we should rename the 'max_cpus' global variable to >> > 'max_hotplug_cpus'. >> >> I have no trouble with their different meanings, although I admit >> I am now quite well acquainted with this code so my bias is strong :-) >> >> Thanks, >> >> Emilio >> >> ---8<--- >> >> Subject: [PATCH] hw: add .{additional_max,min,default}_cpus fields to >> machine_class >> >> max_cpus needs to be an upper bound on the number of vCPUs >> initialized; otherwise TCG region initialization breaks. >> >> Some boards initialize a hard-coded number of vCPUs, which is not >> captured by the global max_cpus. Fix it by adding the .min_cpus >> field to machine_class. >> >> A subset of those boards can also instantiate additional vCPUs >> beyond what is specified in -smp, e.g. via -global overrides. To >> ensure that max_cpus remains an upper bound on the number of vCPUs >> that can be instantiated, we also add the .additional_max_cpus field >> to machine_class. > > Setting max_cpus > smp_cpus is supposed to be useful only for CPU > hotplug, and this doesn't seem to be the case here. I would > prefer to make the CPU creation code not care about max_cpus at > all unless it is dealing with CPU hotplug, but this would require > auditing existing code that could be allocating data structures > based on max_cpus. > > In either case, it looks like the series sent by Alistair: > Subject: [PATCH v1 0/2] Xilinx ZCU102 fixes for 2.11 > makes max_additional_cpus unnecessary, doesn't it? Yes, it doesn't need the max_additional_cpus at all. I would still like to see the default_cpus in, as I think that is really useful. Otherwise 2.10 has 4 A53s and 2.11 has 1 A53 by default which I think is confusing. Thanks, Alistair > > >> >> This commit also changes some user-facing behaviour: we now die if >> -smp is below this hard-coded vCPU minimum instead of silently >> ignoring the passed -smp value (sometimes announcing this by printing >> a warning). However, the introduction of .default_cpus lessens the >> likelihood that users will notice this: if -smp isn't set, we now >> assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, >> if a user does not set -smp, they always get a correct number of vCPUs. >> >> This change fixes 3468b59 ("tcg: enable multiple TCG contexts in >> softmmu", 2017-10-24), which broke TCG initialization for some >> ARM boards. >> >> Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 >> Reported-by: Thomas Huth <thuth@redhat.com> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Emilio G. Cota <cota@braap.org> >> --- >> hw/arm/exynos4_boards.c | 12 ++++-------- >> hw/arm/raspi.c | 2 ++ >> hw/arm/xlnx-zcu102.c | 9 ++++++++- >> include/hw/boards.h | 10 ++++++++++ >> vl.c | 38 +++++++++++++++++++++++++++++++++++--- >> 5 files changed, 59 insertions(+), 12 deletions(-) >> >> diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c >> index f1441ec..750162c 100644 >> --- a/hw/arm/exynos4_boards.c >> +++ b/hw/arm/exynos4_boards.c >> @@ -27,7 +27,6 @@ >> #include "qemu-common.h" >> #include "cpu.h" >> #include "sysemu/sysemu.h" >> -#include "sysemu/qtest.h" >> #include "hw/sysbus.h" >> #include "net/net.h" >> #include "hw/arm/arm.h" >> @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, >> Exynos4BoardType board_type) >> { >> Exynos4BoardState *s = g_new(Exynos4BoardState, 1); >> - MachineClass *mc = MACHINE_GET_CLASS(machine); >> - >> - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { >> - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" >> - " value", >> - mc->name, EXYNOS4210_NCPUS); >> - } >> >> exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; >> exynos4_board_binfo.board_id = exynos4_board_id[board_type]; >> @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) >> mc->desc = "Samsung NURI board (Exynos4210)"; >> mc->init = nuri_init; >> mc->max_cpus = EXYNOS4210_NCPUS; >> + mc->min_cpus = EXYNOS4210_NCPUS; >> + mc->default_cpus = EXYNOS4210_NCPUS; >> mc->ignore_memory_transaction_failures = true; >> } >> >> @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) >> mc->desc = "Samsung SMDKC210 board (Exynos4210)"; >> mc->init = smdkc210_init; >> mc->max_cpus = EXYNOS4210_NCPUS; >> + mc->min_cpus = EXYNOS4210_NCPUS; >> + mc->default_cpus = EXYNOS4210_NCPUS; >> mc->ignore_memory_transaction_failures = true; >> } >> >> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >> index 5941c9f..cd5fa8c 100644 >> --- a/hw/arm/raspi.c >> +++ b/hw/arm/raspi.c >> @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) >> mc->no_floppy = 1; >> mc->no_cdrom = 1; >> mc->max_cpus = BCM2836_NCPUS; >> + mc->min_cpus = BCM2836_NCPUS; >> + mc->default_cpus = BCM2836_NCPUS; >> mc->default_ram_size = 1024 * 1024 * 1024; >> mc->ignore_memory_transaction_failures = true; >> }; >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c >> index e2d15a1..0f1837e 100644 >> --- a/hw/arm/xlnx-zcu102.c >> +++ b/hw/arm/xlnx-zcu102.c >> @@ -185,6 +185,10 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) >> mc->block_default_type = IF_IDE; >> mc->units_per_default_bus = 1; >> mc->ignore_memory_transaction_failures = true; >> + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; >> + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; >> + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; >> + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; >> } >> >> static const TypeInfo xlnx_ep108_machine_init_typeinfo = { >> @@ -240,7 +244,10 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) >> mc->block_default_type = IF_IDE; >> mc->units_per_default_bus = 1; >> mc->ignore_memory_transaction_failures = true; >> - mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; >> + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; >> + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; >> + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; >> + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; >> } >> >> static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 191a5b3..be582dd 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -102,6 +102,13 @@ typedef struct { >> >> /** >> * MachineClass: >> + * @max_cpus: maximum number of CPUs supported. Default: 1 >> + * @max_additional_cpus: # of CPUs that can be instantiated in addition to those >> + * instantiated with -smp. For instance, if a machine has 4 main CPUs and 2 >> + * optional CPUs (e.g. instantiated via -global overrides), then @max_cpus >> + * and @max_additional_cpus will be set to 4 and 2, respectively. Default: 0 >> + * @min_cpus: minimum number of CPUs supported. Default: 1 >> + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 >> * @get_hotplug_handler: this function is called during bus-less >> * device hotplug. If defined it returns pointer to an instance >> * of HotplugHandler object, which handles hotplug operation >> @@ -167,6 +174,9 @@ struct MachineClass { >> BlockInterfaceType block_default_type; >> int units_per_default_bus; >> int max_cpus; >> + int max_additional_cpus; >> + int min_cpus; >> + int default_cpus; >> unsigned int no_serial:1, >> no_parallel:1, >> use_virtcon:1, >> diff --git a/vl.c b/vl.c >> index ec29909..e3ee36e 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; >> Chardev *sclp_hds[MAX_SCLP_CONSOLES]; >> int win2k_install_hack = 0; >> int singlestep = 0; >> -int smp_cpus = 1; >> -unsigned int max_cpus = 1; >> +int smp_cpus; >> +unsigned int max_cpus; >> int smp_cores = 1; >> int smp_threads = 1; >> int acpi_enabled = 1; >> @@ -4330,12 +4330,44 @@ int main(int argc, char **argv, char **envp) >> smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); >> >> machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ >> + machine_class->min_cpus = machine_class->min_cpus ?: 1; >> + machine_class->default_cpus = machine_class->default_cpus ?: 1; >> + >> + /* if -smp is not set, default to mc->default_cpus */ >> + if (!smp_cpus) { >> + smp_cpus = machine_class->default_cpus; >> + max_cpus = machine_class->default_cpus; >> + } >> + >> + /* sanity-check smp_cpus and max_cpus */ >> + if (smp_cpus < machine_class->min_cpus) { >> + error_report("Invalid SMP CPUs %d. The min CPUs " >> + "supported by machine '%s' is %d", smp_cpus, >> + machine_class->name, machine_class->min_cpus); >> + exit(1); >> + } >> if (max_cpus > machine_class->max_cpus) { >> - error_report("Invalid SMP CPUs %d. The max CPUs " >> + error_report("Invalid max SMP CPUs %d. The max CPUs " >> "supported by machine '%s' is %d", max_cpus, >> machine_class->name, machine_class->max_cpus); >> exit(1); >> } >> + if (max_cpus < machine_class->min_cpus) { >> + error_report("Invalid max SMP CPUs %d. The min CPUs " >> + "supported by machine '%s' is %d", max_cpus, >> + machine_class->name, machine_class->min_cpus); >> + exit(1); >> + } >> + >> + /* >> + * Some boards can instantiate additional CPUs, e.g. by overriding >> + * device params via -global arguments. Update max_cpus so that >> + * we are sure to have an upper bound on the maximum number of >> + * vCPUs that might be instantiated. >> + */ >> + if (machine_class->max_additional_cpus) { >> + max_cpus += machine_class->max_additional_cpus; >> + } >> >> /* >> * Get the default machine options from the machine if it is not already >> -- >> 2.7.4 >> > > -- > Eduardo >
On Tue, Nov 07, 2017 at 11:48:13AM -0800, Alistair Francis wrote: > On Tue, Nov 7, 2017 at 11:32 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Nov 06, 2017 at 04:54:54PM -0500, Emilio G. Cota wrote: > >> On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote: > >> > On Fri, Nov 03, 2017 at 06:24:07PM -0400, Emilio G. Cota wrote: > >> > > /** > >> > > * MachineClass: > >> > > * @max_cpus: maximum number of CPUs supported. Default: 1 > >> > > + * @force_max_cpus: if set, force the global max_cpus to match @max_cpus > >> > > * @min_cpus: minimum number of CPUs supported. Default: 1 > >> > > * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > >> > > * @get_hotplug_handler: this function is called during bus-less > >> > > >> > If we have a field containing the default value for smp_cpus > >> > (default_cpus), what about a default_max_cpus field for the > >> > default value of max_cpus? This could make the rules a bit > >> > easier to follow. > >> > > >> > (But I wonder if there's a way we can do this without introducing > >> > another MachineClass field and making the initialization code > >> > more complex.) > >> > >> I don't think the rules are too hard to follow, since they're trivial > >> for most machines because the defaults are reasonable (i.e. they > >> only set max_cpus, and everything else is set to 1). Some machines > >> though are tricky in what can and cannot support, e.g. the Xilinx > >> ones we've been dealing with in this thread. > >> > >> wrt .default_max_cpus instead of .force_max_cpus: it lets users > >> shoot themselves in the foot, so I wouldn't go this route. > >> For instance, what do we do if the user passes maxcpus < default_max_cpus? > >> max_cpus would be set to maxcpus, yet they could still pass the > >> -global override that instantiates two extra CPUs, and at that point > >> we'd fail (in TCG) without providing a meaningful error. > >> > >> We could increase maxcpus to default_max_cpus, but at that point > >> default_max_cpus becomes the same thing as force_max_cpus (!). > >> > >> An alternative I've come up with is to have a field that specifies how > >> many CPUs could be created in addition to those specified in max_cpus > >> (in the Xilinx case these would be the RPUs). Patch below. > >> > >> > The fact that MachineClass::max_cpus and the 'max_cpus' globals > >> > have completely different meanings makes this confusing enough. > >> > Maybe we should rename the 'max_cpus' global variable to > >> > 'max_hotplug_cpus'. > >> > >> I have no trouble with their different meanings, although I admit > >> I am now quite well acquainted with this code so my bias is strong :-) > >> > >> Thanks, > >> > >> Emilio > >> > >> ---8<--- > >> > >> Subject: [PATCH] hw: add .{additional_max,min,default}_cpus fields to > >> machine_class > >> > >> max_cpus needs to be an upper bound on the number of vCPUs > >> initialized; otherwise TCG region initialization breaks. > >> > >> Some boards initialize a hard-coded number of vCPUs, which is not > >> captured by the global max_cpus. Fix it by adding the .min_cpus > >> field to machine_class. > >> > >> A subset of those boards can also instantiate additional vCPUs > >> beyond what is specified in -smp, e.g. via -global overrides. To > >> ensure that max_cpus remains an upper bound on the number of vCPUs > >> that can be instantiated, we also add the .additional_max_cpus field > >> to machine_class. > > > > Setting max_cpus > smp_cpus is supposed to be useful only for CPU > > hotplug, and this doesn't seem to be the case here. I would > > prefer to make the CPU creation code not care about max_cpus at > > all unless it is dealing with CPU hotplug, but this would require > > auditing existing code that could be allocating data structures > > based on max_cpus. > > > > In either case, it looks like the series sent by Alistair: > > Subject: [PATCH v1 0/2] Xilinx ZCU102 fixes for 2.11 > > makes max_additional_cpus unnecessary, doesn't it? > > Yes, it doesn't need the max_additional_cpus at all. > > I would still like to see the default_cpus in, as I think that is > really useful. Otherwise 2.10 has 4 A53s and 2.11 has 1 A53 by default > which I think is confusing. I agree with the addition of min_cpus and default_cpus. But I would like to avoid making xlnx-* the only exceptional cases where max_additional_cpus is required, so I like the approach in your series. > > Thanks, > Alistair > > > > > > >> > >> This commit also changes some user-facing behaviour: we now die if > >> -smp is below this hard-coded vCPU minimum instead of silently > >> ignoring the passed -smp value (sometimes announcing this by printing > >> a warning). However, the introduction of .default_cpus lessens the > >> likelihood that users will notice this: if -smp isn't set, we now > >> assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, > >> if a user does not set -smp, they always get a correct number of vCPUs. > >> > >> This change fixes 3468b59 ("tcg: enable multiple TCG contexts in > >> softmmu", 2017-10-24), which broke TCG initialization for some > >> ARM boards. > >> > >> Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 > >> Reported-by: Thomas Huth <thuth@redhat.com> > >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> > >> Signed-off-by: Emilio G. Cota <cota@braap.org> > >> --- > >> hw/arm/exynos4_boards.c | 12 ++++-------- > >> hw/arm/raspi.c | 2 ++ > >> hw/arm/xlnx-zcu102.c | 9 ++++++++- > >> include/hw/boards.h | 10 ++++++++++ > >> vl.c | 38 +++++++++++++++++++++++++++++++++++--- > >> 5 files changed, 59 insertions(+), 12 deletions(-) > >> > >> diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c > >> index f1441ec..750162c 100644 > >> --- a/hw/arm/exynos4_boards.c > >> +++ b/hw/arm/exynos4_boards.c > >> @@ -27,7 +27,6 @@ > >> #include "qemu-common.h" > >> #include "cpu.h" > >> #include "sysemu/sysemu.h" > >> -#include "sysemu/qtest.h" > >> #include "hw/sysbus.h" > >> #include "net/net.h" > >> #include "hw/arm/arm.h" > >> @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, > >> Exynos4BoardType board_type) > >> { > >> Exynos4BoardState *s = g_new(Exynos4BoardState, 1); > >> - MachineClass *mc = MACHINE_GET_CLASS(machine); > >> - > >> - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { > >> - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" > >> - " value", > >> - mc->name, EXYNOS4210_NCPUS); > >> - } > >> > >> exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; > >> exynos4_board_binfo.board_id = exynos4_board_id[board_type]; > >> @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) > >> mc->desc = "Samsung NURI board (Exynos4210)"; > >> mc->init = nuri_init; > >> mc->max_cpus = EXYNOS4210_NCPUS; > >> + mc->min_cpus = EXYNOS4210_NCPUS; > >> + mc->default_cpus = EXYNOS4210_NCPUS; > >> mc->ignore_memory_transaction_failures = true; > >> } > >> > >> @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) > >> mc->desc = "Samsung SMDKC210 board (Exynos4210)"; > >> mc->init = smdkc210_init; > >> mc->max_cpus = EXYNOS4210_NCPUS; > >> + mc->min_cpus = EXYNOS4210_NCPUS; > >> + mc->default_cpus = EXYNOS4210_NCPUS; > >> mc->ignore_memory_transaction_failures = true; > >> } > >> > >> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > >> index 5941c9f..cd5fa8c 100644 > >> --- a/hw/arm/raspi.c > >> +++ b/hw/arm/raspi.c > >> @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) > >> mc->no_floppy = 1; > >> mc->no_cdrom = 1; > >> mc->max_cpus = BCM2836_NCPUS; > >> + mc->min_cpus = BCM2836_NCPUS; > >> + mc->default_cpus = BCM2836_NCPUS; > >> mc->default_ram_size = 1024 * 1024 * 1024; > >> mc->ignore_memory_transaction_failures = true; > >> }; > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > >> index e2d15a1..0f1837e 100644 > >> --- a/hw/arm/xlnx-zcu102.c > >> +++ b/hw/arm/xlnx-zcu102.c > >> @@ -185,6 +185,10 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > >> mc->block_default_type = IF_IDE; > >> mc->units_per_default_bus = 1; > >> mc->ignore_memory_transaction_failures = true; > >> + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > >> + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > >> + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > >> + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > >> } > >> > >> static const TypeInfo xlnx_ep108_machine_init_typeinfo = { > >> @@ -240,7 +244,10 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > >> mc->block_default_type = IF_IDE; > >> mc->units_per_default_bus = 1; > >> mc->ignore_memory_transaction_failures = true; > >> - mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > >> + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > >> + mc->max_additional_cpus = XLNX_ZYNQMP_NUM_RPU_CPUS; > >> + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > >> + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > >> } > >> > >> static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { > >> diff --git a/include/hw/boards.h b/include/hw/boards.h > >> index 191a5b3..be582dd 100644 > >> --- a/include/hw/boards.h > >> +++ b/include/hw/boards.h > >> @@ -102,6 +102,13 @@ typedef struct { > >> > >> /** > >> * MachineClass: > >> + * @max_cpus: maximum number of CPUs supported. Default: 1 > >> + * @max_additional_cpus: # of CPUs that can be instantiated in addition to those > >> + * instantiated with -smp. For instance, if a machine has 4 main CPUs and 2 > >> + * optional CPUs (e.g. instantiated via -global overrides), then @max_cpus > >> + * and @max_additional_cpus will be set to 4 and 2, respectively. Default: 0 > >> + * @min_cpus: minimum number of CPUs supported. Default: 1 > >> + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > >> * @get_hotplug_handler: this function is called during bus-less > >> * device hotplug. If defined it returns pointer to an instance > >> * of HotplugHandler object, which handles hotplug operation > >> @@ -167,6 +174,9 @@ struct MachineClass { > >> BlockInterfaceType block_default_type; > >> int units_per_default_bus; > >> int max_cpus; > >> + int max_additional_cpus; > >> + int min_cpus; > >> + int default_cpus; > >> unsigned int no_serial:1, > >> no_parallel:1, > >> use_virtcon:1, > >> diff --git a/vl.c b/vl.c > >> index ec29909..e3ee36e 100644 > >> --- a/vl.c > >> +++ b/vl.c > >> @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; > >> Chardev *sclp_hds[MAX_SCLP_CONSOLES]; > >> int win2k_install_hack = 0; > >> int singlestep = 0; > >> -int smp_cpus = 1; > >> -unsigned int max_cpus = 1; > >> +int smp_cpus; > >> +unsigned int max_cpus; > >> int smp_cores = 1; > >> int smp_threads = 1; > >> int acpi_enabled = 1; > >> @@ -4330,12 +4330,44 @@ int main(int argc, char **argv, char **envp) > >> smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > >> > >> machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > >> + machine_class->min_cpus = machine_class->min_cpus ?: 1; > >> + machine_class->default_cpus = machine_class->default_cpus ?: 1; > >> + > >> + /* if -smp is not set, default to mc->default_cpus */ > >> + if (!smp_cpus) { > >> + smp_cpus = machine_class->default_cpus; > >> + max_cpus = machine_class->default_cpus; > >> + } > >> + > >> + /* sanity-check smp_cpus and max_cpus */ > >> + if (smp_cpus < machine_class->min_cpus) { > >> + error_report("Invalid SMP CPUs %d. The min CPUs " > >> + "supported by machine '%s' is %d", smp_cpus, > >> + machine_class->name, machine_class->min_cpus); > >> + exit(1); > >> + } > >> if (max_cpus > machine_class->max_cpus) { > >> - error_report("Invalid SMP CPUs %d. The max CPUs " > >> + error_report("Invalid max SMP CPUs %d. The max CPUs " > >> "supported by machine '%s' is %d", max_cpus, > >> machine_class->name, machine_class->max_cpus); > >> exit(1); > >> } > >> + if (max_cpus < machine_class->min_cpus) { > >> + error_report("Invalid max SMP CPUs %d. The min CPUs " > >> + "supported by machine '%s' is %d", max_cpus, > >> + machine_class->name, machine_class->min_cpus); > >> + exit(1); > >> + } > >> + > >> + /* > >> + * Some boards can instantiate additional CPUs, e.g. by overriding > >> + * device params via -global arguments. Update max_cpus so that > >> + * we are sure to have an upper bound on the maximum number of > >> + * vCPUs that might be instantiated. > >> + */ > >> + if (machine_class->max_additional_cpus) { > >> + max_cpus += machine_class->max_additional_cpus; > >> + } > >> > >> /* > >> * Get the default machine options from the machine if it is not already > >> -- > >> 2.7.4 > >> > > > > -- > > Eduardo > >
On Fri, Nov 03, 2017 at 02:47:33PM -0400, Emilio G. Cota wrote: > max_cpus needs to be an upper bound on the number of vCPUs > initialized; otherwise TCG region initialization breaks. > > Some boards initialize a hard-coded number of vCPUs, which is not > captured by the global max_cpus. Fix it by adding the .min_cpus > field to machine_class. > > This commit also changes some user-facing behaviour: we now die if > -smp is below this hard-coded vCPU minimum instead of silently > ignoring the passed -smp value (sometimes announcing this by printing > a warning). However, the introduction of .default_cpus lessens the > likelihood that users will notice this: if -smp isn't set, we now > assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, > if a user does not set -smp, they always get a correct number of vCPUs. > > This change fixes 3468b59 ("tcg: enable multiple TCG contexts in > softmmu", 2017-10-24), which broke TCG initialization for some > ARM boards. > > Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 > Reported-by: Thomas Huth <thuth@redhat.com> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > hw/arm/exynos4_boards.c | 12 ++++-------- > hw/arm/raspi.c | 2 ++ > hw/arm/xlnx-zcu102.c | 5 +++++ > include/hw/boards.h | 5 +++++ > vl.c | 28 +++++++++++++++++++++++++--- > 5 files changed, 41 insertions(+), 11 deletions(-) > > diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c > index f1441ec..750162c 100644 > --- a/hw/arm/exynos4_boards.c > +++ b/hw/arm/exynos4_boards.c > @@ -27,7 +27,6 @@ > #include "qemu-common.h" > #include "cpu.h" > #include "sysemu/sysemu.h" > -#include "sysemu/qtest.h" > #include "hw/sysbus.h" > #include "net/net.h" > #include "hw/arm/arm.h" > @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, > Exynos4BoardType board_type) > { > Exynos4BoardState *s = g_new(Exynos4BoardState, 1); > - MachineClass *mc = MACHINE_GET_CLASS(machine); > - > - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { > - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" > - " value", > - mc->name, EXYNOS4210_NCPUS); > - } > > exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; > exynos4_board_binfo.board_id = exynos4_board_id[board_type]; > @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung NURI board (Exynos4210)"; > mc->init = nuri_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) > mc->desc = "Samsung SMDKC210 board (Exynos4210)"; > mc->init = smdkc210_init; > mc->max_cpus = EXYNOS4210_NCPUS; > + mc->min_cpus = EXYNOS4210_NCPUS; > + mc->default_cpus = EXYNOS4210_NCPUS; > mc->ignore_memory_transaction_failures = true; > } > > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index 5941c9f..cd5fa8c 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) > mc->no_floppy = 1; > mc->no_cdrom = 1; > mc->max_cpus = BCM2836_NCPUS; > + mc->min_cpus = BCM2836_NCPUS; > + mc->default_cpus = BCM2836_NCPUS; > mc->default_ram_size = 1024 * 1024 * 1024; > mc->ignore_memory_transaction_failures = true; > }; > diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c > index e2d15a1..395d1b5 100644 > --- a/hw/arm/xlnx-zcu102.c > +++ b/hw/arm/xlnx-zcu102.c > @@ -185,6 +185,9 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_IDE; > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_ep108_machine_init_typeinfo = { > @@ -241,6 +244,8 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) > mc->units_per_default_bus = 1; > mc->ignore_memory_transaction_failures = true; > mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; > + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; > } > > static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 191a5b3..62f160e 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -102,6 +102,9 @@ typedef struct { > > /** > * MachineClass: > + * @max_cpus: maximum number of CPUs supported. Default: 1 > + * @min_cpus: minimum number of CPUs supported. Default: 1 > + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 > * @get_hotplug_handler: this function is called during bus-less > * device hotplug. If defined it returns pointer to an instance > * of HotplugHandler object, which handles hotplug operation > @@ -167,6 +170,8 @@ struct MachineClass { > BlockInterfaceType block_default_type; > int units_per_default_bus; > int max_cpus; > + int min_cpus; > + int default_cpus; > unsigned int no_serial:1, > no_parallel:1, > use_virtcon:1, > diff --git a/vl.c b/vl.c > index ec29909..3ca5ee8 100644 > --- a/vl.c > +++ b/vl.c > @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; > Chardev *sclp_hds[MAX_SCLP_CONSOLES]; > int win2k_install_hack = 0; > int singlestep = 0; > -int smp_cpus = 1; > -unsigned int max_cpus = 1; > +int smp_cpus; > +unsigned int max_cpus; > int smp_cores = 1; > int smp_threads = 1; > int acpi_enabled = 1; > @@ -4330,12 +4330,34 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > + machine_class->min_cpus = machine_class->min_cpus ?: 1; > + machine_class->default_cpus = machine_class->default_cpus ?: 1; > + > + /* if -smp is not set, default to mc->default_cpus */ > + if (!smp_cpus) { > + smp_cpus = machine_class->default_cpus; > + max_cpus = machine_class->default_cpus; > + } I suggest doing this before smp_parse(), so any validation of smp_cpus inside smp_parse will apply to the value we're setting here (e.g. the replay_add_blocker() call in smp_parse() will work). > + > + /* sanity-check smp_cpus and max_cpus */ > + if (smp_cpus < machine_class->min_cpus) { > + error_report("Invalid SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", smp_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > if (max_cpus > machine_class->max_cpus) { > - error_report("Invalid SMP CPUs %d. The max CPUs " > + error_report("Invalid max SMP CPUs %d. The max CPUs " > "supported by machine '%s' is %d", max_cpus, > machine_class->name, machine_class->max_cpus); > exit(1); > } > + if (max_cpus < machine_class->min_cpus) { smp_parse() already ensures max_cpus >= smp_cpus, and you are already checking if smp_cpus < machine_class->min_cpus above. Is it really possible to trigger this error message? Except for that, the patch looks good to me. > + error_report("Invalid max SMP CPUs %d. The min CPUs " > + "supported by machine '%s' is %d", max_cpus, > + machine_class->name, machine_class->min_cpus); > + exit(1); > + } > > /* > * Get the default machine options from the machine if it is not already > -- > 2.7.4 > >
On 11/06/2017 09:13 PM, Emilio G. Cota wrote: > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global > > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24) > introduces a per-CPUClass bool that we check so that the target CPU > is initialized for TCG only once. This works well except when > we end up creating more than one CPUClass, in which case we end > up incorrectly initializing TCG more than once, i.e. once for > each CPUClass. > > This can be replicated with: > $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \ > -global driver=xlnx,,zynqmp,property=has_rpu,value=on > In this case the class name of the "RPUs" is prefixed by "cortex-r5-", > whereas the "regular" CPUs are prefixed by "cortex-a53-". This > results in two CPUClass instances being created. > > Fix it by introducing a static variable, so that only the first > target CPU being initialized will initialize the target-dependent > part of TCG, regardless of CPUClass instances. Hah! So, I had been thinking of the xylinx ARM + Microblaze case, where we really do need two different initializations. I never imagined that two different ARM parts had different CPUClasses. So I guess it's my initial patch that unified this that's more buggy than not. r~
On Wed, Nov 08, 2017 at 10:29:43PM +0100, Richard Henderson wrote: > On 11/06/2017 09:13 PM, Emilio G. Cota wrote: > > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global > > > > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24) > > introduces a per-CPUClass bool that we check so that the target CPU > > is initialized for TCG only once. This works well except when > > we end up creating more than one CPUClass, in which case we end > > up incorrectly initializing TCG more than once, i.e. once for > > each CPUClass. > > > > This can be replicated with: > > $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \ > > -global driver=xlnx,,zynqmp,property=has_rpu,value=on > > In this case the class name of the "RPUs" is prefixed by "cortex-r5-", > > whereas the "regular" CPUs are prefixed by "cortex-a53-". This > > results in two CPUClass instances being created. > > > > Fix it by introducing a static variable, so that only the first > > target CPU being initialized will initialize the target-dependent > > part of TCG, regardless of CPUClass instances. > > Hah! > > So, I had been thinking of the xylinx ARM + Microblaze case, where we really do > need two different initializations. I never imagined that two different ARM > parts had different CPUClasses. Is xylinx ARM + Microblaze something that already works (and would be broken by this patch), or something planned for the future? > > So I guess it's my initial patch that unified this that's more buggy than not. We still have the option of reverting the original patch, but (if it doesn't break anything) this patch looks like a simpler fix for 2.11 than a full revert.
On Wed, Nov 8, 2017 at 1:52 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Nov 08, 2017 at 10:29:43PM +0100, Richard Henderson wrote: >> On 11/06/2017 09:13 PM, Emilio G. Cota wrote: >> > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global >> > >> > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24) >> > introduces a per-CPUClass bool that we check so that the target CPU >> > is initialized for TCG only once. This works well except when >> > we end up creating more than one CPUClass, in which case we end >> > up incorrectly initializing TCG more than once, i.e. once for >> > each CPUClass. >> > >> > This can be replicated with: >> > $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \ >> > -global driver=xlnx,,zynqmp,property=has_rpu,value=on >> > In this case the class name of the "RPUs" is prefixed by "cortex-r5-", >> > whereas the "regular" CPUs are prefixed by "cortex-a53-". This >> > results in two CPUClass instances being created. >> > >> > Fix it by introducing a static variable, so that only the first >> > target CPU being initialized will initialize the target-dependent >> > part of TCG, regardless of CPUClass instances. >> >> Hah! >> >> So, I had been thinking of the xylinx ARM + Microblaze case, where we really do >> need two different initializations. I never imagined that two different ARM >> parts had different CPUClasses. > > Is xylinx ARM + Microblaze something that already works (and > would be broken by this patch), or something planned for the > future? Something planned for the future, it has never worked. Alistair > >> >> So I guess it's my initial patch that unified this that's more buggy than not. > > We still have the option of reverting the original patch, but (if > it doesn't break anything) this patch looks like a simpler fix > for 2.11 than a full revert. > > -- > Eduardo >
On Tue, Nov 07, 2017 at 18:15:45 -0200, Eduardo Habkost wrote: > On Fri, Nov 03, 2017 at 02:47:33PM -0400, Emilio G. Cota wrote: > > @@ -4330,12 +4330,34 @@ int main(int argc, char **argv, char **envp) > > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > > + machine_class->min_cpus = machine_class->min_cpus ?: 1; > > + machine_class->default_cpus = machine_class->default_cpus ?: 1; > > + > > + /* if -smp is not set, default to mc->default_cpus */ > > + if (!smp_cpus) { > > + smp_cpus = machine_class->default_cpus; > > + max_cpus = machine_class->default_cpus; > > + } > > I suggest doing this before smp_parse(), so any validation of > smp_cpus inside smp_parse will apply to the value we're setting > here (e.g. the replay_add_blocker() call in smp_parse() will > work). (snip) > > + if (max_cpus < machine_class->min_cpus) { > > smp_parse() already ensures max_cpus >= smp_cpus, and you are > already checking if smp_cpus < machine_class->min_cpus above. Is > it really possible to trigger this error message? > > Except for that, the patch looks good to me. Both very good points! I've modified the patch accordingly. Thanks, Emilio
diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c index f1441ec..750162c 100644 --- a/hw/arm/exynos4_boards.c +++ b/hw/arm/exynos4_boards.c @@ -27,7 +27,6 @@ #include "qemu-common.h" #include "cpu.h" #include "sysemu/sysemu.h" -#include "sysemu/qtest.h" #include "hw/sysbus.h" #include "net/net.h" #include "hw/arm/arm.h" @@ -129,13 +128,6 @@ exynos4_boards_init_common(MachineState *machine, Exynos4BoardType board_type) { Exynos4BoardState *s = g_new(Exynos4BoardState, 1); - MachineClass *mc = MACHINE_GET_CLASS(machine); - - if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) { - error_report("%s board supports only %d CPU cores, ignoring smp_cpus" - " value", - mc->name, EXYNOS4210_NCPUS); - } exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type]; exynos4_board_binfo.board_id = exynos4_board_id[board_type]; @@ -189,6 +181,8 @@ static void nuri_class_init(ObjectClass *oc, void *data) mc->desc = "Samsung NURI board (Exynos4210)"; mc->init = nuri_init; mc->max_cpus = EXYNOS4210_NCPUS; + mc->min_cpus = EXYNOS4210_NCPUS; + mc->default_cpus = EXYNOS4210_NCPUS; mc->ignore_memory_transaction_failures = true; } @@ -205,6 +199,8 @@ static void smdkc210_class_init(ObjectClass *oc, void *data) mc->desc = "Samsung SMDKC210 board (Exynos4210)"; mc->init = smdkc210_init; mc->max_cpus = EXYNOS4210_NCPUS; + mc->min_cpus = EXYNOS4210_NCPUS; + mc->default_cpus = EXYNOS4210_NCPUS; mc->ignore_memory_transaction_failures = true; } diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index 5941c9f..cd5fa8c 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -167,6 +167,8 @@ static void raspi2_machine_init(MachineClass *mc) mc->no_floppy = 1; mc->no_cdrom = 1; mc->max_cpus = BCM2836_NCPUS; + mc->min_cpus = BCM2836_NCPUS; + mc->default_cpus = BCM2836_NCPUS; mc->default_ram_size = 1024 * 1024 * 1024; mc->ignore_memory_transaction_failures = true; }; diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c index e2d15a1..395d1b5 100644 --- a/hw/arm/xlnx-zcu102.c +++ b/hw/arm/xlnx-zcu102.c @@ -185,6 +185,9 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) mc->block_default_type = IF_IDE; mc->units_per_default_bus = 1; mc->ignore_memory_transaction_failures = true; + mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; } static const TypeInfo xlnx_ep108_machine_init_typeinfo = { @@ -241,6 +244,8 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) mc->units_per_default_bus = 1; mc->ignore_memory_transaction_failures = true; mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; + mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; } static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { diff --git a/include/hw/boards.h b/include/hw/boards.h index 191a5b3..62f160e 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -102,6 +102,9 @@ typedef struct { /** * MachineClass: + * @max_cpus: maximum number of CPUs supported. Default: 1 + * @min_cpus: minimum number of CPUs supported. Default: 1 + * @default_cpus: number of CPUs instantiated if none are specified. Default: 1 * @get_hotplug_handler: this function is called during bus-less * device hotplug. If defined it returns pointer to an instance * of HotplugHandler object, which handles hotplug operation @@ -167,6 +170,8 @@ struct MachineClass { BlockInterfaceType block_default_type; int units_per_default_bus; int max_cpus; + int min_cpus; + int default_cpus; unsigned int no_serial:1, no_parallel:1, use_virtcon:1, diff --git a/vl.c b/vl.c index ec29909..3ca5ee8 100644 --- a/vl.c +++ b/vl.c @@ -160,8 +160,8 @@ Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES]; Chardev *sclp_hds[MAX_SCLP_CONSOLES]; int win2k_install_hack = 0; int singlestep = 0; -int smp_cpus = 1; -unsigned int max_cpus = 1; +int smp_cpus; +unsigned int max_cpus; int smp_cores = 1; int smp_threads = 1; int acpi_enabled = 1; @@ -4330,12 +4330,34 @@ int main(int argc, char **argv, char **envp) smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ + machine_class->min_cpus = machine_class->min_cpus ?: 1; + machine_class->default_cpus = machine_class->default_cpus ?: 1; + + /* if -smp is not set, default to mc->default_cpus */ + if (!smp_cpus) { + smp_cpus = machine_class->default_cpus; + max_cpus = machine_class->default_cpus; + } + + /* sanity-check smp_cpus and max_cpus */ + if (smp_cpus < machine_class->min_cpus) { + error_report("Invalid SMP CPUs %d. The min CPUs " + "supported by machine '%s' is %d", smp_cpus, + machine_class->name, machine_class->min_cpus); + exit(1); + } if (max_cpus > machine_class->max_cpus) { - error_report("Invalid SMP CPUs %d. The max CPUs " + error_report("Invalid max SMP CPUs %d. The max CPUs " "supported by machine '%s' is %d", max_cpus, machine_class->name, machine_class->max_cpus); exit(1); } + if (max_cpus < machine_class->min_cpus) { + error_report("Invalid max SMP CPUs %d. The min CPUs " + "supported by machine '%s' is %d", max_cpus, + machine_class->name, machine_class->min_cpus); + exit(1); + } /* * Get the default machine options from the machine if it is not already
max_cpus needs to be an upper bound on the number of vCPUs initialized; otherwise TCG region initialization breaks. Some boards initialize a hard-coded number of vCPUs, which is not captured by the global max_cpus. Fix it by adding the .min_cpus field to machine_class. This commit also changes some user-facing behaviour: we now die if -smp is below this hard-coded vCPU minimum instead of silently ignoring the passed -smp value (sometimes announcing this by printing a warning). However, the introduction of .default_cpus lessens the likelihood that users will notice this: if -smp isn't set, we now assign the value in .default_cpus to both smp_cpus and max_cpus. IOW, if a user does not set -smp, they always get a correct number of vCPUs. This change fixes 3468b59 ("tcg: enable multiple TCG contexts in softmmu", 2017-10-24), which broke TCG initialization for some ARM boards. Fixes: 3468b59e18b179bc63c7ce934de912dfa9596122 Reported-by: Thomas Huth <thuth@redhat.com> Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Emilio G. Cota <cota@braap.org> --- hw/arm/exynos4_boards.c | 12 ++++-------- hw/arm/raspi.c | 2 ++ hw/arm/xlnx-zcu102.c | 5 +++++ include/hw/boards.h | 5 +++++ vl.c | 28 +++++++++++++++++++++++++--- 5 files changed, 41 insertions(+), 11 deletions(-)