Re: [PATCH v8 07/10] target/ppc/power8-pmu.c: add PM_RUN_INST_CMPL (0xFA) event

[David Gibson]

On Thu, Nov 25, 2021 at 12:08:14PM -0300, Daniel Henrique Barboza wrote:
> PM_RUN_INST_CMPL, instructions completed with the run latch set, is
> the architected PowerISA v3.1 event defined with PMC4SEL = 0xFA.
> 
> Implement it by checking for the CTRL RUN bit before incrementing the
> counter. To make this work properly we also need to force a new
> translation block each time SPR_CTRL is written. A small tweak in
> pmu_increment_insns() is then needed to only increment this event
> if the thread has the run latch.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Obviously, it would also be possible to treat the runlatch
instructions event like the all-instructions event but force an update
on runlatch changes.  Having to incoporate CTRL into the active
counter logic as well as the other stuff seems like it might make
things messier that way overall though.

> ---
>  target/ppc/cpu.h        |  4 ++++
>  target/ppc/cpu_init.c   |  2 +-
>  target/ppc/power8-pmu.c | 24 ++++++++++++++++++++++--
>  target/ppc/spr_tcg.h    |  1 +
>  target/ppc/translate.c  | 12 ++++++++++++
>  5 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 38cd2b5c43..993884164f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -304,6 +304,7 @@ typedef enum {
>      PMU_EVENT_INACTIVE,
>      PMU_EVENT_CYCLES,
>      PMU_EVENT_INSTRUCTIONS,
> +    PMU_EVENT_INSN_RUN_LATCH,
>  } PMUEventType;
>  
>  
> /*****************************************************************************/
> @@ -389,6 +390,9 @@ typedef enum {
>  #define MMCR1_PMC4SEL_START 56
>  #define MMCR1_PMC4EVT_EXTR (64 - MMCR1_PMC4SEL_START - MMCR1_EVT_SIZE)
>  
> +/* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
> +#define CTRL_RUN PPC_BIT(63)
> +
>  /* LPCR bits */
>  #define LPCR_VPM0         PPC_BIT(0)
>  #define LPCR_VPM1         PPC_BIT(1)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 2d72dde26d..ecce4c7c1e 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6749,7 +6749,7 @@ static void register_book3s_ctrl_sprs(CPUPPCState *env)
>  {
>      spr_register(env, SPR_CTRL, "SPR_CTRL",
>                   SPR_NOACCESS, SPR_NOACCESS,
> -                 SPR_NOACCESS, &spr_write_generic,
> +                 SPR_NOACCESS, &spr_write_CTRL,
>                   0x00000000);
>      spr_register(env, SPR_UCTRL, "SPR_UCTRL",
>                   &spr_read_ureg, SPR_NOACCESS,
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 59d0def79d..98797f0b2f 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -96,6 +96,15 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int 
> sprn)
>              evt_type = PMU_EVENT_CYCLES;
>          }
>          break;
> +    case 0xFA:
> +        /*
> +         * PMC4SEL = 0xFA is the "instructions completed
> +         * with run latch set" event.
> +         */
> +        if (sprn == SPR_POWER_PMC4) {
> +            evt_type = PMU_EVENT_INSN_RUN_LATCH;
> +        }
> +        break;
>      case 0xFE:
>          /*
>           * PMC1SEL = 0xFE is the architected PowerISA v3.1
> @@ -119,11 +128,22 @@ static bool pmu_increment_insns(CPUPPCState *env, 
> uint32_t num_insns)
>  
>      /* PMC6 never counts instructions */
>      for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> -        if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
> +        PMUEventType evt_type = pmc_get_event(env, sprn);
> +        bool insn_event = evt_type == PMU_EVENT_INSTRUCTIONS ||
> +                          evt_type == PMU_EVENT_INSN_RUN_LATCH;
> +
> +        if (pmc_is_inactive(env, sprn) || !insn_event) {
>              continue;
>          }
>  
> -        env->spr[sprn] += num_insns;
> +        if (evt_type == PMU_EVENT_INSTRUCTIONS) {
> +            env->spr[sprn] += num_insns;
> +        }
> +
> +        if (evt_type == PMU_EVENT_INSN_RUN_LATCH &&
> +            env->spr[SPR_CTRL] & CTRL_RUN) {
> +            env->spr[sprn] += num_insns;
> +        }
>  
>          if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
>              pmc_has_overflow_enabled(env, sprn)) {
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 1d6521eedc..f98d97c0ba 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -28,6 +28,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int 
> gprn);
>  void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
>  void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn);
>  void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index ccc83d0603..d0e361a9d1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -403,6 +403,18 @@ void spr_write_generic(DisasContext *ctx, int sprn, int 
> gprn)
>      spr_store_dump_spr(sprn);
>  }
>  
> +void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> +{
> +    spr_write_generic(ctx, sprn, gprn);
> +
> +    /*
> +     * SPR_CTRL writes must force a new translation block,
> +     * allowing the PMU to calculate the run latch events with
> +     * more accuracy.
> +     */
> +    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> +}
> +
>  #if !defined(CONFIG_USER_ONLY)
>  void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>  {

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature