From: David Gibson <david@gibson.dropbear.id.au>
To: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: richard.henderson@linaro.org, qemu-devel@nongnu.org,
groug@kaod.org, qemu-ppc@nongnu.org, clg@kaod.org,
matheus.ferst@eldorado.org.br
Subject: Re: [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs
Date: Tue, 7 Sep 2021 11:38:37 +1000 [thread overview]
Message-ID: <YTbCnSz86hsUAF/+@yekko> (raw)
In-Reply-To: <20210903203116.80628-3-danielhb413@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9639 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-09-07 2:16 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-03 20:31 [PATCH v3 00/15] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 01/15] target/ppc: add user read functions for MMCR0 and MMCR2 Daniel Henrique Barboza
2021-09-07 1:27 ` David Gibson
2021-09-22 11:23 ` Matheus K. Ferst
2021-09-22 21:10 ` Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs Daniel Henrique Barboza
2021-09-07 1:38 ` David Gibson [this message]
2021-09-23 14:39 ` Daniel Henrique Barboza
2021-09-27 5:08 ` David Gibson
2021-09-27 23:05 ` Daniel Henrique Barboza
2021-10-07 1:17 ` David Gibson
2021-09-03 20:31 ` [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG Daniel Henrique Barboza
2021-09-07 1:48 ` David Gibson
2021-09-22 11:24 ` Matheus K. Ferst
2021-09-24 14:41 ` Daniel Henrique Barboza
2021-09-24 18:34 ` Matheus K. Ferst
2021-09-24 19:05 ` Daniel Henrique Barboza
2021-09-27 5:04 ` David Gibson
2021-09-03 20:31 ` [PATCH v3 04/15] target/ppc/power8_pmu.c: enable PMC1-PMC4 events Daniel Henrique Barboza
2021-09-07 1:50 ` David Gibson
2021-09-03 20:31 ` [PATCH v3 05/15] target/ppc: PMU: add instruction counting Daniel Henrique Barboza
2021-09-07 1:57 ` David Gibson
2021-09-21 21:11 ` Daniel Henrique Barboza
2021-09-27 4:59 ` David Gibson
2021-09-03 20:31 ` [PATCH v3 06/15] target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 07/15] target/ppc/power8_pmu.c: add PMC14/PMC56 counter freeze bits Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 08/15] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
2021-09-09 11:47 ` Matheus K. Ferst
2021-09-22 19:41 ` Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 09/15] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 10/15] target/ppc/excp_helper.c: EBB handling adjustments Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 11/15] target/ppc/power8_pmu.c: enable PMC1 counter negative overflow Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 12/15] target/ppc/power8_pmu.c: cycles overflow with all PMCs Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 13/15] target/ppc: PMU: insns counter negative overflow support Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 14/15] target/ppc/translate: PMU: handle setting of PMCs while running Daniel Henrique Barboza
2021-09-03 20:31 ` [PATCH v3 15/15] target/ppc/power8_pmu.c: handle overflow bits when PMU is running Daniel Henrique Barboza
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YTbCnSz86hsUAF/+@yekko \
--to=david@gibson.dropbear.id.au \
--cc=clg@kaod.org \
--cc=danielhb413@gmail.com \
--cc=groug@kaod.org \
--cc=matheus.ferst@eldorado.org.br \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).