* [PATCH v2 01/16] target/ppc: add user write access control for PMU SPRs
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 4:26 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 02/16] target/ppc: add user read functions for MMCR0 and MMCR2 Daniel Henrique Barboza
` (14 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
We're going to add PMU support for TCG PPC64 chips, based on IBM POWER8+
emulation and following PowerISA v3.1.
PowerISA v3.1 defines two PMU registers groups, A and B:
- group A contains all performance monitor counters (PMCs), MMCR0, MMCR2
and MMCRA;
- group B contains MMCR1, MMCR3, SIER, SIER2, SIER3, SIAR, SDAR.
Group A have read/write non-privileged access depending on MMCR0_PMCC
bits. Group B is always userspace read only.
Userspace will require to write Group A registers, and at the same time
some Linux PMU selftests deliberately test if we are allowing write
access when we shouldn't. This patch address the access control of Group
A PMU registers by doing the following:
- add a 'pmcc_clear' flag in DisasContext. This will map whether
MMCR0_PMCC bits are cleared by checking HFLAGS_PMCCCLEAR;
- create a spr_write_PMU_groupA_ureg() that will be used to all
userspace writes of PMU regs. The reg will apply the proper access
control before forwarding execution to spr_write_ureg().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 4 ++++
target/ppc/cpu_init.c | 18 +++++++++---------
target/ppc/helper_regs.c | 3 +++
target/ppc/spr_tcg.h | 1 +
target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
5 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 500205229c..627fc8d732 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -342,6 +342,9 @@ typedef struct ppc_v3_pate_t {
#define MSR_RI 1 /* Recoverable interrupt 1 */
#define MSR_LE 0 /* Little-endian mode 1 hflags */
+/* PMU bits */
+#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
+
/* LPCR bits */
#define LPCR_VPM0 PPC_BIT(0)
#define LPCR_VPM1 PPC_BIT(1)
@@ -606,6 +609,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 66deb18a6b..c72c7fabea 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6868,7 +6868,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_ureg, SPR_NOACCESS,
+ &spr_read_ureg, &spr_write_PMU_groupA_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
@@ -6876,31 +6876,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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_USIAR, "USIAR",
@@ -6976,7 +6976,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_ureg, SPR_NOACCESS,
+ &spr_read_ureg, &spr_write_PMU_groupA_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 0be5f347d5..027ec4c3f7 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -40,6 +40,7 @@ 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_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn);
#ifndef CONFIG_USER_ONLY
void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 171b216e17..3a1eafbba8 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;
@@ -526,6 +527,41 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
}
#endif
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+/*
+ * User write function for PMU group A regs. PowerISA v3.1
+ * defines Group A sprs as:
+ *
+ * "The non-privileged read/write Performance Monitor registers
+ * (i.e., the PMCs, MMCR0, MMCR2, and MMCRA at SPR numbers
+ * 771-776, 779, 769, and 770, respectively)"
+ *
+ * These SPRs have a common user write access control via
+ * MMCR0 bits 44 and 45 (PMCC).
+ */
+void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+ /*
+ * 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."
+ */
+ if (ctx->pmcc_clear) {
+ gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
+ return;
+ }
+ spr_write_ureg(ctx, sprn, gprn);
+}
+#else
+void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_noaccess(ctx, gprn, sprn);
+}
+#endif
+
/* SPR common to all non-embedded PowerPC */
/* DECR */
#if !defined(CONFIG_USER_ONLY)
@@ -8539,6 +8575,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) {
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 01/16] target/ppc: add user write access control for PMU SPRs
2021-08-24 16:30 ` [PATCH v2 01/16] target/ppc: add user write access control for PMU SPRs Daniel Henrique Barboza
@ 2021-08-25 4:26 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2021-08-25 4:26 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, richard.henderson, qemu-devel, groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 9256 bytes --]
On Tue, Aug 24, 2021 at 01:30:17PM -0300, Daniel Henrique Barboza wrote:
> We're going to add PMU support for TCG PPC64 chips, based on IBM POWER8+
> emulation and following PowerISA v3.1.
>
> PowerISA v3.1 defines two PMU registers groups, A and B:
>
> - group A contains all performance monitor counters (PMCs), MMCR0, MMCR2
> and MMCRA;
>
> - group B contains MMCR1, MMCR3, SIER, SIER2, SIER3, SIAR, SDAR.
>
> Group A have read/write non-privileged access depending on MMCR0_PMCC
> bits. Group B is always userspace read only.
>
> Userspace will require to write Group A registers, and at the same time
> some Linux PMU selftests deliberately test if we are allowing write
> access when we shouldn't. This patch address the access control of Group
> A PMU registers by doing the following:
>
> - add a 'pmcc_clear' flag in DisasContext. This will map whether
> MMCR0_PMCC bits are cleared by checking HFLAGS_PMCCCLEAR;
>
> - create a spr_write_PMU_groupA_ureg() that will be used to all
> userspace writes of PMU regs. The reg will apply the proper access
> control before forwarding execution to spr_write_ureg().
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Hoping to get a review from Richard as well before merging.
> ---
> target/ppc/cpu.h | 4 ++++
> target/ppc/cpu_init.c | 18 +++++++++---------
> target/ppc/helper_regs.c | 3 +++
> target/ppc/spr_tcg.h | 1 +
> target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 54 insertions(+), 9 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 500205229c..627fc8d732 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -342,6 +342,9 @@ typedef struct ppc_v3_pate_t {
> #define MSR_RI 1 /* Recoverable interrupt 1 */
> #define MSR_LE 0 /* Little-endian mode 1 hflags */
>
> +/* PMU bits */
> +#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
> +
> /* LPCR bits */
> #define LPCR_VPM0 PPC_BIT(0)
> #define LPCR_VPM1 PPC_BIT(1)
> @@ -606,6 +609,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 66deb18a6b..c72c7fabea 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6868,7 +6868,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_ureg, SPR_NOACCESS,
> + &spr_read_ureg, &spr_write_PMU_groupA_ureg,
> &spr_read_ureg, &spr_write_ureg,
> 0x00000000);
> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> @@ -6876,31 +6876,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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_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_PMU_groupA_ureg,
> &spr_read_ureg, &spr_write_ureg,
> 0x00000000);
> spr_register(env, SPR_POWER_USIAR, "USIAR",
> @@ -6976,7 +6976,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_ureg, SPR_NOACCESS,
> + &spr_read_ureg, &spr_write_PMU_groupA_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 0be5f347d5..027ec4c3f7 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -40,6 +40,7 @@ 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_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn);
>
> #ifndef CONFIG_USER_ONLY
> void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 171b216e17..3a1eafbba8 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;
> @@ -526,6 +527,41 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
> }
> #endif
>
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +/*
> + * User write function for PMU group A regs. PowerISA v3.1
> + * defines Group A sprs as:
> + *
> + * "The non-privileged read/write Performance Monitor registers
> + * (i.e., the PMCs, MMCR0, MMCR2, and MMCRA at SPR numbers
> + * 771-776, 779, 769, and 770, respectively)"
> + *
> + * These SPRs have a common user write access control via
> + * MMCR0 bits 44 and 45 (PMCC).
> + */
> +void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
> +{
> + /*
> + * 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."
> + */
> + if (ctx->pmcc_clear) {
> + gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> + return;
> + }
> + spr_write_ureg(ctx, sprn, gprn);
> +}
> +#else
> +void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
> +{
> + spr_noaccess(ctx, gprn, sprn);
> +}
> +#endif
> +
> /* SPR common to all non-embedded PowerPC */
> /* DECR */
> #if !defined(CONFIG_USER_ONLY)
> @@ -8539,6 +8575,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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 02/16] target/ppc: add user read functions for MMCR0 and MMCR2
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 01/16] target/ppc: add user write access control for PMU SPRs Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 4:30 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 03/16] target/ppc: add exclusive user write function for MMCR0 Daniel Henrique Barboza
` (13 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Gustavo Romero, Daniel Henrique Barboza,
richard.henderson, groug, qemu-ppc, clg, david
From: Gustavo Romero <gromero@linux.ibm.com>
This patch adds handling of UMMCR0 and UMMCR2 user read which,
according to PowerISA 3.1, has some bits ommited to the
userspace.
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.h | 5 +++++
target/ppc/cpu_init.c | 4 ++--
target/ppc/spr_tcg.h | 2 ++
target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
4 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 627fc8d732..739005ba29 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -343,6 +343,11 @@ typedef struct ppc_v3_pate_t {
#define MSR_LE 0 /* Little-endian mode 1 hflags */
/* PMU bits */
+#define MMCR0_FC PPC_BIT(32) /* Freeze Counters */
+#define MMCR0_PMAO PPC_BIT(56) /* Perf Monitor Alert Ocurred */
+#define MMCR0_PMAE PPC_BIT(37) /* Perf Monitor Alert Enable */
+#define MMCR0_EBE PPC_BIT(43) /* Perf Monitor EBB Enable */
+#define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
/* LPCR bits */
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index c72c7fabea..5510c3799f 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6868,7 +6868,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_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_MMCR0_ureg, &spr_write_PMU_groupA_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
@@ -6976,7 +6976,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_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_MMCR2_ureg, &spr_write_PMU_groupA_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_USIER, "USIER",
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 027ec4c3f7..64ef2cd089 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -32,6 +32,8 @@ void spr_write_lr(DisasContext *ctx, int sprn, int gprn);
void spr_read_ctr(DisasContext *ctx, int gprn, int sprn);
void spr_write_ctr(DisasContext *ctx, int sprn, int gprn);
void spr_read_ureg(DisasContext *ctx, int gprn, int sprn);
+void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn);
+void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn);
void spr_read_tbl(DisasContext *ctx, int gprn, int sprn);
void spr_read_tbu(DisasContext *ctx, int gprn, int sprn);
void spr_read_atbl(DisasContext *ctx, int gprn, int sprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 3a1eafbba8..ec4160378d 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -520,6 +520,43 @@ void spr_read_ureg(DisasContext *ctx, int gprn, int sprn)
gen_load_spr(cpu_gpr[gprn], sprn + 0x10);
}
+void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
+{
+ TCGv t0 = 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.
+ */
+ gen_load_spr(t0, SPR_POWER_MMCR0);
+ tcg_gen_andi_tl(t0, t0, MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE);
+ tcg_gen_mov_tl(cpu_gpr[gprn], t0);
+
+ tcg_temp_free(t0);
+}
+
+void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn)
+{
+ TCGv t0 = tcg_temp_new();
+
+ /*
+ * On read, filter out all bits that are not FCnP0 bits.
+ * When MMCR0[PMCC] is set to 0b10 or 0b11, providing
+ * problem state programs read/write access to MMCR2,
+ * only the FCnP0 bits can be accessed. All other bits are
+ * not changed when mtspr is executed in problem state, and
+ * all other bits return 0s when mfspr is executed in problem
+ * state, according to ISA v3.1, section 10.4.6 Monitor Mode
+ * Control Register 2, p. 1316, third paragraph.
+ */
+ gen_load_spr(t0, SPR_POWER_MMCR2);
+ tcg_gen_andi_tl(t0, t0, 0x4020100804020000UL);
+ tcg_gen_mov_tl(cpu_gpr[gprn], t0);
+
+ tcg_temp_free(t0);
+}
+
#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
{
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 02/16] target/ppc: add user read functions for MMCR0 and MMCR2
2021-08-24 16:30 ` [PATCH v2 02/16] target/ppc: add user read functions for MMCR0 and MMCR2 Daniel Henrique Barboza
@ 2021-08-25 4:30 ` David Gibson
2021-08-25 12:25 ` Paul A. Clarke
0 siblings, 1 reply; 33+ messages in thread
From: David Gibson @ 2021-08-25 4:30 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, Gustavo Romero, richard.henderson, qemu-devel,
groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 5612 bytes --]
On Tue, Aug 24, 2021 at 01:30:18PM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
>
> This patch adds handling of UMMCR0 and UMMCR2 user read which,
> according to PowerISA 3.1, has some bits ommited to the
Nit: One 'm' in "omited".
> userspace.
>
> 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.h | 5 +++++
> target/ppc/cpu_init.c | 4 ++--
> target/ppc/spr_tcg.h | 2 ++
> target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 627fc8d732..739005ba29 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -343,6 +343,11 @@ typedef struct ppc_v3_pate_t {
> #define MSR_LE 0 /* Little-endian mode 1 hflags */
>
> /* PMU bits */
> +#define MMCR0_FC PPC_BIT(32) /* Freeze Counters */
> +#define MMCR0_PMAO PPC_BIT(56) /* Perf Monitor Alert Ocurred */
> +#define MMCR0_PMAE PPC_BIT(37) /* Perf Monitor Alert Enable */
> +#define MMCR0_EBE PPC_BIT(43) /* Perf Monitor EBB Enable */
> +#define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
> #define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
>
> /* LPCR bits */
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index c72c7fabea..5510c3799f 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6868,7 +6868,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_ureg, &spr_write_PMU_groupA_ureg,
> + &spr_read_MMCR0_ureg, &spr_write_PMU_groupA_ureg,
Hrm.. so combined with the previous patch this means that userspace
can write any bit in MMCR0, but only read some of them. Is that
really correct?
> &spr_read_ureg, &spr_write_ureg,
> 0x00000000);
> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> @@ -6976,7 +6976,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_ureg, &spr_write_PMU_groupA_ureg,
> + &spr_read_MMCR2_ureg, &spr_write_PMU_groupA_ureg,
> &spr_read_ureg, &spr_write_ureg,
> 0x00000000);
> spr_register(env, SPR_POWER_USIER, "USIER",
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 027ec4c3f7..64ef2cd089 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -32,6 +32,8 @@ void spr_write_lr(DisasContext *ctx, int sprn, int gprn);
> void spr_read_ctr(DisasContext *ctx, int gprn, int sprn);
> void spr_write_ctr(DisasContext *ctx, int sprn, int gprn);
> void spr_read_ureg(DisasContext *ctx, int gprn, int sprn);
> +void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn);
> +void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn);
> void spr_read_tbl(DisasContext *ctx, int gprn, int sprn);
> void spr_read_tbu(DisasContext *ctx, int gprn, int sprn);
> void spr_read_atbl(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 3a1eafbba8..ec4160378d 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -520,6 +520,43 @@ void spr_read_ureg(DisasContext *ctx, int gprn, int sprn)
> gen_load_spr(cpu_gpr[gprn], sprn + 0x10);
> }
>
> +void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> + TCGv t0 = 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.
> + */
> + gen_load_spr(t0, SPR_POWER_MMCR0);
> + tcg_gen_andi_tl(t0, t0, MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE);
I think #defining this mask somewhere would be worthwhile.
> + tcg_gen_mov_tl(cpu_gpr[gprn], t0);
> +
> + tcg_temp_free(t0);
> +}
> +
> +void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> + TCGv t0 = tcg_temp_new();
> +
> + /*
> + * On read, filter out all bits that are not FCnP0 bits.
> + * When MMCR0[PMCC] is set to 0b10 or 0b11, providing
> + * problem state programs read/write access to MMCR2,
> + * only the FCnP0 bits can be accessed. All other bits are
> + * not changed when mtspr is executed in problem state, and
> + * all other bits return 0s when mfspr is executed in problem
> + * state, according to ISA v3.1, section 10.4.6 Monitor Mode
> + * Control Register 2, p. 1316, third paragraph.
> + */
> + gen_load_spr(t0, SPR_POWER_MMCR2);
> + tcg_gen_andi_tl(t0, t0, 0x4020100804020000UL);
Even more so here, where it's a big bare literal.
> + tcg_gen_mov_tl(cpu_gpr[gprn], t0);
> +
> + tcg_temp_free(t0);
> +}
> +
> #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
> {
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread* RE: [PATCH v2 02/16] target/ppc: add user read functions for MMCR0 and MMCR2
2021-08-25 4:30 ` David Gibson
@ 2021-08-25 12:25 ` Paul A. Clarke
2021-08-25 13:35 ` David Gibson
0 siblings, 1 reply; 33+ messages in thread
From: Paul A. Clarke @ 2021-08-25 12:25 UTC (permalink / raw)
To: David Gibson
Cc: gustavo.romero, Gustavo Romero, Daniel Henrique Barboza,
richard.henderson, qemu-devel, groug, qemu-ppc, clg
On Wed, Aug 25, 2021 at 02:30:11PM +1000, David Gibson wrote:
> On Tue, Aug 24, 2021 at 01:30:18PM -0300, Daniel Henrique Barboza wrote:
> > From: Gustavo Romero <gromero@linux.ibm.com>
> >
> > This patch adds handling of UMMCR0 and UMMCR2 user read which,
> > according to PowerISA 3.1, has some bits ommited to the
>
> Nit: One 'm' in "omited".
Let's trade that extra 'm' for a 't', FTW: "omitted". :-)
PC
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 02/16] target/ppc: add user read functions for MMCR0 and MMCR2
2021-08-25 12:25 ` Paul A. Clarke
@ 2021-08-25 13:35 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2021-08-25 13:35 UTC (permalink / raw)
To: Paul A. Clarke
Cc: gustavo.romero, Gustavo Romero, Daniel Henrique Barboza,
richard.henderson, qemu-devel, groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 739 bytes --]
On Wed, Aug 25, 2021 at 07:25:10AM -0500, Paul A. Clarke wrote:
> On Wed, Aug 25, 2021 at 02:30:11PM +1000, David Gibson wrote:
> > On Tue, Aug 24, 2021 at 01:30:18PM -0300, Daniel Henrique Barboza wrote:
> > > From: Gustavo Romero <gromero@linux.ibm.com>
> > >
> > > This patch adds handling of UMMCR0 and UMMCR2 user read which,
> > > according to PowerISA 3.1, has some bits ommited to the
> >
> > Nit: One 'm' in "omited".
>
> Let's trade that extra 'm' for a 't', FTW: "omitted". :-)
Er.. yes. Muphry's law in action.
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 03/16] target/ppc: add exclusive user write function for MMCR0
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 01/16] target/ppc: add user write access control for PMU SPRs Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 02/16] target/ppc: add user read functions for MMCR0 and MMCR2 Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 4:37 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 04/16] target/ppc: PMU basic cycle count for pseries TCG Daniel Henrique Barboza
` (12 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Gustavo Romero, Daniel Henrique Barboza,
richard.henderson, groug, qemu-ppc, clg, david
From: Gustavo Romero <gromero@linux.ibm.com>
Similar to the previous patch, user write on some PowerPC
PMU regs, in this case, MMCR0, is limited. Create a new
function to handle that.
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 | 2 +-
target/ppc/spr_tcg.h | 1 +
target/ppc/translate.c | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 5510c3799f..860716da18 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6868,7 +6868,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_write_PMU_groupA_ureg,
+ &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 64ef2cd089..5c383dae3d 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -43,6 +43,7 @@ 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_PMU_groupA_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);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index ec4160378d..b48eec83e3 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -592,11 +592,49 @@ void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
}
spr_write_ureg(ctx, sprn, gprn);
}
+
+void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+ TCGv t0, t1;
+
+ /*
+ * MMCR0 is a Group A SPR. The same write access control
+ * done in spr_write_PMU_groupA_ureg() applies.
+ */
+ 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_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(SPR_POWER_MMCR0, t1);
+
+ tcg_temp_free(t0);
+ tcg_temp_free(t1);
+}
#else
void spr_write_PMU_groupA_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
/* SPR common to all non-embedded PowerPC */
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 03/16] target/ppc: add exclusive user write function for MMCR0
2021-08-24 16:30 ` [PATCH v2 03/16] target/ppc: add exclusive user write function for MMCR0 Daniel Henrique Barboza
@ 2021-08-25 4:37 ` David Gibson
2021-08-25 14:01 ` Daniel Henrique Barboza
0 siblings, 1 reply; 33+ messages in thread
From: David Gibson @ 2021-08-25 4:37 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, Gustavo Romero, richard.henderson, qemu-devel,
groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 4148 bytes --]
On Tue, Aug 24, 2021 at 01:30:19PM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
>
> Similar to the previous patch, user write on some PowerPC
> PMU regs, in this case, MMCR0, is limited. Create a new
> function to handle that.
Ok.. ok, this fixes my concern on the previous patch. Best to avoid
the confusing interim step though. I'm not sure there's a lot of use
adding the general "group A" helper if you're going to replace them
with more specific functions shortly afterwards.
>
> 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 | 2 +-
> target/ppc/spr_tcg.h | 1 +
> target/ppc/translate.c | 38 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 5510c3799f..860716da18 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6868,7 +6868,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_write_PMU_groupA_ureg,
> + &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
> &spr_read_ureg, &spr_write_ureg,
> 0x00000000);
> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 64ef2cd089..5c383dae3d 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -43,6 +43,7 @@ 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_PMU_groupA_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);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index ec4160378d..b48eec83e3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -592,11 +592,49 @@ void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
> }
> spr_write_ureg(ctx, sprn, gprn);
> }
> +
> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> +{
> + TCGv t0, t1;
> +
> + /*
> + * MMCR0 is a Group A SPR. The same write access control
> + * done in spr_write_PMU_groupA_ureg() applies.
> + */
> + 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_FC | MMCR0_PMAO | MMCR0_PMAE);
> + gen_load_spr(t1, SPR_POWER_MMCR0);
> + tcg_gen_andi_tl(t1, t1, ~(MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE));
Since you're reusing them again here, definitely want a #define for
this 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_PMU_groupA_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
>
> /* SPR common to all non-embedded PowerPC */
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 03/16] target/ppc: add exclusive user write function for MMCR0
2021-08-25 4:37 ` David Gibson
@ 2021-08-25 14:01 ` Daniel Henrique Barboza
0 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-25 14:01 UTC (permalink / raw)
To: David Gibson
Cc: gustavo.romero, Gustavo Romero, richard.henderson, qemu-devel,
groug, qemu-ppc, clg
On 8/25/21 1:37 AM, David Gibson wrote:
> On Tue, Aug 24, 2021 at 01:30:19PM -0300, Daniel Henrique Barboza wrote:
>> From: Gustavo Romero <gromero@linux.ibm.com>
>>
>> Similar to the previous patch, user write on some PowerPC
>> PMU regs, in this case, MMCR0, is limited. Create a new
>> function to handle that.
>
> Ok.. ok, this fixes my concern on the previous patch. Best to avoid
> the confusing interim step though. I'm not sure there's a lot of use
> adding the general "group A" helper if you're going to replace them
> with more specific functions shortly afterwards.
Perhaps we should merge patches 2 and 3 together. I'll also take into
consideration that patch 15 will also add custom handling of more PMU
regs and, perhaps, drop patch 1 entirely since the only control left
for groupA will be MMCRA which can either be added separately or ignored
(the kernel EBB tests doesn't bother with MMCRA permissions).
Daniel
>
>>
>> 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 | 2 +-
>> target/ppc/spr_tcg.h | 1 +
>> target/ppc/translate.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 5510c3799f..860716da18 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -6868,7 +6868,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_write_PMU_groupA_ureg,
>> + &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
>> &spr_read_ureg, &spr_write_ureg,
>> 0x00000000);
>> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
>> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
>> index 64ef2cd089..5c383dae3d 100644
>> --- a/target/ppc/spr_tcg.h
>> +++ b/target/ppc/spr_tcg.h
>> @@ -43,6 +43,7 @@ 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_PMU_groupA_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);
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index ec4160378d..b48eec83e3 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -592,11 +592,49 @@ void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
>> }
>> spr_write_ureg(ctx, sprn, gprn);
>> }
>> +
>> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>> +{
>> + TCGv t0, t1;
>> +
>> + /*
>> + * MMCR0 is a Group A SPR. The same write access control
>> + * done in spr_write_PMU_groupA_ureg() applies.
>> + */
>> + 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_FC | MMCR0_PMAO | MMCR0_PMAE);
>> + gen_load_spr(t1, SPR_POWER_MMCR0);
>> + tcg_gen_andi_tl(t1, t1, ~(MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE));
>
> Since you're reusing them again here, definitely want a #define for
> this 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_PMU_groupA_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
>>
>> /* SPR common to all non-embedded PowerPC */
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 04/16] target/ppc: PMU basic cycle count for pseries TCG
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (2 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 03/16] target/ppc: add exclusive user write function for MMCR0 Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 5:19 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 05/16] target/ppc/power8_pmu.c: enable PMC1-PMC4 events Daniel Henrique Barboza
` (11 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
This patch adds the barebones of the PMU logic by enabling cycle
counting, done via the performance monitor counter 6. The overall logic
goes as follows:
- a helper is added to control the PMU state on each MMCR0 write. This
allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
is cleared or set;
- MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
having to spin the PMU right at system init;
- the intended usage is to freeze the counters by setting MMCR0_FC, do
any additional setting of events to be counted via MMCR1 (not
implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
freeze counters to read the results - on the fly reading of the PMCs
will return the starting value of each one.
Since there will be more PMU exclusive code to be added next, put the
PMU logic in its own helper to keep all in the same place. The name of
the new helper file, power8_pmu.c, is an indicative that the PMU logic
has been tested with the IBM POWER chip family, POWER8 being the oldest
version tested. This doesn't mean that this PMU logic will break with
any other PPC64 chip that implements Book3s, but since we can't assert
that this PMU will work with all available Book3s emulated processors
we're choosing to be explicit.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 6 ++++
target/ppc/cpu_init.c | 6 ++--
target/ppc/helper.h | 1 +
target/ppc/meson.build | 1 +
target/ppc/power8_pmu.c | 74 +++++++++++++++++++++++++++++++++++++++++
target/ppc/spr_tcg.h | 1 +
target/ppc/translate.c | 17 +++++++++-
7 files changed, 102 insertions(+), 4 deletions(-)
create mode 100644 target/ppc/power8_pmu.c
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 739005ba29..6878d950de 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1176,6 +1176,12 @@ struct CPUPPCState {
uint32_t tm_vscr;
uint64_t tm_dscr;
uint64_t tm_tar;
+
+ /*
+ * PMU base time value used by the PMU to calculate
+ * running cycles.
+ */
+ uint64_t pmu_base_time;
};
#define SET_FIT_PERIOD(a_, b_, c_, d_) \
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 860716da18..71f052b052 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6821,8 +6821,8 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
{
spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
- KVM_REG_PPC_MMCR0, 0x00000000);
+ &spr_read_generic, &spr_write_MMCR0_generic,
+ KVM_REG_PPC_MMCR0, 0x80000000);
spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
SPR_NOACCESS, SPR_NOACCESS,
&spr_read_generic, &spr_write_generic,
@@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
&spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
&spr_read_ureg, &spr_write_ureg,
- 0x00000000);
+ 0x80000000);
spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
&spr_read_ureg, SPR_NOACCESS,
&spr_read_ureg, &spr_write_ureg,
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 4076aa281e..5122632784 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -20,6 +20,7 @@ DEF_HELPER_1(rfscv, void, env)
DEF_HELPER_1(hrfid, void, env)
DEF_HELPER_2(store_lpcr, void, env, tl)
DEF_HELPER_2(store_pcr, void, env, tl)
+DEF_HELPER_2(store_mmcr0, void, env, tl)
#endif
DEF_HELPER_1(check_tlb_flush_local, void, env)
DEF_HELPER_1(check_tlb_flush_global, void, env)
diff --git a/target/ppc/meson.build b/target/ppc/meson.build
index b85f295703..278ce07da9 100644
--- a/target/ppc/meson.build
+++ b/target/ppc/meson.build
@@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
'int_helper.c',
'mem_helper.c',
'misc_helper.c',
+ 'power8_pmu.c',
'timebase_helper.c',
'translate.c',
))
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
new file mode 100644
index 0000000000..47de38a99e
--- /dev/null
+++ b/target/ppc/power8_pmu.c
@@ -0,0 +1,74 @@
+/*
+ * PMU emulation helpers for TCG IBM POWER chips
+ *
+ * Copyright IBM Corp. 2021
+ *
+ * Authors:
+ * Daniel Henrique Barboza <danielhb413@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "cpu.h"
+#include "helper_regs.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+
+static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
+ uint64_t time_delta)
+{
+ /*
+ * The pseries and pvn clock runs at 1Ghz, meaning that
+ * 1 nanosec equals 1 cycle.
+ */
+ env->spr[sprn] += time_delta;
+}
+
+static void update_cycles_PMCs(CPUPPCState *env)
+{
+ uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ uint64_t time_delta = now - env->pmu_base_time;
+
+ update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
+}
+
+void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
+{
+ target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
+ bool curr_FC = curr_value & MMCR0_FC;
+ bool new_FC = value & MMCR0_FC;
+
+ env->spr[SPR_POWER_MMCR0] = value;
+
+ /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
+ if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
+ hreg_compute_hflags(env);
+ }
+
+ /*
+ * In an frozen count (FC) bit change:
+ *
+ * - if PMCs were running (curr_FC = false) and we're freezing
+ * them (new_FC = true), save the PMCs values in the registers.
+ *
+ * - if PMCs were frozen (curr_FC = true) and we're activating
+ * them (new_FC = false), set the new base_time for future cycle
+ * calculations.
+ */
+ if (curr_FC != new_FC) {
+ if (!curr_FC) {
+ update_cycles_PMCs(env);
+ } else {
+ env->pmu_base_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ }
+ }
+}
+
+#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 5c383dae3d..2c5b056fc1 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -25,6 +25,7 @@
void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
+void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn);
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 b48eec83e3..e4f75ba380 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -401,6 +401,19 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
spr_store_dump_spr(sprn);
}
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
+{
+ gen_icount_io_start(ctx);
+ gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
+}
+#else
+void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_write_generic(ctx, sprn, gprn);
+}
+#endif
+
#if !defined(CONFIG_USER_ONLY)
void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
{
@@ -609,6 +622,8 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
t0 = tcg_temp_new();
t1 = tcg_temp_new();
+ gen_icount_io_start(ctx);
+
/*
* Filter out all bits but FC, PMAO, and PMAE, according
* to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
@@ -620,7 +635,7 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
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(SPR_POWER_MMCR0, t1);
+ gen_helper_store_mmcr0(cpu_env, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 04/16] target/ppc: PMU basic cycle count for pseries TCG
2021-08-24 16:30 ` [PATCH v2 04/16] target/ppc: PMU basic cycle count for pseries TCG Daniel Henrique Barboza
@ 2021-08-25 5:19 ` David Gibson
2021-08-25 14:05 ` Daniel Henrique Barboza
0 siblings, 1 reply; 33+ messages in thread
From: David Gibson @ 2021-08-25 5:19 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, richard.henderson, qemu-devel, groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 10623 bytes --]
On Tue, Aug 24, 2021 at 01:30:20PM -0300, Daniel Henrique Barboza wrote:
> This patch adds the barebones of the PMU logic by enabling cycle
> counting, done via the performance monitor counter 6. The overall logic
> goes as follows:
>
> - a helper is added to control the PMU state on each MMCR0 write. This
> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
> is cleared or set;
>
> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
> having to spin the PMU right at system init;
>
> - the intended usage is to freeze the counters by setting MMCR0_FC, do
> any additional setting of events to be counted via MMCR1 (not
> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
> freeze counters to read the results - on the fly reading of the PMCs
> will return the starting value of each one.
Ok, I like how this is simpler than the previous version. Since qemu
is not a cycle-accurate simulator, we basically have a choice in
emulating the PMU:
1) we can maintain the illusion that the cpu clock goes at the
advertised speed w.r.t. real time
or 2) we can maintain the illusion that instructions complete roughly
as fast as we expect w.r.t. the cpu clock
We can't do both at the same time. Well... in theory we kind of could
(on a time averaged basis at least) if we decouple the guest's notion
of "real time" from actual real time. But that introduces a bunch of
other complications, so I don't think we want to go there.
Since it's simpler, I think (1) is probably the way to go, which is
what you've done here.
> Since there will be more PMU exclusive code to be added next, put the
> PMU logic in its own helper to keep all in the same place. The name of
> the new helper file, power8_pmu.c, is an indicative that the PMU logic
> has been tested with the IBM POWER chip family, POWER8 being the oldest
> version tested. This doesn't mean that this PMU logic will break with
> any other PPC64 chip that implements Book3s, but since we can't assert
> that this PMU will work with all available Book3s emulated processors
> we're choosing to be explicit.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/cpu.h | 6 ++++
> target/ppc/cpu_init.c | 6 ++--
> target/ppc/helper.h | 1 +
> target/ppc/meson.build | 1 +
> target/ppc/power8_pmu.c | 74 +++++++++++++++++++++++++++++++++++++++++
> target/ppc/spr_tcg.h | 1 +
> target/ppc/translate.c | 17 +++++++++-
> 7 files changed, 102 insertions(+), 4 deletions(-)
> create mode 100644 target/ppc/power8_pmu.c
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 739005ba29..6878d950de 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1176,6 +1176,12 @@ struct CPUPPCState {
> uint32_t tm_vscr;
> uint64_t tm_dscr;
> uint64_t tm_tar;
> +
> + /*
> + * PMU base time value used by the PMU to calculate
> + * running cycles.
> + */
> + uint64_t pmu_base_time;
> };
>
> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 860716da18..71f052b052 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6821,8 +6821,8 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
> {
> spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> - KVM_REG_PPC_MMCR0, 0x00000000);
> + &spr_read_generic, &spr_write_MMCR0_generic,
s/spr_write_MMCR0_generic/spr_write_MMCR0/
The generic refers to the fact that it's covering any register which
will just read back whatever value is written to it. Now that you're
applying MMCR0 specific logic, it's not generic any more.
> + KVM_REG_PPC_MMCR0, 0x80000000);
> spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_generic, &spr_write_generic,
> @@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
> spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
> &spr_read_ureg, &spr_write_ureg,
> - 0x00000000);
> + 0x80000000);
> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> &spr_read_ureg, SPR_NOACCESS,
> &spr_read_ureg, &spr_write_ureg,
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 4076aa281e..5122632784 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -20,6 +20,7 @@ DEF_HELPER_1(rfscv, void, env)
> DEF_HELPER_1(hrfid, void, env)
> DEF_HELPER_2(store_lpcr, void, env, tl)
> DEF_HELPER_2(store_pcr, void, env, tl)
> +DEF_HELPER_2(store_mmcr0, void, env, tl)
> #endif
> DEF_HELPER_1(check_tlb_flush_local, void, env)
> DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index b85f295703..278ce07da9 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
> 'int_helper.c',
> 'mem_helper.c',
> 'misc_helper.c',
> + 'power8_pmu.c',
> 'timebase_helper.c',
> 'translate.c',
> ))
> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
> new file mode 100644
> index 0000000000..47de38a99e
> --- /dev/null
> +++ b/target/ppc/power8_pmu.c
> @@ -0,0 +1,74 @@
> +/*
> + * PMU emulation helpers for TCG IBM POWER chips
> + *
> + * Copyright IBM Corp. 2021
> + *
> + * Authors:
> + * Daniel Henrique Barboza <danielhb413@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "cpu.h"
> +#include "helper_regs.h"
> +#include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +
> +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> + uint64_t time_delta)
> +{
> + /*
> + * The pseries and pvn clock runs at 1Ghz, meaning that
> + * 1 nanosec equals 1 cycle.
> + */
> + env->spr[sprn] += time_delta;
> +}
> +
> +static void update_cycles_PMCs(CPUPPCState *env)
> +{
> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + uint64_t time_delta = now - env->pmu_base_time;
> +
> + update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
> +}
> +
> +void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> +{
> + target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
> + bool curr_FC = curr_value & MMCR0_FC;
> + bool new_FC = value & MMCR0_FC;
> +
> + env->spr[SPR_POWER_MMCR0] = value;
> +
> + /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
> + if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
> + hreg_compute_hflags(env);
> + }
> +
> + /*
> + * In an frozen count (FC) bit change:
> + *
> + * - if PMCs were running (curr_FC = false) and we're freezing
> + * them (new_FC = true), save the PMCs values in the registers.
> + *
> + * - if PMCs were frozen (curr_FC = true) and we're activating
> + * them (new_FC = false), set the new base_time for future cycle
> + * calculations.
> + */
> + if (curr_FC != new_FC) {
> + if (!curr_FC) {
> + update_cycles_PMCs(env);
> + } else {
> + env->pmu_base_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + }
> + }
> +}
> +
> +#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 5c383dae3d..2c5b056fc1 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -25,6 +25,7 @@
> void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
> void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
> void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn);
> 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 b48eec83e3..e4f75ba380 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -401,6 +401,19 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> spr_store_dump_spr(sprn);
> }
>
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
> +{
> + gen_icount_io_start(ctx);
Since you're not using icount any more, do you still need these?
> + gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
> +}
> +#else
> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
> +{
> + spr_write_generic(ctx, sprn, gprn);
> +}
> +#endif
> +
> #if !defined(CONFIG_USER_ONLY)
> void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
> {
> @@ -609,6 +622,8 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> t0 = tcg_temp_new();
> t1 = tcg_temp_new();
>
> + gen_icount_io_start(ctx);
> +
> /*
> * Filter out all bits but FC, PMAO, and PMAE, according
> * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
> @@ -620,7 +635,7 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> 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(SPR_POWER_MMCR0, t1);
> + gen_helper_store_mmcr0(cpu_env, t1);
Do you need this change? Won't the gen_store_spr() basically do the
same thing as the gen_helpersince spr_write_MMCR0() expands to a
gen_helper anyway?
>
> tcg_temp_free(t0);
> tcg_temp_free(t1);
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 04/16] target/ppc: PMU basic cycle count for pseries TCG
2021-08-25 5:19 ` David Gibson
@ 2021-08-25 14:05 ` Daniel Henrique Barboza
0 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-25 14:05 UTC (permalink / raw)
To: David Gibson
Cc: gustavo.romero, richard.henderson, qemu-devel, groug, qemu-ppc, clg
On 8/25/21 2:19 AM, David Gibson wrote:
> On Tue, Aug 24, 2021 at 01:30:20PM -0300, Daniel Henrique Barboza wrote:
>> This patch adds the barebones of the PMU logic by enabling cycle
>> counting, done via the performance monitor counter 6. The overall logic
>> goes as follows:
>>
>> - a helper is added to control the PMU state on each MMCR0 write. This
>> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
>> is cleared or set;
>>
>> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
>> having to spin the PMU right at system init;
>>
>> - the intended usage is to freeze the counters by setting MMCR0_FC, do
>> any additional setting of events to be counted via MMCR1 (not
>> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
>> freeze counters to read the results - on the fly reading of the PMCs
>> will return the starting value of each one.
>
> Ok, I like how this is simpler than the previous version. Since qemu
> is not a cycle-accurate simulator, we basically have a choice in
> emulating the PMU:
> 1) we can maintain the illusion that the cpu clock goes at the
> advertised speed w.r.t. real time
> or 2) we can maintain the illusion that instructions complete roughly
> as fast as we expect w.r.t. the cpu clock
>
> We can't do both at the same time. Well... in theory we kind of could
> (on a time averaged basis at least) if we decouple the guest's notion
> of "real time" from actual real time. But that introduces a bunch of
> other complications, so I don't think we want to go there.
>
> Since it's simpler, I think (1) is probably the way to go, which is
> what you've done here.
>
>> Since there will be more PMU exclusive code to be added next, put the
>> PMU logic in its own helper to keep all in the same place. The name of
>> the new helper file, power8_pmu.c, is an indicative that the PMU logic
>> has been tested with the IBM POWER chip family, POWER8 being the oldest
>> version tested. This doesn't mean that this PMU logic will break with
>> any other PPC64 chip that implements Book3s, but since we can't assert
>> that this PMU will work with all available Book3s emulated processors
>> we're choosing to be explicit.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> target/ppc/cpu.h | 6 ++++
>> target/ppc/cpu_init.c | 6 ++--
>> target/ppc/helper.h | 1 +
>> target/ppc/meson.build | 1 +
>> target/ppc/power8_pmu.c | 74 +++++++++++++++++++++++++++++++++++++++++
>> target/ppc/spr_tcg.h | 1 +
>> target/ppc/translate.c | 17 +++++++++-
>> 7 files changed, 102 insertions(+), 4 deletions(-)
>> create mode 100644 target/ppc/power8_pmu.c
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 739005ba29..6878d950de 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1176,6 +1176,12 @@ struct CPUPPCState {
>> uint32_t tm_vscr;
>> uint64_t tm_dscr;
>> uint64_t tm_tar;
>> +
>> + /*
>> + * PMU base time value used by the PMU to calculate
>> + * running cycles.
>> + */
>> + uint64_t pmu_base_time;
>> };
>>
>> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 860716da18..71f052b052 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -6821,8 +6821,8 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>> {
>> spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
>> SPR_NOACCESS, SPR_NOACCESS,
>> - &spr_read_generic, &spr_write_generic,
>> - KVM_REG_PPC_MMCR0, 0x00000000);
>> + &spr_read_generic, &spr_write_MMCR0_generic,
>
> s/spr_write_MMCR0_generic/spr_write_MMCR0/
>
> The generic refers to the fact that it's covering any register which
> will just read back whatever value is written to it. Now that you're
> applying MMCR0 specific logic, it's not generic any more.
>
>> + KVM_REG_PPC_MMCR0, 0x80000000);
>> spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
>> SPR_NOACCESS, SPR_NOACCESS,
>> &spr_read_generic, &spr_write_generic,
>> @@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>> spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
>> &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
>> &spr_read_ureg, &spr_write_ureg,
>> - 0x00000000);
>> + 0x80000000);
>> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
>> &spr_read_ureg, SPR_NOACCESS,
>> &spr_read_ureg, &spr_write_ureg,
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 4076aa281e..5122632784 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -20,6 +20,7 @@ DEF_HELPER_1(rfscv, void, env)
>> DEF_HELPER_1(hrfid, void, env)
>> DEF_HELPER_2(store_lpcr, void, env, tl)
>> DEF_HELPER_2(store_pcr, void, env, tl)
>> +DEF_HELPER_2(store_mmcr0, void, env, tl)
>> #endif
>> DEF_HELPER_1(check_tlb_flush_local, void, env)
>> DEF_HELPER_1(check_tlb_flush_global, void, env)
>> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
>> index b85f295703..278ce07da9 100644
>> --- a/target/ppc/meson.build
>> +++ b/target/ppc/meson.build
>> @@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
>> 'int_helper.c',
>> 'mem_helper.c',
>> 'misc_helper.c',
>> + 'power8_pmu.c',
>> 'timebase_helper.c',
>> 'translate.c',
>> ))
>> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
>> new file mode 100644
>> index 0000000000..47de38a99e
>> --- /dev/null
>> +++ b/target/ppc/power8_pmu.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * PMU emulation helpers for TCG IBM POWER chips
>> + *
>> + * Copyright IBM Corp. 2021
>> + *
>> + * Authors:
>> + * Daniel Henrique Barboza <danielhb413@gmail.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#include "cpu.h"
>> +#include "helper_regs.h"
>> +#include "exec/exec-all.h"
>> +#include "exec/helper-proto.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/main-loop.h"
>> +
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>> +
>> +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
>> + uint64_t time_delta)
>> +{
>> + /*
>> + * The pseries and pvn clock runs at 1Ghz, meaning that
>> + * 1 nanosec equals 1 cycle.
>> + */
>> + env->spr[sprn] += time_delta;
>> +}
>> +
>> +static void update_cycles_PMCs(CPUPPCState *env)
>> +{
>> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> + uint64_t time_delta = now - env->pmu_base_time;
>> +
>> + update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
>> +}
>> +
>> +void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>> +{
>> + target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
>> + bool curr_FC = curr_value & MMCR0_FC;
>> + bool new_FC = value & MMCR0_FC;
>> +
>> + env->spr[SPR_POWER_MMCR0] = value;
>> +
>> + /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
>> + if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
>> + hreg_compute_hflags(env);
>> + }
>> +
>> + /*
>> + * In an frozen count (FC) bit change:
>> + *
>> + * - if PMCs were running (curr_FC = false) and we're freezing
>> + * them (new_FC = true), save the PMCs values in the registers.
>> + *
>> + * - if PMCs were frozen (curr_FC = true) and we're activating
>> + * them (new_FC = false), set the new base_time for future cycle
>> + * calculations.
>> + */
>> + if (curr_FC != new_FC) {
>> + if (!curr_FC) {
>> + update_cycles_PMCs(env);
>> + } else {
>> + env->pmu_base_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> + }
>> + }
>> +}
>> +
>> +#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
>> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
>> index 5c383dae3d..2c5b056fc1 100644
>> --- a/target/ppc/spr_tcg.h
>> +++ b/target/ppc/spr_tcg.h
>> @@ -25,6 +25,7 @@
>> void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
>> void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
>> void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
>> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn);
>> 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 b48eec83e3..e4f75ba380 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -401,6 +401,19 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>> spr_store_dump_spr(sprn);
>> }
>>
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
>> +{
>> + gen_icount_io_start(ctx);
>
> Since you're not using icount any more, do you still need these?
I believe we do. We're not using icount, but we should support icount guests.
And if an icount guest runs this code we'll have a 'bad icount read' error
because we're doing operations with the virtual clock.
I explained this rationale in patch 14. I should've mentioned that in this patch
instead.
>
>> + gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
>> +}
>> +#else
>> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
>> +{
>> + spr_write_generic(ctx, sprn, gprn);
>> +}
>> +#endif
>> +
>> #if !defined(CONFIG_USER_ONLY)
>> void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>> {
>> @@ -609,6 +622,8 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>> t0 = tcg_temp_new();
>> t1 = tcg_temp_new();
>>
>> + gen_icount_io_start(ctx);
>> +
>> /*
>> * Filter out all bits but FC, PMAO, and PMAE, according
>> * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
>> @@ -620,7 +635,7 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>> 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(SPR_POWER_MMCR0, t1);
>> + gen_helper_store_mmcr0(cpu_env, t1);
>
> Do you need this change? Won't the gen_store_spr() basically do the
> same thing as the gen_helpersince spr_write_MMCR0() expands to a
> gen_helper anyway?
Never realized that. I'll try it out.
Daniel
>
>>
>> tcg_temp_free(t0);
>> tcg_temp_free(t1);
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 05/16] target/ppc/power8_pmu.c: enable PMC1-PMC4 events
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (3 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 04/16] target/ppc: PMU basic cycle count for pseries TCG Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 5:23 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 06/16] target/ppc: PMU: add instruction counting Daniel Henrique Barboza
` (10 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
This patch enable all PMCs but PMC5 to count cycles. To do that we
need to implement MMCR1 bits where the event are stored, retrieve
them, see if the PMC was configured with a PM_CYC event, and
calculate cycles if that's the case.
PowerISA v3.1 defines the following conditions to count cycles:
- PMC1 set with the event 0xF0;
- PMC6, which always count cycles
However, the PowerISA also defines a range of 'implementation dependent'
events that the chip can use in the 0x01-0xBF range. Turns out that IBM
POWER chips implements some non-ISA events, and the Linux kernel makes uses
of them. For instance, 0x1E is an implementation specific event that
counts cycles in PMCs 1-4 that the kernel uses. Let's also support 0x1E
to count cycles to allow for existing kernels to behave properly with the
PMU.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 8 +++++++
target/ppc/power8_pmu.c | 53 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 6878d950de..e5df644a3c 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -350,6 +350,14 @@ typedef struct ppc_v3_pate_t {
#define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
+#define MMCR1_PMC1SEL_SHIFT (63 - 39)
+#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
+#define MMCR1_PMC2SEL_SHIFT (63 - 47)
+#define MMCR1_PMC2SEL PPC_BITMASK(40, 47)
+#define MMCR1_PMC3SEL_SHIFT (63 - 55)
+#define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
+#define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
+
/* LPCR bits */
#define LPCR_VPM0 PPC_BIT(0)
#define LPCR_VPM1 PPC_BIT(1)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 47de38a99e..007007824d 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -31,10 +31,63 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
env->spr[sprn] += time_delta;
}
+static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
+ uint64_t time_delta)
+{
+ uint8_t event;
+
+ switch (sprn) {
+ case SPR_POWER_PMC1:
+ event = MMCR1_PMC1SEL & env->spr[SPR_POWER_MMCR1];
+ event = event >> MMCR1_PMC1SEL_SHIFT;
+ break;
+ case SPR_POWER_PMC2:
+ event = MMCR1_PMC2SEL & env->spr[SPR_POWER_MMCR1];
+ event = event >> MMCR1_PMC2SEL_SHIFT;
+ break;
+ case SPR_POWER_PMC3:
+ event = MMCR1_PMC3SEL & env->spr[SPR_POWER_MMCR1];
+ event = event >> MMCR1_PMC3SEL_SHIFT;
+ break;
+ case SPR_POWER_PMC4:
+ event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
+ break;
+ default:
+ return;
+ }
+
+ /*
+ * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
+ * that counts cycles using PMC1.
+ *
+ * IBM POWER chips also has support for an implementation dependent
+ * event, 0x1E, that enables cycle counting on PMCs 1-4. The
+ * Linux kernel makes extensive use of 0x1E, so let's also support
+ * it.
+ */
+ switch (event) {
+ case 0xF0:
+ if (sprn == SPR_POWER_PMC1) {
+ update_PMC_PM_CYC(env, sprn, time_delta);
+ }
+ break;
+ case 0x1E:
+ update_PMC_PM_CYC(env, sprn, time_delta);
+ break;
+ default:
+ return;
+ }
+}
+
static void update_cycles_PMCs(CPUPPCState *env)
{
uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
uint64_t time_delta = now - env->pmu_base_time;
+ int sprn;
+
+ for (sprn = SPR_POWER_PMC1; sprn < SPR_POWER_PMC5; sprn++) {
+ update_programmable_PMC_reg(env, sprn, time_delta);
+ }
update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 05/16] target/ppc/power8_pmu.c: enable PMC1-PMC4 events
2021-08-24 16:30 ` [PATCH v2 05/16] target/ppc/power8_pmu.c: enable PMC1-PMC4 events Daniel Henrique Barboza
@ 2021-08-25 5:23 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2021-08-25 5:23 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, richard.henderson, qemu-devel, groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 4501 bytes --]
On Tue, Aug 24, 2021 at 01:30:21PM -0300, Daniel Henrique Barboza wrote:
> This patch enable all PMCs but PMC5 to count cycles. To do that we
> need to implement MMCR1 bits where the event are stored, retrieve
> them, see if the PMC was configured with a PM_CYC event, and
> calculate cycles if that's the case.
>
> PowerISA v3.1 defines the following conditions to count cycles:
>
> - PMC1 set with the event 0xF0;
> - PMC6, which always count cycles
>
> However, the PowerISA also defines a range of 'implementation dependent'
> events that the chip can use in the 0x01-0xBF range. Turns out that IBM
> POWER chips implements some non-ISA events, and the Linux kernel makes uses
> of them. For instance, 0x1E is an implementation specific event that
> counts cycles in PMCs 1-4 that the kernel uses. Let's also support 0x1E
> to count cycles to allow for existing kernels to behave properly with the
> PMU.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/cpu.h | 8 +++++++
> target/ppc/power8_pmu.c | 53 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6878d950de..e5df644a3c 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -350,6 +350,14 @@ typedef struct ppc_v3_pate_t {
> #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
> #define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
>
> +#define MMCR1_PMC1SEL_SHIFT (63 - 39)
> +#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> +#define MMCR1_PMC2SEL_SHIFT (63 - 47)
> +#define MMCR1_PMC2SEL PPC_BITMASK(40, 47)
> +#define MMCR1_PMC3SEL_SHIFT (63 - 55)
> +#define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
> +#define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
Defining macros to actually pull out the event values based on
extract64() is probably a bit more qemu-idiomatic.
> /* LPCR bits */
> #define LPCR_VPM0 PPC_BIT(0)
> #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
> index 47de38a99e..007007824d 100644
> --- a/target/ppc/power8_pmu.c
> +++ b/target/ppc/power8_pmu.c
> @@ -31,10 +31,63 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> env->spr[sprn] += time_delta;
> }
>
> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> + uint64_t time_delta)
> +{
> + uint8_t event;
> +
> + switch (sprn) {
> + case SPR_POWER_PMC1:
> + event = MMCR1_PMC1SEL & env->spr[SPR_POWER_MMCR1];
> + event = event >> MMCR1_PMC1SEL_SHIFT;
> + break;
> + case SPR_POWER_PMC2:
> + event = MMCR1_PMC2SEL & env->spr[SPR_POWER_MMCR1];
> + event = event >> MMCR1_PMC2SEL_SHIFT;
> + break;
> + case SPR_POWER_PMC3:
> + event = MMCR1_PMC3SEL & env->spr[SPR_POWER_MMCR1];
> + event = event >> MMCR1_PMC3SEL_SHIFT;
> + break;
> + case SPR_POWER_PMC4:
> + event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
> + break;
> + default:
> + return;
> + }
> +
> + /*
> + * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
> + * that counts cycles using PMC1.
> + *
> + * IBM POWER chips also has support for an implementation dependent
> + * event, 0x1E, that enables cycle counting on PMCs 1-4. The
> + * Linux kernel makes extensive use of 0x1E, so let's also support
> + * it.
> + */
> + switch (event) {
> + case 0xF0:
> + if (sprn == SPR_POWER_PMC1) {
> + update_PMC_PM_CYC(env, sprn, time_delta);
> + }
> + break;
> + case 0x1E:
> + update_PMC_PM_CYC(env, sprn, time_delta);
> + break;
> + default:
> + return;
> + }
> +}
> +
> static void update_cycles_PMCs(CPUPPCState *env)
> {
> uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> uint64_t time_delta = now - env->pmu_base_time;
> + int sprn;
> +
> + for (sprn = SPR_POWER_PMC1; sprn < SPR_POWER_PMC5; sprn++) {
> + update_programmable_PMC_reg(env, sprn, time_delta);
> + }
>
> update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
> }
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 06/16] target/ppc: PMU: add instruction counting
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (4 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 05/16] target/ppc/power8_pmu.c: enable PMC1-PMC4 events Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 5:31 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 07/16] target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event Daniel Henrique Barboza
` (9 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
The PMU is already counting cycles by calculating time elapsed in
nanoseconds. Counting instructions is a different matter and requires
another approach.
This patch adds the capability of counting completed instructions
(Perf event PM_INST_CMPL) by counting the amount of instructions
translated in each translation block right before exiting it.
A new pmu_count_insns() helper in translation.c was added to do that.
After verifying that the PMU is running (MMCR0_FC bit not set), we
call helper_insns_inc(). This is new helper from power8_pmu.c that
will add the instructions to the relevant counters.
At this moment helper_insns_inc() is just summing instructions to
counters, but it will also trigger counter negative overflow in a
later patch.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/helper.h | 1 +
target/ppc/power8_pmu.c | 61 ++++++++++++++++++++++++++++++++++++++---
target/ppc/translate.c | 46 +++++++++++++++++++++++++++++++
3 files changed, 104 insertions(+), 4 deletions(-)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 5122632784..47dbbe6da1 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -21,6 +21,7 @@ DEF_HELPER_1(hrfid, void, env)
DEF_HELPER_2(store_lpcr, void, env, tl)
DEF_HELPER_2(store_pcr, void, env, tl)
DEF_HELPER_2(store_mmcr0, void, env, tl)
+DEF_HELPER_2(insns_inc, void, env, i32)
#endif
DEF_HELPER_1(check_tlb_flush_local, void, env)
DEF_HELPER_1(check_tlb_flush_global, void, env)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 007007824d..311eaa358f 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -31,10 +31,9 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
env->spr[sprn] += time_delta;
}
-static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
- uint64_t time_delta)
+static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
{
- uint8_t event;
+ int event = 0x0;
switch (sprn) {
case SPR_POWER_PMC1:
@@ -53,9 +52,17 @@ static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
break;
default:
- return;
+ break;
}
+ return event;
+}
+
+static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
+ uint64_t time_delta)
+{
+ uint8_t event = get_PMC_event(env, sprn);
+
/*
* MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
* that counts cycles using PMC1.
@@ -124,4 +131,50 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
}
}
+static bool pmc_counting_insns(CPUPPCState *env, int sprn)
+{
+ bool ret = false;
+ uint8_t event;
+
+ if (sprn == SPR_POWER_PMC5) {
+ return true;
+ }
+
+ event = get_PMC_event(env, sprn);
+
+ /*
+ * Event 0x2 is an implementation-dependent event that IBM
+ * POWER chips implement (at least since POWER8) that is
+ * equivalent to PM_INST_CMPL. Let's support this event on
+ * all programmable PMCs.
+ *
+ * Event 0xFE is the PowerISA v3.1 architected event to
+ * sample PM_INST_CMPL using PMC1.
+ */
+ switch (sprn) {
+ case SPR_POWER_PMC1:
+ return event == 0x2 || event == 0xFE;
+ case SPR_POWER_PMC2:
+ case SPR_POWER_PMC3:
+ case SPR_POWER_PMC4:
+ return event == 0x2;
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+/* This helper assumes that the PMC is running. */
+void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
+{
+ int sprn;
+
+ for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
+ if (pmc_counting_insns(env, sprn)) {
+ env->spr[sprn] += num_insns;
+ }
+ }
+}
+
#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index e4f75ba380..d45ce79a3e 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4425,6 +4425,30 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
#endif
}
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+static void pmu_count_insns(DisasContext *ctx)
+{
+ TCGv t_mmcr0FC = tcg_constant_i64(MMCR0_FC);
+ TCGv t0 = tcg_temp_new();
+ TCGLabel *l_exit = gen_new_label();
+
+ /* Do not bother calling the helper if the PMU is frozen */
+ gen_load_spr(t0, SPR_POWER_MMCR0);
+ tcg_gen_andi_tl(t0, t0, MMCR0_FC);
+ tcg_gen_brcond_tl(TCG_COND_EQ, t0, t_mmcr0FC, l_exit);
+
+ gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
+
+ gen_set_label(l_exit);
+ tcg_temp_free(t_mmcr0FC);
+ tcg_temp_free(t0);
+}
+#else
+static void pmu_count_insns(DisasContext *ctx)
+{
+ return;
+}
+#endif
static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
{
return translator_use_goto_tb(&ctx->base, dest);
@@ -4439,9 +4463,17 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
} else if (sse & (CPU_SINGLE_STEP | CPU_BRANCH_STEP)) {
gen_helper_raise_exception(cpu_env, tcg_constant_i32(gen_prep_dbgex(ctx)));
} else {
+ pmu_count_insns(ctx);
tcg_gen_exit_tb(NULL, 0);
}
} else {
+ /*
+ * tcg_gen_lookup_and_goto_ptr will exit the TB if
+ * CF_NO_GOTO_PTR is set. Count insns now.
+ */
+ if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
+ pmu_count_insns(ctx);
+ }
tcg_gen_lookup_and_goto_ptr();
}
}
@@ -4453,6 +4485,8 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
dest = (uint32_t) dest;
}
if (use_goto_tb(ctx, dest)) {
+ pmu_count_insns(ctx);
+
tcg_gen_goto_tb(n);
tcg_gen_movi_tl(cpu_nip, dest & ~3);
tcg_gen_exit_tb(ctx->base.tb, n);
@@ -8785,6 +8819,8 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
switch (is_jmp) {
case DISAS_TOO_MANY:
if (use_goto_tb(ctx, nip)) {
+ pmu_count_insns(ctx);
+
tcg_gen_goto_tb(0);
gen_update_nip(ctx, nip);
tcg_gen_exit_tb(ctx->base.tb, 0);
@@ -8795,6 +8831,14 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
gen_update_nip(ctx, nip);
/* fall through */
case DISAS_CHAIN:
+ /*
+ * tcg_gen_lookup_and_goto_ptr will exit the TB if
+ * CF_NO_GOTO_PTR is set. Count insns now.
+ */
+ if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
+ pmu_count_insns(ctx);
+ }
+
tcg_gen_lookup_and_goto_ptr();
break;
@@ -8802,6 +8846,8 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
gen_update_nip(ctx, nip);
/* fall through */
case DISAS_EXIT:
+ pmu_count_insns(ctx);
+
tcg_gen_exit_tb(NULL, 0);
break;
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 06/16] target/ppc: PMU: add instruction counting
2021-08-24 16:30 ` [PATCH v2 06/16] target/ppc: PMU: add instruction counting Daniel Henrique Barboza
@ 2021-08-25 5:31 ` David Gibson
2021-08-25 14:10 ` Daniel Henrique Barboza
0 siblings, 1 reply; 33+ messages in thread
From: David Gibson @ 2021-08-25 5:31 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, richard.henderson, qemu-devel, groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 8334 bytes --]
On Tue, Aug 24, 2021 at 01:30:22PM -0300, Daniel Henrique Barboza wrote:
> The PMU is already counting cycles by calculating time elapsed in
> nanoseconds. Counting instructions is a different matter and requires
> another approach.
>
> This patch adds the capability of counting completed instructions
> (Perf event PM_INST_CMPL) by counting the amount of instructions
> translated in each translation block right before exiting it.
>
> A new pmu_count_insns() helper in translation.c was added to do that.
> After verifying that the PMU is running (MMCR0_FC bit not set), we
> call helper_insns_inc(). This is new helper from power8_pmu.c that
> will add the instructions to the relevant counters.
>
> At this moment helper_insns_inc() is just summing instructions to
> counters, but it will also trigger counter negative overflow in a
> later patch.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/helper.h | 1 +
> target/ppc/power8_pmu.c | 61 ++++++++++++++++++++++++++++++++++++++---
> target/ppc/translate.c | 46 +++++++++++++++++++++++++++++++
> 3 files changed, 104 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 5122632784..47dbbe6da1 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -21,6 +21,7 @@ DEF_HELPER_1(hrfid, void, env)
> DEF_HELPER_2(store_lpcr, void, env, tl)
> DEF_HELPER_2(store_pcr, void, env, tl)
> DEF_HELPER_2(store_mmcr0, void, env, tl)
> +DEF_HELPER_2(insns_inc, void, env, i32)
> #endif
> DEF_HELPER_1(check_tlb_flush_local, void, env)
> DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
> index 007007824d..311eaa358f 100644
> --- a/target/ppc/power8_pmu.c
> +++ b/target/ppc/power8_pmu.c
> @@ -31,10 +31,9 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> env->spr[sprn] += time_delta;
> }
>
> -static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> - uint64_t time_delta)
> +static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
> {
> - uint8_t event;
> + int event = 0x0;
>
> switch (sprn) {
> case SPR_POWER_PMC1:
> @@ -53,9 +52,17 @@ static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
> break;
> default:
> - return;
> + break;
> }
>
> + return event;
> +}
> +
> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> + uint64_t time_delta)
> +{
> + uint8_t event = get_PMC_event(env, sprn);
> +
> /*
> * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
> * that counts cycles using PMC1.
> @@ -124,4 +131,50 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> }
> }
>
> +static bool pmc_counting_insns(CPUPPCState *env, int sprn)
> +{
> + bool ret = false;
> + uint8_t event;
> +
> + if (sprn == SPR_POWER_PMC5) {
> + return true;
> + }
> +
> + event = get_PMC_event(env, sprn);
> +
> + /*
> + * Event 0x2 is an implementation-dependent event that IBM
> + * POWER chips implement (at least since POWER8) that is
> + * equivalent to PM_INST_CMPL. Let's support this event on
> + * all programmable PMCs.
> + *
> + * Event 0xFE is the PowerISA v3.1 architected event to
> + * sample PM_INST_CMPL using PMC1.
> + */
> + switch (sprn) {
> + case SPR_POWER_PMC1:
> + return event == 0x2 || event == 0xFE;
> + case SPR_POWER_PMC2:
> + case SPR_POWER_PMC3:
> + case SPR_POWER_PMC4:
> + return event == 0x2;
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +/* This helper assumes that the PMC is running. */
> +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> +{
> + int sprn;
> +
> + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> + if (pmc_counting_insns(env, sprn)) {
> + env->spr[sprn] += num_insns;
> + }
> + }
> +}
> +
> #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index e4f75ba380..d45ce79a3e 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4425,6 +4425,30 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
> #endif
> }
>
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +static void pmu_count_insns(DisasContext *ctx)
> +{
> + TCGv t_mmcr0FC = tcg_constant_i64(MMCR0_FC);
> + TCGv t0 = tcg_temp_new();
> + TCGLabel *l_exit = gen_new_label();
> +
> + /* Do not bother calling the helper if the PMU is frozen */
> + gen_load_spr(t0, SPR_POWER_MMCR0);
> + tcg_gen_andi_tl(t0, t0, MMCR0_FC);
> + tcg_gen_brcond_tl(TCG_COND_EQ, t0, t_mmcr0FC, l_exit);
> +
> + gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
> +
> + gen_set_label(l_exit);
> + tcg_temp_free(t_mmcr0FC);
> + tcg_temp_free(t0);
> +}
> +#else
> +static void pmu_count_insns(DisasContext *ctx)
> +{
> + return;
> +}
> +#endif
> static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
> {
> return translator_use_goto_tb(&ctx->base, dest);
> @@ -4439,9 +4463,17 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
> } else if (sse & (CPU_SINGLE_STEP | CPU_BRANCH_STEP)) {
> gen_helper_raise_exception(cpu_env, tcg_constant_i32(gen_prep_dbgex(ctx)));
> } else {
> + pmu_count_insns(ctx);
> tcg_gen_exit_tb(NULL, 0);
> }
> } else {
> + /*
> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
> + * CF_NO_GOTO_PTR is set. Count insns now.
> + */
> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
> + pmu_count_insns(ctx);
> + }
Oof. IIUC this will at least generate the instructions to read MMCR0
and check FC, all the time, even if we haven't touched the PMU. That
sounds like it could be pretty expensive. Couldn't we instead
determine if we're counting instructions when we generate the context,
then only generate the code to bump the counter if that's set.
Obviously changes to the MMCRs would need to force a new translation
block.
> tcg_gen_lookup_and_goto_ptr();
> }
> }
> @@ -4453,6 +4485,8 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
> dest = (uint32_t) dest;
> }
> if (use_goto_tb(ctx, dest)) {
> + pmu_count_insns(ctx);
> +
> tcg_gen_goto_tb(n);
> tcg_gen_movi_tl(cpu_nip, dest & ~3);
> tcg_gen_exit_tb(ctx->base.tb, n);
> @@ -8785,6 +8819,8 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> switch (is_jmp) {
> case DISAS_TOO_MANY:
> if (use_goto_tb(ctx, nip)) {
> + pmu_count_insns(ctx);
> +
> tcg_gen_goto_tb(0);
> gen_update_nip(ctx, nip);
> tcg_gen_exit_tb(ctx->base.tb, 0);
> @@ -8795,6 +8831,14 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> gen_update_nip(ctx, nip);
> /* fall through */
> case DISAS_CHAIN:
> + /*
> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
> + * CF_NO_GOTO_PTR is set. Count insns now.
> + */
> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
> + pmu_count_insns(ctx);
> + }
> +
> tcg_gen_lookup_and_goto_ptr();
> break;
>
> @@ -8802,6 +8846,8 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> gen_update_nip(ctx, nip);
> /* fall through */
> case DISAS_EXIT:
> + pmu_count_insns(ctx);
> +
> tcg_gen_exit_tb(NULL, 0);
> break;
>
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 06/16] target/ppc: PMU: add instruction counting
2021-08-25 5:31 ` David Gibson
@ 2021-08-25 14:10 ` Daniel Henrique Barboza
0 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-25 14:10 UTC (permalink / raw)
To: David Gibson
Cc: gustavo.romero, richard.henderson, qemu-devel, groug, qemu-ppc, clg
On 8/25/21 2:31 AM, David Gibson wrote:
> On Tue, Aug 24, 2021 at 01:30:22PM -0300, Daniel Henrique Barboza wrote:
>> The PMU is already counting cycles by calculating time elapsed in
>> nanoseconds. Counting instructions is a different matter and requires
>> another approach.
>>
>> This patch adds the capability of counting completed instructions
>> (Perf event PM_INST_CMPL) by counting the amount of instructions
>> translated in each translation block right before exiting it.
>>
>> A new pmu_count_insns() helper in translation.c was added to do that.
>> After verifying that the PMU is running (MMCR0_FC bit not set), we
>> call helper_insns_inc(). This is new helper from power8_pmu.c that
>> will add the instructions to the relevant counters.
>>
>> At this moment helper_insns_inc() is just summing instructions to
>> counters, but it will also trigger counter negative overflow in a
>> later patch.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> target/ppc/helper.h | 1 +
>> target/ppc/power8_pmu.c | 61 ++++++++++++++++++++++++++++++++++++++---
>> target/ppc/translate.c | 46 +++++++++++++++++++++++++++++++
>> 3 files changed, 104 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 5122632784..47dbbe6da1 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -21,6 +21,7 @@ DEF_HELPER_1(hrfid, void, env)
>> DEF_HELPER_2(store_lpcr, void, env, tl)
>> DEF_HELPER_2(store_pcr, void, env, tl)
>> DEF_HELPER_2(store_mmcr0, void, env, tl)
>> +DEF_HELPER_2(insns_inc, void, env, i32)
>> #endif
>> DEF_HELPER_1(check_tlb_flush_local, void, env)
>> DEF_HELPER_1(check_tlb_flush_global, void, env)
>> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
>> index 007007824d..311eaa358f 100644
>> --- a/target/ppc/power8_pmu.c
>> +++ b/target/ppc/power8_pmu.c
>> @@ -31,10 +31,9 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
>> env->spr[sprn] += time_delta;
>> }
>>
>> -static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>> - uint64_t time_delta)
>> +static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
>> {
>> - uint8_t event;
>> + int event = 0x0;
>>
>> switch (sprn) {
>> case SPR_POWER_PMC1:
>> @@ -53,9 +52,17 @@ static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>> event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
>> break;
>> default:
>> - return;
>> + break;
>> }
>>
>> + return event;
>> +}
>> +
>> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>> + uint64_t time_delta)
>> +{
>> + uint8_t event = get_PMC_event(env, sprn);
>> +
>> /*
>> * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
>> * that counts cycles using PMC1.
>> @@ -124,4 +131,50 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>> }
>> }
>>
>> +static bool pmc_counting_insns(CPUPPCState *env, int sprn)
>> +{
>> + bool ret = false;
>> + uint8_t event;
>> +
>> + if (sprn == SPR_POWER_PMC5) {
>> + return true;
>> + }
>> +
>> + event = get_PMC_event(env, sprn);
>> +
>> + /*
>> + * Event 0x2 is an implementation-dependent event that IBM
>> + * POWER chips implement (at least since POWER8) that is
>> + * equivalent to PM_INST_CMPL. Let's support this event on
>> + * all programmable PMCs.
>> + *
>> + * Event 0xFE is the PowerISA v3.1 architected event to
>> + * sample PM_INST_CMPL using PMC1.
>> + */
>> + switch (sprn) {
>> + case SPR_POWER_PMC1:
>> + return event == 0x2 || event == 0xFE;
>> + case SPR_POWER_PMC2:
>> + case SPR_POWER_PMC3:
>> + case SPR_POWER_PMC4:
>> + return event == 0x2;
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/* This helper assumes that the PMC is running. */
>> +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
>> +{
>> + int sprn;
>> +
>> + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
>> + if (pmc_counting_insns(env, sprn)) {
>> + env->spr[sprn] += num_insns;
>> + }
>> + }
>> +}
>> +
>> #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index e4f75ba380..d45ce79a3e 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -4425,6 +4425,30 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
>> #endif
>> }
>>
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>> +static void pmu_count_insns(DisasContext *ctx)
>> +{
>> + TCGv t_mmcr0FC = tcg_constant_i64(MMCR0_FC);
>> + TCGv t0 = tcg_temp_new();
>> + TCGLabel *l_exit = gen_new_label();
>> +
>> + /* Do not bother calling the helper if the PMU is frozen */
>> + gen_load_spr(t0, SPR_POWER_MMCR0);
>> + tcg_gen_andi_tl(t0, t0, MMCR0_FC);
>> + tcg_gen_brcond_tl(TCG_COND_EQ, t0, t_mmcr0FC, l_exit);
>> +
>> + gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
>> +
>> + gen_set_label(l_exit);
>> + tcg_temp_free(t_mmcr0FC);
>> + tcg_temp_free(t0);
>> +}
>> +#else
>> +static void pmu_count_insns(DisasContext *ctx)
>> +{
>> + return;
>> +}
>> +#endif
>> static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
>> {
>> return translator_use_goto_tb(&ctx->base, dest);
>> @@ -4439,9 +4463,17 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
>> } else if (sse & (CPU_SINGLE_STEP | CPU_BRANCH_STEP)) {
>> gen_helper_raise_exception(cpu_env, tcg_constant_i32(gen_prep_dbgex(ctx)));
>> } else {
>> + pmu_count_insns(ctx);
>> tcg_gen_exit_tb(NULL, 0);
>> }
>> } else {
>> + /*
>> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
>> + * CF_NO_GOTO_PTR is set. Count insns now.
>> + */
>> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
>> + pmu_count_insns(ctx);
>> + }
>
> Oof. IIUC this will at least generate the instructions to read MMCR0
> and check FC, all the time, even if we haven't touched the PMU. That
> sounds like it could be pretty expensive. Couldn't we instead
> determine if we're counting instructions when we generate the context,
> then only generate the code to bump the counter if that's set.
> Obviously changes to the MMCRs would need to force a new translation
> block.
We can add MMCR0_FC to hflags and check for ctx->mmcr0_fc.
I'm not sure about how to force a new translation block. Is it done via
ctx->base.is_jmp = DISAS_EXIT_UPDATE?
Daniel
>
>> tcg_gen_lookup_and_goto_ptr();
>> }
>> }
>> @@ -4453,6 +4485,8 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>> dest = (uint32_t) dest;
>> }
>> if (use_goto_tb(ctx, dest)) {
>> + pmu_count_insns(ctx);
>> +
>> tcg_gen_goto_tb(n);
>> tcg_gen_movi_tl(cpu_nip, dest & ~3);
>> tcg_gen_exit_tb(ctx->base.tb, n);
>> @@ -8785,6 +8819,8 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>> switch (is_jmp) {
>> case DISAS_TOO_MANY:
>> if (use_goto_tb(ctx, nip)) {
>> + pmu_count_insns(ctx);
>> +
>> tcg_gen_goto_tb(0);
>> gen_update_nip(ctx, nip);
>> tcg_gen_exit_tb(ctx->base.tb, 0);
>> @@ -8795,6 +8831,14 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>> gen_update_nip(ctx, nip);
>> /* fall through */
>> case DISAS_CHAIN:
>> + /*
>> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
>> + * CF_NO_GOTO_PTR is set. Count insns now.
>> + */
>> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
>> + pmu_count_insns(ctx);
>> + }
>> +
>> tcg_gen_lookup_and_goto_ptr();
>> break;
>>
>> @@ -8802,6 +8846,8 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>> gen_update_nip(ctx, nip);
>> /* fall through */
>> case DISAS_EXIT:
>> + pmu_count_insns(ctx);
>> +
>> tcg_gen_exit_tb(NULL, 0);
>> break;
>>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 07/16] target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (5 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 06/16] target/ppc: PMU: add instruction counting Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 5:32 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 08/16] target/ppc/power8_pmu.c: add PMC14/PMC56 counter freeze bits Daniel Henrique Barboza
` (8 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
PM_RUN_INST_CMPL, instructions completed with the run latch set, is
the architected PowerISA v3.1 event defined with PMC4SEL = 0xFA.
Implement it by checking for the CTRL RUN bit before incrementing the
counter.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 3 +++
target/ppc/power8_pmu.c | 25 ++++++++++++++++++++-----
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e5df644a3c..60e5e1159a 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -358,6 +358,9 @@ typedef struct ppc_v3_pate_t {
#define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
#define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
+/* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
+#define CTRL_RUN PPC_BIT(63)
+
/* LPCR bits */
#define LPCR_VPM0 PPC_BIT(0)
#define LPCR_VPM1 PPC_BIT(1)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 311eaa358f..9154fca5fd 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -131,10 +131,10 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
}
}
-static bool pmc_counting_insns(CPUPPCState *env, int sprn)
+static bool pmc_counting_insns(CPUPPCState *env, int sprn,
+ uint8_t event)
{
bool ret = false;
- uint8_t event;
if (sprn == SPR_POWER_PMC5) {
return true;
@@ -156,8 +156,15 @@ static bool pmc_counting_insns(CPUPPCState *env, int sprn)
return event == 0x2 || event == 0xFE;
case SPR_POWER_PMC2:
case SPR_POWER_PMC3:
- case SPR_POWER_PMC4:
return event == 0x2;
+ case SPR_POWER_PMC4:
+ /*
+ * Event 0xFA is the "instructions completed with run latch
+ * set" event. Consider it as instruction counting event.
+ * The caller is responsible for handling it separately
+ * from PM_INST_CMPL.
+ */
+ return event == 0x2 || event == 0xFA;
default:
break;
}
@@ -171,8 +178,16 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
int sprn;
for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
- if (pmc_counting_insns(env, sprn)) {
- env->spr[sprn] += num_insns;
+ uint8_t event = get_PMC_event(env, sprn);
+
+ if (pmc_counting_insns(env, sprn, event)) {
+ if (sprn == SPR_POWER_PMC4 && event == 0xFA) {
+ if (env->spr[SPR_CTRL] & CTRL_RUN) {
+ env->spr[SPR_POWER_PMC4] += num_insns;
+ }
+ } else {
+ env->spr[sprn] += num_insns;
+ }
}
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 07/16] target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event
2021-08-24 16:30 ` [PATCH v2 07/16] target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event Daniel Henrique Barboza
@ 2021-08-25 5:32 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2021-08-25 5:32 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, richard.henderson, qemu-devel, groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 3268 bytes --]
On Tue, Aug 24, 2021 at 01:30:23PM -0300, Daniel Henrique Barboza wrote:
> PM_RUN_INST_CMPL, instructions completed with the run latch set, is
> the architected PowerISA v3.1 event defined with PMC4SEL = 0xFA.
>
> Implement it by checking for the CTRL RUN bit before incrementing the
> counter.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/cpu.h | 3 +++
> target/ppc/power8_pmu.c | 25 ++++++++++++++++++++-----
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e5df644a3c..60e5e1159a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -358,6 +358,9 @@ typedef struct ppc_v3_pate_t {
> #define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
> #define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
>
> +/* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
> +#define CTRL_RUN PPC_BIT(63)
> +
> /* LPCR bits */
> #define LPCR_VPM0 PPC_BIT(0)
> #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
> index 311eaa358f..9154fca5fd 100644
> --- a/target/ppc/power8_pmu.c
> +++ b/target/ppc/power8_pmu.c
> @@ -131,10 +131,10 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> }
> }
>
> -static bool pmc_counting_insns(CPUPPCState *env, int sprn)
> +static bool pmc_counting_insns(CPUPPCState *env, int sprn,
> + uint8_t event)
> {
> bool ret = false;
> - uint8_t event;
>
> if (sprn == SPR_POWER_PMC5) {
> return true;
> @@ -156,8 +156,15 @@ static bool pmc_counting_insns(CPUPPCState *env, int sprn)
> return event == 0x2 || event == 0xFE;
> case SPR_POWER_PMC2:
> case SPR_POWER_PMC3:
> - case SPR_POWER_PMC4:
> return event == 0x2;
> + case SPR_POWER_PMC4:
> + /*
> + * Event 0xFA is the "instructions completed with run latch
> + * set" event. Consider it as instruction counting event.
> + * The caller is responsible for handling it separately
> + * from PM_INST_CMPL.
> + */
> + return event == 0x2 || event == 0xFA;
> default:
> break;
> }
> @@ -171,8 +178,16 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> int sprn;
>
> for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> - if (pmc_counting_insns(env, sprn)) {
> - env->spr[sprn] += num_insns;
> + uint8_t event = get_PMC_event(env, sprn);
> +
> + if (pmc_counting_insns(env, sprn, event)) {
> + if (sprn == SPR_POWER_PMC4 && event == 0xFA) {
> + if (env->spr[SPR_CTRL] & CTRL_RUN) {
> + env->spr[SPR_POWER_PMC4] += num_insns;
This is only correct if changes to CTRL force a new translation
block. Is that true at the moment?
> + }
> + } else {
> + env->spr[sprn] += num_insns;
> + }
> }
> }
> }
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 08/16] target/ppc/power8_pmu.c: add PMC14/PMC56 counter freeze bits
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (6 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 07/16] target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 09/16] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
` (7 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
We're missing two counter freeze bits that are used to further control
how the PMCs behaves: MMCR0_FC14 and MMCR0_FC56. These bits can frozen
PMCs separately: MMCR0_FC14 freezes PMCs 1 to 4 and MMCR0_FC56 freezes
PMCs 5 and 6.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 2 ++
target/ppc/power8_pmu.c | 25 ++++++++++++++++++++++---
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 60e5e1159a..105ee75a01 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -349,6 +349,8 @@ typedef struct ppc_v3_pate_t {
#define MMCR0_EBE PPC_BIT(43) /* Perf Monitor EBB Enable */
#define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
+#define MMCR0_FC14 PPC_BIT(58)
+#define MMCR0_FC56 PPC_BIT(59)
#define MMCR1_PMC1SEL_SHIFT (63 - 39)
#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 9154fca5fd..4545fe7810 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -58,6 +58,15 @@ static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
return event;
}
+static bool pmc_is_running(CPUPPCState *env, int sprn)
+{
+ if (sprn < SPR_POWER_PMC5) {
+ return !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC14);
+ }
+
+ return !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC56);
+}
+
static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
uint64_t time_delta)
{
@@ -90,13 +99,19 @@ static void update_cycles_PMCs(CPUPPCState *env)
{
uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
uint64_t time_delta = now - env->pmu_base_time;
+ bool PMC14_running = !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC14);
+ bool PMC6_running = !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC56);
int sprn;
- for (sprn = SPR_POWER_PMC1; sprn < SPR_POWER_PMC5; sprn++) {
- update_programmable_PMC_reg(env, sprn, time_delta);
+ if (PMC14_running) {
+ for (sprn = SPR_POWER_PMC1; sprn < SPR_POWER_PMC5; sprn++) {
+ update_programmable_PMC_reg(env, sprn, time_delta);
+ }
}
- update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
+ if (PMC6_running) {
+ update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
+ }
}
void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
@@ -136,6 +151,10 @@ static bool pmc_counting_insns(CPUPPCState *env, int sprn,
{
bool ret = false;
+ if (!pmc_is_running(env, sprn)) {
+ return false;
+ }
+
if (sprn == SPR_POWER_PMC5) {
return true;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v2 09/16] PPC64/TCG: Implement 'rfebb' instruction
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (7 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 08/16] target/ppc/power8_pmu.c: add PMC14/PMC56 counter freeze bits Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-30 12:12 ` Matheus K. Ferst
2021-08-24 16:30 ` [PATCH v2 10/16] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
` (6 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
An Event-Based Branch (EBB) allows applications to change the NIA when a
event-based exception occurs. Event-based exceptions are enabled by
setting the Branch Event Status and Control Register (BESCR). If the
event-based exception is enabled when the exception occurs, an EBB
happens.
The EBB will:
- set the Global Enable (GE) bit of BESCR to 0;
- set bits 0-61 of the Event-Based Branch Return Register (EBBRR) to the
effective address of the NIA that would have executed if the EBB
didn't happen;
- Instruction fetch and execution will continue in the effective address
contained in the Event-Based Branch Handler Register (EBBHR).
The EBB Handler will process the event and then execute the Return From
Event-Based Branch (rfebb) instruction. rfebb sets BESCR_GE and then
redirects execution to the address pointed in EBBRR. This process is
described in the PowerISA v3.1, Book II, Chapter 6 [1].
This patch implements the rfebb instruction. Descriptions of all
relevant BESCR bits are also added - this patch is only using BESCR_GE,
but the next patches will use the remaining bits.
[1] https://wiki.raptorcs.com/w/images/f/f5/PowerISA_public.v3.1.pdf
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 13 +++++++++++
target/ppc/excp_helper.c | 24 +++++++++++++++++++
target/ppc/helper.h | 1 +
target/ppc/insn32.decode | 5 ++++
target/ppc/translate.c | 2 ++
target/ppc/translate/branch-impl.c.inc | 32 ++++++++++++++++++++++++++
6 files changed, 77 insertions(+)
create mode 100644 target/ppc/translate/branch-impl.c.inc
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 105ee75a01..9d5eb9ead4 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -363,6 +363,19 @@ typedef struct ppc_v3_pate_t {
/* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
#define CTRL_RUN PPC_BIT(63)
+/* EBB/BESCR bits */
+/* Global Enable */
+#define BESCR_GE PPC_BIT(0)
+/* External Event-based Exception Enable */
+#define BESCR_EE PPC_BIT(30)
+/* Performance Monitor Event-based Exception Enable */
+#define BESCR_PME PPC_BIT(31)
+/* External Event-based Exception Occurred */
+#define BESCR_EEO PPC_BIT(62)
+/* Performance Monitor Event-based Exception Occurred */
+#define BESCR_PMEO PPC_BIT(63)
+#define BESCR_INVALID PPC_BITMASK(32, 33)
+
/* LPCR bits */
#define LPCR_VPM0 PPC_BIT(0)
#define LPCR_VPM1 PPC_BIT(1)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 7b6ac16eef..058b063d8a 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1281,6 +1281,30 @@ void helper_hrfid(CPUPPCState *env)
}
#endif
+#ifdef CONFIG_TCG
+void helper_rfebb(CPUPPCState *env, target_ulong s)
+{
+ /*
+ * Handling of BESCR bits 32:33 according to PowerISA v3.1:
+ *
+ * "If BESCR 32:33 != 0b00 the instruction is treated as if
+ * the instruction form were invalid."
+ */
+ if (env->spr[SPR_BESCR] & BESCR_INVALID) {
+ raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+ POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
+ }
+
+ env->nip = env->spr[SPR_EBBRR];
+
+ if (s) {
+ env->spr[SPR_BESCR] |= BESCR_GE;
+ } else {
+ env->spr[SPR_BESCR] &= ~BESCR_GE;
+ }
+}
+#endif
+
/*****************************************************************************/
/* Embedded PowerPC specific helpers */
void helper_40x_rfci(CPUPPCState *env)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 47dbbe6da1..91a86992a5 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -18,6 +18,7 @@ DEF_HELPER_2(pminsn, void, env, i32)
DEF_HELPER_1(rfid, void, env)
DEF_HELPER_1(rfscv, void, env)
DEF_HELPER_1(hrfid, void, env)
+DEF_HELPER_2(rfebb, void, env, tl)
DEF_HELPER_2(store_lpcr, void, env, tl)
DEF_HELPER_2(store_pcr, void, env, tl)
DEF_HELPER_2(store_mmcr0, void, env, tl)
diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index 9fd8d6b817..bd0b88b0b6 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -124,3 +124,8 @@ SETNBCR 011111 ..... ..... ----- 0111100000 - @X_bi
## Vector Bit Manipulation Instruction
VCFUGED 000100 ..... ..... ..... 10101001101 @VX
+
+### rfebb
+&RFEBB s:uint8_t
+@RFEBB ......-------------- s:1 .......... - &RFEBB
+RFEBB 010011-------------- . 0010010010 - @RFEBB
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index d45ce79a3e..d4cfc567cf 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7643,6 +7643,8 @@ static int times_4(DisasContext *ctx, int x)
#include "translate/spe-impl.c.inc"
+#include "translate/branch-impl.c.inc"
+
/* Handles lfdp, lxsd, lxssp */
static void gen_dform39(DisasContext *ctx)
{
diff --git a/target/ppc/translate/branch-impl.c.inc b/target/ppc/translate/branch-impl.c.inc
new file mode 100644
index 0000000000..2e309e9889
--- /dev/null
+++ b/target/ppc/translate/branch-impl.c.inc
@@ -0,0 +1,32 @@
+/*
+ * Power ISA decode for branch instructions
+ *
+ * Copyright IBM Corp. 2021
+ *
+ * Authors:
+ * Daniel Henrique Barboza <danielhb413@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+
+static bool trans_RFEBB(DisasContext *ctx, arg_RFEBB *arg)
+{
+ REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
+
+ gen_icount_io_start(ctx);
+ gen_update_cfar(ctx, ctx->cia);
+ gen_helper_rfebb(cpu_env, cpu_gpr[arg->s]);
+
+ ctx->base.is_jmp = DISAS_CHAIN;
+
+ return true;
+}
+#else
+static bool trans_RFEBB(DisasContext *ctx, arg_RFEBB *arg)
+{
+ return true;
+}
+#endif
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 09/16] PPC64/TCG: Implement 'rfebb' instruction
2021-08-24 16:30 ` [PATCH v2 09/16] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
@ 2021-08-30 12:12 ` Matheus K. Ferst
2021-08-30 18:41 ` Daniel Henrique Barboza
0 siblings, 1 reply; 33+ messages in thread
From: Matheus K. Ferst @ 2021-08-30 12:12 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel
Cc: gustavo.romero, richard.henderson, groug, qemu-ppc, clg, david
On 24/08/2021 13:30, Daniel Henrique Barboza wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>
> An Event-Based Branch (EBB) allows applications to change the NIA when a
> event-based exception occurs. Event-based exceptions are enabled by
> setting the Branch Event Status and Control Register (BESCR). If the
> event-based exception is enabled when the exception occurs, an EBB
> happens.
>
> The EBB will:
>
> - set the Global Enable (GE) bit of BESCR to 0;
> - set bits 0-61 of the Event-Based Branch Return Register (EBBRR) to the
> effective address of the NIA that would have executed if the EBB
> didn't happen;
> - Instruction fetch and execution will continue in the effective address
> contained in the Event-Based Branch Handler Register (EBBHR).
>
> The EBB Handler will process the event and then execute the Return From
> Event-Based Branch (rfebb) instruction. rfebb sets BESCR_GE and then
> redirects execution to the address pointed in EBBRR. This process is
> described in the PowerISA v3.1, Book II, Chapter 6 [1].
>
> This patch implements the rfebb instruction. Descriptions of all
> relevant BESCR bits are also added - this patch is only using BESCR_GE,
> but the next patches will use the remaining bits.
>
> [1] https://wiki.raptorcs.com/w/images/f/f5/PowerISA_public.v3.1.pdf
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/cpu.h | 13 +++++++++++
> target/ppc/excp_helper.c | 24 +++++++++++++++++++
> target/ppc/helper.h | 1 +
> target/ppc/insn32.decode | 5 ++++
> target/ppc/translate.c | 2 ++
> target/ppc/translate/branch-impl.c.inc | 32 ++++++++++++++++++++++++++
> 6 files changed, 77 insertions(+)
> create mode 100644 target/ppc/translate/branch-impl.c.inc
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 105ee75a01..9d5eb9ead4 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -363,6 +363,19 @@ typedef struct ppc_v3_pate_t {
> /* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
> #define CTRL_RUN PPC_BIT(63)
>
> +/* EBB/BESCR bits */
> +/* Global Enable */
> +#define BESCR_GE PPC_BIT(0)
> +/* External Event-based Exception Enable */
> +#define BESCR_EE PPC_BIT(30)
> +/* Performance Monitor Event-based Exception Enable */
> +#define BESCR_PME PPC_BIT(31)
> +/* External Event-based Exception Occurred */
> +#define BESCR_EEO PPC_BIT(62)
> +/* Performance Monitor Event-based Exception Occurred */
> +#define BESCR_PMEO PPC_BIT(63)
> +#define BESCR_INVALID PPC_BITMASK(32, 33)
> +
> /* LPCR bits */
> #define LPCR_VPM0 PPC_BIT(0)
> #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 7b6ac16eef..058b063d8a 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1281,6 +1281,30 @@ void helper_hrfid(CPUPPCState *env)
> }
> #endif
>
> +#ifdef CONFIG_TCG
> +void helper_rfebb(CPUPPCState *env, target_ulong s)
> +{
> + /*
> + * Handling of BESCR bits 32:33 according to PowerISA v3.1:
> + *
> + * "If BESCR 32:33 != 0b00 the instruction is treated as if
> + * the instruction form were invalid."
> + */
> + if (env->spr[SPR_BESCR] & BESCR_INVALID) {
> + raise_exception_err(env, POWERPC_EXCP_PROGRAM,
> + POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
> + }
> +
> + env->nip = env->spr[SPR_EBBRR];
Hi Daniel,
Should we check msr_is_64bit and crop EBBRR here? Also, I believe this
helper should be inside a #if defined(TARGET_PPC64) ...
> +
> + if (s) {
> + env->spr[SPR_BESCR] |= BESCR_GE;
> + } else {
> + env->spr[SPR_BESCR] &= ~BESCR_GE;
> + }
> +}
> +#endif
> +
> /*****************************************************************************/
> /* Embedded PowerPC specific helpers */
> void helper_40x_rfci(CPUPPCState *env)
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 47dbbe6da1..91a86992a5 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -18,6 +18,7 @@ DEF_HELPER_2(pminsn, void, env, i32)
> DEF_HELPER_1(rfid, void, env)
> DEF_HELPER_1(rfscv, void, env)
> DEF_HELPER_1(hrfid, void, env)
> +DEF_HELPER_2(rfebb, void, env, tl)
... as its DEF_HELPER is.
> DEF_HELPER_2(store_lpcr, void, env, tl)
> DEF_HELPER_2(store_pcr, void, env, tl)
> DEF_HELPER_2(store_mmcr0, void, env, tl)
> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
> index 9fd8d6b817..bd0b88b0b6 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -124,3 +124,8 @@ SETNBCR 011111 ..... ..... ----- 0111100000 - @X_bi
> ## Vector Bit Manipulation Instruction
>
> VCFUGED 000100 ..... ..... ..... 10101001101 @VX
> +
> +### rfebb
> +&RFEBB s:uint8_t
> +@RFEBB ......-------------- s:1 .......... - &RFEBB
> +RFEBB 010011-------------- . 0010010010 - @RFEBB
Maybe it only makes sense with Book I instructions, but we've been
naming fmt_def/arg_def according to the instruction format, so I'd
suggest naming it @XL/&XL or @XL_s/&XL_s.
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index d45ce79a3e..d4cfc567cf 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7643,6 +7643,8 @@ static int times_4(DisasContext *ctx, int x)
>
> #include "translate/spe-impl.c.inc"
>
> +#include "translate/branch-impl.c.inc"
> +
> /* Handles lfdp, lxsd, lxssp */
> static void gen_dform39(DisasContext *ctx)
> {
> diff --git a/target/ppc/translate/branch-impl.c.inc b/target/ppc/translate/branch-impl.c.inc
> new file mode 100644
> index 0000000000..2e309e9889
> --- /dev/null
> +++ b/target/ppc/translate/branch-impl.c.inc
> @@ -0,0 +1,32 @@
> +/*
> + * Power ISA decode for branch instructions
> + *
> + * Copyright IBM Corp. 2021
> + *
> + * Authors:
> + * Daniel Henrique Barboza <danielhb413@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +
> +static bool trans_RFEBB(DisasContext *ctx, arg_RFEBB *arg)
> +{
> + REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
> +
> + gen_icount_io_start(ctx);
> + gen_update_cfar(ctx, ctx->cia);
> + gen_helper_rfebb(cpu_env, cpu_gpr[arg->s]);
> +
> + ctx->base.is_jmp = DISAS_CHAIN;
> +
> + return true;
> +}
> +#else
> +static bool trans_RFEBB(DisasContext *ctx, arg_RFEBB *arg)
> +{
> + return true;
That would be a no-op for !TARGET_PPC64, I think it's more appropriate
to call gen_invalid(ctx) or let REQUIRE_INSNS_FLAGS2 handle this. I'm
not sure what should be done in the CONFIG_USER_ONLY case.
> +}
> +#endif
> --
> 2.31.1
>
>
--
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 09/16] PPC64/TCG: Implement 'rfebb' instruction
2021-08-30 12:12 ` Matheus K. Ferst
@ 2021-08-30 18:41 ` Daniel Henrique Barboza
0 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-30 18:41 UTC (permalink / raw)
To: Matheus K. Ferst, qemu-devel
Cc: gustavo.romero, richard.henderson, groug, qemu-ppc, clg, david
On 8/30/21 9:12 AM, Matheus K. Ferst wrote:
> On 24/08/2021 13:30, Daniel Henrique Barboza wrote:
>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>>
>> An Event-Based Branch (EBB) allows applications to change the NIA when a
>> event-based exception occurs. Event-based exceptions are enabled by
>> setting the Branch Event Status and Control Register (BESCR). If the
>> event-based exception is enabled when the exception occurs, an EBB
>> happens.
>>
>> The EBB will:
>>
>> - set the Global Enable (GE) bit of BESCR to 0;
>> - set bits 0-61 of the Event-Based Branch Return Register (EBBRR) to the
>> effective address of the NIA that would have executed if the EBB
>> didn't happen;
>> - Instruction fetch and execution will continue in the effective address
>> contained in the Event-Based Branch Handler Register (EBBHR).
>>
>> The EBB Handler will process the event and then execute the Return From
>> Event-Based Branch (rfebb) instruction. rfebb sets BESCR_GE and then
>> redirects execution to the address pointed in EBBRR. This process is
>> described in the PowerISA v3.1, Book II, Chapter 6 [1].
>>
>> This patch implements the rfebb instruction. Descriptions of all
>> relevant BESCR bits are also added - this patch is only using BESCR_GE,
>> but the next patches will use the remaining bits.
>>
>> [1] https://wiki.raptorcs.com/w/images/f/f5/PowerISA_public.v3.1.pdf
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> target/ppc/cpu.h | 13 +++++++++++
>> target/ppc/excp_helper.c | 24 +++++++++++++++++++
>> target/ppc/helper.h | 1 +
>> target/ppc/insn32.decode | 5 ++++
>> target/ppc/translate.c | 2 ++
>> target/ppc/translate/branch-impl.c.inc | 32 ++++++++++++++++++++++++++
>> 6 files changed, 77 insertions(+)
>> create mode 100644 target/ppc/translate/branch-impl.c.inc
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 105ee75a01..9d5eb9ead4 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -363,6 +363,19 @@ typedef struct ppc_v3_pate_t {
>> /* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
>> #define CTRL_RUN PPC_BIT(63)
>>
>> +/* EBB/BESCR bits */
>> +/* Global Enable */
>> +#define BESCR_GE PPC_BIT(0)
>> +/* External Event-based Exception Enable */
>> +#define BESCR_EE PPC_BIT(30)
>> +/* Performance Monitor Event-based Exception Enable */
>> +#define BESCR_PME PPC_BIT(31)
>> +/* External Event-based Exception Occurred */
>> +#define BESCR_EEO PPC_BIT(62)
>> +/* Performance Monitor Event-based Exception Occurred */
>> +#define BESCR_PMEO PPC_BIT(63)
>> +#define BESCR_INVALID PPC_BITMASK(32, 33)
>> +
>> /* LPCR bits */
>> #define LPCR_VPM0 PPC_BIT(0)
>> #define LPCR_VPM1 PPC_BIT(1)
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 7b6ac16eef..058b063d8a 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -1281,6 +1281,30 @@ void helper_hrfid(CPUPPCState *env)
>> }
>> #endif
>>
>> +#ifdef CONFIG_TCG
>> +void helper_rfebb(CPUPPCState *env, target_ulong s)
>> +{
>> + /*
>> + * Handling of BESCR bits 32:33 according to PowerISA v3.1:
>> + *
>> + * "If BESCR 32:33 != 0b00 the instruction is treated as if
>> + * the instruction form were invalid."
>> + */
>> + if (env->spr[SPR_BESCR] & BESCR_INVALID) {
>> + raise_exception_err(env, POWERPC_EXCP_PROGRAM,
>> + POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
>> + }
>> +
>> + env->nip = env->spr[SPR_EBBRR];
>
> Hi Daniel,
>
> Should we check msr_is_64bit and crop EBBRR here? Also, I believe this helper should be inside a #if defined(TARGET_PPC64) ...
Good catch. Yes, we need. PowerISA mentions:
"If there are no pending event-based exceptions, then
the next instruction is fetched from the address
EBBRR 0:61 || 0b00 (when MSR SF =1) or 32 0 ||
EBBRR 32:61 || 0b00 (when MSR SF =0)."
I'll fix it in the next spin, together with the 'if defined(TARGET_PPC64)' you also mentioned.
>
>> +
>> + if (s) {
>> + env->spr[SPR_BESCR] |= BESCR_GE;
>> + } else {
>> + env->spr[SPR_BESCR] &= ~BESCR_GE;
>> + }
>> +}
>> +#endif
>> +
>> /*****************************************************************************/
>> /* Embedded PowerPC specific helpers */
>> void helper_40x_rfci(CPUPPCState *env)
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 47dbbe6da1..91a86992a5 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -18,6 +18,7 @@ DEF_HELPER_2(pminsn, void, env, i32)
>> DEF_HELPER_1(rfid, void, env)
>> DEF_HELPER_1(rfscv, void, env)
>> DEF_HELPER_1(hrfid, void, env)
>> +DEF_HELPER_2(rfebb, void, env, tl)
>
> ... as its DEF_HELPER is.
>
>> DEF_HELPER_2(store_lpcr, void, env, tl)
>> DEF_HELPER_2(store_pcr, void, env, tl)
>> DEF_HELPER_2(store_mmcr0, void, env, tl)
>> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
>> index 9fd8d6b817..bd0b88b0b6 100644
>> --- a/target/ppc/insn32.decode
>> +++ b/target/ppc/insn32.decode
>> @@ -124,3 +124,8 @@ SETNBCR 011111 ..... ..... ----- 0111100000 - @X_bi
>> ## Vector Bit Manipulation Instruction
>>
>> VCFUGED 000100 ..... ..... ..... 10101001101 @VX
>> +
>> +### rfebb
>> +&RFEBB s:uint8_t
>> +@RFEBB ......-------------- s:1 .......... - &RFEBB
>> +RFEBB 010011-------------- . 0010010010 - @RFEBB
>
> Maybe it only makes sense with Book I instructions, but we've been naming fmt_def/arg_def according to the instruction format, so I'd suggest naming it @XL/&XL or @XL_s/&XL_s.
Alright.
>
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index d45ce79a3e..d4cfc567cf 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -7643,6 +7643,8 @@ static int times_4(DisasContext *ctx, int x)
>>
>> #include "translate/spe-impl.c.inc"
>>
>> +#include "translate/branch-impl.c.inc"
>> +
>> /* Handles lfdp, lxsd, lxssp */
>> static void gen_dform39(DisasContext *ctx)
>> {
>> diff --git a/target/ppc/translate/branch-impl.c.inc b/target/ppc/translate/branch-impl.c.inc
>> new file mode 100644
>> index 0000000000..2e309e9889
>> --- /dev/null
>> +++ b/target/ppc/translate/branch-impl.c.inc
>> @@ -0,0 +1,32 @@
>> +/*
>> + * Power ISA decode for branch instructions
>> + *
>> + * Copyright IBM Corp. 2021
>> + *
>> + * Authors:
>> + * Daniel Henrique Barboza <danielhb413@gmail.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>> +
>> +static bool trans_RFEBB(DisasContext *ctx, arg_RFEBB *arg)
>> +{
>> + REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
>> +
>> + gen_icount_io_start(ctx);
>> + gen_update_cfar(ctx, ctx->cia);
>> + gen_helper_rfebb(cpu_env, cpu_gpr[arg->s]);
>> +
>> + ctx->base.is_jmp = DISAS_CHAIN;
>> +
>> + return true;
>> +}
>> +#else
>> +static bool trans_RFEBB(DisasContext *ctx, arg_RFEBB *arg)
>> +{
>> + return true;
>
> That would be a no-op for !TARGET_PPC64, I think it's more appropriate to call gen_invalid(ctx) or let REQUIRE_INSNS_FLAGS2 handle this. I'm not sure what should be done in the CONFIG_USER_ONLY case.
I'll use 'gen_invalid(ctx)' since this is what is being done in fixedpoint-impl.c.inc
(albeit for a different reason I guess).
Thanks,
Daniel
>
>> +}
>> +#endif
>> --
>> 2.31.1
>>
>>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 10/16] target/ppc: PMU Event-Based exception support
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (8 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 09/16] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-25 5:37 ` David Gibson
2021-08-24 16:30 ` [PATCH v2 11/16] target/ppc/excp_helper.c: EBB handling adjustments Daniel Henrique Barboza
` (5 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Gustavo Romero, Daniel Henrique Barboza,
richard.henderson, groug, qemu-ppc, clg, david
From: Gustavo Romero <gromero@linux.ibm.com>
Following up the rfebb implementation, this patch adds the EBB exception
support that are triggered by Performance Monitor alerts. This exception
occurs when an enabled PMU condition or event happens and both MMCR0_EBE
and BESCR_PME are set.
The supported PM alerts will consist of counter negative conditions of
the PMU counters. This will be achieved by a timer mechanism that will
predict when a counter becomes negative. The PMU timer callback will set
the appropriate bits in MMCR0 and fire a PMC interrupt. The EBB
exception code will then set the appropriate BESCR bits, set the next
instruction pointer to the address pointed by the return register
(SPR_EBBRR), and redirect execution to the handler (pointed by
SPR_EBBHR).
This patch sets the basic structure of interrupts and timers. The
following patches will add the counter negative logic for the registers.
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>
---
hw/ppc/spapr_cpu_core.c | 6 ++++++
target/ppc/cpu.h | 12 +++++++++++-
target/ppc/excp_helper.c | 28 +++++++++++++++++++++++++++
target/ppc/power8_pmu.c | 41 ++++++++++++++++++++++++++++++++++++++++
target/ppc/power8_pmu.h | 25 ++++++++++++++++++++++++
5 files changed, 111 insertions(+), 1 deletion(-)
create mode 100644 target/ppc/power8_pmu.h
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 4f316a6f9d..c7a342c4aa 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -20,6 +20,7 @@
#include "target/ppc/kvm_ppc.h"
#include "hw/ppc/ppc.h"
#include "target/ppc/mmu-hash64.h"
+#include "target/ppc/power8_pmu.h"
#include "sysemu/numa.h"
#include "sysemu/reset.h"
#include "sysemu/hw_accel.h"
@@ -266,6 +267,11 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
return false;
}
+ /* Init PMU interrupt timer (TCG only) */
+ if (!kvm_enabled()) {
+ cpu_ppc_pmu_timer_init(env);
+ }
+
if (!sc->pre_3_0_migration) {
vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
cpu->machine_data);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 9d5eb9ead4..535754ddff 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -129,8 +129,9 @@ enum {
/* ISA 3.00 additions */
POWERPC_EXCP_HVIRT = 101,
POWERPC_EXCP_SYSCALL_VECTORED = 102, /* scv exception */
+ POWERPC_EXCP_EBB = 103, /* Event-based branch exception */
/* EOL */
- POWERPC_EXCP_NB = 103,
+ POWERPC_EXCP_NB = 104,
/* QEMU exceptions: special cases we want to stop translation */
POWERPC_EXCP_SYSCALL_USER = 0x203, /* System call in user mode only */
};
@@ -1047,6 +1048,8 @@ struct ppc_radix_page_info {
#define PPC_CPU_OPCODES_LEN 0x40
#define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
+#define PMU_TIMERS_LEN 5
+
struct CPUPPCState {
/* Most commonly used resources during translated code execution first */
target_ulong gpr[32]; /* general purpose registers */
@@ -1208,6 +1211,12 @@ struct CPUPPCState {
* running cycles.
*/
uint64_t pmu_base_time;
+
+ /*
+ * Timers used to fire performance monitor alerts and
+ * interrupts. All PMCs but PMC5 has a timer.
+ */
+ QEMUTimer *pmu_intr_timers[PMU_TIMERS_LEN];
};
#define SET_FIT_PERIOD(a_, b_, c_, d_) \
@@ -2424,6 +2433,7 @@ enum {
PPC_INTERRUPT_HMI, /* Hypervisor Maintenance interrupt */
PPC_INTERRUPT_HDOORBELL, /* Hypervisor Doorbell interrupt */
PPC_INTERRUPT_HVIRT, /* Hypervisor virtualization interrupt */
+ PPC_INTERRUPT_PMC, /* Performance Monitor Counter interrupt */
};
/* Processor Compatibility mask (PCR) */
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 058b063d8a..e97898c5f4 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -821,6 +821,22 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
cpu_abort(cs, "Non maskable external exception "
"is not implemented yet !\n");
break;
+ case POWERPC_EXCP_EBB: /* Event-based branch exception */
+ if ((env->spr[SPR_BESCR] & BESCR_GE) &&
+ (env->spr[SPR_BESCR] & BESCR_PME)) {
+ target_ulong nip;
+
+ env->spr[SPR_BESCR] &= ~BESCR_GE; /* Clear GE */
+ env->spr[SPR_BESCR] |= BESCR_PMEO; /* Set PMEO */
+ env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
+ nip = env->spr[SPR_EBBHR]; /* EBB handler */
+ powerpc_set_excp_state(cpu, nip, env->msr);
+ }
+ /*
+ * This interrupt is handled by userspace. No need
+ * to proceed.
+ */
+ return;
default:
excp_invalid:
cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
@@ -1068,6 +1084,18 @@ static void ppc_hw_interrupt(CPUPPCState *env)
powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_THERM);
return;
}
+ /* PMC -> Event-based branch exception */
+ if (env->pending_interrupts & (1 << PPC_INTERRUPT_PMC)) {
+ /*
+ * Performance Monitor event-based exception can only
+ * occur in problem state.
+ */
+ if (msr_pr == 1) {
+ env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PMC);
+ powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_EBB);
+ return;
+ }
+ }
}
if (env->resume_as_sreset) {
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 4545fe7810..a57b602125 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -12,12 +12,14 @@
#include "qemu/osdep.h"
+#include "power8_pmu.h"
#include "cpu.h"
#include "helper_regs.h"
#include "exec/exec-all.h"
#include "exec/helper-proto.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
+#include "hw/ppc/ppc.h"
#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
@@ -114,6 +116,45 @@ static void update_cycles_PMCs(CPUPPCState *env)
}
}
+static void cpu_ppc_pmu_timer_cb(void *opaque)
+{
+ PowerPCCPU *cpu = opaque;
+ CPUPPCState *env = &cpu->env;
+ uint64_t mmcr0;
+
+ mmcr0 = env->spr[SPR_POWER_MMCR0];
+ if (env->spr[SPR_POWER_MMCR0] & MMCR0_EBE) {
+ /* freeeze counters if needed */
+ if (mmcr0 & MMCR0_FCECE) {
+ mmcr0 &= ~MMCR0_FCECE;
+ mmcr0 |= MMCR0_FC;
+ }
+
+ /* Clear PMAE and set PMAO */
+ if (mmcr0 & MMCR0_PMAE) {
+ mmcr0 &= ~MMCR0_PMAE;
+ mmcr0 |= MMCR0_PMAO;
+ }
+ env->spr[SPR_POWER_MMCR0] = mmcr0;
+
+ /* Fire the PMC hardware exception */
+ ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
+ }
+}
+
+void cpu_ppc_pmu_timer_init(CPUPPCState *env)
+{
+ PowerPCCPU *cpu = env_archcpu(env);
+ QEMUTimer *timer;
+ int i;
+
+ for (i = 0; i < PMU_TIMERS_LEN; i++) {
+ timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_pmu_timer_cb,
+ cpu);
+ env->pmu_intr_timers[i] = timer;
+ }
+}
+
void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
{
target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
diff --git a/target/ppc/power8_pmu.h b/target/ppc/power8_pmu.h
new file mode 100644
index 0000000000..34a9d0e8a2
--- /dev/null
+++ b/target/ppc/power8_pmu.h
@@ -0,0 +1,25 @@
+/*
+ * PMU emulation helpers for TCG IBM POWER chips
+ *
+ * Copyright IBM Corp. 2021
+ *
+ * Authors:
+ * Daniel Henrique Barboza <danielhb413@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef PMU_BOOK3S_HELPER
+#define PMU_BOOK3S_HELPER
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+
+void cpu_ppc_pmu_timer_init(CPUPPCState *env);
+
+#endif
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 10/16] target/ppc: PMU Event-Based exception support
2021-08-24 16:30 ` [PATCH v2 10/16] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
@ 2021-08-25 5:37 ` David Gibson
2021-08-30 19:09 ` Daniel Henrique Barboza
0 siblings, 1 reply; 33+ messages in thread
From: David Gibson @ 2021-08-25 5:37 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: gustavo.romero, Gustavo Romero, richard.henderson, qemu-devel,
groug, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 9580 bytes --]
On Tue, Aug 24, 2021 at 01:30:26PM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
>
> Following up the rfebb implementation, this patch adds the EBB exception
> support that are triggered by Performance Monitor alerts. This exception
> occurs when an enabled PMU condition or event happens and both MMCR0_EBE
> and BESCR_PME are set.
>
> The supported PM alerts will consist of counter negative conditions of
> the PMU counters. This will be achieved by a timer mechanism that will
> predict when a counter becomes negative. The PMU timer callback will set
> the appropriate bits in MMCR0 and fire a PMC interrupt. The EBB
> exception code will then set the appropriate BESCR bits, set the next
> instruction pointer to the address pointed by the return register
> (SPR_EBBRR), and redirect execution to the handler (pointed by
> SPR_EBBHR).
>
> This patch sets the basic structure of interrupts and timers. The
> following patches will add the counter negative logic for the
> registers.
A timer makes sense for tripping cycles based EBB events. But for
instructions based EBB events, shouldn't you instead have a test in
the update instructions path which trips the event if you've passed
the magic number?
>
> 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>
> ---
> hw/ppc/spapr_cpu_core.c | 6 ++++++
> target/ppc/cpu.h | 12 +++++++++++-
> target/ppc/excp_helper.c | 28 +++++++++++++++++++++++++++
> target/ppc/power8_pmu.c | 41 ++++++++++++++++++++++++++++++++++++++++
> target/ppc/power8_pmu.h | 25 ++++++++++++++++++++++++
> 5 files changed, 111 insertions(+), 1 deletion(-)
> create mode 100644 target/ppc/power8_pmu.h
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4f316a6f9d..c7a342c4aa 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,7 @@
> #include "target/ppc/kvm_ppc.h"
> #include "hw/ppc/ppc.h"
> #include "target/ppc/mmu-hash64.h"
> +#include "target/ppc/power8_pmu.h"
> #include "sysemu/numa.h"
> #include "sysemu/reset.h"
> #include "sysemu/hw_accel.h"
> @@ -266,6 +267,11 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
> return false;
> }
>
> + /* Init PMU interrupt timer (TCG only) */
> + if (!kvm_enabled()) {
> + cpu_ppc_pmu_timer_init(env);
> + }
> +
> if (!sc->pre_3_0_migration) {
> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> cpu->machine_data);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 9d5eb9ead4..535754ddff 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -129,8 +129,9 @@ enum {
> /* ISA 3.00 additions */
> POWERPC_EXCP_HVIRT = 101,
> POWERPC_EXCP_SYSCALL_VECTORED = 102, /* scv exception */
> + POWERPC_EXCP_EBB = 103, /* Event-based branch exception */
> /* EOL */
> - POWERPC_EXCP_NB = 103,
> + POWERPC_EXCP_NB = 104,
> /* QEMU exceptions: special cases we want to stop translation */
> POWERPC_EXCP_SYSCALL_USER = 0x203, /* System call in user mode only */
> };
> @@ -1047,6 +1048,8 @@ struct ppc_radix_page_info {
> #define PPC_CPU_OPCODES_LEN 0x40
> #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
>
> +#define PMU_TIMERS_LEN 5
> +
> struct CPUPPCState {
> /* Most commonly used resources during translated code execution first */
> target_ulong gpr[32]; /* general purpose registers */
> @@ -1208,6 +1211,12 @@ struct CPUPPCState {
> * running cycles.
> */
> uint64_t pmu_base_time;
> +
> + /*
> + * Timers used to fire performance monitor alerts and
> + * interrupts. All PMCs but PMC5 has a timer.
> + */
> + QEMUTimer *pmu_intr_timers[PMU_TIMERS_LEN];
> };
>
> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
> @@ -2424,6 +2433,7 @@ enum {
> PPC_INTERRUPT_HMI, /* Hypervisor Maintenance interrupt */
> PPC_INTERRUPT_HDOORBELL, /* Hypervisor Doorbell interrupt */
> PPC_INTERRUPT_HVIRT, /* Hypervisor virtualization interrupt */
> + PPC_INTERRUPT_PMC, /* Performance Monitor Counter interrupt */
> };
>
> /* Processor Compatibility mask (PCR) */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 058b063d8a..e97898c5f4 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -821,6 +821,22 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> cpu_abort(cs, "Non maskable external exception "
> "is not implemented yet !\n");
> break;
> + case POWERPC_EXCP_EBB: /* Event-based branch exception */
> + if ((env->spr[SPR_BESCR] & BESCR_GE) &&
> + (env->spr[SPR_BESCR] & BESCR_PME)) {
> + target_ulong nip;
> +
> + env->spr[SPR_BESCR] &= ~BESCR_GE; /* Clear GE */
> + env->spr[SPR_BESCR] |= BESCR_PMEO; /* Set PMEO */
> + env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
> + nip = env->spr[SPR_EBBHR]; /* EBB handler */
> + powerpc_set_excp_state(cpu, nip, env->msr);
> + }
> + /*
> + * This interrupt is handled by userspace. No need
> + * to proceed.
> + */
> + return;
> default:
> excp_invalid:
> cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
> @@ -1068,6 +1084,18 @@ static void ppc_hw_interrupt(CPUPPCState *env)
> powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_THERM);
> return;
> }
> + /* PMC -> Event-based branch exception */
> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_PMC)) {
> + /*
> + * Performance Monitor event-based exception can only
> + * occur in problem state.
> + */
> + if (msr_pr == 1) {
> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PMC);
> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_EBB);
> + return;
> + }
> + }
> }
>
> if (env->resume_as_sreset) {
> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
> index 4545fe7810..a57b602125 100644
> --- a/target/ppc/power8_pmu.c
> +++ b/target/ppc/power8_pmu.c
> @@ -12,12 +12,14 @@
>
> #include "qemu/osdep.h"
>
> +#include "power8_pmu.h"
> #include "cpu.h"
> #include "helper_regs.h"
> #include "exec/exec-all.h"
> #include "exec/helper-proto.h"
> #include "qemu/error-report.h"
> #include "qemu/main-loop.h"
> +#include "hw/ppc/ppc.h"
>
> #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>
> @@ -114,6 +116,45 @@ static void update_cycles_PMCs(CPUPPCState *env)
> }
> }
>
> +static void cpu_ppc_pmu_timer_cb(void *opaque)
> +{
> + PowerPCCPU *cpu = opaque;
> + CPUPPCState *env = &cpu->env;
> + uint64_t mmcr0;
> +
> + mmcr0 = env->spr[SPR_POWER_MMCR0];
> + if (env->spr[SPR_POWER_MMCR0] & MMCR0_EBE) {
> + /* freeeze counters if needed */
> + if (mmcr0 & MMCR0_FCECE) {
> + mmcr0 &= ~MMCR0_FCECE;
> + mmcr0 |= MMCR0_FC;
> + }
> +
> + /* Clear PMAE and set PMAO */
> + if (mmcr0 & MMCR0_PMAE) {
> + mmcr0 &= ~MMCR0_PMAE;
> + mmcr0 |= MMCR0_PMAO;
> + }
> + env->spr[SPR_POWER_MMCR0] = mmcr0;
> +
> + /* Fire the PMC hardware exception */
> + ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
> + }
> +}
> +
> +void cpu_ppc_pmu_timer_init(CPUPPCState *env)
> +{
> + PowerPCCPU *cpu = env_archcpu(env);
> + QEMUTimer *timer;
> + int i;
> +
> + for (i = 0; i < PMU_TIMERS_LEN; i++) {
> + timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_pmu_timer_cb,
> + cpu);
> + env->pmu_intr_timers[i] = timer;
> + }
> +}
> +
> void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> {
> target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
> diff --git a/target/ppc/power8_pmu.h b/target/ppc/power8_pmu.h
> new file mode 100644
> index 0000000000..34a9d0e8a2
> --- /dev/null
> +++ b/target/ppc/power8_pmu.h
> @@ -0,0 +1,25 @@
> +/*
> + * PMU emulation helpers for TCG IBM POWER chips
> + *
> + * Copyright IBM Corp. 2021
> + *
> + * Authors:
> + * Daniel Henrique Barboza <danielhb413@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef PMU_BOOK3S_HELPER
> +#define PMU_BOOK3S_HELPER
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +
> +void cpu_ppc_pmu_timer_init(CPUPPCState *env);
> +
> +#endif
--
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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 10/16] target/ppc: PMU Event-Based exception support
2021-08-25 5:37 ` David Gibson
@ 2021-08-30 19:09 ` Daniel Henrique Barboza
0 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-30 19:09 UTC (permalink / raw)
To: David Gibson
Cc: gustavo.romero, Gustavo Romero, richard.henderson, qemu-devel,
groug, qemu-ppc, clg
On 8/25/21 2:37 AM, David Gibson wrote:
> On Tue, Aug 24, 2021 at 01:30:26PM -0300, Daniel Henrique Barboza wrote:
>> From: Gustavo Romero <gromero@linux.ibm.com>
>>
>> Following up the rfebb implementation, this patch adds the EBB exception
>> support that are triggered by Performance Monitor alerts. This exception
>> occurs when an enabled PMU condition or event happens and both MMCR0_EBE
>> and BESCR_PME are set.
>>
>> The supported PM alerts will consist of counter negative conditions of
>> the PMU counters. This will be achieved by a timer mechanism that will
>> predict when a counter becomes negative. The PMU timer callback will set
>> the appropriate bits in MMCR0 and fire a PMC interrupt. The EBB
>> exception code will then set the appropriate BESCR bits, set the next
>> instruction pointer to the address pointed by the return register
>> (SPR_EBBRR), and redirect execution to the handler (pointed by
>> SPR_EBBHR).
>>
>> This patch sets the basic structure of interrupts and timers. The
>> following patches will add the counter negative logic for the
>> registers.
>
> A timer makes sense for tripping cycles based EBB events. But for
> instructions based EBB events, shouldn't you instead have a test in
> the update instructions path which trips the event if you've passed
> the magic number?
IIUC this is done in patch 14.
Thanks,
Daniel
>
>>
>> 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>
>> ---
>> hw/ppc/spapr_cpu_core.c | 6 ++++++
>> target/ppc/cpu.h | 12 +++++++++++-
>> target/ppc/excp_helper.c | 28 +++++++++++++++++++++++++++
>> target/ppc/power8_pmu.c | 41 ++++++++++++++++++++++++++++++++++++++++
>> target/ppc/power8_pmu.h | 25 ++++++++++++++++++++++++
>> 5 files changed, 111 insertions(+), 1 deletion(-)
>> create mode 100644 target/ppc/power8_pmu.h
>>
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 4f316a6f9d..c7a342c4aa 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -20,6 +20,7 @@
>> #include "target/ppc/kvm_ppc.h"
>> #include "hw/ppc/ppc.h"
>> #include "target/ppc/mmu-hash64.h"
>> +#include "target/ppc/power8_pmu.h"
>> #include "sysemu/numa.h"
>> #include "sysemu/reset.h"
>> #include "sysemu/hw_accel.h"
>> @@ -266,6 +267,11 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> return false;
>> }
>>
>> + /* Init PMU interrupt timer (TCG only) */
>> + if (!kvm_enabled()) {
>> + cpu_ppc_pmu_timer_init(env);
>> + }
>> +
>> if (!sc->pre_3_0_migration) {
>> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
>> cpu->machine_data);
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 9d5eb9ead4..535754ddff 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -129,8 +129,9 @@ enum {
>> /* ISA 3.00 additions */
>> POWERPC_EXCP_HVIRT = 101,
>> POWERPC_EXCP_SYSCALL_VECTORED = 102, /* scv exception */
>> + POWERPC_EXCP_EBB = 103, /* Event-based branch exception */
>> /* EOL */
>> - POWERPC_EXCP_NB = 103,
>> + POWERPC_EXCP_NB = 104,
>> /* QEMU exceptions: special cases we want to stop translation */
>> POWERPC_EXCP_SYSCALL_USER = 0x203, /* System call in user mode only */
>> };
>> @@ -1047,6 +1048,8 @@ struct ppc_radix_page_info {
>> #define PPC_CPU_OPCODES_LEN 0x40
>> #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
>>
>> +#define PMU_TIMERS_LEN 5
>> +
>> struct CPUPPCState {
>> /* Most commonly used resources during translated code execution first */
>> target_ulong gpr[32]; /* general purpose registers */
>> @@ -1208,6 +1211,12 @@ struct CPUPPCState {
>> * running cycles.
>> */
>> uint64_t pmu_base_time;
>> +
>> + /*
>> + * Timers used to fire performance monitor alerts and
>> + * interrupts. All PMCs but PMC5 has a timer.
>> + */
>> + QEMUTimer *pmu_intr_timers[PMU_TIMERS_LEN];
>> };
>>
>> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
>> @@ -2424,6 +2433,7 @@ enum {
>> PPC_INTERRUPT_HMI, /* Hypervisor Maintenance interrupt */
>> PPC_INTERRUPT_HDOORBELL, /* Hypervisor Doorbell interrupt */
>> PPC_INTERRUPT_HVIRT, /* Hypervisor virtualization interrupt */
>> + PPC_INTERRUPT_PMC, /* Performance Monitor Counter interrupt */
>> };
>>
>> /* Processor Compatibility mask (PCR) */
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 058b063d8a..e97898c5f4 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -821,6 +821,22 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>> cpu_abort(cs, "Non maskable external exception "
>> "is not implemented yet !\n");
>> break;
>> + case POWERPC_EXCP_EBB: /* Event-based branch exception */
>> + if ((env->spr[SPR_BESCR] & BESCR_GE) &&
>> + (env->spr[SPR_BESCR] & BESCR_PME)) {
>> + target_ulong nip;
>> +
>> + env->spr[SPR_BESCR] &= ~BESCR_GE; /* Clear GE */
>> + env->spr[SPR_BESCR] |= BESCR_PMEO; /* Set PMEO */
>> + env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
>> + nip = env->spr[SPR_EBBHR]; /* EBB handler */
>> + powerpc_set_excp_state(cpu, nip, env->msr);
>> + }
>> + /*
>> + * This interrupt is handled by userspace. No need
>> + * to proceed.
>> + */
>> + return;
>> default:
>> excp_invalid:
>> cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
>> @@ -1068,6 +1084,18 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>> powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_THERM);
>> return;
>> }
>> + /* PMC -> Event-based branch exception */
>> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_PMC)) {
>> + /*
>> + * Performance Monitor event-based exception can only
>> + * occur in problem state.
>> + */
>> + if (msr_pr == 1) {
>> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PMC);
>> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_EBB);
>> + return;
>> + }
>> + }
>> }
>>
>> if (env->resume_as_sreset) {
>> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
>> index 4545fe7810..a57b602125 100644
>> --- a/target/ppc/power8_pmu.c
>> +++ b/target/ppc/power8_pmu.c
>> @@ -12,12 +12,14 @@
>>
>> #include "qemu/osdep.h"
>>
>> +#include "power8_pmu.h"
>> #include "cpu.h"
>> #include "helper_regs.h"
>> #include "exec/exec-all.h"
>> #include "exec/helper-proto.h"
>> #include "qemu/error-report.h"
>> #include "qemu/main-loop.h"
>> +#include "hw/ppc/ppc.h"
>>
>> #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>>
>> @@ -114,6 +116,45 @@ static void update_cycles_PMCs(CPUPPCState *env)
>> }
>> }
>>
>> +static void cpu_ppc_pmu_timer_cb(void *opaque)
>> +{
>> + PowerPCCPU *cpu = opaque;
>> + CPUPPCState *env = &cpu->env;
>> + uint64_t mmcr0;
>> +
>> + mmcr0 = env->spr[SPR_POWER_MMCR0];
>> + if (env->spr[SPR_POWER_MMCR0] & MMCR0_EBE) {
>> + /* freeeze counters if needed */
>> + if (mmcr0 & MMCR0_FCECE) {
>> + mmcr0 &= ~MMCR0_FCECE;
>> + mmcr0 |= MMCR0_FC;
>> + }
>> +
>> + /* Clear PMAE and set PMAO */
>> + if (mmcr0 & MMCR0_PMAE) {
>> + mmcr0 &= ~MMCR0_PMAE;
>> + mmcr0 |= MMCR0_PMAO;
>> + }
>> + env->spr[SPR_POWER_MMCR0] = mmcr0;
>> +
>> + /* Fire the PMC hardware exception */
>> + ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
>> + }
>> +}
>> +
>> +void cpu_ppc_pmu_timer_init(CPUPPCState *env)
>> +{
>> + PowerPCCPU *cpu = env_archcpu(env);
>> + QEMUTimer *timer;
>> + int i;
>> +
>> + for (i = 0; i < PMU_TIMERS_LEN; i++) {
>> + timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_pmu_timer_cb,
>> + cpu);
>> + env->pmu_intr_timers[i] = timer;
>> + }
>> +}
>> +
>> void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>> {
>> target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
>> diff --git a/target/ppc/power8_pmu.h b/target/ppc/power8_pmu.h
>> new file mode 100644
>> index 0000000000..34a9d0e8a2
>> --- /dev/null
>> +++ b/target/ppc/power8_pmu.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * PMU emulation helpers for TCG IBM POWER chips
>> + *
>> + * Copyright IBM Corp. 2021
>> + *
>> + * Authors:
>> + * Daniel Henrique Barboza <danielhb413@gmail.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef PMU_BOOK3S_HELPER
>> +#define PMU_BOOK3S_HELPER
>> +
>> +#include "qemu/osdep.h"
>> +#include "cpu.h"
>> +#include "exec/exec-all.h"
>> +#include "exec/helper-proto.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/main-loop.h"
>> +
>> +void cpu_ppc_pmu_timer_init(CPUPPCState *env);
>> +
>> +#endif
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 11/16] target/ppc/excp_helper.c: EBB handling adjustments
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (9 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 10/16] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 12/16] target/ppc/power8_pmu.c: enable PMC1 counter negative overflow Daniel Henrique Barboza
` (4 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
The current logic is only considering event-based exceptions triggered
by the performance monitor. This is true now, but we might want to add
support for external event-based exceptions in the future.
Let's make it a bit easier to do so by adding the bit logic that would
happen in case we were dealing with an external event-based exception.
While we're at it, add a few comments explaining why we're setting and
clearing BESCR bits.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/excp_helper.c | 45 ++++++++++++++++++++++++++++++++++------
1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index e97898c5f4..b1e803034e 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -822,14 +822,47 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
"is not implemented yet !\n");
break;
case POWERPC_EXCP_EBB: /* Event-based branch exception */
- if ((env->spr[SPR_BESCR] & BESCR_GE) &&
- (env->spr[SPR_BESCR] & BESCR_PME)) {
+ if (env->spr[SPR_BESCR] & BESCR_GE) {
target_ulong nip;
- env->spr[SPR_BESCR] &= ~BESCR_GE; /* Clear GE */
- env->spr[SPR_BESCR] |= BESCR_PMEO; /* Set PMEO */
- env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
- nip = env->spr[SPR_EBBHR]; /* EBB handler */
+ /*
+ * If we have Performance Monitor Event-Based exception
+ * enabled (BESCR_PME) and a Performance Monitor alert
+ * occurred (MMCR0_PMAO), clear BESCR_PME and set BESCR_PMEO
+ * (Performance Monitor Event-Based Exception Occurred).
+ *
+ * Software is responsible for clearing both BESCR_PMEO and
+ * MMCR0_PMAO after the event has been handled.
+ */
+ if ((env->spr[SPR_BESCR] & BESCR_PME) &&
+ (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAO)) {
+ env->spr[SPR_BESCR] &= ~BESCR_PME;
+ env->spr[SPR_BESCR] |= BESCR_PMEO;
+ }
+
+ /*
+ * In the case of External Event-Based exceptions, do a
+ * similar logic with BESCR_EE and BESCR_EEO. BESCR_EEO must
+ * also be cleared by software.
+ *
+ * PowerISA 3.1 considers that we'll not have BESCR_PMEO and
+ * BESCR_EEO set at the same time. We can check for BESCR_PMEO
+ * being not set in step above to see if this exception was
+ * trigged by an external event.
+ */
+ if (env->spr[SPR_BESCR] & BESCR_EE &&
+ !(env->spr[SPR_BESCR] & BESCR_PMEO)) {
+ env->spr[SPR_BESCR] &= ~BESCR_EE;
+ env->spr[SPR_BESCR] |= BESCR_EEO;
+ }
+
+ /*
+ * Clear BESCR_GE, save NIP for 'rfebb' and point the
+ * execution to the event handler (SPR_EBBHR) address.
+ */
+ env->spr[SPR_BESCR] &= ~BESCR_GE;
+ env->spr[SPR_EBBRR] = env->nip;
+ nip = env->spr[SPR_EBBHR];
powerpc_set_excp_state(cpu, nip, env->msr);
}
/*
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v2 12/16] target/ppc/power8_pmu.c: enable PMC1 counter negative overflow
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (10 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 11/16] target/ppc/excp_helper.c: EBB handling adjustments Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 13/16] target/ppc/power8_pmu.c: cycles overflow with all PMCs Daniel Henrique Barboza
` (3 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
This patch starts the counter negative EBB support by enabling PMC1
counter negative overflow when PMC1 is counting cycles.
A counter negative overflow happens when a performance monitor counter
reaches the value 0x80000000. When that happens, if counter negative
condition events are enabled in that counter, a performance monitor
alert is triggered. For PMC1, this condition is enabled by MMCR0_PMC1CE.
Cycle counting is done by calculating elapsed time between the time
the PMU started to run and when the PMU is shut down. Our clock is
fixed in 1Ghz, so 1 cycle equals 1 nanoseconds. The same idea is used to
predict a counter negative overflow: calculate the amount of nanoseconds
for the timer to reach 0x80000000, start a timer with it and trigger the
performance monitor alert. If event-based exceptions are enabled (bit
MMCR0_EBE), we'll go ahead and fire a PPC_INTERRUPT_PMC.
A new function 'start_cycle_count_session' was added to encapsulate the
most common steps of cycle calculation: redefine base time and start
overflow timers. This will avoid code repetition in the next patches.
Counter overflow for the remaining counters will be added shortly.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 1 +
target/ppc/power8_pmu.c | 96 +++++++++++++++++++++++++++++++++--------
2 files changed, 79 insertions(+), 18 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 535754ddff..b9d5dca983 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -352,6 +352,7 @@ typedef struct ppc_v3_pate_t {
#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
#define MMCR0_FC14 PPC_BIT(58)
#define MMCR0_FC56 PPC_BIT(59)
+#define MMCR0_PMC1CE PPC_BIT(48)
#define MMCR1_PMC1SEL_SHIFT (63 - 39)
#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index a57b602125..d10f371b5f 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -33,6 +33,8 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
env->spr[sprn] += time_delta;
}
+#define COUNTER_NEGATIVE_VAL 0x80000000
+
static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
{
int event = 0x0;
@@ -116,30 +118,88 @@ static void update_cycles_PMCs(CPUPPCState *env)
}
}
+static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
+{
+ int64_t remaining_cyc;
+
+ if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
+ return 0;
+ }
+
+ remaining_cyc = COUNTER_NEGATIVE_VAL - env->spr[sprn];
+ return remaining_cyc;
+}
+
+static bool counter_negative_cond_enabled(uint64_t mmcr0)
+{
+ return mmcr0 & MMCR0_PMC1CE;
+}
+
+/*
+ * A cycle count session consists of the basic operations we
+ * need to do to support PM_CYC events: redefine a new base_time
+ * to be used to calculate PMC values and start overflow timers.
+ */
+static void start_cycle_count_session(CPUPPCState *env)
+{
+ uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ uint64_t timeout;
+
+ env->pmu_base_time = now;
+
+ /*
+ * Always delete existing overflow timers when starting a
+ * new cycle counting session.
+ */
+ timer_del(env->pmu_intr_timers[0]);
+
+ if (!counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
+ return;
+ }
+
+ if (!pmc_is_running(env, SPR_POWER_PMC1)) {
+ return;
+ }
+
+ if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
+ return;
+ }
+
+ switch (get_PMC_event(env, SPR_POWER_PMC1)) {
+ case 0xF0:
+ case 0x1E:
+ timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
+ break;
+ default:
+ return;
+ }
+
+ timer_mod(env->pmu_intr_timers[0], now + timeout);
+}
+
static void cpu_ppc_pmu_timer_cb(void *opaque)
{
PowerPCCPU *cpu = opaque;
CPUPPCState *env = &cpu->env;
- uint64_t mmcr0;
-
- mmcr0 = env->spr[SPR_POWER_MMCR0];
- if (env->spr[SPR_POWER_MMCR0] & MMCR0_EBE) {
- /* freeeze counters if needed */
- if (mmcr0 & MMCR0_FCECE) {
- mmcr0 &= ~MMCR0_FCECE;
- mmcr0 |= MMCR0_FC;
- }
- /* Clear PMAE and set PMAO */
- if (mmcr0 & MMCR0_PMAE) {
- mmcr0 &= ~MMCR0_PMAE;
- mmcr0 |= MMCR0_PMAO;
- }
- env->spr[SPR_POWER_MMCR0] = mmcr0;
+ if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_EBE)) {
+ return;
+ }
- /* Fire the PMC hardware exception */
- ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
+ if (env->spr[SPR_POWER_MMCR0] & MMCR0_FCECE) {
+ env->spr[SPR_POWER_MMCR0] &= ~MMCR0_FCECE;
+ env->spr[SPR_POWER_MMCR0] |= MMCR0_FC;
}
+
+ update_cycles_PMCs(env);
+
+ if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAE) {
+ env->spr[SPR_POWER_MMCR0] &= ~MMCR0_PMAE;
+ env->spr[SPR_POWER_MMCR0] |= MMCR0_PMAO;
+ }
+
+ /* Fire the PMC hardware exception */
+ ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
}
void cpu_ppc_pmu_timer_init(CPUPPCState *env)
@@ -182,7 +242,7 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
if (!curr_FC) {
update_cycles_PMCs(env);
} else {
- env->pmu_base_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ start_cycle_count_session(env);
}
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v2 13/16] target/ppc/power8_pmu.c: cycles overflow with all PMCs
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (11 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 12/16] target/ppc/power8_pmu.c: enable PMC1 counter negative overflow Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 14/16] target/ppc: PMU: insns counter negative overflow support Daniel Henrique Barboza
` (2 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
All performance monitor counters can trigger a counter negative
condition if the proper MMCR0 bits are set. This patch does that
for all PMCs that can count cycles by doing the following:
- pmc_counter_negative_enabled() will check whether a given PMC is
eligible to trigger the counter negative alert;
- get_counter_neg_timeout() will return the timeout for the counter
negative condition for a given PMC, or -1 if the PMC is not able to
trigger this alert;
- the existing counter_negative_cond_enabled() now must consider the
counter negative bit for PMCs 2-6, MMCR0_PMCjCE;
- start_cycle_count_session() will start overflow timers for all eligible
PMCs.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 1 +
target/ppc/power8_pmu.c | 116 ++++++++++++++++++++++++++++++++++------
2 files changed, 100 insertions(+), 17 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index b9d5dca983..f4337e1621 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -353,6 +353,7 @@ typedef struct ppc_v3_pate_t {
#define MMCR0_FC14 PPC_BIT(58)
#define MMCR0_FC56 PPC_BIT(59)
#define MMCR0_PMC1CE PPC_BIT(48)
+#define MMCR0_PMCjCE PPC_BIT(49)
#define MMCR1_PMC1SEL_SHIFT (63 - 39)
#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index d10f371b5f..690476051d 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -130,9 +130,81 @@ static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
return remaining_cyc;
}
+static bool pmc_counter_negative_enabled(CPUPPCState *env, int sprn)
+{
+ if (!pmc_is_running(env, sprn)) {
+ return false;
+ }
+
+ switch (sprn) {
+ case SPR_POWER_PMC1:
+ return env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE;
+
+ case SPR_POWER_PMC2:
+ case SPR_POWER_PMC3:
+ case SPR_POWER_PMC4:
+ case SPR_POWER_PMC5:
+ case SPR_POWER_PMC6:
+ return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
+
+ default:
+ break;
+ }
+
+ return false;
+}
+
+static int64_t get_counter_neg_timeout(CPUPPCState *env, int sprn)
+{
+ int64_t timeout = -1;
+
+ if (!pmc_counter_negative_enabled(env, sprn)) {
+ return -1;
+ }
+
+ if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
+ return 0;
+ }
+
+ switch (sprn) {
+ case SPR_POWER_PMC1:
+ case SPR_POWER_PMC2:
+ case SPR_POWER_PMC3:
+ case SPR_POWER_PMC4:
+ switch (get_PMC_event(env, sprn)) {
+ case 0xF0:
+ if (sprn == SPR_POWER_PMC1) {
+ timeout = get_CYC_timeout(env, sprn);
+ }
+ break;
+ case 0x1E:
+ timeout = get_CYC_timeout(env, sprn);
+ break;
+ }
+
+ break;
+ case SPR_POWER_PMC6:
+ timeout = get_CYC_timeout(env, sprn);
+ break;
+ default:
+ break;
+ }
+
+ return timeout;
+}
+
static bool counter_negative_cond_enabled(uint64_t mmcr0)
{
- return mmcr0 & MMCR0_PMC1CE;
+ return mmcr0 & (MMCR0_PMC1CE | MMCR0_PMCjCE);
+}
+
+static void pmu_delete_timers(CPUPPCState *env)
+{
+ int i;
+
+ for (i = 0; i < PMU_TIMERS_LEN; i++) {
+ timer_del(env->pmu_intr_timers[i]);
+ }
}
/*
@@ -143,7 +215,8 @@ static bool counter_negative_cond_enabled(uint64_t mmcr0)
static void start_cycle_count_session(CPUPPCState *env)
{
uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- uint64_t timeout;
+ int64_t timeout;
+ int i;
env->pmu_base_time = now;
@@ -151,30 +224,32 @@ static void start_cycle_count_session(CPUPPCState *env)
* Always delete existing overflow timers when starting a
* new cycle counting session.
*/
- timer_del(env->pmu_intr_timers[0]);
+ pmu_delete_timers(env);
if (!counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
return;
}
- if (!pmc_is_running(env, SPR_POWER_PMC1)) {
- return;
- }
+ /*
+ * Scroll through all programmable PMCs start counter overflow
+ * timers for PM_CYC events, if needed.
+ */
+ for (i = SPR_POWER_PMC1; i < SPR_POWER_PMC5; i++) {
+ timeout = get_counter_neg_timeout(env, i);
- if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
- return;
- }
+ if (timeout == -1) {
+ continue;
+ }
- switch (get_PMC_event(env, SPR_POWER_PMC1)) {
- case 0xF0:
- case 0x1E:
- timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
- break;
- default:
- return;
+ timer_mod(env->pmu_intr_timers[i - SPR_POWER_PMC1],
+ now + timeout);
}
- timer_mod(env->pmu_intr_timers[0], now + timeout);
+ /* Check for counter neg timeout in PMC6 */
+ timeout = get_counter_neg_timeout(env, SPR_POWER_PMC6);
+ if (timeout != -1) {
+ timer_mod(env->pmu_intr_timers[PMU_TIMERS_LEN - 1], now + timeout);
+ }
}
static void cpu_ppc_pmu_timer_cb(void *opaque)
@@ -189,6 +264,13 @@ static void cpu_ppc_pmu_timer_cb(void *opaque)
if (env->spr[SPR_POWER_MMCR0] & MMCR0_FCECE) {
env->spr[SPR_POWER_MMCR0] &= ~MMCR0_FCECE;
env->spr[SPR_POWER_MMCR0] |= MMCR0_FC;
+
+ /*
+ * Delete all pending timers if we need to freeze
+ * the PMC. We'll restart them when the PMC starts
+ * running again.
+ */
+ pmu_delete_timers(env);
}
update_cycles_PMCs(env);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v2 14/16] target/ppc: PMU: insns counter negative overflow support
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (12 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 13/16] target/ppc/power8_pmu.c: cycles overflow with all PMCs Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 15/16] target/ppc/translate: PMU: handle setting of PMCs while running Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 16/16] target/ppc/power8_pmu.c: handle overflow bits when PMU is running Daniel Henrique Barboza
15 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
Enabling counter negative overflow for the PMCs that are counting
instructions is simpler than when counting cycles. Instruction
counting is done via helper_insns_inc(), which is called every
time a TB ends.
Firing a performance monitor alert due to a counter negative overflow
in this case is a matter of checking if the counter value is over
0x80000000 each time the counters are incremented and, if counter negative
events are enabled for that specific counter, trigger the PM alert.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/power8_pmu.c | 23 +++++++++++++++++++++--
target/ppc/translate.c | 8 ++++++++
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 690476051d..28db086225 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -252,9 +252,8 @@ static void start_cycle_count_session(CPUPPCState *env)
}
}
-static void cpu_ppc_pmu_timer_cb(void *opaque)
+static void fire_PMC_interrupt(PowerPCCPU *cpu)
{
- PowerPCCPU *cpu = opaque;
CPUPPCState *env = &cpu->env;
if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_EBE)) {
@@ -284,6 +283,13 @@ static void cpu_ppc_pmu_timer_cb(void *opaque)
ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
}
+static void cpu_ppc_pmu_timer_cb(void *opaque)
+{
+ PowerPCCPU *cpu = opaque;
+
+ fire_PMC_interrupt(cpu);
+}
+
void cpu_ppc_pmu_timer_init(CPUPPCState *env)
{
PowerPCCPU *cpu = env_archcpu(env);
@@ -377,6 +383,8 @@ static bool pmc_counting_insns(CPUPPCState *env, int sprn,
/* This helper assumes that the PMC is running. */
void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
{
+ bool overflow_triggered = false;
+ PowerPCCPU *cpu;
int sprn;
for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
@@ -390,8 +398,19 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
} else {
env->spr[sprn] += num_insns;
}
+
+ if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL &&
+ pmc_counter_negative_enabled(env, sprn)) {
+ overflow_triggered = true;
+ env->spr[sprn] = COUNTER_NEGATIVE_VAL;
+ }
}
}
+
+ if (overflow_triggered) {
+ cpu = env_archcpu(env);
+ fire_PMC_interrupt(cpu);
+ }
}
#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index d4cfc567cf..8302022852 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4437,6 +4437,14 @@ static void pmu_count_insns(DisasContext *ctx)
tcg_gen_andi_tl(t0, t0, MMCR0_FC);
tcg_gen_brcond_tl(TCG_COND_EQ, t0, t_mmcr0FC, l_exit);
+ /*
+ * The PMU insns_inc() helper stops the internal PMU timer if a
+ * counter overflows happens. In that case, if the guest is
+ * running with icount and we do not handle it beforehand,
+ * the helper can trigger a 'bad icount read'.
+ */
+ gen_icount_io_start(ctx);
+
gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
gen_set_label(l_exit);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v2 15/16] target/ppc/translate: PMU: handle setting of PMCs while running
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (13 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 14/16] target/ppc: PMU: insns counter negative overflow support Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
2021-08-24 16:30 ` [PATCH v2 16/16] target/ppc/power8_pmu.c: handle overflow bits when PMU is running Daniel Henrique Barboza
15 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
The initial PMU support were made under the assumption that the counters
would be set before running the PMU and read after either freezing the
PMU manually or via a performance monitor alert.
Turns out that some EBB powerpc kernel tests set the counters after
unfreezing the counters. Setting a PMC value when the PMU is running
means that, at that moment, the baseline for calculating cycle
events needs to be updated. Updating this baseline means that we need
to update all the PMCs with their actual value at that moment. Any
xisting counter negative timer needs to be discarded an a new one,
with the updated values, must be set again.
This patch does that via a new 'helper_store_pmc()' that is called in
the mtspr() callbacks of PMU counters. With this change, EBB powerpc kernel
tests such as 'no_handler_test' are now passing.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu_init.c | 24 ++++++++++++------------
target/ppc/helper.h | 1 +
target/ppc/power8_pmu.c | 27 +++++++++++++++++++++++++++
target/ppc/spr_tcg.h | 2 ++
target/ppc/translate.c | 35 +++++++++++++++++++++++++++++++++++
5 files changed, 77 insertions(+), 12 deletions(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 71f052b052..563c457572 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6833,27 +6833,27 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
KVM_REG_PPC_MMCRA, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC1, "PMC1",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_PMC_generic,
KVM_REG_PPC_PMC1, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC2, "PMC2",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_PMC_generic,
KVM_REG_PPC_PMC2, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC3, "PMC3",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_PMC_generic,
KVM_REG_PPC_PMC3, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC4, "PMC4",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_PMC_generic,
KVM_REG_PPC_PMC4, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC5, "PMC5",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_PMC_generic,
KVM_REG_PPC_PMC5, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC6, "PMC6",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_PMC_generic,
KVM_REG_PPC_PMC6, 0x00000000);
spr_register_kvm(env, SPR_POWER_SIAR, "SIAR",
SPR_NOACCESS, SPR_NOACCESS,
@@ -6880,27 +6880,27 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UPMC1, "UPMC1",
- &spr_read_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_ureg, &spr_write_PMC_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UPMC2, "UPMC2",
- &spr_read_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_ureg, &spr_write_PMC_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UPMC3, "UPMC3",
- &spr_read_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_ureg, &spr_write_PMC_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UPMC4, "UPMC4",
- &spr_read_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_ureg, &spr_write_PMC_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UPMC5, "UPMC5",
- &spr_read_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_ureg, &spr_write_PMC_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_UPMC6, "UPMC6",
- &spr_read_ureg, &spr_write_PMU_groupA_ureg,
+ &spr_read_ureg, &spr_write_PMC_ureg,
&spr_read_ureg, &spr_write_ureg,
0x00000000);
spr_register(env, SPR_POWER_USIAR, "USIAR",
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 91a86992a5..52cb62b9e1 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -23,6 +23,7 @@ DEF_HELPER_2(store_lpcr, void, env, tl)
DEF_HELPER_2(store_pcr, void, env, tl)
DEF_HELPER_2(store_mmcr0, void, env, tl)
DEF_HELPER_2(insns_inc, void, env, i32)
+DEF_HELPER_3(store_pmc, void, env, i32, i64)
#endif
DEF_HELPER_1(check_tlb_flush_local, void, env)
DEF_HELPER_1(check_tlb_flush_global, void, env)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 28db086225..d235cc2b53 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -116,6 +116,14 @@ static void update_cycles_PMCs(CPUPPCState *env)
if (PMC6_running) {
update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
}
+
+ /*
+ * Update base_time for future calculations if we updated
+ * the PMCs while the PMU was running.
+ */
+ if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_FC)) {
+ env->pmu_base_time = now;
+ }
}
static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
@@ -413,4 +421,23 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
}
}
+void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
+{
+ bool pmu_frozen = env->spr[SPR_POWER_MMCR0] & MMCR0_FC;
+
+ if (pmu_frozen) {
+ env->spr[sprn] = value;
+ return;
+ }
+
+ /*
+ * Update counters with the events counted so far, define
+ * the new value of the PMC and start a new cycle count
+ * session.
+ */
+ update_cycles_PMCs(env);
+ env->spr[sprn] = value;
+ start_cycle_count_session(env);
+}
+
#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 2c5b056fc1..84f8ef39ab 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -26,6 +26,7 @@ void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn);
+void spr_write_PMC_generic(DisasContext *ctx, int sprn, int gprn);
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);
@@ -45,6 +46,7 @@ void spr_read_spefscr(DisasContext *ctx, int gprn, int sprn);
void spr_write_spefscr(DisasContext *ctx, int sprn, int gprn);
void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn);
+void spr_write_PMC_ureg(DisasContext *ctx, int sprn, int gprn);
#ifndef CONFIG_USER_ONLY
void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 8302022852..d241795131 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -407,13 +407,29 @@ void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
gen_icount_io_start(ctx);
gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
}
+
+void spr_write_PMC_generic(DisasContext *ctx, int sprn, int gprn)
+{
+ TCGv_i32 t_sprn = tcg_const_i32(sprn);
+
+ gen_icount_io_start(ctx);
+ gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
+
+ tcg_temp_free_i32(t_sprn);
+}
#else
void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
{
spr_write_generic(ctx, sprn, gprn);
}
+void spr_write_PMC_generic(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_write_generic(ctx, sprn, gprn);
+}
#endif
+
+
#if !defined(CONFIG_USER_ONLY)
void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
{
@@ -640,6 +656,20 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
tcg_temp_free(t0);
tcg_temp_free(t1);
}
+
+void spr_write_PMC_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+ /*
+ * All PMCs belongs to Group A SPRs. The same write access
+ * control done in spr_write_PMU_groupA_ureg() applies.
+ */
+ if (ctx->pmcc_clear) {
+ gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
+ return;
+ }
+
+ spr_write_PMC_generic(ctx, sprn + 0x10, gprn);
+}
#else
void spr_write_PMU_groupA_ureg(DisasContext *ctx, int sprn, int gprn)
{
@@ -650,6 +680,11 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
{
spr_noaccess(ctx, gprn, sprn);
}
+
+void spr_write_PMC_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_noaccess(ctx, gprn, sprn);
+}
#endif
/* SPR common to all non-embedded PowerPC */
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v2 16/16] target/ppc/power8_pmu.c: handle overflow bits when PMU is running
2021-08-24 16:30 [PATCH v2 00/16] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (14 preceding siblings ...)
2021-08-24 16:30 ` [PATCH v2 15/16] target/ppc/translate: PMU: handle setting of PMCs while running Daniel Henrique Barboza
@ 2021-08-24 16:30 ` Daniel Henrique Barboza
15 siblings, 0 replies; 33+ messages in thread
From: Daniel Henrique Barboza @ 2021-08-24 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: gustavo.romero, Daniel Henrique Barboza, richard.henderson,
groug, qemu-ppc, clg, david
Up until this moment we were assuming that the counter negative
enabled bits, PMC1CE and PMCjCE, would never be changed when the
PMU is already started.
Turns out that there is no such restriction in the PowerISA v3.1,
and software can enable/disable overflow conditions of the counters
at any time.
To support this scenario, track the overflow bits state when a
write in MMCR0 is made in which the run state of the PMU (MMCR0_FC
bit) didn't change and, if some overflow bit were changed in the
middle of a cycle count session, restart it.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/power8_pmu.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index d235cc2b53..e02986f18a 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -340,6 +340,30 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
} else {
start_cycle_count_session(env);
}
+ } else {
+ /*
+ * No change in MMCR0_FC state, but if the PMU is running and
+ * a change in the counter negative overflow bits is made,
+ * we need to restart a new cycle count session to restart
+ * the appropriate overflow timers.
+ */
+ if (curr_FC) {
+ return;
+ }
+
+ bool pmc1ce_curr = curr_value & MMCR0_PMC1CE;
+ bool pmc1ce_new = value & MMCR0_PMC1CE;
+ bool pmcjce_curr = curr_value & MMCR0_PMCjCE;
+ bool pmcjce_new = value & MMCR0_PMCjCE;
+
+ if (pmc1ce_curr == pmc1ce_new && pmcjce_curr == pmcjce_new) {
+ return;
+ }
+
+ /* Update the counter with the events counted so far */
+ update_cycles_PMCs(env);
+
+ start_cycle_count_session(env);
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread