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

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

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
>