> On 08-Jul-2020, at 5:12 PM, Michael Ellerman wrote: > > Athira Rajeev > writes: > >> PowerISA v3.1 has few updates for the Branch History Rolling Buffer(BHRB). > ^ > a >> First is the addition of BHRB disable bit and second new filtering > ^ > is >> 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. > >> whether BHRB entries are written when BHRB recording is enabled by other >> bits. Patch implements support for this BHRB disable bit. > ^ > This > >> Secondly PowerISA v3.1 introduce filtering support for > > .. that should be in a separate patch please. > >> PERF_SAMPLE_BRANCH_IND_CALL/COND. The patch adds BHRB filter support > ^ > This >> 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. > >> 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 > >> Signed-off-by: Athira Rajeev >> --- >> 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, > >> + * address,hence include a check before filtering code > ^ ^ > addresses, hence . >> */ >> - 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. > >> >> /* 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. > >> 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. > >> /* >> * 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, >> + >> + /* 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. > >> + mmcra |= MMCRA_BHRB_DISABLE; >> + >> + /* Write SPRN_MMCRA if mmcra has either disabled > > Comment format is wrong. > >> + * instruction sampling or BHRB > > Full stop please. Sure > >> + */ >> + 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. > >> + 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? > >> + >> /* 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 >> + if (cpu_has_feature(CPU_FTR_ARCH_31)) >> + mmcra &= ~MMCRA_BHRB_DISABLE; > > Newline please. > >> 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 > >> 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. > >> + * 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. > >> + >> 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