diff mbox series

target/arm: ensure we use current exception state after SCR update

Message ID 20191209143723.6368-1-alex.bennee@linaro.org
State New
Headers show
Series target/arm: ensure we use current exception state after SCR update | expand

Commit Message

Alex Bennée Dec. 9, 2019, 2:37 p.m. UTC 
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(-)

-- 
2.20.1

Comments

Philippe Mathieu-Daudé Dec. 9, 2019, 2:44 p.m. UTC | #1 
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);

>               /*

>
no-reply@patchew.org Dec. 9, 2019, 7:28 p.m. UTC | #2 
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
Richard Henderson Dec. 12, 2019, 12:31 a.m. UTC | #3 
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~
diff mbox series

Patch

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);
             /*