* [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40
@ 2020-08-11 1:27 Gustavo Romero
2020-08-11 9:31 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo Romero @ 2020-08-11 1:27 UTC (permalink / raw)
To: qemu-ppc; +Cc: clg, david, qemu-devel, gromero
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40
2020-08-11 1:27 [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40 Gustavo Romero
@ 2020-08-11 9:31 ` Peter Maydell
2020-08-11 13:33 ` Gustavo Romero
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2020-08-11 9:31 UTC (permalink / raw)
To: Gustavo Romero
Cc: QEMU Developers, qemu-ppc, Cédric Le Goater, David Gibson
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40
2020-08-11 9:31 ` Peter Maydell
@ 2020-08-11 13:33 ` Gustavo Romero
2020-08-11 14:24 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo Romero @ 2020-08-11 13:33 UTC (permalink / raw)
To: Peter Maydell, Gustavo Romero
Cc: David Gibson, qemu-ppc, QEMU Developers, Cédric Le Goater
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40
2020-08-11 13:33 ` Gustavo Romero
@ 2020-08-11 14:24 ` Peter Maydell
2020-08-11 15:09 ` Gustavo Romero
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2020-08-11 14:24 UTC (permalink / raw)
To: Gustavo Romero
Cc: Cédric Le Goater, David Gibson, qemu-ppc, QEMU Developers,
Gustavo Romero
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40
2020-08-11 14:24 ` Peter Maydell
@ 2020-08-11 15:09 ` Gustavo Romero
2020-08-11 15:12 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo Romero @ 2020-08-11 15:09 UTC (permalink / raw)
To: Peter Maydell
Cc: Cédric Le Goater, David Gibson, qemu-ppc, QEMU Developers,
Gustavo Romero
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40
2020-08-11 15:09 ` Gustavo Romero
@ 2020-08-11 15:12 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2020-08-11 15:12 UTC (permalink / raw)
To: Gustavo Romero
Cc: Cédric Le Goater, David Gibson, qemu-ppc, QEMU Developers,
Gustavo Romero
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-11 15:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-11 1:27 [PATCH] target/ppc: Integrate icount to purr, vtb, and tbu40 Gustavo Romero
2020-08-11 9:31 ` Peter Maydell
2020-08-11 13:33 ` Gustavo Romero
2020-08-11 14:24 ` Peter Maydell
2020-08-11 15:09 ` Gustavo Romero
2020-08-11 15:12 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.