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 <danielhb...@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
