[PATCH 1/5] powerpc/perf: Add new power pmu flag "PPMU_P10_DD1" for power10 DD1

[PATCH 1/5] powerpc/perf: Add new power pmu flag "PPMU_P10_DD1" for power10 DD1

[Madhavan Srinivasan]

From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Add a new power PMU flag "PPMU_P10_DD1" which can be
used to conditionally add any code path for power10 DD1 processor
version. Also modify power10 PMU driver code to set this
flag only for DD1, based on the Processor Version Register (PVR)
value.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
 arch/powerpc/include/asm/perf_event_server.h | 1 +
 arch/powerpc/perf/power10-pmu.c              | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
index f6acabb6c9be..3b7baba01c92 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -82,6 +82,7 @@ struct power_pmu {
 #define PPMU_ARCH_207S		0x00000080 /* PMC is architecture v2.07S */
 #define PPMU_NO_SIAR		0x00000100 /* Do not use SIAR */
 #define PPMU_ARCH_31		0x00000200 /* Has MMCR3, SIER2 and SIER3 */
+#define PPMU_P10_DD1		0x00000400 /* Is power10 DD1 processor version */
 
 /*
  * Values for flags to get_alternatives()
diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index 9dbe8f9b89b4..a01e87f0b8d0 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c
@@ -403,6 +403,7 @@ static struct power_pmu power10_pmu = {
 
 int init_power10_pmu(void)
 {
+	unsigned int pvr;
 	int rc;
 
 	/* Comes from cpu_specs[] */
@@ -410,6 +411,11 @@ int init_power10_pmu(void)
 	    strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power10"))
 		return -ENODEV;
 
+	pvr = mfspr(SPRN_PVR);
+	/* Add the ppmu flag for power10 DD1 */
+	if ((PVR_CFG(pvr) == 1))
+		power10_pmu.flags |= PPMU_P10_DD1;
+
 	/* Set the PERF_REG_EXTENDED_MASK here */
 	PERF_REG_EXTENDED_MASK = PERF_REG_PMU_MASK_31;
 
-- 
2.26.2

[PATCH v2 2/5] powerpc/perf: Drop the check for SIAR_VALID

[Madhavan Srinivasan]

From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

In power10 DD1, there is an issue that causes the SIAR_VALID
bit of Sampled Instruction Event Register(SIER) not to be
set. But the SIAR_VALID bit is used for fetching the instruction
address from Sampled Instruction Address Register(SIAR), and
marked events are sampled only if the SIAR_VALID bit is set.
So drop the check for SIAR_VALID and return true always incase of
power10 DD1.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
Changelog v1:
- Drop the check for SIER[CMPL] and retur true instead
- Made changes to commit message

 arch/powerpc/perf/core-book3s.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 08643cba1494..3b62dbb94796 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -350,7 +350,14 @@ static inline int siar_valid(struct pt_regs *regs)
 	int marked = mmcra & MMCRA_SAMPLE_ENABLE;
 
 	if (marked) {
-		if (ppmu->flags & PPMU_HAS_SIER)
+		/*
+		 * SIER[SIAR_VALID] is not set for some
+		 * marked events on power10 DD1, so drop
+		 * the check for SIER[SIAR_VALID] and return true.
+		 */
+		if (ppmu->flags & PPMU_P10_DD1)
+			return 0x1;
+		else if (ppmu->flags & PPMU_HAS_SIER)
 			return regs->dar & SIER_SIAR_VALID;
 
 		if (ppmu->flags & PPMU_SIAR_VALID)
-- 
2.26.2

[PATCH 3/5] powerpc/perf: Use the address from SIAR register to set cpumode flags

[Madhavan Srinivasan]

From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

While setting the processor mode for any sample, `perf_get_misc_flags`
expects the privilege level to differentiate the userspace and kernel
address. On power10 DD1, there is an issue that causes [MSR_HV MSR_PR] bits
of Sampled Instruction Event Register (SIER) not to be set for marked
events. Hence add a check to use the address in Sampled Instruction Address
Register (SIAR) to identify the privilege level.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 3b62dbb94796..6be0349e01ad 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -250,10 +250,24 @@ static inline u32 perf_flags_from_msr(struct pt_regs *regs)
 static inline u32 perf_get_misc_flags(struct pt_regs *regs)
 {
 	bool use_siar = regs_use_siar(regs);
+	unsigned long mmcra = regs->dsisr;
+	int marked = mmcra & MMCRA_SAMPLE_ENABLE;
 
 	if (!use_siar)
 		return perf_flags_from_msr(regs);
 
+	/*
+	 * Check the address in SIAR to identify the
+	 * privilege levels since the SIER[MSR_HV, MSR_PR]
+	 * bits are not set for marked events in power10
+	 * DD1.
+	 */
+	if (marked && (ppmu->flags & PPMU_P10_DD1)) {
+		if (is_kernel_addr(mfspr(SPRN_SIAR)))
+			return PERF_RECORD_MISC_KERNEL;
+		return PERF_RECORD_MISC_USER;
+	}
+
 	/*
 	 * If we don't have flags in MMCRA, rather than using
 	 * the MSR, we intuit the flags from the address in
-- 
2.26.2

[PATCH 4/5] powerpc/perf: Exclude kernel samples while counting events in user space.

[Madhavan Srinivasan]

From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

By setting exclude_kernel for user space profiling, we set the
freeze bits in Monitor Mode Control Register. Due to hardware
limitation, sometimes, Sampled Instruction Address register (SIAR)
captures kernel address even when counter freeze bits are set in
Monitor Mode Control Register (MMCR2). Patch adds a check to drop
these samples at such conditions.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 6be0349e01ad..e675c7c8ce0e 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -2142,6 +2142,18 @@ static void record_and_restart(struct perf_event *event, unsigned long val,
 	local64_set(&event->hw.period_left, left);
 	perf_event_update_userpage(event);
 
+	/*
+	 * Setting exclude_kernel will only freeze the
+	 * Performance Monitor counters and we may have
+	 * kernel address captured in SIAR. Hence drop
+	 * the kernel sample captured during user space
+	 * profiling. Setting `record` to zero will also
+	 * make sure event throlling is handled.
+	 */
+	if (event->attr.exclude_kernel && record)
+		if (is_kernel_addr(mfspr(SPRN_SIAR)))
+			record = 0;
+
 	/*
 	 * Finally record data if requested.
 	 */
-- 
2.26.2

[PATCH 5/5] powerpc/perf: use regs->nip when siar is zero

[Madhavan Srinivasan]

In power10 DD1, there is an issue where the
Sampled Instruction Address Register (SIAR)
not latching to the sampled address during
random sampling. This results in value of 0s
in the SIAR. Patch adds a check to use regs->nip
when SIAR is zero.

Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index e675c7c8ce0e..63de77eb0ac0 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -263,9 +263,16 @@ static inline u32 perf_get_misc_flags(struct pt_regs *regs)
 	 * DD1.
 	 */
 	if (marked && (ppmu->flags & PPMU_P10_DD1)) {
-		if (is_kernel_addr(mfspr(SPRN_SIAR)))
-			return PERF_RECORD_MISC_KERNEL;
-		return PERF_RECORD_MISC_USER;
+		unsigned long siar = mfspr(SPRN_SIAR);
+		if (siar) {
+			if (is_kernel_addr(siar))
+				return PERF_RECORD_MISC_KERNEL;
+			return PERF_RECORD_MISC_USER;
+		} else {
+			if (is_kernel_addr(regs->nip))
+				return PERF_RECORD_MISC_KERNEL;
+			return PERF_RECORD_MISC_USER;
+		}
 	}
 
 	/*
@@ -2211,8 +2218,14 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
 unsigned long perf_instruction_pointer(struct pt_regs *regs)
 {
 	bool use_siar = regs_use_siar(regs);
+	unsigned long siar = mfspr(SPRN_SIAR);
 
-	if (use_siar && siar_valid(regs))
+	if (ppmu->flags & PPMU_P10_DD1) {
+		if (siar)
+			return siar;
+		else
+			return regs->nip;
+	} else if (use_siar && siar_valid(regs))
 		return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
 	else if (use_siar)
 		return 0;		// no valid instruction pointer
-- 
2.26.2

Re: [PATCH 5/5] powerpc/perf: use regs->nip when siar is zero

[Christophe Leroy]

Le 21/10/2020 à 10:53, Madhavan Srinivasan a écrit :
> In power10 DD1, there is an issue where the
> Sampled Instruction Address Register (SIAR)
> not latching to the sampled address during
> random sampling. This results in value of 0s
> in the SIAR. Patch adds a check to use regs->nip
> when SIAR is zero.

Why not use regs->nip at all time in that case, and not read SPRN_SIAR at all ?

Christophe


> 
> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> ---
>   arch/powerpc/perf/core-book3s.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index e675c7c8ce0e..63de77eb0ac0 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -263,9 +263,16 @@ static inline u32 perf_get_misc_flags(struct pt_regs *regs)
>   	 * DD1.
>   	 */
>   	if (marked && (ppmu->flags & PPMU_P10_DD1)) {
> -		if (is_kernel_addr(mfspr(SPRN_SIAR)))
> -			return PERF_RECORD_MISC_KERNEL;
> -		return PERF_RECORD_MISC_USER;
> +		unsigned long siar = mfspr(SPRN_SIAR);
> +		if (siar) {
> +			if (is_kernel_addr(siar))
> +				return PERF_RECORD_MISC_KERNEL;
> +			return PERF_RECORD_MISC_USER;
> +		} else {
> +			if (is_kernel_addr(regs->nip))
> +				return PERF_RECORD_MISC_KERNEL;
> +			return PERF_RECORD_MISC_USER;
> +		}
>   	}
>   
>   	/*
> @@ -2211,8 +2218,14 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
>   unsigned long perf_instruction_pointer(struct pt_regs *regs)
>   {
>   	bool use_siar = regs_use_siar(regs);
> +	unsigned long siar = mfspr(SPRN_SIAR);
>   
> -	if (use_siar && siar_valid(regs))
> +	if (ppmu->flags & PPMU_P10_DD1) {
> +		if (siar)
> +			return siar;
> +		else
> +			return regs->nip;
> +	} else if (use_siar && siar_valid(regs))
>   		return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
>   	else if (use_siar)
>   		return 0;		// no valid instruction pointer
> 

Re: [PATCH 5/5] powerpc/perf: use regs->nip when siar is zero

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 21/10/2020 à 10:53, Madhavan Srinivasan a écrit :
>> In power10 DD1, there is an issue where the
>> Sampled Instruction Address Register (SIAR)
>> not latching to the sampled address during
>> random sampling. This results in value of 0s
>> in the SIAR. Patch adds a check to use regs->nip
>> when SIAR is zero.
>
> Why not use regs->nip at all time in that case, and not read SPRN_SIAR at all ?

Yeah that's a reasonable question.

I can't really find anywhere in the ISA that explains it.

I believe the main (or only?) reason is that interrupts might be
disabled when the PMU samples the instruction. So in that case the SIAR
will point at an instruction somewhere in interrupts-off code, whereas
the NIP will point to the location where we re-enabled interrupts and
took the PMU interrupt.

cheers

Re: [PATCH 5/5] powerpc/perf: use regs->nip when siar is zero

[Madhavan Srinivasan]

On 10/22/20 6:55 AM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 21/10/2020 à 10:53, Madhavan Srinivasan a écrit :
>>> In power10 DD1, there is an issue where the
>>> Sampled Instruction Address Register (SIAR)
>>> not latching to the sampled address during
>>> random sampling. This results in value of 0s
>>> in the SIAR. Patch adds a check to use regs->nip
>>> when SIAR is zero.
>> Why not use regs->nip at all time in that case, and not read SPRN_SIAR at all ?
> Yeah that's a reasonable question.
>
> I can't really find anywhere in the ISA that explains it.
>
> I believe the main (or only?) reason is that interrupts might be
> disabled when the PMU samples the instruction. So in that case the SIAR
> will point at an instruction somewhere in interrupts-off code, whereas
> the NIP will point to the location where we re-enabled interrupts and
> took the PMU interrupt.

sorry for the delayed response, was out.

thats correct and also we see SIAR zeroing only for some of
the events. We still want to use the SIAR address for
the sample creation and use nip only if it  zero.

Maddy


>
> cheers