* [PATCH 1/3] target/arm: Don't skip M-profile reset entirely in user mode
2021-09-14 12:07 [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode Peter Maydell
@ 2021-09-14 12:07 ` Peter Maydell
2021-09-14 12:07 ` [PATCH 2/3] target/arm: Always clear exclusive monitor on reset Peter Maydell
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2021-09-14 12:07 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Christophe Lyon
Currently all of the M-profile specific code in arm_cpu_reset() is
inside a !defined(CONFIG_USER_ONLY) ifdef block. This is
unintentional: it happened because originally the only
M-profile-specific handling was the setup of the initial SP and PC
from the vector table, which is system-emulation only. But then we
added a lot of other M-profile setup to the same "if (ARM_FEATURE_M)"
code block without noticing that it was all inside a not-user-mode
ifdef. This has generally been harmless, but with the addition of
v8.1M low-overhead-loop support we ran into a problem: the reset of
FPSCR.LTPSIZE to 4 was only being done for system emulation mode, so
if a user-mode guest tried to execute the LE instruction it would
incorrectly take a UsageFault.
Adjust the ifdefs so only the really system-emulation specific parts
are covered. Because this means we now run some reset code that sets
up initial values in the FPCCR and similar FPU related registers,
explicitly set up the registers controlling FPU context handling in
user-emulation mode so that the FPU works by design and not by
chance.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/613
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/cpu.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index d631c4683c4..98eed6c6d31 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -265,12 +265,15 @@ static void arm_cpu_reset(DeviceState *dev)
env->uncached_cpsr = ARM_CPU_MODE_SVC;
}
env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
+#endif
if (arm_feature(env, ARM_FEATURE_M)) {
+#ifndef CONFIG_USER_ONLY
uint32_t initial_msp; /* Loaded from 0x0 */
uint32_t initial_pc; /* Loaded from 0x4 */
uint8_t *rom;
uint32_t vecbase;
+#endif
if (cpu_isar_feature(aa32_lob, cpu)) {
/*
@@ -324,6 +327,8 @@ static void arm_cpu_reset(DeviceState *dev)
env->v7m.fpccr[M_REG_S] = R_V7M_FPCCR_ASPEN_MASK |
R_V7M_FPCCR_LSPEN_MASK | R_V7M_FPCCR_S_MASK;
}
+
+#ifndef CONFIG_USER_ONLY
/* Unlike A/R profile, M profile defines the reset LR value */
env->regs[14] = 0xffffffff;
@@ -352,8 +357,22 @@ static void arm_cpu_reset(DeviceState *dev)
env->regs[13] = initial_msp & 0xFFFFFFFC;
env->regs[15] = initial_pc & ~1;
env->thumb = initial_pc & 1;
+#else
+ /*
+ * For user mode we run non-secure and with access to the FPU.
+ * The FPU context is active (ie does not need further setup)
+ * and is owned by non-secure.
+ */
+ env->v7m.secure = false;
+ env->v7m.nsacr = 0xcff;
+ env->v7m.cpacr[M_REG_NS] = 0xf0ffff;
+ env->v7m.fpccr[M_REG_S] &=
+ ~(R_V7M_FPCCR_LSPEN_MASK | R_V7M_FPCCR_S_MASK);
+ env->v7m.control[M_REG_S] |= R_V7M_CONTROL_FPCA_MASK;
+#endif
}
+#ifndef CONFIG_USER_ONLY
/* AArch32 has a hard highvec setting of 0xFFFF0000. If we are currently
* executing as AArch32 then check if highvecs are enabled and
* adjust the PC accordingly.
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] target/arm: Always clear exclusive monitor on reset
2021-09-14 12:07 [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode Peter Maydell
2021-09-14 12:07 ` [PATCH 1/3] target/arm: Don't skip M-profile reset entirely in user mode Peter Maydell
@ 2021-09-14 12:07 ` Peter Maydell
2021-09-14 12:07 ` [PATCH 3/3] target/arm: Consolidate ifdef blocks in reset Peter Maydell
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2021-09-14 12:07 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Christophe Lyon
There's no particular reason why the exclusive monitor should
be only cleared on reset in system emulation mode. It doesn't
hurt if it isn't cleared in user mode, but we might as well
reduce the amount of code we have that's inside an ifdef.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/cpu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 98eed6c6d31..7b1665a1d0c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -381,15 +381,15 @@ static void arm_cpu_reset(DeviceState *dev)
env->regs[15] = 0xFFFF0000;
}
+ env->vfp.xregs[ARM_VFP_FPEXC] = 0;
+#endif
+
/* M profile requires that reset clears the exclusive monitor;
* A profile does not, but clearing it makes more sense than having it
* set with an exclusive access on address zero.
*/
arm_clear_exclusive(env);
- env->vfp.xregs[ARM_VFP_FPEXC] = 0;
-#endif
-
if (arm_feature(env, ARM_FEATURE_PMSA)) {
if (cpu->pmsav7_dregion > 0) {
if (arm_feature(env, ARM_FEATURE_V8)) {
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] target/arm: Consolidate ifdef blocks in reset
2021-09-14 12:07 [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode Peter Maydell
2021-09-14 12:07 ` [PATCH 1/3] target/arm: Don't skip M-profile reset entirely in user mode Peter Maydell
2021-09-14 12:07 ` [PATCH 2/3] target/arm: Always clear exclusive monitor on reset Peter Maydell
@ 2021-09-14 12:07 ` Peter Maydell
2021-09-14 13:10 ` [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode Richard Henderson
2021-09-15 9:57 ` Christophe Lyon
4 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2021-09-14 12:07 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Christophe Lyon
Move an ifndef CONFIG_USER_ONLY code block up in arm_cpu_reset() so
it can be merged with another earlier one.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/cpu.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7b1665a1d0c..4df640b9974 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -265,6 +265,16 @@ static void arm_cpu_reset(DeviceState *dev)
env->uncached_cpsr = ARM_CPU_MODE_SVC;
}
env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
+
+ /* AArch32 has a hard highvec setting of 0xFFFF0000. If we are currently
+ * executing as AArch32 then check if highvecs are enabled and
+ * adjust the PC accordingly.
+ */
+ if (A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_V) {
+ env->regs[15] = 0xFFFF0000;
+ }
+
+ env->vfp.xregs[ARM_VFP_FPEXC] = 0;
#endif
if (arm_feature(env, ARM_FEATURE_M)) {
@@ -372,18 +382,6 @@ static void arm_cpu_reset(DeviceState *dev)
#endif
}
-#ifndef CONFIG_USER_ONLY
- /* AArch32 has a hard highvec setting of 0xFFFF0000. If we are currently
- * executing as AArch32 then check if highvecs are enabled and
- * adjust the PC accordingly.
- */
- if (A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_V) {
- env->regs[15] = 0xFFFF0000;
- }
-
- env->vfp.xregs[ARM_VFP_FPEXC] = 0;
-#endif
-
/* M profile requires that reset clears the exclusive monitor;
* A profile does not, but clearing it makes more sense than having it
* set with an exclusive access on address zero.
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode
2021-09-14 12:07 [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode Peter Maydell
` (2 preceding siblings ...)
2021-09-14 12:07 ` [PATCH 3/3] target/arm: Consolidate ifdef blocks in reset Peter Maydell
@ 2021-09-14 13:10 ` Richard Henderson
2021-09-15 9:57 ` Christophe Lyon
4 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2021-09-14 13:10 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Christophe Lyon
On 9/14/21 5:07 AM, Peter Maydell wrote:
> Peter Maydell (3):
> target/arm: Don't skip M-profile reset entirely in user mode
> target/arm: Always clear exclusive monitor on reset
> target/arm: Consolidate ifdef blocks in reset
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode
2021-09-14 12:07 [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode Peter Maydell
` (3 preceding siblings ...)
2021-09-14 13:10 ` [PATCH 0/3] target/arm: Set FPSCR.LTPSIZE for user-mode Richard Henderson
@ 2021-09-15 9:57 ` Christophe Lyon
4 siblings, 0 replies; 6+ messages in thread
From: Christophe Lyon @ 2021-09-15 9:57 UTC (permalink / raw)
To: Peter Maydell; +Cc: Christophe Lyon, qemu-arm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]
Hi Peter,
Thanks for the prompt patches!
On Tue, Sep 14, 2021 at 2:08 PM Peter Maydell <peter.maydell@linaro.org>
wrote:
> This patchset fixes https://gitlab.com/qemu-project/qemu/-/issues/613
> which is a bug where we weren't setting FPSCR.LTPSIZE correctly
> out of reset for the user-mode emulator. The effect is that
> when using an M-profile CPU with the low-overhead-branch or MVE
> extensions (ie the Cortex-M55) with the linux-user QEMU the 'LE'
> instruction would take a UserFault and MVE instructions would
> be incorrectly predicated.
>
> This is the result of some over-exuberant ifdeffery in the
> arm_cpu_reset() function. Patch 1 fixes that so that most of
> the M-profile-specific reset handling is not ifdeffed, and
> when we're in user mode we specifically set the FPU state up
> cleanly. Patches 2 and 3 are just follow-on tidyup.
>
> Christophe, if you are in a position to test this series with:
> M55 (has Security, MVE and LOB)
> M33 (has Security extension but not MVE/LOB)
> M7 or M4 (no Security, but does have FPU)
> that would be ideal. I don't really have much in the way of
> test cases for usermode to hand, so it's possible that I
> forgot something in the init of the FPU state that might break
> one of those combinations. (It does fix the test case attached
> to the bug report.)
>
I ran the GCC testsuite with these configurations, and found no regression
compared to qemu-6.0, thanks.
In the M55 case, this also enabled many more tests, which is great!
Christophe
> thanks
> -- PMM
>
> Peter Maydell (3):
> target/arm: Don't skip M-profile reset entirely in user mode
> target/arm: Always clear exclusive monitor on reset
> target/arm: Consolidate ifdef blocks in reset
>
> target/arm/cpu.c | 39 ++++++++++++++++++++++++++++-----------
> 1 file changed, 28 insertions(+), 11 deletions(-)
>
> --
> 2.20.1
>
>
>
[-- Attachment #2: Type: text/html, Size: 2660 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread