On 24/05/24 12:55, Michael Ellerman wrote:
> Hi Anjali,
>
> Anjali K <anja...@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 <anja...@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
