| Message ID | 20210414134112.25620-4-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: Make sure that tb->size != 0 after translation | expand |
On 14.04.21 15:41, Ilya Leoshkevich wrote: > If arch-specific code generates a translation block of size 0, > tb_gen_code() may generate a spurious exception. Add an assertion in > order to catch such situations early. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > accel/tcg/translate-all.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index ba6ab09790..93b2dae112 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -1913,6 +1913,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, > > tcg_ctx->cpu = env_cpu(env); > gen_intermediate_code(cpu, tb, max_insns); > + assert(tb->size != 0); > tcg_ctx->cpu = NULL; > max_insns = tb->icount; > > Did you double-check the xtensa issue? Reviewed-by: David Hildenbrand <david@redhat.com>
On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote: > On 14.04.21 15:41, Ilya Leoshkevich wrote: > > If arch-specific code generates a translation block of size 0, > > tb_gen_code() may generate a spurious exception. Add an assertion > > in > > order to catch such situations early. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > accel/tcg/translate-all.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > > index ba6ab09790..93b2dae112 100644 > > --- a/accel/tcg/translate-all.c > > +++ b/accel/tcg/translate-all.c > > @@ -1913,6 +1913,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, > > > > tcg_ctx->cpu = env_cpu(env); > > gen_intermediate_code(cpu, tb, max_insns); > > + assert(tb->size != 0); > > tcg_ctx->cpu = NULL; > > max_insns = tb->icount; > > > > > > Did you double-check the xtensa issue? Oh, I'm sorry, I completely forgot about that one. I just ran the test locally, and apparently it fails because of this new assert, so I'll have to write the 4th patch now. Thanks! > > Reviewed-by: David Hildenbrand <david@redhat.com> >
On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote: > > Did you double-check the xtensa issue? > > Oh, I'm sorry, I completely forgot about that one. I just ran the > test locally, and apparently it fails because of this new assert, so > I'll have to write the 4th patch now. Thanks! Just curious, what xtensa issue?
On 4/14/21 11:03 AM, Max Filippov wrote: > On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: >> On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote: >>> Did you double-check the xtensa issue? >> >> Oh, I'm sorry, I completely forgot about that one. I just ran the >> test locally, and apparently it fails because of this new assert, so >> I'll have to write the 4th patch now. Thanks! > > Just curious, what xtensa issue? Returning from xtensa_tr_translate_insn with tb->size == 0. Basically, dc->base.pc_next needs to be incremented even for illegal instructions, preferably by the number of bytes consumed while determining that the insn is illegal. r~
On Wed, Apr 14, 2021 at 12:43 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/14/21 11:03 AM, Max Filippov wrote: > > On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > >> On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote: > >>> Did you double-check the xtensa issue? > >> > >> Oh, I'm sorry, I completely forgot about that one. I just ran the > >> test locally, and apparently it fails because of this new assert, so > >> I'll have to write the 4th patch now. Thanks! > > > > Just curious, what xtensa issue? > > Returning from xtensa_tr_translate_insn with tb->size == 0. > > Basically, dc->base.pc_next needs to be incremented even for illegal > instructions, preferably by the number of bytes consumed while determining that > the insn is illegal. I see a few places where target/xtensa may do that. E.g. it does that on entry to an exception handler to allow for debugging its first instruction. No guest code is consumed to make this decision, would size 1 work in that case? I'll take a look.
On Wed, 2021-04-14 at 18:23 -0700, Max Filippov wrote: > On Wed, Apr 14, 2021 at 12:43 PM Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > On 4/14/21 11:03 AM, Max Filippov wrote: > > > On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich < > > > iii@linux.ibm.com> wrote: > > > > On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote: > > > > > Did you double-check the xtensa issue? > > > > > > > > Oh, I'm sorry, I completely forgot about that one. I just ran the > > > > test locally, and apparently it fails because of this new assert, > > > > so > > > > I'll have to write the 4th patch now. Thanks! > > > > > > Just curious, what xtensa issue? > > > > Returning from xtensa_tr_translate_insn with tb->size == 0. > > > > Basically, dc->base.pc_next needs to be incremented even for illegal > > instructions, preferably by the number of bytes consumed while > > determining that > > the insn is illegal. > > I see a few places where target/xtensa may do that. E.g. it does that > on entry > to an exception handler to allow for debugging its first instruction. > No guest code > is consumed to make this decision, would size 1 work in that case? > I'll take a look. Returning 1 was my idea as well. Here is what seems to fix the failure and what I'm currently testing locally: --- a/target/xtensa/translate.c +++ b/target/xtensa/translate.c @@ -917,6 +917,8 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc) "unknown instruction length (pc = %08x)\n", dc->pc); gen_exception_cause(dc, ILLEGAL_INSTRUCTION_CAUSE); + dc->base.pc_next = dc->pc + 1; + dc->base.is_jmp = DISAS_NORETURN; return; } @@ -1274,11 +1276,13 @@ static void xtensa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) if ((tb_cflags(dc->base.tb) & CF_USE_ICOUNT) && (dc->base.tb->flags & XTENSA_TBFLAG_YIELD)) { gen_exception(dc, EXCP_YIELD); + dc->base.pc_next = dc->pc + 1; dc->base.is_jmp = DISAS_NORETURN; return; } if (dc->base.tb->flags & XTENSA_TBFLAG_EXCEPTION) { gen_exception(dc, EXCP_DEBUG); + dc->base.pc_next = dc->pc + 1; dc->base.is_jmp = DISAS_NORETURN; return; }
On 4/14/21 6:23 PM, Max Filippov wrote: > I see a few places where target/xtensa may do that. E.g. it does that on entry > to an exception handler to allow for debugging its first instruction. > No guest code > is consumed to make this decision, would size 1 work in that case? > I'll take a look. Yes, any positive value will do. r~
On Thu, 15 Apr 2021 at 02:24, Max Filippov <jcmvbkbc@gmail.com> wrote: > I see a few places where target/xtensa may do that. E.g. it does that on entry > to an exception handler to allow for debugging its first instruction. That should now be handled by the common code, I think (see commits a7ba744f4082ab and ba3c35d9c402636) -- does xtensa still need to special case this ? thanks -- PMM
On Thu, Apr 15, 2021 at 8:03 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 15 Apr 2021 at 02:24, Max Filippov <jcmvbkbc@gmail.com> wrote: > > I see a few places where target/xtensa may do that. E.g. it does that on entry > > to an exception handler to allow for debugging its first instruction. > > That should now be handled by the common code, I think (see commits > a7ba744f4082ab and ba3c35d9c402636) -- does xtensa still need to > special case this ? Thanks for letting me know. It now stops there twice, so no, special casing is no longer needed. I'll send a patch.
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index ba6ab09790..93b2dae112 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1913,6 +1913,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, tcg_ctx->cpu = env_cpu(env); gen_intermediate_code(cpu, tb, max_insns); + assert(tb->size != 0); tcg_ctx->cpu = NULL; max_insns = tb->icount;
If arch-specific code generates a translation block of size 0, tb_gen_code() may generate a spurious exception. Add an assertion in order to catch such situations early. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- accel/tcg/translate-all.c | 1 + 1 file changed, 1 insertion(+)