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

On 2/23/23 19:17, Mark Rutland wrote:

On Mon, Feb 13, 2023 at 01:53:56PM +0530, Anshuman Khandual wrote:

quoted

On 2/9/23 01:06, Mark Rutland wrote:

quoted

On Fri, Jan 13, 2023 at 10:41:51AM +0530, Anshuman Khandual wrote:

quoted

On 1/12/23 19:59, Mark Rutland wrote:

quoted

On Thu, Jan 05, 2023 at 08:40:38AM +0530, Anshuman Khandual wrote:

quoted

@@ -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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel