From: David Gibson <david@gibson.dropbear.id.au>
To: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: gustavo.romero@linaro.org, Gustavo Romero <gromero@linux.ibm.com>,
qemu-devel@nongnu.org, groug@kaod.org, qemu-ppc@nongnu.org,
clg@kaod.org
Subject: Re: [PATCH 03/19] target/ppc: add exclusive user write function for PMU regs
Date: Tue, 10 Aug 2021 13:29:23 +1000 [thread overview]
Message-ID: <YRHyk2n6xvTQ6Eyl@yekko> (raw)
In-Reply-To: <20210809131057.1694145-4-danielhb413@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9107 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-08-10 4:27 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-09 13:10 [PATCH 00/19] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2021-08-09 13:10 ` [PATCH 01/19] target/ppc: add exclusive Book3S PMU reg read/write functions Daniel Henrique Barboza
2021-08-10 3:19 ` David Gibson
2021-08-10 13:06 ` Daniel Henrique Barboza
2021-08-09 13:10 ` [PATCH 02/19] target/ppc: add exclusive user read function for PMU regs Daniel Henrique Barboza
2021-08-10 3:21 ` David Gibson
2021-08-09 13:10 ` [PATCH 03/19] target/ppc: add exclusive user write " Daniel Henrique Barboza
2021-08-10 3:29 ` David Gibson [this message]
2021-08-11 0:05 ` Richard Henderson
2021-08-09 13:10 ` [PATCH 04/19] target/ppc: PMU Book3s basic insns count for pseries TCG Daniel Henrique Barboza
2021-08-10 3:39 ` David Gibson
2021-08-10 13:24 ` Daniel Henrique Barboza
2021-08-16 17:53 ` Daniel Henrique Barboza
2021-08-17 2:59 ` David Gibson
2021-08-17 9:30 ` Daniel Henrique Barboza
2021-08-18 5:48 ` David Gibson
2021-08-11 0:26 ` Richard Henderson
2021-08-09 13:10 ` [PATCH 05/19] target/ppc/pmu_book3s_helper.c: eliminate code repetition Daniel Henrique Barboza
2021-08-10 3:40 ` David Gibson
2021-08-09 13:10 ` [PATCH 06/19] target/ppc/pmu_book3s_helper: enable PMC1-PMC4 events Daniel Henrique Barboza
2021-08-10 3:42 ` David Gibson
2021-08-10 15:03 ` Daniel Henrique Barboza
2021-08-10 23:08 ` Daniel Henrique Barboza
2021-08-11 23:27 ` Daniel Henrique Barboza
2021-08-12 1:52 ` David Gibson
2021-08-11 3:32 ` David Gibson
2021-08-09 13:10 ` [PATCH 07/19] target/ppc/pmu_book3s_helper.c: icount fine tuning Daniel Henrique Barboza
2021-08-09 13:10 ` [PATCH 08/19] target/ppc/pmu_book3s_helper.c: do an actual cycles calculation Daniel Henrique Barboza
2021-08-11 0:34 ` Richard Henderson
2021-08-09 13:10 ` [PATCH 09/19] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
2021-08-10 3:50 ` David Gibson
2021-08-10 19:32 ` Daniel Henrique Barboza
2021-08-11 0:42 ` Richard Henderson
2021-08-11 3:36 ` David Gibson
2021-08-11 0:41 ` Richard Henderson
2021-08-09 13:10 ` [PATCH 10/19] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
2021-08-10 3:55 ` David Gibson
2021-08-11 0:50 ` Richard Henderson
2021-08-09 13:10 ` [PATCH 11/19] target/ppc/excp_helper.c: POWERPC_EXCP_EBB adjustments Daniel Henrique Barboza
2021-08-09 13:10 ` [PATCH 12/19] target/ppc/pmu_book3s_helper.c: enable PMC1 counter negative EBB Daniel Henrique Barboza
2021-08-10 4:01 ` David Gibson
2021-08-10 20:26 ` Daniel Henrique Barboza
2021-08-11 3:40 ` David Gibson
2021-08-11 11:18 ` Daniel Henrique Barboza
2021-08-12 3:39 ` David Gibson
2021-08-12 4:45 ` Richard Henderson
2021-08-12 4:56 ` Richard Henderson
2021-08-12 10:17 ` Daniel Henrique Barboza
2021-08-12 21:24 ` Daniel Henrique Barboza
2021-08-13 0:35 ` Richard Henderson
2021-08-14 19:13 ` Daniel Henrique Barboza
2021-08-15 19:24 ` Richard Henderson
2021-08-09 13:10 ` [PATCH 13/19] target/ppc/translate: PMU: handle setting of PMCs while running Daniel Henrique Barboza
2021-08-10 4:06 ` David Gibson
2021-08-10 20:44 ` Daniel Henrique Barboza
2021-08-11 3:46 ` David Gibson
2021-08-09 13:10 ` [PATCH 14/19] target/ppc/pmu_book3s_helper.c: add generic timeout helpers Daniel Henrique Barboza
2021-08-10 4:09 ` David Gibson
2021-08-09 13:10 ` [PATCH 15/19] target/ppc/pmu_book3s_helper: enable counter negative for all PMCs Daniel Henrique Barboza
2021-08-10 4:11 ` David Gibson
2021-08-10 21:02 ` Daniel Henrique Barboza
2021-08-12 1:44 ` David Gibson
2021-08-09 13:10 ` [PATCH 16/19] target/ppc/pmu_book3s_helper: adding 0xFA event Daniel Henrique Barboza
2021-08-10 4:13 ` David Gibson
2021-08-09 13:10 ` [PATCH 17/19] target/ppc/pmu_book3s_helper.c: add PMC14/PMC56 counter freeze bits Daniel Henrique Barboza
2021-08-09 13:10 ` [PATCH 18/19] target/ppc/pmu_book3s_helper.c: add PM_CMPLU_STALL mock events Daniel Henrique Barboza
2021-08-10 4:17 ` David Gibson
2021-08-10 19:48 ` Daniel Henrique Barboza
2021-08-11 3:37 ` David Gibson
2021-08-09 13:10 ` [PATCH 19/19] docs/specs: add PPC64 TCG PMU-EBB documentation 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=YRHyk2n6xvTQ6Eyl@yekko \
--to=david@gibson.dropbear.id.au \
--cc=clg@kaod.org \
--cc=danielhb413@gmail.com \
--cc=gromero@linux.ibm.com \
--cc=groug@kaod.org \
--cc=gustavo.romero@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.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).