[PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[Peter Maydell]

The sparc frontend is now the only user of the obsolete gen_io_end()
function (used for icount support). This patchset removes the
use from sparc as well, and then tidies up the generic icount
infrastructure to remove the function altogether.

This is for-6.2 material because it's just cleanup.

There is a slight difficulty here with testing this: icount
doesn't seem to work for sparc Linux guests in master at the
moment. For instance if you get the advent calendar image from
  https://www.qemu-advent-calendar.org/2018/download/day11.tar.xz
it will boot without icount with a command line like
  qemu-system-sparc -display none -vga none -machine SS-20 -serial stdio -kernel /tmp/day11/zImage.elf
But if you add '-icount auto' it will get as far as
"bootconsole [earlyprom0] disabled" and then apparently hang.
I'm not sure what's going on here :-(
(I filed this as https://gitlab.com/qemu-project/qemu/-/issues/499)

Anyway, these patches don't make the situation any worse...

-- PMM


Peter Maydell (2):
  target/sparc: Drop use of gen_io_end()
  tcg: Drop gen_io_end()

 docs/devel/tcg-icount.rst |  3 ---
 include/exec/gen-icount.h | 27 ++++++++++-----------------
 target/sparc/translate.c  | 25 ++++++++++---------------
 3 files changed, 20 insertions(+), 35 deletions(-)

-- 
2.20.1

[PATCH for-6.2 1/2] target/sparc: Drop use of gen_io_end()

[Peter Maydell]

The gen_io_end() function is obsolete (as documented in
docs/devel/tcg-icount.rst). Where an instruction is an I/O
operation, the translator frontend should call gen_io_start()
before generating the code which does the I/O, and then
end the TB immediately after this insn.

Remove the calls to gen_io_end() in the SPARC frontend,
and ensure that the insns which were calling it end the
TB if they didn't do so already.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/translate.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 11de5a49631..bb70ba17deb 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -3401,7 +3401,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                         tcg_temp_free_i32(r_const);
                         gen_store_gpr(dc, rd, cpu_dst);
                         if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-                            gen_io_end();
+                            /* I/O operations in icount mode must end the TB */
+                            dc->base.is_jmp = DISAS_EXIT;
                         }
                     }
                     break;
@@ -3454,7 +3455,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                         tcg_temp_free_i32(r_const);
                         gen_store_gpr(dc, rd, cpu_dst);
                         if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-                            gen_io_end();
+                            /* I/O operations in icount mode must end the TB */
+                            dc->base.is_jmp = DISAS_EXIT;
                         }
                     }
                     break;
@@ -3588,7 +3590,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                         tcg_temp_free_ptr(r_tickptr);
                         tcg_temp_free_i32(r_const);
                         if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-                            gen_io_end();
+                            /* I/O operations in icount mode must end the TB */
+                            dc->base.is_jmp = DISAS_EXIT;
                         }
                     }
                     break;
@@ -4582,7 +4585,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                                 }
                                 gen_helper_wrpstate(cpu_env, cpu_tmp0);
                                 if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-                                    gen_io_end();
+                                    /* I/O ops in icount mode must end the TB */
+                                    dc->base.is_jmp = DISAS_EXIT;
                                 }
                                 dc->npc = DYNAMIC_PC;
                                 break;
@@ -4598,7 +4602,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                                 }
                                 gen_helper_wrpil(cpu_env, cpu_tmp0);
                                 if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-                                    gen_io_end();
+                                    /* I/O ops in icount mode must end the TB */
+                                    dc->base.is_jmp = DISAS_EXIT;
                                 }
                                 break;
                             case 9: // cwp
@@ -4697,10 +4702,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                                     gen_helper_tick_set_limit(r_tickptr,
                                                               cpu_hstick_cmpr);
                                     tcg_temp_free_ptr(r_tickptr);
-                                    if (tb_cflags(dc->base.tb) &
-                                           CF_USE_ICOUNT) {
-                                        gen_io_end();
-                                    }
                                     /* End TB to handle timer interrupt */
                                     dc->base.is_jmp = DISAS_EXIT;
                                 }
@@ -5327,9 +5328,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                                 gen_io_start();
                             }
                             gen_helper_done(cpu_env);
-                            if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-                                gen_io_end();
-                            }
                             goto jmp_insn;
                         case 1:
                             if (!supervisor(dc))
@@ -5340,9 +5338,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                                 gen_io_start();
                             }
                             gen_helper_retry(cpu_env);
-                            if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-                                gen_io_end();
-                            }
                             goto jmp_insn;
                         default:
                             goto illegal_insn;
-- 
2.20.1

Re: [PATCH for-6.2 1/2] target/sparc: Drop use of gen_io_end()

[Mark Cave-Ayland]

On 24/07/2021 14:49, Peter Maydell wrote:

> The gen_io_end() function is obsolete (as documented in
> docs/devel/tcg-icount.rst). Where an instruction is an I/O
> operation, the translator frontend should call gen_io_start()
> before generating the code which does the I/O, and then
> end the TB immediately after this insn.
> 
> Remove the calls to gen_io_end() in the SPARC frontend,
> and ensure that the insns which were calling it end the
> TB if they didn't do so already.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/sparc/translate.c | 25 ++++++++++---------------
>   1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> index 11de5a49631..bb70ba17deb 100644
> --- a/target/sparc/translate.c
> +++ b/target/sparc/translate.c
> @@ -3401,7 +3401,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                           tcg_temp_free_i32(r_const);
>                           gen_store_gpr(dc, rd, cpu_dst);
>                           if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> -                            gen_io_end();
> +                            /* I/O operations in icount mode must end the TB */
> +                            dc->base.is_jmp = DISAS_EXIT;
>                           }
>                       }
>                       break;
> @@ -3454,7 +3455,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                           tcg_temp_free_i32(r_const);
>                           gen_store_gpr(dc, rd, cpu_dst);
>                           if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> -                            gen_io_end();
> +                            /* I/O operations in icount mode must end the TB */
> +                            dc->base.is_jmp = DISAS_EXIT;
>                           }
>                       }
>                       break;
> @@ -3588,7 +3590,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                           tcg_temp_free_ptr(r_tickptr);
>                           tcg_temp_free_i32(r_const);
>                           if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> -                            gen_io_end();
> +                            /* I/O operations in icount mode must end the TB */
> +                            dc->base.is_jmp = DISAS_EXIT;
>                           }
>                       }
>                       break;
> @@ -4582,7 +4585,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                                   }
>                                   gen_helper_wrpstate(cpu_env, cpu_tmp0);
>                                   if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> -                                    gen_io_end();
> +                                    /* I/O ops in icount mode must end the TB */
> +                                    dc->base.is_jmp = DISAS_EXIT;
>                                   }
>                                   dc->npc = DYNAMIC_PC;
>                                   break;
> @@ -4598,7 +4602,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                                   }
>                                   gen_helper_wrpil(cpu_env, cpu_tmp0);
>                                   if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> -                                    gen_io_end();
> +                                    /* I/O ops in icount mode must end the TB */
> +                                    dc->base.is_jmp = DISAS_EXIT;
>                                   }
>                                   break;
>                               case 9: // cwp
> @@ -4697,10 +4702,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                                       gen_helper_tick_set_limit(r_tickptr,
>                                                                 cpu_hstick_cmpr);
>                                       tcg_temp_free_ptr(r_tickptr);
> -                                    if (tb_cflags(dc->base.tb) &
> -                                           CF_USE_ICOUNT) {
> -                                        gen_io_end();
> -                                    }
>                                       /* End TB to handle timer interrupt */
>                                       dc->base.is_jmp = DISAS_EXIT;
>                                   }
> @@ -5327,9 +5328,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                                   gen_io_start();
>                               }
>                               gen_helper_done(cpu_env);
> -                            if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> -                                gen_io_end();
> -                            }
>                               goto jmp_insn;
>                           case 1:
>                               if (!supervisor(dc))
> @@ -5340,9 +5338,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
>                                   gen_io_start();
>                               }
>                               gen_helper_retry(cpu_env);
> -                            if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> -                                gen_io_end();
> -                            }
>                               goto jmp_insn;
>                           default:
>                               goto illegal_insn;
> 

Thanks Peter, this is something that has been in the back of my mind as a TODO for a 
while but I haven't managed to get around to it. From what I can see the conversion 
looks good so:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH for-6.2 2/2] tcg: Drop gen_io_end()

[Peter Maydell]

Now we have removed all the uses of gen_io_end() from target frontends,
the only callsite is inside gen_tb_start(). Inline the code there,
and remove the reference to it from the documentation.

While we are inlining the code, switch it to use tcg_constant_i32()
so we don't have to manually create and destroy a TCG temporary.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/tcg-icount.rst |  3 ---
 include/exec/gen-icount.h | 27 ++++++++++-----------------
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/docs/devel/tcg-icount.rst b/docs/devel/tcg-icount.rst
index 8d67b6c076a..50c8e8dabc1 100644
--- a/docs/devel/tcg-icount.rst
+++ b/docs/devel/tcg-icount.rst
@@ -92,6 +92,3 @@ When the translator is handling an instruction of this kind:
     }
 
 * it must end the TB immediately after this instruction
-
-Note that some older front-ends call a "gen_io_end()" function:
-this is obsolete and should not be used.
diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 467529d84c5..610cba58feb 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -17,22 +17,6 @@ static inline void gen_io_start(void)
     tcg_temp_free_i32(tmp);
 }
 
-/*
- * cpu->can_do_io is cleared automatically at the beginning of
- * each translation block.  The cost is minimal and only paid
- * for -icount, plus it would be very easy to forget doing it
- * in the translator.  Therefore, backends only need to call
- * gen_io_start.
- */
-static inline void gen_io_end(void)
-{
-    TCGv_i32 tmp = tcg_const_i32(0);
-    tcg_gen_st_i32(tmp, cpu_env,
-                   offsetof(ArchCPU, parent_obj.can_do_io) -
-                   offsetof(ArchCPU, env));
-    tcg_temp_free_i32(tmp);
-}
-
 static inline void gen_tb_start(const TranslationBlock *tb)
 {
     TCGv_i32 count;
@@ -64,7 +48,16 @@ static inline void gen_tb_start(const TranslationBlock *tb)
         tcg_gen_st16_i32(count, cpu_env,
                          offsetof(ArchCPU, neg.icount_decr.u16.low) -
                          offsetof(ArchCPU, env));
-        gen_io_end();
+        /*
+         * cpu->can_do_io is cleared automatically here at the beginning of
+         * each translation block.  The cost is minimal and only paid for
+         * -icount, plus it would be very easy to forget doing it in the
+         * translator. Doing it here means we don't need a gen_io_end() to
+         * go with gen_io_start().
+         */
+        tcg_gen_st_i32(tcg_constant_i32(0), cpu_env,
+                       offsetof(ArchCPU, parent_obj.can_do_io) -
+                       offsetof(ArchCPU, env));
     }
 
     tcg_temp_free_i32(count);
-- 
2.20.1

Re: [PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[Richard Henderson]

On 7/24/21 3:49 AM, Peter Maydell wrote:
> Peter Maydell (2):
>    target/sparc: Drop use of gen_io_end()
>    tcg: Drop gen_io_end()

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

r~

Re: [PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[Peter Maydell]

On Sat, 24 Jul 2021 at 14:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> There is a slight difficulty here with testing this: icount
> doesn't seem to work for sparc Linux guests in master at the
> moment. For instance if you get the advent calendar image from
>   https://www.qemu-advent-calendar.org/2018/download/day11.tar.xz
> it will boot without icount with a command line like
>   qemu-system-sparc -display none -vga none -machine SS-20 -serial stdio -kernel /tmp/day11/zImage.elf
> But if you add '-icount auto' it will get as far as
> "bootconsole [earlyprom0] disabled" and then apparently hang.
> I'm not sure what's going on here :-(
> (I filed this as https://gitlab.com/qemu-project/qemu/-/issues/499)

This turns out to be a recent regression, caused by commit 78ff82bb
("accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS"). It's
an intermittent rather than a 100% reproducible hang.

-- PMM

Re: [PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[Richard Henderson]

On 7/24/21 10:27 AM, Peter Maydell wrote:
> On Sat, 24 Jul 2021 at 14:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>> There is a slight difficulty here with testing this: icount
>> doesn't seem to work for sparc Linux guests in master at the
>> moment. For instance if you get the advent calendar image from
>>    https://www.qemu-advent-calendar.org/2018/download/day11.tar.xz
>> it will boot without icount with a command line like
>>    qemu-system-sparc -display none -vga none -machine SS-20 -serial stdio -kernel /tmp/day11/zImage.elf
>> But if you add '-icount auto' it will get as far as
>> "bootconsole [earlyprom0] disabled" and then apparently hang.
>> I'm not sure what's going on here :-(
>> (I filed this as https://gitlab.com/qemu-project/qemu/-/issues/499)
> 
> This turns out to be a recent regression, caused by commit 78ff82bb
> ("accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS"). It's
> an intermittent rather than a 100% reproducible hang.

Ouch.  Ok, I'll have a look.

r~

Re: [PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[Peter Maydell]

On Sat, 24 Jul 2021 at 21:48, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/24/21 10:27 AM, Peter Maydell wrote:
> > On Sat, 24 Jul 2021 at 14:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> There is a slight difficulty here with testing this: icount
> >> doesn't seem to work for sparc Linux guests in master at the
> >> moment. For instance if you get the advent calendar image from
> >>    https://www.qemu-advent-calendar.org/2018/download/day11.tar.xz
> >> it will boot without icount with a command line like
> >>    qemu-system-sparc -display none -vga none -machine SS-20 -serial stdio -kernel /tmp/day11/zImage.elf
> >> But if you add '-icount auto' it will get as far as
> >> "bootconsole [earlyprom0] disabled" and then apparently hang.
> >> I'm not sure what's going on here :-(
> >> (I filed this as https://gitlab.com/qemu-project/qemu/-/issues/499)
> >
> > This turns out to be a recent regression, caused by commit 78ff82bb
> > ("accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS"). It's
> > an intermittent rather than a 100% reproducible hang.
>
> Ouch.  Ok, I'll have a look.

I did a bit more messing around with a repro case under rr,
and I think I now see why we end up hanging, although I'm not
100% sure what best to do to fix it:

 * We have a TB with 512 insns (tb->icount == 512)
 * We want to execute 511 insns (icount_decr == 511)
 * the code generated by gen_tb_start() does "subtract
   this TB's instruction count from icount_decr.u16.low, and
   if this is negative jump to the exitlabel". 511 - 512 == -1,
   so we exit the TB with status TB_EXIT_REQUESTED and without
   executing any guest insns
 * in cpu_loop_exit_tb() we look at insns_left, which is still 511,
   so we don't take the "early return because exit_request" path
 * we calculate a new insns_left = MIN(CF_COUNT_MASK, cpu->icount_budget).
   icount_budget is 59267, so the new insns_left is still 511.
 * we set icount_decr.u16.low to insns_left (ie same value as before)
 * we set cpu->icount_extra to icount_budget - insns_left, which
   is 58756
 * because icount_extra is non-zero, we don't set cflags_next_tb
   to force us to find an exactly 511 insn TB
 * so we come out to the cpu_exec() main loop, and find again
   the same 512 insn TB we started with.
 * Nothing changed from the last time we tried to execute it so
   we just go round and round in circles never making any progress...

We don't get this failure mode if CF_COUNT_MASK is larger than
TCG_MAX_INSNS, because the calculation of insns_left will
produce a larger number than TCG_MAX_INSNS, unless we really
are running out of icount budget (in which case icount_extra
should be 0 and we will force execution of that smaller TB).

So the primary bug here is that cpu_loop_exec_tb() needs updating
to follow the new logic of "allow insns_left = TCG_MAX_INSNS and
indicate that with 0 in the CF_COUNT_MASK field".

Q: in cpu_loop_exec_tb() in this calculation:

    /*
     * If the next tb has more instructions than we have left to
     * execute we need to ensure we find/generate a TB with exactly
     * insns_left instructions in it.
     */
    if (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount)  {
        cpu->cflags_next_tb = (tb->cflags & ~CF_COUNT_MASK) | insns_left;
    }

why are we testing "!cpu->icount_extra" ? The thing the comment
says we're looking for ("this TB has more insns than we have
left to execute") would be just "insns_left > 0 && insns_left < tb->icount".
And the code generated in gen_tb_start() will exit without doing anything
if insns_left < tb->icount (as described above), so we'll get into
an infinite loop pretty much any time we decide not to force execution
of a smaller TB. It's merely that we're much more likely to do so
with CF_COUNT_MASK==511, because we are accidentally very often trying
to execute 511 insns of a 512 insn TB when we could execute all 512.

thanks
-- PMM

Re: [PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[Peter Maydell]

On Sat, 24 Jul 2021 at 14:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The sparc frontend is now the only user of the obsolete gen_io_end()
> function (used for icount support). This patchset removes the
> use from sparc as well, and then tidies up the generic icount
> infrastructure to remove the function altogether.
>
> This is for-6.2 material because it's just cleanup.

> Peter Maydell (2):
>   target/sparc: Drop use of gen_io_end()
>   tcg: Drop gen_io_end()

Mark, are you planning a sparc pullreq, or should I take these
via target-arm.next?

thanks
-- PMM

Re: [PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()

[Mark Cave-Ayland]

On 01/09/2021 08:58, Peter Maydell wrote:

> On Sat, 24 Jul 2021 at 14:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> The sparc frontend is now the only user of the obsolete gen_io_end()
>> function (used for icount support). This patchset removes the
>> use from sparc as well, and then tidies up the generic icount
>> infrastructure to remove the function altogether.
>>
>> This is for-6.2 material because it's just cleanup.
> 
>> Peter Maydell (2):
>>    target/sparc: Drop use of gen_io_end()
>>    tcg: Drop gen_io_end()
> 
> Mark, are you planning a sparc pullreq, or should I take these
> via target-arm.next?

I can take them: my plan was to take the sun4m smp fix and the updated ESCC patches 
together, so I can add these too.


ATB,

Mark.