[PATCH v2 0/3] target/arm: Allow compilation without CONFIG_ARM_V7M

[PATCH v2 0/3] target/arm: Allow compilation without CONFIG_ARM_V7M

[Thomas Huth]

We've got a switch to disable v7m code since a long time - but it
currently cannot be disabled since linking then fails due to missing
functions. But thanks to the clean-ups that have been done during the
past years, it's not that difficult anymore to finally make it possible
to disable CONFIG_ARM_V7M: We just have to move some v7m-related code
out of cpu32.c to a separate file (that we only compile if the switch
CONFIG_ARM_V7M is enabled) and make sure to use the stub functions in
m_helper.c if it is disabled. Then we can finally remove the hard-coded
"select ARM_V7M" from the Kconfig file.

v2:
- Updated a comment
- Avoid #ifdef in cpu-v7m.c, handle it via meson.build instead

Thomas Huth (3):
  target/arm: Move v7m-related code from cpu32.c into a separate file
  target/arm/tcg/m_helper.c: Include the full helpers only with
    CONFIG_ARM_V7M
  target/arm/Kconfig: Stop requiring CONFIG_ARM_V7M

 target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
 target/arm/tcg/cpu32.c     | 261 ---------------------------------
 target/arm/tcg/m_helper.c  |   3 +-
 target/arm/Kconfig         |   4 -
 target/arm/meson.build     |   3 +
 target/arm/tcg/meson.build |   3 +
 6 files changed, 298 insertions(+), 266 deletions(-)
 create mode 100644 target/arm/tcg/cpu-v7m.c

-- 
2.43.0

[PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Thomas Huth]

Move the code to a separate file so that we do not have to compile
it anymore if CONFIG_ARM_V7M is not set.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
 target/arm/tcg/cpu32.c     | 261 ---------------------------------
 target/arm/meson.build     |   3 +
 target/arm/tcg/meson.build |   3 +
 4 files changed, 296 insertions(+), 261 deletions(-)
 create mode 100644 target/arm/tcg/cpu-v7m.c

diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
new file mode 100644
index 0000000000..89a25444a2
--- /dev/null
+++ b/target/arm/tcg/cpu-v7m.c
@@ -0,0 +1,290 @@
+/*
+ * QEMU ARMv7-M TCG-only CPUs.
+ *
+ * Copyright (c) 2012 SUSE LINUX Products GmbH
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "hw/core/tcg-cpu-ops.h"
+#include "internals.h"
+
+#if !defined(CONFIG_USER_ONLY)
+
+#include "hw/intc/armv7m_nvic.h"
+
+static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
+{
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    bool ret = false;
+
+    /*
+     * ARMv7-M interrupt masking works differently than -A or -R.
+     * There is no FIQ/IRQ distinction. Instead of I and F bits
+     * masking FIQ and IRQ interrupts, an exception is taken only
+     * if it is higher priority than the current execution priority
+     * (which depends on state like BASEPRI, FAULTMASK and the
+     * currently active exception).
+     */
+    if (interrupt_request & CPU_INTERRUPT_HARD
+        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
+        cs->exception_index = EXCP_IRQ;
+        cc->tcg_ops->do_interrupt(cs);
+        ret = true;
+    }
+    return ret;
+}
+
+#endif /* !CONFIG_USER_ONLY */
+
+static void cortex_m0_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    set_feature(&cpu->env, ARM_FEATURE_V6);
+    set_feature(&cpu->env, ARM_FEATURE_M);
+
+    cpu->midr = 0x410cc200;
+
+    /*
+     * These ID register values are not guest visible, because
+     * we do not implement the Main Extension. They must be set
+     * to values corresponding to the Cortex-M0's implemented
+     * features, because QEMU generally controls its emulation
+     * by looking at ID register fields. We use the same values as
+     * for the M3.
+     */
+    cpu->isar.id_pfr0 = 0x00000030;
+    cpu->isar.id_pfr1 = 0x00000200;
+    cpu->isar.id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->isar.id_mmfr0 = 0x00000030;
+    cpu->isar.id_mmfr1 = 0x00000000;
+    cpu->isar.id_mmfr2 = 0x00000000;
+    cpu->isar.id_mmfr3 = 0x00000000;
+    cpu->isar.id_isar0 = 0x01141110;
+    cpu->isar.id_isar1 = 0x02111000;
+    cpu->isar.id_isar2 = 0x21112231;
+    cpu->isar.id_isar3 = 0x01111110;
+    cpu->isar.id_isar4 = 0x01310102;
+    cpu->isar.id_isar5 = 0x00000000;
+    cpu->isar.id_isar6 = 0x00000000;
+}
+
+static void cortex_m3_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
+    cpu->midr = 0x410fc231;
+    cpu->pmsav7_dregion = 8;
+    cpu->isar.id_pfr0 = 0x00000030;
+    cpu->isar.id_pfr1 = 0x00000200;
+    cpu->isar.id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->isar.id_mmfr0 = 0x00000030;
+    cpu->isar.id_mmfr1 = 0x00000000;
+    cpu->isar.id_mmfr2 = 0x00000000;
+    cpu->isar.id_mmfr3 = 0x00000000;
+    cpu->isar.id_isar0 = 0x01141110;
+    cpu->isar.id_isar1 = 0x02111000;
+    cpu->isar.id_isar2 = 0x21112231;
+    cpu->isar.id_isar3 = 0x01111110;
+    cpu->isar.id_isar4 = 0x01310102;
+    cpu->isar.id_isar5 = 0x00000000;
+    cpu->isar.id_isar6 = 0x00000000;
+}
+
+static void cortex_m4_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
+    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
+    cpu->midr = 0x410fc240; /* r0p0 */
+    cpu->pmsav7_dregion = 8;
+    cpu->isar.mvfr0 = 0x10110021;
+    cpu->isar.mvfr1 = 0x11000011;
+    cpu->isar.mvfr2 = 0x00000000;
+    cpu->isar.id_pfr0 = 0x00000030;
+    cpu->isar.id_pfr1 = 0x00000200;
+    cpu->isar.id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->isar.id_mmfr0 = 0x00000030;
+    cpu->isar.id_mmfr1 = 0x00000000;
+    cpu->isar.id_mmfr2 = 0x00000000;
+    cpu->isar.id_mmfr3 = 0x00000000;
+    cpu->isar.id_isar0 = 0x01141110;
+    cpu->isar.id_isar1 = 0x02111000;
+    cpu->isar.id_isar2 = 0x21112231;
+    cpu->isar.id_isar3 = 0x01111110;
+    cpu->isar.id_isar4 = 0x01310102;
+    cpu->isar.id_isar5 = 0x00000000;
+    cpu->isar.id_isar6 = 0x00000000;
+}
+
+static void cortex_m7_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
+    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
+    cpu->midr = 0x411fc272; /* r1p2 */
+    cpu->pmsav7_dregion = 8;
+    cpu->isar.mvfr0 = 0x10110221;
+    cpu->isar.mvfr1 = 0x12000011;
+    cpu->isar.mvfr2 = 0x00000040;
+    cpu->isar.id_pfr0 = 0x00000030;
+    cpu->isar.id_pfr1 = 0x00000200;
+    cpu->isar.id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->isar.id_mmfr0 = 0x00100030;
+    cpu->isar.id_mmfr1 = 0x00000000;
+    cpu->isar.id_mmfr2 = 0x01000000;
+    cpu->isar.id_mmfr3 = 0x00000000;
+    cpu->isar.id_isar0 = 0x01101110;
+    cpu->isar.id_isar1 = 0x02112000;
+    cpu->isar.id_isar2 = 0x20232231;
+    cpu->isar.id_isar3 = 0x01111131;
+    cpu->isar.id_isar4 = 0x01310132;
+    cpu->isar.id_isar5 = 0x00000000;
+    cpu->isar.id_isar6 = 0x00000000;
+}
+
+static void cortex_m33_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    set_feature(&cpu->env, ARM_FEATURE_V8);
+    set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
+    set_feature(&cpu->env, ARM_FEATURE_M_SECURITY);
+    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
+    cpu->midr = 0x410fd213; /* r0p3 */
+    cpu->pmsav7_dregion = 16;
+    cpu->sau_sregion = 8;
+    cpu->isar.mvfr0 = 0x10110021;
+    cpu->isar.mvfr1 = 0x11000011;
+    cpu->isar.mvfr2 = 0x00000040;
+    cpu->isar.id_pfr0 = 0x00000030;
+    cpu->isar.id_pfr1 = 0x00000210;
+    cpu->isar.id_dfr0 = 0x00200000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->isar.id_mmfr0 = 0x00101F40;
+    cpu->isar.id_mmfr1 = 0x00000000;
+    cpu->isar.id_mmfr2 = 0x01000000;
+    cpu->isar.id_mmfr3 = 0x00000000;
+    cpu->isar.id_isar0 = 0x01101110;
+    cpu->isar.id_isar1 = 0x02212000;
+    cpu->isar.id_isar2 = 0x20232232;
+    cpu->isar.id_isar3 = 0x01111131;
+    cpu->isar.id_isar4 = 0x01310132;
+    cpu->isar.id_isar5 = 0x00000000;
+    cpu->isar.id_isar6 = 0x00000000;
+    cpu->clidr = 0x00000000;
+    cpu->ctr = 0x8000c000;
+}
+
+static void cortex_m55_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    set_feature(&cpu->env, ARM_FEATURE_V8);
+    set_feature(&cpu->env, ARM_FEATURE_V8_1M);
+    set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
+    set_feature(&cpu->env, ARM_FEATURE_M_SECURITY);
+    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
+    cpu->midr = 0x410fd221; /* r0p1 */
+    cpu->revidr = 0;
+    cpu->pmsav7_dregion = 16;
+    cpu->sau_sregion = 8;
+    /* These are the MVFR* values for the FPU + full MVE configuration */
+    cpu->isar.mvfr0 = 0x10110221;
+    cpu->isar.mvfr1 = 0x12100211;
+    cpu->isar.mvfr2 = 0x00000040;
+    cpu->isar.id_pfr0 = 0x20000030;
+    cpu->isar.id_pfr1 = 0x00000230;
+    cpu->isar.id_dfr0 = 0x10200000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->isar.id_mmfr0 = 0x00111040;
+    cpu->isar.id_mmfr1 = 0x00000000;
+    cpu->isar.id_mmfr2 = 0x01000000;
+    cpu->isar.id_mmfr3 = 0x00000011;
+    cpu->isar.id_isar0 = 0x01103110;
+    cpu->isar.id_isar1 = 0x02212000;
+    cpu->isar.id_isar2 = 0x20232232;
+    cpu->isar.id_isar3 = 0x01111131;
+    cpu->isar.id_isar4 = 0x01310132;
+    cpu->isar.id_isar5 = 0x00000000;
+    cpu->isar.id_isar6 = 0x00000000;
+    cpu->clidr = 0x00000000; /* caches not implemented */
+    cpu->ctr = 0x8303c003;
+}
+
+static const struct TCGCPUOps arm_v7m_tcg_ops = {
+    .initialize = arm_translate_init,
+    .synchronize_from_tb = arm_cpu_synchronize_from_tb,
+    .debug_excp_handler = arm_debug_excp_handler,
+    .restore_state_to_opc = arm_restore_state_to_opc,
+
+#ifdef CONFIG_USER_ONLY
+    .record_sigsegv = arm_cpu_record_sigsegv,
+    .record_sigbus = arm_cpu_record_sigbus,
+#else
+    .tlb_fill = arm_cpu_tlb_fill,
+    .cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt,
+    .do_interrupt = arm_v7m_cpu_do_interrupt,
+    .do_transaction_failed = arm_cpu_do_transaction_failed,
+    .do_unaligned_access = arm_cpu_do_unaligned_access,
+    .adjust_watchpoint_address = arm_adjust_watchpoint_address,
+    .debug_check_watchpoint = arm_debug_check_watchpoint,
+    .debug_check_breakpoint = arm_debug_check_breakpoint,
+#endif /* !CONFIG_USER_ONLY */
+};
+
+static void arm_v7m_class_init(ObjectClass *oc, void *data)
+{
+    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
+    CPUClass *cc = CPU_CLASS(oc);
+
+    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[] = {
+    { .name = "cortex-m0",   .initfn = cortex_m0_initfn,
+                             .class_init = arm_v7m_class_init },
+    { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
+                             .class_init = arm_v7m_class_init },
+    { .name = "cortex-m4",   .initfn = cortex_m4_initfn,
+                             .class_init = arm_v7m_class_init },
+    { .name = "cortex-m7",   .initfn = cortex_m7_initfn,
+                             .class_init = arm_v7m_class_init },
+    { .name = "cortex-m33",  .initfn = cortex_m33_initfn,
+                             .class_init = arm_v7m_class_init },
+    { .name = "cortex-m55",  .initfn = cortex_m55_initfn,
+                             .class_init = arm_v7m_class_init },
+};
+
+static void arm_v7m_cpu_register_types(void)
+{
+    size_t i;
+
+    for (i = 0; i < ARRAY_SIZE(arm_v7m_cpus); ++i) {
+        arm_cpu_register(&arm_v7m_cpus[i]);
+    }
+}
+
+type_init(arm_v7m_cpu_register_types)
diff --git a/target/arm/tcg/cpu32.c b/target/arm/tcg/cpu32.c
index d9e0e2a4dd..7132dc4b6f 100644
--- a/target/arm/tcg/cpu32.c
+++ b/target/arm/tcg/cpu32.c
@@ -17,9 +17,6 @@
 #include "hw/boards.h"
 #endif
 #include "cpregs.h"
-#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
-#include "hw/intc/armv7m_nvic.h"
-#endif
 
 
 /* Share AArch32 -cpu max features with AArch64. */
@@ -98,32 +95,6 @@ void aa32_max_features(ARMCPU *cpu)
 /* CPU models. These are not needed for the AArch64 linux-user build. */
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
 
-#if !defined(CONFIG_USER_ONLY)
-static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
-{
-    CPUClass *cc = CPU_GET_CLASS(cs);
-    ARMCPU *cpu = ARM_CPU(cs);
-    CPUARMState *env = &cpu->env;
-    bool ret = false;
-
-    /*
-     * ARMv7-M interrupt masking works differently than -A or -R.
-     * There is no FIQ/IRQ distinction. Instead of I and F bits
-     * masking FIQ and IRQ interrupts, an exception is taken only
-     * if it is higher priority than the current execution priority
-     * (which depends on state like BASEPRI, FAULTMASK and the
-     * currently active exception).
-     */
-    if (interrupt_request & CPU_INTERRUPT_HARD
-        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
-        cs->exception_index = EXCP_IRQ;
-        cc->tcg_ops->do_interrupt(cs);
-        ret = true;
-    }
-    return ret;
-}
-#endif /* !CONFIG_USER_ONLY */
-
 static void arm926_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -571,195 +542,6 @@ static void cortex_a15_initfn(Object *obj)
     define_arm_cp_regs(cpu, cortexa15_cp_reginfo);
 }
 
-static void cortex_m0_initfn(Object *obj)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-    set_feature(&cpu->env, ARM_FEATURE_V6);
-    set_feature(&cpu->env, ARM_FEATURE_M);
-
-    cpu->midr = 0x410cc200;
-
-    /*
-     * These ID register values are not guest visible, because
-     * we do not implement the Main Extension. They must be set
-     * to values corresponding to the Cortex-M0's implemented
-     * features, because QEMU generally controls its emulation
-     * by looking at ID register fields. We use the same values as
-     * for the M3.
-     */
-    cpu->isar.id_pfr0 = 0x00000030;
-    cpu->isar.id_pfr1 = 0x00000200;
-    cpu->isar.id_dfr0 = 0x00100000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x00000030;
-    cpu->isar.id_mmfr1 = 0x00000000;
-    cpu->isar.id_mmfr2 = 0x00000000;
-    cpu->isar.id_mmfr3 = 0x00000000;
-    cpu->isar.id_isar0 = 0x01141110;
-    cpu->isar.id_isar1 = 0x02111000;
-    cpu->isar.id_isar2 = 0x21112231;
-    cpu->isar.id_isar3 = 0x01111110;
-    cpu->isar.id_isar4 = 0x01310102;
-    cpu->isar.id_isar5 = 0x00000000;
-    cpu->isar.id_isar6 = 0x00000000;
-}
-
-static void cortex_m3_initfn(Object *obj)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-    set_feature(&cpu->env, ARM_FEATURE_V7);
-    set_feature(&cpu->env, ARM_FEATURE_M);
-    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
-    cpu->midr = 0x410fc231;
-    cpu->pmsav7_dregion = 8;
-    cpu->isar.id_pfr0 = 0x00000030;
-    cpu->isar.id_pfr1 = 0x00000200;
-    cpu->isar.id_dfr0 = 0x00100000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x00000030;
-    cpu->isar.id_mmfr1 = 0x00000000;
-    cpu->isar.id_mmfr2 = 0x00000000;
-    cpu->isar.id_mmfr3 = 0x00000000;
-    cpu->isar.id_isar0 = 0x01141110;
-    cpu->isar.id_isar1 = 0x02111000;
-    cpu->isar.id_isar2 = 0x21112231;
-    cpu->isar.id_isar3 = 0x01111110;
-    cpu->isar.id_isar4 = 0x01310102;
-    cpu->isar.id_isar5 = 0x00000000;
-    cpu->isar.id_isar6 = 0x00000000;
-}
-
-static void cortex_m4_initfn(Object *obj)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    set_feature(&cpu->env, ARM_FEATURE_V7);
-    set_feature(&cpu->env, ARM_FEATURE_M);
-    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
-    cpu->midr = 0x410fc240; /* r0p0 */
-    cpu->pmsav7_dregion = 8;
-    cpu->isar.mvfr0 = 0x10110021;
-    cpu->isar.mvfr1 = 0x11000011;
-    cpu->isar.mvfr2 = 0x00000000;
-    cpu->isar.id_pfr0 = 0x00000030;
-    cpu->isar.id_pfr1 = 0x00000200;
-    cpu->isar.id_dfr0 = 0x00100000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x00000030;
-    cpu->isar.id_mmfr1 = 0x00000000;
-    cpu->isar.id_mmfr2 = 0x00000000;
-    cpu->isar.id_mmfr3 = 0x00000000;
-    cpu->isar.id_isar0 = 0x01141110;
-    cpu->isar.id_isar1 = 0x02111000;
-    cpu->isar.id_isar2 = 0x21112231;
-    cpu->isar.id_isar3 = 0x01111110;
-    cpu->isar.id_isar4 = 0x01310102;
-    cpu->isar.id_isar5 = 0x00000000;
-    cpu->isar.id_isar6 = 0x00000000;
-}
-
-static void cortex_m7_initfn(Object *obj)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    set_feature(&cpu->env, ARM_FEATURE_V7);
-    set_feature(&cpu->env, ARM_FEATURE_M);
-    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
-    cpu->midr = 0x411fc272; /* r1p2 */
-    cpu->pmsav7_dregion = 8;
-    cpu->isar.mvfr0 = 0x10110221;
-    cpu->isar.mvfr1 = 0x12000011;
-    cpu->isar.mvfr2 = 0x00000040;
-    cpu->isar.id_pfr0 = 0x00000030;
-    cpu->isar.id_pfr1 = 0x00000200;
-    cpu->isar.id_dfr0 = 0x00100000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x00100030;
-    cpu->isar.id_mmfr1 = 0x00000000;
-    cpu->isar.id_mmfr2 = 0x01000000;
-    cpu->isar.id_mmfr3 = 0x00000000;
-    cpu->isar.id_isar0 = 0x01101110;
-    cpu->isar.id_isar1 = 0x02112000;
-    cpu->isar.id_isar2 = 0x20232231;
-    cpu->isar.id_isar3 = 0x01111131;
-    cpu->isar.id_isar4 = 0x01310132;
-    cpu->isar.id_isar5 = 0x00000000;
-    cpu->isar.id_isar6 = 0x00000000;
-}
-
-static void cortex_m33_initfn(Object *obj)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    set_feature(&cpu->env, ARM_FEATURE_V8);
-    set_feature(&cpu->env, ARM_FEATURE_M);
-    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
-    set_feature(&cpu->env, ARM_FEATURE_M_SECURITY);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
-    cpu->midr = 0x410fd213; /* r0p3 */
-    cpu->pmsav7_dregion = 16;
-    cpu->sau_sregion = 8;
-    cpu->isar.mvfr0 = 0x10110021;
-    cpu->isar.mvfr1 = 0x11000011;
-    cpu->isar.mvfr2 = 0x00000040;
-    cpu->isar.id_pfr0 = 0x00000030;
-    cpu->isar.id_pfr1 = 0x00000210;
-    cpu->isar.id_dfr0 = 0x00200000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x00101F40;
-    cpu->isar.id_mmfr1 = 0x00000000;
-    cpu->isar.id_mmfr2 = 0x01000000;
-    cpu->isar.id_mmfr3 = 0x00000000;
-    cpu->isar.id_isar0 = 0x01101110;
-    cpu->isar.id_isar1 = 0x02212000;
-    cpu->isar.id_isar2 = 0x20232232;
-    cpu->isar.id_isar3 = 0x01111131;
-    cpu->isar.id_isar4 = 0x01310132;
-    cpu->isar.id_isar5 = 0x00000000;
-    cpu->isar.id_isar6 = 0x00000000;
-    cpu->clidr = 0x00000000;
-    cpu->ctr = 0x8000c000;
-}
-
-static void cortex_m55_initfn(Object *obj)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    set_feature(&cpu->env, ARM_FEATURE_V8);
-    set_feature(&cpu->env, ARM_FEATURE_V8_1M);
-    set_feature(&cpu->env, ARM_FEATURE_M);
-    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
-    set_feature(&cpu->env, ARM_FEATURE_M_SECURITY);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
-    cpu->midr = 0x410fd221; /* r0p1 */
-    cpu->revidr = 0;
-    cpu->pmsav7_dregion = 16;
-    cpu->sau_sregion = 8;
-    /* These are the MVFR* values for the FPU + full MVE configuration */
-    cpu->isar.mvfr0 = 0x10110221;
-    cpu->isar.mvfr1 = 0x12100211;
-    cpu->isar.mvfr2 = 0x00000040;
-    cpu->isar.id_pfr0 = 0x20000030;
-    cpu->isar.id_pfr1 = 0x00000230;
-    cpu->isar.id_dfr0 = 0x10200000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x00111040;
-    cpu->isar.id_mmfr1 = 0x00000000;
-    cpu->isar.id_mmfr2 = 0x01000000;
-    cpu->isar.id_mmfr3 = 0x00000011;
-    cpu->isar.id_isar0 = 0x01103110;
-    cpu->isar.id_isar1 = 0x02212000;
-    cpu->isar.id_isar2 = 0x20232232;
-    cpu->isar.id_isar3 = 0x01111131;
-    cpu->isar.id_isar4 = 0x01310132;
-    cpu->isar.id_isar5 = 0x00000000;
-    cpu->isar.id_isar6 = 0x00000000;
-    cpu->clidr = 0x00000000; /* caches not implemented */
-    cpu->ctr = 0x8303c003;
-}
-
 static const ARMCPRegInfo cortexr5_cp_reginfo[] = {
     /* Dummy the TCM region regs for the moment */
     { .name = "ATCM", .cp = 15, .opc1 = 0, .crn = 9, .crm = 1, .opc2 = 0,
@@ -1018,37 +800,6 @@ static void pxa270c5_initfn(Object *obj)
     cpu->reset_sctlr = 0x00000078;
 }
 
-static const struct TCGCPUOps arm_v7m_tcg_ops = {
-    .initialize = arm_translate_init,
-    .synchronize_from_tb = arm_cpu_synchronize_from_tb,
-    .debug_excp_handler = arm_debug_excp_handler,
-    .restore_state_to_opc = arm_restore_state_to_opc,
-
-#ifdef CONFIG_USER_ONLY
-    .record_sigsegv = arm_cpu_record_sigsegv,
-    .record_sigbus = arm_cpu_record_sigbus,
-#else
-    .tlb_fill = arm_cpu_tlb_fill,
-    .cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt,
-    .do_interrupt = arm_v7m_cpu_do_interrupt,
-    .do_transaction_failed = arm_cpu_do_transaction_failed,
-    .do_unaligned_access = arm_cpu_do_unaligned_access,
-    .adjust_watchpoint_address = arm_adjust_watchpoint_address,
-    .debug_check_watchpoint = arm_debug_check_watchpoint,
-    .debug_check_breakpoint = arm_debug_check_breakpoint,
-#endif /* !CONFIG_USER_ONLY */
-};
-
-static void arm_v7m_class_init(ObjectClass *oc, void *data)
-{
-    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
-    CPUClass *cc = CPU_CLASS(oc);
-
-    acc->info = data;
-    cc->tcg_ops = &arm_v7m_tcg_ops;
-    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.
@@ -1131,18 +882,6 @@ static const ARMCPUInfo arm_tcg_cpus[] = {
     { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
     { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
     { .name = "cortex-a15",  .initfn = cortex_a15_initfn },
-    { .name = "cortex-m0",   .initfn = cortex_m0_initfn,
-                             .class_init = arm_v7m_class_init },
-    { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
-                             .class_init = arm_v7m_class_init },
-    { .name = "cortex-m4",   .initfn = cortex_m4_initfn,
-                             .class_init = arm_v7m_class_init },
-    { .name = "cortex-m7",   .initfn = cortex_m7_initfn,
-                             .class_init = arm_v7m_class_init },
-    { .name = "cortex-m33",  .initfn = cortex_m33_initfn,
-                             .class_init = arm_v7m_class_init },
-    { .name = "cortex-m55",  .initfn = cortex_m55_initfn,
-                             .class_init = arm_v7m_class_init },
     { .name = "cortex-r5",   .initfn = cortex_r5_initfn },
     { .name = "cortex-r5f",  .initfn = cortex_r5f_initfn },
     { .name = "cortex-r52",  .initfn = cortex_r52_initfn },
diff --git a/target/arm/meson.build b/target/arm/meson.build
index 46b5a21eb3..2e10464dbb 100644
--- a/target/arm/meson.build
+++ b/target/arm/meson.build
@@ -26,6 +26,8 @@ arm_system_ss.add(files(
   'ptw.c',
 ))
 
+arm_user_ss = ss.source_set()
+
 subdir('hvf')
 
 if 'CONFIG_TCG' in config_all_accel
@@ -36,3 +38,4 @@ endif
 
 target_arch += {'arm': arm_ss}
 target_system_arch += {'arm': arm_system_ss}
+target_user_arch += {'arm': arm_user_ss}
diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
index 6fca38f2cc..3b1a9f0fc5 100644
--- a/target/arm/tcg/meson.build
+++ b/target/arm/tcg/meson.build
@@ -55,3 +55,6 @@ arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
 arm_system_ss.add(files(
   'psci.c',
 ))
+
+arm_system_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('cpu-v7m.c'))
+arm_user_ss.add(when: 'TARGET_AARCH64', if_false: files('cpu-v7m.c'))
-- 
2.43.0

Re: [PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Paolo Bonzini]

On 1/29/24 09:18, Thomas Huth wrote:
> Move the code to a separate file so that we do not have to compile
> it anymore if CONFIG_ARM_V7M is not set.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
>   target/arm/tcg/cpu32.c     | 261 ---------------------------------
>   target/arm/meson.build     |   3 +
>   target/arm/tcg/meson.build |   3 +
>   4 files changed, 296 insertions(+), 261 deletions(-)
>   create mode 100644 target/arm/tcg/cpu-v7m.c
> 
> diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
> new file mode 100644
> index 0000000000..89a25444a2
> --- /dev/null
> +++ b/target/arm/tcg/cpu-v7m.c
> @@ -0,0 +1,290 @@
> +/*
> + * QEMU ARMv7-M TCG-only CPUs.
> + *
> + * Copyright (c) 2012 SUSE LINUX Products GmbH
> + *
> + * This code is licensed under the GNU GPL v2 or later.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/core/tcg-cpu-ops.h"
> +#include "internals.h"
> +
> +#if !defined(CONFIG_USER_ONLY)
> +
> +#include "hw/intc/armv7m_nvic.h"
> +
> +static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    bool ret = false;
> +
> +    /*
> +     * ARMv7-M interrupt masking works differently than -A or -R.
> +     * There is no FIQ/IRQ distinction. Instead of I and F bits
> +     * masking FIQ and IRQ interrupts, an exception is taken only
> +     * if it is higher priority than the current execution priority
> +     * (which depends on state like BASEPRI, FAULTMASK and the
> +     * currently active exception).
> +     */
> +    if (interrupt_request & CPU_INTERRUPT_HARD
> +        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
> +        cs->exception_index = EXCP_IRQ;
> +        cc->tcg_ops->do_interrupt(cs);
> +        ret = true;
> +    }
> +    return ret;
> +}
> +
> +#endif /* !CONFIG_USER_ONLY */
> +
> +static void cortex_m0_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    set_feature(&cpu->env, ARM_FEATURE_V6);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +
> +    cpu->midr = 0x410cc200;
> +
> +    /*
> +     * These ID register values are not guest visible, because
> +     * we do not implement the Main Extension. They must be set
> +     * to values corresponding to the Cortex-M0's implemented
> +     * features, because QEMU generally controls its emulation
> +     * by looking at ID register fields. We use the same values as
> +     * for the M3.
> +     */
> +    cpu->isar.id_pfr0 = 0x00000030;
> +    cpu->isar.id_pfr1 = 0x00000200;
> +    cpu->isar.id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x00000030;
> +    cpu->isar.id_mmfr1 = 0x00000000;
> +    cpu->isar.id_mmfr2 = 0x00000000;
> +    cpu->isar.id_mmfr3 = 0x00000000;
> +    cpu->isar.id_isar0 = 0x01141110;
> +    cpu->isar.id_isar1 = 0x02111000;
> +    cpu->isar.id_isar2 = 0x21112231;
> +    cpu->isar.id_isar3 = 0x01111110;
> +    cpu->isar.id_isar4 = 0x01310102;
> +    cpu->isar.id_isar5 = 0x00000000;
> +    cpu->isar.id_isar6 = 0x00000000;
> +}
> +
> +static void cortex_m3_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
> +    cpu->midr = 0x410fc231;
> +    cpu->pmsav7_dregion = 8;
> +    cpu->isar.id_pfr0 = 0x00000030;
> +    cpu->isar.id_pfr1 = 0x00000200;
> +    cpu->isar.id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x00000030;
> +    cpu->isar.id_mmfr1 = 0x00000000;
> +    cpu->isar.id_mmfr2 = 0x00000000;
> +    cpu->isar.id_mmfr3 = 0x00000000;
> +    cpu->isar.id_isar0 = 0x01141110;
> +    cpu->isar.id_isar1 = 0x02111000;
> +    cpu->isar.id_isar2 = 0x21112231;
> +    cpu->isar.id_isar3 = 0x01111110;
> +    cpu->isar.id_isar4 = 0x01310102;
> +    cpu->isar.id_isar5 = 0x00000000;
> +    cpu->isar.id_isar6 = 0x00000000;
> +}
> +
> +static void cortex_m4_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
> +    cpu->midr = 0x410fc240; /* r0p0 */
> +    cpu->pmsav7_dregion = 8;
> +    cpu->isar.mvfr0 = 0x10110021;
> +    cpu->isar.mvfr1 = 0x11000011;
> +    cpu->isar.mvfr2 = 0x00000000;
> +    cpu->isar.id_pfr0 = 0x00000030;
> +    cpu->isar.id_pfr1 = 0x00000200;
> +    cpu->isar.id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x00000030;
> +    cpu->isar.id_mmfr1 = 0x00000000;
> +    cpu->isar.id_mmfr2 = 0x00000000;
> +    cpu->isar.id_mmfr3 = 0x00000000;
> +    cpu->isar.id_isar0 = 0x01141110;
> +    cpu->isar.id_isar1 = 0x02111000;
> +    cpu->isar.id_isar2 = 0x21112231;
> +    cpu->isar.id_isar3 = 0x01111110;
> +    cpu->isar.id_isar4 = 0x01310102;
> +    cpu->isar.id_isar5 = 0x00000000;
> +    cpu->isar.id_isar6 = 0x00000000;
> +}
> +
> +static void cortex_m7_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
> +    cpu->midr = 0x411fc272; /* r1p2 */
> +    cpu->pmsav7_dregion = 8;
> +    cpu->isar.mvfr0 = 0x10110221;
> +    cpu->isar.mvfr1 = 0x12000011;
> +    cpu->isar.mvfr2 = 0x00000040;
> +    cpu->isar.id_pfr0 = 0x00000030;
> +    cpu->isar.id_pfr1 = 0x00000200;
> +    cpu->isar.id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x00100030;
> +    cpu->isar.id_mmfr1 = 0x00000000;
> +    cpu->isar.id_mmfr2 = 0x01000000;
> +    cpu->isar.id_mmfr3 = 0x00000000;
> +    cpu->isar.id_isar0 = 0x01101110;
> +    cpu->isar.id_isar1 = 0x02112000;
> +    cpu->isar.id_isar2 = 0x20232231;
> +    cpu->isar.id_isar3 = 0x01111131;
> +    cpu->isar.id_isar4 = 0x01310132;
> +    cpu->isar.id_isar5 = 0x00000000;
> +    cpu->isar.id_isar6 = 0x00000000;
> +}
> +
> +static void cortex_m33_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    set_feature(&cpu->env, ARM_FEATURE_V8);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
> +    set_feature(&cpu->env, ARM_FEATURE_M_SECURITY);
> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
> +    cpu->midr = 0x410fd213; /* r0p3 */
> +    cpu->pmsav7_dregion = 16;
> +    cpu->sau_sregion = 8;
> +    cpu->isar.mvfr0 = 0x10110021;
> +    cpu->isar.mvfr1 = 0x11000011;
> +    cpu->isar.mvfr2 = 0x00000040;
> +    cpu->isar.id_pfr0 = 0x00000030;
> +    cpu->isar.id_pfr1 = 0x00000210;
> +    cpu->isar.id_dfr0 = 0x00200000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x00101F40;
> +    cpu->isar.id_mmfr1 = 0x00000000;
> +    cpu->isar.id_mmfr2 = 0x01000000;
> +    cpu->isar.id_mmfr3 = 0x00000000;
> +    cpu->isar.id_isar0 = 0x01101110;
> +    cpu->isar.id_isar1 = 0x02212000;
> +    cpu->isar.id_isar2 = 0x20232232;
> +    cpu->isar.id_isar3 = 0x01111131;
> +    cpu->isar.id_isar4 = 0x01310132;
> +    cpu->isar.id_isar5 = 0x00000000;
> +    cpu->isar.id_isar6 = 0x00000000;
> +    cpu->clidr = 0x00000000;
> +    cpu->ctr = 0x8000c000;
> +}
> +
> +static void cortex_m55_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    set_feature(&cpu->env, ARM_FEATURE_V8);
> +    set_feature(&cpu->env, ARM_FEATURE_V8_1M);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_M_MAIN);
> +    set_feature(&cpu->env, ARM_FEATURE_M_SECURITY);
> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
> +    cpu->midr = 0x410fd221; /* r0p1 */
> +    cpu->revidr = 0;
> +    cpu->pmsav7_dregion = 16;
> +    cpu->sau_sregion = 8;
> +    /* These are the MVFR* values for the FPU + full MVE configuration */
> +    cpu->isar.mvfr0 = 0x10110221;
> +    cpu->isar.mvfr1 = 0x12100211;
> +    cpu->isar.mvfr2 = 0x00000040;
> +    cpu->isar.id_pfr0 = 0x20000030;
> +    cpu->isar.id_pfr1 = 0x00000230;
> +    cpu->isar.id_dfr0 = 0x10200000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x00111040;
> +    cpu->isar.id_mmfr1 = 0x00000000;
> +    cpu->isar.id_mmfr2 = 0x01000000;
> +    cpu->isar.id_mmfr3 = 0x00000011;
> +    cpu->isar.id_isar0 = 0x01103110;
> +    cpu->isar.id_isar1 = 0x02212000;
> +    cpu->isar.id_isar2 = 0x20232232;
> +    cpu->isar.id_isar3 = 0x01111131;
> +    cpu->isar.id_isar4 = 0x01310132;
> +    cpu->isar.id_isar5 = 0x00000000;
> +    cpu->isar.id_isar6 = 0x00000000;
> +    cpu->clidr = 0x00000000; /* caches not implemented */
> +    cpu->ctr = 0x8303c003;
> +}
> +
> +static const struct TCGCPUOps arm_v7m_tcg_ops = {
> +    .initialize = arm_translate_init,
> +    .synchronize_from_tb = arm_cpu_synchronize_from_tb,
> +    .debug_excp_handler = arm_debug_excp_handler,
> +    .restore_state_to_opc = arm_restore_state_to_opc,
> +
> +#ifdef CONFIG_USER_ONLY
> +    .record_sigsegv = arm_cpu_record_sigsegv,
> +    .record_sigbus = arm_cpu_record_sigbus,
> +#else
> +    .tlb_fill = arm_cpu_tlb_fill,
> +    .cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt,
> +    .do_interrupt = arm_v7m_cpu_do_interrupt,
> +    .do_transaction_failed = arm_cpu_do_transaction_failed,
> +    .do_unaligned_access = arm_cpu_do_unaligned_access,
> +    .adjust_watchpoint_address = arm_adjust_watchpoint_address,
> +    .debug_check_watchpoint = arm_debug_check_watchpoint,
> +    .debug_check_breakpoint = arm_debug_check_breakpoint,
> +#endif /* !CONFIG_USER_ONLY */
> +};
> +
> +static void arm_v7m_class_init(ObjectClass *oc, void *data)
> +{
> +    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> +    CPUClass *cc = CPU_CLASS(oc);
> +
> +    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[] = {
> +    { .name = "cortex-m0",   .initfn = cortex_m0_initfn,
> +                             .class_init = arm_v7m_class_init },
> +    { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
> +                             .class_init = arm_v7m_class_init },
> +    { .name = "cortex-m4",   .initfn = cortex_m4_initfn,
> +                             .class_init = arm_v7m_class_init },
> +    { .name = "cortex-m7",   .initfn = cortex_m7_initfn,
> +                             .class_init = arm_v7m_class_init },
> +    { .name = "cortex-m33",  .initfn = cortex_m33_initfn,
> +                             .class_init = arm_v7m_class_init },
> +    { .name = "cortex-m55",  .initfn = cortex_m55_initfn,
> +                             .class_init = arm_v7m_class_init },

I'm not sure these CPU models make sense for linux-user but, since this 
patch makes no functional change, they can be removed later.

Paolo

Re: [PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Peter Maydell]

On Mon, 29 Jan 2024 at 08:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 1/29/24 09:18, Thomas Huth wrote:
> > +static const ARMCPUInfo arm_v7m_cpus[] = {
> > +    { .name = "cortex-m0",   .initfn = cortex_m0_initfn,
> > +                             .class_init = arm_v7m_class_init },
> > +    { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
> > +                             .class_init = arm_v7m_class_init },
> > +    { .name = "cortex-m4",   .initfn = cortex_m4_initfn,
> > +                             .class_init = arm_v7m_class_init },
> > +    { .name = "cortex-m7",   .initfn = cortex_m7_initfn,
> > +                             .class_init = arm_v7m_class_init },
> > +    { .name = "cortex-m33",  .initfn = cortex_m33_initfn,
> > +                             .class_init = arm_v7m_class_init },
> > +    { .name = "cortex-m55",  .initfn = cortex_m55_initfn,
> > +                             .class_init = arm_v7m_class_init },
>
> I'm not sure these CPU models make sense for linux-user

They do -- Linux has M-profile support, and we also have users
for the linux-user emulation with M-profile CPUs (eg people run
the gcc test suite that way).

-- PMM

Re: [PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Peter Maydell]

On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>
> Move the code to a separate file so that we do not have to compile
> it anymore if CONFIG_ARM_V7M is not set.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
>  target/arm/tcg/cpu32.c     | 261 ---------------------------------
>  target/arm/meson.build     |   3 +
>  target/arm/tcg/meson.build |   3 +
>  4 files changed, 296 insertions(+), 261 deletions(-)
>  create mode 100644 target/arm/tcg/cpu-v7m.c
>
> diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
> new file mode 100644
> index 0000000000..89a25444a2
> --- /dev/null
> +++ b/target/arm/tcg/cpu-v7m.c
> @@ -0,0 +1,290 @@
> +/*
> + * QEMU ARMv7-M TCG-only CPUs.
> + *
> + * Copyright (c) 2012 SUSE LINUX Products GmbH
> + *
> + * This code is licensed under the GNU GPL v2 or later.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/core/tcg-cpu-ops.h"
> +#include "internals.h"
> +
> +#if !defined(CONFIG_USER_ONLY)
> +
> +#include "hw/intc/armv7m_nvic.h"
> +
> +static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    bool ret = false;
> +
> +    /*
> +     * ARMv7-M interrupt masking works differently than -A or -R.
> +     * There is no FIQ/IRQ distinction. Instead of I and F bits
> +     * masking FIQ and IRQ interrupts, an exception is taken only
> +     * if it is higher priority than the current execution priority
> +     * (which depends on state like BASEPRI, FAULTMASK and the
> +     * currently active exception).
> +     */
> +    if (interrupt_request & CPU_INTERRUPT_HARD
> +        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
> +        cs->exception_index = EXCP_IRQ;
> +        cc->tcg_ops->do_interrupt(cs);
> +        ret = true;
> +    }
> +    return ret;
> +}
> +
> +#endif /* !CONFIG_USER_ONLY */

I wonder if this function could go in target/arm/tcg/m_helper.c:
it looks a bit odd in this file, which is mostly initfns for
specific CPU types. But it was in cpu32.c so I'm happy that
we just move it to cpu-v7m.c for now.

> diff --git a/target/arm/meson.build b/target/arm/meson.build
> index 46b5a21eb3..2e10464dbb 100644
> --- a/target/arm/meson.build
> +++ b/target/arm/meson.build
> @@ -26,6 +26,8 @@ arm_system_ss.add(files(
>    'ptw.c',
>  ))
>
> +arm_user_ss = ss.source_set()
> +
>  subdir('hvf')
>
>  if 'CONFIG_TCG' in config_all_accel
> @@ -36,3 +38,4 @@ endif
>
>  target_arch += {'arm': arm_ss}
>  target_system_arch += {'arm': arm_system_ss}
> +target_user_arch += {'arm': arm_user_ss}
> diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
> index 6fca38f2cc..3b1a9f0fc5 100644
> --- a/target/arm/tcg/meson.build
> +++ b/target/arm/tcg/meson.build
> @@ -55,3 +55,6 @@ arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
>  arm_system_ss.add(files(
>    'psci.c',
>  ))
> +
> +arm_system_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('cpu-v7m.c'))
> +arm_user_ss.add(when: 'TARGET_AARCH64', if_false: files('cpu-v7m.c'))

Why do we need to add this new arm_user_ss() sourceset,
when we didn't need it for the A/R profile CPUs?
What goes wrong if we add it to arm_ss() the way we do cpu32.c?

-- PMM

Re: [PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Thomas Huth]

On 01/02/2024 15.17, Peter Maydell wrote:
> On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>>
>> Move the code to a separate file so that we do not have to compile
>> it anymore if CONFIG_ARM_V7M is not set.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
>>   target/arm/tcg/cpu32.c     | 261 ---------------------------------
>>   target/arm/meson.build     |   3 +
>>   target/arm/tcg/meson.build |   3 +
>>   4 files changed, 296 insertions(+), 261 deletions(-)
>>   create mode 100644 target/arm/tcg/cpu-v7m.c
>>
>> diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
>> new file mode 100644
>> index 0000000000..89a25444a2
>> --- /dev/null
>> +++ b/target/arm/tcg/cpu-v7m.c
>> @@ -0,0 +1,290 @@
>> +/*
>> + * QEMU ARMv7-M TCG-only CPUs.
>> + *
>> + * Copyright (c) 2012 SUSE LINUX Products GmbH
>> + *
>> + * This code is licensed under the GNU GPL v2 or later.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "cpu.h"
>> +#include "hw/core/tcg-cpu-ops.h"
>> +#include "internals.h"
>> +
>> +#if !defined(CONFIG_USER_ONLY)
>> +
>> +#include "hw/intc/armv7m_nvic.h"
>> +
>> +static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>> +{
>> +    CPUClass *cc = CPU_GET_CLASS(cs);
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +    bool ret = false;
>> +
>> +    /*
>> +     * ARMv7-M interrupt masking works differently than -A or -R.
>> +     * There is no FIQ/IRQ distinction. Instead of I and F bits
>> +     * masking FIQ and IRQ interrupts, an exception is taken only
>> +     * if it is higher priority than the current execution priority
>> +     * (which depends on state like BASEPRI, FAULTMASK and the
>> +     * currently active exception).
>> +     */
>> +    if (interrupt_request & CPU_INTERRUPT_HARD
>> +        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
>> +        cs->exception_index = EXCP_IRQ;
>> +        cc->tcg_ops->do_interrupt(cs);
>> +        ret = true;
>> +    }
>> +    return ret;
>> +}
>> +
>> +#endif /* !CONFIG_USER_ONLY */
> 
> I wonder if this function could go in target/arm/tcg/m_helper.c:
> it looks a bit odd in this file, which is mostly initfns for
> specific CPU types. But it was in cpu32.c so I'm happy that
> we just move it to cpu-v7m.c for now.

The only user of this function are the arm_v7m_tcg_ops that are defined 
later in the cpu-v7m.c file, so I think it makes sense to keep it here.

>> diff --git a/target/arm/meson.build b/target/arm/meson.build
>> index 46b5a21eb3..2e10464dbb 100644
>> --- a/target/arm/meson.build
>> +++ b/target/arm/meson.build
>> @@ -26,6 +26,8 @@ arm_system_ss.add(files(
>>     'ptw.c',
>>   ))
>>
>> +arm_user_ss = ss.source_set()
>> +
>>   subdir('hvf')
>>
>>   if 'CONFIG_TCG' in config_all_accel
>> @@ -36,3 +38,4 @@ endif
>>
>>   target_arch += {'arm': arm_ss}
>>   target_system_arch += {'arm': arm_system_ss}
>> +target_user_arch += {'arm': arm_user_ss}
>> diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
>> index 6fca38f2cc..3b1a9f0fc5 100644
>> --- a/target/arm/tcg/meson.build
>> +++ b/target/arm/tcg/meson.build
>> @@ -55,3 +55,6 @@ arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
>>   arm_system_ss.add(files(
>>     'psci.c',
>>   ))
>> +
>> +arm_system_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('cpu-v7m.c'))
>> +arm_user_ss.add(when: 'TARGET_AARCH64', if_false: files('cpu-v7m.c'))
> 
> Why do we need to add this new arm_user_ss() sourceset,
> when we didn't need it for the A/R profile CPUs?

cpu32.c gets added to the arm_ss source set which is linked into all 
possible arm-related binaries (qemu-system-aarch64, qemu-system-arm, 
qemu-aarch64 and qemu-arm).

The goal of this rework is to have the v7m code only linked into the 
binaries that need it, i.e. qemu-system-arm/aarch64 if CONFIG_ARM_V7M is 
set, or the 32-bit qemu-arm linux-user binary.

> What goes wrong if we add it to arm_ss() the way we do cpu32.c?

The problem is that CONFIG_ARM_V7M is not set for the linux-user builds, so 
the code does not get included in the "qemu-arm" binary anymore.
Now, the obvious answer to that statement is of course: Let's add 
CONFIG_ARM_V7M to configs/targets/arm-linux-user.mak, too! ... but I tried 
that already, and it also does not work, since then we'll suddenly try to 
include the hw/intc/armv7m_nvic.c stuff into the qemu-arm binary, which of 
course also does not work. It might be possible to rework that by moving 
armv7m_nvic.c from specific_ss to system_ss, but looks like that will 
require a *lot* of other reworks (e.g. arm_feature() is not available for 
common code). Another solution might be to move armv7m_nvic.c into the 
hw/arm/ directory and add it there to arm_ss instead ... it's then a little 
bit weird that this is the only interrupt controller there, but at least the 
changes would be quite trivial. What do you think?

  Thomas

Re: [PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Thomas Huth]

On 01/02/2024 19.52, Thomas Huth wrote:
> On 01/02/2024 15.17, Peter Maydell wrote:
>> On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> Move the code to a separate file so that we do not have to compile
>>> it anymore if CONFIG_ARM_V7M is not set.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
>>>   target/arm/tcg/cpu32.c     | 261 ---------------------------------
>>>   target/arm/meson.build     |   3 +
>>>   target/arm/tcg/meson.build |   3 +
>>>   4 files changed, 296 insertions(+), 261 deletions(-)
>>>   create mode 100644 target/arm/tcg/cpu-v7m.c
>>>
>>> diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
>>> new file mode 100644
>>> index 0000000000..89a25444a2
>>> --- /dev/null
>>> +++ b/target/arm/tcg/cpu-v7m.c
>>> @@ -0,0 +1,290 @@
>>> +/*
>>> + * QEMU ARMv7-M TCG-only CPUs.
>>> + *
>>> + * Copyright (c) 2012 SUSE LINUX Products GmbH
>>> + *
>>> + * This code is licensed under the GNU GPL v2 or later.
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "cpu.h"
>>> +#include "hw/core/tcg-cpu-ops.h"
>>> +#include "internals.h"
>>> +
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +
>>> +#include "hw/intc/armv7m_nvic.h"
>>> +
>>> +static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>> +{
>>> +    CPUClass *cc = CPU_GET_CLASS(cs);
>>> +    ARMCPU *cpu = ARM_CPU(cs);
>>> +    CPUARMState *env = &cpu->env;
>>> +    bool ret = false;
>>> +
>>> +    /*
>>> +     * ARMv7-M interrupt masking works differently than -A or -R.
>>> +     * There is no FIQ/IRQ distinction. Instead of I and F bits
>>> +     * masking FIQ and IRQ interrupts, an exception is taken only
>>> +     * if it is higher priority than the current execution priority
>>> +     * (which depends on state like BASEPRI, FAULTMASK and the
>>> +     * currently active exception).
>>> +     */
>>> +    if (interrupt_request & CPU_INTERRUPT_HARD
>>> +        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
>>> +        cs->exception_index = EXCP_IRQ;
>>> +        cc->tcg_ops->do_interrupt(cs);
>>> +        ret = true;
>>> +    }
>>> +    return ret;
>>> +}
>>> +
>>> +#endif /* !CONFIG_USER_ONLY */
>>
>> I wonder if this function could go in target/arm/tcg/m_helper.c:
>> it looks a bit odd in this file, which is mostly initfns for
>> specific CPU types. But it was in cpu32.c so I'm happy that
>> we just move it to cpu-v7m.c for now.
> 
> The only user of this function are the arm_v7m_tcg_ops that are defined 
> later in the cpu-v7m.c file, so I think it makes sense to keep it here.
> 
>>> diff --git a/target/arm/meson.build b/target/arm/meson.build
>>> index 46b5a21eb3..2e10464dbb 100644
>>> --- a/target/arm/meson.build
>>> +++ b/target/arm/meson.build
>>> @@ -26,6 +26,8 @@ arm_system_ss.add(files(
>>>     'ptw.c',
>>>   ))
>>>
>>> +arm_user_ss = ss.source_set()
>>> +
>>>   subdir('hvf')
>>>
>>>   if 'CONFIG_TCG' in config_all_accel
>>> @@ -36,3 +38,4 @@ endif
>>>
>>>   target_arch += {'arm': arm_ss}
>>>   target_system_arch += {'arm': arm_system_ss}
>>> +target_user_arch += {'arm': arm_user_ss}
>>> diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
>>> index 6fca38f2cc..3b1a9f0fc5 100644
>>> --- a/target/arm/tcg/meson.build
>>> +++ b/target/arm/tcg/meson.build
>>> @@ -55,3 +55,6 @@ arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
>>>   arm_system_ss.add(files(
>>>     'psci.c',
>>>   ))
>>> +
>>> +arm_system_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('cpu-v7m.c'))
>>> +arm_user_ss.add(when: 'TARGET_AARCH64', if_false: files('cpu-v7m.c'))
>>
>> Why do we need to add this new arm_user_ss() sourceset,
>> when we didn't need it for the A/R profile CPUs?
> 
> cpu32.c gets added to the arm_ss source set which is linked into all 
> possible arm-related binaries (qemu-system-aarch64, qemu-system-arm, 
> qemu-aarch64 and qemu-arm).
> 
> The goal of this rework is to have the v7m code only linked into the 
> binaries that need it, i.e. qemu-system-arm/aarch64 if CONFIG_ARM_V7M is 
> set, or the 32-bit qemu-arm linux-user binary.
> 
>> What goes wrong if we add it to arm_ss() the way we do cpu32.c?
> 
> The problem is that CONFIG_ARM_V7M is not set for the linux-user builds, so 
> the code does not get included in the "qemu-arm" binary anymore.
> Now, the obvious answer to that statement is of course: Let's add 
> CONFIG_ARM_V7M to configs/targets/arm-linux-user.mak, too! ... but I tried 
> that already, and it also does not work, since then we'll suddenly try to 
> include the hw/intc/armv7m_nvic.c stuff into the qemu-arm binary, which of 
> course also does not work. It might be possible to rework that by moving 
> armv7m_nvic.c from specific_ss to system_ss, but looks like that will 
> require a *lot* of other reworks (e.g. arm_feature() is not available for 
> common code). Another solution might be to move armv7m_nvic.c into the 
> hw/arm/ directory and add it there to arm_ss instead ... it's then a little 
> bit weird that this is the only interrupt controller there, but at least the 
> changes would be quite trivial. What do you think?

  Hi Peter!

Any hints how to continue here? Respin the series with changes? Or is the 
current shape OK? Or are these changes rather unwanted and I should rather 
forget about these patches?

  Thomas

Re: [PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Peter Maydell]

On Thu, 1 Feb 2024 at 18:52, Thomas Huth <thuth@redhat.com> wrote:
>
> On 01/02/2024 15.17, Peter Maydell wrote:
> > On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
> >> diff --git a/target/arm/meson.build b/target/arm/meson.build
> >> index 46b5a21eb3..2e10464dbb 100644
> >> --- a/target/arm/meson.build
> >> +++ b/target/arm/meson.build
> >> @@ -26,6 +26,8 @@ arm_system_ss.add(files(
> >>     'ptw.c',
> >>   ))
> >>
> >> +arm_user_ss = ss.source_set()
> >> +
> >>   subdir('hvf')
> >>
> >>   if 'CONFIG_TCG' in config_all_accel
> >> @@ -36,3 +38,4 @@ endif
> >>
> >>   target_arch += {'arm': arm_ss}
> >>   target_system_arch += {'arm': arm_system_ss}
> >> +target_user_arch += {'arm': arm_user_ss}
> >> diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
> >> index 6fca38f2cc..3b1a9f0fc5 100644
> >> --- a/target/arm/tcg/meson.build
> >> +++ b/target/arm/tcg/meson.build
> >> @@ -55,3 +55,6 @@ arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
> >>   arm_system_ss.add(files(
> >>     'psci.c',
> >>   ))
> >> +
> >> +arm_system_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('cpu-v7m.c'))
> >> +arm_user_ss.add(when: 'TARGET_AARCH64', if_false: files('cpu-v7m.c'))
> >
> > Why do we need to add this new arm_user_ss() sourceset,
> > when we didn't need it for the A/R profile CPUs?
>
> cpu32.c gets added to the arm_ss source set which is linked into all
> possible arm-related binaries (qemu-system-aarch64, qemu-system-arm,
> qemu-aarch64 and qemu-arm).
>
> The goal of this rework is to have the v7m code only linked into the
> binaries that need it, i.e. qemu-system-arm/aarch64 if CONFIG_ARM_V7M is
> set, or the 32-bit qemu-arm linux-user binary.
>
> > What goes wrong if we add it to arm_ss() the way we do cpu32.c?
>
> The problem is that CONFIG_ARM_V7M is not set for the linux-user builds, so
> the code does not get included in the "qemu-arm" binary anymore.
> Now, the obvious answer to that statement is of course: Let's add
> CONFIG_ARM_V7M to configs/targets/arm-linux-user.mak, too! ... but I tried
> that already, and it also does not work, since then we'll suddenly try to
> include the hw/intc/armv7m_nvic.c stuff into the qemu-arm binary, which of
> course also does not work. It might be possible to rework that by moving
> armv7m_nvic.c from specific_ss to system_ss, but looks like that will
> require a *lot* of other reworks (e.g. arm_feature() is not available for
> common code). Another solution might be to move armv7m_nvic.c into the
> hw/arm/ directory and add it there to arm_ss instead ... it's then a little
> bit weird that this is the only interrupt controller there, but at least the
> changes would be quite trivial. What do you think?

The NVIC is in some sense part of the CPU, but I think I'd rather
not move it. (You'll also find that setting CONFIG_ARM_V7M pulls
in code in hw/misc (RAS handling) and hw/timer (systick timer), so
it's not just hw/intc code that's affected here.)

I guess this is kind of happening because we have one symbol
CONFIG_ARM_V7M and we're using it both for "we want the v7M
system components like the NVIC and timer" and for "we want
the v7M core CPU support". So maybe another approach here would
be to have CONFIG_ARM_V7M and CONFIG_ARM_V7M_CPU or something,
where linux-user selects just the latter. But unless you think
that approach would work out more cleanly, I'm OK with how this
patch does things.

(Sorry for not getting back to this series for so long.)

thanks
-- PMM

[PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Thomas Huth]

If CONFIG_ARM_V7M is not set, we don't want to include the full-fledged
helper functions that require additional functions for linking. The
reduced set of the linux-user functions works fine as stubs in this
case, so change the #ifdef statement accordingly.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/arm/tcg/m_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
index d1f1e02acc..a5a6e96fc3 100644
--- a/target/arm/tcg/m_helper.c
+++ b/target/arm/tcg/m_helper.c
@@ -22,6 +22,7 @@
 #endif
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/intc/armv7m_nvic.h"
+#include CONFIG_DEVICES
 #endif
 
 static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
@@ -69,7 +70,7 @@ uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure)
     return value;
 }
 
-#ifdef CONFIG_USER_ONLY
+#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_ARM_V7M)
 
 void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
 {
-- 
2.43.0

Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Peter Maydell]

On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>
> If CONFIG_ARM_V7M is not set, we don't want to include the full-fledged
> helper functions that require additional functions for linking. The
> reduced set of the linux-user functions works fine as stubs in this
> case, so change the #ifdef statement accordingly.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/arm/tcg/m_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
> index d1f1e02acc..a5a6e96fc3 100644
> --- a/target/arm/tcg/m_helper.c
> +++ b/target/arm/tcg/m_helper.c
> @@ -22,6 +22,7 @@
>  #endif
>  #if !defined(CONFIG_USER_ONLY)
>  #include "hw/intc/armv7m_nvic.h"
> +#include CONFIG_DEVICES
>  #endif
>
>  static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
> @@ -69,7 +70,7 @@ uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure)
>      return value;
>  }
>
> -#ifdef CONFIG_USER_ONLY
> +#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_ARM_V7M)

This looks a bit odd. If we don't have CONFIG_ARM_V7M
why are we compiling this file at all?

>
>  void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>  {

thanks
-- PMM

Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Thomas Huth]

On 01/02/2024 15.19, Peter Maydell wrote:
> On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>>
>> If CONFIG_ARM_V7M is not set, we don't want to include the full-fledged
>> helper functions that require additional functions for linking. The
>> reduced set of the linux-user functions works fine as stubs in this
>> case, so change the #ifdef statement accordingly.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   target/arm/tcg/m_helper.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
>> index d1f1e02acc..a5a6e96fc3 100644
>> --- a/target/arm/tcg/m_helper.c
>> +++ b/target/arm/tcg/m_helper.c
>> @@ -22,6 +22,7 @@
>>   #endif
>>   #if !defined(CONFIG_USER_ONLY)
>>   #include "hw/intc/armv7m_nvic.h"
>> +#include CONFIG_DEVICES
>>   #endif
>>
>>   static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
>> @@ -69,7 +70,7 @@ uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure)
>>       return value;
>>   }
>>
>> -#ifdef CONFIG_USER_ONLY
>> +#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_ARM_V7M)
> 
> This looks a bit odd. If we don't have CONFIG_ARM_V7M
> why are we compiling this file at all?

We'll get failures during linking otherwise. target/arm/helper.h still 
defines code that requires the v7m_* helper functions:

/usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o: 
qemu/target/arm/helper.h:76: undefined reference to `helper_v7m_vlldm'
/usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o: 
qemu/target/arm/helper.h:75: undefined reference to `helper_v7m_vlstm'
/usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o: 
qemu/target/arm/helper.h:73: undefined reference to 
`helper_v7m_preserve_fp_state'

etc.

And then other files are depending on these helpers, too, e.g. 
target/arm/tcg/translate-vfp.c calls gen_helper_v7m_preserve_fp_state() etc. 
... I guess it might be possible to disable all those spots with some 
#ifdefs, too, but it's getting quite ugly that way. It's way nicer to use 
the stub functions that we have in m_helper already anyway.

  Thomas

Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Peter Maydell]

On Thu, 1 Feb 2024 at 19:12, Thomas Huth <thuth@redhat.com> wrote:
>
> On 01/02/2024 15.19, Peter Maydell wrote:
> > On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> If CONFIG_ARM_V7M is not set, we don't want to include the full-fledged
> >> helper functions that require additional functions for linking. The
> >> reduced set of the linux-user functions works fine as stubs in this
> >> case, so change the #ifdef statement accordingly.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>   target/arm/tcg/m_helper.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
> >> index d1f1e02acc..a5a6e96fc3 100644
> >> --- a/target/arm/tcg/m_helper.c
> >> +++ b/target/arm/tcg/m_helper.c
> >> @@ -22,6 +22,7 @@
> >>   #endif
> >>   #if !defined(CONFIG_USER_ONLY)
> >>   #include "hw/intc/armv7m_nvic.h"
> >> +#include CONFIG_DEVICES
> >>   #endif
> >>
> >>   static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
> >> @@ -69,7 +70,7 @@ uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure)
> >>       return value;
> >>   }
> >>
> >> -#ifdef CONFIG_USER_ONLY
> >> +#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_ARM_V7M)
> >
> > This looks a bit odd. If we don't have CONFIG_ARM_V7M
> > why are we compiling this file at all?
>
> We'll get failures during linking otherwise. target/arm/helper.h still
> defines code that requires the v7m_* helper functions:
>
> /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
> qemu/target/arm/helper.h:76: undefined reference to `helper_v7m_vlldm'
> /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
> qemu/target/arm/helper.h:75: undefined reference to `helper_v7m_vlstm'
> /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
> qemu/target/arm/helper.h:73: undefined reference to
> `helper_v7m_preserve_fp_state'
>
> etc.

OK, but what we want in that case is either (a) avoid referring
to the functions if we're not building for V7M (as you say,
may be awkward) or else (b) stub versions of the functions that
abort() if called. We don't really want the linux-user versions
of the functions.

Does having an m-helper_stub.c which we build if not CONFIG_ARM_V7M
and having m_helper.c only built with CONFIG_ARM_V7M look
feasible?

thanks
-- PMM

Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Thomas Huth]

On 04/03/2024 16.22, Peter Maydell wrote:
> On Thu, 1 Feb 2024 at 19:12, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 01/02/2024 15.19, Peter Maydell wrote:
>>> On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> If CONFIG_ARM_V7M is not set, we don't want to include the full-fledged
>>>> helper functions that require additional functions for linking. The
>>>> reduced set of the linux-user functions works fine as stubs in this
>>>> case, so change the #ifdef statement accordingly.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    target/arm/tcg/m_helper.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
>>>> index d1f1e02acc..a5a6e96fc3 100644
>>>> --- a/target/arm/tcg/m_helper.c
>>>> +++ b/target/arm/tcg/m_helper.c
>>>> @@ -22,6 +22,7 @@
>>>>    #endif
>>>>    #if !defined(CONFIG_USER_ONLY)
>>>>    #include "hw/intc/armv7m_nvic.h"
>>>> +#include CONFIG_DEVICES
>>>>    #endif
>>>>
>>>>    static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
>>>> @@ -69,7 +70,7 @@ uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure)
>>>>        return value;
>>>>    }
>>>>
>>>> -#ifdef CONFIG_USER_ONLY
>>>> +#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_ARM_V7M)
>>>
>>> This looks a bit odd. If we don't have CONFIG_ARM_V7M
>>> why are we compiling this file at all?
>>
>> We'll get failures during linking otherwise. target/arm/helper.h still
>> defines code that requires the v7m_* helper functions:
>>
>> /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
>> qemu/target/arm/helper.h:76: undefined reference to `helper_v7m_vlldm'
>> /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
>> qemu/target/arm/helper.h:75: undefined reference to `helper_v7m_vlstm'
>> /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
>> qemu/target/arm/helper.h:73: undefined reference to
>> `helper_v7m_preserve_fp_state'
>>
>> etc.
> 
> OK, but what we want in that case is either (a) avoid referring
> to the functions if we're not building for V7M (as you say,
> may be awkward) or else (b) stub versions of the functions that
> abort() if called. We don't really want the linux-user versions
> of the functions.
> 
> Does having an m-helper_stub.c which we build if not CONFIG_ARM_V7M
> and having m_helper.c only built with CONFIG_ARM_V7M look
> feasible?

I gave it a try, but then we end up again with the problem that I already 
mentioned in the discussion about patch 1: CONFIG_ARM_V7M is not set for the 
linux-user binaries, so m_helper.c would not get included there anymore and 
we end up with lots of link failures.

So if you don't like the current shape, I guess this needs a little bit more 
pondering 'til it gets acceptable.

But could you maybe at least pick up the first patch already? ... since it's 
a patch with lots of code movement in it, this is quite ugly to rebase it 
each time someone touches some lines of code in that area...

  Thomas

Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Peter Maydell]

On Fri, 8 Mar 2024 at 12:54, Thomas Huth <thuth@redhat.com> wrote:
> I gave it a try, but then we end up again with the problem that I already
> mentioned in the discussion about patch 1: CONFIG_ARM_V7M is not set for the
> linux-user binaries, so m_helper.c would not get included there anymore and
> we end up with lots of link failures.
>
> So if you don't like the current shape, I guess this needs a little bit more
> pondering 'til it gets acceptable.
>
> But could you maybe at least pick up the first patch already? ... since it's
> a patch with lots of code movement in it, this is quite ugly to rebase it
> each time someone touches some lines of code in that area...

I don't object to taking the first patch, but... it doesn't apply
so it needs rebasing :-/ If you can rebase and send it out this
afternoon I can put it in a pullreq I'm working on.

-- PMM

Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Thomas Huth]

On 08/03/2024 15.00, Peter Maydell wrote:
> On Fri, 8 Mar 2024 at 12:54, Thomas Huth <thuth@redhat.com> wrote:
>> I gave it a try, but then we end up again with the problem that I already
>> mentioned in the discussion about patch 1: CONFIG_ARM_V7M is not set for the
>> linux-user binaries, so m_helper.c would not get included there anymore and
>> we end up with lots of link failures.
>>
>> So if you don't like the current shape, I guess this needs a little bit more
>> pondering 'til it gets acceptable.
>>
>> But could you maybe at least pick up the first patch already? ... since it's
>> a patch with lots of code movement in it, this is quite ugly to rebase it
>> each time someone touches some lines of code in that area...
> 
> I don't object to taking the first patch, but... it doesn't apply
> so it needs rebasing :-/ If you can rebase and send it out this
> afternoon I can put it in a pullreq I'm working on.

That's what I meant with it's "ugly to rebase it" ;-) ... fortunately, it 
was only a rather simple conflict this time.

I sent out a v3 now, also including another try for the second patch (which 
now includes the stubs that assert() in the m_helper.c file itself instead 
of using a separate stub file). Not sure whether that looks much nicer, but 
at least it seems to work from a compilation/linking perspective. Anyway, if 
you could at least include patch 1 from that v3 in your next pull request, 
that would be great!

  Thanks,
   Thomas

[PATCH v2 3/3] target/arm/Kconfig: Stop requiring CONFIG_ARM_V7M

[Thomas Huth]

Now that we made sure that ARM_V7M code only gets compiled if really
needed, we can drop the hard requirement for CONFIG_ARM_V7M in the
Kconfig file.

Tested-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/arm/Kconfig | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/target/arm/Kconfig b/target/arm/Kconfig
index bf57d739cd..3fffdcb61b 100644
--- a/target/arm/Kconfig
+++ b/target/arm/Kconfig
@@ -2,10 +2,6 @@ config ARM
     bool
     select ARM_COMPATIBLE_SEMIHOSTING if TCG
 
-    # We need to select this until we move m_helper.c and the
-    # translate.c v7m helpers under ARM_V7M.
-    select ARM_V7M if TCG
-
 config AARCH64
     bool
     select ARM
-- 
2.43.0

Re: [PATCH v2 0/3] target/arm: Allow compilation without CONFIG_ARM_V7M

[Thomas Huth]

On 29/01/2024 09.18, Thomas Huth wrote:
> We've got a switch to disable v7m code since a long time - but it
> currently cannot be disabled since linking then fails due to missing
> functions. But thanks to the clean-ups that have been done during the
> past years, it's not that difficult anymore to finally make it possible
> to disable CONFIG_ARM_V7M: We just have to move some v7m-related code
> out of cpu32.c to a separate file (that we only compile if the switch
> CONFIG_ARM_V7M is enabled) and make sure to use the stub functions in
> m_helper.c if it is disabled. Then we can finally remove the hard-coded
> "select ARM_V7M" from the Kconfig file.
> 
> v2:
> - Updated a comment
> - Avoid #ifdef in cpu-v7m.c, handle it via meson.build instead
> 
> Thomas Huth (3):
>    target/arm: Move v7m-related code from cpu32.c into a separate file
>    target/arm/tcg/m_helper.c: Include the full helpers only with
>      CONFIG_ARM_V7M
>    target/arm/Kconfig: Stop requiring CONFIG_ARM_V7M
> 
>   target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
>   target/arm/tcg/cpu32.c     | 261 ---------------------------------
>   target/arm/tcg/m_helper.c  |   3 +-
>   target/arm/Kconfig         |   4 -
>   target/arm/meson.build     |   3 +
>   target/arm/tcg/meson.build |   3 +
>   6 files changed, 298 insertions(+), 266 deletions(-)
>   create mode 100644 target/arm/tcg/cpu-v7m.c

*ping*

Any hints how to continue here? Or is the series fine as it is?

  Thomas