Re: [PATCH v3 4/4] powerpc/perf: macros for PowerISA v3.0 format encoding

[Madhavan Srinivasan]

On Tuesday 29 November 2016 10:15 AM, Michael Ellerman wrote:

Madhavan Srinivasan <ma...@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