Hi Peter, Here is version 2 of the ARM semi-hosting cleanup patches. The re-base had failed due to a change in the gen_exception_internal_insn API which now takes the PC instead of offset from pc_next. There is also the a minor indentation fix. Alex Bennée (4): target/arm: handle M-profile semihosting at translate time target/arm: handle A-profile T32 semihosting at translate time target/arm: handle A-profile A32 semihosting at translate time target/arm: remove run time semihosting checks Emilio G. Cota (1): atomic_template: fix indentation in GEN_ATOMIC_HELPER accel/tcg/atomic_template.h | 2 +- target/arm/helper.c | 96 +++++++++---------------------------- target/arm/m_helper.c | 18 +++---- target/arm/translate.c | 64 +++++++++++++++++++++---- 4 files changed, 85 insertions(+), 95 deletions(-) -- 2.20.1
We do this for other semihosting calls so we might as well do it for M-profile as well. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- v2 - update for change to gen_exception_internal_insn API --- target/arm/m_helper.c | 18 ++++++------------ target/arm/translate.c | 20 +++++++++++++++++++- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c index 884d35d2b02..27cd2f3f964 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -2114,19 +2114,13 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs) break; } break; + case EXCP_SEMIHOST: + qemu_log_mask(CPU_LOG_INT, + "...handling as semihosting call 0x%x\n", + env->regs[0]); + env->regs[0] = do_arm_semihosting(env); + return; case EXCP_BKPT: - if (semihosting_enabled()) { - int nr; - nr = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env)) & 0xff; - if (nr == 0xab) { - env->regs[15] += 2; - qemu_log_mask(CPU_LOG_INT, - "...handling as semihosting call 0x%x\n", - env->regs[0]); - env->regs[0] = do_arm_semihosting(env); - return; - } - } armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_DEBUG, false); break; case EXCP_IRQ: diff --git a/target/arm/translate.c b/target/arm/translate.c index 615859e23c5..816d46b2205 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -10931,6 +10931,24 @@ illegal_op: unallocated_encoding(s); } +/* + * Thumb BKPT. On M-profile CPUs this may be a semihosting call which + * we can process much the same way as gen_hlt() above. + */ +static inline void gen_thumb_bkpt(DisasContext *s, int imm8) +{ + if (arm_dc_feature(s, ARM_FEATURE_M) && + semihosting_enabled() && +#ifndef CONFIG_USER_ONLY + s->current_el != 0 && +#endif + (imm8 == 0xab)) { + gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST); + return; + } + gen_exception_bkpt_insn(s, syn_aa32_bkpt(imm8, true)); +} + static void disas_thumb_insn(DisasContext *s, uint32_t insn) { uint32_t val, op, rm, rn, rd, shift, cond; @@ -11553,7 +11571,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn) { int imm8 = extract32(insn, 0, 8); ARCH(5); - gen_exception_bkpt_insn(s, syn_aa32_bkpt(imm8, true)); + gen_thumb_bkpt(s, imm8); break; } -- 2.20.1
As for the other semihosting calls we can resolve this at translate time. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- v2 - update for change to gen_exception_internal_insn API --- target/arm/translate.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/target/arm/translate.c b/target/arm/translate.c index 816d46b2205..673994ed1a1 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -10949,6 +10949,24 @@ static inline void gen_thumb_bkpt(DisasContext *s, int imm8) gen_exception_bkpt_insn(s, syn_aa32_bkpt(imm8, true)); } +/* + * Thumb SWI. On A-profile CPUs this may be a semihosting call. + */ +static inline void gen_thumb_swi(DisasContext *s, int imm8) +{ + if (semihosting_enabled() && +#ifndef CONFIG_USER_ONLY + s->current_el != 0 && +#endif + (imm8 == 0xab)) { + gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST); + return; + } + gen_set_pc_im(s, s->base.pc_next); + s->svc_imm = imm8; + s->base.is_jmp = DISAS_SWI; +} + static void disas_thumb_insn(DisasContext *s, uint32_t insn) { uint32_t val, op, rm, rn, rd, shift, cond; @@ -11700,10 +11718,8 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn) goto undef; if (cond == 0xf) { - /* swi */ - gen_set_pc_im(s, s->base.pc_next); - s->svc_imm = extract32(insn, 0, 8); - s->base.is_jmp = DISAS_SWI; + /* swi/svc */ + gen_thumb_swi(s, extract32(insn, 0, 8)); break; } /* generate a conditional jump to next instruction */ -- 2.20.1
As for the other semihosting calls we can resolve this at translate time. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- v2 - update for change to gen_exception_internal_insn API --- target/arm/translate.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/target/arm/translate.c b/target/arm/translate.c index 673994ed1a1..03396a1dde3 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -7683,6 +7683,22 @@ static void arm_skip_unless(DisasContext *s, uint32_t cond) arm_gen_test_cc(cond ^ 1, s->condlabel); } +static inline void gen_arm_swi(DisasContext *s, int imm24) +{ + if (semihosting_enabled() && +#ifndef CONFIG_USER_ONLY + s->current_el != 0 && +#endif + (imm24 == 0x123456)) { + gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST); + return; + } + + gen_set_pc_im(s, s->base.pc_next); + s->svc_imm = imm24; + s->base.is_jmp = DISAS_SWI; +} + static void disas_arm_insn(DisasContext *s, unsigned int insn) { unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh; @@ -9230,9 +9246,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn) break; case 0xf: /* swi */ - gen_set_pc_im(s, s->base.pc_next); - s->svc_imm = extract32(insn, 0, 24); - s->base.is_jmp = DISAS_SWI; + gen_arm_swi(s, extract32(insn, 0, 24)); break; default: illegal_op: -- 2.20.1
Now we do all our checking and use a common EXCP_SEMIHOST for semihosting operations we can make helper code a lot simpler. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- v2 - fix re-base conflicts - hoist EXCP_SEMIHOST check - comment cleanups v5 - move CONFIG_TCG ifdefs --- target/arm/helper.c | 96 +++++++++++---------------------------------- 1 file changed, 22 insertions(+), 74 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 507026c9154..a87ae6d46a1 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -8339,88 +8339,32 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) new_el, env->pc, pstate_read(env)); } -static inline bool check_for_semihosting(CPUState *cs) -{ +/* + * Do semihosting call and set the appropriate return value. All the + * permission and validity checks have been done at translate time. + * + * We only see semihosting exceptions in TCG only as they are not + * trapped to the hypervisor in KVM. + */ #ifdef CONFIG_TCG - /* Check whether this exception is a semihosting call; if so - * then handle it and return true; otherwise return false. - */ +static void handle_semihosting(CPUState *cs) +{ ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; if (is_a64(env)) { - if (cs->exception_index == EXCP_SEMIHOST) { - /* This is always the 64-bit semihosting exception. - * The "is this usermode" and "is semihosting enabled" - * checks have been done at translate time. - */ - qemu_log_mask(CPU_LOG_INT, - "...handling as semihosting call 0x%" PRIx64 "\n", - env->xregs[0]); - env->xregs[0] = do_arm_semihosting(env); - return true; - } - return false; + qemu_log_mask(CPU_LOG_INT, + "...handling as semihosting call 0x%" PRIx64 "\n", + env->xregs[0]); + env->xregs[0] = do_arm_semihosting(env); } else { - uint32_t imm; - - /* Only intercept calls from privileged modes, to provide some - * semblance of security. - */ - if (cs->exception_index != EXCP_SEMIHOST && - (!semihosting_enabled() || - ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR))) { - return false; - } - - switch (cs->exception_index) { - case EXCP_SEMIHOST: - /* This is always a semihosting call; the "is this usermode" - * and "is semihosting enabled" checks have been done at - * translate time. - */ - break; - case EXCP_SWI: - /* Check for semihosting interrupt. */ - if (env->thumb) { - imm = arm_lduw_code(env, env->regs[15] - 2, arm_sctlr_b(env)) - & 0xff; - if (imm == 0xab) { - break; - } - } else { - imm = arm_ldl_code(env, env->regs[15] - 4, arm_sctlr_b(env)) - & 0xffffff; - if (imm == 0x123456) { - break; - } - } - return false; - case EXCP_BKPT: - /* See if this is a semihosting syscall. */ - if (env->thumb) { - imm = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env)) - & 0xff; - if (imm == 0xab) { - env->regs[15] += 2; - break; - } - } - return false; - default: - return false; - } - qemu_log_mask(CPU_LOG_INT, "...handling as semihosting call 0x%x\n", env->regs[0]); env->regs[0] = do_arm_semihosting(env); - return true; } -#else - return false; -#endif } +#endif /* Handle a CPU exception for A and R profile CPUs. * Do any appropriate logging, handle PSCI calls, and then hand off @@ -8451,13 +8395,17 @@ void arm_cpu_do_interrupt(CPUState *cs) return; } - /* Semihosting semantics depend on the register width of the - * code that caused the exception, not the target exception level, - * so must be handled here. + /* + * Semihosting semantics depend on the register width of the code + * that caused the exception, not the target exception level, so + * must be handled here. */ - if (check_for_semihosting(cs)) { +#ifdef CONFIG_TCG + if (cs->exception_index == EXCP_SEMIHOST) { + handle_semihosting(cs); return; } +#endif /* Hooks may change global state so BQL should be held, also the * BQL needs to be held for any modification of -- 2.20.1
From: "Emilio G. Cota" <cota@braap.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Signed-off-by: Emilio G. Cota <cota@braap.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- accel/tcg/atomic_template.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h index df9c8388178..287433d809b 100644 --- a/accel/tcg/atomic_template.h +++ b/accel/tcg/atomic_template.h @@ -149,7 +149,7 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr, #define GEN_ATOMIC_HELPER(X) \ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \ - ABI_TYPE val EXTRA_ARGS) \ + ABI_TYPE val EXTRA_ARGS) \ { \ ATOMIC_MMU_DECLS; \ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \ -- 2.20.1
On Wed, 4 Sep 2019 at 12:22, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Hi Peter,
>
> Here is version 2 of the ARM semi-hosting cleanup patches. The re-base
> had failed due to a change in the gen_exception_internal_insn API
> which now takes the PC instead of offset from pc_next. There is also
> the a minor indentation fix.
Hi -- I'm afraid you'll need to rebase this again, as Richard's
decodetree refactoring has now landed.
thanks
-- PMM