* [PATCH] target/arm: ensure we use current exception state after SCR update
@ 2019-12-09 14:37 Alex Bennée
2019-12-09 14:44 ` Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Alex Bennée @ 2019-12-09 14:37 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Richard Henderson, qemu-arm, Alex Bennée,
Philippe Mathieu-Daudé
A write to the SCR can change the effective EL by droppping the system
from secure to non-secure mode. However if we use a cached current_el
from before the change we'll rebuild the flags incorrectly. To fix
this we overload the ARM_CP_CURRENTEL flag for the register and ensure
the new EL is used when recomputing the flags.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
---
target/arm/cpu.h | 1 +
target/arm/helper.h | 1 +
target/arm/helper.c | 14 +++++++++++++-
target/arm/translate.c | 6 +++++-
4 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index bd4d5b4445b..d2ef4644d8f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2262,6 +2262,7 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
#define ARM_CP_NOP (ARM_CP_SPECIAL | 0x0100)
#define ARM_CP_WFI (ARM_CP_SPECIAL | 0x0200)
#define ARM_CP_NZCV (ARM_CP_SPECIAL | 0x0300)
+/* Re-read the current EL, don't use cached values */
#define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | 0x0400)
#define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | 0x0500)
#define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
diff --git a/target/arm/helper.h b/target/arm/helper.h
index 3d4ec267a2c..e345bdb726a 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -91,6 +91,7 @@ DEF_HELPER_2(get_user_reg, i32, env, i32)
DEF_HELPER_3(set_user_reg, void, env, i32, i32)
DEF_HELPER_FLAGS_2(rebuild_hflags_m32, TCG_CALL_NO_RWG, void, env, int)
+DEF_HELPER_FLAGS_1(rebuild_hflags_a32_newel, TCG_CALL_NO_RWG, void, env)
DEF_HELPER_FLAGS_2(rebuild_hflags_a32, TCG_CALL_NO_RWG, void, env, int)
DEF_HELPER_FLAGS_2(rebuild_hflags_a64, TCG_CALL_NO_RWG, void, env, int)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 489c31504a6..db2e33224d6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5179,7 +5179,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
.opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
.access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
.resetvalue = 0, .writefn = scr_write },
- { .name = "SCR", .type = ARM_CP_ALIAS,
+ { .name = "SCR", .type = ARM_CP_ALIAS | ARM_CP_CURRENTEL,
.cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
.access = PL1_RW, .accessfn = access_trap_aa32s_el1,
.fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
@@ -11437,6 +11437,18 @@ void HELPER(rebuild_hflags_m32)(CPUARMState *env, int el)
env->hflags = rebuild_hflags_m32(env, fp_el, mmu_idx);
}
+/*
+ * If we have triggered a EL state change we can't rely on the
+ * translator having passed it too us, we need to recompute.
+ */
+void HELPER(rebuild_hflags_a32_newel)(CPUARMState *env)
+{
+ int el = arm_current_el(env);
+ int fp_el = fp_exception_el(env, el);
+ ARMMMUIdx mmu_idx = arm_mmu_idx_el(env, el);
+ env->hflags = rebuild_hflags_a32(env, fp_el, mmu_idx);
+}
+
void HELPER(rebuild_hflags_a32)(CPUARMState *env, int el)
{
int fp_el = fp_exception_el(env, el);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 4d5d4bd8886..59213310065 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7083,7 +7083,11 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
if (arm_dc_feature(s, ARM_FEATURE_M)) {
gen_helper_rebuild_hflags_m32(cpu_env, tcg_el);
} else {
- gen_helper_rebuild_hflags_a32(cpu_env, tcg_el);
+ if (ri->type & ARM_CP_CURRENTEL) {
+ gen_helper_rebuild_hflags_a32_newel(cpu_env);
+ } else {
+ gen_helper_rebuild_hflags_a32(cpu_env, tcg_el);
+ }
}
tcg_temp_free_i32(tcg_el);
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] target/arm: ensure we use current exception state after SCR update
2019-12-09 14:37 [PATCH] target/arm: ensure we use current exception state after SCR update Alex Bennée
@ 2019-12-09 14:44 ` Philippe Mathieu-Daudé
2019-12-09 19:28 ` no-reply
2019-12-12 0:31 ` Richard Henderson
2 siblings, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-09 14:44 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: Peter Maydell, qemu-arm, Richard Henderson
On 12/9/19 3:37 PM, Alex Bennée wrote:
> A write to the SCR can change the effective EL by droppping the system
> from secure to non-secure mode. However if we use a cached current_el
> from before the change we'll rebuild the flags incorrectly. To fix
> this we overload the ARM_CP_CURRENTEL flag for the register and ensure
> the new EL is used when recomputing the flags.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks! This patch indeed fixes my problem :)
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> target/arm/cpu.h | 1 +
> target/arm/helper.h | 1 +
> target/arm/helper.c | 14 +++++++++++++-
> target/arm/translate.c | 6 +++++-
> 4 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index bd4d5b4445b..d2ef4644d8f 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2262,6 +2262,7 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
> #define ARM_CP_NOP (ARM_CP_SPECIAL | 0x0100)
> #define ARM_CP_WFI (ARM_CP_SPECIAL | 0x0200)
> #define ARM_CP_NZCV (ARM_CP_SPECIAL | 0x0300)
> +/* Re-read the current EL, don't use cached values */
> #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | 0x0400)
> #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | 0x0500)
> #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
> diff --git a/target/arm/helper.h b/target/arm/helper.h
> index 3d4ec267a2c..e345bdb726a 100644
> --- a/target/arm/helper.h
> +++ b/target/arm/helper.h
> @@ -91,6 +91,7 @@ DEF_HELPER_2(get_user_reg, i32, env, i32)
> DEF_HELPER_3(set_user_reg, void, env, i32, i32)
>
> DEF_HELPER_FLAGS_2(rebuild_hflags_m32, TCG_CALL_NO_RWG, void, env, int)
> +DEF_HELPER_FLAGS_1(rebuild_hflags_a32_newel, TCG_CALL_NO_RWG, void, env)
> DEF_HELPER_FLAGS_2(rebuild_hflags_a32, TCG_CALL_NO_RWG, void, env, int)
> DEF_HELPER_FLAGS_2(rebuild_hflags_a64, TCG_CALL_NO_RWG, void, env, int)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 489c31504a6..db2e33224d6 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5179,7 +5179,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
> .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
> .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
> .resetvalue = 0, .writefn = scr_write },
> - { .name = "SCR", .type = ARM_CP_ALIAS,
> + { .name = "SCR", .type = ARM_CP_ALIAS | ARM_CP_CURRENTEL,
> .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
> .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
> @@ -11437,6 +11437,18 @@ void HELPER(rebuild_hflags_m32)(CPUARMState *env, int el)
> env->hflags = rebuild_hflags_m32(env, fp_el, mmu_idx);
> }
>
> +/*
> + * If we have triggered a EL state change we can't rely on the
> + * translator having passed it too us, we need to recompute.
> + */
> +void HELPER(rebuild_hflags_a32_newel)(CPUARMState *env)
> +{
> + int el = arm_current_el(env);
> + int fp_el = fp_exception_el(env, el);
> + ARMMMUIdx mmu_idx = arm_mmu_idx_el(env, el);
> + env->hflags = rebuild_hflags_a32(env, fp_el, mmu_idx);
> +}
> +
> void HELPER(rebuild_hflags_a32)(CPUARMState *env, int el)
> {
> int fp_el = fp_exception_el(env, el);
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 4d5d4bd8886..59213310065 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -7083,7 +7083,11 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
> if (arm_dc_feature(s, ARM_FEATURE_M)) {
> gen_helper_rebuild_hflags_m32(cpu_env, tcg_el);
> } else {
> - gen_helper_rebuild_hflags_a32(cpu_env, tcg_el);
> + if (ri->type & ARM_CP_CURRENTEL) {
> + gen_helper_rebuild_hflags_a32_newel(cpu_env);
> + } else {
> + gen_helper_rebuild_hflags_a32(cpu_env, tcg_el);
> + }
> }
> tcg_temp_free_i32(tcg_el);
> /*
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] target/arm: ensure we use current exception state after SCR update
2019-12-09 14:37 [PATCH] target/arm: ensure we use current exception state after SCR update Alex Bennée
2019-12-09 14:44 ` Philippe Mathieu-Daudé
@ 2019-12-09 19:28 ` no-reply
2019-12-12 0:31 ` Richard Henderson
2 siblings, 0 replies; 4+ messages in thread
From: no-reply @ 2019-12-09 19:28 UTC (permalink / raw)
To: alex.bennee
Cc: peter.maydell, philmd, richard.henderson, qemu-devel, qemu-arm,
alex.bennee
Patchew URL: https://patchew.org/QEMU/20191209143723.6368-1-alex.bennee@linaro.org/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-h1p3v7cj/src/docker-src.2019-12-09-14.28.28.18212] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-h1p3v7cj/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 0m3.985s
user 0m2.582s
The full log is available at
http://patchew.org/logs/20191209143723.6368-1-alex.bennee@linaro.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target/arm: ensure we use current exception state after SCR update
2019-12-09 14:37 [PATCH] target/arm: ensure we use current exception state after SCR update Alex Bennée
2019-12-09 14:44 ` Philippe Mathieu-Daudé
2019-12-09 19:28 ` no-reply
@ 2019-12-12 0:31 ` Richard Henderson
2 siblings, 0 replies; 4+ messages in thread
From: Richard Henderson @ 2019-12-12 0:31 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: Peter Maydell, qemu-arm, Philippe Mathieu-Daudé
On 12/9/19 6:37 AM, Alex Bennée wrote:
> +/* Re-read the current EL, don't use cached values */
> #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | 0x0400)
> #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | 0x0500)
> #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
...
> @@ -5179,7 +5179,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
> .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
> .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
> .resetvalue = 0, .writefn = scr_write },
> - { .name = "SCR", .type = ARM_CP_ALIAS,
> + { .name = "SCR", .type = ARM_CP_ALIAS | ARM_CP_CURRENTEL,
I don't think you should reuse this value. It is not a simple bit.
While ARM_CP_* all appear to implement a 16-bit quantity, the type field is
already an int, and so can easily hold more. I think you should use a new bit
for this.
r~
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-12 0:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-09 14:37 [PATCH] target/arm: ensure we use current exception state after SCR update Alex Bennée
2019-12-09 14:44 ` Philippe Mathieu-Daudé
2019-12-09 19:28 ` no-reply
2019-12-12 0:31 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).