perf: arm_pmuv3: Zero initialize hw_id branch stack field

[PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field

Posted by James Clark 1 month, 3 weeks ago

PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
userspace, but it's never set by the BRBE driver. Zero initialize it as
it should be according to the docs:

   * For the architectures whose raw branch records are
   * already stored in age order, the hw_idx should be 0.

It's probably too risky to remove PERF_SAMPLE_BRANCH_HW_INDEX from BRBE
now in case anyone is setting it and reading the value, but zero
initializing the whole struct also protects against the same issue with
new fields that are added in the future.

Fixes: 58074a0fce66 ("perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)")
Signed-off-by: James Clark <james.clark@linaro.org>
---
Very small fix spotted by Sashiko. It was probably always zero during
testing or never looked at.
---
 drivers/perf/arm_pmuv3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
index 8014ff766cff..b9a8592bf112 100644
--- a/drivers/perf/arm_pmuv3.c
+++ b/drivers/perf/arm_pmuv3.c
@@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
 		struct pmu_hw_events *events_cpu;
 
 		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
-		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
+		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);
 		if (!events_cpu->branch_stack)
 			return -ENOMEM;
 	}

---
base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
change-id: 20260807-james-brbe-init-hw-idx-53dec54fe266

Best regards,
--  
James Clark <james.clark@linaro.org>

Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field

Posted by Anshuman Khandual 1 month, 3 weeks ago

On 07/08/26 2:44 PM, James Clark wrote:
> PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
> userspace, but it's never set by the BRBE driver. Zero initialize it as
> it should be according to the docs:
> 
>    * For the architectures whose raw branch records are
>    * already stored in age order, the hw_idx should be 0.

The in code documentation while defining perf_branch_stack.
Probably a good idea to specify the same above.

 * For the architectures whose raw branch records are
 * already stored in age order, the hw_idx should be 0.
 */
struct perf_branch_stack {
	u64				nr;
	u64				hw_idx;
	struct perf_branch_entry	entries[];
};

> 
> It's probably too risky to remove PERF_SAMPLE_BRANCH_HW_INDEX from BRBE
> now in case anyone is setting it and reading the value, but zero
> initializing the whole struct also protects against the same issue with
> new fields that are added in the future.

Agreed. Because PERF_SAMPLE_BRANCH_HW_INDEX is supported in BRBE,
hw_idx pushed to the userspace should be zero if HW never updates.
This is definitely better than dropping PERF_SAMPLE_BRANCH_HW_INDEX
flag all together to avoid breaking current users (if any).
> 
> Fixes: 58074a0fce66 ("perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)")
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
> Very small fix spotted by Sashiko. It was probably always zero during
> testing or never looked at.
> ---
>  drivers/perf/arm_pmuv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 8014ff766cff..b9a8592bf112 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
>  		struct pmu_hw_events *events_cpu;
>  
>  		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
> -		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
> +		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);
>  		if (!events_cpu->branch_stack)
>  			return -ENOMEM;
>  	}
> 
> ---
> base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
> change-id: 20260807-james-brbe-init-hw-idx-53dec54fe266
> 
> Best regards,
> --  
> James Clark <james.clark@linaro.org>
>

Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field

Posted by James Clark 1 month, 3 weeks ago


On 07/08/2026 11:44, Anshuman Khandual wrote:
> On 07/08/26 2:44 PM, James Clark wrote:
>> PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
>> userspace, but it's never set by the BRBE driver. Zero initialize it as
>> it should be according to the docs:
>>
>>     * For the architectures whose raw branch records are
>>     * already stored in age order, the hw_idx should be 0.
> 
> The in code documentation while defining perf_branch_stack.
> Probably a good idea to specify the same above.
> 
>   * For the architectures whose raw branch records are
>   * already stored in age order, the hw_idx should be 0.
>   */
> struct perf_branch_stack {
> 	u64				nr;
> 	u64				hw_idx;
> 	struct perf_branch_entry	entries[];
> };
> 

I found it easily enough. I wouldn't want to put the same comment in two 
places and risk one of them going stale. And if I take it away from one 
place and move it to the struct then it's just missing from somewhere 
else instead. So I think I'd rather leave this one.

>>
>> It's probably too risky to remove PERF_SAMPLE_BRANCH_HW_INDEX from BRBE
>> now in case anyone is setting it and reading the value, but zero
>> initializing the whole struct also protects against the same issue with
>> new fields that are added in the future.
> 
> Agreed. Because PERF_SAMPLE_BRANCH_HW_INDEX is supported in BRBE,
> hw_idx pushed to the userspace should be zero if HW never updates.
> This is definitely better than dropping PERF_SAMPLE_BRANCH_HW_INDEX
> flag all together to avoid breaking current users (if any).
>>
>> Fixes: 58074a0fce66 ("perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)")
>> Signed-off-by: James Clark <james.clark@linaro.org>
> 
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 

Thanks

>> ---
>> Very small fix spotted by Sashiko. It was probably always zero during
>> testing or never looked at.
>> ---
>>   drivers/perf/arm_pmuv3.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index 8014ff766cff..b9a8592bf112 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
>>   		struct pmu_hw_events *events_cpu;
>>   
>>   		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
>> -		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
>> +		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);
>>   		if (!events_cpu->branch_stack)
>>   			return -ENOMEM;
>>   	}
>>
>> ---
>> base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
>> change-id: 20260807-james-brbe-init-hw-idx-53dec54fe266
>>
>> Best regards,
>> --
>> James Clark <james.clark@linaro.org>
>>
>

Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field

Posted by Will Deacon 1 month, 3 weeks ago

On Fri, Aug 07, 2026 at 01:18:59PM +0100, James Clark wrote:
> 
> 
> On 07/08/2026 11:44, Anshuman Khandual wrote:
> > On 07/08/26 2:44 PM, James Clark wrote:
> > > PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
> > > userspace, but it's never set by the BRBE driver. Zero initialize it as
> > > it should be according to the docs:
> > > 
> > >     * For the architectures whose raw branch records are
> > >     * already stored in age order, the hw_idx should be 0.
> > 
> > The in code documentation while defining perf_branch_stack.
> > Probably a good idea to specify the same above.
> > 
> >   * For the architectures whose raw branch records are
> >   * already stored in age order, the hw_idx should be 0.
> >   */
> > struct perf_branch_stack {
> > 	u64				nr;
> > 	u64				hw_idx;
> > 	struct perf_branch_entry	entries[];
> > };
> > 
> 
> I found it easily enough. I wouldn't want to put the same comment in two
> places and risk one of them going stale. And if I take it away from one
> place and move it to the struct then it's just missing from somewhere else
> instead. So I think I'd rather leave this one.

Yup, I've queued it as-is.

Cheers,

Will