* [PATCH 1/4] target/arm: Make cpu_register() and set_feature() available for other files
2019-09-21 15:04 [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Thomas Huth
@ 2019-09-21 15:04 ` Thomas Huth
2019-09-23 14:28 ` Auger Eric
2019-09-21 15:04 ` [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c Thomas Huth
` (3 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-21 15:04 UTC (permalink / raw)
To: qemu-devel, Peter Maydell
Cc: Philippe Mathieu-Daudé, qemu-arm, Richard Henderson
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] target/arm: Make cpu_register() and set_feature() available for other files
2019-09-21 15:04 ` [PATCH 1/4] target/arm: Make cpu_register() and set_feature() available for other files Thomas Huth
@ 2019-09-23 14:28 ` Auger Eric
0 siblings, 0 replies; 28+ messages in thread
From: Auger Eric @ 2019-09-23 14:28 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-21 15:04 [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Thomas Huth
2019-09-21 15:04 ` [PATCH 1/4] target/arm: Make cpu_register() and set_feature() available for other files Thomas Huth
@ 2019-09-21 15:04 ` Thomas Huth
2019-09-23 14:31 ` Auger Eric
2019-09-23 14:34 ` Peter Maydell
2019-09-21 15:04 ` [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds Thomas Huth
` (2 subsequent siblings)
4 siblings, 2 replies; 28+ messages in thread
From: Thomas Huth @ 2019-09-21 15:04 UTC (permalink / raw)
To: qemu-devel, Peter Maydell
Cc: Philippe Mathieu-Daudé, qemu-arm, Richard Henderson
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-21 15:04 ` [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c Thomas Huth
@ 2019-09-23 14:31 ` Auger Eric
2019-09-23 18:09 ` Thomas Huth
2019-09-23 14:34 ` Peter Maydell
1 sibling, 1 reply; 28+ messages in thread
From: Auger Eric @ 2019-09-23 14:31 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-23 14:31 ` Auger Eric
@ 2019-09-23 18:09 ` Thomas Huth
2019-09-23 18:45 ` Peter Maydell
2019-09-24 11:02 ` Auger Eric
0 siblings, 2 replies; 28+ messages in thread
From: Thomas Huth @ 2019-09-23 18:09 UTC (permalink / raw)
To: Auger Eric, qemu-devel, Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-23 18:09 ` Thomas Huth
@ 2019-09-23 18:45 ` Peter Maydell
2019-09-23 18:51 ` Thomas Huth
2019-09-24 11:02 ` Auger Eric
1 sibling, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2019-09-23 18:45 UTC (permalink / raw)
To: Thomas Huth
Cc: Auger Eric, Richard Henderson, qemu-arm,
Philippe Mathieu-Daudé,
QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-23 18:45 ` Peter Maydell
@ 2019-09-23 18:51 ` Thomas Huth
2019-09-23 18:54 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-23 18:51 UTC (permalink / raw)
To: Peter Maydell
Cc: Auger Eric, Richard Henderson, qemu-arm,
Philippe Mathieu-Daudé,
QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-23 18:51 ` Thomas Huth
@ 2019-09-23 18:54 ` Peter Maydell
0 siblings, 0 replies; 28+ messages in thread
From: Peter Maydell @ 2019-09-23 18:54 UTC (permalink / raw)
To: Thomas Huth
Cc: Auger Eric, Richard Henderson, qemu-arm,
Philippe Mathieu-Daudé,
QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-23 18:09 ` Thomas Huth
2019-09-23 18:45 ` Peter Maydell
@ 2019-09-24 11:02 ` Auger Eric
2019-09-24 11:06 ` Thomas Huth
1 sibling, 1 reply; 28+ messages in thread
From: Auger Eric @ 2019-09-24 11:02 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-24 11:02 ` Auger Eric
@ 2019-09-24 11:06 ` Thomas Huth
2019-09-24 11:24 ` Auger Eric
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-24 11:06 UTC (permalink / raw)
To: Auger Eric, qemu-devel, Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-24 11:06 ` Thomas Huth
@ 2019-09-24 11:24 ` Auger Eric
0 siblings, 0 replies; 28+ messages in thread
From: Auger Eric @ 2019-09-24 11:24 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-21 15:04 ` [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c Thomas Huth
2019-09-23 14:31 ` Auger Eric
@ 2019-09-23 14:34 ` Peter Maydell
2019-09-24 9:52 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2019-09-23 14:34 UTC (permalink / raw)
To: Thomas Huth
Cc: Philippe Mathieu-Daudé,
qemu-arm, Richard Henderson, QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c
2019-09-23 14:34 ` Peter Maydell
@ 2019-09-24 9:52 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-24 9:52 UTC (permalink / raw)
To: Peter Maydell, Thomas Huth; +Cc: qemu-arm, Richard Henderson, QEMU Developers
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-21 15:04 [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Thomas Huth
2019-09-21 15:04 ` [PATCH 1/4] target/arm: Make cpu_register() and set_feature() available for other files Thomas Huth
2019-09-21 15:04 ` [PATCH 2/4] target/arm: Move cortex-m related functions to new file v7m.c Thomas Huth
@ 2019-09-21 15:04 ` Thomas Huth
2019-09-23 14:52 ` Peter Maydell
2019-09-21 15:04 ` [PATCH 4/4] default-configs: Do not enforce CONFIG_ARM_V7M anymore Thomas Huth
2019-09-23 8:37 ` [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Philippe Mathieu-Daudé
4 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-21 15:04 UTC (permalink / raw)
To: qemu-devel, Peter Maydell
Cc: Philippe Mathieu-Daudé, qemu-arm, Richard Henderson
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-21 15:04 ` [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds Thomas Huth
@ 2019-09-23 14:52 ` Peter Maydell
2019-09-23 17:54 ` Thomas Huth
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2019-09-23 14:52 UTC (permalink / raw)
To: Thomas Huth
Cc: Philippe Mathieu-Daudé,
qemu-arm, Richard Henderson, QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-23 14:52 ` Peter Maydell
@ 2019-09-23 17:54 ` Thomas Huth
2019-09-23 18:27 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-23 17:54 UTC (permalink / raw)
To: Peter Maydell
Cc: Philippe Mathieu-Daudé,
qemu-arm, Richard Henderson, QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-23 17:54 ` Thomas Huth
@ 2019-09-23 18:27 ` Peter Maydell
2019-09-23 18:36 ` Thomas Huth
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2019-09-23 18:27 UTC (permalink / raw)
To: Thomas Huth
Cc: Philippe Mathieu-Daudé,
qemu-arm, Richard Henderson, QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-23 18:27 ` Peter Maydell
@ 2019-09-23 18:36 ` Thomas Huth
2019-09-23 18:50 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-23 18:36 UTC (permalink / raw)
To: Peter Maydell
Cc: Philippe Mathieu-Daudé,
qemu-arm, Richard Henderson, QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-23 18:36 ` Thomas Huth
@ 2019-09-23 18:50 ` Peter Maydell
2019-09-24 4:44 ` Thomas Huth
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2019-09-23 18:50 UTC (permalink / raw)
To: Thomas Huth
Cc: Philippe Mathieu-Daudé,
qemu-arm, Richard Henderson, QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-23 18:50 ` Peter Maydell
@ 2019-09-24 4:44 ` Thomas Huth
2019-09-24 9:42 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-24 4:44 UTC (permalink / raw)
To: Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé,
QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-24 4:44 ` Thomas Huth
@ 2019-09-24 9:42 ` Peter Maydell
2019-09-24 9:48 ` Thomas Huth
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2019-09-24 9:42 UTC (permalink / raw)
To: Thomas Huth
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé,
QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-24 9:42 ` Peter Maydell
@ 2019-09-24 9:48 ` Thomas Huth
2019-09-24 10:01 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-24 9:48 UTC (permalink / raw)
To: Peter Maydell
Cc: Richard Henderson, qemu-arm, Philippe Mathieu-Daudé,
QEMU Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds
2019-09-24 9:48 ` Thomas Huth
@ 2019-09-24 10:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-24 10:01 UTC (permalink / raw)
To: Thomas Huth, Peter Maydell; +Cc: qemu-arm, Richard Henderson, QEMU Developers
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/4] default-configs: Do not enforce CONFIG_ARM_V7M anymore
2019-09-21 15:04 [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Thomas Huth
` (2 preceding siblings ...)
2019-09-21 15:04 ` [PATCH 3/4] hw/arm: Move armv7m_nvic.c to hw/arm/ and always enable it for arm builds Thomas Huth
@ 2019-09-21 15:04 ` Thomas Huth
2019-09-23 8:37 ` [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Philippe Mathieu-Daudé
4 siblings, 0 replies; 28+ messages in thread
From: Thomas Huth @ 2019-09-21 15:04 UTC (permalink / raw)
To: qemu-devel, Peter Maydell
Cc: Philippe Mathieu-Daudé, qemu-arm, Richard Henderson
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n
2019-09-21 15:04 [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Thomas Huth
` (3 preceding siblings ...)
2019-09-21 15:04 ` [PATCH 4/4] default-configs: Do not enforce CONFIG_ARM_V7M anymore Thomas Huth
@ 2019-09-23 8:37 ` Philippe Mathieu-Daudé
2019-09-23 8:50 ` Thomas Huth
4 siblings, 1 reply; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-23 8:37 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Peter Maydell; +Cc: qemu-arm, Richard Henderson
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?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n
2019-09-23 8:37 ` [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n Philippe Mathieu-Daudé
@ 2019-09-23 8:50 ` Thomas Huth
2019-09-23 8:58 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2019-09-23 8:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, Peter Maydell
Cc: qemu-arm, Richard Henderson
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/4] Make it possible to compile with CONFIG_ARM_V7M=n
2019-09-23 8:50 ` Thomas Huth
@ 2019-09-23 8:58 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-23 8:58 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Peter Maydell; +Cc: qemu-arm, Richard Henderson
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.
^ permalink raw reply [flat|nested] 28+ messages in thread