[PATCH 1/2] power: Define PV_POWER7P

[PATCH 1/2] power: Define PV_POWER7P

[Sukadev Bhattiprolu]

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
-- 
1.7.1

[PATCH 2/2] [perf][power]: Sample only if SIAR-Valid bit is set in P7+

[Sukadev Bhattiprolu]

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

Re: [PATCH 1/2] power: Define PV_POWER7P

[Gabriel Paubert]

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

Re: [PATCH 1/2] power: Define PV_POWER7P

[Sukadev Bhattiprolu]

Gabriel Paubert [paubert@iram.es] wrote:
| > +#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.

Agree with both your points above. I re-sent the patches with updates.

Sukadev