* [PATCH 2/2] [perf][power]: Sample only if SIAR-Valid bit is set in P7+
2012-07-12 23:59 [PATCH 1/2] power: Define PV_POWER7P Sukadev Bhattiprolu
@ 2012-07-13 0:00 ` Sukadev Bhattiprolu
2012-07-13 6:46 ` [PATCH 1/2] power: Define PV_POWER7P Gabriel Paubert
1 sibling, 0 replies; 4+ messages in thread
From: Sukadev Bhattiprolu @ 2012-07-13 0:00 UTC (permalink / raw)
To: benh; +Cc: michaele, Anton Blanchard, sukadev, linuxppc-dev, cel, khandual
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Mon, 2 Jul 2012 08:06:14 -0700
Subject: [PATCH 2/2] [perf][power]: Sample only if SIAR-Valid bit is set in P7+
On POWER7+ two new bits (mmcra[35] and mmcra[36]) indicate whether the
contents of SIAR and SDAR are valid.
For marked instructions on P7+, we must save the contents of SIAR and
SDAR registers only if these new bits are set.
This code/check for the SIAR-Valid bit is specific to P7+, so rather than
waste a CPU-feature bit use the PVR flag.
Note that Carl Love proposed a similar change for oprofile:
https://lkml.org/lkml/2012/6/22/309
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/perf_event_server.h | 1 +
arch/powerpc/include/asm/reg.h | 4 +++
arch/powerpc/perf/core-book3s.c | 38 ++++++++++++++++++++++---
arch/powerpc/perf/power7-pmu.c | 3 ++
4 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
index 078019b..9710be3 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -49,6 +49,7 @@ struct power_pmu {
#define PPMU_ALT_SIPR 2 /* uses alternate posn for SIPR/HV */
#define PPMU_NO_SIPR 4 /* no SIPR/HV in MMCRA at all */
#define PPMU_NO_CONT_SAMPLING 8 /* no continuous sampling */
+#define PPMU_SIAR_VALID 16 /* Processor has SIAR Valid bit */
/*
* Values for flags to get_alternatives()
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index b3fc2c1..6c47f0b 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -601,6 +601,10 @@
#define POWER6_MMCRA_SIPR 0x0000020000000000ULL
#define POWER6_MMCRA_THRM 0x00000020UL
#define POWER6_MMCRA_OTHER 0x0000000EUL
+
+#define POWER7P_MMCRA_SIAR_VALID 0x10000000 /* P7+ SIAR contents valid */
+#define POWER7P_MMCRA_SDAR_VALID 0x08000000 /* P7+ SDAR contents valid */
+
#define SPRN_PMC1 787
#define SPRN_PMC2 788
#define SPRN_PMC3 789
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 8f84bcb..0a392d8 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -103,14 +103,20 @@ static inline unsigned long perf_ip_adjust(struct pt_regs *regs)
* If we're not doing instruction sampling, give them the SDAR
* (sampled data address). If we are doing instruction sampling, then
* only give them the SDAR if it corresponds to the instruction
- * pointed to by SIAR; this is indicated by the [POWER6_]MMCRA_SDSYNC
- * bit in MMCRA.
+ * pointed to by SIAR; this is indicated by the [POWER6_]MMCRA_SDSYNC or
+ * the [POWER7P_]MMCRA_SDAR_VALID bit in MMCRA.
*/
static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
{
unsigned long mmcra = regs->dsisr;
- unsigned long sdsync = (ppmu->flags & PPMU_ALT_SIPR) ?
- POWER6_MMCRA_SDSYNC : MMCRA_SDSYNC;
+ unsigned long sdsync;
+
+ if (ppmu->flags & PPMU_SIAR_VALID)
+ sdsync = POWER7P_MMCRA_SDAR_VALID;
+ else if (ppmu->flags & PPMU_ALT_SIPR)
+ sdsync = POWER6_MMCRA_SDSYNC;
+ else
+ sdsync = MMCRA_SDSYNC;
if (!(mmcra & MMCRA_SAMPLE_ENABLE) || (mmcra & sdsync))
*addrp = mfspr(SPRN_SDAR);
@@ -1248,6 +1254,25 @@ struct pmu power_pmu = {
.event_idx = power_pmu_event_idx,
};
+
+/*
+ * On processors like P7+ that have the SIAR-Valid bit, marked instructions
+ * must be sampled only if the SIAR-valid bit is set.
+ *
+ * For unmarked instructions and for processors that don't have the SIAR-Valid
+ * bit, assume that SIAR is valid.
+ */
+static inline int siar_valid(struct pt_regs *regs)
+{
+ unsigned long mmcra = regs->dsisr;
+ int marked = mmcra & MMCRA_SAMPLE_ENABLE;
+
+ if ((ppmu->flags & PPMU_SIAR_VALID) && marked)
+ return mmcra & POWER7P_MMCRA_SIAR_VALID;
+
+ return 1;
+}
+
/*
* A counter has overflowed; update its count and record
* things if requested. Note that interrupts are hard-disabled
@@ -1281,7 +1306,7 @@ static void record_and_restart(struct perf_event *event, unsigned long val,
left += period;
if (left <= 0)
left = period;
- record = 1;
+ record = siar_valid(regs);
event->hw.last_period = event->hw.sample_period;
}
if (left < 0x80000000LL)
@@ -1340,6 +1365,9 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
!(mmcra & MMCRA_SAMPLE_ENABLE))
return regs->nip;
+ if (!siar_valid(regs))
+ return 0; // no valid instruction pointer
+
return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
}
diff --git a/arch/powerpc/perf/power7-pmu.c b/arch/powerpc/perf/power7-pmu.c
index 1251e4d..4d53124 100644
--- a/arch/powerpc/perf/power7-pmu.c
+++ b/arch/powerpc/perf/power7-pmu.c
@@ -373,6 +373,9 @@ static int __init init_power7_pmu(void)
strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power7"))
return -ENODEV;
+ if (__is_processor(PV_POWER7P))
+ power7_pmu.flags |= PPMU_SIAR_VALID;
+
return register_power_pmu(&power7_pmu);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] power: Define PV_POWER7P
2012-07-12 23:59 [PATCH 1/2] power: Define PV_POWER7P Sukadev Bhattiprolu
2012-07-13 0:00 ` [PATCH 2/2] [perf][power]: Sample only if SIAR-Valid bit is set in P7+ Sukadev Bhattiprolu
@ 2012-07-13 6:46 ` Gabriel Paubert
2012-07-16 21:25 ` Sukadev Bhattiprolu
1 sibling, 1 reply; 4+ messages in thread
From: Gabriel Paubert @ 2012-07-13 6:46 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: michaele, Anton Blanchard, sukadev, linuxppc-dev, cel, khandual
On Thu, Jul 12, 2012 at 04:59:12PM -0700, Sukadev Bhattiprolu wrote:
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Tue, 3 Jul 2012 13:32:46 -0700
> Subject: [PATCH 1/2] power: Define PV_POWER7P
>
> This change is based on the patch that Carl Love posted to LKML
>
> https://lkml.org/lkml/2012/6/22/309
>
> It is included here for completeness and to enable building. When
> the above patch is merged, this patch can be ignored.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/reg.h | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index f0cb7f4..b3fc2c1 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -1014,6 +1014,7 @@
> #define PV_970FX 0x003C
> #define PV_POWER6 0x003E
> #define PV_POWER7 0x003F
> +#define PV_POWER7P 0x004A
> #define PV_630 0x0040
> #define PV_630p 0x0041
> #define PV_970MP 0x0044
Hmm, before this patch the PVR definitions were sorted in ascending
numerical order, at least for the list of 64 bit processors. Your
patch breaks this, which is not a good idea IMHO.
For example, the 970* processors are already interspersed with other
processors to maintain numerical order, therefore I don't see why the
POWER7P could not be between 970GX and BE.
Another inconsistency is that all other "plus" variants seem to
use a lower case "p" suffix. So it would be better to use POWER7p.
Regards,
Gabriel
^ permalink raw reply [flat|nested] 4+ messages in thread