[Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Emilio G. Cota]

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;
+    }
+
+    /* 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
-- 
2.7.4

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Emilio G. Cota]

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

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost]

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)

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Emilio G. Cota]

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 "

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost]

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 "

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Emilio G. Cota]

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
-- 
2.7.4

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Emilio G. Cota]

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
-- 
2.7.4

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Alistair Francis]

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
>
>

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Emilio G. Cota]

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

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Alistair Francis]

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
>

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Alistair Francis]

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
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Philippe Mathieu-Daudé]

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
>>
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Alistair Francis]

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
>>>
>>
>

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost]

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
> >
> >

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Philippe Mathieu-Daudé]

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
> 

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Alistair Francis]

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
>

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost]

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
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Alistair Francis]

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
>

Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Emilio G. Cota]

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