linux-kernel - Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220216132257.GB56419@leoy-ThinkPad-X240s>
Date:   Wed, 16 Feb 2022 21:22:57 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     German Gomez <german.gomez@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        will@...nel.org, mark.rutland@....com, james.clark@....com
Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register
 bit CX

On Tue, Feb 15, 2022 at 02:29:27PM +0000, German Gomez wrote:

[...]

> Thanks for the suggestion. I recorded the following stacktrace:
> 
>  perf-323841 [052] d.... 3996.528812: arm_spe_pmu_setup_aux: (arm_spe_pmu_setup_aux+0x60/0x1c0 [arm_spe_pmu])
>  perf-323841 [052] d.... 3996.528813: <stack trace>

Yeah, this show arm_spe_pmu_setup_aux() is called in perf process.

>  => kprobe_dispatcher
>  => kprobe_breakpoint_handler
>  => call_break_hook
>  => brk_handler
>  => do_debug_exception
>  => el1_dbg
>  => el1h_64_sync_handler
>  => el1h_64_sync
>  => arm_spe_pmu_setup_aux
>  => perf_mmap
>  => mmap_region
>  => do_mmap
>  => vm_mmap_pgoff
>  => ksys_mmap_pgoff
>  => __arm64_sys_mmap
>  => invoke_syscall
>  => el0_svc_common.constprop.0
>  => do_el0_svc
>  => el0_svc
>  => el0t_64_sync_handler
>  => el0t_64_sync
> 
> So for a v2 I may include something like this:

The change looks good to me, please see below minor comment.

> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index d44bcc29d..aadec5a0e 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -45,6 +45,7 @@ struct arm_spe_pmu_buf {
>      int                    nr_pages;
>      bool                    snapshot;
>      void                    *base;
> +    u64                    pmscr;
>  };
>  
>  struct arm_spe_pmu {
> @@ -748,7 +749,7 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>          write_sysreg_s(reg, SYS_PMSICR_EL1);
>      }
>  
> -    reg = arm_spe_event_to_pmscr(event);
> +    reg = ((struct arm_spe_pmu_buf *) perf_get_aux(handle))->pmscr;
>      isb();
>      write_sysreg_s(reg, SYS_PMSCR_EL1);

Just nitpick (or it's just my preferring coding style), we can define
a local pointer variable 'buf':

  struct arm_spe_pmu_buf *buf = perf_get_aux(handle);

  ...

  isb();
  write_sysreg_s(buf->pmscr, SYS_PMSCR_EL1);

Thanks,
Leo

>  }
> @@ -855,6 +856,8 @@ static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
>      if (!pglist)
>          goto out_free_buf;
>  
> +    buf->pmscr = arm_spe_event_to_pmscr(event);
> +
>      for (i = 0; i < nr_pages; ++i)
>          pglist[i] = virt_to_page(pages[i]);
> 
> >
> > Thanks,
> > Leo