Re: [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs

[David Gibson]

On Fri, Sep 03, 2021 at 05:31:03PM -0300, Daniel Henrique Barboza wrote:
> The PMU needs to enable writing of its uregs to userspace, otherwise
> Perf applications will not able to setup the counters correctly. This
> patch enables user space writing of all PMU uregs.
> 
> MMCR0 is a special case because its userspace writing access is controlled
> by MMCR0_PMCC bits. There are 4 configurations available (0b00, 0b01,
> 0b10 and 0b11) but for our purposes here we're handling only
> MMCR0_PMCC = 0b00. In this case, if userspace tries to write MMCR0, a
> hypervisor emulation assistance interrupt occurs.
> 
> This is being done by adding HFLAGS_PMCCCLEAR to hflags. This flag
> indicates if MMCR0_PMCC is cleared (0b00), and a new 'pmcc_clear' flag in
> DisasContext allow us to use it in spr_write_MMCR0_ureg().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  target/ppc/cpu.h         |  1 +
>  target/ppc/cpu_init.c    | 18 +++++++-------
>  target/ppc/helper_regs.c |  3 +++
>  target/ppc/spr_tcg.h     |  3 ++-
>  target/ppc/translate.c   | 53 +++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f68bb8d8aa..8dfbb62022 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -616,6 +616,7 @@ enum {
>      HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
>      HFLAGS_FP = 13,  /* MSR_FP */
>      HFLAGS_PR = 14,  /* MSR_PR */
> +    HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
>      HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>      HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
>  
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 9efc6c2d87..bb5ea04c61 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6867,7 +6867,7 @@ 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_MMCR0_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> @@ -6875,31 +6875,31 @@ static void register_book3s_pmu_user_sprs(CPUPPCState 
> *env)
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC1, "UPMC1",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,

Surely this can't be write.  AFAICT spr_write_ureg() will
unconditionally allow full userspace write access.  That can't be
right - otherwise the OS could never safely use the PMU for itself.

>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC2, "UPMC2",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC3, "UPMC3",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC4, "UPMC4",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC5, "UPMC5",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC6, "UPMC6",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_USIAR, "USIAR",
> @@ -6975,7 +6975,7 @@ 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_MMCR2_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR2_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_USIER, "USIER",
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 405450d863..4c1d9575ac 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -106,6 +106,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState 
> *env)
>      if (env->spr[SPR_LPCR] & LPCR_GTSE) {
>          hflags |= 1 << HFLAGS_GTSE;
>      }
> +    if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
> +        hflags |= 1 << HFLAGS_PMCCCLEAR;
> +    }
>  
>  #ifndef CONFIG_USER_ONLY
>      if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 30cb6c3fdc..094466a2b2 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -42,6 +42,8 @@ void spr_read_601_rtcl(DisasContext *ctx, int gprn, int 
> sprn);
>  void spr_read_601_rtcu(DisasContext *ctx, int gprn, int sprn);
>  void spr_read_spefscr(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_spefscr(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn);
>  
>  #ifndef CONFIG_USER_ONLY
>  void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
> @@ -96,7 +98,6 @@ void spr_read_mas73(DisasContext *ctx, int gprn, int sprn);
>  #ifdef TARGET_PPC64
>  void spr_read_cfar(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_cfar(DisasContext *ctx, int sprn, int gprn);
> -void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_purr(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_purr(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_hdecr(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index b2ead144d1..0babde3131 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -175,6 +175,7 @@ struct DisasContext {
>      bool spe_enabled;
>      bool tm_enabled;
>      bool gtse;
> +    bool pmcc_clear;
>      ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>      int singlestep_enabled;
>      uint32_t flags;
> @@ -561,7 +562,56 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int 
> gprn)
>  {
>      gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
>  }
> -#endif
> +
> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)


Could you put this def in the PMU specific file, rather than the
enormous translate.c?

> +{
> +    TCGv t0, t1;
> +
> +    /*
> +     * For group A PMU sprs, if PMCC = 0b00, PowerISA v3.1
> +     * dictates that:
> +     *
> +     * "If an attempt is made to write to an SPR in group A in
> +     * problem state, a Hypervisor Emulation Assistance
> +     * interrupt will occur."
> +     *
> +     * MMCR0 is a Group A SPR and can't be written by userspace
> +     * if PMCC = 0b00.
> +     */
> +    if (ctx->pmcc_clear) {
> +        gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +        return;
> +    }
> +
> +    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_UREG_MASK);
> +    gen_load_spr(t1, SPR_POWER_MMCR0);
> +    tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
> +    /* Keep all other bits intact */
> +    tcg_gen_or_tl(t1, t1, t0);
> +    gen_store_spr(SPR_POWER_MMCR0, t1);
> +
> +    tcg_temp_free(t0);
> +    tcg_temp_free(t1);
> +}
> +#else
> +void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)

Why do you need another definition of spr_write_ureg() here?

> +{
> +    spr_noaccess(ctx, gprn, sprn);
> +}
> +
> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> +{
> +    spr_noaccess(ctx, gprn, sprn);
> +}
> +#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
>  
>  /* SPR common to all non-embedded PowerPC */
>  /* DECR */
> @@ -8576,6 +8626,7 @@ static void ppc_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>      ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
>      ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>      ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
> +    ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
>  
>      ctx->singlestep_enabled = 0;
>      if ((hflags >> HFLAGS_SE) & 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