[PATCH-for-9.0 00/11] hw/arm: Step toward building qemu-system-{arm, aarch64} altogether

[PATCH-for-9.0 00/11] hw/arm: Step toward building qemu-system-{arm, aarch64} altogether

[Philippe Mathieu-Daudé]

Hi,

This series is a step toward having a single qemu-system-aarch64
binary for both ARM and Aarch64 variants.

First we add the TypeInfo::can_register() handler to QOM, to be
able to decide at runtime if a type can be registered. We'll
later use the target_aarch64_available() method to restrict some
QOM types to the aarch64 build.

Then few cleanups allow us to build the Raspi machines and its
components as target-agnostic. To do that, instead of embedding
a CPUState in its SoC container, we use a pointer to it. Since
the type is forward-declared by "cpu-qom.h", we can use that in
our hw/ headers. Then the correct CPU is instanciated by calling
object_new() instead of object_initialize_child().

Finally objects are moved to meson system_ss[] source set to be
built once.

Does that look reasonable to keep merging TARGET_ARM/AARCH64?

Thanks,

Phil.

Philippe Mathieu-Daudé (11):
  qom: Introduce the TypeInfo::can_register() handler
  target/arm: Add target_aarch64_available() helper
  target/arm: Declare ARM_CPU_TYPE_NAME/SUFFIX in 'cpu-qom.h'
  target/arm: Move ARM_CPU_IRQ/FIQ definitions to 'cpu-qom.h'
  target/arm: Move GTIMER definitions to 'cpu-defs.h'
  hw/arm/bcm2836: Simplify use of 'reset-cbar' property
  hw/arm/bcm2836: Simplify access to 'start-powered-off' property
  hw/arm/bcm2836: Use ARM_CPU 'mp-affinity' property
  hw/arm/bcm2836: Allocate ARM CPU state with object_new()
  hw/arm/raspi: Build bcm2836.o and raspi.o objects once
  hw/intc/meson: Simplify how arm_gicv3_kvm.o objects are built

 include/hw/arm/bcm2836.h    |  4 ++--
 include/qom/object.h        |  4 ++++
 target/arm/cpu-defs.h       | 19 ++++++++++++++++
 target/arm/cpu-qom.h        | 11 ++++++++++
 target/arm/cpu.h            | 16 +-------------
 hw/arm/bcm2836.c            | 43 ++++++++++++++++---------------------
 hw/arm/raspi.c              |  8 +++----
 hw/intc/arm_gicv3_its_kvm.c |  1 +
 hw/intc/arm_gicv3_kvm.c     |  1 +
 qom/object.c                |  3 +++
 target/arm/cpu.c            |  9 ++++++++
 hw/arm/meson.build          |  6 ++++--
 hw/intc/meson.build         |  6 ++++--
 13 files changed, 80 insertions(+), 51 deletions(-)
 create mode 100644 target/arm/cpu-defs.h

-- 
2.41.0

[RFC PATCH-for-9.0 01/11] qom: Introduce the TypeInfo::can_register() handler

[Philippe Mathieu-Daudé]

Add a helper to decide at runtime whether a type can
be registered to the QOM framework or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qom/object.h | 4 ++++
 qom/object.c         | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index afccd24ca7..0d42fe17de 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -372,6 +372,8 @@ struct Object
  * struct TypeInfo:
  * @name: The name of the type.
  * @parent: The name of the parent type.
+ * @can_register: This optional function is called before a type is registered.
+ *   If it exists and returns false, the type is not registered.
  * @instance_size: The size of the object (derivative of #Object).  If
  *   @instance_size is 0, then the size of the object will be the size of the
  *   parent object.
@@ -414,6 +416,8 @@ struct TypeInfo
     const char *name;
     const char *parent;
 
+    bool (*can_register)(void);
+
     size_t instance_size;
     size_t instance_align;
     void (*instance_init)(Object *obj);
diff --git a/qom/object.c b/qom/object.c
index 95c0dc8285..f09b6b5a92 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -150,6 +150,9 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
 TypeImpl *type_register(const TypeInfo *info)
 {
     assert(info->parent);
+    if (info->can_register && !info->can_register()) {
+        return NULL;
+    }
     return type_register_internal(info);
 }
 
-- 
2.41.0

[RFC PATCH-for-9.0 02/11] target/arm: Add target_aarch64_available() helper

[Philippe Mathieu-Daudé]

We want to build HW models once, but don't want to
register types when all prerequisites are satisfied. Add
the target_aarch64_available() to know at runtime whether
TARGET_AARCH64 is built-in.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu-qom.h | 2 ++
 target/arm/cpu.c     | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
index 02b914c876..bf6b3604ed 100644
--- a/target/arm/cpu-qom.h
+++ b/target/arm/cpu-qom.h
@@ -33,4 +33,6 @@ typedef struct AArch64CPUClass AArch64CPUClass;
 DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
                        TYPE_AARCH64_CPU)
 
+bool target_aarch64_available(void);
+
 #endif
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 25e9d2ae7b..1990c04089 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2548,3 +2548,12 @@ static void arm_cpu_register_types(void)
 }
 
 type_init(arm_cpu_register_types)
+
+bool target_aarch64_available(void)
+{
+#ifdef TARGET_AARCH64
+    return true;
+#else
+    return false;
+#endif
+}
-- 
2.41.0

[PATCH-for-9.0 03/11] target/arm: Declare ARM_CPU_TYPE_NAME/SUFFIX in 'cpu-qom.h'

[Philippe Mathieu-Daudé]

Missed in commit 2d56be5a29 ("target: Declare
FOO_CPU_TYPE_NAME/SUFFIX in 'cpu-qom.h'"). See
it for more details.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu-qom.h | 3 +++
 target/arm/cpu.h     | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
index bf6b3604ed..be307037ff 100644
--- a/target/arm/cpu-qom.h
+++ b/target/arm/cpu-qom.h
@@ -33,6 +33,9 @@ typedef struct AArch64CPUClass AArch64CPUClass;
 DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
                        TYPE_AARCH64_CPU)
 
+#define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
+#define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
+
 bool target_aarch64_available(void);
 
 #endif
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a0282e0d28..d369275827 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2834,8 +2834,6 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync);
 #define ARM_CPUID_TI915T      0x54029152
 #define ARM_CPUID_TI925T      0x54029252
 
-#define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
-#define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
 #define CPU_RESOLVING_TYPE TYPE_ARM_CPU
 
 #define TYPE_ARM_HOST_CPU "host-" TYPE_ARM_CPU
-- 
2.41.0

[PATCH-for-9.0 04/11] target/arm: Move ARM_CPU_IRQ/FIQ definitions to 'cpu-qom.h'

[Philippe Mathieu-Daudé]

The ARM_CPU_IRQ/FIQ definitions are meant for the ARM CPU
QOM model. Move them to "cpu-qom.h" so any QOM code can
use them.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Or do these definitions belong to cpu-defs.h?
---
 target/arm/cpu-qom.h | 6 ++++++
 target/arm/cpu.h     | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
index be307037ff..38030450f7 100644
--- a/target/arm/cpu-qom.h
+++ b/target/arm/cpu-qom.h
@@ -36,6 +36,12 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
 #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
 #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
 
+/* Meanings of the ARMCPU object's four inbound GPIO lines */
+#define ARM_CPU_IRQ 0
+#define ARM_CPU_FIQ 1
+#define ARM_CPU_VIRQ 2
+#define ARM_CPU_VFIQ 3
+
 bool target_aarch64_available(void);
 
 #endif
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d369275827..124d829742 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -107,12 +107,6 @@ enum {
 #define offsetofhigh32(S, M) (offsetof(S, M) + sizeof(uint32_t))
 #endif
 
-/* Meanings of the ARMCPU object's four inbound GPIO lines */
-#define ARM_CPU_IRQ 0
-#define ARM_CPU_FIQ 1
-#define ARM_CPU_VIRQ 2
-#define ARM_CPU_VFIQ 3
-
 /* ARM-specific extra insn start words:
  * 1: Conditional execution bits
  * 2: Partial exception syndrome for data aborts
-- 
2.41.0

[PATCH-for-9.0 05/11] target/arm: Move GTIMER definitions to 'cpu-defs.h'

[Philippe Mathieu-Daudé]

To allow GTIMER_* definitions to be used by non-ARM specific
hardware models, move them to a new target agnostic "cpu-defs.h"
header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu-defs.h | 19 +++++++++++++++++++
 target/arm/cpu.h      |  8 +-------
 hw/arm/bcm2836.c      |  1 +
 3 files changed, 21 insertions(+), 7 deletions(-)
 create mode 100644 target/arm/cpu-defs.h

diff --git a/target/arm/cpu-defs.h b/target/arm/cpu-defs.h
new file mode 100644
index 0000000000..1ad76aff14
--- /dev/null
+++ b/target/arm/cpu-defs.h
@@ -0,0 +1,19 @@
+/*
+ * ARM "target agnostic" CPU definitions
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#ifndef ARM_CPU_DEFS_H
+#define ARM_CPU_DEFS_H
+
+#define GTIMER_PHYS     0
+#define GTIMER_VIRT     1
+#define GTIMER_HYP      2
+#define GTIMER_SEC      3
+#define GTIMER_HYPVIRT  4
+#define NUM_GTIMERS     5
+
+#endif
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 124d829742..8107e4d446 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -24,6 +24,7 @@
 #include "qemu/cpu-float.h"
 #include "hw/registerfields.h"
 #include "cpu-qom.h"
+#include "target/arm/cpu-defs.h"
 #include "exec/cpu-defs.h"
 #include "qapi/qapi-types-common.h"
 
@@ -154,13 +155,6 @@ typedef struct ARMGenericTimer {
     uint64_t ctl; /* Timer Control register */
 } ARMGenericTimer;
 
-#define GTIMER_PHYS     0
-#define GTIMER_VIRT     1
-#define GTIMER_HYP      2
-#define GTIMER_SEC      3
-#define GTIMER_HYPVIRT  4
-#define NUM_GTIMERS     5
-
 #define VTCR_NSW (1u << 29)
 #define VTCR_NSA (1u << 30)
 #define VSTCR_SW VTCR_NSW
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 166dc896c0..6986b71cb4 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -15,6 +15,7 @@
 #include "hw/arm/bcm2836.h"
 #include "hw/arm/raspi_platform.h"
 #include "hw/sysbus.h"
+#include "target/arm/cpu-defs.h"
 
 struct BCM283XClass {
     /*< private >*/
-- 
2.41.0

[PATCH-for-9.0 06/11] hw/arm/bcm2836: Simplify use of 'reset-cbar' property

[Philippe Mathieu-Daudé]

bcm2836_realize() is called by

 - bcm2836_class_init() which sets:

    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7")

 - bcm2837_class_init() which sets:

    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a53")

Both Cortex-A7 / A53 have the ARM_FEATURE_CBAR set. If it isn't,
then this is a programming error: use &error_abort.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/bcm2836.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 6986b71cb4..055c909e95 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -132,10 +132,8 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
         s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n;
 
         /* set periphbase/CBAR value for CPU-local registers */
-        if (!object_property_set_int(OBJECT(&s->cpu[n].core), "reset-cbar",
-                                     bc->peri_base, errp)) {
-            return;
-        }
+        object_property_set_int(OBJECT(&s->cpu[n].core), "reset-cbar",
+                                bc->peri_base, &error_abort);
 
         /* start powered off if not enabled */
         if (!object_property_set_bool(OBJECT(&s->cpu[n].core),
-- 
2.41.0

[PATCH-for-9.0 07/11] hw/arm/bcm2836: Simplify access to 'start-powered-off' property

[Philippe Mathieu-Daudé]

All ARM CPUs have the 'start-powered-off' property since commit
5de164304a ("arm: Allow secondary KVM CPUs to be booted via PSCI").

Note: since commit c1b701587e ("target/arm: Move start-powered-off
property to generic CPUState"), all CPUs for all targets have it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/bcm2836.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 055c909e95..198f9b5730 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -136,12 +136,8 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
                                 bc->peri_base, &error_abort);
 
         /* start powered off if not enabled */
-        if (!object_property_set_bool(OBJECT(&s->cpu[n].core),
-                                      "start-powered-off",
-                                      n >= s->enabled_cpus,
-                                      errp)) {
-            return;
-        }
+        object_property_set_bool(OBJECT(&s->cpu[n].core), "start-powered-off",
+                                 n >= s->enabled_cpus, &error_abort);
 
         if (!qdev_realize(DEVICE(&s->cpu[n].core), NULL, errp)) {
             return;
-- 
2.41.0

[PATCH-for-9.0 08/11] hw/arm/bcm2836: Use ARM_CPU 'mp-affinity' property

[Philippe Mathieu-Daudé]

The 'mp-affinity' property is present since commit 15a21fe028
("target-arm: Add mp-affinity property for ARM CPU class").
Use it and remove a /* TODO */ comment. Since all ARM CPUs
have this property, use &error_abort, because this call can
not fail.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/bcm2836.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 198f9b5730..8031a74600 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -128,8 +128,8 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
         qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0));
 
     for (n = 0; n < BCM283X_NCPUS; n++) {
-        /* TODO: this should be converted to a property of ARM_CPU */
-        s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n;
+        object_property_set_int(OBJECT(&s->cpu[n].core), "mp-affinity",
+                                (bc->clusterid << 8) | n, &error_abort);
 
         /* set periphbase/CBAR value for CPU-local registers */
         object_property_set_int(OBJECT(&s->cpu[n].core), "reset-cbar",
-- 
2.41.0

[RFC PATCH-for-9.0 09/11] hw/arm/bcm2836: Allocate ARM CPU state with object_new()

[Philippe Mathieu-Daudé]

The ARMCPU type is forward declared as a pointer to all hw/ files.
Its declaration is restricted to target/arm/ files. By using a
pointer in BCM283XState instead of embedding the whole CPU state,
we don't need to include "cpu.h" which is target-specific.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/arm/bcm2836.h |  4 ++--
 hw/arm/bcm2836.c         | 19 ++++++++++---------
 hw/arm/raspi.c           |  2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
index 6f90cabfa3..784bab0aad 100644
--- a/include/hw/arm/bcm2836.h
+++ b/include/hw/arm/bcm2836.h
@@ -14,7 +14,7 @@
 
 #include "hw/arm/bcm2835_peripherals.h"
 #include "hw/intc/bcm2836_control.h"
-#include "target/arm/cpu.h"
+#include "target/arm/cpu-qom.h"
 #include "qom/object.h"
 
 #define TYPE_BCM283X "bcm283x"
@@ -38,7 +38,7 @@ struct BCM283XState {
     uint32_t enabled_cpus;
 
     struct {
-        ARMCPU core;
+        ARMCPU *core;
     } cpu[BCM283X_NCPUS];
     BCM2836ControlState control;
     BCM2835PeripheralState peripherals;
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 8031a74600..4f5acee77e 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -39,8 +39,9 @@ static void bcm2836_init(Object *obj)
     int n;
 
     for (n = 0; n < bc->core_count; n++) {
-        object_initialize_child(obj, "cpu[*]", &s->cpu[n].core,
-                                bc->cpu_type);
+        s->cpu[n].core = ARM_CPU(object_new(bc->cpu_type));
+        object_property_add_child(obj, "cpu[*]", OBJECT(s->cpu[n].core));
+        qdev_realize_and_unref(DEVICE(s->cpu[n].core), NULL, &error_abort);
     }
     if (bc->core_count > 1) {
         qdev_property_add_static(DEVICE(obj), &bcm2836_enabled_cores_property);
@@ -139,24 +140,24 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
         object_property_set_bool(OBJECT(&s->cpu[n].core), "start-powered-off",
                                  n >= s->enabled_cpus, &error_abort);
 
-        if (!qdev_realize(DEVICE(&s->cpu[n].core), NULL, errp)) {
+        if (!qdev_realize(DEVICE(s->cpu[n].core), NULL, errp)) {
             return;
         }
 
         /* Connect irq/fiq outputs from the interrupt controller. */
         qdev_connect_gpio_out_named(DEVICE(&s->control), "irq", n,
-                qdev_get_gpio_in(DEVICE(&s->cpu[n].core), ARM_CPU_IRQ));
+                qdev_get_gpio_in(DEVICE(s->cpu[n].core), ARM_CPU_IRQ));
         qdev_connect_gpio_out_named(DEVICE(&s->control), "fiq", n,
-                qdev_get_gpio_in(DEVICE(&s->cpu[n].core), ARM_CPU_FIQ));
+                qdev_get_gpio_in(DEVICE(s->cpu[n].core), ARM_CPU_FIQ));
 
         /* Connect timers from the CPU to the interrupt controller */
-        qdev_connect_gpio_out(DEVICE(&s->cpu[n].core), GTIMER_PHYS,
+        qdev_connect_gpio_out(DEVICE(s->cpu[n].core), GTIMER_PHYS,
                 qdev_get_gpio_in_named(DEVICE(&s->control), "cntpnsirq", n));
-        qdev_connect_gpio_out(DEVICE(&s->cpu[n].core), GTIMER_VIRT,
+        qdev_connect_gpio_out(DEVICE(s->cpu[n].core), GTIMER_VIRT,
                 qdev_get_gpio_in_named(DEVICE(&s->control), "cntvirq", n));
-        qdev_connect_gpio_out(DEVICE(&s->cpu[n].core), GTIMER_HYP,
+        qdev_connect_gpio_out(DEVICE(s->cpu[n].core), GTIMER_HYP,
                 qdev_get_gpio_in_named(DEVICE(&s->control), "cnthpirq", n));
-        qdev_connect_gpio_out(DEVICE(&s->cpu[n].core), GTIMER_SEC,
+        qdev_connect_gpio_out(DEVICE(s->cpu[n].core), GTIMER_SEC,
                 qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq", n));
     }
 }
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index cc4c4ec9bf..01c391b90a 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -252,7 +252,7 @@ static void setup_boot(MachineState *machine, RaspiProcessorId processor_id,
         s->binfo.firmware_loaded = true;
     }
 
-    arm_load_kernel(&s->soc.cpu[0].core, machine, &s->binfo);
+    arm_load_kernel(s->soc.cpu[0].core, machine, &s->binfo);
 }
 
 static void raspi_machine_init(MachineState *machine)
-- 
2.41.0

[RFC PATCH-for-9.0 10/11] hw/arm/raspi: Build bcm2836.o and raspi.o objects once

[Philippe Mathieu-Daudé]

Use the target_aarch64_available() method to restrict
Aarch64 specific models. They will only be added at runtime
if TARGET_AARCH64 is built in.

The Raspberry Pi models can now be built once for all targets.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/bcm2836.c   | 5 +----
 hw/arm/raspi.c     | 6 ++----
 hw/arm/meson.build | 6 ++++--
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 4f5acee77e..ecf434c8ce 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -194,7 +194,6 @@ static void bcm2836_class_init(ObjectClass *oc, void *data)
     dc->realize = bcm2836_realize;
 };
 
-#ifdef TARGET_AARCH64
 static void bcm2837_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -207,7 +206,6 @@ static void bcm2837_class_init(ObjectClass *oc, void *data)
     bc->clusterid = 0x0;
     dc->realize = bcm2836_realize;
 };
-#endif
 
 static const TypeInfo bcm283x_types[] = {
     {
@@ -218,12 +216,11 @@ static const TypeInfo bcm283x_types[] = {
         .name           = TYPE_BCM2836,
         .parent         = TYPE_BCM283X,
         .class_init     = bcm2836_class_init,
-#ifdef TARGET_AARCH64
     }, {
         .name           = TYPE_BCM2837,
         .parent         = TYPE_BCM283X,
         .class_init     = bcm2837_class_init,
-#endif
+        .can_register   = target_aarch64_available,
     }, {
         .name           = TYPE_BCM283X,
         .parent         = TYPE_DEVICE,
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 01c391b90a..979937b9ac 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -349,7 +349,6 @@ static void raspi2b_machine_class_init(ObjectClass *oc, void *data)
     raspi_machine_class_common_init(mc, rmc->board_rev);
 };
 
-#ifdef TARGET_AARCH64
 static void raspi3ap_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -367,7 +366,6 @@ static void raspi3b_machine_class_init(ObjectClass *oc, void *data)
     rmc->board_rev = 0xa02082;
     raspi_machine_class_common_init(mc, rmc->board_rev);
 };
-#endif /* TARGET_AARCH64 */
 
 static const TypeInfo raspi_machine_types[] = {
     {
@@ -382,16 +380,16 @@ static const TypeInfo raspi_machine_types[] = {
         .name           = MACHINE_TYPE_NAME("raspi2b"),
         .parent         = TYPE_RASPI_MACHINE,
         .class_init     = raspi2b_machine_class_init,
-#ifdef TARGET_AARCH64
     }, {
         .name           = MACHINE_TYPE_NAME("raspi3ap"),
         .parent         = TYPE_RASPI_MACHINE,
         .class_init     = raspi3ap_machine_class_init,
+        .can_register   = target_aarch64_available,
     }, {
         .name           = MACHINE_TYPE_NAME("raspi3b"),
         .parent         = TYPE_RASPI_MACHINE,
         .class_init     = raspi3b_machine_class_init,
-#endif
+        .can_register   = target_aarch64_available,
     }, {
         .name           = TYPE_RASPI_MACHINE,
         .parent         = TYPE_MACHINE,
diff --git a/hw/arm/meson.build b/hw/arm/meson.build
index 68245d3ad1..15d60685d0 100644
--- a/hw/arm/meson.build
+++ b/hw/arm/meson.build
@@ -38,7 +38,6 @@ arm_ss.add(when: 'CONFIG_STRONGARM', if_true: files('strongarm.c'))
 arm_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('allwinner-a10.c', 'cubieboard.c'))
 arm_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true: files('allwinner-h3.c', 'orangepi.c'))
 arm_ss.add(when: 'CONFIG_ALLWINNER_R40', if_true: files('allwinner-r40.c', 'bananapi_m2u.c'))
-arm_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2836.c', 'raspi.c'))
 arm_ss.add(when: 'CONFIG_STM32F100_SOC', if_true: files('stm32f100_soc.c'))
 arm_ss.add(when: 'CONFIG_STM32F205_SOC', if_true: files('stm32f205_soc.c'))
 arm_ss.add(when: 'CONFIG_STM32F405_SOC', if_true: files('stm32f405_soc.c'))
@@ -68,7 +67,10 @@ arm_ss.add(when: 'CONFIG_XEN', if_true: files('xen_arm.c'))
 
 system_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmu-common.c'))
 system_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4_boards.c'))
-system_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_peripherals.c'))
+system_ss.add(when: 'CONFIG_RASPI', if_true: files(
+  'bcm2835_peripherals.c',
+  'bcm2836.c',
+  'raspi.c'))
 system_ss.add(when: 'CONFIG_TOSA', if_true: files('tosa.c'))
 
 hw_arch += {'arm': arm_ss}
-- 
2.41.0

[RFC PATCH-for-9.0 11/11] hw/intc/meson: Simplify how arm_gicv3_kvm.o objects are built

[Philippe Mathieu-Daudé]

Use the target_aarch64_available() to build the ARM_GIC_KVM types
regardless the ARM/AARCH64 targets are selected, but restrict its
registration to TARGET_AARCH64 presence at runtime.

This will help to have a single binary running both ARM/Aarch64.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/arm_gicv3_its_kvm.c | 1 +
 hw/intc/arm_gicv3_kvm.c     | 1 +
 hw/intc/meson.build         | 6 ++++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index f7df602cff..b3063c4cd7 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -261,6 +261,7 @@ static const TypeInfo kvm_arm_its_info = {
     .instance_size = sizeof(GICv3ITSState),
     .class_init = kvm_arm_its_class_init,
     .class_size = sizeof(KVMARMITSClass),
+    .can_register = target_aarch64_available,
 };
 
 static void kvm_arm_its_register_types(void)
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 77eb37e131..33321dee5d 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -909,6 +909,7 @@ static const TypeInfo kvm_arm_gicv3_info = {
     .instance_size = sizeof(GICv3State),
     .class_init = kvm_arm_gicv3_class_init,
     .class_size = sizeof(KVMARMGICv3Class),
+    .can_register = target_aarch64_available,
 };
 
 static void kvm_arm_gicv3_register_types(void)
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index ed355941d1..d45eb76f36 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -40,8 +40,10 @@ endif
 specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c'))
 specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c'))
 specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: files('arm_gicv3_cpuif.c'))
-specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files('arm_gic_kvm.c'))
-specific_ss.add(when: ['CONFIG_ARM_GIC_KVM', 'TARGET_AARCH64'], if_true: files('arm_gicv3_kvm.c', 'arm_gicv3_its_kvm.c'))
+specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files(
+  'arm_gic_kvm.c',
+  'arm_gicv3_kvm.c',
+  'arm_gicv3_its_kvm.c'))
 specific_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_nvic.c'))
 specific_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_irqmp.c'))
 specific_ss.add(when: 'CONFIG_IOAPIC', if_true: files('ioapic.c'))
-- 
2.41.0

Re: [RFC PATCH-for-9.0 02/11] target/arm: Add target_aarch64_available() helper

[Philippe Mathieu-Daudé]

On 22/11/23 19:30, Philippe Mathieu-Daudé wrote:
> We want to build HW models once, but don't want to
> register types when all prerequisites are satisfied. Add
> the target_aarch64_available() to know at runtime whether
> TARGET_AARCH64 is built-in.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu-qom.h | 2 ++
>   target/arm/cpu.c     | 9 +++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
> index 02b914c876..bf6b3604ed 100644
> --- a/target/arm/cpu-qom.h
> +++ b/target/arm/cpu-qom.h
> @@ -33,4 +33,6 @@ typedef struct AArch64CPUClass AArch64CPUClass;
>   DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
>                          TYPE_AARCH64_CPU)
>   
> +bool target_aarch64_available(void);
> +
>   #endif
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 25e9d2ae7b..1990c04089 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2548,3 +2548,12 @@ static void arm_cpu_register_types(void)
>   }
>   
>   type_init(arm_cpu_register_types)
> +
> +bool target_aarch64_available(void)
> +{
> +#ifdef TARGET_AARCH64
> +    return true;
> +#else
> +    return false;
> +#endif
> +}

I forgot to precise here, what was discussed during the previous 2
years. Eventually qemu-system-arm is absorbed by qemu-system-aarch64,
but to keep backward compatibility we add a new qemu-system-arm wrapper
which simply calls 'qemu-system-aarch64 --32bit-only' (or better named
option) forwarding the same command line. target_aarch64_available()
then becomes:

   bool target_aarch64_available(void)
   {
     return option_32bit_only == false;
   }

Re: [RFC PATCH-for-9.0 01/11] qom: Introduce the TypeInfo::can_register() handler

[Thomas Huth]

On 22/11/2023 19.30, Philippe Mathieu-Daudé wrote:
> Add a helper to decide at runtime whether a type can
> be registered to the QOM framework or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/qom/object.h | 4 ++++
>   qom/object.c         | 3 +++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index afccd24ca7..0d42fe17de 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -372,6 +372,8 @@ struct Object
>    * struct TypeInfo:
>    * @name: The name of the type.
>    * @parent: The name of the parent type.
> + * @can_register: This optional function is called before a type is registered.
> + *   If it exists and returns false, the type is not registered.

The second sentence is quite hard to parse, since it is not quite clear what 
"it" refers to (type or function) and what "registered" means in this 
context (you don't mention type_register() here).

Maybe rather something like:

If set, type_register() uses this function to decide whether the type can be 
registered or not.

?

>    * @instance_size: The size of the object (derivative of #Object).  If
>    *   @instance_size is 0, then the size of the object will be the size of the
>    *   parent object.
> @@ -414,6 +416,8 @@ struct TypeInfo
>       const char *name;
>       const char *parent;
>   
> +    bool (*can_register)(void);
> +
>       size_t instance_size;
>       size_t instance_align;
>       void (*instance_init)(Object *obj);
> diff --git a/qom/object.c b/qom/object.c
> index 95c0dc8285..f09b6b5a92 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -150,6 +150,9 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
>   TypeImpl *type_register(const TypeInfo *info)
>   {
>       assert(info->parent);
> +    if (info->can_register && !info->can_register()) {
> +        return NULL;
> +    }

I have to say that I don't like it too much, since you're trying to fix a 
problem here in common code that clearly belongs to the code in hw/arm/ instead.

What about dropping it, and changing your last patch to replace the 
DEFINE_TYPES(raspi_machine_types) in hw/arm/raspi.c with your own 
implementation of type_register_static_array() that checks the condition there?

  Thomas

Re: [RFC PATCH-for-9.0 01/11] qom: Introduce the TypeInfo::can_register() handler

[Philippe Mathieu-Daudé]

On 23/11/23 16:09, Thomas Huth wrote:
> On 22/11/2023 19.30, Philippe Mathieu-Daudé wrote:
>> Add a helper to decide at runtime whether a type can
>> be registered to the QOM framework or not.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/qom/object.h | 4 ++++
>>   qom/object.c         | 3 +++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index afccd24ca7..0d42fe17de 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -372,6 +372,8 @@ struct Object
>>    * struct TypeInfo:
>>    * @name: The name of the type.
>>    * @parent: The name of the parent type.
>> + * @can_register: This optional function is called before a type is 
>> registered.
>> + *   If it exists and returns false, the type is not registered.
> 
> The second sentence is quite hard to parse, since it is not quite clear 
> what "it" refers to (type or function) and what "registered" means in 
> this context (you don't mention type_register() here).
> 
> Maybe rather something like:
> 
> If set, type_register() uses this function to decide whether the type 
> can be registered or not.
> 
> ?
> 
>>    * @instance_size: The size of the object (derivative of #Object).  If
>>    *   @instance_size is 0, then the size of the object will be the 
>> size of the
>>    *   parent object.
>> @@ -414,6 +416,8 @@ struct TypeInfo
>>       const char *name;
>>       const char *parent;
>> +    bool (*can_register)(void);
>> +
>>       size_t instance_size;
>>       size_t instance_align;
>>       void (*instance_init)(Object *obj);
>> diff --git a/qom/object.c b/qom/object.c
>> index 95c0dc8285..f09b6b5a92 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -150,6 +150,9 @@ static TypeImpl *type_register_internal(const 
>> TypeInfo *info)
>>   TypeImpl *type_register(const TypeInfo *info)
>>   {
>>       assert(info->parent);
>> +    if (info->can_register && !info->can_register()) {
>> +        return NULL;
>> +    }
> 
> I have to say that I don't like it too much, since you're trying to fix 
> a problem here in common code that clearly belongs to the code in 
> hw/arm/ instead.
> 
> What about dropping it, and changing your last patch to replace the 
> DEFINE_TYPES(raspi_machine_types) in hw/arm/raspi.c with your own 
> implementation of type_register_static_array() that checks the condition 
> there?

This isn't ARM specific, it happens I started to unify ARM/aarch64
binaries.

Types can be registered depending on build-time (config/host specific)
definitions and runtime ones. How can we check for runtime if not via
this simple helper?

Still ARM, but as example what I have then is (module meson):

-- >8 --
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8f033f59..e7f566f05d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1102,6 +1102,7 @@ typedef struct ARMCPUInfo {
      const char *name;
      void (*initfn)(Object *obj);
      void (*class_init)(ObjectClass *oc, void *data);
+    bool (*can_register)(void);
  } ARMCPUInfo;

  /**
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 558a85e6d7..109fb160b5 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2512,6 +2512,7 @@ void arm_cpu_register(const ARMCPUInfo *info)
          .instance_init = arm_cpu_instance_init,
          .class_init = info->class_init ?: cpu_register_class_init,
          .class_data = (void *)info,
+        .can_register = info->can_register,
      };

      type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 1e9c6c85ae..c3b7e5666c 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -744,7 +744,8 @@ static void aarch64_max_initfn(Object *obj)
  static const ARMCPUInfo aarch64_cpus[] = {
      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
-    { .name = "max",                .initfn = aarch64_max_initfn },
+    { .name = "max",                .initfn = aarch64_max_initfn,
+                                    .can_register = 
target_aarch64_available },
  #if defined(CONFIG_KVM) || defined(CONFIG_HVF)
      { .name = "host",               .initfn = aarch64_host_initfn },
  #endif
diff --git a/target/arm/tcg/cpu32.c b/target/arm/tcg/cpu32.c
index d9e0e2a4dd..dcad399a60 100644
--- a/target/arm/tcg/cpu32.c
+++ b/target/arm/tcg/cpu32.c
@@ -1049,7 +1049,6 @@ static void arm_v7m_class_init(ObjectClass *oc, 
void *data)
      cc->gdb_core_xml_file = "arm-m-profile.xml";
  }

-#ifndef TARGET_AARCH64
  /*
   * -cpu max: a CPU with as many features enabled as our emulation 
supports.
   * The version of '-cpu max' for qemu-system-aarch64 is defined in 
cpu64.c;
@@ -1112,7 +1111,11 @@ static void arm_max_initfn(Object *obj)
      cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
  #endif
  }
-#endif /* !TARGET_AARCH64 */
+
+static bool aa32_only(void)
+{
+    return !target_aarch64_available();
+}

  static const ARMCPUInfo arm_tcg_cpus[] = {
      { .name = "arm926",      .initfn = arm926_initfn },
@@ -1162,9 +1165,8 @@ static const ARMCPUInfo arm_tcg_cpus[] = {
      { .name = "pxa270-b1",   .initfn = pxa270b1_initfn },
      { .name = "pxa270-c0",   .initfn = pxa270c0_initfn },
      { .name = "pxa270-c5",   .initfn = pxa270c5_initfn },
-#ifndef TARGET_AARCH64
-    { .name = "max",         .initfn = arm_max_initfn },
-#endif
+    { .name = "max",         .initfn = arm_max_initfn,
+                             .can_register = aa32_only },
  #ifdef CONFIG_USER_ONLY
      { .name = "any",         .initfn = arm_max_initfn },
  #endif
---

Re: [RFC PATCH-for-9.0 01/11] qom: Introduce the TypeInfo::can_register() handler

[Thomas Huth]

On 23/11/2023 17.03, Philippe Mathieu-Daudé wrote:
> On 23/11/23 16:09, Thomas Huth wrote:
>> On 22/11/2023 19.30, Philippe Mathieu-Daudé wrote:
>>> Add a helper to decide at runtime whether a type can
>>> be registered to the QOM framework or not.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/qom/object.h | 4 ++++
>>>   qom/object.c         | 3 +++
>>>   2 files changed, 7 insertions(+)
>>>
>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>> index afccd24ca7..0d42fe17de 100644
>>> --- a/include/qom/object.h
>>> +++ b/include/qom/object.h
>>> @@ -372,6 +372,8 @@ struct Object
>>>    * struct TypeInfo:
>>>    * @name: The name of the type.
>>>    * @parent: The name of the parent type.
>>> + * @can_register: This optional function is called before a type is 
>>> registered.
>>> + *   If it exists and returns false, the type is not registered.
>>
>> The second sentence is quite hard to parse, since it is not quite clear 
>> what "it" refers to (type or function) and what "registered" means in this 
>> context (you don't mention type_register() here).
>>
>> Maybe rather something like:
>>
>> If set, type_register() uses this function to decide whether the type can 
>> be registered or not.
>>
>> ?
>>
>>>    * @instance_size: The size of the object (derivative of #Object).  If
>>>    *   @instance_size is 0, then the size of the object will be the size 
>>> of the
>>>    *   parent object.
>>> @@ -414,6 +416,8 @@ struct TypeInfo
>>>       const char *name;
>>>       const char *parent;
>>> +    bool (*can_register)(void);
>>> +
>>>       size_t instance_size;
>>>       size_t instance_align;
>>>       void (*instance_init)(Object *obj);
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 95c0dc8285..f09b6b5a92 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -150,6 +150,9 @@ static TypeImpl *type_register_internal(const 
>>> TypeInfo *info)
>>>   TypeImpl *type_register(const TypeInfo *info)
>>>   {
>>>       assert(info->parent);
>>> +    if (info->can_register && !info->can_register()) {
>>> +        return NULL;
>>> +    }
>>
>> I have to say that I don't like it too much, since you're trying to fix a 
>> problem here in common code that clearly belongs to the code in hw/arm/ 
>> instead.
>>
>> What about dropping it, and changing your last patch to replace the 
>> DEFINE_TYPES(raspi_machine_types) in hw/arm/raspi.c with your own 
>> implementation of type_register_static_array() that checks the condition 
>> there?
> 
> This isn't ARM specific, it happens I started to unify ARM/aarch64
> binaries.
> 
> Types can be registered depending on build-time (config/host specific)
> definitions and runtime ones. How can we check for runtime if not via
> this simple helper?
> 
> Still ARM, but as example what I have then is (module meson):
...
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 1e9c6c85ae..c3b7e5666c 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -744,7 +744,8 @@ static void aarch64_max_initfn(Object *obj)
>   static const ARMCPUInfo aarch64_cpus[] = {
>       { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
>       { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> -    { .name = "max",                .initfn = aarch64_max_initfn },
> +    { .name = "max",                .initfn = aarch64_max_initfn,
> +                                    .can_register = 
> target_aarch64_available },
>   #if defined(CONFIG_KVM) || defined(CONFIG_HVF)
>       { .name = "host",               .initfn = aarch64_host_initfn },
>   #endif

Picking this one as an example, I think I'd rather modify the for-loop in
aarch64_cpu_register_types() to check for the availability there... sounds 
much easier to understand for me than having a callback function.

Anyway, that's just my personal taste - if others agree with your solution 
instead, I won't insist on my idea.

  Thomas

Re: [RFC PATCH-for-9.0 01/11] qom: Introduce the TypeInfo::can_register() handler

[Philippe Mathieu-Daudé]

On 23/11/23 17:24, Thomas Huth wrote:
> On 23/11/2023 17.03, Philippe Mathieu-Daudé wrote:
>> On 23/11/23 16:09, Thomas Huth wrote:
>>> On 22/11/2023 19.30, Philippe Mathieu-Daudé wrote:
>>>> Add a helper to decide at runtime whether a type can
>>>> be registered to the QOM framework or not.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   include/qom/object.h | 4 ++++
>>>>   qom/object.c         | 3 +++
>>>>   2 files changed, 7 insertions(+)
>>>>
>>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>>> index afccd24ca7..0d42fe17de 100644
>>>> --- a/include/qom/object.h
>>>> +++ b/include/qom/object.h
>>>> @@ -372,6 +372,8 @@ struct Object
>>>>    * struct TypeInfo:
>>>>    * @name: The name of the type.
>>>>    * @parent: The name of the parent type.
>>>> + * @can_register: This optional function is called before a type is 
>>>> registered.
>>>> + *   If it exists and returns false, the type is not registered.
>>>
>>> The second sentence is quite hard to parse, since it is not quite 
>>> clear what "it" refers to (type or function) and what "registered" 
>>> means in this context (you don't mention type_register() here).
>>>
>>> Maybe rather something like:
>>>
>>> If set, type_register() uses this function to decide whether the type 
>>> can be registered or not.
>>>
>>> ?
>>>
>>>>    * @instance_size: The size of the object (derivative of 
>>>> #Object).  If
>>>>    *   @instance_size is 0, then the size of the object will be the 
>>>> size of the
>>>>    *   parent object.
>>>> @@ -414,6 +416,8 @@ struct TypeInfo
>>>>       const char *name;
>>>>       const char *parent;
>>>> +    bool (*can_register)(void);
>>>> +
>>>>       size_t instance_size;
>>>>       size_t instance_align;
>>>>       void (*instance_init)(Object *obj);
>>>> diff --git a/qom/object.c b/qom/object.c
>>>> index 95c0dc8285..f09b6b5a92 100644
>>>> --- a/qom/object.c
>>>> +++ b/qom/object.c
>>>> @@ -150,6 +150,9 @@ static TypeImpl *type_register_internal(const 
>>>> TypeInfo *info)
>>>>   TypeImpl *type_register(const TypeInfo *info)
>>>>   {
>>>>       assert(info->parent);
>>>> +    if (info->can_register && !info->can_register()) {
>>>> +        return NULL;
>>>> +    }
>>>
>>> I have to say that I don't like it too much, since you're trying to 
>>> fix a problem here in common code that clearly belongs to the code in 
>>> hw/arm/ instead.
>>>
>>> What about dropping it, and changing your last patch to replace the 
>>> DEFINE_TYPES(raspi_machine_types) in hw/arm/raspi.c with your own 
>>> implementation of type_register_static_array() that checks the 
>>> condition there?
>>
>> This isn't ARM specific, it happens I started to unify ARM/aarch64
>> binaries.
>>
>> Types can be registered depending on build-time (config/host specific)
>> definitions and runtime ones. How can we check for runtime if not via
>> this simple helper?
>>
>> Still ARM, but as example what I have then is (module meson):
> ...
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 1e9c6c85ae..c3b7e5666c 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -744,7 +744,8 @@ static void aarch64_max_initfn(Object *obj)
>>   static const ARMCPUInfo aarch64_cpus[] = {
>>       { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
>>       { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
>> -    { .name = "max",                .initfn = aarch64_max_initfn },
>> +    { .name = "max",                .initfn = aarch64_max_initfn,
>> +                                    .can_register = 
>> target_aarch64_available },
>>   #if defined(CONFIG_KVM) || defined(CONFIG_HVF)
>>       { .name = "host",               .initfn = aarch64_host_initfn },
>>   #endif
> 
> Picking this one as an example, I think I'd rather modify the for-loop in
> aarch64_cpu_register_types() to check for the availability there... 
> sounds much easier to understand for me than having a callback function.

OK.

> Anyway, that's just my personal taste - if others agree with your 
> solution instead, I won't insist on my idea.

This is a RFC so let's discuss :) I think there is a need to filter
QOM types at runtime (at least in "Single Binary" or "Heterogeneous
machines"), but I might be wrong.
Maybe we can filter that elsewhere (here seemed the simplest / more
natural place). I'll keep looking.

Regards,

Phil.

Re: [PATCH-for-9.0 03/11] target/arm: Declare ARM_CPU_TYPE_NAME/SUFFIX in 'cpu-qom.h'

[Richard Henderson]

On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
> Missed in commit 2d56be5a29 ("target: Declare
> FOO_CPU_TYPE_NAME/SUFFIX in 'cpu-qom.h'"). See
> it for more details.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu-qom.h | 3 +++
>   target/arm/cpu.h     | 2 --
>   2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-9.0 04/11] target/arm: Move ARM_CPU_IRQ/FIQ definitions to 'cpu-qom.h'

[Richard Henderson]

On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
> The ARM_CPU_IRQ/FIQ definitions are meant for the ARM CPU
> QOM model. Move them to "cpu-qom.h" so any QOM code can
> use them.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
> Or do these definitions belong to cpu-defs.h?

I think they belong with the qom bits.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH-for-9.0 05/11] target/arm: Move GTIMER definitions to 'cpu-defs.h'

[Richard Henderson]

On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
> To allow GTIMER_* definitions to be used by non-ARM specific
> hardware models, move them to a new target agnostic "cpu-defs.h"
> header.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu-defs.h | 19 +++++++++++++++++++
>   target/arm/cpu.h      |  8 +-------
>   hw/arm/bcm2836.c      |  1 +
>   3 files changed, 21 insertions(+), 7 deletions(-)
>   create mode 100644 target/arm/cpu-defs.h
> 
> diff --git a/target/arm/cpu-defs.h b/target/arm/cpu-defs.h
> new file mode 100644
> index 0000000000..1ad76aff14
> --- /dev/null
> +++ b/target/arm/cpu-defs.h
> @@ -0,0 +1,19 @@
> +/*
> + * ARM "target agnostic" CPU definitions
> + *
> + *  Copyright (c) 2003 Fabrice Bellard
> + *
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#ifndef ARM_CPU_DEFS_H
> +#define ARM_CPU_DEFS_H
> +
> +#define GTIMER_PHYS     0
> +#define GTIMER_VIRT     1
> +#define GTIMER_HYP      2
> +#define GTIMER_SEC      3
> +#define GTIMER_HYPVIRT  4
> +#define NUM_GTIMERS     5
> +
> +#endif

Hmm.  cpu-defs.h is pretty generic.
Without looking forward in the patch series, perhaps better as gtimer.h?

Is hw/arm/bcm2836.c really "non-arm-specific"?  Or did you mean "non-ARMCPU-specific"?


r~

Re: [PATCH-for-9.0 06/11] hw/arm/bcm2836: Simplify use of 'reset-cbar' property

[Richard Henderson]

On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
> bcm2836_realize() is called by
> 
>   - bcm2836_class_init() which sets:
> 
>      bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7")
> 
>   - bcm2837_class_init() which sets:
> 
>      bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a53")
> 
> Both Cortex-A7 / A53 have the ARM_FEATURE_CBAR set. If it isn't,
> then this is a programming error: use &error_abort.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH-for-9.0 07/11] hw/arm/bcm2836: Simplify access to 'start-powered-off' property

[Richard Henderson]

On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
> All ARM CPUs have the 'start-powered-off' property since commit
> 5de164304a ("arm: Allow secondary KVM CPUs to be booted via PSCI").
> 
> Note: since commit c1b701587e ("target/arm: Move start-powered-off
> property to generic CPUState"), all CPUs for all targets have it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH-for-9.0 08/11] hw/arm/bcm2836: Use ARM_CPU 'mp-affinity' property

[Richard Henderson]

On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
> The 'mp-affinity' property is present since commit 15a21fe028
> ("target-arm: Add mp-affinity property for ARM CPU class").
> Use it and remove a /* TODO */ comment. Since all ARM CPUs
> have this property, use &error_abort, because this call can
> not fail.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [RFC PATCH-for-9.0 09/11] hw/arm/bcm2836: Allocate ARM CPU state with object_new()

[Richard Henderson]

On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
> The ARMCPU type is forward declared as a pointer to all hw/ files.
> Its declaration is restricted to target/arm/ files. By using a
> pointer in BCM283XState instead of embedding the whole CPU state,
> we don't need to include "cpu.h" which is target-specific.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/arm/bcm2836.h |  4 ++--
>   hw/arm/bcm2836.c         | 19 ++++++++++---------
>   hw/arm/raspi.c           |  2 +-
>   3 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
> index 6f90cabfa3..784bab0aad 100644
> --- a/include/hw/arm/bcm2836.h
> +++ b/include/hw/arm/bcm2836.h
> @@ -14,7 +14,7 @@
>   
>   #include "hw/arm/bcm2835_peripherals.h"
>   #include "hw/intc/bcm2836_control.h"
> -#include "target/arm/cpu.h"
> +#include "target/arm/cpu-qom.h"
>   #include "qom/object.h"
>   
>   #define TYPE_BCM283X "bcm283x"
> @@ -38,7 +38,7 @@ struct BCM283XState {
>       uint32_t enabled_cpus;
>   
>       struct {
> -        ARMCPU core;
> +        ARMCPU *core;
>       } cpu[BCM283X_NCPUS];

I'd be tempted to drop the unused struct:

     ARMCPU *cpu[BCM283X_NCPUS];

while you're at it.  Anyway,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH-for-9.0 05/11] target/arm: Move GTIMER definitions to 'cpu-defs.h'

[Philippe Mathieu-Daudé]

On 28/11/23 15:02, Richard Henderson wrote:
> On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
>> To allow GTIMER_* definitions to be used by non-ARM specific
>> hardware models, move them to a new target agnostic "cpu-defs.h"
>> header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/arm/cpu-defs.h | 19 +++++++++++++++++++
>>   target/arm/cpu.h      |  8 +-------
>>   hw/arm/bcm2836.c      |  1 +
>>   3 files changed, 21 insertions(+), 7 deletions(-)
>>   create mode 100644 target/arm/cpu-defs.h
>>
>> diff --git a/target/arm/cpu-defs.h b/target/arm/cpu-defs.h
>> new file mode 100644
>> index 0000000000..1ad76aff14
>> --- /dev/null
>> +++ b/target/arm/cpu-defs.h
>> @@ -0,0 +1,19 @@
>> +/*
>> + * ARM "target agnostic" CPU definitions
>> + *
>> + *  Copyright (c) 2003 Fabrice Bellard
>> + *
>> + * SPDX-License-Identifier: LGPL-2.1-or-later
>> + */
>> +
>> +#ifndef ARM_CPU_DEFS_H
>> +#define ARM_CPU_DEFS_H
>> +
>> +#define GTIMER_PHYS     0
>> +#define GTIMER_VIRT     1
>> +#define GTIMER_HYP      2
>> +#define GTIMER_SEC      3
>> +#define GTIMER_HYPVIRT  4
>> +#define NUM_GTIMERS     5
>> +
>> +#endif
> 
> Hmm.  cpu-defs.h is pretty generic.
> Without looking forward in the patch series, perhaps better as gtimer.h?

- target specific parameters used by accel/ (stay) in "cpu-param.h"
   (Ideally the single header included by accel/)

- target accelerator implementation details (stay) in "cpu.h"
   Shouldn't be used outside of target/

- architecture definitions in "arch-defs.h"
   (used by target/ and hw/)

- QEMU specific implementation details in "cpu-qom.h"
   (mostly used by hw/)

> 
> Is hw/arm/bcm2836.c really "non-arm-specific"?  Or did you mean 
> "non-ARMCPU-specific"?

I'd like to eventually have it instantiate a CPU which is not ARM based.