From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58307) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvrFf-0007v2-Og for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:33:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvrFc-0005KD-Uh for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:33:11 -0500 Received: from mail-lb0-x22f.google.com ([2a00:1450:4010:c04::22f]:34587) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvrFc-0005K9-Jp for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:33:08 -0500 Received: by lbbwb3 with SMTP id wb3so105272968lbb.1 for ; Mon, 09 Nov 2015 10:33:07 -0800 (PST) References: <1446488173-2621-1-git-send-email-serge.fdrv@gmail.com> From: Sergey Fedorov Message-ID: <5640E6E1.6050605@gmail.com> Date: Mon, 9 Nov 2015 21:33:05 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On 06.11.2015 15:46, Peter Maydell wrote: > On 2 November 2015 at 18:16, Sergey Fedorov 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 >> --- >> 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.) Thanks for the catch, I'll fix this. > 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.) This one, too. > The third one is in the breakpoint handling, which I guess > is the case we're aiming to fix? Yes, that is what I'm going to fix. I'll mention this in a commit message. > 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) This issue is a subtle one. Looks like I'm going to have to spend some time getting into this. Actually, I just don't understand this code for now :) Would it be OK, if I just do what you suggest and send v2 as soon as possible and get into this later on? > 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. I'm going to prepare a separate patch to fix this. Thanks, Sergey > >> /* indicate that the hash table must be used to find the next TB */ >> tcg_gen_exit_tb(0); >> break; >> -- >> 1.9.1