Re: [PATCH 03/19] target/ppc: add exclusive user write function for PMU regs

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:41AM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
> 
> Similar to the previous patch, write read on PowerPC PMU regs
> requires extra handling than the generic write ureg functions.
> 
> This patch adds a specific write function for user PMU SPRs,
> spr_write_pmu_ureg(). MMCR0 and PMC1 are being treated before
> write, all other registers will be default to what is done in
> spr_write_ureg(), for now.
> 
> Since spr_write_pmu_ureg() needs to have a look in SPR_POWER_MMCR0
> to validate if the write is valid or not, we're adding a 'spr'
> array in DisasContext that points to env->spr.
> 
> CC: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  target/ppc/cpu_init.c  | 26 +++++++++++++-------------
>  target/ppc/spr_tcg.h   |  1 +
>  target/ppc/translate.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index d30aa0fe1e..71062809c8 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6868,47 +6868,47 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState 
> *env)
>  static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>  {
>      spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC1, "UPMC1",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC2, "UPMC2",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC3, "UPMC3",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC4, "UPMC4",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC5, "UPMC5",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC6, "UPMC6",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_USIAR, "USIAR",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_USDAR, "USDAR",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>  }
> @@ -6976,8 +6976,8 @@ static void register_power8_pmu_sup_sprs(CPUPPCState 
> *env)
>  static void register_power8_pmu_user_sprs(CPUPPCState *env)
>  {
>      spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
> -                 &spr_read_pmu_ureg, SPR_NOACCESS,
> -                 &spr_read_pmu_ureg, &spr_write_ureg,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
> +                 &spr_read_pmu_ureg, &spr_write_pmu_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_USIER, "USIER",
>                   &spr_read_generic, SPR_NOACCESS,
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 84ecba220f..40b5de34b9 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_read_pmu_generic(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_pmu_generic(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_pmu_ureg(DisasContext *ctx, int gprn, int sprn);
> +void spr_write_pmu_ureg(DisasContext *ctx, int gprn, int sprn);
>  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 d3a4d42ff8..29b0a340a9 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -176,6 +176,7 @@ struct DisasContext {
>      bool tm_enabled;
>      bool gtse;
>      ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> +    target_ulong *spr; /* Needed to check rights for mfspr/mtspr */
>      int singlestep_enabled;
>      uint32_t flags;
>      uint64_t insns_flags;
> @@ -573,6 +574,46 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int 
> gprn)
>  {
>      gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
>  }
> +
> +/* User special write access to PMU SPRs  */
> +void spr_write_pmu_ureg(DisasContext *ctx, int sprn, int gprn)
> +{
> +    TCGv t0, t1;
> +    int effective_sprn = sprn + 0x10;
> +
> +    if (((ctx->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
> +        /* Hypervisor Emulation Assistance interrupt */
> +        gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +        return;
> +    }
> +
> +    switch (effective_sprn) {
> +    case SPR_POWER_MMCR0:

Same comments as earlier patches about stacked multiplexing.

> +        t0 = tcg_temp_new();
> +        t1 = tcg_temp_new();
> +
> +        /*
> +         * Filter out all bits but FC, PMAO, and PMAE, according
> +         * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
> +         * fourth paragraph.
> +         */
> +        tcg_gen_andi_tl(t0, cpu_gpr[gprn],
> +                        MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE);
> +        gen_load_spr(t1, SPR_POWER_MMCR0);
> +        tcg_gen_andi_tl(t1, t1, ~(MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE));
> +        /* Keep all other bits intact */
> +        tcg_gen_or_tl(t1, t1, t0);
> +        gen_store_spr(effective_sprn, t1);
> +
> +        tcg_temp_free(t0);
> +        tcg_temp_free(t1);
> +        break;
> +    default:
> +        gen_store_spr(effective_sprn, cpu_gpr[gprn]);
> +        break;
> +    }
> +}
> +
>  #endif
>  
>  /* SPR common to all non-embedded PowerPC */
> @@ -8563,6 +8604,7 @@ static void ppc_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>      uint32_t hflags = ctx->base.tb->flags;
>  
>      ctx->spr_cb = env->spr_cb;
> +    ctx->spr = env->spr;

Eep... with that one line you're copying 8kiB of data into the context
structure.  That sounds undesirable.. especially since it look like
you only check 8 bytes of it.

Plus.. TBH, I'm a bit fuzzy on how the disascontext stuff works, but
I'm not sure copying the stuff here is correct.  I think it gets set
up at the beginning of each translated basic block.  But I don't think
anything prevents there being multiple mtmmcr0 instructions in a
single translated block, which means the values you're checking will
be outdated for the second one.

I think instead you need to actually generate the instructions to read
from MMCR0 and conditionally generate an exception if the permission
bit isn't set.  Or else just move all the PMU SPR logic to helper
functions which can just look at env->spr directly.  But that might
not be desirable since I expect the counter reads at least might be a
hot path.

>      ctx->pr = (hflags >> HFLAGS_PR) & 1;
>      ctx->mem_idx = (hflags >> HFLAGS_DMMU_IDX) & 7;
>      ctx->dr = (hflags >> HFLAGS_DR) & 1;

-- 
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