On 08-Jul-2020, at 1:13 PM, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:

On Tue, Jul 07, 2020 at = 05:17:55PM +1000, Michael Neuling wrote:
On Wed, 2020-07-01 at 05:20 -0400, = Athira Rajeev wrote:
PowerISA v3.1 has few updates for the Branch History Rolling = Buffer(BHRB).
First is the addition of BHRB disable bit = and second new filtering
modes for BHRB.

BHRB disable is controlled via Monitor Mode Control Register = A (MMCRA)
bit 26, namely "BHRB Recording Disable = (BHRBRD)". This field controls
whether BHRB entries are = written when BHRB recording is enabled by other
bits. = Patch implements support for this BHRB disable bit.

Probably good to note here that = this is backwards compatible. So if you have a
kernel that = doesn't know about this bit, it'll clear it and hence you still get
BHRB.

You should also note why you'd want to do = disable this (ie. the core will run
faster).

Secondly = PowerISA v3.1 introduce filtering support for
PERF_SAMPLE_BRANCH_IND_CALL/COND. The patch adds BHRB filter = support
for "ind_call" and "cond" in = power10_bhrb_filter_map().

'commit = bb19af816025 ("powerpc/perf: Prevent kernel address leak to userspace
via BHRB buffer")'
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]=3D1 address to be written to BHRB buffer.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
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 ++++++++++++++

This touches the idle code so we = should get those guys on CC (adding Vaidy and
Ego).

4 files = changed, 59 insertions(+), 8 deletions(-)

[..snip..]

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 =3D 0;
+ = unsigned long mmcra_bhrb =3D 0;

We are saving the whole of MMCRA aren't we ? We might want to = just
name it mmcra = in that case.

struct = p9_sprs sprs =3D {}; /* avoid false used-uninitialised */
= bool sprs_saved =3D false;

@@ -657,6 = +658,15 @@ static unsigned long power9_idle_stop(unsigned long
psscr, bool mmu_on)
  */
= mmcr0 = = =3D mfspr(SPRN_MMCR0);
}
+
+ = if (cpu_has_feature(CPU_FTR_ARCH_31)) {
+ /* = POWER10 uses MMCRA[:26] as BHRB disable bit
+ * to disable BHRB logic = when not used. Hence Save and
+ * restore MMCRA after a = state-loss idle.
+ */

Multi-line comment usually has the first line = blank.

Hi = Gautham

Yes, we are saving whole of = MMCRA.

= /*
= ; * Line 1
= * Line 2
* = .
= * .
* = .
= * Line N
*/

+ = = mmcra_bhrb =3D mfspr(SPRN_MMCRA);

Why is the bhrb bit of mmcra special here?

The comment above could include the consequence of not saving = and
restoring = MMCRA i.e

- If the user = hasn't asked for the BHRB to be
written the value of MMCRA[BHRBD] =3D 1.

- On wakeup from stop, = MMCRA[BHRBD] will be 0, since MMCRA is not a
previleged resource and = will be lost.

- Thus, if we = do not save and restore the MMCRA[BHRBD], the hardware
will be needlessly writing = to the BHRB in the problem mode.

+ }
+
= if ((psscr & PSSCR_RL_MASK) >=3D pnv_first_spr_loss_level) = {
sprs.lpcr =3D mfspr(SPRN_LPCR);
= = sprs.hfscr =3D 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.

--
Thanks and Regards
gautham.