Re: [PULL 18/23] accel/tcg: re-factor non-RAM execution code

["Alex Bennée" <alex.bennee@linaro.org>]

--8<---------------cut here---------------start------------->8---

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 15 Apr 2021 at 14:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Thu, 18 Feb 2021 at 09:47, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >
>> > There is no real need to use CF_NOCACHE here. As long as the TB isn't
>> > linked to other TBs or included in the QHT or jump cache then it will
>> > only get executed once.
>> >
>> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Message-Id: <20210213130325.14781-19-alex.bennee@linaro.org>
>>
>> Hi; I've just noticed that this commit seems to break the case of:
>>  * execution of code not from a RAM block
>>  * when icount is enabled
>>  * and an instruction is an IO insn that triggers io-recompile
>>
>> because:
>>
>> > @@ -2097,6 +2086,17 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>> >          tb_reset_jump(tb, 1);
>> >      }
>> >
>> > +    /*
>> > +     * If the TB is not associated with a physical RAM page then
>> > +     * it must be a temporary one-insn TB, and we have nothing to do
>> > +     * except fill in the page_addr[] fields. Return early before
>> > +     * attempting to link to other TBs or add to the lookup table.
>> > +     */
>> > +    if (phys_pc == -1) {
>> > +        tb->page_addr[0] = tb->page_addr[1] = -1;
>> > +        return tb;
>> > +    }
>>
>> we used to fall through here, which meant we called
>> tcg_tb_insert(tb). No we no longer do. That's bad, because
>> cpu_io_recompile() does:
>>
>>     tb = tcg_tb_lookup(retaddr);
>>     if (!tb) {
>>         cpu_abort(cpu, "cpu_io_recompile: could not find TB for pc=%p",
>>                   (void *)retaddr);
>>     }
>>
>> and since it can no longer find the TB, QEMU aborts.
>
> Adding the tcg_tb_insert() call to the early exit path:
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index ba6ab09790e..6014285e4dc 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2081,6 +2081,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>       */
>      if (phys_pc == -1) {
>          tb->page_addr[0] = tb->page_addr[1] = -1;
> +        tcg_tb_insert(tb);
>          return tb;
>      }
>
> seems to fix my test case, but I don't know enough about the new
> design here to know if that has undesirable side effects.

No we don't want to do that as the comment says above. However as it's a
single instruction block it can do IO so could you try this instead
please:

--8<---------------cut here---------------start------------->8---
accel/tcg: avoid re-translating one-shot instructions

By definition a single instruction is capable of being an IO
instruction. This avoids a problem of triggering a cpu_io_recompile on
a non-cached translation which would only do exactly this anyway.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

1 file changed, 1 insertion(+), 1 deletion(-)
accel/tcg/translate-all.c | 2 +-

modified   accel/tcg/translate-all.c
@@ -1863,7 +1863,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
     if (phys_pc == -1) {
         /* Generate a one-shot TB with 1 insn in it */
-        cflags = (cflags & ~CF_COUNT_MASK) | 1;
+        cflags = (cflags & ~CF_COUNT_MASK) | CF_LAST_IO | 1;
     }
 
     max_insns = cflags & CF_COUNT_MASK;
--8<---------------cut here---------------end--------------->8---


-- 
Alex Bennée