diff mbox series

[v3,3/3] accel/tcg: Assert that tb->size != 0 after translation

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

Commit Message

Ilya Leoshkevich April 14, 2021, 1:41 p.m. UTC
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(+)

Comments

David Hildenbrand April 14, 2021, 2:48 p.m. UTC | #1
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>
Ilya Leoshkevich April 14, 2021, 4:45 p.m. UTC | #2
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>
>
Max Filippov April 14, 2021, 6:03 p.m. UTC | #3
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?
Richard Henderson April 14, 2021, 7:43 p.m. UTC | #4
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~
Max Filippov April 15, 2021, 1:23 a.m. UTC | #5
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.
Ilya Leoshkevich April 15, 2021, 11:14 a.m. UTC | #6
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;
     }
Richard Henderson April 15, 2021, 2:33 p.m. UTC | #7
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~
Peter Maydell April 15, 2021, 3:02 p.m. UTC | #8
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
Max Filippov April 15, 2021, 8:20 p.m. UTC | #9
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 mbox series

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;