[PATCH v2 0/7] target/arm: Remove TYPE_AARCH64_CPU class

[PATCH v2 0/7] target/arm: Remove TYPE_AARCH64_CPU class

[Peter Maydell]

Currently we have a class hierarchy for Arm CPUs where
all the 32-bit CPUs (including M-profile) inherit directly
from TYPE_ARM_CPU, but the 64-bit CPUs inherit from
TYPE_AARCH64_CPU, which is a subclass of TYPE_ARM_CPU.
This subclass does essentially two things:
 * it sets up fields and methods for the gdbstub so that
   the gdbstub presents an AArch64 CPU to gdb rather than
   an AArch32 one
 * it defines the 'aarch64' CPU property which you can use
   with KVM to disable AArch64 and create a 32-bit VM
   (with "-cpu host,aarch64=off")

This is a bit weird, because the 32-bit CPU you create with
KVM and aarch64=off is still a subclass of TYPE_AARCH64_CPU.
It also still presents gdb with an AArch64 CPU, so you
effectively can't use the gdbstub with this kind of VM.

This patchseries removes TYPE_AARCH64_CPU so that all CPUs,
both AArch32 and AArch64, directly inherit from TYPE_ARM_CPU.
This lets us fix the bug with gdbstub and "aarch64=off".

In this version I fix the bug that Philippe found where we
were calling arm_gdbstub_is_aarch64() too early, before the
object had been created and the "is it AArch64 or not?"
question resolved. This is basically moving the gdb_init_cpu()
call into cpu_exec_realizefn(), but we need to do a little
adjustment of microblaze for that to work.

Version 2 of the patchset:
 * patches 1-5 from v1 are already upstream
 * new patches 1-3 here delay the call to gdb_init_cpu()
   so that it is done only after the CPU object is at least
   inited and we definitely know the final value of the
   'aarch64' property
 * patches 4-7 are the old 6-9 and have been reviewed

thanks
-- PMM

Peter Maydell (7):
  target/microblaze: Use 'obj' in DEVICE() casts in mb_cpu_initfn()
  target/microblaze: Delay gdb_register_coprocessor() to realize
  hw/core/cpu-common: Don't init gdbstub until cpu_exec_realizefn()
  target/arm: Present AArch64 gdbstub based on ARM_FEATURE_AARCH64
  target/arm: Move aarch64 CPU property code to TYPE_ARM_CPU
  target/arm/kvm: don't check TYPE_AARCH64_CPU
  target/arm: Remove TYPE_AARCH64_CPU

 target/arm/cpu-qom.h    |  5 ---
 target/arm/cpu.h        |  4 --
 target/arm/internals.h  |  3 +-
 hw/core/cpu-common.c    |  3 +-
 target/arm/cpu.c        | 36 ++++++++++++++++++
 target/arm/cpu64.c      | 82 +----------------------------------------
 target/arm/kvm.c        |  3 +-
 target/arm/tcg/cpu64.c  |  2 +-
 target/microblaze/cpu.c | 22 +++++------
 9 files changed, 52 insertions(+), 108 deletions(-)

-- 
2.43.0

[PATCH v2 1/7] target/microblaze: Use 'obj' in DEVICE() casts in mb_cpu_initfn()

[Peter Maydell]

We're about to make a change that removes the only other use
of the 'cpu' local variable in mb_cpu_initfn(); since the
DEVICE() casts work fine with the Object*, use that instead,
so that we can remove the local variable when we make the
following change.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/microblaze/cpu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 00a2730de4d..d92a43191bd 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -333,11 +333,11 @@ static void mb_cpu_initfn(Object *obj)
 
 #ifndef CONFIG_USER_ONLY
     /* Inbound IRQ and FIR lines */
-    qdev_init_gpio_in(DEVICE(cpu), microblaze_cpu_set_irq, 2);
-    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_dp, "ns_axi_dp", 1);
-    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_ip, "ns_axi_ip", 1);
-    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_dc, "ns_axi_dc", 1);
-    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_ic, "ns_axi_ic", 1);
+    qdev_init_gpio_in(DEVICE(obj), microblaze_cpu_set_irq, 2);
+    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_dp, "ns_axi_dp", 1);
+    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_ip, "ns_axi_ip", 1);
+    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_dc, "ns_axi_dc", 1);
+    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_ic, "ns_axi_ic", 1);
 #endif
 
     /* Restricted 'endianness' property is equivalent of 'little-endian' */
-- 
2.43.0

[PATCH v2 2/7] target/microblaze: Delay gdb_register_coprocessor() to realize

[Peter Maydell]

Currently the microblaze code calls gdb_register_coprocessor() in its
initfn.  This works, but we would like to delay setting up GDB
registers until realize.  All other target architectures only call
gdb_register_coprocessor() in realize, after the call to
cpu_exec_realizefn().

Move the microblaze gdb_register_coprocessor() use, bringing it
in line with other targets.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/microblaze/cpu.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index d92a43191bd..b8dae83ce0c 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -252,6 +252,11 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    gdb_register_coprocessor(cs, mb_cpu_gdb_read_stack_protect,
+                             mb_cpu_gdb_write_stack_protect,
+                             gdb_find_static_feature("microblaze-stack-protect.xml"),
+                             0);
+
     qemu_init_vcpu(cs);
 
     version = cpu->cfg.version ? cpu->cfg.version : DEFAULT_CPU_VERSION;
@@ -324,13 +329,6 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
 
 static void mb_cpu_initfn(Object *obj)
 {
-    MicroBlazeCPU *cpu = MICROBLAZE_CPU(obj);
-
-    gdb_register_coprocessor(CPU(cpu), mb_cpu_gdb_read_stack_protect,
-                             mb_cpu_gdb_write_stack_protect,
-                             gdb_find_static_feature("microblaze-stack-protect.xml"),
-                             0);
-
 #ifndef CONFIG_USER_ONLY
     /* Inbound IRQ and FIR lines */
     qdev_init_gpio_in(DEVICE(obj), microblaze_cpu_set_irq, 2);
-- 
2.43.0

[PATCH v2 3/7] hw/core/cpu-common: Don't init gdbstub until cpu_exec_realizefn()

[Peter Maydell]

Currently we call gdb_init_cpu() in cpu_common_initfn(), which is
very early in the CPU object's init->realize creation sequence.  In
particular this happens before the architecture-specific subclass's
init fn has even run.  This means that gdb_init_cpu() can only do
things that depend strictly on the class, not on the object, because
the CPUState* that it is passed is currently half-initialized.

In commit a1f728ecc90cf6c6 we accidentally broke this rule, by adding
a call to the gdb_get_core_xml_file method which takes the CPUState.
At the moment we get away with this because the only implementation
doesn't actually look at the pointer it is passed.  However the whole
reason we created that method was so that we could make the "which
XML file?" decision based on a property of the CPU object, and we
currently can't change the Arm implementation of the method to do
what we want without causing wrong behaviour or a crash.

The ordering restrictions here are:
 * we must call gdb_init_cpu before:
   - any call to gdb_register_coprocessor()
   - any use of the gdb_num_regs field (this is only used
     in code that's about to call gdb_register_coprocessor()
     and wants to know the first register number of the
     set of registers it's about to add)
 * we must call gdb_init_cpu after CPU properties have been
   set, which is to say somewhere in realize

The function cpu_exec_realizefn() meets both of these requirements,
as it is called by the architecture-specific CPU realize function
early in realize, before any calls ot gdb_register_coprocessor().
Move the gdb_init_cpu() call to there.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/cpu-common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 92c40b6bf83..39e674aca21 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -234,6 +234,8 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
         return false;
     }
 
+    gdb_init_cpu(cpu);
+
     /* Wait until cpu initialization complete before exposing cpu. */
     cpu_list_add(cpu);
 
@@ -304,7 +306,6 @@ static void cpu_common_initfn(Object *obj)
     /* cache the cpu class for the hotpath */
     cpu->cc = CPU_GET_CLASS(cpu);
 
-    gdb_init_cpu(cpu);
     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
     cpu->as = NULL;
-- 
2.43.0

[PATCH v2 4/7] target/arm: Present AArch64 gdbstub based on ARM_FEATURE_AARCH64

[Peter Maydell]

Currently we provide an AArch64 gdbstub for CPUs which are
TYPE_AARCH64_CPU, and an AArch32 gdbstub for those which are only
TYPE_ARM_CPU.  This mostly does the right thing, except in the
corner case of KVM with -cpu host,aarch64=off.  That produces a CPU
which is TYPE_AARCH64_CPU but which has ARM_FEATURE_AARCH64 removed
and which to the guest is in AArch32 mode.

Now we have moved all the handling of AArch64-vs-AArch32 gdbstub
behaviour into TYPE_ARM_CPU we can change the condition we use for
whether to select the AArch64 gdbstub to look at ARM_FEATURE_AARCH64.
This will mean that we now correctly provide an AArch32 gdbstub for
aarch64=off CPUs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/internals.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 4d3d84ffebd..f1c06a3fd89 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1831,7 +1831,7 @@ void aarch64_add_sme_properties(Object *obj);
 /* Return true if the gdbstub is presenting an AArch64 CPU */
 static inline bool arm_gdbstub_is_aarch64(ARMCPU *cpu)
 {
-    return object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU);
+    return arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
 }
 
 /* Read the CONTROL register as the MRS instruction would. */
-- 
2.43.0

[PATCH v2 5/7] target/arm: Move aarch64 CPU property code to TYPE_ARM_CPU

[Peter Maydell]

The only thing we have left in the TYPE_AARCH64_CPU class that makes
it different to TYPE_ARM_CPU is that we register the handling of the
"aarch64" property there.

Move the handling of this property to the base class, where we make
it a property of the object rather than of the class, and add it
to the CPU if it has the ARM_FEATURE_AARCH64 property present at
init.  This is in line with how we handle other Arm CPU properties,
and should not change which CPUs it's visible for.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.c   | 36 ++++++++++++++++++++++++++++++++++++
 target/arm/cpu64.c | 33 ---------------------------------
 2 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5e951675c60..73a2a197667 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1610,6 +1610,35 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
     cpu->has_pmu = value;
 }
 
+static bool aarch64_cpu_get_aarch64(Object *obj, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    return arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
+}
+
+static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    /*
+     * At this time, this property is only allowed if KVM is enabled.  This
+     * restriction allows us to avoid fixing up functionality that assumes a
+     * uniform execution state like do_interrupt.
+     */
+    if (value == false) {
+        if (!kvm_enabled() || !kvm_arm_aarch32_supported()) {
+            error_setg(errp, "'aarch64' feature cannot be disabled "
+                             "unless KVM is enabled and 32-bit EL1 "
+                             "is supported");
+            return;
+        }
+        unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
+    } else {
+        set_feature(&cpu->env, ARM_FEATURE_AARCH64);
+    }
+}
+
 unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
 {
     /*
@@ -1737,6 +1766,13 @@ void arm_cpu_post_init(Object *obj)
      */
     arm_cpu_propagate_feature_implications(cpu);
 
+    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+        object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64,
+                                       aarch64_cpu_set_aarch64);
+        object_property_set_description(obj, "aarch64",
+                                        "Set on/off to enable/disable aarch64 "
+                                        "execution state ");
+    }
     if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
         arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {
         qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_cbar_property);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 00629a5d1d1..e527465a3ca 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -781,45 +781,12 @@ static const ARMCPUInfo aarch64_cpus[] = {
 #endif
 };
 
-static bool aarch64_cpu_get_aarch64(Object *obj, Error **errp)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    return arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
-}
-
-static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    /* At this time, this property is only allowed if KVM is enabled.  This
-     * restriction allows us to avoid fixing up functionality that assumes a
-     * uniform execution state like do_interrupt.
-     */
-    if (value == false) {
-        if (!kvm_enabled() || !kvm_arm_aarch32_supported()) {
-            error_setg(errp, "'aarch64' feature cannot be disabled "
-                             "unless KVM is enabled and 32-bit EL1 "
-                             "is supported");
-            return;
-        }
-        unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
-    } else {
-        set_feature(&cpu->env, ARM_FEATURE_AARCH64);
-    }
-}
-
 static void aarch64_cpu_finalizefn(Object *obj)
 {
 }
 
 static void aarch64_cpu_class_init(ObjectClass *oc, const void *data)
 {
-    object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
-                                   aarch64_cpu_set_aarch64);
-    object_class_property_set_description(oc, "aarch64",
-                                          "Set on/off to enable/disable aarch64 "
-                                          "execution state ");
 }
 
 static void aarch64_cpu_instance_init(Object *obj)
-- 
2.43.0

[PATCH v2 6/7] target/arm/kvm: don't check TYPE_AARCH64_CPU

[Peter Maydell]

We want to merge TYPE_AARCH64_CPU with TYPE_ARM_CPU, so enforcing in
kvm_arch_init_vcpu() that the CPU class is a subclass of
TYPE_AARCH64_CPU will no longer be possible.

It's safe to just remove this test, because any purely-AArch32 CPU
will fail the "kvm_target isn't set" check, because we no longer
support the old AArch32-host KVM setup and so CPUs like the Cortex-A7
no longer set cpu->kvm_target. Only the 'host', 'max', and the
odd special cases 'cortex-a53' and 'cortex-a57' set kvm_target.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/kvm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 97de8c7e939..6b2c788e0fa 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -1882,8 +1882,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     CPUARMState *env = &cpu->env;
     uint64_t psciver;
 
-    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
-        !object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU)) {
+    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
         error_report("KVM is not supported for this guest CPU type");
         return -EINVAL;
     }
-- 
2.43.0

[PATCH v2 7/7] target/arm: Remove TYPE_AARCH64_CPU

[Peter Maydell]

The TYPE_AARCH64_CPU class is an abstract type that is the parent of
all the AArch64 CPUs.  It now has no special behaviour of its own, so
we can eliminate it and make the AArch64 CPUs directly inherit from
TYPE_ARM_CPU.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu-qom.h   |  5 -----
 target/arm/cpu.h       |  4 ----
 target/arm/internals.h |  1 -
 target/arm/cpu64.c     | 49 +-----------------------------------------
 target/arm/tcg/cpu64.c |  2 +-
 5 files changed, 2 insertions(+), 59 deletions(-)

diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
index b497667d61e..2fcb0e12525 100644
--- a/target/arm/cpu-qom.h
+++ b/target/arm/cpu-qom.h
@@ -28,11 +28,6 @@ OBJECT_DECLARE_CPU_TYPE(ARMCPU, ARMCPUClass, ARM_CPU)
 
 #define TYPE_ARM_MAX_CPU "max-" TYPE_ARM_CPU
 
-#define TYPE_AARCH64_CPU "aarch64-cpu"
-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)
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index fdcf8cd1ae0..a394c7d46d7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1141,10 +1141,6 @@ struct ARMCPUClass {
     ResettablePhases parent_phases;
 };
 
-struct AArch64CPUClass {
-    ARMCPUClass parent_class;
-};
-
 /* Callback functions for the generic timer's timers. */
 void arm_gt_ptimer_cb(void *opaque);
 void arm_gt_vtimer_cb(void *opaque);
diff --git a/target/arm/internals.h b/target/arm/internals.h
index f1c06a3fd89..7be34388fc2 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -353,7 +353,6 @@ static inline int r14_bank_number(int mode)
 }
 
 void arm_cpu_register(const ARMCPUInfo *info);
-void aarch64_cpu_register(const ARMCPUInfo *info);
 
 void register_cp_regs_for_features(ARMCPU *cpu);
 void init_cpreg_list(ARMCPU *cpu);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index e527465a3ca..200da1c489b 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -781,59 +781,12 @@ static const ARMCPUInfo aarch64_cpus[] = {
 #endif
 };
 
-static void aarch64_cpu_finalizefn(Object *obj)
-{
-}
-
-static void aarch64_cpu_class_init(ObjectClass *oc, const void *data)
-{
-}
-
-static void aarch64_cpu_instance_init(Object *obj)
-{
-    ARMCPUClass *acc = ARM_CPU_GET_CLASS(obj);
-
-    acc->info->initfn(obj);
-    arm_cpu_post_init(obj);
-}
-
-static void cpu_register_class_init(ObjectClass *oc, const void *data)
-{
-    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
-
-    acc->info = data;
-}
-
-void aarch64_cpu_register(const ARMCPUInfo *info)
-{
-    TypeInfo type_info = {
-        .parent = TYPE_AARCH64_CPU,
-        .instance_init = aarch64_cpu_instance_init,
-        .class_init = info->class_init ?: cpu_register_class_init,
-        .class_data = info,
-    };
-
-    type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
-    type_register_static(&type_info);
-    g_free((void *)type_info.name);
-}
-
-static const TypeInfo aarch64_cpu_type_info = {
-    .name = TYPE_AARCH64_CPU,
-    .parent = TYPE_ARM_CPU,
-    .instance_finalize = aarch64_cpu_finalizefn,
-    .abstract = true,
-    .class_init = aarch64_cpu_class_init,
-};
-
 static void aarch64_cpu_register_types(void)
 {
     size_t i;
 
-    type_register_static(&aarch64_cpu_type_info);
-
     for (i = 0; i < ARRAY_SIZE(aarch64_cpus); ++i) {
-        aarch64_cpu_register(&aarch64_cpus[i]);
+        arm_cpu_register(&aarch64_cpus[i]);
     }
 }
 
diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
index 29ab0ac79da..5d8ed2794d3 100644
--- a/target/arm/tcg/cpu64.c
+++ b/target/arm/tcg/cpu64.c
@@ -1316,7 +1316,7 @@ static void aarch64_cpu_register_types(void)
     size_t i;
 
     for (i = 0; i < ARRAY_SIZE(aarch64_cpus); ++i) {
-        aarch64_cpu_register(&aarch64_cpus[i]);
+        arm_cpu_register(&aarch64_cpus[i]);
     }
 }
 
-- 
2.43.0

Re: [PATCH v2 1/7] target/microblaze: Use 'obj' in DEVICE() casts in mb_cpu_initfn()

[Philippe Mathieu-Daudé]

On 29/4/25 15:21, Peter Maydell wrote:
> We're about to make a change that removes the only other use
> of the 'cpu' local variable in mb_cpu_initfn(); since the
> DEVICE() casts work fine with the Object*, use that instead,
> so that we can remove the local variable when we make the
> following change.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/microblaze/cpu.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 1/7] target/microblaze: Use 'obj' in DEVICE() casts in mb_cpu_initfn()

[Edgar E. Iglesias]

On Tue, Apr 29, 2025 at 02:21:54PM +0100, Peter Maydell wrote:
> We're about to make a change that removes the only other use
> of the 'cpu' local variable in mb_cpu_initfn(); since the
> DEVICE() casts work fine with the Object*, use that instead,
> so that we can remove the local variable when we make the
> following change.
>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/microblaze/cpu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 00a2730de4d..d92a43191bd 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -333,11 +333,11 @@ static void mb_cpu_initfn(Object *obj)
>  
>  #ifndef CONFIG_USER_ONLY
>      /* Inbound IRQ and FIR lines */
> -    qdev_init_gpio_in(DEVICE(cpu), microblaze_cpu_set_irq, 2);
> -    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_dp, "ns_axi_dp", 1);
> -    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_ip, "ns_axi_ip", 1);
> -    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_dc, "ns_axi_dc", 1);
> -    qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_ic, "ns_axi_ic", 1);
> +    qdev_init_gpio_in(DEVICE(obj), microblaze_cpu_set_irq, 2);
> +    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_dp, "ns_axi_dp", 1);
> +    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_ip, "ns_axi_ip", 1);
> +    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_dc, "ns_axi_dc", 1);
> +    qdev_init_gpio_in_named(DEVICE(obj), mb_cpu_ns_axi_ic, "ns_axi_ic", 1);
>  #endif
>  
>      /* Restricted 'endianness' property is equivalent of 'little-endian' */
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/7] target/microblaze: Delay gdb_register_coprocessor() to realize

[Edgar E. Iglesias]

On Tue, Apr 29, 2025 at 02:21:55PM +0100, Peter Maydell wrote:
> Currently the microblaze code calls gdb_register_coprocessor() in its
> initfn.  This works, but we would like to delay setting up GDB
> registers until realize.  All other target architectures only call
> gdb_register_coprocessor() in realize, after the call to
> cpu_exec_realizefn().
> 
> Move the microblaze gdb_register_coprocessor() use, bringing it
> in line with other targets.
>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/microblaze/cpu.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index d92a43191bd..b8dae83ce0c 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -252,6 +252,11 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    gdb_register_coprocessor(cs, mb_cpu_gdb_read_stack_protect,
> +                             mb_cpu_gdb_write_stack_protect,
> +                             gdb_find_static_feature("microblaze-stack-protect.xml"),
> +                             0);
> +
>      qemu_init_vcpu(cs);
>  
>      version = cpu->cfg.version ? cpu->cfg.version : DEFAULT_CPU_VERSION;
> @@ -324,13 +329,6 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>  static void mb_cpu_initfn(Object *obj)
>  {
> -    MicroBlazeCPU *cpu = MICROBLAZE_CPU(obj);
> -
> -    gdb_register_coprocessor(CPU(cpu), mb_cpu_gdb_read_stack_protect,
> -                             mb_cpu_gdb_write_stack_protect,
> -                             gdb_find_static_feature("microblaze-stack-protect.xml"),
> -                             0);
> -
>  #ifndef CONFIG_USER_ONLY
>      /* Inbound IRQ and FIR lines */
>      qdev_init_gpio_in(DEVICE(obj), microblaze_cpu_set_irq, 2);
> -- 
> 2.43.0
> 

Re: [PATCH v2 3/7] hw/core/cpu-common: Don't init gdbstub until cpu_exec_realizefn()

[Edgar E. Iglesias]

On Tue, Apr 29, 2025 at 02:21:56PM +0100, Peter Maydell wrote:
> Currently we call gdb_init_cpu() in cpu_common_initfn(), which is
> very early in the CPU object's init->realize creation sequence.  In
> particular this happens before the architecture-specific subclass's
> init fn has even run.  This means that gdb_init_cpu() can only do
> things that depend strictly on the class, not on the object, because
> the CPUState* that it is passed is currently half-initialized.
> 
> In commit a1f728ecc90cf6c6 we accidentally broke this rule, by adding
> a call to the gdb_get_core_xml_file method which takes the CPUState.
> At the moment we get away with this because the only implementation
> doesn't actually look at the pointer it is passed.  However the whole
> reason we created that method was so that we could make the "which
> XML file?" decision based on a property of the CPU object, and we
> currently can't change the Arm implementation of the method to do
> what we want without causing wrong behaviour or a crash.
> 
> The ordering restrictions here are:
>  * we must call gdb_init_cpu before:
>    - any call to gdb_register_coprocessor()
>    - any use of the gdb_num_regs field (this is only used
>      in code that's about to call gdb_register_coprocessor()
>      and wants to know the first register number of the
>      set of registers it's about to add)
>  * we must call gdb_init_cpu after CPU properties have been
>    set, which is to say somewhere in realize
> 
> The function cpu_exec_realizefn() meets both of these requirements,
> as it is called by the architecture-specific CPU realize function
> early in realize, before any calls ot gdb_register_coprocessor().
> Move the gdb_init_cpu() call to there.

Sounds good to me:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>



> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/core/cpu-common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 92c40b6bf83..39e674aca21 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -234,6 +234,8 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
>          return false;
>      }
>  
> +    gdb_init_cpu(cpu);
> +
>      /* Wait until cpu initialization complete before exposing cpu. */
>      cpu_list_add(cpu);
>  
> @@ -304,7 +306,6 @@ static void cpu_common_initfn(Object *obj)
>      /* cache the cpu class for the hotpath */
>      cpu->cc = CPU_GET_CLASS(cpu);
>  
> -    gdb_init_cpu(cpu);
>      cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>      cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
>      cpu->as = NULL;
> -- 
> 2.43.0
> 

Re: [PATCH v2 1/7] target/microblaze: Use 'obj' in DEVICE() casts in mb_cpu_initfn()

[Richard Henderson]

On 4/29/25 06:21, Peter Maydell wrote:
> We're about to make a change that removes the only other use
> of the 'cpu' local variable in mb_cpu_initfn(); since the
> DEVICE() casts work fine with the Object*, use that instead,
> so that we can remove the local variable when we make the
> following change.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/microblaze/cpu.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

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

r~

Re: [PATCH v2 2/7] target/microblaze: Delay gdb_register_coprocessor() to realize

[Richard Henderson]

On 4/29/25 06:21, Peter Maydell wrote:
> Currently the microblaze code calls gdb_register_coprocessor() in its
> initfn.  This works, but we would like to delay setting up GDB
> registers until realize.  All other target architectures only call
> gdb_register_coprocessor() in realize, after the call to
> cpu_exec_realizefn().
> 
> Move the microblaze gdb_register_coprocessor() use, bringing it
> in line with other targets.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/microblaze/cpu.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)

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

r~

Re: [PATCH v2 3/7] hw/core/cpu-common: Don't init gdbstub until cpu_exec_realizefn()

[Richard Henderson]

On 4/29/25 06:21, Peter Maydell wrote:
> Currently we call gdb_init_cpu() in cpu_common_initfn(), which is
> very early in the CPU object's init->realize creation sequence.  In
> particular this happens before the architecture-specific subclass's
> init fn has even run.  This means that gdb_init_cpu() can only do
> things that depend strictly on the class, not on the object, because
> the CPUState* that it is passed is currently half-initialized.
> 
> In commit a1f728ecc90cf6c6 we accidentally broke this rule, by adding
> a call to the gdb_get_core_xml_file method which takes the CPUState.
> At the moment we get away with this because the only implementation
> doesn't actually look at the pointer it is passed.  However the whole
> reason we created that method was so that we could make the "which
> XML file?" decision based on a property of the CPU object, and we
> currently can't change the Arm implementation of the method to do
> what we want without causing wrong behaviour or a crash.
> 
> The ordering restrictions here are:
>   * we must call gdb_init_cpu before:
>     - any call to gdb_register_coprocessor()
>     - any use of the gdb_num_regs field (this is only used
>       in code that's about to call gdb_register_coprocessor()
>       and wants to know the first register number of the
>       set of registers it's about to add)
>   * we must call gdb_init_cpu after CPU properties have been
>     set, which is to say somewhere in realize
> 
> The function cpu_exec_realizefn() meets both of these requirements,
> as it is called by the architecture-specific CPU realize function
> early in realize, before any calls ot gdb_register_coprocessor().
> Move the gdb_init_cpu() call to there.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/core/cpu-common.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

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

r~

Re: [PATCH v2 4/7] target/arm: Present AArch64 gdbstub based on ARM_FEATURE_AARCH64

[Richard Henderson]

On 4/29/25 06:21, Peter Maydell wrote:
> Currently we provide an AArch64 gdbstub for CPUs which are
> TYPE_AARCH64_CPU, and an AArch32 gdbstub for those which are only
> TYPE_ARM_CPU.  This mostly does the right thing, except in the
> corner case of KVM with -cpu host,aarch64=off.  That produces a CPU
> which is TYPE_AARCH64_CPU but which has ARM_FEATURE_AARCH64 removed
> and which to the guest is in AArch32 mode.
> 
> Now we have moved all the handling of AArch64-vs-AArch32 gdbstub
> behaviour into TYPE_ARM_CPU we can change the condition we use for
> whether to select the AArch64 gdbstub to look at ARM_FEATURE_AARCH64.
> This will mean that we now correctly provide an AArch32 gdbstub for
> aarch64=off CPUs.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/internals.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

Re: [PATCH v2 5/7] target/arm: Move aarch64 CPU property code to TYPE_ARM_CPU

[Richard Henderson]

On 4/29/25 06:21, Peter Maydell wrote:
> The only thing we have left in the TYPE_AARCH64_CPU class that makes
> it different to TYPE_ARM_CPU is that we register the handling of the
> "aarch64" property there.
> 
> Move the handling of this property to the base class, where we make
> it a property of the object rather than of the class, and add it
> to the CPU if it has the ARM_FEATURE_AARCH64 property present at
> init.  This is in line with how we handle other Arm CPU properties,
> and should not change which CPUs it's visible for.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/cpu.c   | 36 ++++++++++++++++++++++++++++++++++++
>   target/arm/cpu64.c | 33 ---------------------------------
>   2 files changed, 36 insertions(+), 33 deletions(-)

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

r~

Re: [PATCH v2 6/7] target/arm/kvm: don't check TYPE_AARCH64_CPU

[Richard Henderson]

On 4/29/25 06:21, Peter Maydell wrote:
> We want to merge TYPE_AARCH64_CPU with TYPE_ARM_CPU, so enforcing in
> kvm_arch_init_vcpu() that the CPU class is a subclass of
> TYPE_AARCH64_CPU will no longer be possible.
> 
> It's safe to just remove this test, because any purely-AArch32 CPU
> will fail the "kvm_target isn't set" check, because we no longer
> support the old AArch32-host KVM setup and so CPUs like the Cortex-A7
> no longer set cpu->kvm_target. Only the 'host', 'max', and the
> odd special cases 'cortex-a53' and 'cortex-a57' set kvm_target.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/kvm.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)


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

r~

Re: [PATCH v2 7/7] target/arm: Remove TYPE_AARCH64_CPU

[Richard Henderson]

On 4/29/25 06:22, Peter Maydell wrote:
> The TYPE_AARCH64_CPU class is an abstract type that is the parent of
> all the AArch64 CPUs.  It now has no special behaviour of its own, so
> we can eliminate it and make the AArch64 CPUs directly inherit from
> TYPE_ARM_CPU.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/cpu-qom.h   |  5 -----
>   target/arm/cpu.h       |  4 ----
>   target/arm/internals.h |  1 -
>   target/arm/cpu64.c     | 49 +-----------------------------------------
>   target/arm/tcg/cpu64.c |  2 +-
>   5 files changed, 2 insertions(+), 59 deletions(-)

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

r~

Re: [PATCH v2 3/7] hw/core/cpu-common: Don't init gdbstub until cpu_exec_realizefn()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Currently we call gdb_init_cpu() in cpu_common_initfn(), which is
> very early in the CPU object's init->realize creation sequence.  In
> particular this happens before the architecture-specific subclass's
> init fn has even run.  This means that gdb_init_cpu() can only do
> things that depend strictly on the class, not on the object, because
> the CPUState* that it is passed is currently half-initialized.
>
> In commit a1f728ecc90cf6c6 we accidentally broke this rule, by adding
> a call to the gdb_get_core_xml_file method which takes the CPUState.
> At the moment we get away with this because the only implementation
> doesn't actually look at the pointer it is passed.  However the whole
> reason we created that method was so that we could make the "which
> XML file?" decision based on a property of the CPU object, and we
> currently can't change the Arm implementation of the method to do
> what we want without causing wrong behaviour or a crash.
>
> The ordering restrictions here are:
>  * we must call gdb_init_cpu before:
>    - any call to gdb_register_coprocessor()
>    - any use of the gdb_num_regs field (this is only used
>      in code that's about to call gdb_register_coprocessor()
>      and wants to know the first register number of the
>      set of registers it's about to add)
>  * we must call gdb_init_cpu after CPU properties have been
>    set, which is to say somewhere in realize
>
> The function cpu_exec_realizefn() meets both of these requirements,
> as it is called by the architecture-specific CPU realize function
> early in realize, before any calls ot gdb_register_coprocessor().
> Move the gdb_init_cpu() call to there.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 3/7] hw/core/cpu-common: Don't init gdbstub until cpu_exec_realizefn()

[Philippe Mathieu-Daudé]

On 29/4/25 15:21, Peter Maydell wrote:
> Currently we call gdb_init_cpu() in cpu_common_initfn(), which is
> very early in the CPU object's init->realize creation sequence.  In
> particular this happens before the architecture-specific subclass's
> init fn has even run.  This means that gdb_init_cpu() can only do
> things that depend strictly on the class, not on the object, because
> the CPUState* that it is passed is currently half-initialized.
> 
> In commit a1f728ecc90cf6c6 we accidentally broke this rule, by adding
> a call to the gdb_get_core_xml_file method which takes the CPUState.

Oops sorry I missed that.

> At the moment we get away with this because the only implementation
> doesn't actually look at the pointer it is passed.  However the whole
> reason we created that method was so that we could make the "which
> XML file?" decision based on a property of the CPU object, and we
> currently can't change the Arm implementation of the method to do
> what we want without causing wrong behaviour or a crash.
> 
> The ordering restrictions here are:
>   * we must call gdb_init_cpu before:
>     - any call to gdb_register_coprocessor()
>     - any use of the gdb_num_regs field (this is only used
>       in code that's about to call gdb_register_coprocessor()
>       and wants to know the first register number of the
>       set of registers it's about to add)
>   * we must call gdb_init_cpu after CPU properties have been
>     set, which is to say somewhere in realize
> 
> The function cpu_exec_realizefn() meets both of these requirements,
> as it is called by the architecture-specific CPU realize function
> early in realize, before any calls ot gdb_register_coprocessor().
> Move the gdb_init_cpu() call to there.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/core/cpu-common.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 2/7] target/microblaze: Delay gdb_register_coprocessor() to realize

[Philippe Mathieu-Daudé]

On 29/4/25 15:21, Peter Maydell wrote:
> Currently the microblaze code calls gdb_register_coprocessor() in its
> initfn.  This works, but we would like to delay setting up GDB
> registers until realize.  All other target architectures only call
> gdb_register_coprocessor() in realize, after the call to
> cpu_exec_realizefn().
> 
> Move the microblaze gdb_register_coprocessor() use, bringing it
> in line with other targets.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/microblaze/cpu.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>