[RFC PATCH] accel/tcg: avoid re-translating one-shot instructions

[RFC PATCH] accel/tcg: avoid re-translating one-shot instructions

[Alex Bennée]

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-recorded translation which then fails because it expects
tcg_tb_lookup() to succeed unconditionally. The normal use case
requires a TB to be able to resolve machine state.

The other users of tcg_tb_lookup() are able to tolerate a missing TB
if the machine state has been resolved by other means - which in the
single-shot case is always true because machine state is synced at the
start of a block.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/translate-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index ba6ab09790..b12d0898d0 100644
--- a/accel/tcg/translate-all.c
+++ b/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;
-- 
2.20.1

Re: [RFC PATCH] accel/tcg: avoid re-translating one-shot instructions

[Peter Maydell]

On Thu, 15 Apr 2021 at 17:25, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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-recorded translation which then fails because it expects
> tcg_tb_lookup() to succeed unconditionally. The normal use case
> requires a TB to be able to resolve machine state.
>
> The other users of tcg_tb_lookup() are able to tolerate a missing TB
> if the machine state has been resolved by other means - which in the
> single-shot case is always true because machine state is synced at the
> start of a block.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  accel/tcg/translate-all.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index ba6ab09790..b12d0898d0 100644
> --- a/accel/tcg/translate-all.c
> +++ b/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;
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [RFC PATCH] accel/tcg: avoid re-translating one-shot instructions

[Richard Henderson]

On 4/15/21 9:24 AM, Alex Bennée wrote:
> 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-recorded translation which then fails because it expects
> tcg_tb_lookup() to succeed unconditionally. The normal use case
> requires a TB to be able to resolve machine state.
> 
> The other users of tcg_tb_lookup() are able to tolerate a missing TB
> if the machine state has been resolved by other means - which in the
> single-shot case is always true because machine state is synced at the
> start of a block.
> 
> Reported-by: Peter Maydell<peter.maydell@linaro.org>
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   accel/tcg/translate-all.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Re: [RFC PATCH] accel/tcg: avoid re-translating one-shot instructions

[Alex Bennée]

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

> On Thu, 15 Apr 2021 at 17:25, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> 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-recorded translation which then fails because it expects
>> tcg_tb_lookup() to succeed unconditionally. The normal use case
>> requires a TB to be able to resolve machine state.
>>
>> The other users of tcg_tb_lookup() are able to tolerate a missing TB
>> if the machine state has been resolved by other means - which in the
>> single-shot case is always true because machine state is synced at the
>> start of a block.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  accel/tcg/translate-all.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> index ba6ab09790..b12d0898d0 100644
>> --- a/accel/tcg/translate-all.c
>> +++ b/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;
>> --
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Are you going to apply this directly or do you want it through a tree?

>
> thanks
> -- PMM


-- 
Alex Bennée

Re: [RFC PATCH] accel/tcg: avoid re-translating one-shot instructions

[Peter Maydell]

On Thu, 15 Apr 2021 at 19:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Thu, 15 Apr 2021 at 17:25, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> 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-recorded translation which then fails because it expects
> >> tcg_tb_lookup() to succeed unconditionally. The normal use case
> >> requires a TB to be able to resolve machine state.
> >>
> >> The other users of tcg_tb_lookup() are able to tolerate a missing TB
> >> if the machine state has been resolved by other means - which in the
> >> single-shot case is always true because machine state is synced at the
> >> start of a block.
> >>
> >> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  accel/tcg/translate-all.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> >> index ba6ab09790..b12d0898d0 100644
> >> --- a/accel/tcg/translate-all.c
> >> +++ b/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;
> >> --
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Are you going to apply this directly or do you want it through a tree?

For the moment I've just added it to the list of "not fixed in 6.0 but
not quite meriting an rc4" items on the Planning page. If we need an rc4
I can apply it directly. (Though if you did whatever testing you'd do on
a pullreq that's beyond just "run it through gitlab" that would be
useful I think.)

thanks
-- PMM