Re: [PATCH V12 08/10] arm64/perf: Add struct brbe_regset helper functions

[Anshuman Khandual]

On 6/21/23 18:45, Mark Rutland wrote:
> Hi Anshuman,
> 
> Thanks, this is looking much better; I just a have a couple of minor comments.
> 
> With those fixed up:
> 
> Acked-by: Mark Rutland <mark.rutland@xxxxxxx>
> 
> Mark.
> 
> On Thu, Jun 15, 2023 at 07:02:37PM +0530, Anshuman Khandual wrote:
>> The primary abstraction level for fetching branch records from BRBE HW has
>> been changed as 'struct brbe_regset', which contains storage for all three
>> BRBE registers i.e BRBSRC, BRBTGT, BRBINF. Whether branch record processing
>> happens in the task sched out path, or in the PMU IRQ handling path, these
>> registers need to be extracted from the HW. Afterwards both live and stored
>> sets need to be stitched together to create final branch records set. This
>> adds required helper functions for such operations.
>>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Tested-by: James Clark <james.clark@xxxxxxx>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>>  drivers/perf/arm_brbe.c | 127 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 127 insertions(+)
>>
>> diff --git a/drivers/perf/arm_brbe.c b/drivers/perf/arm_brbe.c
>> index 4729cb49282b..f6693699fade 100644
>> --- a/drivers/perf/arm_brbe.c
>> +++ b/drivers/perf/arm_brbe.c
>> @@ -44,6 +44,133 @@ static void select_brbe_bank(int bank)
>>  	isb();
>>  }
>>  
>> +static bool __read_brbe_regset(struct brbe_regset *entry, int idx)
>> +{
>> +	entry->brbinf = get_brbinf_reg(idx);
>> +
>> +	/*
>> +	 * There are no valid entries anymore on the buffer.
>> +	 * Abort the branch record processing to save some
>> +	 * cycles and also reduce the capture/process load
>> +	 * for the user space as well.
>> +	 */
> 
> This comment refers to the process of handling multiple entries, though it's
> only handling one entry, and I don't think we need to mention saving cycles here.
> 
> Could we please delete this comment entirely? The comment above
> capture_brbe_regset() already explains that we read until the first invalid
> entry.

Sure, will drop the comment.

> 
>> +	if (brbe_invalid(entry->brbinf))
>> +		return false;
>> +
>> +	entry->brbsrc = get_brbsrc_reg(idx);
>> +	entry->brbtgt = get_brbtgt_reg(idx);
>> +	return true;
>> +}
>> +
>> +/*
>> + * This scans over BRBE register banks and captures individual branch records
>> + * [BRBSRC, BRBTGT, BRBINF] into a pre-allocated 'struct brbe_regset' buffer,
>> + * until an invalid one gets encountered. The caller for this function needs
>> + * to ensure BRBE is an appropriate state before the records can be captured.
>> + */
> 
> Could we simplify this to:
> 
> /*
>  * Read all BRBE entries in HW until the first invalid entry.
>  *
>  * The caller must ensure that the BRBE is not concurrently modifying these
>  * entries.
>  */

Okay, will change the comment as suggested.