Re: [PATCH] oprofile: add support for Cavium ThunderX2

Hi Will

On 08/03/2019 21:32, William Cohen wrote:
> On 3/8/19 6:27 AM, Dave Aldridge wrote:
>> This commit adds support for Cavium ThunderX2 devices.
>>
>> Orabug: 29315936
> 
> Hi,
> 
> Thanks for the patch.  There are some comments about it inlined below. -Will
Thanks for the review comments, I've addressed them below...

>>
>> Signed-off-by: Dave Aldridge <[email protected]>
>> Reviewed-by: Eric Saint-Etienne <[email protected]>
>> ---
>>   events/Makefile.am                    |   1 +
>>   events/arm/armv8-thunderx2/events     | 115 ++++++++++++++++++++++++++++++++++
>>   events/arm/armv8-thunderx2/unit_masks |   3 +
>>   libop/op_cpu_type.c                   |   6 ++
>>   libop/op_cpu_type.h                   |   1 +
>>   libop/op_events.c                     |   1 +
>>   utils/ophelp.c                        |   7 +++
>>   7 files changed, 134 insertions(+)
>>   create mode 100644 events/arm/armv8-thunderx2/events
>>   create mode 100644 events/arm/armv8-thunderx2/unit_masks
>>
>> diff --git a/events/Makefile.am b/events/Makefile.am
>> index 174df0b..c57f0eb 100644
>> --- a/events/Makefile.am
>> +++ b/events/Makefile.am
>> @@ -59,6 +59,7 @@ event_files = \
>>   	arm/armv8-xgene/events arm/armv8-xgene/unit_masks \
>>   	arm/armv8-ca57/events arm/armv8-ca57/unit_masks \
>>   	arm/armv8-ca53/events arm/armv8-ca53/unit_masks \
>> +	arm/armv8-thunderx2/events arm/armv8-thunderx2/unit_masks \
>>   	mips/20K/events mips/20K/unit_masks \
>>   	mips/24K/events mips/24K/unit_masks \
>>   	mips/25K/events mips/25K/unit_masks \
>> diff --git a/events/arm/armv8-thunderx2/events b/events/arm/armv8-thunderx2/events
>> new file mode 100644
>> index 0000000..5871f42
>> --- /dev/null
>> +++ b/events/arm/armv8-thunderx2/events
>> @@ -0,0 +1,115 @@
>> +# Cavium ThunderX2 events
> 
> It would be useful to include in the comment a pointer to where these event number came from.  It looks like the following might be what was used for this:
> 
> https://www.marvell.com/documents/hrur6mybdvk5uki1w0z7/
> 
> However, if there is a more up-to-date document that might explains some of the differences seen.
I used a more upto date document, this accounts for most of the 
observations below.

> 
>> +#
>> +# The ThunderX2 processor excludes a few of the basic ARMv8 architected events.
>> +# Thus, need to explicitly list them rather than include
>> +# arm/armv8-pmuv3-common
>> +
>> +# The basic ARMv8 architect events supported by ThunderX2
>> +event:0x00 um:zero minimum:500 name:SW_INCR : Instruction architecturally executed, condition code check pass, software increment
>> +event:0x01 um:zero minimum:5000 name:L1I_CACHE_REFILL : Level 1 instruction cache refill
>> +event:0x02 um:zero minimum:5000 name:L1I_TLB_REFILL : Level 1 instruction TLB refill
>> +event:0x03 um:zero minimum:5000 name:L1D_CACHE_REFILL : Level 1 data cache refill
>> +event:0x04 um:zero minimum:5000 name:L1D_CACHE : Level 1 data cache access
>> +event:0x05 um:zero minimum:5000 name:L1D_TLB_REFILL : Level 1 data TLB refill
>> +event:0x06 um:zero minimum:100000 name:LD_RETIRED : Instruction architecturally executed, condition code check pass, load
>> +event:0x07 um:zero minimum:100000 name:ST_RETIRED : Instruction architecturally executed, condition code check pass, store
>> +event:0x08 um:zero minimum:100000 name:INST_RETIRED : Instruction architecturally executed
>> +event:0x09 um:zero minimum:500 name:EXC_TAKEN : Exception taken
>> +event:0x0A um:zero minimum:500 name:EXC_RETURN : Instruction architecturally executed, condition code check pass, exception return
>> +event:0x0B um:zero minimum:500 name:CID_WRITE_RETIRED : Instruction architecturally executed, condition code check pass, write to CONTEXTIDR
>> +event:0x0C um:zero minimum:5000 name:PC_WRITE_RETIRED : Instruction architecturally executed, condition code check pass, software change of the PC
> Is PC_WRITE_RETIRED (0x0c) seems to be excluded from the PDF document mentioned in the URL above.
> 
>> +event:0x0D um:zero minimum:5000 name:BR_IMMED_RETIRED : Instruction architecturally executed, immediate branch
>> +event:0x0E um:zero minimum:5000 name:BR_RETURN_RETIRED : Instruction architecturally executed, condition code check pass, procedure return
>> +event:0x0F um:zero minimum:500 name:UNALIGNED_LDST_RETIRED : Instruction architecturally executed, condition code check pass, unaligned load or store
>> +event:0x10 um:zero minimum:5000 name:BR_MIS_PRED : Mispredicted or not predicted branch speculatively executed
>> +event:0x11 um:zero minimum:100000 name:CPU_CYCLES : Cycle
>> +event:0x12 um:zero minimum:5000 name:BR_PRED : Predictable branch speculatively executed
>> +event:0x13 um:zero minimum:100000 name:MEM_ACCESS : Data memory access
>> +event:0x14 um:zero minimum:5000 name:L1I_CACHE : Level 1 instruction cache access
> 
> The following events (0x15-0x19) are listed in the PDF.  What is the reason they are commented out?
> 
>> +# event:0x15 um:zero minimum:5000 name:L1D_CACHE_WB : Level 1 data cache write-back
>> +# event:0x16 um:zero minimum:5000 name:L2D_CACHE : Level 2 data cache access
>> +# event:0x17 um:zero minimum:5000 name:L2D_CACHE_REFILL : Level 2 data cache refill
>> +# event:0x18 um:zero minimum:5000 name:L2D_CACHE_WB : Level 2 data cache write-back
>> +# event:0x19 um:zero minimum:5000 name:BUS_ACCESS : Bus access
> 
> The MEMORY_ERROR (0x1a) is not listed in the document.
>> +event:0x1A um:zero minimum:500 name:MEMORY_ERROR : Local memory error
> 
>> +event:0x1B um:zero minimum:100000 name:INST_SPEC : Operation speculatively executed
>> +event:0x1C um:zero minimum:5000 name:TTBR_WRITE_RETIRED : Instruction architecturally executed, condition code check pass, write to TTBR
>> +# event:0x1D um:zero minimum:5000 name:BUS_CYCLES : Bus cycle
> 
> The following event CHAIN (0x1e) is not useful and should be removed from the list.
Sure, no probs.

>> +event:0x1E um:zero minimum:5000 name:CHAIN : Odd-numbered counters, increment the count by one for each overflow of the preceding even-numbered counter. Even-numbered counters there is no increment
> 
> The events 0x1f and 0x20 are listed in the document.  What is the reason they are commented out?
> 
>> +# event:0x1F um:zero minimum:5000 name:L1D_CACHE_ALLOCATE : Level 1 data cache allocation without refill
>> +# event:0x20 um:zero minimum:5000 name:L2D_CACHE_ALLOCATE : Level 2 data cache allocation without refill
> 
>> +event:0x21 um:zero minimum:5000 name:BR_RETIRED : Instruction architecturally executed, branch
>> +event:0x22 um:zero minimum:5000 name:BR_MIS_PRED_RETIRED : Instruction architecturally executed, mispredicted branch
>> +event:0x23 um:zero minimum:5000 name:STALL_FRONTEND : No operation issued due to the frontend
>> +event:0x24 um:zero minimum:5000 name:STALL_BACKEND : No operation issued due to backend
>> +event:0x25 um:zero minimum:5000 name:L1D_TLB : Attributable Level 1 data or unified TLB access
>> +event:0x26 um:zero minimum:5000 name:L1I_TLB : Attributable Level 1 instruction TLB access
> 
> Events 0x27-0x2c are not listed in the PDF document.
>> +event:0x27 um:zero minimum:5000 name:L2I_CACHE : Attributable Level 2 instruction cache access> +event:0x28 um:zero minimum:5000 name:L2I_CACHE_REFILL : Attributable Level 2 instruction cache refill
>> +# event: 0x29 um:zero minimum:5000 name:L3D_CACHE_ALLOCATE : Attributable Level 3 data or unified cache allocation without refill
>> +event:0x2A um:zero minimum:5000 name:L3D_CACHE_REFILL : Attributable Level 3 data cache refill
>> +event:0x2B um:zero minimum:5000 name:L3D_CACHE : Attributable Level 3 data cache access
>> +event:0x2C um:zero minimum:5000 name:L3D_CACHE_WB : Attributable Level 3 data or unified cache write-back
> 
>> +event:0x2D um:zero minimum:5000 name:L2D_TLB_REFILL : Attributable Level 2 data or unified TLB refill
>> +event:0x2E um:zero minimum:5000 name:L2I_TLB_REFILL : Attributable Level 2 instruction TLB refill
>> +event:0x2F um:zero minimum:5000 name:L2D_TLB : Attributable Level 2 data or unified TLB access
> 
> Event 0x30 listed in PDF document.  Reason for it commented out?
>> +# event: 0x30 um:zero minimum:5000 name:L2I_TLB : Attributable Level 2 instruction TLB access
> 
> Events 0x31-0x37 not listed in the PDF.
>> +# event: 0x31 um:zero minimum:5000 name:REMOTE_ACCESS : Attributable access to another socket in a multi-socket system> +event:0x32 um:zero minimum:5000 name:LL_CACHE : Attributable Last Level data cache access
>> +event:0x33 um:zero minimum:5000 name:LL_CACHE_MISS : Attributable Last level data or unified cache miss
>> +event:0x34 um:zero minimum:5000 name:DTLB_WALK : Attributable data or unified TLB access with at least one translation table walk
>> +event:0x35 um:zero minimum:5000 name:ITLB_WALK : Attributable instruction TLB access with at least one translation table walk
>> +event:0x36 um:zero minimum:5000 name:LL_CACHE_RD : Attributable Last Level cache memory read
>> +event:0x37 um:zero minimum:5000 name:LL_CACHE_MISS_RD : Attributable Last Level cache memory read miss
>> +
>> +# ThunderX2 specific events
>> +event:0x040 um:zero minimum:10007 name:L1D_CACHE_LD : L1 data cache access - Read
>> +event:0x041 um:zero minimum:10007 name:L1D_CACHE_ST : L1 data cache access - Write
>> +event:0x042 um:zero minimum:10007 name:L1D_CACHE_REFILL_LD : L1 data cache refill - Read
> Events 0x43-0x47 listed in PDF.
>> +event:0x048 um:zero minimum:10007 name:L1D_CACHE_INVAL : L1 data cache invalidate
>> +event:0x04C um:zero minimum:10007 name:L1D_TLB_REFILL_LD : L1 data TLB refill - Read
>> +event:0x04D um:zero minimum:10007 name:L1D_TLB_REFILL_ST : L1 data TLB refill - Write
>> +event:0x04E um:zero minimum:10007 name:L1D_TLB_LD : L1 data TLB access - Read
>> +event:0x04F um:zero minimum:10007 name:L1D_TLB_ST : L1 data TLB access - Write
> 
> A number of L2D cache events (0x50-0x58) in PDF omitted.
> 
>> +event:0x05C um:zero minimum:10007 name:L2D_TLB_REFILL_LD : MTLB accesses that miss in the MTLB - Read
>> +event:0x05D um:zero minimum:10007 name:L2D_TLB_REFILL_ST : MTLB accesses that miss in the MTLB - Write
> (0x5e-0x5f) from PDF omitted.
> (0x60-0x67) from PDF omitted.
>> +event:0x068 um:zero minimum:10007 name:UNALIGNED_LD_SPEC : Unaligned access - Read
>> +event:0x069 um:zero minimum:10007 name:UNALIGNED_ST_SPEC : Unaligned access - Write
>> +event:0x06A um:zero minimum:10007 name:UNALIGNED_LDST_SPEC : Unaligned access
>> +event:0x06C um:zero minimum:10007 name:LDREX_SPEC : Exclusive operation speculatively executed - Load exclusive
>> +event:0x06D um:zero minimum:10007 name:STREX_PASS_SPEC : Exclusive operation speculative executed - Store exclusive pass
>> +event:0x06E um:zero minimum:10007 name:STREX_FAIL_SPEC : Exclusive operation speculative executed - Store exclusive fail
> STREX_SPEC(0x6f) from PDF omitted.
>> +event:0x070 um:zero minimum:10007 name:LD_SPEC : Operation speculatively executed - Load
>> +event:0x071 um:zero minimum:10007 name:ST_SPEC : Operation speculatively executed - Store
>> +event:0x072 um:zero minimum:10007 name:LDST_SPEC : Operation speculatively executed - Load or store
>> +event:0x073 um:zero minimum:10007 name:DP_SPEC : Operation speculatively executed - Integer data processing
>> +event:0x074 um:zero minimum:10007 name:ASE_SPEC : Operation speculatively executed - Advanced SIMD
>> +event:0x075 um:zero minimum:10007 name:VFP_SPEC : Operation speculatively executed - FP
>> +event:0x077 um:zero minimum:10007 name:CRYPTO_SPEC : Crypto instructions
>> +event:0x078 um:zero minimum:10007 name:BR_IMMED_SPEC : Branch speculative executed - Immediate branch
>> +event:0x079 um:zero minimum:10007 name:BR_RETURN_SPEC : Branch speculative executed - Procedure return
>> +event:0x07A um:zero minimum:10007 name:BR_INDIRECT_SPEC : Branch speculative executed - Indirect branch
>> +event:0x07C um:zero minimum:10007 name:ISB_SPEC : Barrier speculatively executed - ISB
>> +event:0x07D um:zero minimum:10007 name:DSB_SPEC : Barrier speculatively executed - DSB
>> +event:0x07E um:zero minimum:10007 name:DMB_SPEC : Barrier speculatively executed - DMB
>> +event:0x081 um:zero minimum:10007 name:EXC_UNDEF : Exception taken, other synchronous
>> +event:0x082 um:zero minimum:10007 name:EXC_SVC : Exception taken, Supervisor Call
>> +event:0x083 um:zero minimum:10007 name:EXC_PABORT : Exception taken, Instruction Abort
>> +event:0x084 um:zero minimum:10007 name:EXC_DABORT : Exception taken, Data Abort or SError
>> +event:0x086 um:zero minimum:10007 name:EXC_IRQ : Exception taken, IRQ
>> +event:0x087 um:zero minimum:10007 name:EXC_FIQ : Exception taken, FIQ
>> +event:0x088 um:zero minimum:10007 name:EXC_SMC : Exception taken, SMC
>> +event:0x08A um:zero minimum:10007 name:EXC_HVC : Exception taken, Hypervisor Call
>> +event:0x08B um:zero minimum:10007 name:EXC_TRAP_PABORT : Exception taken, Instruction Abort not taken locally
>> +event:0x08C um:zero minimum:10007 name:EXC_TRAP_DABORT : Exception taken, Data Abort or SError not taken locally
>> +event:0x08D um:zero minimum:10007 name:EXC_TRAP_OTHER : Exception taken, other traps not taken locally
>> +event:0x08E um:zero minimum:10007 name:EXC_TRAP_IRQ : Exception taken, IRQ not taken locally
>> +event:0x08F um:zero minimum:10007 name:EXC_TRAP_FIQ : Exception taken, FIQ not taken locally
>> +event:0x090 um:zero minimum:10007 name:RC_LD_SPEC : Release consistency instruction speculatively executed - Load Acquire
>> +event:0x091 um:zero minimum:10007 name:RC_ST_SPEC : Release consistency instruction speculatively executed - Store Release
> 
> The PDF didn't have (0xa0-0xa6).
>> +event:0x0A0 um:zero minimum:10007 name:L3D_CACHE_RD : L3 cache access - Read
>> +event:0x0A1 um:zero minimum:10007 name:L3D_CACHE_WR : L3 cache access - Write
>> +event:0x0A2 um:zero minimum:10007 name:L3D_CACHE_REFILL_RD : L3 cache refill - Read
>> +event:0x0A3 um:zero minimum:10007 name:L3D_CACHE_REFILL_WR : L3 cache refill - Write
>> +event:0x0A6 um:zero minimum:10007 name:L3D_CACHE_WB_VICTIM : L3 cache access that caused a writeback to occur
> 
> The PDF has events in the range 0xc1 to 0x21c that are not in the event list.
>> +
>> diff --git a/events/arm/armv8-thunderx2/unit_masks b/events/arm/armv8-thunderx2/unit_masks
>> new file mode 100644
>> index 0000000..9ace2eb
>> --- /dev/null
>> +++ b/events/arm/armv8-thunderx2/unit_masks
>> @@ -0,0 +1,3 @@
>> +# ARMv8 architected events unit masks
>> +#
>> +include:arm/armv8-pmuv3-common
>> diff --git a/libop/op_cpu_type.c b/libop/op_cpu_type.c
>> index d2babd1..061aa70 100644
>> --- a/libop/op_cpu_type.c
>> +++ b/libop/op_cpu_type.c
>> @@ -122,6 +122,7 @@ static struct cpu_descr const cpu_descrs[MAX_CPU_TYPE] = {
>>   	{ "ARM Cortex-A17", "arm/armv7-ca17", CPU_ARM_V7_CA17, 7 },
>>   	{ "ppc64 POWER9", "ppc64/power9", CPU_PPC64_POWER9, 6 },
>>   	{ "Intel Knights Landing", "i386/knightslanding", CPU_KNIGHTSLANDING, 4 },
>> +	{ "Cavium ThunderX2", "arm/armv8-thunderx2", CPU_ARM_V8_CAVIUM_THUNDERX2, 6 },
>>   };
>>    
>>   static size_t const nr_cpu_descrs = sizeof(cpu_descrs) / sizeof(struct cpu_descr);
>> @@ -442,6 +443,11 @@ static op_cpu _get_arm_cpu_type(void)
>>   		case 2:
>>   			return op_get_cpu_number("arm/xscale2");
>>   		}
> 
> How likely is there to be processors with the alternative ID for Thunderx2 (vendorid = 0x42, cpuid = 0x516) as in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/pmu-events/arch/arm64/mapfile.csv ?
> 
That is a good question, I guess it depends upon possible future 
acquisitions. I was testing on a box with the vendor id == 0x43, happy 
to add 0x42 as well.

>> +	} else if (vendorid == 0x43) { /* Cavium */
>> +		switch (cpuid) {
>> +		case 0x0af:
>> +			return op_get_cpu_number("arm/armv8-thunderx2");
>> +		}
>>   	}
>>   
>>   	return CPU_NO_GOOD;
>> diff --git a/libop/op_cpu_type.h b/libop/op_cpu_type.h
>> index 3bfd4eb..3ed9590 100644
>> --- a/libop/op_cpu_type.h
>> +++ b/libop/op_cpu_type.h
>> @@ -108,6 +108,7 @@ typedef enum {
>>   	CPU_ARM_V7_CA17, /* ARM Cortex-A17 */
>>   	CPU_PPC64_POWER9, /**< ppc64 POWER8 family */
>>   	CPU_KNIGHTSLANDING, /** Intel Knights Landing microarchitecture */
>> +	CPU_ARM_V8_CAVIUM_THUNDERX2, /* Cavium ThunderX2 */
>>   	MAX_CPU_TYPE
>>   } op_cpu;
>>   
>> diff --git a/libop/op_events.c b/libop/op_events.c
>> index 5125647..000b1dd 100644
>> --- a/libop/op_events.c
>> +++ b/libop/op_events.c
>> @@ -1247,6 +1247,7 @@ void op_default_event(op_cpu cpu_type, struct op_default_event_descr * descr)
>>   		case CPU_ARM_V8_APM_XGENE:
>>   		case CPU_ARM_V8_CA57:
>>   		case CPU_ARM_V8_CA53:
>> +		case CPU_ARM_V8_CAVIUM_THUNDERX2:
>>   			descr->name = "CPU_CYCLES";
>>   			break;
>>   
>> diff --git a/utils/ophelp.c b/utils/ophelp.c
>> index a07b215..764b871 100644
>> --- a/utils/ophelp.c
>> +++ b/utils/ophelp.c
>> @@ -662,6 +662,13 @@ int main(int argc, char const * argv[])
>>   			"Cortex A57 DDI (ARM DDI 0500D, revision r0p2)\n";
>>   		break;
>>   
>> +	case CPU_ARM_V8_CAVIUM_THUNDERX2:
>> +                event_doc =
>> +			"See ARM Architecture Reference Manual \n"
>> +			"ARMv8, for ARMv8-A architecture profile\n"
>> +			"DDI (ARM DDI0487D.a)\n";
>> +		break;
>> +
>>   	case CPU_PPC64_POWER4:
>>   	case CPU_PPC64_POWER5:
>>   	case CPU_PPC64_POWER6:
>>
> 
I'll work on a v2 patch.

Dave