Re: [PATCH] perf vendor events arm64: Add hip08 implementation defined PMU core events

[Ganapatrao Kulkarni]

Hi John,

On Wed, Oct 18, 2017 at 10:07 PM, John Garry <john.garry@xxxxxxxxxx> wrote:
> On 18/10/2017 11:49, Will Deacon wrote:
>>
>> On Wed, Oct 18, 2017 at 10:25:39AM +0100, John Garry wrote:
>>>
>>> On 17/10/2017 13:59, Will Deacon wrote:
>>>>
>>>> Hi Shaokun,
>>>>
>>>> Thanks for the patch. One comment below.
>>>>
>>>> On Tue, Oct 17, 2017 at 03:01:39PM +0800, Shaokun Zhang wrote:
>>>>>
>>>>> This is a short list of useful implementation defined PMU events of
>>>>> hip08, other supported events are not listed in this JSON file.
>>>>>
>>>>> This patch is dependent on Cavium's patch-v9 (Add support for
>>>>> ThunderX2 pmu events using json files), Link:
>>>>> https://www.spinics.net/lists/arm-kernel/msg611895.html
>>>>>
>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@xxxxxxxxxxxxx>
>>>>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>>>>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>>>>> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>>>>> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>>>>> Cc: Will Deacon <will.deacon@xxxxxxx>
>>>>> Cc: Ganapatrao Kulkarni <ganapatrao.kulkarni@xxxxxxxxxx>
>>>>> Cc: John Garry <john.garry@xxxxxxxxxx>
>>>>> ---
>>>>> .../arch/arm64/hisilicon/hip08-imp-def.json        | 176
>>>>> +++++++++++++++++++++
>>>>> tools/perf/pmu-events/arch/arm64/mapfile.csv       |   1 +
>>>>> 2 files changed, 177 insertions(+)
>>>>> create mode 100644
>>>>> tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
>>>>>
>>>>> diff --git
>>>>> a/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
>>>>> b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
>>>>> new file mode 100644
>>>>> index 0000000..6bb31da
>>>>> --- /dev/null
>>>>> +++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
>>>>> @@ -0,0 +1,176 @@
>>>>> +[
>>>>> +    {
>>>>> +        "PublicDescription": "Attributable Level 1 data cache access,
>>>>> read",
>>>>> +        "EventCode": "0x40",
>>>>> +        "EventName": "L1D_CACHE_RD",
>>>>> +        "BriefDescription": "L1D cache access, read",
>>>>> +    },
>>>>> +    {
>>>>> +        "PublicDescription": "Attributable Level 1 data cache access,
>>>>> write",
>>>>> +        "EventCode": "0x41",
>>>>> +        "EventName": "L1D_CACHE_WR",
>>>>> +        "BriefDescription": "L1D cache access, write",
>>>>> +    },
>>>>
>>>>
>>>> So these are the same as the events in cavium/thunderx2-imp-def.json and
>>>> should be factored out. In fact, ARM recommends event numbers for
>>>> 0x40-0xBF,
>>>> so the best thing would be to have those defined in their own file, then
>>>> have a way for the various CPU-specific .json files to pick and chose
>>>> the
>>>> events they need from there.
>>>
>>>
>>> Right, this seems reasonable. Just need to check on feasible.
>>>
>>> In terms of coordinating this work, shall we do it? Will arm64+ThunderX
>>> support be accepted as is?
>>
>>
>> Yes, that would be my preference if you don't mind.
>
>
>
> Cool.
>
> I am looking at this topic now. But I am doubting the folder structure
> again.
>
> Firstly, we still have this comment in the README:
> All the topic JSON files for a CPU model/family should be in a separate
> sub directory.
>
> Now, when thunderx3 or hip09 comes along, I assume that their jsons will
> similarly go into cavium and hisilcon folders, respectively. So, for
> example, we add thunderx3 json, like this:
> arm64/cavium/thunderx3-imp-def.json
> [
>     {
>         "PublicDescription": "foo",
>         "EventCode": "0x40",
>         "EventName": "bar",
>         "BriefDescription": "sieve",
>     },
> ]
>
> #Family-model,Version,Filename,EventType
> 0x00000000420f5160,v1,cavium,core
> 0x00000000420f5161,v1,cavium,core

certainly, there is Part number(PartNum, bits [15:4] ) change from
thunderx2 to thunderx3.
thunderx3 should have its own json file describing all its supported events.
same applies to other SoCs as well.

IIUC, the idea of ignoring Revision and Variants is that, the PMU
design/version wont change across Revisions and Variants.

>
> Then we have generated pmu_events.c, like this:
> #include "../../pmu-events/pmu-events.h"
> struct pmu_event pme_cavium[] = {
> {
>     .name = "bar",
>     .event = "event=0x40",
>     .desc = "sieve",
>     .topic = "thunderx3 imp def",
>     .long_desc = "foo",
> },
> {
>     .name = "l1d_cache_rd",
>     .event = "event=0x40",
>     .desc = "L1D cache read",
>     .topic = "thunderx2 imp def",
>     .long_desc = "Attributable Level 1 data cache access, read",
> },
> {
>     .name = "l1d_cache_wr",
>     .event = "event=0x41",
>     .desc = "L1D cache write",
>     .topic = "thunderx2 imp def",
>     .long_desc = "Attributable Level 1 data cache access, write ",
> },
>
> [ ... ]
>
> {
>     .name = 0,
>     .event = 0,
>     .desc = 0,
> },
> };
> struct pmu_events_map pmu_events_map[] = {
> {
>     .cpuid = "0x00000000420f5160",
>     .version = "v1",
>     .type = "core",
>     .table = pme_cavium
> },
> {
>     .cpuid = "0x00000000420f5161",
>     .version = "v1",
>     .type = "core",
>     .table = pme_cavium
> },
> {
>     .cpuid = 0,
>     .version = 0,
>     .type = 0,
>     .table = 0,
> },
> };
>
> It doesn't look right, espcially since we have conflicting definitions for
> event 0x40. We really should have table per cpu.

as i said above,  there is no possibility of having different events
mapped to same event id on a given platform.
>
> John
>
>
>>
>> Will
>>
>> .
>>
>
>

thanks
Ganapat