Re: [PATCH V7 5/6] arm64/perf: Add branch stack support in ARMV8 PMU

[Anshuman Khandual]

On 2/23/23 19:17, Mark Rutland wrote:
> On Mon, Feb 13, 2023 at 01:53:56PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 2/9/23 01:06, Mark Rutland wrote:
>>> On Fri, Jan 13, 2023 at 10:41:51AM +0530, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 1/12/23 19:59, Mark Rutland wrote:
>>>>> On Thu, Jan 05, 2023 at 08:40:38AM +0530, Anshuman Khandual wrote:
>>>>>> @@ -878,6 +890,13 @@ static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu)
>>>>>>  		if (!armpmu_event_set_period(event))
>>>>>>  			continue;
>>>>>>  
>>>>>> +		if (has_branch_stack(event)) {
>>>>>> +			WARN_ON(!cpuc->branches);
>>>>>> +			armv8pmu_branch_read(cpuc, event);
>>>>>> +			data.br_stack = &cpuc->branches->branch_stack;
>>>>>> +			data.sample_flags |= PERF_SAMPLE_BRANCH_STACK;
>>>>>> +		}
>>>>>
>>>>> How do we ensure the data we're getting isn't changed under our feet? Is BRBE
>>>>> disabled at this point?
>>>>
>>>> Right, BRBE is paused after a PMU IRQ. We also ensure the buffer is disabled for
>>>> all exception levels, i.e removing BRBCR_EL1_E0BRE/E1BRE from the configuration,
>>>> before initiating the actual read, which eventually populates the data.br_stack.
>>>
>>> Ok; just to confirm, what exactly is the condition that enforces that BRBE is
>>> disabled? Is that *while* there's an overflow asserted, or does something else
>>> get set at the instant the overflow occurs?
>>
>> - BRBE can be disabled completely via BRBCR_EL1_E0BRE/E1BRE irrespective of PMU interrupt
>> - But with PMU interrupt, it just pauses if BRBCR_EL1_FZP is enabled
> 
> IIUC the distinction between "disabled completely" and "just pauses" doesn't
> really matter to us, and a pause is sufficient for use to be able to read and
> manipulate the records.
As I learned from the HW folks earlier, but seems like we might have to revisit
this understanding once again.

'Pause' state ensures that no new branch records could not get into the buffer
which is necessary, but not sufficient enough condition before all the branch
records could be processed in software. BRBE "disabled completely" via putting
in prohibited region (implicitly during PMU interrupt while tracing user only
sessions, explicitly while tracing user/kernel/hv sessions) is still necessary.

> 
> I also note that we always set BRBCR_EL1.FZP.
> 
> Am I missing something?

We always set BRBCR_EL1.FZP, but during PMU interrupt while processing branch
records, there are certain distinctions. 

user only traces:

	- Ensuring BRBFCR_EL1_PAUSED being set is not necessary
	- BRBE is already in prohibited region (IRQ handler in EL1)
	- Exception transition serve as a context synchronization event
	- Branch records can be read and processed right away
	- Return after clearing BRBFCR_EL1_PAUSED followed by BRB_IALL
	- isb() is not even necessary before returning 
	- ERET from EL1 will ensure a context a synchronization event

privilege traces:
	
	- Ensuring BRBFCR_EL1_PAUSED is necessary
	- Ensuring BRBE is in prohibited state - SW clears BRBCR_EL1_E1BR
	- isb() is required to ensure BRBE is prohibited state before reading
	- Return after clearing BRBFCR_EL1_PAUSED followed by BRB_IALL
	- isb() is required while returning from IRQ handler

I had suggested differentiating user only sessions, because it can save multiple
isb() instances and register write accesses which is not possible for privilege
trace sessions.

- Anshuman