Re: [RFC V1 03/11] arm64/perf: Update struct arm_pmu for BRBE

[Rob Herring]

On Mon, Jan 24, 2022 at 10:00:45AM +0530, Anshuman Khandual wrote:
> This updates struct arm_pmu to include all required helpers that will drive

>From submitting-patches.rst:

Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.

> BRBE functionality for a given PMU implementation. These are the following.

Don't describe what the change is, one can read the diff for that. 
Answer why it is needed.

One thing to answer in the commit msg is why we need the hooks here.  
Have we concluded that adding BRBE hooks to struct arm_pmu for what is 
an armv8 specific feature is the right approach? I don't recall 
reaching that conclusion.

> 
> - brbe_filter	: Convert perf event filters into BRBE HW filters
> - brbe_probe	: Probe BRBE HW and capture its attributes
> - brbe_enable	: Enable BRBE HW with a given config
> - brbe_disable	: Disable BRBE HW
> - brbe_read	: Read BRBE buffer for captured branch records
> - brbe_reset	: Reset BRBE buffer
> - brbe_supported: Whether BRBE is supported or not

The function names seem pretty self-explanatory, but the text is needed, 
shouldn't it be in the struct declaration.

I'm not really a fan of patches adding dead code. That's not any less to 
review. Restructuring with 'no functional change' OTOH is helpful in 
reviewing.

> A BRBE driver implementation needs to provide these functionalities.
> 
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-perf-users@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> ---
>  arch/arm64/kernel/perf_event.c | 36 ++++++++++++++++++++++++++++++++++
>  include/linux/perf/arm_pmu.h   |  7 +++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index cab678ed6618..f6a47036b0b4 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -1015,6 +1015,35 @@ static int armv8pmu_filter_match(struct perf_event *event)
>  	return evtype != ARMV8_PMUV3_PERFCTR_CHAIN;
>  }
>  
> +static void armv8pmu_brbe_filter(struct pmu_hw_events *hw_event, struct perf_event *event)
> +{
> +}
> +
> +static void armv8pmu_brbe_enable(struct pmu_hw_events *hw_event)
> +{
> +}
> +
> +static void armv8pmu_brbe_disable(struct pmu_hw_events *hw_event)
> +{
> +}
> +
> +static void armv8pmu_brbe_read(struct pmu_hw_events *hw_event, struct perf_event *event)
> +{
> +}
> +
> +static void armv8pmu_brbe_probe(struct pmu_hw_events *hw_event)
> +{
> +}
> +
> +static void armv8pmu_brbe_reset(struct pmu_hw_events *hw_event)
> +{
> +}
> +
> +static bool armv8pmu_brbe_supported(struct perf_event *event)
> +{
> +	return false;
> +}
> +
>  static void armv8pmu_reset(void *info)
>  {
>  	struct arm_pmu *cpu_pmu = (struct arm_pmu *)info;
> @@ -1247,6 +1276,13 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>  
>  	cpu_pmu->pmu.event_idx		= armv8pmu_user_event_idx;
>  
> +	cpu_pmu->brbe_filter		= armv8pmu_brbe_filter;
> +	cpu_pmu->brbe_enable		= armv8pmu_brbe_enable;
> +	cpu_pmu->brbe_disable		= armv8pmu_brbe_disable;
> +	cpu_pmu->brbe_read		= armv8pmu_brbe_read;
> +	cpu_pmu->brbe_probe		= armv8pmu_brbe_probe;
> +	cpu_pmu->brbe_reset		= armv8pmu_brbe_reset;
> +	cpu_pmu->brbe_supported		= armv8pmu_brbe_supported;
>  	cpu_pmu->name			= name;
>  	cpu_pmu->map_event		= map_event;
>  	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = events ?
> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index 2512e2f9cd4e..c0dd0d6c5883 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -98,6 +98,13 @@ struct arm_pmu {
>  	void		(*reset)(void *);
>  	int		(*map_event)(struct perf_event *event);
>  	int		(*filter_match)(struct perf_event *event);
> +	void		(*brbe_filter)(struct pmu_hw_events *hw_events, struct perf_event *event);
> +	void		(*brbe_probe)(struct pmu_hw_events *hw_events);
> +	void		(*brbe_enable)(struct pmu_hw_events *hw_events);
> +	void		(*brbe_disable)(struct pmu_hw_events *hw_events);
> +	void		(*brbe_read)(struct pmu_hw_events *hw_events, struct perf_event *event);
> +	void		(*brbe_reset)(struct pmu_hw_events *hw_events);
> +	bool		(*brbe_supported)(struct perf_event *event);
>  	int		num_events;
>  	bool		secure_access; /* 32-bit ARM only */
>  #define ARMV8_PMUV3_MAX_COMMON_EVENTS		0x40
> -- 
> 2.25.1
> 
>