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

[Michael Ellerman <mpe@ellerman.id.au>]

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