| Message ID | 1480296076-28880-5-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes: > diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c > index 2a2040ea5f99..e747bbf06661 100644 > --- a/arch/powerpc/perf/isa207-common.c > +++ b/arch/powerpc/perf/isa207-common.c > @@ -55,6 +55,81 @@ static inline bool event_is_fab_match(u64 event) > return (event == 0x30056 || event == 0x4f052); > } > > +static bool is_event_valid(u64 event) > +{ > + if (cpu_has_feature(CPU_FTR_ARCH_300) && > + (cpu_has_feature(CPU_FTR_POWER9_DD1)) && You don't need ARCH_300 in these checks. POWER9_DD1 implies ARCH_300. And the way you've written it you have two arms that use EVENT_VALID_MASK, which is confusing. > + (event & ~EVENT_VALID_MASK)) > + return false; > + else if (cpu_has_feature(CPU_FTR_ARCH_300) && > + (event & ~ISA300_EVENT_VALID_MASK)) > + return false; > + else if (event & ~EVENT_VALID_MASK) > + return false; > + > + return true; > +} I think it would read better as: u64 valid_mask = EVENT_VALID_MASK; if (cpu_has_feature(CPU_FTR_ARCH_300) && !cpu_has_feature(CPU_FTR_POWER9_DD1)) valid_mask = ISA300_EVENT_VALID_MASK; return !(event & ~valid_mask); > +static u64 mmcra_sdar_mode(u64 event) > +{ > + u64 sm; > + > + if (cpu_has_feature(CPU_FTR_ARCH_300) && > + (cpu_has_feature(CPU_FTR_POWER9_DD1))) { > + goto sm_tlb; > + } else if (cpu_has_feature(CPU_FTR_ARCH_300)) { > + sm = (event >> ISA300_SDAR_MODE_SHIFT) & ISA300_SDAR_MODE_MASK; > + if (sm) > + return sm<<MMCRA_SDAR_MODE_SHIFT; > + } else > + goto sm_tlb; > + > +sm_tlb: > + return MMCRA_SDAR_MODE_TLB; > +} You should not need a goto to implement that logic. > +static u64 thresh_cmp_val(u64 value) > +{ > + if (cpu_has_feature(CPU_FTR_ARCH_300) && > + (cpu_has_feature(CPU_FTR_POWER9_DD1))) > + goto thr_cmp; > + else if (cpu_has_feature(CPU_FTR_ARCH_300)) > + return value<<ISA300_MMCRA_THR_CMP_SHIFT; > + else > + goto thr_cmp; > +thr_cmp: > + return value<<MMCRA_THR_CMP_SHIFT; > +} And similarly for this one and all the others. > diff --git a/arch/powerpc/perf/isa207-common.h b/arch/powerpc/perf/isa207-common.h > index 4d0a4e5017c2..0a240635cf48 100644 > --- a/arch/powerpc/perf/isa207-common.h > +++ b/arch/powerpc/perf/isa207-common.h > @@ -134,6 +134,24 @@ > PERF_SAMPLE_BRANCH_KERNEL |\ > PERF_SAMPLE_BRANCH_HV) > > +/* Contants to support PowerISA v3.0 encoding format */ > +#define ISA300_EVENT_COMBINE_SHIFT 10 /* Combine bit */ > +#define ISA300_EVENT_COMBINE_MASK 0x3ull > +#define ISA300_SDAR_MODE_SHIFT 50 > +#define ISA300_SDAR_MODE_MASK 0x3ull As I said in the previous patch, calling these P9 would be more accurate I think. And shorter. cheers
On Tuesday 29 November 2016 10:15 AM, Michael Ellerman wrote: > Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes: > >> diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c >> index 2a2040ea5f99..e747bbf06661 100644 >> --- a/arch/powerpc/perf/isa207-common.c >> +++ b/arch/powerpc/perf/isa207-common.c >> @@ -55,6 +55,81 @@ static inline bool event_is_fab_match(u64 event) >> return (event == 0x30056 || event == 0x4f052); >> } >> >> +static bool is_event_valid(u64 event) >> +{ >> + if (cpu_has_feature(CPU_FTR_ARCH_300) && >> + (cpu_has_feature(CPU_FTR_POWER9_DD1)) && > You don't need ARCH_300 in these checks. > > POWER9_DD1 implies ARCH_300. > > And the way you've written it you have two arms that use > EVENT_VALID_MASK, which is confusing. > >> + (event & ~EVENT_VALID_MASK)) >> + return false; >> + else if (cpu_has_feature(CPU_FTR_ARCH_300) && >> + (event & ~ISA300_EVENT_VALID_MASK)) >> + return false; >> + else if (event & ~EVENT_VALID_MASK) >> + return false; >> + >> + return true; >> +} > I think it would read better as: > > u64 valid_mask = EVENT_VALID_MASK; > > if (cpu_has_feature(CPU_FTR_ARCH_300) && !cpu_has_feature(CPU_FTR_POWER9_DD1)) > valid_mask = ISA300_EVENT_VALID_MASK; > > return !(event & ~valid_mask); Yes. This is much better. Will make the changes Thanks for review Maddy >> +static u64 mmcra_sdar_mode(u64 event) >> +{ >> + u64 sm; >> + >> + if (cpu_has_feature(CPU_FTR_ARCH_300) && >> + (cpu_has_feature(CPU_FTR_POWER9_DD1))) { >> + goto sm_tlb; >> + } else if (cpu_has_feature(CPU_FTR_ARCH_300)) { >> + sm = (event >> ISA300_SDAR_MODE_SHIFT) & ISA300_SDAR_MODE_MASK; >> + if (sm) >> + return sm<<MMCRA_SDAR_MODE_SHIFT; >> + } else >> + goto sm_tlb; >> + >> +sm_tlb: >> + return MMCRA_SDAR_MODE_TLB; >> +} > You should not need a goto to implement that logic. > >> +static u64 thresh_cmp_val(u64 value) >> +{ >> + if (cpu_has_feature(CPU_FTR_ARCH_300) && >> + (cpu_has_feature(CPU_FTR_POWER9_DD1))) >> + goto thr_cmp; >> + else if (cpu_has_feature(CPU_FTR_ARCH_300)) >> + return value<<ISA300_MMCRA_THR_CMP_SHIFT; >> + else >> + goto thr_cmp; >> +thr_cmp: >> + return value<<MMCRA_THR_CMP_SHIFT; >> +} > And similarly for this one and all the others. > >> diff --git a/arch/powerpc/perf/isa207-common.h b/arch/powerpc/perf/isa207-common.h >> index 4d0a4e5017c2..0a240635cf48 100644 >> --- a/arch/powerpc/perf/isa207-common.h >> +++ b/arch/powerpc/perf/isa207-common.h >> @@ -134,6 +134,24 @@ >> PERF_SAMPLE_BRANCH_KERNEL |\ >> PERF_SAMPLE_BRANCH_HV) >> >> +/* Contants to support PowerISA v3.0 encoding format */ >> +#define ISA300_EVENT_COMBINE_SHIFT 10 /* Combine bit */ >> +#define ISA300_EVENT_COMBINE_MASK 0x3ull >> +#define ISA300_SDAR_MODE_SHIFT 50 >> +#define ISA300_SDAR_MODE_MASK 0x3ull > As I said in the previous patch, calling these P9 would be more accurate > I think. And shorter. > > > cheers >
diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c index 2a2040ea5f99..e747bbf06661 100644 --- a/arch/powerpc/perf/isa207-common.c +++ b/arch/powerpc/perf/isa207-common.c @@ -55,6 +55,81 @@ static inline bool event_is_fab_match(u64 event) return (event == 0x30056 || event == 0x4f052); } +static bool is_event_valid(u64 event) +{ + if (cpu_has_feature(CPU_FTR_ARCH_300) && + (cpu_has_feature(CPU_FTR_POWER9_DD1)) && + (event & ~EVENT_VALID_MASK)) + return false; + else if (cpu_has_feature(CPU_FTR_ARCH_300) && + (event & ~ISA300_EVENT_VALID_MASK)) + return false; + else if (event & ~EVENT_VALID_MASK) + return false; + + return true; +} + +static u64 mmcra_sdar_mode(u64 event) +{ + u64 sm; + + if (cpu_has_feature(CPU_FTR_ARCH_300) && + (cpu_has_feature(CPU_FTR_POWER9_DD1))) { + goto sm_tlb; + } else if (cpu_has_feature(CPU_FTR_ARCH_300)) { + sm = (event >> ISA300_SDAR_MODE_SHIFT) & ISA300_SDAR_MODE_MASK; + if (sm) + return sm<<MMCRA_SDAR_MODE_SHIFT; + } else + goto sm_tlb; + +sm_tlb: + return MMCRA_SDAR_MODE_TLB; +} + +static u64 thresh_cmp_val(u64 value) +{ + if (cpu_has_feature(CPU_FTR_ARCH_300) && + (cpu_has_feature(CPU_FTR_POWER9_DD1))) + goto thr_cmp; + else if (cpu_has_feature(CPU_FTR_ARCH_300)) + return value<<ISA300_MMCRA_THR_CMP_SHIFT; + else + goto thr_cmp; +thr_cmp: + return value<<MMCRA_THR_CMP_SHIFT; +} + +static unsigned long combine_from_event(u64 event) +{ + unsigned long combine; + + if (cpu_has_feature(CPU_FTR_ARCH_300) && + (cpu_has_feature(CPU_FTR_POWER9_DD1))) + combine = (event >> EVENT_COMBINE_SHIFT) & EVENT_COMBINE_MASK; + else if (cpu_has_feature(CPU_FTR_ARCH_300)) + combine = (event >> ISA300_EVENT_COMBINE_SHIFT) & ISA300_EVENT_COMBINE_MASK; + else + combine = (event >> EVENT_COMBINE_SHIFT) & EVENT_COMBINE_MASK; + + return combine; +} + +static unsigned long combine_shift(unsigned long pmc) +{ + if (cpu_has_feature(CPU_FTR_ARCH_300) && + (cpu_has_feature(CPU_FTR_POWER9_DD1))) + goto comb_shift; + else if (cpu_has_feature(CPU_FTR_ARCH_300)) + return ISA300_MMCR1_COMBINE_SHIFT(pmc); + else + goto comb_shift; + +comb_shift: + return MMCR1_COMBINE_SHIFT(pmc); +} + int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp) { unsigned int unit, pmc, cache, ebb; @@ -62,7 +137,7 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp) mask = value = 0; - if (event & ~EVENT_VALID_MASK) + if (!is_event_valid(event)) return -1; pmc = (event >> EVENT_PMC_SHIFT) & EVENT_PMC_MASK; @@ -189,15 +264,13 @@ int isa207_compute_mmcr(u64 event[], int n_ev, pmc_inuse |= 1 << pmc; } - /* In continuous sampling mode, update SDAR on TLB miss */ - mmcra = MMCRA_SDAR_MODE_TLB; - mmcr1 = mmcr2 = 0; + mmcra = mmcr1 = mmcr2 = 0; /* Second pass: assign PMCs, set all MMCR1 fields */ for (i = 0; i < n_ev; ++i) { pmc = (event[i] >> EVENT_PMC_SHIFT) & EVENT_PMC_MASK; unit = (event[i] >> EVENT_UNIT_SHIFT) & EVENT_UNIT_MASK; - combine = (event[i] >> EVENT_COMBINE_SHIFT) & EVENT_COMBINE_MASK; + combine = combine_from_event(event[i]); psel = event[i] & EVENT_PSEL_MASK; if (!pmc) { @@ -211,10 +284,13 @@ int isa207_compute_mmcr(u64 event[], int n_ev, if (pmc <= 4) { mmcr1 |= unit << MMCR1_UNIT_SHIFT(pmc); - mmcr1 |= combine << MMCR1_COMBINE_SHIFT(pmc); + mmcr1 |= combine << combine_shift(pmc); mmcr1 |= psel << MMCR1_PMCSEL_SHIFT(pmc); } + /* In continuous sampling mode, update SDAR on TLB miss */ + mmcra |= mmcra_sdar_mode(event[i]); + if (event[i] & EVENT_IS_L1) { cache = event[i] >> EVENT_CACHE_SEL_SHIFT; mmcr1 |= (cache & 1) << MMCR1_IC_QUAL_SHIFT; @@ -245,7 +321,7 @@ int isa207_compute_mmcr(u64 event[], int n_ev, val = (event[i] >> EVENT_THR_SEL_SHIFT) & EVENT_THR_SEL_MASK; mmcra |= val << MMCRA_THR_SEL_SHIFT; val = (event[i] >> EVENT_THR_CMP_SHIFT) & EVENT_THR_CMP_MASK; - mmcra |= val << MMCRA_THR_CMP_SHIFT; + mmcra |= thresh_cmp_val(val); } if (event[i] & EVENT_WANTS_BHRB) { diff --git a/arch/powerpc/perf/isa207-common.h b/arch/powerpc/perf/isa207-common.h index 4d0a4e5017c2..0a240635cf48 100644 --- a/arch/powerpc/perf/isa207-common.h +++ b/arch/powerpc/perf/isa207-common.h @@ -134,6 +134,24 @@ PERF_SAMPLE_BRANCH_KERNEL |\ PERF_SAMPLE_BRANCH_HV) +/* Contants to support PowerISA v3.0 encoding format */ +#define ISA300_EVENT_COMBINE_SHIFT 10 /* Combine bit */ +#define ISA300_EVENT_COMBINE_MASK 0x3ull +#define ISA300_SDAR_MODE_SHIFT 50 +#define ISA300_SDAR_MODE_MASK 0x3ull + +#define ISA300_EVENT_VALID_MASK \ + ((ISA300_SDAR_MODE_MASK<< ISA300_SDAR_MODE_SHIFT | \ + (EVENT_THRESH_MASK << EVENT_THRESH_SHIFT) | \ + (EVENT_SAMPLE_MASK << EVENT_SAMPLE_SHIFT) | \ + (EVENT_CACHE_SEL_MASK << EVENT_CACHE_SEL_SHIFT) | \ + (EVENT_PMC_MASK << EVENT_PMC_SHIFT) | \ + (EVENT_UNIT_MASK << EVENT_UNIT_SHIFT) | \ + (ISA300_EVENT_COMBINE_MASK << ISA300_EVENT_COMBINE_SHIFT) | \ + (EVENT_MARKED_MASK << EVENT_MARKED_SHIFT) | \ + EVENT_LINUX_MASK | \ + EVENT_PSEL_MASK)) + /* * Layout of constraint bits: * @@ -210,15 +228,22 @@ #define MMCR1_DC_QUAL_SHIFT 47 #define MMCR1_IC_QUAL_SHIFT 46 +/* MMCR1 Combine bits macro for PowerISA v3.0 */ +#define ISA300_MMCR1_COMBINE_SHIFT(pmc) (38 - ((pmc - 1) * 2)) + /* Bits in MMCRA for PowerISA v2.07 */ #define MMCRA_SAMP_MODE_SHIFT 1 #define MMCRA_SAMP_ELIG_SHIFT 4 #define MMCRA_THR_CTL_SHIFT 8 #define MMCRA_THR_SEL_SHIFT 16 #define MMCRA_THR_CMP_SHIFT 32 -#define MMCRA_SDAR_MODE_TLB (1ull << 42) +#define MMCRA_SDAR_MODE_SHIFT 42 +#define MMCRA_SDAR_MODE_TLB (1ull << MMCRA_SDAR_MODE_SHIFT) #define MMCRA_IFM_SHIFT 30 +/* MMCR1 Threshold Compare bit constant for PowerISA v3.0 */ +#define ISA300_MMCRA_THR_CMP_SHIFT 45 + /* Bits in MMCR2 for PowerISA v2.07 */ #define MMCR2_FCS(pmc) (1ull << (63 - (((pmc) - 1) * 9))) #define MMCR2_FCP(pmc) (1ull << (62 - (((pmc) - 1) * 9)))
Patch to add macros and contants to support the PowerISA v3.0 raw event encoding format. Couple of functions added since some of the bits fields like PMCxCOMB and THRESH_CMP has different width and location within MMCR* in PowerISA v3.0. Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- arch/powerpc/perf/isa207-common.c | 90 ++++++++++++++++++++++++++++++++++++--- arch/powerpc/perf/isa207-common.h | 27 +++++++++++- 2 files changed, 109 insertions(+), 8 deletions(-)