Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register
 bit CX
To:     Leo Yan <leo.yan@linaro.org>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        will@kernel.org, mark.rutland@arm.com, james.clark@arm.com
References: <20220117124432.3119132-1-german.gomez@arm.com>
 <20220117124432.3119132-2-german.gomez@arm.com>
 <20220205153940.GB391033@leoy-ThinkPad-X240s>
 <4d5951ee-d7d2-1e76-eb24-5f3c46d1662c@arm.com>
 <20220208130047.GA273989@leoy-ThinkPad-X240s>
 <e68839bc-b4f0-1fe8-1748-484254ded37a@arm.com>
 <20220211104528.GA475776@leoy-ThinkPad-X240s>
 <d59e99ad-dfc6-27ee-d952-7455d8e9fda6@arm.com>
 <20220216132257.GB56419@leoy-ThinkPad-X240s>
From:   German Gomez <german.gomez@arm.com>
Message-ID: <7d50f7ba-d8e5-1fbc-e9e6-749a24e26555@arm.com>
Date:   Wed, 16 Feb 2022 15:16:01 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20220216132257.GB56419@leoy-ThinkPad-X240s>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Hi Leo,

Thanks for the review

On 16/02/2022 13:22, Leo Yan wrote:
> 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);

I need to make sure perf_get_aux(..) is called between perf_aux_output_begin and *_end though (so, after arm_spe_perf_aux_output_begin(..)):

 buf = perf_get_aux(handle);
 reg = buf->pmscr;
 isb();
 write_sysreg_s(buf, SYS_PMSCR_EL1);

Alternatively, we set the register inside of perf_aux_output_begin. It might be confusing for casual readers because the function handles a case where perf_get_aux(..) returns NULL.

Alternatively, we could also wrap perf_get_aux(..) in a static inline function that returns the correct type and do:

 reg = arm_spe_get_aux(handle)->pmscr;
 isb();
 write_sysreg_s(reg, SYS_PMSCR_EL1);

So that it looks cleaner.

>
>   ...
>
>   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