Re: [PATCH] aarch64 oprofile support [v3]

[Maynard J. <may...@us...>]

On 02/11/2014 09:37 AM, Will Deacon wrote:
> On Tue, Feb 11, 2014 at 03:28:11PM +0000, Maynard Johnson wrote:
>> On 02/11/2014 04:25 AM, Will Deacon wrote:
>>>>> @@ -394,6 +395,13 @@ static op_cpu _get_arm_cpu_type(void)
>>>>>                       return op_get_cpu_number("arm/armv7-ca9");
>>>>>               case 0xc0f:
>>>>>                       return op_get_cpu_number("arm/armv7-ca15");
>>>>> +             case 0xd00:
>>>>> +                     return op_get_cpu_number("arm/armv8-pmuv3");
>>>>> +             }
>>>>> +     } else if (vendorid == 0x50) {  /* Applied Micro Circuits Corpation */
>>>>> +             switch (cpuid) {
>>>>> +             case 0x000:
>>>>> +                     return op_get_cpu_number("arm/armv8-pmuv3");
>>>
>>> ...these strings won't hit in the events directory. In the absence of x-gene
>> Good catch!
>>> specific events, we should probably direct the latter case at
>>> "arm/armv8-pmuv3-common" and remove the first case (0xd00), since it will be
>>> updated to point at, for example, the ca57 events in future.
>> Unfortunately, with the patch that I most recently posted in this thread,
>> that won't work because calling
>> op_get_cpu_number("arm/armv8-pmuv3-common") will return CPU_NO_GOOD (note
>> below there is no cpu_descr with a name field of
>> "arm/armv8-pmuv3-common"):
>>
>> 	@@ -129,6 +129,7 @@ static struct cpu_descr const cpu_descrs[MAX_CPU_TYPE] = {
>>  		{ "e6500", "ppc/e6500", CPU_PPC_E6500, 6 },
>> 	 	{ "Intel Silvermont microarchitecture", "i386/silvermont", CPU_SILVERMONT, 2 },
>>  		{ "ARMv7 Krait", "arm/armv7-krait", CPU_ARM_KRAIT, 5 },
>> 	+	{ "ARM AArch64", "arm/armv8-ca57", CPU_ARM_V8_CA57, 6 },
>> 	 };
>>
>> Perhaps we should go back to the idea of defining just a "common" armv8
>> processor type, and skip the idea of trying to shoehorn in a pseudo
>> processor-specific type when there is none yet.  So the above patch
>> excerpt becomes:
>>
>> 	@@ -129,6 +129,7 @@ static struct cpu_descr const cpu_descrs[MAX_CPU_TYPE] = {
>>  		{ "e6500", "ppc/e6500", CPU_PPC_E6500, 6 },
>> 	 	{ "Intel Silvermont microarchitecture", "i386/silvermont", CPU_SILVERMONT, 2 },
>>  		{ "ARMv7 Krait", "arm/armv7-krait", CPU_ARM_KRAIT, 5 },
>> 	+	{ "ARMv8 pmuv3 Common", "arm/armv8-pmuv3-common", CPU_ARM_V8_PMUV3_COMMON, 6 },
>>                     ^--- is this acceptable?  This is what would be displayed on the first line of 'ophelp' for the CPU type.
>> 	 };
>>
>> And of course we would remove the events/arm/armv8-ca57 events and
>> unit_masks files, too.
> 
> I'd rather just add x-gene as the CPU and point it directly at the common
> events, since there's no publicly available documentation and it is the CPU
> which Will is playing with.
> 
> e.g:
>   { "APM X-Gene", "arm/armv8-pmuv3-common", CPU_ARM_V8_APM_XGENE, 6 },
> 
> If/when that documentation becomes available, then we can add the relevant
> files and redirect the lookup table to those. An alternative is to add the
> dummy files for X-gene instead of Cortex-A57, and include them in this
> series.
Let's do the latter.  I'll re-swizzle the patch and post it for one final review.

-Maynard
> 
> Thoughts/opinions?
> 
> Will
>