Re: [PATCH 1/8] target/ppc: introduce power8-pmu-insn-cnt.c.inc

[Daniel Henrique Barboza]

On 12/22/21 15:00, Cédric Le Goater wrote:
> On 12/22/21 14:45, Daniel Henrique Barboza wrote:
> > We're going to add a significant amount of TCG ops code for
> > instruction counting, eventually getting rid of the 'helper_insn_inc'
> > helper entirely.
> >
> > Create a new file to avoid putting even more stuff on the already
> > crowded target/ppc/translate.c.
>
> We already have a power8-pmu-regs.c.inc file. Couldn't we use it or is it
> a bad idea ?

That was my first idea. But then I thought that this code would feel out of 
place
in power8-pmu-regs.c.inc since, at this moment, the file only has SPR read/write
implementations that are being declared by spr_tcg.h to be used by cpu_init.c.


I guess it's a matter of taste. I'm ok putting this code in 
power8-pmu-regs.c.inc if
you think it's better.



Daniel

> Thanks,
>
> C.
>
>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   target/ppc/power8-pmu-insn-cnt.c.inc | 54 ++++++++++++++++++++++++++++
> >   target/ppc/translate.c               | 44 ++---------------------
> >   2 files changed, 56 insertions(+), 42 deletions(-)
> >   create mode 100644 target/ppc/power8-pmu-insn-cnt.c.inc
> >
> > diff --git a/target/ppc/power8-pmu-insn-cnt.c.inc 
> > b/target/ppc/power8-pmu-insn-cnt.c.inc
> > new file mode 100644
> > index 0000000000..2febbcc27e
> > --- /dev/null
> > +++ b/target/ppc/power8-pmu-insn-cnt.c.inc
> > @@ -0,0 +1,54 @@
> > +/*
> > + * PMU instruction counting for TCG IBM POWER chips
> > + *
> > + * Copyright IBM Corp. 2021
> > + *
> > + * Authors:
> > + *  Daniel Henrique Barboza      <danielhb413@gmail.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#if defined(TARGET_PPC64)
> > +static void pmu_count_insns(DisasContext *ctx)
> > +{
> > +    /*
> > +     * Do not bother calling the helper if the PMU isn't counting
> > +     * instructions.
> > +     */
> > +    if (!ctx->pmu_insn_cnt) {
> > +        return;
> > +    }
> > +
> > + #if !defined(CONFIG_USER_ONLY)
> > +    /*
> > +     * The PMU insns_inc() helper stops the internal PMU timer if a
> > +     * counter overflows happens. In that case, if the guest is
> > +     * running with icount and we do not handle it beforehand,
> > +     * the helper can trigger a 'bad icount read'.
> > +     */
> > +    gen_icount_io_start(ctx);
> > +
> > +    gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
> > +#else
> > +    /*
> > +     * User mode can read (but not write) PMC5 and start/stop
> > +     * the PMU via MMCR0_FC. In this case just increment
> > +     * PMC5 with base.num_insns.
> > +     */
> > +    TCGv t0 = tcg_temp_new();
> > +
> > +    gen_load_spr(t0, SPR_POWER_PMC5);
> > +    tcg_gen_addi_tl(t0, t0, ctx->base.num_insns);
> > +    gen_store_spr(SPR_POWER_PMC5, t0);
> > +
> > +    tcg_temp_free(t0);
> > +#endif /* #if !defined(CONFIG_USER_ONLY) */
> > +}
> > +#else
> > +static void pmu_count_insns(DisasContext *ctx)
> > +{
> > +    return;
> > +}
> > +#endif /* #if defined(TARGET_PPC64) */
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 114456148c..44773bc6cd 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -4183,48 +4183,8 @@ static inline void gen_update_cfar(DisasContext *ctx, 
> > target_ulong nip)
> >   #endif
> >   }
> > -#if defined(TARGET_PPC64)
> > -static void pmu_count_insns(DisasContext *ctx)
> > -{
> > -    /*
> > -     * Do not bother calling the helper if the PMU isn't counting
> > -     * instructions.
> > -     */
> > -    if (!ctx->pmu_insn_cnt) {
> > -        return;
> > -    }
> > -
> > - #if !defined(CONFIG_USER_ONLY)
> > -    /*
> > -     * The PMU insns_inc() helper stops the internal PMU timer if a
> > -     * counter overflows happens. In that case, if the guest is
> > -     * running with icount and we do not handle it beforehand,
> > -     * the helper can trigger a 'bad icount read'.
> > -     */
> > -    gen_icount_io_start(ctx);
> > -
> > -    gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
> > -#else
> > -    /*
> > -     * User mode can read (but not write) PMC5 and start/stop
> > -     * the PMU via MMCR0_FC. In this case just increment
> > -     * PMC5 with base.num_insns.
> > -     */
> > -    TCGv t0 = tcg_temp_new();
> > -
> > -    gen_load_spr(t0, SPR_POWER_PMC5);
> > -    tcg_gen_addi_tl(t0, t0, ctx->base.num_insns);
> > -    gen_store_spr(SPR_POWER_PMC5, t0);
> > -
> > -    tcg_temp_free(t0);
> > -#endif /* #if !defined(CONFIG_USER_ONLY) */
> > -}
> > -#else
> > -static void pmu_count_insns(DisasContext *ctx)
> > -{
> > -    return;
> > -}
> > -#endif /* #if defined(TARGET_PPC64) */
> > +/* For pmu_count_insns */
> > +#include "target/ppc/power8-pmu-insn-cnt.c.inc"
> >   static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
> >   {