Re: [PATCH V5 6/7] arm64/perf: Add BRBE driver

[James Clark]

On 17/11/2022 05:45, Anshuman Khandual wrote:
> 
> 
> On 11/16/22 22:12, James Clark wrote:
>>
>>
>> On 07/11/2022 06:25, Anshuman Khandual wrote:
>> [...]
>>
>>> +static void perf_branch_to_brbcr(struct pmu_hw_events *cpuc, int branch_type)
>>> +{
>>> +	cpuc->brbcr = (BRBCR_EL1_CC | BRBCR_EL1_MPRED);
>>> +
>>> +	if (branch_type & PERF_SAMPLE_BRANCH_USER)
>>> +		cpuc->brbcr |= BRBCR_EL1_E0BRE;
>>> +
>>> +	if (branch_type & PERF_SAMPLE_BRANCH_NO_CYCLES)
>>> +		cpuc->brbcr &= ~BRBCR_EL1_CC;
>>> +
>>> +	if (branch_type & PERF_SAMPLE_BRANCH_NO_FLAGS)
>>> +		cpuc->brbcr &= ~BRBCR_EL1_MPRED;
>>> +
>>> +	if (branch_type & PERF_SAMPLE_BRANCH_KERNEL)
>>> +		cpuc->brbcr |= BRBCR_EL1_E1BRE;
>>> +	else
>>> +		return;
>>> +
>>> +	/*
>>> +	 * The exception and exception return branches could be
>>> +	 * captured only when the event has necessary privilege
>>> +	 * indicated via branch type PERF_SAMPLE_BRANCH_KERNEL,
>>> +	 * which has been ascertained in generic perf. Please
>>> +	 * refer perf_copy_attr() for more details.
>>> +	 */
>>> +	if (branch_type & PERF_SAMPLE_BRANCH_ANY) {
>>> +		cpuc->brbcr |= BRBCR_EL1_EXCEPTION;
>>> +		cpuc->brbcr |= BRBCR_EL1_ERTN;
>>
>> Because this comes after the PERF_SAMPLE_BRANCH_KERNEL check, it's
>> impossible to get syscall records from userspace. When you enable kernel
>> branch records, the buffer always fills up before it gets to userspace.
> 
> Just to summerize.
> 
> System call [user_addr -> kernel_addr] and return [kernel_addr -> user_addr]
> records are impossible to be captured, because
> 
> - Without PERF_SAMPLE_BRANCH_KERNEL, BRBCR_EL1_EXCEPTION/ERTN are not set
> - With PERF_SAMPLE_BRANCH_KERNEL, buffer fills up with in kernel branches
> 

Yep that's it

> Did you try with latest fix, that clears the paused BRBE after reading branch
> records during PMU interrupt ? That fix creates much more samples than before.
> 

Yes that's with the latest fix. It may even make the problem more
obvious with the fix rather than without.

>>
>> Can you move this to the top so that it can be set if either
>> PERF_SAMPLE_BRANCH_USER or PERF_SAMPLE_BRANCH_KERNEL is set. The
> 
> Why should they depend on privilege filters i.e PERF_SAMPLE_BRANCH_USER/KERNEL
> rather than just branch filters PERF_SAMPLE_BRANCH_ANY/ANY_CALL/ANY_RETURN ?
> 

Exactly, I don't think they should depend on the privilege level. But at
the moment we return before setting them unless
PERF_SAMPLE_BRANCH_KERNEL is set.

>> hardware already handles the security by giving partial records with the
>> kernel part zeroed out so I don't think the driver needs to add any
>> additional rules other than setting BRBCR_EL1_E1BRE or BRBCR_EL1_E0BRE.
> 
> Basically BRBCR_EL1_EXCEPTION/BRBCR_EL1_ERTN should be treated like any other
> branch filter rather than privilege filters as is the case now ?

I think so yes

> 
>>
>> For example I moved it to the top, removed the return below and then I
>> get syscall partial records:
>>
>> ....  5: 0000000000745d0c -> 0000000000000000 0 cycles  P   9fbfbfbf SYSCALL
>>
>> I also get ERETS but with only the userspace part set:
>>
>> .....  4: 0000000000000000 -> 0000000000745d10 0 cycles  P   9fbfbfbf ERET
> But with both user and kernel privilege filters being set, these should have
> been complete branch records containing both user and kernel addresses ?

Yes, but I only set PERF_SAMPLE_BRANCH_USER, I should have given the
perf command as well:

  perf record -j any,save_type,u -- syscall_loop

Where syscall_loop obviously generates lots of SYSCALLS and ERETS. But
with both user and kernel you just don't get to that point before the
buffer fills up. At least in per process mode, maybe with -a the timings
are different.

> 
>>
>>> +		return;
>>> +	}
>>> +
>>> +	if (branch_type & PERF_SAMPLE_BRANCH_ANY_CALL)
>>> +		cpuc->brbcr |= BRBCR_EL1_EXCEPTION;
>>> +
>>> +	if (branch_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
>>> +		cpuc->brbcr |= BRBCR_EL1_ERTN;
>>> +}
>>> +
> 
> [....]