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
