[PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n

[PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n

[Thomas Huth]

We've got CONFIG_ARM_V7M, but it currently can't be disabled.
Here are some patches that should allow to disable the switch
(if the corresponding boards are disabled, too).

RFC -> v1:
 - Move the definitions into internals.h instead of cpu.h in the 1st patch
 - Instead of creating an ugly stubs file, simply make armv7m_nvic.c
   mandatory for linking.

Thomas Huth (4):
  target/arm: Make cpu_register() and set_feature() available for other
    files
  target/arm: Move cortex-m related functions to new file v7m.c
  hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm
    builds
  default-configs: Do not enforce CONFIG_ARM_V7M anymore

 default-configs/arm-softmmu.mak |   3 -
 hw/arm/Makefile.objs            |   2 +
 hw/{intc => arm}/armv7m_nvic.c  |   0
 hw/arm/trace-events             |  17 +++
 hw/intc/Makefile.objs           |   1 -
 hw/intc/trace-events            |  17 ---
 target/arm/Makefile.objs        |   1 +
 target/arm/cpu.c                | 166 +--------------------------
 target/arm/cpu64.c              |  17 +--
 target/arm/internals.h          |  18 +++
 target/arm/v7m.c                | 193 ++++++++++++++++++++++++++++++++
 11 files changed, 234 insertions(+), 201 deletions(-)
 rename hw/{intc => arm}/armv7m_nvic.c (100%)
 create mode 100644 target/arm/v7m.c

-- 
2.18.1

[PATCH 1/4] target/arm: Make cpu_register() and set_feature() available for other files

[Thomas Huth]

Move the common set_feature() and unset_feature() functions from cpu.c and
cpu64.c to internals.h, and make cpu_register() (renamed to arm_cpu_register())
available from there, too, so we can register CPUs also from other files
in the future.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/arm/cpu.c       | 20 ++------------------
 target/arm/cpu64.c     | 17 +----------------
 target/arm/internals.h | 18 ++++++++++++++++++
 3 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2399c14471..f1f9eecdc8 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -614,16 +614,6 @@ static bool arm_cpu_virtio_is_big_endian(CPUState *cs)
 
 #endif
 
-static inline void set_feature(CPUARMState *env, int feature)
-{
-    env->features |= 1ULL << feature;
-}
-
-static inline void unset_feature(CPUARMState *env, int feature)
-{
-    env->features &= ~(1ULL << feature);
-}
-
 static int
 print_insn_thumb1(bfd_vma pc, disassemble_info *info)
 {
@@ -2515,12 +2505,6 @@ static void arm_max_initfn(Object *obj)
 
 #endif /* !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) */
 
-struct ARMCPUInfo {
-    const char *name;
-    void (*initfn)(Object *obj);
-    void (*class_init)(ObjectClass *oc, void *data);
-};
-
 static const ARMCPUInfo arm_cpus[] = {
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
     { .name = "arm926",      .initfn = arm926_initfn },
@@ -2681,7 +2665,7 @@ static void cpu_register_class_init(ObjectClass *oc, void *data)
     acc->info = data;
 }
 
-static void cpu_register(const ARMCPUInfo *info)
+void arm_cpu_register(const ARMCPUInfo *info)
 {
     TypeInfo type_info = {
         .parent = TYPE_ARM_CPU,
@@ -2722,7 +2706,7 @@ static void arm_cpu_register_types(void)
     type_register_static(&idau_interface_type_info);
 
     while (info->name) {
-        cpu_register(info);
+        arm_cpu_register(info);
         info++;
     }
 
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index d7f5bf610a..5239ba5529 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "cpu.h"
+#include "internals.h"
 #include "qemu/module.h"
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/loader.h"
@@ -29,16 +30,6 @@
 #include "kvm_arm.h"
 #include "qapi/visitor.h"
 
-static inline void set_feature(CPUARMState *env, int feature)
-{
-    env->features |= 1ULL << feature;
-}
-
-static inline void unset_feature(CPUARMState *env, int feature)
-{
-    env->features &= ~(1ULL << feature);
-}
-
 #ifndef CONFIG_USER_ONLY
 static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
@@ -396,12 +387,6 @@ static void aarch64_max_initfn(Object *obj)
     }
 }
 
-struct ARMCPUInfo {
-    const char *name;
-    void (*initfn)(Object *obj);
-    void (*class_init)(ObjectClass *oc, void *data);
-};
-
 static const ARMCPUInfo aarch64_cpus[] = {
     { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
     { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 232d963875..e71196ed5f 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1046,4 +1046,22 @@ void arm_log_exception(int idx);
 
 #endif /* !CONFIG_USER_ONLY */
 
+static inline void set_feature(CPUARMState *env, int feature)
+{
+    env->features |= 1ULL << feature;
+}
+
+static inline void unset_feature(CPUARMState *env, int feature)
+{
+    env->features &= ~(1ULL << feature);
+}
+
+struct ARMCPUInfo {
+    const char *name;
+    void (*initfn)(Object *obj);
+    void (*class_init)(ObjectClass *oc, void *data);
+};
+
+void arm_cpu_register(const ARMCPUInfo *info);
+
 #endif
-- 
2.18.1

[PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Thomas Huth]

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

[PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Thomas Huth]

qemu-system-arm/-aarch64 currently can't be built without setting the
switch CONFIG_ARM_V7M=y - which we currently always do in the config file
default-configs/arm-softmmu.mak. This is because the code in target/arm/
calls many functions from this armv7m_nvic.c, and thus linking fails
without this file.

So armv7m_nvic.c should not be under the CONFIG_ARM_V7M switch, but always
compiled for arm builds. Since we can not simply do this in hw/intc/ (with
"obj-y += ..." it would get compiled for all other architectures, too),
let's move the file to hw/arm/ instead and always enable it there.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/Makefile.objs           |  2 ++
 hw/{intc => arm}/armv7m_nvic.c |  0
 hw/arm/trace-events            | 17 +++++++++++++++++
 hw/intc/Makefile.objs          |  1 -
 hw/intc/trace-events           | 17 -----------------
 5 files changed, 19 insertions(+), 18 deletions(-)
 rename hw/{intc => arm}/armv7m_nvic.c (100%)

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 43ce8d5b19..3c94d383a0 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -27,6 +27,8 @@ obj-$(CONFIG_VEXPRESS) += vexpress.o
 obj-$(CONFIG_ZYNQ) += xilinx_zynq.o
 obj-$(CONFIG_SABRELITE) += sabrelite.o
 
+# Note: armv7m_nvic.o is currently always required for linking
+obj-y += armv7m_nvic.o
 obj-$(CONFIG_ARM_V7M) += armv7m.o
 obj-$(CONFIG_EXYNOS4) += exynos4210.o
 obj-$(CONFIG_PXA2XX) += pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
diff --git a/hw/intc/armv7m_nvic.c b/hw/arm/armv7m_nvic.c
similarity index 100%
rename from hw/intc/armv7m_nvic.c
rename to hw/arm/armv7m_nvic.c
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 0acedcedc6..3068202a4c 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -1,5 +1,22 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# armv7m_nvic.c
+nvic_recompute_state(int vectpending, int vectpending_prio, int exception_prio) "NVIC state recomputed: vectpending %d vectpending_prio %d exception_prio %d"
+nvic_recompute_state_secure(int vectpending, bool vectpending_is_s_banked, int vectpending_prio, int exception_prio) "NVIC state recomputed: vectpending %d is_s_banked %d vectpending_prio %d exception_prio %d"
+nvic_set_prio(int irq, bool secure, uint8_t prio) "NVIC set irq %d secure-bank %d priority %d"
+nvic_irq_update(int vectpending, int pendprio, int exception_prio, int level) "NVIC vectpending %d pending prio %d exception_prio %d: setting irq line to %d"
+nvic_escalate_prio(int irq, int irqprio, int runprio) "NVIC escalating irq %d to HardFault: insufficient priority %d >= %d"
+nvic_escalate_disabled(int irq) "NVIC escalating irq %d to HardFault: disabled"
+nvic_set_pending(int irq, bool secure, bool targets_secure, bool derived, int en, int prio) "NVIC set pending irq %d secure-bank %d targets_secure %d derived %d (enabled: %d priority %d)"
+nvic_clear_pending(int irq, bool secure, int en, int prio) "NVIC clear pending irq %d secure-bank %d (enabled: %d priority %d)"
+nvic_acknowledge_irq(int irq, int prio) "NVIC acknowledge IRQ: %d now active (prio %d)"
+nvic_get_pending_irq_info(int irq, bool secure) "NVIC next IRQ %d: targets_secure: %d"
+nvic_complete_irq(int irq, bool secure) "NVIC complete IRQ %d (secure %d)"
+nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
+nvic_set_nmi_level(int level) "NVIC external NMI level set to %d"
+nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+nvic_sysreg_write(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+
 # virt-acpi-build.c
 virt_acpi_setup(void) "No fw cfg or ACPI disabled. Bailing out."
 
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index f726d87532..2d981abb4e 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -26,7 +26,6 @@ obj-$(CONFIG_APIC) += apic.o apic_common.o
 obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
 obj-$(call land,$(CONFIG_ARM_GIC_KVM),$(TARGET_AARCH64)) += arm_gicv3_kvm.o
 obj-$(call land,$(CONFIG_ARM_GIC_KVM),$(TARGET_AARCH64)) += arm_gicv3_its_kvm.o
-obj-$(CONFIG_ARM_V7M) += armv7m_nvic.o
 obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
 obj-$(CONFIG_GRLIB) += grlib_irqmp.o
 obj-$(CONFIG_IOAPIC) += ioapic.o
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 90c9d07c1a..09a7fedee8 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -179,23 +179,6 @@ gicv3_redist_badwrite(uint32_t cpu, uint64_t offset, uint64_t data, unsigned siz
 gicv3_redist_set_irq(uint32_t cpu, int irq, int level) "GICv3 redistributor 0x%x interrupt %d level changed to %d"
 gicv3_redist_send_sgi(uint32_t cpu, int irq) "GICv3 redistributor 0x%x pending SGI %d"
 
-# armv7m_nvic.c
-nvic_recompute_state(int vectpending, int vectpending_prio, int exception_prio) "NVIC state recomputed: vectpending %d vectpending_prio %d exception_prio %d"
-nvic_recompute_state_secure(int vectpending, bool vectpending_is_s_banked, int vectpending_prio, int exception_prio) "NVIC state recomputed: vectpending %d is_s_banked %d vectpending_prio %d exception_prio %d"
-nvic_set_prio(int irq, bool secure, uint8_t prio) "NVIC set irq %d secure-bank %d priority %d"
-nvic_irq_update(int vectpending, int pendprio, int exception_prio, int level) "NVIC vectpending %d pending prio %d exception_prio %d: setting irq line to %d"
-nvic_escalate_prio(int irq, int irqprio, int runprio) "NVIC escalating irq %d to HardFault: insufficient priority %d >= %d"
-nvic_escalate_disabled(int irq) "NVIC escalating irq %d to HardFault: disabled"
-nvic_set_pending(int irq, bool secure, bool targets_secure, bool derived, int en, int prio) "NVIC set pending irq %d secure-bank %d targets_secure %d derived %d (enabled: %d priority %d)"
-nvic_clear_pending(int irq, bool secure, int en, int prio) "NVIC clear pending irq %d secure-bank %d (enabled: %d priority %d)"
-nvic_acknowledge_irq(int irq, int prio) "NVIC acknowledge IRQ: %d now active (prio %d)"
-nvic_get_pending_irq_info(int irq, bool secure) "NVIC next IRQ %d: targets_secure: %d"
-nvic_complete_irq(int irq, bool secure) "NVIC complete IRQ %d (secure %d)"
-nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
-nvic_set_nmi_level(int level) "NVIC external NMI level set to %d"
-nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
-nvic_sysreg_write(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
-
 # heathrow_pic.c
 heathrow_write(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" %u: 0x%"PRIx64
 heathrow_read(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" %u: 0x%"PRIx64
-- 
2.18.1

[PATCH 4/4] default-configs: Do not enforce CONFIG_ARM_V7M anymore

[Thomas Huth]

The arm builds can now be done without CONFIG_ARM_V7M, so do not
enforce this config switch anymore, it's getting selected in
hw/arm/Kconfig automatically if needed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 default-configs/arm-softmmu.mak | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 1f2e0e7fde..64f5ac24bf 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -1,8 +1,5 @@
 # Default configuration for arm-softmmu
 
-# TODO: ARM_V7M is currently always required - make this more flexible!
-CONFIG_ARM_V7M=y
-
 # CONFIG_PCI_DEVICES=n
 # CONFIG_TEST_DEVICES=n
 
-- 
2.18.1

Re: [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n

[Philippe Mathieu-Daudé]

Hi Thomas,

On 9/21/19 5:04 PM, Thomas Huth wrote:
> We've got CONFIG_ARM_V7M, but it currently can't be disabled.
> Here are some patches that should allow to disable the switch
> (if the corresponding boards are disabled, too).

What about the ARMv4/v5/v6/v7r?

Re: [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n

[Thomas Huth]

On 23/09/2019 10.37, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 9/21/19 5:04 PM, Thomas Huth wrote:
>> We've got CONFIG_ARM_V7M, but it currently can't be disabled.
>> Here are some patches that should allow to disable the switch
>> (if the corresponding boards are disabled, too).
> 
> What about the ARMv4/v5/v6/v7r?

We don't have config switches for these yet, do we? You started to
introduce them in some of your TCG-disablement patches which are not
merged yet, so that's something that should be addressed once they are
merged, or directly in your series. Here, I wanted to provide you with
these patches so that the arm_cpu_register() is available for your
patches / later clean-up patches ... I mentioned that in the RFC cover
letter
(https://www.mail-archive.com/qemu-devel@nongnu.org/msg641907.html) but
forgot to copy / update that paragraph for this cover letter here again :-/

 Thomas

Re: [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n

[Philippe Mathieu-Daudé]

On 9/23/19 10:50 AM, Thomas Huth wrote:
> On 23/09/2019 10.37, Philippe Mathieu-Daudé wrote:
>> Hi Thomas,
>>
>> On 9/21/19 5:04 PM, Thomas Huth wrote:
>>> We've got CONFIG_ARM_V7M, but it currently can't be disabled.
>>> Here are some patches that should allow to disable the switch
>>> (if the corresponding boards are disabled, too).
>>
>> What about the ARMv4/v5/v6/v7r?
> 
> We don't have config switches for these yet, do we? You started to
> introduce them in some of your TCG-disablement patches which are not
> merged yet, so that's something that should be addressed once they are
> merged, or directly in your series. Here, I wanted to provide you with
> these patches so that the arm_cpu_register() is available for your
> patches / later clean-up patches ... I mentioned that in the RFC cover
> letter
> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg641907.html) but
> forgot to copy / update that paragraph for this cover letter here again :-/

Oh OK, I thought it was a complete replacement of my series, now I
understand this only address the V7M case which is the sole switch
available as of today.

Thanks,

Phil.

Re: [PATCH 1/4] target/arm: Make cpu_register() and set_feature() available for other files

[Auger Eric]

Hi Thomas,

On 9/21/19 5:04 PM, Thomas Huth wrote:
> Move the common set_feature() and unset_feature() functions from cpu.c and
> cpu64.c to internals.h, and make cpu_register() (renamed to arm_cpu_register())
> available from there, too, so we can register CPUs also from other files
> in the future.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  target/arm/cpu.c       | 20 ++------------------
>  target/arm/cpu64.c     | 17 +----------------
>  target/arm/internals.h | 18 ++++++++++++++++++
>  3 files changed, 21 insertions(+), 34 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2399c14471..f1f9eecdc8 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -614,16 +614,6 @@ static bool arm_cpu_virtio_is_big_endian(CPUState *cs)
>  
>  #endif
>  
> -static inline void set_feature(CPUARMState *env, int feature)
> -{
> -    env->features |= 1ULL << feature;
> -}
> -
> -static inline void unset_feature(CPUARMState *env, int feature)
> -{
> -    env->features &= ~(1ULL << feature);
> -}
> -
>  static int
>  print_insn_thumb1(bfd_vma pc, disassemble_info *info)
>  {
> @@ -2515,12 +2505,6 @@ static void arm_max_initfn(Object *obj)
>  
>  #endif /* !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) */
>  
> -struct ARMCPUInfo {
> -    const char *name;
> -    void (*initfn)(Object *obj);
> -    void (*class_init)(ObjectClass *oc, void *data);
> -};
> -
>  static const ARMCPUInfo arm_cpus[] = {
>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>      { .name = "arm926",      .initfn = arm926_initfn },
> @@ -2681,7 +2665,7 @@ static void cpu_register_class_init(ObjectClass *oc, void *data)
>      acc->info = data;
>  }
>  
> -static void cpu_register(const ARMCPUInfo *info)
> +void arm_cpu_register(const ARMCPUInfo *info)
>  {
>      TypeInfo type_info = {
>          .parent = TYPE_ARM_CPU,
> @@ -2722,7 +2706,7 @@ static void arm_cpu_register_types(void)
>      type_register_static(&idau_interface_type_info);
>  
>      while (info->name) {
> -        cpu_register(info);
> +        arm_cpu_register(info);
>          info++;
>      }
>  
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index d7f5bf610a..5239ba5529 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -21,6 +21,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "cpu.h"
> +#include "internals.h"
>  #include "qemu/module.h"
>  #if !defined(CONFIG_USER_ONLY)
>  #include "hw/loader.h"
> @@ -29,16 +30,6 @@
>  #include "kvm_arm.h"
>  #include "qapi/visitor.h"
>  
> -static inline void set_feature(CPUARMState *env, int feature)
> -{
> -    env->features |= 1ULL << feature;
> -}
> -
> -static inline void unset_feature(CPUARMState *env, int feature)
> -{
> -    env->features &= ~(1ULL << feature);
> -}
> -
>  #ifndef CONFIG_USER_ONLY
>  static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> @@ -396,12 +387,6 @@ static void aarch64_max_initfn(Object *obj)
>      }
>  }
>  
> -struct ARMCPUInfo {
> -    const char *name;
> -    void (*initfn)(Object *obj);
> -    void (*class_init)(ObjectClass *oc, void *data);
> -};
> -
>  static const ARMCPUInfo aarch64_cpus[] = {
>      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
>      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 232d963875..e71196ed5f 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1046,4 +1046,22 @@ void arm_log_exception(int idx);
>  
>  #endif /* !CONFIG_USER_ONLY */
>  
> +static inline void set_feature(CPUARMState *env, int feature)
> +{
> +    env->features |= 1ULL << feature;
> +}
> +
> +static inline void unset_feature(CPUARMState *env, int feature)
> +{
> +    env->features &= ~(1ULL << feature);
> +}
> +
> +struct ARMCPUInfo {
> +    const char *name;
> +    void (*initfn)(Object *obj);
> +    void (*class_init)(ObjectClass *oc, void *data);
> +};
> +
> +void arm_cpu_register(const ARMCPUInfo *info);
> +
>  #endif
> 

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Auger Eric]

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

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Peter Maydell]

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

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Peter Maydell]

On Sat, 21 Sep 2019 at 16:04, Thomas Huth <thuth@redhat.com> wrote:
>
> qemu-system-arm/-aarch64 currently can't be built without setting the
> switch CONFIG_ARM_V7M=y - which we currently always do in the config file
> default-configs/arm-softmmu.mak. This is because the code in target/arm/
> calls many functions from this armv7m_nvic.c, and thus linking fails
> without this file.
>
> So armv7m_nvic.c should not be under the CONFIG_ARM_V7M switch, but always
> compiled for arm builds. Since we can not simply do this in hw/intc/ (with
> "obj-y += ..." it would get compiled for all other architectures, too),
> let's move the file to hw/arm/ instead and always enable it there.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/arm/Makefile.objs           |  2 ++
>  hw/{intc => arm}/armv7m_nvic.c |  0
>  hw/arm/trace-events            | 17 +++++++++++++++++
>  hw/intc/Makefile.objs          |  1 -
>  hw/intc/trace-events           | 17 -----------------
>  5 files changed, 19 insertions(+), 18 deletions(-)
>  rename hw/{intc => arm}/armv7m_nvic.c (100%)

Please don't move this file. This is an interrupt
controller (with some other functionality jammed into it)
and so it should be in hw/intc. It's true that it's rather
tightly-coupled to the CPU, but the same is true for our
GICv3 model.

thanks
-- PMM

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Thomas Huth]

On 23/09/2019 16.52, Peter Maydell wrote:
> On Sat, 21 Sep 2019 at 16:04, Thomas Huth <thuth@redhat.com> wrote:
>>
>> qemu-system-arm/-aarch64 currently can't be built without setting the
>> switch CONFIG_ARM_V7M=y - which we currently always do in the config file
>> default-configs/arm-softmmu.mak. This is because the code in target/arm/
>> calls many functions from this armv7m_nvic.c, and thus linking fails
>> without this file.
>>
>> So armv7m_nvic.c should not be under the CONFIG_ARM_V7M switch, but always
>> compiled for arm builds. Since we can not simply do this in hw/intc/ (with
>> "obj-y += ..." it would get compiled for all other architectures, too),
>> let's move the file to hw/arm/ instead and always enable it there.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/arm/Makefile.objs           |  2 ++
>>  hw/{intc => arm}/armv7m_nvic.c |  0
>>  hw/arm/trace-events            | 17 +++++++++++++++++
>>  hw/intc/Makefile.objs          |  1 -
>>  hw/intc/trace-events           | 17 -----------------
>>  5 files changed, 19 insertions(+), 18 deletions(-)
>>  rename hw/{intc => arm}/armv7m_nvic.c (100%)
> 
> Please don't move this file. This is an interrupt
> controller (with some other functionality jammed into it)
> and so it should be in hw/intc. It's true that it's rather
> tightly-coupled to the CPU, but the same is true for our
> GICv3 model.

Ok, then what would you suggest to solve the problem that this file has
always to be linked into the binary? I can't use "obj-y += ..." in
hw/intc/Makefile.objs since that would mean that the file also gets
compiled for non-Arm boards. Would you prefer a bunch of stubs instead
that get used if CONFIG_ARM_V7M is not set?

 Thomas

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Thomas Huth]

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

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Peter Maydell]

On Mon, 23 Sep 2019 at 18:54, Thomas Huth <thuth@redhat.com> wrote:
> Ok, then what would you suggest to solve the problem that this file has
> always to be linked into the binary? I can't use "obj-y += ..." in
> hw/intc/Makefile.objs since that would mean that the file also gets
> compiled for non-Arm boards. Would you prefer a bunch of stubs instead
> that get used if CONFIG_ARM_V7M is not set?

I thought obj-y was for only-this-target and obj-common-y was
for all-boards ?

thanks
-- PMM

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Thomas Huth]

On 23/09/2019 20.27, Peter Maydell wrote:
> On Mon, 23 Sep 2019 at 18:54, Thomas Huth <thuth@redhat.com> wrote:
>> Ok, then what would you suggest to solve the problem that this file has
>> always to be linked into the binary? I can't use "obj-y += ..." in
>> hw/intc/Makefile.objs since that would mean that the file also gets
>> compiled for non-Arm boards. Would you prefer a bunch of stubs instead
>> that get used if CONFIG_ARM_V7M is not set?
> 
> I thought obj-y was for only-this-target and obj-common-y was
> for all-boards ?

Well, obj-y is for the current target that gets compiled. But if you use
it in a Makefile that gets used by all targets, the file gets compiled
for each target individually.

Just try to change "obj-$(CONFIG_ARM_V7M) += armv7m_nvic.o" into
"obj-y += armv7m_nvic.o" in hw/int/Makefile.objs, and you'll see it break:

  CC      alpha-softmmu/hw/intc/armv7m_nvic.o
In file included from include/hw/intc/armv7m_nvic.h:13,
                 from hw/intc/armv7m_nvic.c:19:
target/arm/cpu.h:1416: error: "FPCR_DZE" redefined [-Werror]
 #define FPCR_DZE    (1 << 9)    /* Divide by Zero exception trap enable */

 Thomas

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Peter Maydell]

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

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Peter Maydell]

On Mon, 23 Sep 2019 at 19:36, Thomas Huth <thuth@redhat.com> wrote:
>
> On 23/09/2019 20.27, Peter Maydell wrote:
> > On Mon, 23 Sep 2019 at 18:54, Thomas Huth <thuth@redhat.com> wrote:
> >> Ok, then what would you suggest to solve the problem that this file has
> >> always to be linked into the binary? I can't use "obj-y += ..." in
> >> hw/intc/Makefile.objs since that would mean that the file also gets
> >> compiled for non-Arm boards. Would you prefer a bunch of stubs instead
> >> that get used if CONFIG_ARM_V7M is not set?
> >
> > I thought obj-y was for only-this-target and obj-common-y was
> > for all-boards ?
>
> Well, obj-y is for the current target that gets compiled. But if you use
> it in a Makefile that gets used by all targets, the file gets compiled
> for each target individually.
>
> Just try to change "obj-$(CONFIG_ARM_V7M) += armv7m_nvic.o" into
> "obj-y += armv7m_nvic.o" in hw/int/Makefile.objs, and you'll see it break:
>
>   CC      alpha-softmmu/hw/intc/armv7m_nvic.o
> In file included from include/hw/intc/armv7m_nvic.h:13,
>                  from hw/intc/armv7m_nvic.c:19:
> target/arm/cpu.h:1416: error: "FPCR_DZE" redefined [-Werror]
>  #define FPCR_DZE    (1 << 9)    /* Divide by Zero exception trap enable */

Sure, so don't define CONFIG_ARM_V7M in a default-config for
a non-Arm architecture. Then you get the behaviour you want:
the file is compiled only for the arm targets.

This is exactly the same as for the GICv3's arm_gicv3_cpuif.c, which
is also protected by a CONFIG_ flag that's only set by the Arm
default-configs, because it needs to be compiled as a target-specific
file for arm and only arm.

thanks
-- PMM

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Thomas Huth]

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

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Peter Maydell]

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

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Thomas Huth]

On 23/09/2019 20.50, Peter Maydell wrote:
> On Mon, 23 Sep 2019 at 19:36, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 23/09/2019 20.27, Peter Maydell wrote:
>>> On Mon, 23 Sep 2019 at 18:54, Thomas Huth <thuth@redhat.com> wrote:
>>>> Ok, then what would you suggest to solve the problem that this file has
>>>> always to be linked into the binary? I can't use "obj-y += ..." in
>>>> hw/intc/Makefile.objs since that would mean that the file also gets
>>>> compiled for non-Arm boards. Would you prefer a bunch of stubs instead
>>>> that get used if CONFIG_ARM_V7M is not set?
>>>
>>> I thought obj-y was for only-this-target and obj-common-y was
>>> for all-boards ?
>>
>> Well, obj-y is for the current target that gets compiled. But if you use
>> it in a Makefile that gets used by all targets, the file gets compiled
>> for each target individually.
>>
>> Just try to change "obj-$(CONFIG_ARM_V7M) += armv7m_nvic.o" into
>> "obj-y += armv7m_nvic.o" in hw/int/Makefile.objs, and you'll see it break:
>>
>>   CC      alpha-softmmu/hw/intc/armv7m_nvic.o
>> In file included from include/hw/intc/armv7m_nvic.h:13,
>>                  from hw/intc/armv7m_nvic.c:19:
>> target/arm/cpu.h:1416: error: "FPCR_DZE" redefined [-Werror]
>>  #define FPCR_DZE    (1 << 9)    /* Divide by Zero exception trap enable */
> 
> Sure, so don't define CONFIG_ARM_V7M in a default-config for
> a non-Arm architecture. Then you get the behaviour you want:
> the file is compiled only for the arm targets.

Sigh, the point of this series is that it should also possible to
compile *without* CONFIG_ARM_V7M in default-configs if you want (and
yes, there are people out there who want to be able to compile a
minimalistic QEMU). It's currently not possible to disable this switch.
But ok, if you're not really interested in providing a possibility to
make qemu-system-arm a little bit more flexible in this regard, never
mind, I'll look into other issues instead.

 Thomas

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Peter Maydell]

On Tue, 24 Sep 2019 at 05:44, Thomas Huth <thuth@redhat.com> wrote:
>
> On 23/09/2019 20.50, Peter Maydell wrote:
> > On Mon, 23 Sep 2019 at 19:36, Thomas Huth <thuth@redhat.com> wrote:
> >> Just try to change "obj-$(CONFIG_ARM_V7M) += armv7m_nvic.o" into
> >> "obj-y += armv7m_nvic.o" in hw/int/Makefile.objs, and you'll see it break:
> >>
> >>   CC      alpha-softmmu/hw/intc/armv7m_nvic.o
> >> In file included from include/hw/intc/armv7m_nvic.h:13,
> >>                  from hw/intc/armv7m_nvic.c:19:
> >> target/arm/cpu.h:1416: error: "FPCR_DZE" redefined [-Werror]
> >>  #define FPCR_DZE    (1 << 9)    /* Divide by Zero exception trap enable */
> >
> > Sure, so don't define CONFIG_ARM_V7M in a default-config for
> > a non-Arm architecture. Then you get the behaviour you want:
> > the file is compiled only for the arm targets.
>
> Sigh, the point of this series is that it should also possible to
> compile *without* CONFIG_ARM_V7M in default-configs if you want (and
> yes, there are people out there who want to be able to compile a
> minimalistic QEMU). It's currently not possible to disable this switch.
> But ok, if you're not really interested in providing a possibility to
> make qemu-system-arm a little bit more flexible in this regard, never
> mind, I'll look into other issues instead.

No, I'm happy that we should be able to compile without CONFIG_ARM_V7M
selected. I'm just confused about why you think this requires that
we move this file out of hw/intc.

Case 1: arm target, CONFIG_ARM_V7M=y (presumably in a Kconfig world
this is set by default if the user doesn't flip that switch):
obj-$(CONFIG_ARM_V7M) expands to obj-y, file compiled, OK
Case 2: arm target, CONFIG_ARM_V7M=n set by user via Kconfig:
obj-$(CONFIG_ARM_V7M) expands to obj-n, file not compiled, which is
also what we want Case 3: not an arm target, CONFIG_ARM_V7M will be
either 'n' (or empty?) and is not settable by user: expands to obj-n
again, which is also what we want

Is there some Kconfig restriction I'm not aware of that
messes things up?

thanks
-- PMM

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Thomas Huth]

On 24/09/2019 11.42, Peter Maydell wrote:
> On Tue, 24 Sep 2019 at 05:44, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 23/09/2019 20.50, Peter Maydell wrote:
>>> On Mon, 23 Sep 2019 at 19:36, Thomas Huth <thuth@redhat.com> wrote:
>>>> Just try to change "obj-$(CONFIG_ARM_V7M) += armv7m_nvic.o" into
>>>> "obj-y += armv7m_nvic.o" in hw/int/Makefile.objs, and you'll see it break:
>>>>
>>>>   CC      alpha-softmmu/hw/intc/armv7m_nvic.o
>>>> In file included from include/hw/intc/armv7m_nvic.h:13,
>>>>                  from hw/intc/armv7m_nvic.c:19:
>>>> target/arm/cpu.h:1416: error: "FPCR_DZE" redefined [-Werror]
>>>>  #define FPCR_DZE    (1 << 9)    /* Divide by Zero exception trap enable */
>>>
>>> Sure, so don't define CONFIG_ARM_V7M in a default-config for
>>> a non-Arm architecture. Then you get the behaviour you want:
>>> the file is compiled only for the arm targets.
>>
>> Sigh, the point of this series is that it should also possible to
>> compile *without* CONFIG_ARM_V7M in default-configs if you want (and
>> yes, there are people out there who want to be able to compile a
>> minimalistic QEMU). It's currently not possible to disable this switch.
>> But ok, if you're not really interested in providing a possibility to
>> make qemu-system-arm a little bit more flexible in this regard, never
>> mind, I'll look into other issues instead.
> 
> No, I'm happy that we should be able to compile without CONFIG_ARM_V7M
> selected. I'm just confused about why you think this requires that
> we move this file out of hw/intc.
> 
> Case 1: arm target, CONFIG_ARM_V7M=y (presumably in a Kconfig world
> this is set by default if the user doesn't flip that switch):
> obj-$(CONFIG_ARM_V7M) expands to obj-y, file compiled, OK
> Case 2: arm target, CONFIG_ARM_V7M=n set by user via Kconfig:
> obj-$(CONFIG_ARM_V7M) expands to obj-n, file not compiled, which is
> also what we want
The problem is this "case 2" - it does not work. For example, try to
delete everything from default-configs/aarch64-softmmu.mak (especially
the "include"), and just stick a "CONFIG_ARM_VIRT=y" in there.
Linking of qemu-system-aarch64 will fail with lots of "undefined
reference to `armv7m_nvic_set_pending'" etc. messages.

 Thomas

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds

[Philippe Mathieu-Daudé]

On 9/24/19 11:48 AM, Thomas Huth wrote:
> On 24/09/2019 11.42, Peter Maydell wrote:
>> On Tue, 24 Sep 2019 at 05:44, Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> On 23/09/2019 20.50, Peter Maydell wrote:
>>>> On Mon, 23 Sep 2019 at 19:36, Thomas Huth <thuth@redhat.com> wrote:
>>>>> Just try to change "obj-$(CONFIG_ARM_V7M) += armv7m_nvic.o" into
>>>>> "obj-y += armv7m_nvic.o" in hw/int/Makefile.objs, and you'll see it break:
>>>>>
>>>>>   CC      alpha-softmmu/hw/intc/armv7m_nvic.o
>>>>> In file included from include/hw/intc/armv7m_nvic.h:13,
>>>>>                  from hw/intc/armv7m_nvic.c:19:
>>>>> target/arm/cpu.h:1416: error: "FPCR_DZE" redefined [-Werror]
>>>>>  #define FPCR_DZE    (1 << 9)    /* Divide by Zero exception trap enable */
>>>>
>>>> Sure, so don't define CONFIG_ARM_V7M in a default-config for
>>>> a non-Arm architecture. Then you get the behaviour you want:
>>>> the file is compiled only for the arm targets.
>>>
>>> Sigh, the point of this series is that it should also possible to
>>> compile *without* CONFIG_ARM_V7M in default-configs if you want (and
>>> yes, there are people out there who want to be able to compile a
>>> minimalistic QEMU). It's currently not possible to disable this switch.
>>> But ok, if you're not really interested in providing a possibility to
>>> make qemu-system-arm a little bit more flexible in this regard, never
>>> mind, I'll look into other issues instead.
>>
>> No, I'm happy that we should be able to compile without CONFIG_ARM_V7M
>> selected. I'm just confused about why you think this requires that
>> we move this file out of hw/intc.
>>
>> Case 1: arm target, CONFIG_ARM_V7M=y (presumably in a Kconfig world
>> this is set by default if the user doesn't flip that switch):
>> obj-$(CONFIG_ARM_V7M) expands to obj-y, file compiled, OK
>> Case 2: arm target, CONFIG_ARM_V7M=n set by user via Kconfig:
>> obj-$(CONFIG_ARM_V7M) expands to obj-n, file not compiled, which is
>> also what we want
> The problem is this "case 2" - it does not work. For example, try to
> delete everything from default-configs/aarch64-softmmu.mak (especially
> the "include"), and just stick a "CONFIG_ARM_VIRT=y" in there.
> Linking of qemu-system-aarch64 will fail with lots of "undefined
> reference to `armv7m_nvic_set_pending'" etc. messages.

This is what I tried to fix with this patch:
"target/arm: Do not build A/M-profile cpus when using KVM"
https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg05006.html

I addressed Richard/your's review comments, so the series is ready for
respin.

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Auger Eric]

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
> 

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Thomas Huth]

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

Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c

[Auger Eric]

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
>