[PATCH for-10.1 0/9] target/arm: Remove TYPE_AARCH64_CPU class

[PATCH for-10.1 0/9] 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".

Most of the gdbstub related CPUClass fields are already methods,
so we can make the existing TYPE_ARM_CPU ones redirect to
the AArch64 functions if ARM_FEATURE_AARCH64 is set. The
odd-one-out is gdb_core_xml_file, so we have to add a new
optional method gdb_get_core_xml_file to allow us to select
the right XML file at runtime. (We could, like gdb_arch_name,
simply replace the existing static string field with the
method for all targets, but at least for this patchset I
didn't want to get into that complexity.)

We make the 'aarch64' property be an object property defined
if the ARM_FEATURE_AARCH64 is set rather than a class property;
this brings it into line with our other CPU properties.

Once we've done that and removed a check on TYPE_AARCH64_CPU
in the KVM code that hasn't been needed since we removed
32-bit Arm host KVM support, we can remove TYPE_AARCH64_CPU
entirely.

(The rationale here is that I think we should be able to
enable 'aarch64=off' for TCG CPUs too, so this will become
less of an odd KVM-specific corner case, and this seemed
worth cleaning up.)

thanks
-- PMM

Peter Maydell (9):
  core/cpu.h: gdb_arch_name string should not be freed
  gdbstub: Allow gdb_core_xml_file to be set at runtime
  target/arm: Handle AArch64 in TYPE_ARM_CPU gdb_arch_name
  target/arm: Handle gdb_core_xml_file in TYPE_ARM_CPU
  target/arm: Handle AArch64 gdb read/write regs in TYPE_ARM_CPU
  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

 include/hw/core/cpu.h    |  8 +++-
 target/arm/cpu-qom.h     |  5 ---
 target/arm/cpu.h         |  4 --
 target/arm/internals.h   |  7 ++-
 gdbstub/gdbstub.c        | 23 ++++++++--
 target/arm/cpu.c         | 55 ++++++++++++++++++++++-
 target/arm/cpu64.c       | 94 +---------------------------------------
 target/arm/gdbstub.c     | 12 +++++
 target/arm/kvm.c         |  3 +-
 target/arm/tcg/cpu-v7m.c |  1 -
 target/arm/tcg/cpu64.c   |  2 +-
 11 files changed, 101 insertions(+), 113 deletions(-)

-- 
2.43.0

[PATCH for-10.1 1/9] core/cpu.h: gdb_arch_name string should not be freed

[Peter Maydell]

The documentation for the CPUClass::gdb_arch_name method claims that
the returned string should be freed with g_free().  This is not
correct: in commit a650683871ba728 we changed this method to
instead return a simple constant string, but forgot to update
the documentation.

Make the documentation match the new semantics.

Fixes: a650683871ba728 ("hw/core/cpu: Return static value with gdb_arch_name()")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/core/cpu.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5d11d26556a..5873ee5998f 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -135,7 +135,8 @@ struct SysemuCPUOps;
  * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
  *           before the insn which triggers a watchpoint rather than after it.
  * @gdb_arch_name: Optional callback that returns the architecture name known
- * to GDB. The caller must free the returned string with g_free.
+ * to GDB. The returned value is expected to be a simple constant string:
+ * the caller will not g_free() it.
  * @disas_set_info: Setup architecture specific components of disassembly info
  * @adjust_watchpoint_address: Perform a target-specific adjustment to an
  * address before attempting to match it against watchpoints.
-- 
2.43.0

[PATCH for-10.1 2/9] gdbstub: Allow gdb_core_xml_file to be set at runtime

[Peter Maydell]

Currently the CPUClass:gdb_core_xml_file setting is a simple 'const
char *' which the CPU class must set to a fixed string.  Allow the
CPU class to instead set a new method gdb_get_core_xml_file() which
returns this string.

This will allow Arm CPUs to use different XML files for AArch32 vs
AArch64 without having to have an extra AArch64-specific class type
purely to give somewhere to set cc->gdb_core_xml_file differently.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/core/cpu.h |  5 +++++
 gdbstub/gdbstub.c     | 23 +++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5873ee5998f..140d8a0bd79 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -132,6 +132,10 @@ struct SysemuCPUOps;
  * @gdb_num_core_regs: Number of core registers accessible to GDB or 0 to infer
  *                     from @gdb_core_xml_file.
  * @gdb_core_xml_file: File name for core registers GDB XML description.
+ * @gdb_get_core_xml_file: Optional callback that returns the file name for
+ * the core registers GDB XML description. The returned value is expected to
+ * be a simple constant string: the caller will not g_free() it. If this
+ * is NULL then @gdb_core_xml_file will be used instead.
  * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
  *           before the insn which triggers a watchpoint rather than after it.
  * @gdb_arch_name: Optional callback that returns the architecture name known
@@ -167,6 +171,7 @@ struct CPUClass {
 
     const char *gdb_core_xml_file;
     const gchar * (*gdb_arch_name)(CPUState *cpu);
+    const char * (*gdb_get_core_xml_file)(CPUState *cpu);
 
     void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
 
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 282e13e163f..565f6b33a90 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -565,15 +565,30 @@ static void gdb_register_feature(CPUState *cpu, int base_reg,
     g_array_append_val(cpu->gdb_regs, s);
 }
 
+static const char *gdb_get_core_xml_file(CPUState *cpu)
+{
+    CPUClass *cc = cpu->cc;
+
+    /*
+     * The CPU class can provide the XML filename via a method,
+     * or as a simple fixed string field.
+     */
+    if (cc->gdb_get_core_xml_file) {
+        return cc->gdb_get_core_xml_file(cpu);
+    }
+    return cc->gdb_core_xml_file;
+}
+
 void gdb_init_cpu(CPUState *cpu)
 {
     CPUClass *cc = cpu->cc;
     const GDBFeature *feature;
+    const char *xmlfile = gdb_get_core_xml_file(cpu);
 
     cpu->gdb_regs = g_array_new(false, false, sizeof(GDBRegisterState));
 
-    if (cc->gdb_core_xml_file) {
-        feature = gdb_find_static_feature(cc->gdb_core_xml_file);
+    if (xmlfile) {
+        feature = gdb_find_static_feature(xmlfile);
         gdb_register_feature(cpu, 0,
                              cc->gdb_read_register, cc->gdb_write_register,
                              feature);
@@ -1644,7 +1659,7 @@ void gdb_extend_qsupported_features(char *qflags)
 static void handle_query_supported(GArray *params, void *user_ctx)
 {
     g_string_printf(gdbserver_state.str_buf, "PacketSize=%x", MAX_PACKET_LENGTH);
-    if (first_cpu->cc->gdb_core_xml_file) {
+    if (gdb_get_core_xml_file(first_cpu)) {
         g_string_append(gdbserver_state.str_buf, ";qXfer:features:read+");
     }
 
@@ -1701,7 +1716,7 @@ static void handle_query_xfer_features(GArray *params, void *user_ctx)
     }
 
     process = gdb_get_cpu_process(gdbserver_state.g_cpu);
-    if (!gdbserver_state.g_cpu->cc->gdb_core_xml_file) {
+    if (!gdb_get_core_xml_file(gdbserver_state.g_cpu)) {
         gdb_put_packet("");
         return;
     }
-- 
2.43.0

[PATCH for-10.1 3/9] target/arm: Handle AArch64 in TYPE_ARM_CPU gdb_arch_name

[Peter Maydell]

Instead of having the TYPE_AARCH64_CPU subclass set
CPUClass::gdb_arch_name to a different function, make the
TYPE_ARM_CPU implementation of the method handle AArch64.

For the moment we make the "is this AArch64?" function test "is the
CPU of TYPE_AARCH64_CPU?", so that this produces no behavioural
change.  When we've moved all the gdbstub related methods across to
the base class, we will be able to change this to be "does the CPU
have the ARM_FEATURE_AARCH64 feature?".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 6 ++++++
 target/arm/cpu.c       | 3 +++
 target/arm/cpu64.c     | 6 ------
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index bb962389192..a14c269fa5a 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1691,6 +1691,12 @@ void aarch64_add_sve_properties(Object *obj);
 void aarch64_add_sme_properties(Object *obj);
 #endif
 
+/* 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);
+}
+
 /* Read the CONTROL register as the MRS instruction would. */
 uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure);
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 01786ac7879..d69403fda90 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2647,6 +2647,9 @@ static const gchar *arm_gdb_arch_name(CPUState *cs)
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
+    if (arm_gdbstub_is_aarch64(cpu)) {
+        return "aarch64";
+    }
     if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
         return "iwmmxt";
     }
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 8188ede5cc8..020b32f21e9 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -813,11 +813,6 @@ static void aarch64_cpu_finalizefn(Object *obj)
 {
 }
 
-static const gchar *aarch64_gdb_arch_name(CPUState *cs)
-{
-    return "aarch64";
-}
-
 static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
@@ -825,7 +820,6 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_read_register = aarch64_cpu_gdb_read_register;
     cc->gdb_write_register = aarch64_cpu_gdb_write_register;
     cc->gdb_core_xml_file = "aarch64-core.xml";
-    cc->gdb_arch_name = aarch64_gdb_arch_name;
 
     object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
                                    aarch64_cpu_set_aarch64);
-- 
2.43.0

[PATCH for-10.1 4/9] target/arm: Handle gdb_core_xml_file in TYPE_ARM_CPU

[Peter Maydell]

Instead of having the TYPE_AARCH64_CPU subclass set
CPUClass:gdb_core_xml_file to a different value from that that
TYPE_ARM_CPU uses, implement the gdb_get_core_xml_file method in the
TYPE_ARM_CPU class to return either the AArch64 or AArch32 XML file
name.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c         | 16 +++++++++++++++-
 target/arm/cpu64.c       |  1 -
 target/arm/tcg/cpu-v7m.c |  1 -
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index d69403fda90..75d5df4879b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2656,6 +2656,20 @@ static const gchar *arm_gdb_arch_name(CPUState *cs)
     return "arm";
 }
 
+static const char *arm_gdb_get_core_xml_file(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    if (arm_gdbstub_is_aarch64(cpu)) {
+        return "aarch64-core.xml";
+    }
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        return "arm-m-profile.xml";
+    }
+    return "arm-core.xml";
+}
+
 #ifndef CONFIG_USER_ONLY
 #include "hw/core/sysemu-cpu-ops.h"
 
@@ -2721,6 +2735,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->sysemu_ops = &arm_sysemu_ops;
 #endif
     cc->gdb_arch_name = arm_gdb_arch_name;
+    cc->gdb_get_core_xml_file = arm_gdb_get_core_xml_file;
     cc->gdb_stop_before_watchpoint = true;
     cc->disas_set_info = arm_disas_set_info;
 
@@ -2743,7 +2758,6 @@ static void cpu_register_class_init(ObjectClass *oc, void *data)
     CPUClass *cc = CPU_CLASS(acc);
 
     acc->info = data;
-    cc->gdb_core_xml_file = "arm-core.xml";
     if (acc->info->deprecation_note) {
         cc->deprecation_note = acc->info->deprecation_note;
     }
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 020b32f21e9..3094df366ec 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -819,7 +819,6 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->gdb_read_register = aarch64_cpu_gdb_read_register;
     cc->gdb_write_register = aarch64_cpu_gdb_write_register;
-    cc->gdb_core_xml_file = "aarch64-core.xml";
 
     object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
                                    aarch64_cpu_set_aarch64);
diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
index c4dd3092726..8acaf860b68 100644
--- a/target/arm/tcg/cpu-v7m.c
+++ b/target/arm/tcg/cpu-v7m.c
@@ -261,7 +261,6 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
 
     acc->info = data;
     cc->tcg_ops = &arm_v7m_tcg_ops;
-    cc->gdb_core_xml_file = "arm-m-profile.xml";
 }
 
 static const ARMCPUInfo arm_v7m_cpus[] = {
-- 
2.43.0

[PATCH for-10.1 5/9] target/arm: Handle AArch64 gdb read/write regs in TYPE_ARM_CPU

[Peter Maydell]

Instead of having the TYPE_AARCH64_CPU subclass set
CPUClass::gdb_read_register and ::gdb_write_register to different
methods from those of the TYPE_ARM_CPU parent class, have the
TYPE_ARM_CPU methods handle either AArch32 or AArch64 at runtime.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu64.c   |  5 -----
 target/arm/gdbstub.c | 12 ++++++++++++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 3094df366ec..2f87df082cd 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -815,11 +815,6 @@ static void aarch64_cpu_finalizefn(Object *obj)
 
 static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
-    CPUClass *cc = CPU_CLASS(oc);
-
-    cc->gdb_read_register = aarch64_cpu_gdb_read_register;
-    cc->gdb_write_register = aarch64_cpu_gdb_write_register;
-
     object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
                                    aarch64_cpu_set_aarch64);
     object_class_property_set_description(oc, "aarch64",
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 30068c22627..ce4497ad7c3 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -44,6 +44,12 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
+#ifdef TARGET_AARCH64
+    if (arm_gdbstub_is_aarch64(cpu)) {
+        return aarch64_cpu_gdb_read_register(cs, mem_buf, n);
+    }
+#endif
+
     if (n < 16) {
         /* Core integer register.  */
         return gdb_get_reg32(mem_buf, env->regs[n]);
@@ -66,6 +72,12 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     CPUARMState *env = &cpu->env;
     uint32_t tmp;
 
+#ifdef TARGET_AARCH64
+    if (arm_gdbstub_is_aarch64(cpu)) {
+        return aarch64_cpu_gdb_write_register(cs, mem_buf, n);
+    }
+#endif
+
     tmp = ldl_p(mem_buf);
 
     /*
-- 
2.43.0

[PATCH for-10.1 6/9] 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>
---
 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 a14c269fa5a..a18d87fa28b 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1694,7 +1694,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 for-10.1 7/9] 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>
---
 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 75d5df4879b..9c6e8f5a935 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1608,6 +1608,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)
 {
     /*
@@ -1735,6 +1764,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 2f87df082cd..49cf06a7bdc 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, 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 for-10.1 8/9] 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>
---
 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 da30bdbb234..7418eb57537 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 for-10.1 9/9] 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>
---
 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 8f52380c88c..c9c53a6294b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1161,10 +1161,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 a18d87fa28b..0c24208f183 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -350,7 +350,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 49cf06a7bdc..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, 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, 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 = (void *)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 for-10.1 1/9] core/cpu.h: gdb_arch_name string should not be freed

[Alex Bennée]

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

> The documentation for the CPUClass::gdb_arch_name method claims that
> the returned string should be freed with g_free().  This is not
> correct: in commit a650683871ba728 we changed this method to
> instead return a simple constant string, but forgot to update
> the documentation.
>
> Make the documentation match the new semantics.
>
> Fixes: a650683871ba728 ("hw/core/cpu: Return static value with gdb_arch_name()")
> 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 for-10.1 2/9] gdbstub: Allow gdb_core_xml_file to be set at runtime

[Alex Bennée]

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

> Currently the CPUClass:gdb_core_xml_file setting is a simple 'const
> char *' which the CPU class must set to a fixed string.  Allow the
> CPU class to instead set a new method gdb_get_core_xml_file() which
> returns this string.
>
> This will allow Arm CPUs to use different XML files for AArch32 vs
> AArch64 without having to have an extra AArch64-specific class type
> purely to give somewhere to set cc->gdb_core_xml_file differently.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH for-10.1 3/9] target/arm: Handle AArch64 in TYPE_ARM_CPU gdb_arch_name

[Alex Bennée]

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

> Instead of having the TYPE_AARCH64_CPU subclass set
> CPUClass::gdb_arch_name to a different function, make the
> TYPE_ARM_CPU implementation of the method handle AArch64.
>
> For the moment we make the "is this AArch64?" function test "is the
> CPU of TYPE_AARCH64_CPU?", so that this produces no behavioural
> change.  When we've moved all the gdbstub related methods across to
> the base class, we will be able to change this to be "does the CPU
> have the ARM_FEATURE_AARCH64 feature?".
>
> 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 for-10.0 1/9] core/cpu.h: gdb_arch_name string should not be freed

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, Peter Maydell wrote:
> The documentation for the CPUClass::gdb_arch_name method claims that
> the returned string should be freed with g_free().  This is not
> correct: in commit a650683871ba728 we changed this method to
> instead return a simple constant string, but forgot to update
> the documentation.
> 
> Make the documentation match the new semantics.
> 
> Fixes: a650683871ba728 ("hw/core/cpu: Return static value with gdb_arch_name()")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/core/cpu.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

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

Re: [PATCH for-10.1 7/9] target/arm: Move aarch64 CPU property code to TYPE_ARM_CPU

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, 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>
> ---
>   target/arm/cpu.c   | 36 ++++++++++++++++++++++++++++++++++++
>   target/arm/cpu64.c | 33 ---------------------------------
>   2 files changed, 36 insertions(+), 33 deletions(-)

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

Re: [PATCH for-10.1 8/9] target/arm/kvm: don't check TYPE_AARCH64_CPU

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, 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>
> ---
>   target/arm/kvm.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

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

Re: [PATCH for-10.1 9/9] target/arm: Remove TYPE_AARCH64_CPU

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, 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>
> ---
>   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(-)

Thanks!

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

Re: [PATCH for-10.1 6/9] target/arm: Present AArch64 gdbstub based on ARM_FEATURE_AARCH64

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, 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>
> ---
>   target/arm/internals.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH for-10.1 5/9] target/arm: Handle AArch64 gdb read/write regs in TYPE_ARM_CPU

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, Peter Maydell wrote:
> Instead of having the TYPE_AARCH64_CPU subclass set
> CPUClass::gdb_read_register and ::gdb_write_register to different
> methods from those of the TYPE_ARM_CPU parent class, have the
> TYPE_ARM_CPU methods handle either AArch32 or AArch64 at runtime.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu64.c   |  5 -----
>   target/arm/gdbstub.c | 12 ++++++++++++
>   2 files changed, 12 insertions(+), 5 deletions(-)

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

Re: [PATCH for-10.1 6/9] target/arm: Present AArch64 gdbstub based on ARM_FEATURE_AARCH64

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, 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>
> ---
>   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 a14c269fa5a..a18d87fa28b 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1694,7 +1694,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)

4 uses, maybe worth removing (directly inlining) as a cleanup
patch on top of this conversion series.

>   {
> -    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. */

Re: [PATCH for-10.1 3/9] target/arm: Handle AArch64 in TYPE_ARM_CPU gdb_arch_name

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, Peter Maydell wrote:
> Instead of having the TYPE_AARCH64_CPU subclass set
> CPUClass::gdb_arch_name to a different function, make the
> TYPE_ARM_CPU implementation of the method handle AArch64.
> 
> For the moment we make the "is this AArch64?" function test "is the
> CPU of TYPE_AARCH64_CPU?", so that this produces no behavioural
> change.  When we've moved all the gdbstub related methods across to
> the base class, we will be able to change this to be "does the CPU
> have the ARM_FEATURE_AARCH64 feature?".
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/internals.h | 6 ++++++
>   target/arm/cpu.c       | 3 +++
>   target/arm/cpu64.c     | 6 ------
>   3 files changed, 9 insertions(+), 6 deletions(-)

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

Re: [PATCH for-10.1 4/9] target/arm: Handle gdb_core_xml_file in TYPE_ARM_CPU

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, Peter Maydell wrote:
> Instead of having the TYPE_AARCH64_CPU subclass set
> CPUClass:gdb_core_xml_file to a different value from that that
> TYPE_ARM_CPU uses, implement the gdb_get_core_xml_file method in the
> TYPE_ARM_CPU class to return either the AArch64 or AArch32 XML file
> name.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu.c         | 16 +++++++++++++++-
>   target/arm/cpu64.c       |  1 -
>   target/arm/tcg/cpu-v7m.c |  1 -
>   3 files changed, 15 insertions(+), 3 deletions(-)

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

Re: [PATCH for-10.1 0/9] target/arm: Remove TYPE_AARCH64_CPU class

[Philippe Mathieu-Daudé]

On 17/3/25 15:28, Peter Maydell wrote:

> This patchseries removes TYPE_AARCH64_CPU so that all CPUs,
> both AArch32 and AArch64, directly inherit from TYPE_ARM_CPU.

> Peter Maydell (9):
>    core/cpu.h: gdb_arch_name string should not be freed
>    gdbstub: Allow gdb_core_xml_file to be set at runtime
>    target/arm: Handle AArch64 in TYPE_ARM_CPU gdb_arch_name
>    target/arm: Handle gdb_core_xml_file in TYPE_ARM_CPU
>    target/arm: Handle AArch64 gdb read/write regs in TYPE_ARM_CPU
>    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

Series queued, thanks!

Re: [PATCH for-10.1 6/9] target/arm: Present AArch64 gdbstub based on ARM_FEATURE_AARCH64

[Philippe Mathieu-Daudé]

Hi Peter,

On 17/3/25 15:28, 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>
> ---
>   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 a14c269fa5a..a18d87fa28b 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1694,7 +1694,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);

Unfortunately this doesn't work well: while a Aarch64 CPU is of type
TYPE_AARCH64_CPU right after being instantiated (not yet QOM realized),
the features are only finalized during arm_cpu_instance_init():

static void arm_cpu_instance_init(Object *obj)
{
     ARMCPUClass *acc = ARM_CPU_GET_CLASS(obj);

     acc->info->initfn(obj);
     arm_cpu_post_init(obj);
}

void arm_cpu_post_init(Object *obj)
{
     ARMCPU *cpu = ARM_CPU(obj);

     /*
      * Some features imply others. Figure this out now, because we
      * are going to look at the feature bits in deciding which
      * properties to add.
      */
     arm_cpu_propagate_feature_implications(cpu);
     ...
}

The GDB feature checks are done earlier:

   object_init_with_type
    -> cpu_common_initfn
        -> gdb_init_cpu
           -> gdb_get_core_xml_file
              -> arm_gdb_get_core_xml_file
                 -> arm_gdbstub_is_aarch64

At this point the feature set is empty, triggering the
assertion in gdb_find_static_feature():

$ ./build/qemu-aarch64 build/tests/tcg/aarch64-linux-user/semihosting
**
ERROR:../../gdbstub/gdbstub.c:494:gdb_find_static_feature: code should 
not be reached
Bail out! ERROR:../../gdbstub/gdbstub.c:494:gdb_find_static_feature: 
code should not be reached
Aborted (core dumped)

I suppose gdb_init_cpu() needs more splitting work. For now I'll drop
this patches 5+ from my queue.

Regards,

Phil.

Re: [PATCH for-10.1 6/9] target/arm: Present AArch64 gdbstub based on ARM_FEATURE_AARCH64

[Peter Maydell]

On Fri, 25 Apr 2025 at 15:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 17/3/25 15:28, 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>
> > ---
> >   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 a14c269fa5a..a18d87fa28b 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -1694,7 +1694,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);
>
> Unfortunately this doesn't work well: while a Aarch64 CPU is of type
> TYPE_AARCH64_CPU right after being instantiated (not yet QOM realized),
> the features are only finalized during arm_cpu_instance_init():

Thanks for finding this. The problem is that gdb_init_cpu() is
being called as part of the base-class instance init, so the
CPU object isn't fully instantiated yet -- it hasn't even run
the aarch64_max_initfn() function yet. So gdb_init_cpu() ends
up calling CPU-specific methods on a half-instantiated object,
which is why it crashes.

But we do need to move things so they happen after realize,
because up til realize the CPU properties might be changed
(otherwise we give the wrong answer for the aarch64=off case).
Except that part of the CPU subclass realize involves calling
gdb_register_coprocessor(), which needs to happen after we've
set up the core regs for gdb.

I think that moving the call to cpu_exec_realizefn() would
work (this gets called near the start of realize, so before
the subclass realize decides to add more gdb registers).
But I'll have to check whether that would be wrong for
some other architecture, and maybe there's a neater way
to do this.

-- PMM