[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArc
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code |
Date: |
Fri, 6 Nov 2015 12:46:29 +0000 |
On 2 November 2015 at 18:16, Sergey Fedorov <address@hidden> wrote:
> AArch32 translation code does not distinguish between DISAS_UPDATE and
> DISAS_JUMP. Thus, we cannot use any of them without first updating PC in
> CPU state. Furthermore, it is too complicated to update PC in CPU state
> before PC gets updated in disas context. So it is hardly possible to
> correctly end TB early if is is not likely to be executed before calling
> disas_*_insn(), e.g. just after calling breakpoint check helper.
>
> Modify DISAS_UPDATE and DISAS_JUMP usage in AArch32 translation and
> apply to them the same semantic as AArch64 translation does:
> - DISAS_UPDATE: update PC in CPU state when finishing translation
> - DISAS_JUMP: preserve current PC value in CPU state when finishing
> translation
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> ---
> target-arm/translate.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 9f1d740..ec740fd 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -870,7 +870,7 @@ static inline void gen_bx_im(DisasContext *s, uint32_t
> addr)
> {
> TCGv_i32 tmp;
>
> - s->is_jmp = DISAS_UPDATE;
> + s->is_jmp = DISAS_JUMP;
> if (s->thumb != (addr & 1)) {
> tmp = tcg_temp_new_i32();
> tcg_gen_movi_i32(tmp, addr & 1);
> @@ -883,7 +883,7 @@ static inline void gen_bx_im(DisasContext *s, uint32_t
> addr)
> /* Set PC and Thumb state from var. var is marked as dead. */
> static inline void gen_bx(DisasContext *s, TCGv_i32 var)
> {
> - s->is_jmp = DISAS_UPDATE;
> + s->is_jmp = DISAS_JUMP;
> tcg_gen_andi_i32(cpu_R[15], var, ~1);
> tcg_gen_andi_i32(var, var, 1);
> store_cpu_field(var, thumb);
> @@ -1062,7 +1062,7 @@ static void gen_exception_insn(DisasContext *s, int
> offset, int excp,
> static inline void gen_lookup_tb(DisasContext *s)
> {
> tcg_gen_movi_i32(cpu_R[15], s->pc & ~1);
> - s->is_jmp = DISAS_UPDATE;
> + s->is_jmp = DISAS_JUMP;
> }
>
> static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
> @@ -4096,7 +4096,7 @@ static void gen_exception_return(DisasContext *s,
> TCGv_i32 pc)
> tmp = load_cpu_field(spsr);
> gen_set_cpsr(tmp, CPSR_ERET_MASK);
> tcg_temp_free_i32(tmp);
> - s->is_jmp = DISAS_UPDATE;
> + s->is_jmp = DISAS_JUMP;
> }
>
> /* Generate a v6 exception return. Marks both values as dead. */
> @@ -4105,7 +4105,7 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc,
> TCGv_i32 cpsr)
> gen_set_cpsr(cpsr, CPSR_ERET_MASK);
> tcg_temp_free_i32(cpsr);
> store_reg(s, 15, pc);
> - s->is_jmp = DISAS_UPDATE;
> + s->is_jmp = DISAS_JUMP;
> }
>
> static void gen_nop_hint(DisasContext *s, int val)
> @@ -9035,7 +9035,7 @@ static void disas_arm_insn(DisasContext *s, unsigned
> int insn)
> tmp = load_cpu_field(spsr);
> gen_set_cpsr(tmp, CPSR_ERET_MASK);
> tcg_temp_free_i32(tmp);
> - s->is_jmp = DISAS_UPDATE;
> + s->is_jmp = DISAS_JUMP;
> }
> }
> break;
> @@ -11488,8 +11488,10 @@ void gen_intermediate_code(CPUARMState *env,
> TranslationBlock *tb)
> gen_goto_tb(dc, 1, dc->pc);
> break;
> default:
> - case DISAS_JUMP:
> case DISAS_UPDATE:
> + gen_set_pc_im(dc, dc->pc);
> + /* fall through */
> + case DISAS_JUMP:
This is also changing the behaviour for the "default" label, which
I think is unintentional. In particular DISAS_EXC doesn't appear
in this switch and so we're relying on the default being "do nothing".
(I think it's just an oversight that DISAS_EXC isn't an explicit
case in this switch, as it is for target-a64.c.)
This patch leaves three instances of DISAS_UPDATE. For two
of those [userspace kernel page and the M profile exception-return
magic addresses], we've just done a gen_exception_internal(), so
updating the PC here is pointless. Those should be DISAS_EXC
instead I think. (The effect of your patch on these is just
generating unreachable code, so no user visible problems.)
The third one is in the breakpoint handling, which I guess
is the case we're aiming to fix?
I think if we want to use DISAS_UPDATE then we also need
to handle it inside the
if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
branch. Specifically, this check:
if (dc->condjmp || !dc->is_jmp)
is trying to say "if this was a conditional jump, or it
has not already set the PC", and so now needs to read
if (dc->condjmp || dc->is_jmp == DISAS_NEXT ||
dc->is_jmp == DISAS_UPDATE)
Also, I've just noticed that we don't do a gen_set_pc_im()
*before* calling gen_helper_check_breakpoints(). That means
that if the possible breakpoint is not at the start of a TB
then the helper function will not see the correct env->pc.
> /* indicate that the hash table must be used to find the next TB
> */
> tcg_gen_exit_tb(0);
> break;
> --
> 1.9.1
>
thanks
-- PMM