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

[Anjali K <anjalik@linux.ibm.com>]

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