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

[Michael Ellerman]

Madhavan Srinivasan <maddy at 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