diff mbox series

[v2] powerpc/perf: Set cpumode flags using sample address

Message ID 20240517094607.422166-1-anjalik@linux.ibm.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] powerpc/perf: Set cpumode flags using sample address | expand

Checks

Context Check Description
snowpatch_ozlabs/github-powerpc_ppctests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_selftests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_sparse success Successfully ran 4 jobs.
snowpatch_ozlabs/github-powerpc_kernel_qemu success Successfully ran 23 jobs.
snowpatch_ozlabs/github-powerpc_clang success Successfully ran 6 jobs.

Commit Message

Anjali K May 17, 2024, 9:46 a.m. UTC 
Currently in some cases, when the sampled instruction address register
latches to a specific address during sampling, there is an inconsistency
in the privilege bits captured in the sampled event register.
For example, a snippet from the perf report on a power10 system is:
Overhead  Address             Command       Shared Object      Symbol
........  ..................  ............  .................  .......................
     2.41%  0x7fff9f94a02c      null_syscall  [unknown]          [k] 0x00007fff9f94a02c
     2.20%  0x7fff9f94a02c      null_syscall  libc.so.6          [.] syscall

perf_get_misc_flags() function looks at the privilege bits to return
the corresponding flags to be used for the address symbol and these
privilege bit details are read from the sampled event register. In the
above snippet, address "0x00007fff9f94a02c" is shown as "k" (kernel) due
to the inconsistent privilege bits captured in the sampled event register.
To address this case, the proposed fix is to additionally check whether the
sampled address is in the kernel area. Since this is specific to the latest
platform, a new pmu flag is added called "PPMU_P10" and is used to
contain the proposed fix.

Signed-off-by: Anjali K <anjalik@linux.ibm.com>
---
Changelog:
V1->V2:
Fixed the build warning reported by the kernel test bot
Added a new flag PPMU_P10 and used it instead of PPMU_ARCH_31 to restrict
the changes to the current platform (Power10)

 arch/powerpc/include/asm/perf_event_server.h |  1 +
 arch/powerpc/perf/core-book3s.c              | 43 ++++++++------------
 arch/powerpc/perf/power10-pmu.c              |  3 +-
 3 files changed, 20 insertions(+), 27 deletions(-)


base-commit: dd5a440a31fae6e459c0d6271dddd62825505361

Comments

Michael Ellerman May 24, 2024, 7:25 a.m. UTC | #1 
Hi Anjali,

Anjali K <anjalik@linux.ibm.com> writes:
> Currently in some cases, when the sampled instruction address register
> latches to a specific address during sampling, there is an inconsistency
> in the privilege bits captured in the sampled event register.
 
I don't really like "inconsistency", it's vague.

The sampled address is correct, and the privilege bits are incorrect.

If someone is offended by that wording you can direct them to me :)

> For example, a snippet from the perf report on a power10 system is:
> Overhead  Address             Command       Shared Object      Symbol
> ........  ..................  ............  .................  .......................
>      2.41%  0x7fff9f94a02c      null_syscall  [unknown]          [k] 0x00007fff9f94a02c
>      2.20%  0x7fff9f94a02c      null_syscall  libc.so.6          [.] syscall
>
> perf_get_misc_flags() function looks at the privilege bits to return
> the corresponding flags to be used for the address symbol and these
> privilege bit details are read from the sampled event register. In the
> above snippet, address "0x00007fff9f94a02c" is shown as "k" (kernel) due
> to the inconsistent privilege bits captured in the sampled event register.
 
"incorrect privilege bits"

> To address this case, the proposed fix is to additionally check whether the
 
"To address this case check whether the"

> sampled address is in the kernel area. Since this is specific to the latest
> platform, a new pmu flag is added called "PPMU_P10" and is used to
> contain the proposed fix.

You should explain why this fix replaces the existing P10_DD1 logic.

> Signed-off-by: Anjali K <anjalik@linux.ibm.com>
> ---
> Changelog:
> V1->V2:
> Fixed the build warning reported by the kernel test bot
> Added a new flag PPMU_P10 and used it instead of PPMU_ARCH_31 to restrict
> the changes to the current platform (Power10)
>
>  arch/powerpc/include/asm/perf_event_server.h |  1 +
>  arch/powerpc/perf/core-book3s.c              | 43 ++++++++------------
>  arch/powerpc/perf/power10-pmu.c              |  3 +-
>  3 files changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
> index e2221d29fdf9..12f7bfb4cab1 100644
> --- a/arch/powerpc/include/asm/perf_event_server.h
> +++ b/arch/powerpc/include/asm/perf_event_server.h
> @@ -90,6 +90,7 @@ struct power_pmu {
>  #define PPMU_ARCH_31		0x00000200 /* Has MMCR3, SIER2 and SIER3 */
>  #define PPMU_P10_DD1		0x00000400 /* Is power10 DD1 processor version */
>  #define PPMU_HAS_ATTR_CONFIG1	0x00000800 /* Using config1 attribute */
> +#define PPMU_P10			0x00001000 /* For power10 pmu */
  
Can you put PPMU_P10 immediately after PPMU_P10_DD1. It's OK to renumber
PPMU_HAS_ATTR_CONFIG1.

> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 6b5f8a94e7d8..8a2677463a73 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -266,31 +266,12 @@ 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;
> +	unsigned long siar = mfspr(SPRN_SIAR);
 
We shouldn't read SPRN_SIAR until we know it will be used.

> +	unsigned long addr;
>  
>  	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)) {
> -		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;
> -		}
> -	}
> -
>  	/*
>  	 * If we don't have flags in MMCRA, rather than using
>  	 * the MSR, we intuit the flags from the address in
> @@ -298,19 +279,29 @@ static inline u32 perf_get_misc_flags(struct pt_regs *regs)
>  	 * results
>  	 */
>  	if (ppmu->flags & PPMU_NO_SIPR) {
> -		unsigned long siar = mfspr(SPRN_SIAR);
>  		if (is_kernel_addr(siar))
>  			return PERF_RECORD_MISC_KERNEL;
>  		return PERF_RECORD_MISC_USER;
>  	}
>  
>  	/* PR has priority over HV, so order below is important */
> -	if (regs_sipr(regs))
> -		return PERF_RECORD_MISC_USER;
> -
> -	if (regs_sihv(regs) && (freeze_events_kernel != MMCR0_FCHV))
> +	if (regs_sipr(regs)) {
> +		if (!(ppmu->flags & PPMU_P10))
> +			return PERF_RECORD_MISC_USER;
> +	} else if (regs_sihv(regs) && (freeze_events_kernel != MMCR0_FCHV))
>  		return PERF_RECORD_MISC_HYPERVISOR;
>  
> +	/*
> +	 * Check the address in SIAR to identify the
> +	 * privilege levels since the SIER[MSR_HV, MSR_PR]
> +	 * bits are not set correctly in power10 sometimes
> +	 */
> +	if (ppmu->flags & PPMU_P10) {
> +		addr = siar ? siar : regs->nip;
> +		if (!is_kernel_addr(addr))
> +			return PERF_RECORD_MISC_USER;
> +	}
> +
>  	return PERF_RECORD_MISC_KERNEL;
>  }

cheers
Anjali K May 27, 2024, 9:06 a.m. UTC | #2 
On 24/05/24 12:55, Michael Ellerman wrote:
> Hi Anjali,
>
> Anjali K <anjalik@linux.ibm.com> writes:
>> Currently in some cases, when the sampled instruction address register
>> latches to a specific address during sampling, there is an inconsistency
>> in the privilege bits captured in the sampled event register.
>  
> I don't really like "inconsistency", it's vague.
>
> The sampled address is correct, and the privilege bits are incorrect.
>
> If someone is offended by that wording you can direct them to me :)
>
>> For example, a snippet from the perf report on a power10 system is:
>> Overhead  Address             Command       Shared Object      Symbol
>> ........  ..................  ............  .................  .......................
>>      2.41%  0x7fff9f94a02c      null_syscall  [unknown]          [k] 0x00007fff9f94a02c
>>      2.20%  0x7fff9f94a02c      null_syscall  libc.so.6          [.] syscall
>>
>> perf_get_misc_flags() function looks at the privilege bits to return
>> the corresponding flags to be used for the address symbol and these
>> privilege bit details are read from the sampled event register. In the
>> above snippet, address "0x00007fff9f94a02c" is shown as "k" (kernel) due
>> to the inconsistent privilege bits captured in the sampled event register.
>  
> "incorrect privilege bits"
>
>> To address this case, the proposed fix is to additionally check whether the
>  
> "To address this case check whether the"
>
>> sampled address is in the kernel area. Since this is specific to the latest
>> platform, a new pmu flag is added called "PPMU_P10" and is used to
>> contain the proposed fix.
> You should explain why this fix replaces the existing P10_DD1 logic.
>
>> Signed-off-by: Anjali K <anjalik@linux.ibm.com>
>> ---
>> Changelog:
>> V1->V2:
>> Fixed the build warning reported by the kernel test bot
>> Added a new flag PPMU_P10 and used it instead of PPMU_ARCH_31 to restrict
>> the changes to the current platform (Power10)
>>
>>  arch/powerpc/include/asm/perf_event_server.h |  1 +
>>  arch/powerpc/perf/core-book3s.c              | 43 ++++++++------------
>>  arch/powerpc/perf/power10-pmu.c              |  3 +-
>>  3 files changed, 20 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
>> index e2221d29fdf9..12f7bfb4cab1 100644
>> --- a/arch/powerpc/include/asm/perf_event_server.h
>> +++ b/arch/powerpc/include/asm/perf_event_server.h
>> @@ -90,6 +90,7 @@ struct power_pmu {
>>  #define PPMU_ARCH_31		0x00000200 /* Has MMCR3, SIER2 and SIER3 */
>>  #define PPMU_P10_DD1		0x00000400 /* Is power10 DD1 processor version */
>>  #define PPMU_HAS_ATTR_CONFIG1	0x00000800 /* Using config1 attribute */
>> +#define PPMU_P10			0x00001000 /* For power10 pmu */
>   
> Can you put PPMU_P10 immediately after PPMU_P10_DD1. It's OK to renumber
> PPMU_HAS_ATTR_CONFIG1.
>
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 6b5f8a94e7d8..8a2677463a73 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -266,31 +266,12 @@ 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;
>> +	unsigned long siar = mfspr(SPRN_SIAR);
>  
> We shouldn't read SPRN_SIAR until we know it will be used.
>
>> +	unsigned long addr;
>>  
>>  	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)) {
>> -		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;
>> -		}
>> -	}
>> -
>>  	/*
>>  	 * If we don't have flags in MMCRA, rather than using
>>  	 * the MSR, we intuit the flags from the address in
>> @@ -298,19 +279,29 @@ static inline u32 perf_get_misc_flags(struct pt_regs *regs)
>>  	 * results
>>  	 */
>>  	if (ppmu->flags & PPMU_NO_SIPR) {
>> -		unsigned long siar = mfspr(SPRN_SIAR);
>>  		if (is_kernel_addr(siar))
>>  			return PERF_RECORD_MISC_KERNEL;
>>  		return PERF_RECORD_MISC_USER;
>>  	}
>>  
>>  	/* PR has priority over HV, so order below is important */
>> -	if (regs_sipr(regs))
>> -		return PERF_RECORD_MISC_USER;
>> -
>> -	if (regs_sihv(regs) && (freeze_events_kernel != MMCR0_FCHV))
>> +	if (regs_sipr(regs)) {
>> +		if (!(ppmu->flags & PPMU_P10))
>> +			return PERF_RECORD_MISC_USER;
>> +	} else if (regs_sihv(regs) && (freeze_events_kernel != MMCR0_FCHV))
>>  		return PERF_RECORD_MISC_HYPERVISOR;
>>  
>> +	/*
>> +	 * Check the address in SIAR to identify the
>> +	 * privilege levels since the SIER[MSR_HV, MSR_PR]
>> +	 * bits are not set correctly in power10 sometimes
>> +	 */
>> +	if (ppmu->flags & PPMU_P10) {
>> +		addr = siar ? siar : regs->nip;
>> +		if (!is_kernel_addr(addr))
>> +			return PERF_RECORD_MISC_USER;
>> +	}
>> +
>>  	return PERF_RECORD_MISC_KERNEL;
>>  }
> cheers

Thank you for reviewing. I will address the review comments and send the next version of the patch.
Anjali K
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
index e2221d29fdf9..12f7bfb4cab1 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -90,6 +90,7 @@  struct power_pmu {
 #define PPMU_ARCH_31		0x00000200 /* Has MMCR3, SIER2 and SIER3 */
 #define PPMU_P10_DD1		0x00000400 /* Is power10 DD1 processor version */
 #define PPMU_HAS_ATTR_CONFIG1	0x00000800 /* Using config1 attribute */
+#define PPMU_P10			0x00001000 /* For power10 pmu */
 
 /*
  * Values for flags to get_alternatives()
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 6b5f8a94e7d8..8a2677463a73 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -266,31 +266,12 @@  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;
+	unsigned long siar = mfspr(SPRN_SIAR);
+	unsigned long addr;
 
 	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)) {
-		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;
-		}
-	}
-
 	/*
 	 * If we don't have flags in MMCRA, rather than using
 	 * the MSR, we intuit the flags from the address in
@@ -298,19 +279,29 @@  static inline u32 perf_get_misc_flags(struct pt_regs *regs)
 	 * results
 	 */
 	if (ppmu->flags & PPMU_NO_SIPR) {
-		unsigned long siar = mfspr(SPRN_SIAR);
 		if (is_kernel_addr(siar))
 			return PERF_RECORD_MISC_KERNEL;
 		return PERF_RECORD_MISC_USER;
 	}
 
 	/* PR has priority over HV, so order below is important */
-	if (regs_sipr(regs))
-		return PERF_RECORD_MISC_USER;
-
-	if (regs_sihv(regs) && (freeze_events_kernel != MMCR0_FCHV))
+	if (regs_sipr(regs)) {
+		if (!(ppmu->flags & PPMU_P10))
+			return PERF_RECORD_MISC_USER;
+	} else if (regs_sihv(regs) && (freeze_events_kernel != MMCR0_FCHV))
 		return PERF_RECORD_MISC_HYPERVISOR;
 
+	/*
+	 * Check the address in SIAR to identify the
+	 * privilege levels since the SIER[MSR_HV, MSR_PR]
+	 * bits are not set correctly in power10 sometimes
+	 */
+	if (ppmu->flags & PPMU_P10) {
+		addr = siar ? siar : regs->nip;
+		if (!is_kernel_addr(addr))
+			return PERF_RECORD_MISC_USER;
+	}
+
 	return PERF_RECORD_MISC_KERNEL;
 }
 
diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index 62a68b6b2d4b..bb57b7cfe640 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c
@@ -593,7 +593,8 @@  static struct power_pmu power10_pmu = {
 	.get_mem_weight		= isa207_get_mem_weight,
 	.disable_pmc		= isa207_disable_pmc,
 	.flags			= PPMU_HAS_SIER | PPMU_ARCH_207S |
-				  PPMU_ARCH_31 | PPMU_HAS_ATTR_CONFIG1,
+				  PPMU_ARCH_31 | PPMU_HAS_ATTR_CONFIG1 |
+				  PPMU_P10,
 	.n_generic		= ARRAY_SIZE(power10_generic_events),
 	.generic_events		= power10_generic_events,
 	.cache_events		= &power10_cache_events,