Re: [PATCH v2 07/10] powerpc/perf: support BHRB disable bit and new filtering modes

On 08-Jul-2020, at 5:12 PM, Michael Ellerman [off-list ref] wrote:

Athira Rajeev <atrajeev@linux.vnet.ibm.com <mailto:atrajeev@linux.vnet.ibm.com>> writes:

quoted

PowerISA v3.1 has few updates for the Branch History Rolling Buffer(BHRB).

                  ^
                  a

quoted

First is the addition of BHRB disable bit and second new filtering

                                                     ^
                                                     is

quoted

modes for BHRB.

BHRB disable is controlled via Monitor Mode Control Register A (MMCRA)
bit 26, namely "BHRB Recording Disable (BHRBRD)". This field controls

Most people call that bit 37.

quoted

whether BHRB entries are written when BHRB recording is enabled by other
bits. Patch implements support for this BHRB disable bit.

      ^
      This

quoted

Secondly PowerISA v3.1 introduce filtering support for

.. that should be in a separate patch please.

quoted

PERF_SAMPLE_BRANCH_IND_CALL/COND. The patch adds BHRB filter support

                                   ^
                                   This

quoted

for "ind_call" and "cond" in power10_bhrb_filter_map().

'commit bb19af816025 ("powerpc/perf: Prevent kernel address leak to userspace via BHRB buffer")'

That doesn't need single quotes, and should be wrapped at 72 columns
like the rest of the text.

quoted

added a check in bhrb_read() to filter the kernel address from BHRB buffer. Patch here modified
it to avoid that check for PowerISA v3.1 based processors, since PowerISA v3.1 allows
only MSR[PR]=1 address to be written to BHRB buffer.

And that should be a separate patch again please.

Sure, I will split these to separate patches

quoted

Signed-off-by: Athira Rajeev <redacted>
---
arch/powerpc/perf/core-book3s.c       | 27 +++++++++++++++++++++------
arch/powerpc/perf/isa207-common.c     | 13 +++++++++++++
arch/powerpc/perf/power10-pmu.c       | 13 +++++++++++--
arch/powerpc/platforms/powernv/idle.c | 14 ++++++++++++++
4 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index fad5159..9709606 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c

@@ -466,9 +466,13 @@ static void power_pmu_bhrb_read(struct perf_event *event, struct cpu_hw_events *

			 * addresses at this point. Check the privileges before
			 * exporting it to userspace (avoid exposure of regions
			 * where we could have speculative execution)
+			 * Incase of ISA 310, BHRB will capture only user-space

                          ^
                          In case of ISA v3.1,

Ok, 

quoted

+			 * address,hence include a check before filtering code

                          ^                                                  ^
                          addresses, hence                                   .

quoted

			 */
-			if (is_kernel_addr(addr) && perf_allow_kernel(&event->attr) != 0)
-				continue;
+			if (!(ppmu->flags & PPMU_ARCH_310S))
+				if (is_kernel_addr(addr) &&
+				perf_allow_kernel(&event->attr) != 0)
+					continue;

The indentation is weird. You should just check all three conditions
with &&.

Ok, will correct this.

quoted

			/* Branches are read most recent first (ie. mfbhrb 0 is
			 * the most recent branch).

@@ -1212,7 +1216,7 @@ static void write_mmcr0(struct cpu_hw_events *cpuhw, unsigned long mmcr0)

static void power_pmu_disable(struct pmu *pmu)
{
	struct cpu_hw_events *cpuhw;
-	unsigned long flags, mmcr0, val;
+	unsigned long flags, mmcr0, val, mmcra = 0;

You initialise it below.

quoted

	if (!ppmu)
		return;

@@ -1245,12 +1249,23 @@ static void power_pmu_disable(struct pmu *pmu)

		mb();
		isync();

+		val = mmcra = cpuhw->mmcr[2];
+

For mmcr0 (above), val is the variable we mutate and mmcr0 is the
original value. But here you've done the reverse, which is confusing.

Yes, I am altering mmcra here and using val as original value. I should have done it reverse.

quoted

		/*
		 * Disable instruction sampling if it was enabled
		 */
-		if (cpuhw->mmcr[2] & MMCRA_SAMPLE_ENABLE) {
-			mtspr(SPRN_MMCRA,
-			      cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
+		if (cpuhw->mmcr[2] & MMCRA_SAMPLE_ENABLE)
+			mmcra = cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE;

You just loaded cpuhw->mmcr[2] into mmcra, use it rather than referring
back to cpuhw->mmcr[2] over and over.

Ok,

quoted

+
+		/* Disable BHRB via mmcra [:26] for p10 if needed */
+		if (!(cpuhw->mmcr[2] & MMCRA_BHRB_DISABLE))

You don't need to check that it's clear AFAICS. Just always set disable
and the check against val below will catch the nop case.

My thought here was to avoid writing to MMCRA ( also avoid mb() and isync() ) if its not needed.
But as you suggested, since I am comparing against original value before writing, I may not need this check.
And I missed feature check here. Will correct it.
 

quoted

+			mmcra |= MMCRA_BHRB_DISABLE;
+
+		/* Write SPRN_MMCRA if mmcra has either disabled

Comment format is wrong.

quoted

+		 * instruction sampling or BHRB

Full stop please.

Sure

quoted

+		 */
+		if (val != mmcra) {
+			mtspr(SPRN_MMCRA, mmcra);
			mb();
			isync();
		}

diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
index 7d4839e..463d925 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c

@@ -404,6 +404,12 @@ int isa207_compute_mmcr(u64 event[], int n_ev,

	mmcra = mmcr1 = mmcr2 = mmcr3 = 0;

+	/* Disable bhrb unless explicitly requested
+	 * by setting MMCRA [:26] bit.
+	 */

Comment format again.

quoted

+	if (cpu_has_feature(CPU_FTR_ARCH_31))
+		mmcra |= MMCRA_BHRB_DISABLE;

Here we do a feature check before setting MMCRA_BHRB_DISABLE, but you
didn't above?

quoted

+
	/* Second pass: assign PMCs, set all MMCR1 fields */
	for (i = 0; i < n_ev; ++i) {
		pmc     = (event[i] >> EVENT_PMC_SHIFT) & EVENT_PMC_MASK;

@@ -475,10 +481,17 @@ int isa207_compute_mmcr(u64 event[], int n_ev,

		}

		if (event[i] & EVENT_WANTS_BHRB) {
+			/* set MMCRA[:26] to 0 for Power10 to enable BHRB */

"set MMCRA[:26] to 0" == "clear MMCRA[:26]”

Ok

quoted

+			if (cpu_has_feature(CPU_FTR_ARCH_31))
+				mmcra &= ~MMCRA_BHRB_DISABLE;

Newline please.

quoted

			val = (event[i] >> EVENT_IFM_SHIFT) & EVENT_IFM_MASK;
			mmcra |= val << MMCRA_IFM_SHIFT;
		}

+		/* set MMCRA[:26] to 0 if there is user request for BHRB */
+		if (cpu_has_feature(CPU_FTR_ARCH_31) && has_branch_stack(pevents[i]))
+			mmcra &= ~MMCRA_BHRB_DISABLE;
+

I think it would be cleaner if you did a single test, eg:

		if (cpu_has_feature(CPU_FTR_ARCH_31) &&
                  (has_branch_stack(pevents[i]) || (event[i] & EVENT_WANTS_BHRB)))
			mmcra &= ~MMCRA_BHRB_DISABLE;

Sure Michael

Thanks for the review. I will address all these changes in the next version

Thanks
Athira 

quoted

		if (pevents[i]->attr.exclude_user)
			mmcr2 |= MMCR2_FCP(pmc);

diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index d64d69d..07fb919 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c

@@ -82,6 +82,8 @@

/* MMCRA IFM bits - POWER10 */
#define POWER10_MMCRA_IFM1		0x0000000040000000UL
+#define POWER10_MMCRA_IFM2		0x0000000080000000UL
+#define POWER10_MMCRA_IFM3		0x00000000C0000000UL
#define POWER10_MMCRA_BHRB_MASK	0x00000000C0000000UL

/* Table of alternatives, sorted by column 0 */

@@ -233,8 +235,15 @@ static u64 power10_bhrb_filter_map(u64 branch_sample_type)

	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
		return -1;

-	if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
-		return -1;
+	if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL) {
+		pmu_bhrb_filter |= POWER10_MMCRA_IFM2;
+		return pmu_bhrb_filter;
+	}
+
+	if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
+		pmu_bhrb_filter |= POWER10_MMCRA_IFM3;
+		return pmu_bhrb_filter;
+	}

	if (branch_sample_type & PERF_SAMPLE_BRANCH_CALL)
		return -1;

diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 2dd4673..7db99c7 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c

@@ -611,6 +611,7 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)

	unsigned long srr1;
	unsigned long pls;
	unsigned long mmcr0 = 0;
+	unsigned long mmcra_bhrb = 0;
	struct p9_sprs sprs = {}; /* avoid false used-uninitialised */
	bool sprs_saved = false;

@@ -657,6 +658,15 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)

		  */
		mmcr0		= mfspr(SPRN_MMCR0);
	}
+
+	if (cpu_has_feature(CPU_FTR_ARCH_31)) {
+		/* POWER10 uses MMCRA[:26] as BHRB disable bit

Comment format.

quoted

+		 * to disable BHRB logic when not used. Hence Save and
+		 * restore MMCRA after a state-loss idle.
+		 */
+		mmcra_bhrb		= mfspr(SPRN_MMCRA);
+	}

It's the whole mmcra it should be called mmcra?

Yes, we are saving the whole mmcra. 

quoted

+
	if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
		sprs.lpcr	= mfspr(SPRN_LPCR);
		sprs.hfscr	= mfspr(SPRN_HFSCR);

@@ -721,6 +731,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)

			mtspr(SPRN_MMCR0, mmcr0);
		}

+		/* Reload MMCRA to restore BHRB disable bit for POWER10 */
+		if (cpu_has_feature(CPU_FTR_ARCH_31))
+			mtspr(SPRN_MMCRA, mmcra_bhrb);
+
		/*
		 * DD2.2 and earlier need to set then clear bit 60 in MMCRA
		 * to ensure the PMU starts running.

cheers