| Message ID | 20210903203116.80628-3-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PMU-EBB support for PPC64 TCG | expand |
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) {
On 9/6/21 22:38, David Gibson wrote: > 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. My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*) are the supermode privileged SPRs that can't be written by userspace. At least this is my understanding from reading commit fd51ff6328e3d98158 that introduced these userspace PMC regs. The reason why these are marked as SPR_NOACCESS is because we didn't bothered writing into them from userspace because we had no PMU logic to work with. > >> &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? Moving into the existing power8_pmu.c helper is annoying because, being a helper file, there is no access to the whole DisasContext declaration (that is open coded in translate.c), and other internal translate.c data like cpu_grp[]. What I was able to do is create a new file in the target/ppc/translate/ dir, power8-pmu-regs.c.impl, and moved all these declarations over there. At very least we're not overloading translate.c. Eldorado, is that ok with you guys? I'm aware that this dir was holding new decode-tree insns implementations but, in this case, it would hold old format spr_read/spr_write code. > >> +{ >> + 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? That's an ooopsie. Thanks, Daniel > >> +{ >> + 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) { >
On Thu, Sep 23, 2021 at 11:39:14AM -0300, Daniel Henrique Barboza wrote: > > > On 9/6/21 22:38, David Gibson wrote: > > 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. > > My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to > allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*) > are the supermode privileged SPRs that can't be written by userspace. At least this > is my understanding from reading commit fd51ff6328e3d98158 that introduced these > userspace PMC regs. Sure, but my point is that these registers are only userspace accessible under certain conditions, IIUC. spr_write_ureg() doesn't test for those conditions, so it will *always* allow write access. > The reason why these are marked as SPR_NOACCESS is because we didn't bothered > writing into them from userspace because we had no PMU logic to work > with. [snip] > > > 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? > > Moving into the existing power8_pmu.c helper is annoying because, being a helper file, > there is no access to the whole DisasContext declaration (that is open coded in > translate.c), and other internal translate.c data like cpu_grp[]. Ah, right. We should probably make that easier someday, but it's not reasonbly in scope for this series. > What I was able to do is create a new file in the target/ppc/translate/ dir, > power8-pmu-regs.c.impl, and moved all these declarations over there. At very least we're > not overloading translate.c. Ah, nice. > Eldorado, is that ok with you guys? I'm aware that this dir was holding new > decode-tree insns implementations but, in this case, it would hold old format > spr_read/spr_write code.
On 9/27/21 02:08, David Gibson wrote: > On Thu, Sep 23, 2021 at 11:39:14AM -0300, Daniel Henrique Barboza wrote: >> >> >> On 9/6/21 22:38, David Gibson wrote: >>> 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. >> >> My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to >> allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*) >> are the supermode privileged SPRs that can't be written by userspace. At least this >> is my understanding from reading commit fd51ff6328e3d98158 that introduced these >> userspace PMC regs. > > Sure, but my point is that these registers are only userspace > accessible under certain conditions, IIUC. spr_write_ureg() doesn't > test for those conditions, so it will *always* allow write access. Got it. I guess I'll end up biting the bullet and exposing both PMCC bits and adding proper read/write access controls for the callbacks we need. This is somewhat out of scope of my original goal with this series, but I guess we'll all better off by doing it right now. I'll add all the read/write ureg functions I'll need in the first patches (the PMC write callback functions are on the patch 14, for instance). That will, hopefully, making it easier to review the rest of the series by going through all the access control and read/write callbacks early on. Thanks, Daniel > >> The reason why these are marked as SPR_NOACCESS is because we didn't bothered >> writing into them from userspace because we had no PMU logic to work >> with. > > [snip] >>>> 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? >> >> Moving into the existing power8_pmu.c helper is annoying because, being a helper file, >> there is no access to the whole DisasContext declaration (that is open coded in >> translate.c), and other internal translate.c data like cpu_grp[]. > > Ah, right. We should probably make that easier someday, but it's not > reasonbly in scope for this series. > >> What I was able to do is create a new file in the target/ppc/translate/ dir, >> power8-pmu-regs.c.impl, and moved all these declarations over there. At very least we're >> not overloading translate.c. > > Ah, nice. > >> Eldorado, is that ok with you guys? I'm aware that this dir was holding new >> decode-tree insns implementations but, in this case, it would hold old format >> spr_read/spr_write code. >
On Mon, Sep 27, 2021 at 08:05:22PM -0300, Daniel Henrique Barboza wrote: > > > On 9/27/21 02:08, David Gibson wrote: > > On Thu, Sep 23, 2021 at 11:39:14AM -0300, Daniel Henrique Barboza wrote: > > > > > > > > > On 9/6/21 22:38, David Gibson wrote: > > > > 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. > > > > > > My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to > > > allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*) > > > are the supermode privileged SPRs that can't be written by userspace. At least this > > > is my understanding from reading commit fd51ff6328e3d98158 that introduced these > > > userspace PMC regs. > > > > Sure, but my point is that these registers are only userspace > > accessible under certain conditions, IIUC. spr_write_ureg() doesn't > > test for those conditions, so it will *always* allow write access. > > > Got it. > > I guess I'll end up biting the bullet and exposing both PMCC bits and adding > proper read/write access controls for the callbacks we need. This is somewhat > out of scope of my original goal with this series, but I guess we'll all better > off by doing it right now. Yeah, sorry to divert you from the EBB stuff, but I don't want to merge PMU support with glaring flaws. > I'll add all the read/write ureg functions I'll need in the first patches (the PMC > write callback functions are on the patch 14, for instance). That will, hopefully, > making it easier to review the rest of the series by going through all the access > control and read/write callbacks early on. That sounds good.
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, &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) +{ + 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) +{ + 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) {
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(-)