Re: [PATCH] target/arm: ensure we use current exception state after SCR update
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~
Re: [PATCH] target/arm: ensure we use current exception state after SCR update
Patchew URL: https://patchew.org/QEMU/20191209143723.6368-1-alex.ben...@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 real0m3.985s user0m2.582s The full log is available at http://patchew.org/logs/20191209143723.6368-1-alex.ben...@linaro.org/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: ensure we use current exception state after SCR update
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 Cc: Richard Henderson Cc: Philippe Mathieu-Daudé Thanks! This patch indeed fixes my problem :) Tested-by: Philippe Mathieu-Daudé --- 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); /*
[PATCH] target/arm: ensure we use current exception state after SCR update
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 Cc: Richard Henderson Cc: Philippe Mathieu-Daudé --- 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