Currently if option '-icount auto' is passed to the QEMU TCG to enable
counting instructions the VM crashes with the following error report when
Linux runs on it:

qemu-system-ppc64: Bad icount read

This happens because read/write access to the SPRs PURR, VTB, and TBU40
is not integrated to the icount framework.

This commit fixes that issue by making the read/write access of these
SPRs aware of icount framework, adding the proper gen_io_start/end() calls
before/after calling the helpers to load/store these SPRs in TCG.

Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
---
 target/ppc/translate_init.inc.c | 35 +++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 7e66822b5d..6bf5795c69 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -284,12 +284,26 @@ static void spr_write_atbu(DisasContext *ctx, int sprn, int gprn)
 ATTRIBUTE_UNUSED
 static void spr_read_purr(DisasContext *ctx, int gprn, int sprn)
 {
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
     gen_helper_load_purr(cpu_gpr[gprn], cpu_env);
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_end();
+        gen_stop_exception(ctx);
+    }
 }
 
 static void spr_write_purr(DisasContext *ctx, int sprn, int gprn)
 {
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
     gen_helper_store_purr(cpu_env, cpu_gpr[gprn]);
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_end();
+        gen_stop_exception(ctx);
+    }
 }
 
 /* HDECR */
@@ -319,17 +333,38 @@ static void spr_write_hdecr(DisasContext *ctx, int sprn, int gprn)
 
 static void spr_read_vtb(DisasContext *ctx, int gprn, int sprn)
 {
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
     gen_helper_load_vtb(cpu_gpr[gprn], cpu_env);
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_end();
+        gen_stop_exception(ctx);
+    }
 }
 
 static void spr_write_vtb(DisasContext *ctx, int sprn, int gprn)
 {
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
     gen_helper_store_vtb(cpu_env, cpu_gpr[gprn]);
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_end();
+        gen_stop_exception(ctx);
+    }
 }
 
 static void spr_write_tbu40(DisasContext *ctx, int sprn, int gprn)
 {
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
     gen_helper_store_tbu40(cpu_env, cpu_gpr[gprn]);
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_end();
+        gen_stop_exception(ctx);
+    }
 }
 
 #endif
-- 
2.17.1

On Tue, 11 Aug 2020 at 02:29, Gustavo Romero <gromero@linux.ibm.com> wrote:
>
> Currently if option '-icount auto' is passed to the QEMU TCG to enable
> counting instructions the VM crashes with the following error report when
> Linux runs on it:
>
> qemu-system-ppc64: Bad icount read
>
> This happens because read/write access to the SPRs PURR, VTB, and TBU40
> is not integrated to the icount framework.
>
> This commit fixes that issue by making the read/write access of these
> SPRs aware of icount framework, adding the proper gen_io_start/end() calls
> before/after calling the helpers to load/store these SPRs in TCG.
>
> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> @@ -284,12 +284,26 @@ static void spr_write_atbu(DisasContext *ctx, int sprn, int gprn)
>  ATTRIBUTE_UNUSED
>  static void spr_read_purr(DisasContext *ctx, int gprn, int sprn)
>  {
> +    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> +        gen_io_start();
> +    }
>      gen_helper_load_purr(cpu_gpr[gprn], cpu_env);
> +    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> +        gen_io_end();
> +        gen_stop_exception(ctx);
> +    }

You don't want to call gen_io_end; you just need to ensure that
you end the TB immediately after this insn. See
docs/devel/tcg-icount.rst.

thanks
-- PMM

Hi Peter,

On 8/11/20 6:31 AM, Peter Maydell wrote:
> On Tue, 11 Aug 2020 at 02:29, Gustavo Romero <gromero@linux.ibm.com> wrote:
>>
>> Currently if option '-icount auto' is passed to the QEMU TCG to enable
>> counting instructions the VM crashes with the following error report when
>> Linux runs on it:
>>
>> qemu-system-ppc64: Bad icount read
>>
>> This happens because read/write access to the SPRs PURR, VTB, and TBU40
>> is not integrated to the icount framework.
>>
>> This commit fixes that issue by making the read/write access of these
>> SPRs aware of icount framework, adding the proper gen_io_start/end() calls
>> before/after calling the helpers to load/store these SPRs in TCG.
>>
>> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
>> @@ -284,12 +284,26 @@ static void spr_write_atbu(DisasContext *ctx, int sprn, int gprn)
>>   ATTRIBUTE_UNUSED
>>   static void spr_read_purr(DisasContext *ctx, int gprn, int sprn)
>>   {
>> +    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
>> +        gen_io_start();
>> +    }
>>       gen_helper_load_purr(cpu_gpr[gprn], cpu_env);
>> +    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
>> +        gen_io_end();
>> +        gen_stop_exception(ctx);
>> +    }
> 
> You don't want to call gen_io_end; you just need to ensure that
> you end the TB immediately after this insn. See
> docs/devel/tcg-icount.rst.

I understand that to ensure that TB ends immediately after these
instructions (I understood you meant all the cases, not just the
spr_read_purr case, right?), the instructions should be a branch
or change CPU state in a way that cannot be deduced at translation
time, and I don't know how to ensure that in these cases, they
are neither, specially for the read access, which doesn't change
any CPU state specifically afaics.

If I remove the gen_io_end() from all these cases the VM gets
stuck at apparently random points of execution (I'm digging
into details right now trying to understand why).

Thanks a lot.


Cheers,
Gustavo

On Tue, 11 Aug 2020 at 14:33, Gustavo Romero <gromero@linux.vnet.ibm.com> wrote:
> On 8/11/20 6:31 AM, Peter Maydell wrote:
> > You don't want to call gen_io_end; you just need to ensure that
> > you end the TB immediately after this insn. See
> > docs/devel/tcg-icount.rst.
>
> I understand that to ensure that TB ends immediately after these
> instructions (I understood you meant all the cases, not just the
> spr_read_purr case, right?), the instructions should be a branch
> or change CPU state in a way that cannot be deduced at translation
> time, and I don't know how to ensure that in these cases, they
> are neither, specially for the read access, which doesn't change
> any CPU state specifically afaics.

No, you have that the wrong way around. *If* an instruction
is a branch or a state-changing one, *then* it must end
the TB. That doesn't mean that *only* those kinds of insn
can end the TB -- other things also can end a TB. (It also
doesn't mean that a branch etc will automatically end the
TB -- it means that if you're writing the bit of target
code that generates code for a branch/etc then you must
specifically ensure that you end the TB.)

> If I remove the gen_io_end() from all these cases the VM gets
> stuck at apparently random points of execution (I'm digging
> into details right now trying to understand why).

Probably because you're not ending the TB after the insn.

PowerPC seems to be doing something slightly weird in this area --
it classifies "stop translation" as a kind of exception
(POWERPC_EXCP_STOP) rather than just using the common-code
is_jmp machinery and setting it to DISAS_EXIT. So you'll
need a ppc expert to say what the right thing is, but my
guess is you want to call gen_stop_exception() -- compare
gen_darn().

thanks
-- PMM

On 8/11/20 11:24 AM, Peter Maydell wrote:
> On Tue, 11 Aug 2020 at 14:33, Gustavo Romero <gromero@linux.vnet.ibm.com> wrote:
>> On 8/11/20 6:31 AM, Peter Maydell wrote:
>>> You don't want to call gen_io_end; you just need to ensure that
>>> you end the TB immediately after this insn. See
>>> docs/devel/tcg-icount.rst.
>>
>> I understand that to ensure that TB ends immediately after these
>> instructions (I understood you meant all the cases, not just the
>> spr_read_purr case, right?), the instructions should be a branch
>> or change CPU state in a way that cannot be deduced at translation
>> time, and I don't know how to ensure that in these cases, they
>> are neither, specially for the read access, which doesn't change
>> any CPU state specifically afaics.
> 
> No, you have that the wrong way around. *If* an instruction
> is a branch or a state-changing one, *then* it must end
> the TB. That doesn't mean that *only* those kinds of insn
> can end the TB -- other things also can end a TB. (It also
> doesn't mean that a branch etc will automatically end the
> TB -- it means that if you're writing the bit of target
> code that generates code for a branch/etc then you must
> specifically ensure that you end the TB.)

ah, ok. I got it now :) Thanks for the explanation.


>> If I remove the gen_io_end() from all these cases the VM gets
>> stuck at apparently random points of execution (I'm digging
>> into details right now trying to understand why).
> 
> Probably because you're not ending the TB after the insn.
> 
> PowerPC seems to be doing something slightly weird in this area --
> it classifies "stop translation" as a kind of exception
> (POWERPC_EXCP_STOP) rather than just using the common-code
> is_jmp machinery and setting it to DISAS_EXIT. So you'll
> need a ppc expert to say what the right thing is, but my
> guess is you want to call gen_stop_exception() -- compare
> gen_darn().

Right, if I change the code to call gen_stop_exception() like
in gen_darn() everything goes fine.

So, I'll send a v2 based on your review then PPC64 folks can
probably take a look at it. Could I add:

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

to v2?


Cheers,
Gustavo

On Tue, 11 Aug 2020 at 16:09, Gustavo Romero <gromero@linux.vnet.ibm.com> wrote:
> Right, if I change the code to call gen_stop_exception() like
> in gen_darn() everything goes fine.
>
> So, I'll send a v2 based on your review then PPC64 folks can
> probably take a look at it. Could I add:
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> to v2?

I'd rather wait and see your patch first; feel free to cc me.

-- PMM