* Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-06 21:54 ` Emilio G. Cota
@ 2017-11-06 22:32 ` Alistair Francis
2017-11-06 23:21 ` Emilio G. Cota
2017-11-07 16:15 ` Philippe Mathieu-Daudé
2017-11-07 19:32 ` [Qemu-devel] " Eduardo Habkost
2 siblings, 1 reply; 23+ messages in thread
From: Alistair Francis @ 2017-11-06 22:32 UTC (permalink / raw)
To: Emilio G. Cota
Cc: Eduardo Habkost, Peter Maydell, Thomas Huth, Igor Mitsyanko,
Richard Henderson, qemu-devel@nongnu.org Developers,
Alistair Francis, qemu-arm, Igor Mammedov, Marcel Apfelbaum,
Edgar E . Iglesias
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
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-06 22:32 ` Alistair Francis
@ 2017-11-06 23:21 ` Emilio G. Cota
2017-11-06 23:33 ` Alistair Francis
0 siblings, 1 reply; 23+ messages in thread
From: Emilio G. Cota @ 2017-11-06 23:21 UTC (permalink / raw)
To: Alistair Francis
Cc: Eduardo Habkost, Peter Maydell, Thomas Huth, Igor Mitsyanko,
Richard Henderson, qemu-devel@nongnu.org Developers, qemu-arm,
Igor Mammedov, Marcel Apfelbaum, Edgar E . Iglesias
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-06 23:21 ` Emilio G. Cota
@ 2017-11-06 23:33 ` Alistair Francis
2017-11-07 0:54 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
0 siblings, 1 reply; 23+ messages in thread
From: Alistair Francis @ 2017-11-06 23:33 UTC (permalink / raw)
To: Emilio G. Cota
Cc: Alistair Francis, Peter Maydell, Thomas Huth, Eduardo Habkost,
Igor Mitsyanko, Richard Henderson,
qemu-devel@nongnu.org Developers, qemu-arm, Edgar E . Iglesias,
Marcel Apfelbaum, Igor Mammedov
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-06 23:33 ` Alistair Francis
@ 2017-11-07 0:54 ` Philippe Mathieu-Daudé
2017-11-07 1:19 ` Alistair Francis
0 siblings, 1 reply; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-11-07 0:54 UTC (permalink / raw)
To: Alistair Francis, Emilio G. Cota, Peter Maydell, Eduardo Habkost,
Igor Mammedov
Cc: Thomas Huth, Igor Mitsyanko, Richard Henderson,
qemu-devel@nongnu.org Developers, qemu-arm, Marcel Apfelbaum
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
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-07 0:54 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
@ 2017-11-07 1:19 ` Alistair Francis
0 siblings, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2017-11-07 1:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Emilio G. Cota, Peter Maydell, Eduardo Habkost,
Igor Mammedov, Thomas Huth, Igor Mitsyanko, Richard Henderson,
qemu-devel@nongnu.org Developers, qemu-arm, Marcel Apfelbaum
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
>>>
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-06 21:54 ` Emilio G. Cota
2017-11-06 22:32 ` Alistair Francis
@ 2017-11-07 16:15 ` Philippe Mathieu-Daudé
2017-11-07 19:32 ` [Qemu-devel] " Eduardo Habkost
2 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-11-07 16:15 UTC (permalink / raw)
To: Emilio G. Cota, Eduardo Habkost, Richard Henderson
Cc: Peter Maydell, Thomas Huth, Igor Mitsyanko, qemu-devel,
Alistair Francis, qemu-arm, Igor Mammedov, Marcel Apfelbaum
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-06 21:54 ` Emilio G. Cota
2017-11-06 22:32 ` Alistair Francis
2017-11-07 16:15 ` Philippe Mathieu-Daudé
@ 2017-11-07 19:32 ` Eduardo Habkost
2017-11-07 19:48 ` Alistair Francis
2 siblings, 1 reply; 23+ messages in thread
From: Eduardo Habkost @ 2017-11-07 19:32 UTC (permalink / raw)
To: Emilio G. Cota
Cc: qemu-devel, Peter Maydell, Richard Henderson, Thomas Huth,
qemu-arm, Igor Mitsyanko, Alistair Francis, Edgar E . Iglesias,
Marcel Apfelbaum, Igor Mammedov
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
>
--
Eduardo
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-07 19:32 ` [Qemu-devel] " Eduardo Habkost
@ 2017-11-07 19:48 ` Alistair Francis
2017-11-07 19:54 ` Eduardo Habkost
0 siblings, 1 reply; 23+ messages in thread
From: Alistair Francis @ 2017-11-07 19:48 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Emilio G. Cota, Peter Maydell, Thomas Huth, Igor Mitsyanko,
Richard Henderson, qemu-devel@nongnu.org Developers,
Alistair Francis, qemu-arm, Igor Mammedov, Marcel Apfelbaum,
Edgar E . Iglesias
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class
2017-11-07 19:48 ` Alistair Francis
@ 2017-11-07 19:54 ` Eduardo Habkost
0 siblings, 0 replies; 23+ messages in thread
From: Eduardo Habkost @ 2017-11-07 19:54 UTC (permalink / raw)
To: Alistair Francis
Cc: Emilio G. Cota, Peter Maydell, Thomas Huth, Igor Mitsyanko,
Richard Henderson, qemu-devel@nongnu.org Developers, qemu-arm,
Igor Mammedov, Marcel Apfelbaum, Edgar E . Iglesias
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
> >
--
Eduardo
^ permalink raw reply [flat|nested] 23+ messages in thread