linux-kernel - Re: [PATCH V12 08/10] arm64/perf: Add struct brbe

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] [day] [month] [year] [list]

Message-ID: <4dc97e9f-e103-9120-373d-8f4f472e8332@arm.com>
Date:   Thu, 22 Jun 2023 07:37:35 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        will@...nel.org, catalin.marinas@....com,
        Mark Brown <broonie@...nel.org>,
        James Clark <james.clark@....com>,
        Rob Herring <robh@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Suzuki Poulose <suzuki.poulose@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH V12 08/10] arm64/perf: Add struct brbe_regset helper
 functions



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@....com>
> 
> 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@....com>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> Tested-by: James Clark <james.clark@....com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>>  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.