We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
CPUs should only be created if the switch is enabled. This can best
be done if the code resides in a separate file, thus move the related
functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
is enabled.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/arm/Makefile.objs |   1 +
 target/arm/cpu.c         | 146 -----------------------------
 target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+), 146 deletions(-)
 create mode 100644 target/arm/v7m.c

diff --git a/target/arm/Makefile.objs b/target/arm/Makefile.objs
index cf26c16f5f..16b9417a8b 100644
--- a/target/arm/Makefile.objs
+++ b/target/arm/Makefile.objs
@@ -61,6 +61,7 @@ obj-y += translate.o op_helper.o
 obj-y += crypto_helper.o
 obj-y += iwmmxt_helper.o vec_helper.o neon_helper.o
 obj-y += m_helper.o
+obj-$(CONFIG_ARM_V7M) += v7m.o
 
 obj-$(CONFIG_SOFTMMU) += psci.o
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index f1f9eecdc8..d5f0d4af61 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -462,31 +462,6 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     return ret;
 }
 
-#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
-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->do_interrupt(cs);
-        ret = true;
-    }
-    return ret;
-}
-#endif
-
 void arm_cpu_update_virq(ARMCPU *cpu)
 {
     /*
@@ -1881,119 +1856,6 @@ static void arm11mpcore_initfn(Object *obj)
     cpu->reset_auxcr = 1;
 }
 
-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;
-}
-
-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->id_pfr0 = 0x00000030;
-    cpu->id_pfr1 = 0x00000200;
-    cpu->id_dfr0 = 0x00100000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->id_mmfr0 = 0x00000030;
-    cpu->id_mmfr1 = 0x00000000;
-    cpu->id_mmfr2 = 0x00000000;
-    cpu->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);
-    set_feature(&cpu->env, ARM_FEATURE_VFP4);
-    cpu->midr = 0x410fc240; /* r0p0 */
-    cpu->pmsav7_dregion = 8;
-    cpu->isar.mvfr0 = 0x10110021;
-    cpu->isar.mvfr1 = 0x11000011;
-    cpu->isar.mvfr2 = 0x00000000;
-    cpu->id_pfr0 = 0x00000030;
-    cpu->id_pfr1 = 0x00000200;
-    cpu->id_dfr0 = 0x00100000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->id_mmfr0 = 0x00000030;
-    cpu->id_mmfr1 = 0x00000000;
-    cpu->id_mmfr2 = 0x00000000;
-    cpu->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_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);
-    set_feature(&cpu->env, ARM_FEATURE_VFP4);
-    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->id_pfr0 = 0x00000030;
-    cpu->id_pfr1 = 0x00000210;
-    cpu->id_dfr0 = 0x00200000;
-    cpu->id_afr0 = 0x00000000;
-    cpu->id_mmfr0 = 0x00101F40;
-    cpu->id_mmfr1 = 0x00000000;
-    cpu->id_mmfr2 = 0x01000000;
-    cpu->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 arm_v7m_class_init(ObjectClass *oc, void *data)
-{
-    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
-    CPUClass *cc = CPU_CLASS(oc);
-
-    acc->info = data;
-#ifndef CONFIG_USER_ONLY
-    cc->do_interrupt = arm_v7m_cpu_do_interrupt;
-#endif
-
-    cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
-}
-
 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,
@@ -2518,14 +2380,6 @@ static const ARMCPUInfo arm_cpus[] = {
     { .name = "arm1136",     .initfn = arm1136_initfn },
     { .name = "arm1176",     .initfn = arm1176_initfn },
     { .name = "arm11mpcore", .initfn = arm11mpcore_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-m33",  .initfn = cortex_m33_initfn,
-                             .class_init = arm_v7m_class_init },
     { .name = "cortex-r5",   .initfn = cortex_r5_initfn },
     { .name = "cortex-r5f",  .initfn = cortex_r5f_initfn },
     { .name = "cortex-a7",   .initfn = cortex_a7_initfn },
diff --git a/target/arm/v7m.c b/target/arm/v7m.c
new file mode 100644
index 0000000000..505043febe
--- /dev/null
+++ b/target/arm/v7m.c
@@ -0,0 +1,193 @@
+/*
+ * ARM v7m helpers.
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/qemu-print.h"
+#include "qemu-common.h"
+#include "target/arm/idau.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "cpu.h"
+#include "internals.h"
+#include "exec/exec-all.h"
+#include "hw/qdev-properties.h"
+#if !defined(CONFIG_USER_ONLY)
+#include "hw/loader.h"
+#include "hw/boards.h"
+#endif
+#include "sysemu/sysemu.h"
+#include "sysemu/tcg.h"
+#include "sysemu/hw_accel.h"
+#include "disas/capstone.h"
+#include "fpu/softfloat.h"
+
+#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
+
+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->do_interrupt(cs);
+        ret = true;
+    }
+    return ret;
+}
+
+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;
+}
+
+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->id_pfr0 = 0x00000030;
+    cpu->id_pfr1 = 0x00000200;
+    cpu->id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->id_mmfr0 = 0x00000030;
+    cpu->id_mmfr1 = 0x00000000;
+    cpu->id_mmfr2 = 0x00000000;
+    cpu->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);
+    set_feature(&cpu->env, ARM_FEATURE_VFP4);
+    cpu->midr = 0x410fc240; /* r0p0 */
+    cpu->pmsav7_dregion = 8;
+    cpu->isar.mvfr0 = 0x10110021;
+    cpu->isar.mvfr1 = 0x11000011;
+    cpu->isar.mvfr2 = 0x00000000;
+    cpu->id_pfr0 = 0x00000030;
+    cpu->id_pfr1 = 0x00000200;
+    cpu->id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->id_mmfr0 = 0x00000030;
+    cpu->id_mmfr1 = 0x00000000;
+    cpu->id_mmfr2 = 0x00000000;
+    cpu->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_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);
+    set_feature(&cpu->env, ARM_FEATURE_VFP4);
+    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->id_pfr0 = 0x00000030;
+    cpu->id_pfr1 = 0x00000210;
+    cpu->id_dfr0 = 0x00200000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->id_mmfr0 = 0x00101F40;
+    cpu->id_mmfr1 = 0x00000000;
+    cpu->id_mmfr2 = 0x01000000;
+    cpu->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 arm_v7m_class_init(ObjectClass *oc, void *data)
+{
+    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
+    CPUClass *cc = CPU_CLASS(oc);
+
+    acc->info = data;
+#ifndef CONFIG_USER_ONLY
+    cc->do_interrupt = arm_v7m_cpu_do_interrupt;
+#endif
+
+    cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
+}
+
+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-m33",  .initfn = cortex_m33_initfn,
+                             .class_init = arm_v7m_class_init },
+    { .name = NULL }
+};
+
+static void arm_v7m_cpu_register_types(void)
+{
+    const ARMCPUInfo *info = arm_v7m_cpus;
+
+    while (info->name) {
+        arm_cpu_register(info);
+        info++;
+    }
+}
+
+type_init(arm_v7m_cpu_register_types)
+
+#endif
-- 
2.18.1

On Sat, 21 Sep 2019 at 16:04, Thomas Huth <thuth@redhat.com> wrote:
>
> We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
> CPUs should only be created if the switch is enabled. This can best
> be done if the code resides in a separate file, thus move the related
> functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
> is enabled.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/arm/Makefile.objs |   1 +
>  target/arm/cpu.c         | 146 -----------------------------
>  target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 194 insertions(+), 146 deletions(-)
>  create mode 100644 target/arm/v7m.c

Calling the new file something with 'cpu' in it would help
to convey that it does the same kinds of things as
cpu.c and cpu64.c. Maybe cpu-m.c or cpu-v7m.c ?

thanks
-- PMM

On 9/23/19 4:34 PM, Peter Maydell wrote:
> On Sat, 21 Sep 2019 at 16:04, Thomas Huth <thuth@redhat.com> wrote:
>>
>> We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
>> CPUs should only be created if the switch is enabled. This can best
>> be done if the code resides in a separate file, thus move the related
>> functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
>> is enabled.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  target/arm/Makefile.objs |   1 +
>>  target/arm/cpu.c         | 146 -----------------------------
>>  target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 194 insertions(+), 146 deletions(-)
>>  create mode 100644 target/arm/v7m.c
> 
> Calling the new file something with 'cpu' in it would help
> to convey that it does the same kinds of things as
> cpu.c and cpu64.c. Maybe cpu-m.c or cpu-v7m.c ?

I agree to your other comment:

  From a CONFIG switch point of view I don't think it's
  worth being able to #ifdef out the various flavours of
  M-profile individually.

So I'm in favor of using cpu-m.c.

Hi Thomas,

On 9/21/19 5:04 PM, Thomas Huth wrote:
> We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
> CPUs should only be created if the switch is enabled. This can best
> be done if the code resides in a separate file, thus move the related
> functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
> is enabled.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/arm/Makefile.objs |   1 +
>  target/arm/cpu.c         | 146 -----------------------------
>  target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 194 insertions(+), 146 deletions(-)
>  create mode 100644 target/arm/v7m.c
> 
> diff --git a/target/arm/Makefile.objs b/target/arm/Makefile.objs
> index cf26c16f5f..16b9417a8b 100644
> --- a/target/arm/Makefile.objs
> +++ b/target/arm/Makefile.objs
> @@ -61,6 +61,7 @@ obj-y += translate.o op_helper.o
>  obj-y += crypto_helper.o
>  obj-y += iwmmxt_helper.o vec_helper.o neon_helper.o
>  obj-y += m_helper.o
> +obj-$(CONFIG_ARM_V7M) += v7m.o
>  
>  obj-$(CONFIG_SOFTMMU) += psci.o
>  
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index f1f9eecdc8..d5f0d4af61 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -462,31 +462,6 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>      return ret;
>  }
>  
> -#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> -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->do_interrupt(cs);
> -        ret = true;
> -    }
> -    return ret;
> -}
> -#endif
> -
>  void arm_cpu_update_virq(ARMCPU *cpu)
>  {
>      /*
> @@ -1881,119 +1856,6 @@ static void arm11mpcore_initfn(Object *obj)
>      cpu->reset_auxcr = 1;
>  }
>  
> -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;
> -}
> -
> -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->id_pfr0 = 0x00000030;
> -    cpu->id_pfr1 = 0x00000200;
> -    cpu->id_dfr0 = 0x00100000;
> -    cpu->id_afr0 = 0x00000000;
> -    cpu->id_mmfr0 = 0x00000030;
> -    cpu->id_mmfr1 = 0x00000000;
> -    cpu->id_mmfr2 = 0x00000000;
> -    cpu->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);
> -    set_feature(&cpu->env, ARM_FEATURE_VFP4);
> -    cpu->midr = 0x410fc240; /* r0p0 */
> -    cpu->pmsav7_dregion = 8;
> -    cpu->isar.mvfr0 = 0x10110021;
> -    cpu->isar.mvfr1 = 0x11000011;
> -    cpu->isar.mvfr2 = 0x00000000;
> -    cpu->id_pfr0 = 0x00000030;
> -    cpu->id_pfr1 = 0x00000200;
> -    cpu->id_dfr0 = 0x00100000;
> -    cpu->id_afr0 = 0x00000000;
> -    cpu->id_mmfr0 = 0x00000030;
> -    cpu->id_mmfr1 = 0x00000000;
> -    cpu->id_mmfr2 = 0x00000000;
> -    cpu->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_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);
> -    set_feature(&cpu->env, ARM_FEATURE_VFP4);
> -    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->id_pfr0 = 0x00000030;
> -    cpu->id_pfr1 = 0x00000210;
> -    cpu->id_dfr0 = 0x00200000;
> -    cpu->id_afr0 = 0x00000000;
> -    cpu->id_mmfr0 = 0x00101F40;
> -    cpu->id_mmfr1 = 0x00000000;
> -    cpu->id_mmfr2 = 0x01000000;
> -    cpu->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 arm_v7m_class_init(ObjectClass *oc, void *data)
> -{
> -    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> -    CPUClass *cc = CPU_CLASS(oc);
> -
> -    acc->info = data;
> -#ifndef CONFIG_USER_ONLY
> -    cc->do_interrupt = arm_v7m_cpu_do_interrupt;
> -#endif
> -
> -    cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
> -}
> -
>  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,
> @@ -2518,14 +2380,6 @@ static const ARMCPUInfo arm_cpus[] = {
>      { .name = "arm1136",     .initfn = arm1136_initfn },
>      { .name = "arm1176",     .initfn = arm1176_initfn },
>      { .name = "arm11mpcore", .initfn = arm11mpcore_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-m33",  .initfn = cortex_m33_initfn,
> -                             .class_init = arm_v7m_class_init },
>      { .name = "cortex-r5",   .initfn = cortex_r5_initfn },
>      { .name = "cortex-r5f",  .initfn = cortex_r5f_initfn },
>      { .name = "cortex-a7",   .initfn = cortex_a7_initfn },
> diff --git a/target/arm/v7m.c b/target/arm/v7m.c
> new file mode 100644
> index 0000000000..505043febe
> --- /dev/null
> +++ b/target/arm/v7m.c
> @@ -0,0 +1,193 @@
> +/*
> + * ARM v7m helpers.
> + *
> + * This code is licensed under the GNU GPL v2 or later.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/qemu-print.h"
> +#include "qemu-common.h"
> +#include "target/arm/idau.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "cpu.h"
> +#include "internals.h"
> +#include "exec/exec-all.h"
> +#include "hw/qdev-properties.h"
> +#if !defined(CONFIG_USER_ONLY)
> +#include "hw/loader.h"
> +#include "hw/boards.h"
> +#endif
> +#include "sysemu/sysemu.h"
> +#include "sysemu/tcg.h"
> +#include "sysemu/hw_accel.h"
> +#include "disas/capstone.h"
> +#include "fpu/softfloat.h"
I guess some of those headers are not needed.
> +
> +#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> +
> +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))) {
so what is the status wrt m_helper.c which stays unconditionally
compiled. m_helper functions seem to called from target/arm/translate.c
mostly. Have you abandoned the stub idea. It may be confusing to have 2
different helper files. At least a comment explaining where a new helper
shall go may be useful.

Thanks

Eric


> +        cs->exception_index = EXCP_IRQ;
> +        cc->do_interrupt(cs);
> +        ret = true;
> +    }
> +    return ret;
> +}
> +
> +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;
> +}
> +
> +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->id_pfr0 = 0x00000030;
> +    cpu->id_pfr1 = 0x00000200;
> +    cpu->id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->id_mmfr0 = 0x00000030;
> +    cpu->id_mmfr1 = 0x00000000;
> +    cpu->id_mmfr2 = 0x00000000;
> +    cpu->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);
> +    set_feature(&cpu->env, ARM_FEATURE_VFP4);
> +    cpu->midr = 0x410fc240; /* r0p0 */
> +    cpu->pmsav7_dregion = 8;
> +    cpu->isar.mvfr0 = 0x10110021;
> +    cpu->isar.mvfr1 = 0x11000011;
> +    cpu->isar.mvfr2 = 0x00000000;
> +    cpu->id_pfr0 = 0x00000030;
> +    cpu->id_pfr1 = 0x00000200;
> +    cpu->id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->id_mmfr0 = 0x00000030;
> +    cpu->id_mmfr1 = 0x00000000;
> +    cpu->id_mmfr2 = 0x00000000;
> +    cpu->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_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);
> +    set_feature(&cpu->env, ARM_FEATURE_VFP4);
> +    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->id_pfr0 = 0x00000030;
> +    cpu->id_pfr1 = 0x00000210;
> +    cpu->id_dfr0 = 0x00200000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->id_mmfr0 = 0x00101F40;
> +    cpu->id_mmfr1 = 0x00000000;
> +    cpu->id_mmfr2 = 0x01000000;
> +    cpu->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 arm_v7m_class_init(ObjectClass *oc, void *data)
> +{
> +    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> +    CPUClass *cc = CPU_CLASS(oc);
> +
> +    acc->info = data;
> +#ifndef CONFIG_USER_ONLY
> +    cc->do_interrupt = arm_v7m_cpu_do_interrupt;
> +#endif
> +
> +    cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
> +}
> +
> +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-m33",  .initfn = cortex_m33_initfn,
> +                             .class_init = arm_v7m_class_init },
> +    { .name = NULL }
> +};
> +
> +static void arm_v7m_cpu_register_types(void)
> +{
> +    const ARMCPUInfo *info = arm_v7m_cpus;
> +
> +    while (info->name) {
> +        arm_cpu_register(info);
> +        info++;
> +    }
> +}
> +
> +type_init(arm_v7m_cpu_register_types)
> +
> +#endif
> 
Thanks

Eric

On 23/09/2019 16.31, Auger Eric wrote:
> Hi Thomas,
> 
> On 9/21/19 5:04 PM, Thomas Huth wrote:
>> We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
>> CPUs should only be created if the switch is enabled. This can best
>> be done if the code resides in a separate file, thus move the related
>> functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
>> is enabled.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  target/arm/Makefile.objs |   1 +
>>  target/arm/cpu.c         | 146 -----------------------------
>>  target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 194 insertions(+), 146 deletions(-)
>>  create mode 100644 target/arm/v7m.c
[...]
>> diff --git a/target/arm/v7m.c b/target/arm/v7m.c
>> new file mode 100644
>> index 0000000000..505043febe
>> --- /dev/null
>> +++ b/target/arm/v7m.c
>> @@ -0,0 +1,193 @@
>> +/*
>> + * ARM v7m helpers.
>> + *
>> + * This code is licensed under the GNU GPL v2 or later.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/qemu-print.h"
>> +#include "qemu-common.h"
>> +#include "target/arm/idau.h"
>> +#include "qemu/module.h"
>> +#include "qapi/error.h"
>> +#include "qapi/visitor.h"
>> +#include "cpu.h"
>> +#include "internals.h"
>> +#include "exec/exec-all.h"
>> +#include "hw/qdev-properties.h"
>> +#if !defined(CONFIG_USER_ONLY)
>> +#include "hw/loader.h"
>> +#include "hw/boards.h"
>> +#endif
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/tcg.h"
>> +#include "sysemu/hw_accel.h"
>> +#include "disas/capstone.h"
>> +#include "fpu/softfloat.h"
>
> I guess some of those headers are not needed.

Yeah, I just copy-n-pasted from the source file ... I'll check what can
be omitted (if this patch series has a chance at all...)

>> +
>> +#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>> +
>> +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))) {
>
> so what is the status wrt m_helper.c which stays unconditionally
> compiled. m_helper functions seem to called from target/arm/translate.c
> mostly. Have you abandoned the stub idea. It may be confusing to have 2
> different helper files. At least a comment explaining where a new helper
> shall go may be useful.

All the HELPER() functions should definitely stay in m_helper.c. They
are required for linking. Or do you prefer a stub file instead? Then we
could maybe make the whole m_helper.c conditional in the Makefile.objs
instead.

However, there's one thing I currently don't quite understand in this
code (since I'm not an ARM guy, sorry) : There are references to "v8" in
m_helper.c, too. Is that related to a separate CPU type, ie. should the
v8 code also be available when CONFIG_ARM_V7M is disabled? Or can the
code in m_helper.c be disabled completely if CONFIG_ARM_V7M is not set?

 Thomas

On Mon, 23 Sep 2019 at 19:09, Thomas Huth <thuth@redhat.com> wrote:
> However, there's one thing I currently don't quite understand in this
> code (since I'm not an ARM guy, sorry) : There are references to "v8" in
> m_helper.c, too. Is that related to a separate CPU type, ie. should the
> v8 code also be available when CONFIG_ARM_V7M is disabled? Or can the
> code in m_helper.c be disabled completely if CONFIG_ARM_V7M is not set?

QEMU's naming conventions here is a bit confusing, for
historical reasons.

Architecturally what we have is:
 * "M-profile" -- this is the flavour of Arm architecture for
microcontrollers; it has some big differences from A and R
profile (eg the exception mechanism is different and it has
a built-in NVIC interrupt controller). All the Cortex-M<anything>
CPUs are M-profile
 * "Arm-v7M" -- this is the v7 flavour of the M-profile
architecture, eg Cortex-M3.
 * "Arm-v6M" -- this looks like it ought to mean "v6 flavour
of M-profile", but if you look at what features it has it's
more like "cut down version of v7M" (fewer instructions, cut
down exception model, etc, but some things which on A-profile
don't appear until v7A are present in v6M). Cortex-M0 and -M1.
 * "Arm-v8M" -- v8 flavour of M-profile. The big change here
is support for TrustZone. Cortex-M33. v8M comes in two
sub-profiles: "mainline", which has all the features like v7M,
and "baseline", which is cut-down in the same way v6M is a
cut-down v7M.

In QEMU, we implemented Cortex-M3 first, and then added -M0 and
-M33 later. So mostly our function naming convention uses
"v7m" when it means "any M-profile"; a few v8M-specific
functions use a "v8m" prefix. Everything in m_helper.c is
M-profile specific; most of it is used by all M-profile cores,
and a few bits are v8M-only or v7M-and-v8M-mainline only.

From a CONFIG switch point of view I don't think it's
worth being able to #ifdef out the various flavours of
M-profile individually.

thanks
-- PMM

On 23/09/2019 20.45, Peter Maydell wrote:
> On Mon, 23 Sep 2019 at 19:09, Thomas Huth <thuth@redhat.com> wrote:
>> However, there's one thing I currently don't quite understand in this
>> code (since I'm not an ARM guy, sorry) : There are references to "v8" in
>> m_helper.c, too. Is that related to a separate CPU type, ie. should the
>> v8 code also be available when CONFIG_ARM_V7M is disabled? Or can the
>> code in m_helper.c be disabled completely if CONFIG_ARM_V7M is not set?
> 
> QEMU's naming conventions here is a bit confusing, for
> historical reasons.
> 
> Architecturally what we have is:
>  * "M-profile" -- this is the flavour of Arm architecture for
> microcontrollers; it has some big differences from A and R
> profile (eg the exception mechanism is different and it has
> a built-in NVIC interrupt controller). All the Cortex-M<anything>
> CPUs are M-profile
>  * "Arm-v7M" -- this is the v7 flavour of the M-profile
> architecture, eg Cortex-M3.
>  * "Arm-v6M" -- this looks like it ought to mean "v6 flavour
> of M-profile", but if you look at what features it has it's
> more like "cut down version of v7M" (fewer instructions, cut
> down exception model, etc, but some things which on A-profile
> don't appear until v7A are present in v6M). Cortex-M0 and -M1.
>  * "Arm-v8M" -- v8 flavour of M-profile. The big change here
> is support for TrustZone. Cortex-M33. v8M comes in two
> sub-profiles: "mainline", which has all the features like v7M,
> and "baseline", which is cut-down in the same way v6M is a
> cut-down v7M.
> 
> In QEMU, we implemented Cortex-M3 first, and then added -M0 and
> -M33 later. So mostly our function naming convention uses
> "v7m" when it means "any M-profile"; a few v8M-specific
> functions use a "v8m" prefix. Everything in m_helper.c is
> M-profile specific; most of it is used by all M-profile cores,
> and a few bits are v8M-only or v7M-and-v8M-mainline only.
> 
> From a CONFIG switch point of view I don't think it's
> worth being able to #ifdef out the various flavours of
> M-profile individually.

Ok, thanks a lot for the explanation! It's much clearer to me now.
So I think it likely would be best to disable the whole m_helper.c code
instead of introducing a new file like v7m.c (but that likely requires a
stub file instead or some #ifdeffing in translate.c).

It also sounds like CONFIG_ARM_V7M should rather be renamed to
CONFIG_ARM_MPROFILE or something similar?

 Thomas

On Mon, 23 Sep 2019 at 19:51, Thomas Huth <thuth@redhat.com> wrote:
> It also sounds like CONFIG_ARM_V7M should rather be renamed to
> CONFIG_ARM_MPROFILE or something similar?

Depends whether it's visible to end-users or not. If it is,
a different name is probably more helpful; if it's just a
symbol used in the QEMU source code/makefiles/etc then you
might as well stick with the V7M naming convention we have
for consistency with the C files.

thanks
-- PMM

Hi Thomas,

On 9/23/19 8:09 PM, Thomas Huth wrote:
> On 23/09/2019 16.31, Auger Eric wrote:
>> Hi Thomas,
>>
>> On 9/21/19 5:04 PM, Thomas Huth wrote:
>>> We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
>>> CPUs should only be created if the switch is enabled. This can best
>>> be done if the code resides in a separate file, thus move the related
>>> functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
>>> is enabled.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  target/arm/Makefile.objs |   1 +
>>>  target/arm/cpu.c         | 146 -----------------------------
>>>  target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 194 insertions(+), 146 deletions(-)
>>>  create mode 100644 target/arm/v7m.c
> [...]
>>> diff --git a/target/arm/v7m.c b/target/arm/v7m.c
>>> new file mode 100644
>>> index 0000000000..505043febe
>>> --- /dev/null
>>> +++ b/target/arm/v7m.c
>>> @@ -0,0 +1,193 @@
>>> +/*
>>> + * ARM v7m helpers.
>>> + *
>>> + * This code is licensed under the GNU GPL v2 or later.
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/qemu-print.h"
>>> +#include "qemu-common.h"
>>> +#include "target/arm/idau.h"
>>> +#include "qemu/module.h"
>>> +#include "qapi/error.h"
>>> +#include "qapi/visitor.h"
>>> +#include "cpu.h"
>>> +#include "internals.h"
>>> +#include "exec/exec-all.h"
>>> +#include "hw/qdev-properties.h"
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +#include "hw/loader.h"
>>> +#include "hw/boards.h"
>>> +#endif
>>> +#include "sysemu/sysemu.h"
>>> +#include "sysemu/tcg.h"
>>> +#include "sysemu/hw_accel.h"
>>> +#include "disas/capstone.h"
>>> +#include "fpu/softfloat.h"
>>
>> I guess some of those headers are not needed.
> 
> Yeah, I just copy-n-pasted from the source file ... I'll check what can
> be omitted (if this patch series has a chance at all...)
> 
>>> +
>>> +#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>>> +
>>> +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))) {
>>
>> so what is the status wrt m_helper.c which stays unconditionally
>> compiled. m_helper functions seem to called from target/arm/translate.c
>> mostly. Have you abandoned the stub idea. It may be confusing to have 2
>> different helper files. At least a comment explaining where a new helper
>> shall go may be useful.
> 
> All the HELPER() functions should definitely stay in m_helper.c. They
> are required for linking. Or do you prefer a stub file instead? Then we
> could maybe make the whole m_helper.c conditional in the Makefile.objs
> instead.

I was simply referring to your previous approach:

Applying [Qemu-devel] [RFC PATCH 3/3] target/arm: Make m_helper.c
optional via CONFIG_ARM_V7M seems to fix the issue
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00333.html

It seems to work fine as it removes the call to armv7m_nvic.c functions
so no need to move the interrupt controller code?

Thanks

Eric

> 
> However, there's one thing I currently don't quite understand in this
> code (since I'm not an ARM guy, sorry) : There are references to "v8" in
> m_helper.c, too. Is that related to a separate CPU type, ie. should the
> v8 code also be available when CONFIG_ARM_V7M is disabled? Or can the
> code in m_helper.c be disabled completely if CONFIG_ARM_V7M is not set?
> 
>  Thomas
> 

On 24/09/2019 13.02, Auger Eric wrote:
> Hi Thomas,
> 
> On 9/23/19 8:09 PM, Thomas Huth wrote:
>> On 23/09/2019 16.31, Auger Eric wrote:
>>> Hi Thomas,
>>>
>>> On 9/21/19 5:04 PM, Thomas Huth wrote:
>>>> We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
>>>> CPUs should only be created if the switch is enabled. This can best
>>>> be done if the code resides in a separate file, thus move the related
>>>> functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
>>>> is enabled.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  target/arm/Makefile.objs |   1 +
>>>>  target/arm/cpu.c         | 146 -----------------------------
>>>>  target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 194 insertions(+), 146 deletions(-)
>>>>  create mode 100644 target/arm/v7m.c
>> [...]
>>>> diff --git a/target/arm/v7m.c b/target/arm/v7m.c
>>>> new file mode 100644
>>>> index 0000000000..505043febe
>>>> --- /dev/null
>>>> +++ b/target/arm/v7m.c
>>>> @@ -0,0 +1,193 @@
>>>> +/*
>>>> + * ARM v7m helpers.
>>>> + *
>>>> + * This code is licensed under the GNU GPL v2 or later.
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/qemu-print.h"
>>>> +#include "qemu-common.h"
>>>> +#include "target/arm/idau.h"
>>>> +#include "qemu/module.h"
>>>> +#include "qapi/error.h"
>>>> +#include "qapi/visitor.h"
>>>> +#include "cpu.h"
>>>> +#include "internals.h"
>>>> +#include "exec/exec-all.h"
>>>> +#include "hw/qdev-properties.h"
>>>> +#if !defined(CONFIG_USER_ONLY)
>>>> +#include "hw/loader.h"
>>>> +#include "hw/boards.h"
>>>> +#endif
>>>> +#include "sysemu/sysemu.h"
>>>> +#include "sysemu/tcg.h"
>>>> +#include "sysemu/hw_accel.h"
>>>> +#include "disas/capstone.h"
>>>> +#include "fpu/softfloat.h"
>>>
>>> I guess some of those headers are not needed.
>>
>> Yeah, I just copy-n-pasted from the source file ... I'll check what can
>> be omitted (if this patch series has a chance at all...)
>>
>>>> +
>>>> +#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>>>> +
>>>> +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))) {
>>>
>>> so what is the status wrt m_helper.c which stays unconditionally
>>> compiled. m_helper functions seem to called from target/arm/translate.c
>>> mostly. Have you abandoned the stub idea. It may be confusing to have 2
>>> different helper files. At least a comment explaining where a new helper
>>> shall go may be useful.
>>
>> All the HELPER() functions should definitely stay in m_helper.c. They
>> are required for linking. Or do you prefer a stub file instead? Then we
>> could maybe make the whole m_helper.c conditional in the Makefile.objs
>> instead.
> 
> I was simply referring to your previous approach:
> 
> Applying [Qemu-devel] [RFC PATCH 3/3] target/arm: Make m_helper.c
> optional via CONFIG_ARM_V7M seems to fix the issue
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00333.html
> 
> It seems to work fine as it removes the call to armv7m_nvic.c functions
> so no need to move the interrupt controller code?

Yes, but then we either need stubs in a separate file, or have to put
lots of #ifdefs into translate.c ... none of those solutions seem to be
really perfect :-(
Anyway, Philippe is currently respinning his series (I think), so I'll
postpone my work now to avoid to interfere with him.

 Thomas

Hi Thomas,

On 9/24/19 1:06 PM, Thomas Huth wrote:
> On 24/09/2019 13.02, Auger Eric wrote:
>> Hi Thomas,
>>
>> On 9/23/19 8:09 PM, Thomas Huth wrote:
>>> On 23/09/2019 16.31, Auger Eric wrote:
>>>> Hi Thomas,
>>>>
>>>> On 9/21/19 5:04 PM, Thomas Huth wrote:
>>>>> We are going to make CONFIG_ARM_V7M optional, so the related cortex-m
>>>>> CPUs should only be created if the switch is enabled. This can best
>>>>> be done if the code resides in a separate file, thus move the related
>>>>> functions to a new file v7m.c which only gets compiled if CONFIG_ARM_V7M
>>>>> is enabled.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  target/arm/Makefile.objs |   1 +
>>>>>  target/arm/cpu.c         | 146 -----------------------------
>>>>>  target/arm/v7m.c         | 193 +++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 194 insertions(+), 146 deletions(-)
>>>>>  create mode 100644 target/arm/v7m.c
>>> [...]
>>>>> diff --git a/target/arm/v7m.c b/target/arm/v7m.c
>>>>> new file mode 100644
>>>>> index 0000000000..505043febe
>>>>> --- /dev/null
>>>>> +++ b/target/arm/v7m.c
>>>>> @@ -0,0 +1,193 @@
>>>>> +/*
>>>>> + * ARM v7m helpers.
>>>>> + *
>>>>> + * This code is licensed under the GNU GPL v2 or later.
>>>>> + *
>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>> + */
>>>>> +
>>>>> +#include "qemu/osdep.h"
>>>>> +#include "qemu/qemu-print.h"
>>>>> +#include "qemu-common.h"
>>>>> +#include "target/arm/idau.h"
>>>>> +#include "qemu/module.h"
>>>>> +#include "qapi/error.h"
>>>>> +#include "qapi/visitor.h"
>>>>> +#include "cpu.h"
>>>>> +#include "internals.h"
>>>>> +#include "exec/exec-all.h"
>>>>> +#include "hw/qdev-properties.h"
>>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>> +#include "hw/loader.h"
>>>>> +#include "hw/boards.h"
>>>>> +#endif
>>>>> +#include "sysemu/sysemu.h"
>>>>> +#include "sysemu/tcg.h"
>>>>> +#include "sysemu/hw_accel.h"
>>>>> +#include "disas/capstone.h"
>>>>> +#include "fpu/softfloat.h"
>>>>
>>>> I guess some of those headers are not needed.
>>>
>>> Yeah, I just copy-n-pasted from the source file ... I'll check what can
>>> be omitted (if this patch series has a chance at all...)
>>>
>>>>> +
>>>>> +#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>>>>> +
>>>>> +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))) {
>>>>
>>>> so what is the status wrt m_helper.c which stays unconditionally
>>>> compiled. m_helper functions seem to called from target/arm/translate.c
>>>> mostly. Have you abandoned the stub idea. It may be confusing to have 2
>>>> different helper files. At least a comment explaining where a new helper
>>>> shall go may be useful.
>>>
>>> All the HELPER() functions should definitely stay in m_helper.c. They
>>> are required for linking. Or do you prefer a stub file instead? Then we
>>> could maybe make the whole m_helper.c conditional in the Makefile.objs
>>> instead.
>>
>> I was simply referring to your previous approach:
>>
>> Applying [Qemu-devel] [RFC PATCH 3/3] target/arm: Make m_helper.c
>> optional via CONFIG_ARM_V7M seems to fix the issue
>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00333.html
>>
>> It seems to work fine as it removes the call to armv7m_nvic.c functions
>> so no need to move the interrupt controller code?
> 
> Yes, but then we either need stubs in a separate file, or have to put
> lots of #ifdefs into translate.c ... none of those solutions seem to be
> really perfect :-(
Yep, the separate stub file looked cleaner to me.

> Anyway, Philippe is currently respinning his series (I think), so I'll
> postpone my work now to avoid to interfere with him.
OK

Thanks

Eric
> 
>  Thomas
>