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

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

Madhavan Srinivasan [off-list ref] writes:

quoted

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.

quoted

+		(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

quoted

+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.

quoted

+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.

quoted

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