[RFC,0/7] perf pmu-events: Support event aliasing for system PMUs
mbox series

Message ID 1579876505-113251-1-git-send-email-john.garry@huawei.com
Headers show
Series
  • perf pmu-events: Support event aliasing for system PMUs
Related show

Message

John Garry Jan. 24, 2020, 2:34 p.m. UTC
Currently event aliasing for only CPU and uncore PMUs is supported. In
fact, only uncore PMUs aliasing for when the uncore PMUs are fixed for a
CPU is supported, which may not always be the case for certain
architectures.

This series adds support for PMU event aliasing for system and other
uncore PMUs which are not fixed to a specific CPU.

For this, we introduce support for another per-arch mapfile, which maps a
particular system identifier to a set of system PMU events for that
system. This is much the same as what we do for CPU event aliasing.

To support this, we need to change how we match a PMU to a mapfile,
whether it should use a CPU or system mapfile. For this we do the
following:

- For CPU PMU, we always match for the event mapfile based on the CPUID.
  This has not changed.

- For an uncore or system PMU, we match first based on the SYSID (if set).
  If this fails, then we match on the CPUID.

  This works for x86, as x86 would not have any system mapfiles for uncore
  PMUs (and match on the CPUID).

Initial reference support is also added for ARM SMMUv3 PMCG (Performance
Monitor Event Group) PMU for HiSilicon hip08 platform with only a single
event so far - see driver in drivers/perf/arm_smmuv3_pmu.c for that driver.

Here is a sample output with this series:

root@ubuntu:/# ./perf list
  [...]

  smmuv3_pmcg_100020/config_cache_miss/              [Kernel PMU event]
  smmuv3_pmcg_100020/config_struct_access/           [Kernel PMU event]
  smmuv3_pmcg_100020/cycles/                         [Kernel PMU event]
  smmuv3_pmcg_100020/pcie_ats_trans_passed/          [Kernel PMU event]
  smmuv3_pmcg_100020/pcie_ats_trans_rq/              [Kernel PMU event]
  smmuv3_pmcg_100020/tlb_miss/                       [Kernel PMU event]
  smmuv3_pmcg_100020/trans_table_walk_access/        [Kernel PMU event]
  smmuv3_pmcg_100020/transaction/                    [Kernel PMU event]

  [...]

smmu v3 pmcg:
  smmuv3_pmcg.l1_tlb                                
       [SMMUv3 PMCG l1_tlb. Unit: smmuv3_pmcg]

  [...]

root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1
Using CPUID 0x00000000480fd010
Using SYSID HIP08
 -> smmuv3_pmcg_200100020/event=0x8a/
 -> smmuv3_pmcg_200140020/event=0x8a/
 -> smmuv3_pmcg_100020/event=0x8a/
 -> smmuv3_pmcg_140020/event=0x8a/
 -> smmuv3_pmcg_200148020/event=0x8a/
 -> smmuv3_pmcg_148020/event=0x8a/
smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850

 Performance counter stats for 'system wide':

               235      smmuv3_pmcg.l1_tlb                                          

       1.001263128 seconds time elapsed

root@ubuntu:/# 

Issues with this series which need to be addressed (aware to me):

- It would be good to have a universal method to identify the system from
  sysfs. Nothing exists which I know about. There is DMI, but this is not
  always available (or has correct info). Maybe systems which want to
  support this feature will need a "soc" driver, and a
  /sys/devices/socX/machine file (which I used for testing this series -
  this driver is out of tree currently).

- Maybe it is ok, but for systems which match on the system identifier,
  uncore PMUs should be in the system mapfile, and, as such, match on the
  system identifier and not CPU identifier.

- We need a better way in jevents.c to give a direct mapping of PMU name
  aliases, i.e. for any PMU name not prefixed with "uncore_", we need to
  add this to table unit_to_pmu[]. Not scalable.

  Having said that, maybe the kernel can introduce some future PMU naming
  convention in future.

John Garry (7):
  perf jevents: Add support for an extra directory level
  perf vendor events arm64: Relocate hip08 core events
  perf jevents: Add support for a system events PMU
  perf pmu: Rename uncore symbols to include system pmus
  perf pmu: Support matching by sysid
  perf vendor events arm64: Relocate uncore events for hip08
  perf vendor events arm64: Add hip08 SMMUv3 PMCG IMP DEF events

 tools/perf/arch/arm64/util/arm-spe.c          |   2 +-
 tools/perf/pmu-events/README                  |  47 ++++++--
 .../hip08/{ => cpu}/core-imp-def.json         |   0
 .../hisilicon/hip08/sys/smmu-v3-pmcg.json     |   9 ++
 .../hip08/{ => sys}/uncore-ddrc.json          |   0
 .../hisilicon/hip08/{ => sys}/uncore-hha.json |   0
 .../hisilicon/hip08/{ => sys}/uncore-l3c.json |   0
 tools/perf/pmu-events/arch/arm64/mapfile.csv  |   2 +-
 .../pmu-events/arch/arm64/mapfile_sys.csv     |  14 +++
 tools/perf/pmu-events/jevents.c               |  65 ++++++++--
 tools/perf/pmu-events/pmu-events.h            |   1 +
 tools/perf/util/evsel.h                       |   2 +-
 tools/perf/util/parse-events.c                |  12 +-
 tools/perf/util/pmu.c                         | 111 +++++++++++++++---
 tools/perf/util/pmu.h                         |   2 +-
 15 files changed, 221 insertions(+), 46 deletions(-)
 rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => cpu}/core-imp-def.json (100%)
 create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip08/sys/smmu-v3-pmcg.json
 rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-ddrc.json (100%)
 rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-hha.json (100%)
 rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-l3c.json (100%)
 create mode 100644 tools/perf/pmu-events/arch/arm64/mapfile_sys.csv

Comments

James Clark Feb. 11, 2020, 3:24 p.m. UTC | #1
Hi John,

I tested this on an Arm N1 board and see the same list of CPU events from the JSONs
picked up so it looks ok from that point of view.

James

On 1/24/20 2:34 PM, John Garry wrote:
> Currently event aliasing for only CPU and uncore PMUs is supported. In
> fact, only uncore PMUs aliasing for when the uncore PMUs are fixed for a
> CPU is supported, which may not always be the case for certain
> architectures.
>
> This series adds support for PMU event aliasing for system and other
> uncore PMUs which are not fixed to a specific CPU.
>
> For this, we introduce support for another per-arch mapfile, which maps a
> particular system identifier to a set of system PMU events for that
> system. This is much the same as what we do for CPU event aliasing.
>
> To support this, we need to change how we match a PMU to a mapfile,
> whether it should use a CPU or system mapfile. For this we do the
> following:
>
> - For CPU PMU, we always match for the event mapfile based on the CPUID.
>   This has not changed.
>
> - For an uncore or system PMU, we match first based on the SYSID (if set).
>   If this fails, then we match on the CPUID.
>
>   This works for x86, as x86 would not have any system mapfiles for uncore
>   PMUs (and match on the CPUID).
>
> Initial reference support is also added for ARM SMMUv3 PMCG (Performance
> Monitor Event Group) PMU for HiSilicon hip08 platform with only a single
> event so far - see driver in drivers/perf/arm_smmuv3_pmu.c for that driver.
>
> Here is a sample output with this series:
>
> root@ubuntu:/# ./perf list
>   [...]
>
>   smmuv3_pmcg_100020/config_cache_miss/              [Kernel PMU event]
>   smmuv3_pmcg_100020/config_struct_access/           [Kernel PMU event]
>   smmuv3_pmcg_100020/cycles/                         [Kernel PMU event]
>   smmuv3_pmcg_100020/pcie_ats_trans_passed/          [Kernel PMU event]
>   smmuv3_pmcg_100020/pcie_ats_trans_rq/              [Kernel PMU event]
>   smmuv3_pmcg_100020/tlb_miss/                       [Kernel PMU event]
>   smmuv3_pmcg_100020/trans_table_walk_access/        [Kernel PMU event]
>   smmuv3_pmcg_100020/transaction/                    [Kernel PMU event]
>
>   [...]
>
> smmu v3 pmcg:
>   smmuv3_pmcg.l1_tlb
>        [SMMUv3 PMCG l1_tlb. Unit: smmuv3_pmcg]
>
>   [...]
>
> root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1
> Using CPUID 0x00000000480fd010
> Using SYSID HIP08
>  -> smmuv3_pmcg_200100020/event=0x8a/
>  -> smmuv3_pmcg_200140020/event=0x8a/
>  -> smmuv3_pmcg_100020/event=0x8a/
>  -> smmuv3_pmcg_140020/event=0x8a/
>  -> smmuv3_pmcg_200148020/event=0x8a/
>  -> smmuv3_pmcg_148020/event=0x8a/
> smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
> smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
> smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
> smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
> smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
> smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850
>
>  Performance counter stats for 'system wide':
>
>                235      smmuv3_pmcg.l1_tlb
>
>        1.001263128 seconds time elapsed
>
> root@ubuntu:/#
>
> Issues with this series which need to be addressed (aware to me):
>
> - It would be good to have a universal method to identify the system from
>   sysfs. Nothing exists which I know about. There is DMI, but this is not
>   always available (or has correct info). Maybe systems which want to
>   support this feature will need a "soc" driver, and a
>   /sys/devices/socX/machine file (which I used for testing this series -
>   this driver is out of tree currently).
>
> - Maybe it is ok, but for systems which match on the system identifier,
>   uncore PMUs should be in the system mapfile, and, as such, match on the
>   system identifier and not CPU identifier.
>
> - We need a better way in jevents.c to give a direct mapping of PMU name
>   aliases, i.e. for any PMU name not prefixed with "uncore_", we need to
>   add this to table unit_to_pmu[]. Not scalable.
>
>   Having said that, maybe the kernel can introduce some future PMU naming
>   convention in future.
>
> John Garry (7):
>   perf jevents: Add support for an extra directory level
>   perf vendor events arm64: Relocate hip08 core events
>   perf jevents: Add support for a system events PMU
>   perf pmu: Rename uncore symbols to include system pmus
>   perf pmu: Support matching by sysid
>   perf vendor events arm64: Relocate uncore events for hip08
>   perf vendor events arm64: Add hip08 SMMUv3 PMCG IMP DEF events
>
>  tools/perf/arch/arm64/util/arm-spe.c          |   2 +-
>  tools/perf/pmu-events/README                  |  47 ++++++--
>  .../hip08/{ => cpu}/core-imp-def.json         |   0
>  .../hisilicon/hip08/sys/smmu-v3-pmcg.json     |   9 ++
>  .../hip08/{ => sys}/uncore-ddrc.json          |   0
>  .../hisilicon/hip08/{ => sys}/uncore-hha.json |   0
>  .../hisilicon/hip08/{ => sys}/uncore-l3c.json |   0
>  tools/perf/pmu-events/arch/arm64/mapfile.csv  |   2 +-
>  .../pmu-events/arch/arm64/mapfile_sys.csv     |  14 +++
>  tools/perf/pmu-events/jevents.c               |  65 ++++++++--
>  tools/perf/pmu-events/pmu-events.h            |   1 +
>  tools/perf/util/evsel.h                       |   2 +-
>  tools/perf/util/parse-events.c                |  12 +-
>  tools/perf/util/pmu.c                         | 111 +++++++++++++++---
>  tools/perf/util/pmu.h                         |   2 +-
>  15 files changed, 221 insertions(+), 46 deletions(-)
>  rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => cpu}/core-imp-def.json (100%)
>  create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip08/sys/smmu-v3-pmcg.json
>  rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-ddrc.json (100%)
>  rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-hha.json (100%)
>  rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-l3c.json (100%)
>  create mode 100644 tools/perf/pmu-events/arch/arm64/mapfile_sys.csv
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
John Garry Feb. 11, 2020, 3:41 p.m. UTC | #2
On 11/02/2020 15:24, James Clark wrote:
> Hi John,

Hi James,

> 
> I tested this on an Arm N1 board and see the same list of CPU events from the JSONs
> picked up so it looks ok from that point of view.

ok, good.

So does this platform have other PMUs (with a kernel driver) which we 
may want to add aliases for, like uncore PMUs, SMMUv3 PMCG, etc?

Thanks,
John

> 
> James
> 
> On 1/24/20 2:34 PM, John Garry wrote:
>> Currently event aliasing for only CPU and uncore PMUs is supported. In
>> fact, only uncore PMUs aliasing for when the uncore PMUs are fixed for a
>> CPU is supported, which may not always be the case for certain
>> architectures.
>>
>> This series adds support for PMU event aliasing for system and other
>> uncore PMUs which are not fixed to a specific CPU.
>>
>> For this, we introduce support for another per-arch mapfile, which maps a
>> particular system identifier to a set of system PMU events for that
>> system. This is much the same as what we do for CPU event aliasing.
>>
>> To support this, we need to change how we match a PMU to a mapfile,
>> whether it should use a CPU or system mapfile. For this we do the
>> following:
>>
>> - For CPU PMU, we always match for the event mapfile based on the CPUID.
>>    This has not changed.
>>
>> - For an uncore or system PMU, we match first based on the SYSID (if set).
>>    If this fails, then we match on the CPUID.
>>
>>    This works for x86, as x86 would not have any system mapfiles for uncore
>>    PMUs (and match on the CPUID).
>>
>> Initial reference support is also added for ARM SMMUv3 PMCG (Performance
>> Monitor Event Group) PMU for HiSilicon hip08 platform with only a single
>> event so far - see driver in drivers/perf/arm_smmuv3_pmu.c for that driver.
>>
>> Here is a sample output with this series:
>>
>> root@ubuntu:/# ./perf list
>>    [...]
>>
>>    smmuv3_pmcg_100020/config_cache_miss/              [Kernel PMU event]
>>    smmuv3_pmcg_100020/config_struct_access/           [Kernel PMU event]
>>    smmuv3_pmcg_100020/cycles/                         [Kernel PMU event]
>>    smmuv3_pmcg_100020/pcie_ats_trans_passed/          [Kernel PMU event]
>>    smmuv3_pmcg_100020/pcie_ats_trans_rq/              [Kernel PMU event]
>>    smmuv3_pmcg_100020/tlb_miss/                       [Kernel PMU event]
>>    smmuv3_pmcg_100020/trans_table_walk_access/        [Kernel PMU event]
>>    smmuv3_pmcg_100020/transaction/                    [Kernel PMU event]
>>
>>    [...]
>>
>> smmu v3 pmcg:
>>    smmuv3_pmcg.l1_tlb
>>         [SMMUv3 PMCG l1_tlb. Unit: smmuv3_pmcg]
>>
>>    [...]
>>
>> root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1
>> Using CPUID 0x00000000480fd010
>> Using SYSID HIP08
>>   -> smmuv3_pmcg_200100020/event=0x8a/
>>   -> smmuv3_pmcg_200140020/event=0x8a/
>>   -> smmuv3_pmcg_100020/event=0x8a/
>>   -> smmuv3_pmcg_140020/event=0x8a/
>>   -> smmuv3_pmcg_200148020/event=0x8a/
>>   -> smmuv3_pmcg_148020/event=0x8a/
>> smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
>> smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
>> smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
>> smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
>> smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
>> smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850
>>
>>   Performance counter stats for 'system wide':
>>
>>                 235      smmuv3_pmcg.l1_tlb
>>
>>         1.001263128 seconds time elapsed
>>
>> root@ubuntu:/#
>>
>> Issues with this series which need to be addressed (aware to me):
>>
>> - It would be good to have a universal method to identify the system from
>>    sysfs. Nothing exists which I know about. There is DMI, but this is not
>>    always available (or has correct info). Maybe systems which want to
>>    support this feature will need a "soc" driver, and a
>>    /sys/devices/socX/machine file (which I used for testing this series -
>>    this driver is out of tree currently).
>>
>> - Maybe it is ok, but for systems which match on the system identifier,
>>    uncore PMUs should be in the system mapfile, and, as such, match on the
>>    system identifier and not CPU identifier.
>>
>> - We need a better way in jevents.c to give a direct mapping of PMU name
>>    aliases, i.e. for any PMU name not prefixed with "uncore_", we need to
>>    add this to table unit_to_pmu[]. Not scalable.
>>
>>    Having said that, maybe the kernel can introduce some future PMU naming
>>    convention in future.
>>
>> John Garry (7):
>>    perf jevents: Add support for an extra directory level
>>    perf vendor events arm64: Relocate hip08 core events
>>    perf jevents: Add support for a system events PMU
>>    perf pmu: Rename uncore symbols to include system pmus
>>    perf pmu: Support matching by sysid
>>    perf vendor events arm64: Relocate uncore events for hip08
>>    perf vendor events arm64: Add hip08 SMMUv3 PMCG IMP DEF events
>>
>>   tools/perf/arch/arm64/util/arm-spe.c          |   2 +-
>>   tools/perf/pmu-events/README                  |  47 ++++++--
>>   .../hip08/{ => cpu}/core-imp-def.json         |   0
>>   .../hisilicon/hip08/sys/smmu-v3-pmcg.json     |   9 ++
>>   .../hip08/{ => sys}/uncore-ddrc.json          |   0
>>   .../hisilicon/hip08/{ => sys}/uncore-hha.json |   0
>>   .../hisilicon/hip08/{ => sys}/uncore-l3c.json |   0
>>   tools/perf/pmu-events/arch/arm64/mapfile.csv  |   2 +-
>>   .../pmu-events/arch/arm64/mapfile_sys.csv     |  14 +++
>>   tools/perf/pmu-events/jevents.c               |  65 ++++++++--
>>   tools/perf/pmu-events/pmu-events.h            |   1 +
>>   tools/perf/util/evsel.h                       |   2 +-
>>   tools/perf/util/parse-events.c                |  12 +-
>>   tools/perf/util/pmu.c                         | 111 +++++++++++++++---
>>   tools/perf/util/pmu.h                         |   2 +-
>>   15 files changed, 221 insertions(+), 46 deletions(-)
>>   rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => cpu}/core-imp-def.json (100%)
>>   create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip08/sys/smmu-v3-pmcg.json
>>   rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-ddrc.json (100%)
>>   rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-hha.json (100%)
>>   rename tools/perf/pmu-events/arch/arm64/hisilicon/hip08/{ => sys}/uncore-l3c.json (100%)
>>   create mode 100644 tools/perf/pmu-events/arch/arm64/mapfile_sys.csv
>>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> .
>
Will Deacon Feb. 18, 2020, 12:57 p.m. UTC | #3
On Fri, Jan 24, 2020 at 10:34:58PM +0800, John Garry wrote:
> Currently event aliasing for only CPU and uncore PMUs is supported. In
> fact, only uncore PMUs aliasing for when the uncore PMUs are fixed for a
> CPU is supported, which may not always be the case for certain
> architectures.
> 
> This series adds support for PMU event aliasing for system and other
> uncore PMUs which are not fixed to a specific CPU.
> 
> For this, we introduce support for another per-arch mapfile, which maps a
> particular system identifier to a set of system PMU events for that
> system. This is much the same as what we do for CPU event aliasing.
> 
> To support this, we need to change how we match a PMU to a mapfile,
> whether it should use a CPU or system mapfile. For this we do the
> following:
> 
> - For CPU PMU, we always match for the event mapfile based on the CPUID.
>   This has not changed.
> 
> - For an uncore or system PMU, we match first based on the SYSID (if set).
>   If this fails, then we match on the CPUID.
> 
>   This works for x86, as x86 would not have any system mapfiles for uncore
>   PMUs (and match on the CPUID).
> 
> Initial reference support is also added for ARM SMMUv3 PMCG (Performance
> Monitor Event Group) PMU for HiSilicon hip08 platform with only a single
> event so far - see driver in drivers/perf/arm_smmuv3_pmu.c for that driver.

Why don't we just expose SMMU_IIDR in the SMMUv3 PMU directory, so that
you can key off that? I'm nervous about coming up with a global "SYSID"
when we don't have the ability to standardise anything in that space.

Will
John Garry Feb. 18, 2020, 1:24 p.m. UTC | #4
On 18/02/2020 12:57, Will Deacon wrote:
> On Fri, Jan 24, 2020 at 10:34:58PM +0800, John Garry wrote:
>> Currently event aliasing for only CPU and uncore PMUs is supported. In
>> fact, only uncore PMUs aliasing for when the uncore PMUs are fixed for a
>> CPU is supported, which may not always be the case for certain
>> architectures.
>>
>> This series adds support for PMU event aliasing for system and other
>> uncore PMUs which are not fixed to a specific CPU.
>>
>> For this, we introduce support for another per-arch mapfile, which maps a
>> particular system identifier to a set of system PMU events for that
>> system. This is much the same as what we do for CPU event aliasing.
>>
>> To support this, we need to change how we match a PMU to a mapfile,
>> whether it should use a CPU or system mapfile. For this we do the
>> following:
>>
>> - For CPU PMU, we always match for the event mapfile based on the CPUID.
>>    This has not changed.
>>
>> - For an uncore or system PMU, we match first based on the SYSID (if set).
>>    If this fails, then we match on the CPUID.
>>
>>    This works for x86, as x86 would not have any system mapfiles for uncore
>>    PMUs (and match on the CPUID).
>>
>> Initial reference support is also added for ARM SMMUv3 PMCG (Performance
>> Monitor Event Group) PMU for HiSilicon hip08 platform with only a single
>> event so far - see driver in drivers/perf/arm_smmuv3_pmu.c for that driver.
> 
> Why don't we just expose SMMU_IIDR in the SMMUv3 PMU directory, so that
> you can key off that? 

That does not sound like a standard sysfs interface.

Anyway, I don't think that works for every case, quoting from 
https://lkml.org/lkml/2019/10/16/465:

"> Note: I do acknowledge that an overall issue is that we assume all 
PMCG IMP DEF events are same for a given SMMU model.

That assumption does technically fail already - I know MMU-600 has
different IMP-DEF events for its TCU and TBUs, however as long as we can
get as far as "this is some part of an MMU-600" the driver should be
able to figure out the rest ..."

So even if it is solvable here, the kernel driver(s) will need to be 
reworked. And that is just solving one case in many.

I'm nervous about coming up with a global "SYSID"
> when we don't have the ability to standardise anything in that space.

I understand totally, especially if any sysid is based on DT bindings.

But this is some sort of standardization:
https://developer.arm.com/docs/den0028/c, see SMCCC_ARCH_SOC_ID

John
Will Deacon Feb. 18, 2020, 1:39 p.m. UTC | #5
On Tue, Feb 18, 2020 at 01:24:38PM +0000, John Garry wrote:
> On 18/02/2020 12:57, Will Deacon wrote:
> > On Fri, Jan 24, 2020 at 10:34:58PM +0800, John Garry wrote:
> > > Currently event aliasing for only CPU and uncore PMUs is supported. In
> > > fact, only uncore PMUs aliasing for when the uncore PMUs are fixed for a
> > > CPU is supported, which may not always be the case for certain
> > > architectures.
> > > 
> > > This series adds support for PMU event aliasing for system and other
> > > uncore PMUs which are not fixed to a specific CPU.
> > > 
> > > For this, we introduce support for another per-arch mapfile, which maps a
> > > particular system identifier to a set of system PMU events for that
> > > system. This is much the same as what we do for CPU event aliasing.
> > > 
> > > To support this, we need to change how we match a PMU to a mapfile,
> > > whether it should use a CPU or system mapfile. For this we do the
> > > following:
> > > 
> > > - For CPU PMU, we always match for the event mapfile based on the CPUID.
> > >    This has not changed.
> > > 
> > > - For an uncore or system PMU, we match first based on the SYSID (if set).
> > >    If this fails, then we match on the CPUID.
> > > 
> > >    This works for x86, as x86 would not have any system mapfiles for uncore
> > >    PMUs (and match on the CPUID).
> > > 
> > > Initial reference support is also added for ARM SMMUv3 PMCG (Performance
> > > Monitor Event Group) PMU for HiSilicon hip08 platform with only a single
> > > event so far - see driver in drivers/perf/arm_smmuv3_pmu.c for that driver.
> > 
> > Why don't we just expose SMMU_IIDR in the SMMUv3 PMU directory, so that
> > you can key off that?
> 
> That does not sound like a standard sysfs interface.

It's standard in the sense that PMUs already have their own directory under
sysfs where you can put things. For example, the "caps" directory is a
dumping ground for all sorts of PMU-specific information.

On the other hand, saying "please go figure out which SoC you're on"
certainly isn't standard and is likely to lead to unreliable, spaghetti
code.

> Anyway, I don't think that works for every case, quoting from
> https://lkml.org/lkml/2019/10/16/465:
> 
> "> Note: I do acknowledge that an overall issue is that we assume all PMCG
> IMP DEF events are same for a given SMMU model.
> 
> That assumption does technically fail already - I know MMU-600 has
> different IMP-DEF events for its TCU and TBUs, however as long as we can
> get as far as "this is some part of an MMU-600" the driver should be
> able to figure out the rest ..."

Perhaps I'm misreading this, but it sounds like if you knew it was an
MMU-600 then you'd be ok. I also don't understand how a SoC ID makes things
any easier in this regard.

> So even if it is solvable here, the kernel driver(s) will need to be
> reworked. And that is just solving one case in many.

PMU drivers will need to expose more information to userspace so that they
can be identified more precisely, yes. I wouldn't say they would need to be
"reworked".

> I'm nervous about coming up with a global "SYSID"
> > when we don't have the ability to standardise anything in that space.
> 
> I understand totally, especially if any sysid is based on DT bindings.

Well if this is going to be ACPI-only then it's a non-starter.

> But this is some sort of standardization:
> https://developer.arm.com/docs/den0028/c, see SMCCC_ARCH_SOC_ID

Yay, firmware :/

Even if this was widely implemented (it's not), I still think that it's
the wrong level of abstraction. Why not do away with ACPI/DT entirely
and predicate everything off the SoC ID?

Will
John Garry Feb. 18, 2020, 4:19 p.m. UTC | #6
>>
>>> Why don't we just expose SMMU_IIDR in the SMMUv3 PMU directory, so that
>>> you can key off that?
>>
>> That does not sound like a standard sysfs interface.
> 
> It's standard in the sense that PMUs already have their own directory under
> sysfs where you can put things. 

Sure, but then the perf tool will need to be able to interpret all these 
custom PMU files, which has scalability issues.

Maybe this would work and I did consider it, but another concern is that 
the PMU drivers will have problems making available some 
implementation-specific identifier at all.

For example, the "caps" directory is a
> dumping ground for all sorts of PMU-specific information.
> 
> On the other hand, saying "please go figure out which SoC you're on"
> certainly isn't standard and is likely to lead to unreliable, spaghetti
> code.

I'm not sure how. The perf tool PMU event aliasing already takes a few 
certain steps to figure out which cpuid to use:

static char *perf_pmu__getcpuid(struct perf_pmu *pmu)
{
	char *cpuid;
	static bool printed;

	cpuid = getenv("PERF_CPUID");
	if (cpuid)
		cpuid = strdup(cpuid);
	if (!cpuid)
		cpuid = get_cpuid_str(pmu);
	if (!cpuid)
		return NULL;

	if (!printed) {
		pr_debug("Using CPUID %s\n", cpuid);
		printed = true;
	}
	return cpuid;
}

And this would be something similar - just read some sysfs file.

> 
>> Anyway, I don't think that works for every case, quoting from
>> https://lkml.org/lkml/2019/10/16/465:
>>
>> "> Note: I do acknowledge that an overall issue is that we assume all PMCG
>> IMP DEF events are same for a given SMMU model.
>>
>> That assumption does technically fail already - I know MMU-600 has
>> different IMP-DEF events for its TCU and TBUs, however as long as we can
>> get as far as "this is some part of an MMU-600" the driver should be
>> able to figure out the rest ..."
> 
> Perhaps I'm misreading this, but it sounds like if you knew it was an
> MMU-600 then you'd be ok. I also don't understand how a SoC ID makes things
> any easier in this regard.

It's doesn't necessarily make things easier in this regard. But using a 
SoC ID is an alternative to checking the SMMU_ID or the kernel driver 
having to know that it was a MMU-600 at all.

> 
>> So even if it is solvable here, the kernel driver(s) will need to be
>> reworked. And that is just solving one case in many.
> 
> PMU drivers will need to expose more information to userspace so that they
> can be identified more precisely, yes. I wouldn't say they would need to be
> "reworked".

OK, so some combination of changes would still be required for the SMMU 
PMCG, IORT, and SMMUv3 drivers.

These changes were included in my RFC.

> 
>> I'm nervous about coming up with a global "SYSID"
>>> when we don't have the ability to standardise anything in that space.
>>
>> I understand totally, especially if any sysid is based on DT bindings.
> 
> Well if this is going to be ACPI-only then it's a non-starter.

No, in fact I would rather not rely on ACPI or DT at all.

> 
>> But this is some sort of standardization:
>> https://developer.arm.com/docs/den0028/c, see SMCCC_ARCH_SOC_ID
> 
> Yay, firmware :/
>  > Even if this was widely implemented (it's not),

The spec is in beta stage now.

And if it's not implemented, then simply the perf tool cannot make PMU 
aliases for those DDRC or similar PMUs (and we would find that the DDRC 
or similar JSON files for those platforms would not be added until it 
does support it).

>I still think that it's
> the wrong level of abstraction. 

As I said above, we could try to expand the PMU sysfs entries for this, 
but I have concerns on how we make some imp specific identifier or what 
this would look like.

Why not do away with ACPI/DT entirely
> and predicate everything off the SoC ID?

As constantly checking what the SoC ID means throughout system 
components does not scale.

John
Mark Rutland Feb. 18, 2020, 5:08 p.m. UTC | #7
On Tue, Feb 18, 2020 at 04:19:32PM +0000, John Garry wrote:
> > > 
> > > > Why don't we just expose SMMU_IIDR in the SMMUv3 PMU directory, so that
> > > > you can key off that?
> > > 
> > > That does not sound like a standard sysfs interface.
> > 
> > It's standard in the sense that PMUs already have their own directory under
> > sysfs where you can put things.
> 
> Sure, but then the perf tool will need to be able to interpret all these
> custom PMU files, which has scalability issues.
> 
> Maybe this would work and I did consider it, but another concern is that the
> PMU drivers will have problems making available some implementation-specific
> identifier at all.
> 
> For example, the "caps" directory is a
> > dumping ground for all sorts of PMU-specific information.
> > 
> > On the other hand, saying "please go figure out which SoC you're on"
> > certainly isn't standard and is likely to lead to unreliable, spaghetti
> > code.
> 
> I'm not sure how. The perf tool PMU event aliasing already takes a few
> certain steps to figure out which cpuid to use:
> 
> static char *perf_pmu__getcpuid(struct perf_pmu *pmu)
> {
> 	char *cpuid;
> 	static bool printed;
> 
> 	cpuid = getenv("PERF_CPUID");
> 	if (cpuid)
> 		cpuid = strdup(cpuid);
> 	if (!cpuid)
> 		cpuid = get_cpuid_str(pmu);
> 	if (!cpuid)
> 		return NULL;
> 
> 	if (!printed) {
> 		pr_debug("Using CPUID %s\n", cpuid);
> 		printed = true;
> 	}
> 	return cpuid;
> }
> 
> And this would be something similar - just read some sysfs file.
> 
> > 
> > > Anyway, I don't think that works for every case, quoting from
> > > https://lkml.org/lkml/2019/10/16/465:
> > > 
> > > "> Note: I do acknowledge that an overall issue is that we assume all PMCG
> > > IMP DEF events are same for a given SMMU model.
> > > 
> > > That assumption does technically fail already - I know MMU-600 has
> > > different IMP-DEF events for its TCU and TBUs, however as long as we can
> > > get as far as "this is some part of an MMU-600" the driver should be
> > > able to figure out the rest ..."
> > 
> > Perhaps I'm misreading this, but it sounds like if you knew it was an
> > MMU-600 then you'd be ok. I also don't understand how a SoC ID makes things
> > any easier in this regard.
> 
> It's doesn't necessarily make things easier in this regard. But using a SoC
> ID is an alternative to checking the SMMU_ID or the kernel driver having to
> know that it was a MMU-600 at all.

Using SOC_ID means that going forward, userspace needs to learn about
the integration details of each SoC in order to identify a component. As
you said:

| As constantly checking what the SoC ID means throughout system components
| does not scale.

... and I think that equally applies to userspace in this case. Who knows how
many SoCs are going to have MMU-600?

I also know that SOC_ID is going to be optional, and I think it's near-certain
that someone will end up producing two SoCs exposing the same ID.

For system PMUs, I'd rather the system PMU driver exposed some sort of
implementation ID. e.g. the SMMU_ID for SMMU. We can give that a generic name,
and mandate that where a driver exposes it, the format/meaning is defined in
the documentation for the driver.

That can be namespace by driver, so e.g. keys would be smmu_sysfs_name/<id> and
ddrc_sysfs_name/<id>.

> > > So even if it is solvable here, the kernel driver(s) will need to be
> > > reworked. And that is just solving one case in many.
> > 
> > PMU drivers will need to expose more information to userspace so that they
> > can be identified more precisely, yes. I wouldn't say they would need to be
> > "reworked".
> 
> OK, so some combination of changes would still be required for the SMMU
> PMCG, IORT, and SMMUv3 drivers.

To expose the SMMU ID, surely that's just the driver? Or are there
implementations where the ID register is bogus and have to be overridden?

Thanks,
Mark.
John Garry Feb. 18, 2020, 5:58 p.m. UTC | #8
On 18/02/2020 17:08, Mark Rutland wrote:
>>> I also don't understand how a SoC ID makes things
>>> any easier in this regard.
>> It's doesn't necessarily make things easier in this regard. But using a SoC
>> ID is an alternative to checking the SMMU_ID or the kernel driver having to
>> know that it was a MMU-600 at all.
> Using SOC_ID means that going forward, userspace needs to learn about
> the integration details of each SoC in order to identify a component. As
> you said:
> 
> | As constantly checking what the SoC ID means throughout system components
> | does not scale.
> 
> ... and I think that equally applies to userspace in this case. Who knows how
> many SoCs are going to have MMU-600?
> 
> I also know that SOC_ID is going to be optional, and I think it's near-certain
> that someone will end up producing two SoCs exposing the same ID.

Wouldn't different SoCs having same SMC SOC_ID and revision be a 
(fixable) mistake in the SMC FW?

And if it's not implemented, then no PMU events aliasing in perf tool 
for those uncore PMUs - nothing gets broken though and no regression. 
But I do understand your concern here.

> 
> For system PMUs, I'd rather the system PMU driver exposed some sort of
> implementation ID. e.g. the SMMU_ID for SMMU. We can give that a generic name,
> and mandate that where a driver exposes it, the format/meaning is defined in
> the documentation for the driver.

Then doesn't that per-PMU ID qualify as brittle and non-standard also?

At least the SMC SoC ID is according to some standard.

And typically most PMU HW would have no ID reg, so where to even get 
this identification info? Joakim Zhang seems to have this problem for 
the imx8 DDRC PMU driver.

> 
> That can be namespace by driver, so e.g. keys would be smmu_sysfs_name/<id> and
> ddrc_sysfs_name/<id>.
> 
>>>> So even if it is solvable here, the kernel driver(s) will need to be
>>>> reworked. And that is just solving one case in many.
>>> PMU drivers will need to expose more information to userspace so that they
>>> can be identified more precisely, yes. I wouldn't say they would need to be
>>> "reworked".
>> OK, so some combination of changes would still be required for the SMMU
>> PMCG, IORT, and SMMUv3 drivers.
> To expose the SMMU ID, surely that's just the driver? 

This case is complicated, like others I anticipate.

So the SMMU PMCG HW has no ID register itself, and this idea relies on 
using the associated SMMUv3 IIDR in lieu. For that, we need to involve 
the IORT, SMMUv3, and SMMU PMCG drivers to create this linkage, and even 
then I still have my doubts on whether this is even proper.

Please see 
https://lore.kernel.org/linux-iommu/1569854031-237636-1-git-send-email-john.garry@huawei.com/ 
for reference.

Or are there
> implementations where the ID register is bogus and have to be overridden?
> 

I will also note that perf tool PMU events framework relies today on 
generating a table of events aliases per CPU and matching based on that. 
If you want to totally disassociate a CPU or any SoC ID mapping, then 
this will require big perf tool rework.

Thanks,
John
Mark Rutland Feb. 18, 2020, 6:13 p.m. UTC | #9
On Tue, Feb 18, 2020 at 05:58:46PM +0000, John Garry wrote:
> On 18/02/2020 17:08, Mark Rutland wrote:
> > > > I also don't understand how a SoC ID makes things
> > > > any easier in this regard.
> > > It's doesn't necessarily make things easier in this regard. But using a SoC
> > > ID is an alternative to checking the SMMU_ID or the kernel driver having to
> > > know that it was a MMU-600 at all.
> > Using SOC_ID means that going forward, userspace needs to learn about
> > the integration details of each SoC in order to identify a component. As
> > you said:
> > 
> > | As constantly checking what the SoC ID means throughout system components
> > | does not scale.
> > 
> > ... and I think that equally applies to userspace in this case. Who knows how
> > many SoCs are going to have MMU-600?
> > 
> > I also know that SOC_ID is going to be optional, and I think it's near-certain
> > that someone will end up producing two SoCs exposing the same ID.
> 
> Wouldn't different SoCs having same SMC SOC_ID and revision be a (fixable)
> mistake in the SMC FW?
> 
> And if it's not implemented, then no PMU events aliasing in perf tool for
> those uncore PMUs - nothing gets broken though and no regression. But I do
> understand your concern here.
> 
> > For system PMUs, I'd rather the system PMU driver exposed some sort of
> > implementation ID. e.g. the SMMU_ID for SMMU. We can give that a generic name,
> > and mandate that where a driver exposes it, the format/meaning is defined in
> > the documentation for the driver.
> 
> Then doesn't that per-PMU ID qualify as brittle and non-standard also?

Not in my mind; any instances of the same IP can have the same ID,
regardless of which SoC they're in. Once userspace learns about
device-foo-4000, it knows about it on all SoCs. That also means you can
support heterogeneous instances in the same SoC.

If a device varies so much on a SoC-by-SoC basis and or the driver has
no idea what to expose, it could be legitimate for the PMU driver to
expose the SoC ID as its PMU-specific ID, but I don't think we should
make that the common/only case.

> At least the SMC SoC ID is according to some standard.
> 
> And typically most PMU HW would have no ID reg, so where to even get this
> identification info? Joakim Zhang seems to have this problem for the imx8
> DDRC PMU driver.

For imx8, the DT compat string or additional properties on the DDRC node
could be used to imply the id.

> > That can be namespace by driver, so e.g. keys would be smmu_sysfs_name/<id> and
> > ddrc_sysfs_name/<id>.
> > 
> > > > > So even if it is solvable here, the kernel driver(s) will need to be
> > > > > reworked. And that is just solving one case in many.
> > > > PMU drivers will need to expose more information to userspace so that they
> > > > can be identified more precisely, yes. I wouldn't say they would need to be
> > > > "reworked".
> > > OK, so some combination of changes would still be required for the SMMU
> > > PMCG, IORT, and SMMUv3 drivers.
> > To expose the SMMU ID, surely that's just the driver?
> 
> This case is complicated, like others I anticipate.
> 
> So the SMMU PMCG HW has no ID register itself, and this idea relies on using
> the associated SMMUv3 IIDR in lieu. For that, we need to involve the IORT,
> SMMUv3, and SMMU PMCG drivers to create this linkage, and even then I still
> have my doubts on whether this is even proper.

Ok, I hadn't appreciated that the PMCG did not have an ID register
itself.

I think that the relationship between the SMMU and PMCG is a stronger
argument against using the SOC_ID. If the PMCGs in a system are
heterogeneous, then you must know the type of the specific instance.

> Please see https://lore.kernel.org/linux-iommu/1569854031-237636-1-git-send-email-john.garry@huawei.com/
> for reference.
> 
> Or are there
> > implementations where the ID register is bogus and have to be overridden?
> > 
> 
> I will also note that perf tool PMU events framework relies today on
> generating a table of events aliases per CPU and matching based on that. If
> you want to totally disassociate a CPU or any SoC ID mapping, then this will
> require big perf tool rework.

I think that might be necessary, as otherwise we're going to back
ourselves into a corner by building what's simple now.

Thanks,
Mark.
Joakim Zhang Feb. 19, 2020, 1:55 a.m. UTC | #10
> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: 2020年2月19日 2:14
> To: John Garry <john.garry@huawei.com>
> Cc: ak@linux.intel.com; Joakim Zhang <qiangqing.zhang@nxp.com>;
> suzuki.poulose@arm.com; peterz@infradead.org; Will Deacon
> <will@kernel.org>; Linuxarm <linuxarm@huawei.com>; acme@kernel.org;
> linux-kernel@vger.kernel.org; Zhangshaokun <zhangshaokun@hisilicon.com>;
> alexander.shishkin@linux.intel.com; mingo@redhat.com;
> james.clark@arm.com; namhyung@kernel.org; jolsa@redhat.com;
> linux-arm-kernel@lists.infradead.org; robin.murphy@arm.com; Sudeep Holla
> <sudeep.holla@arm.com>
> Subject: Re: [PATCH RFC 0/7] perf pmu-events: Support event aliasing for
> system PMUs

[...]
> > And typically most PMU HW would have no ID reg, so where to even get
> > this identification info? Joakim Zhang seems to have this problem for
> > the imx8 DDRC PMU driver.
> 
> For imx8, the DT compat string or additional properties on the DDRC node
> could be used to imply the id.

Hi Mark,

Yes, actually we can expose something like DDRC_ID to indicate each specific DDR controller, to point out the filter feature.
But, even the SoCs integrated the same DDRC_ID, just say that they have the same DDRC controller.

From user space, the usage is different, for example:

i.MX8MM and i.MX8MN, they use the same driver(DDRC_ID) and cortex-a53 integrated.

If we want to monitor VPU, their *master id* is different from SoCs.
On i.MX8MM, event is imx8_ddr0/axid-read,axi_id=0x08/
On i.MX8MN, event is imx8_ddr0/axid-read,axi_id=0x12/

I try to write a JSON file to use these events, for now, I only can locate the file at the directory: tools/perf/pmu-events/arch/arm64/arm/cortex-a53/

Perf tool loads all events when CPUID matched, which is now unreasonable, we want related events are loaded for specific SoC.

All events will also be loaded if we use DDRC_ID to match in the future, this seems to not be a good ideal.

Best Regards,
Joakim Zhang

[....]
John Garry Feb. 19, 2020, 8:44 a.m. UTC | #11
On 19/02/2020 01:55, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Mark Rutland <mark.rutland@arm.com>
>> Sent: 2020年2月19日 2:14
>> To: John Garry <john.garry@huawei.com>
>> Cc: ak@linux.intel.com; Joakim Zhang <qiangqing.zhang@nxp.com>;
>> suzuki.poulose@arm.com; peterz@infradead.org; Will Deacon
>> <will@kernel.org>; Linuxarm <linuxarm@huawei.com>; acme@kernel.org;
>> linux-kernel@vger.kernel.org; Zhangshaokun <zhangshaokun@hisilicon.com>;
>> alexander.shishkin@linux.intel.com; mingo@redhat.com;
>> james.clark@arm.com; namhyung@kernel.org; jolsa@redhat.com;
>> linux-arm-kernel@lists.infradead.org; robin.murphy@arm.com; Sudeep Holla
>> <sudeep.holla@arm.com>
>> Subject: Re: [PATCH RFC 0/7] perf pmu-events: Support event aliasing for
>> system PMUs
> 
> [...]
>>> And typically most PMU HW would have no ID reg, so where to even get
>>> this identification info? Joakim Zhang seems to have this problem for
>>> the imx8 DDRC PMU driver.
>>
>> For imx8, the DT compat string or additional properties on the DDRC node
>> could be used to imply the id.
> 
> Hi Mark,
> 
> Yes, actually we can expose something like DDRC_ID to indicate each specific DDR controller, to point out the filter feature.
> But, even the SoCs integrated the same DDRC_ID, just say that they have the same DDRC controller.
> 
>  From user space, the usage is different, for example:
> 
> i.MX8MM and i.MX8MN, they use the same driver(DDRC_ID) and cortex-a53 integrated.
> 
> If we want to monitor VPU, their *master id* is different from SoCs.
> On i.MX8MM, event is imx8_ddr0/axid-read,axi_id=0x08/
> On i.MX8MN, event is imx8_ddr0/axid-read,axi_id=0x12/
> 
> I try to write a JSON file to use these events, for now, I only can locate the file at the directory: tools/perf/pmu-events/arch/arm64/arm/cortex-a53/
> 
> Perf tool loads all events when CPUID matched, which is now unreasonable, we want related events are loaded for specific SoC.

so we could have a folder like .../arch/arm64/nxp/system for these 
JSONs. The perf tool can be updated to handle CPU and system events in 
separate folders.

> 
> All events will also be loaded if we use DDRC_ID to match in the future, this seems to not be a good ideal.

The important part is knowing which events are supported per 
implementation. Is there any method in the driver of knowing the 
specific implementation, like any DT compat string? Least preferred 
option would be DT machine ID.

> 

John
John Garry Feb. 19, 2020, 8:50 a.m. UTC | #12
>>
>>> For system PMUs, I'd rather the system PMU driver exposed some sort of
>>> implementation ID. e.g. the SMMU_ID for SMMU. We can give that a generic name,
>>> and mandate that where a driver exposes it, the format/meaning is defined in
>>> the documentation for the driver.
>>
>> Then doesn't that per-PMU ID qualify as brittle and non-standard also?
> 
> Not in my mind; any instances of the same IP can have the same ID,
> regardless of which SoC they're in. Once userspace learns about
> device-foo-4000, it knows about it on all SoCs. That also means you can
> support heterogeneous instances in the same SoC.

Sure, but this device-foo-4000 naming is a concern for standardization, 
stable ABI, and perf tool support.

> 
> If a device varies so much on a SoC-by-SoC basis and or the driver has
> no idea what to expose, it could be legitimate for the PMU driver to
> expose the SoC ID as its PMU-specific ID, but I don't think we should
> make that the common/only case.

But where does the PMU driver get the SoC ID? Does it have to check DT 
machine ID, ACPI OEM ID, or SMCCC SOC ID?

I can't imagine that you really want this stuff in the kernel PMU 
drivers, but that's your call.

> 
>> At least the SMC SoC ID is according to some standard.
>>
>> And typically most PMU HW would have no ID reg, so where to even get this
>> identification info? Joakim Zhang seems to have this problem for the imx8
>> DDRC PMU driver.
> 
> For imx8, the DT compat string or additional properties on the DDRC node
> could be used to imply the id.

Fine, but it's the ACPI case which is what I am concerned about.

> 
>>> That can be namespace by driver, so e.g. keys would be smmu_sysfs_name/<id> and
>>> ddrc_sysfs_name/<id>.
>>>
>>>>>> So even if it is solvable here, the kernel driver(s) will need to be
>>>>>> reworked. And that is just solving one case in many.
>>>>> PMU drivers will need to expose more information to userspace so that they
>>>>> can be identified more precisely, yes. I wouldn't say they would need to be
>>>>> "reworked".
>>>> OK, so some combination of changes would still be required for the SMMU
>>>> PMCG, IORT, and SMMUv3 drivers.
>>> To expose the SMMU ID, surely that's just the driver?
>>
>> This case is complicated, like others I anticipate.
>>
>> So the SMMU PMCG HW has no ID register itself, and this idea relies on using
>> the associated SMMUv3 IIDR in lieu. For that, we need to involve the IORT,
>> SMMUv3, and SMMU PMCG drivers to create this linkage, and even then I still
>> have my doubts on whether this is even proper.
> 
> Ok, I hadn't appreciated that the PMCG did not have an ID register
> itself.
> 
> I think that the relationship between the SMMU and PMCG is a stronger
> argument against using the SOC_ID. If the PMCGs in a system are
> heterogeneous, then you must know the type of the specific instance.

Perf tool PMU events can handle that. Each PMCG PMU instance has a 
different name - the name encoding includes the HW base address, so 
always unique per system - and then so the JSON can know this and have 
events specific per instance.

> 
>> Please see https://lore.kernel.org/linux-iommu/1569854031-237636-1-git-send-email-john.garry@huawei.com/
>> for reference.
>>
>> Or are there
>>> implementations where the ID register is bogus and have to be overridden?
>>>
>>
>> I will also note that perf tool PMU events framework relies today on
>> generating a table of events aliases per CPU and matching based on that. If
>> you want to totally disassociate a CPU or any SoC ID mapping, then this will
>> require big perf tool rework.
> 
> I think that might be necessary, as otherwise we're going to back
> ourselves into a corner by building what's simple now.

I appreciate what you're saying. I'll check this idea further.

Cheers,
John
Joakim Zhang Feb. 19, 2020, 12:40 p.m. UTC | #13
> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: 2020年2月19日 16:44
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Mark Rutland
> <mark.rutland@arm.com>
> Cc: ak@linux.intel.com; suzuki.poulose@arm.com; peterz@infradead.org; Will
> Deacon <will@kernel.org>; Linuxarm <linuxarm@huawei.com>;
> acme@kernel.org; linux-kernel@vger.kernel.org; Zhangshaokun
> <zhangshaokun@hisilicon.com>; alexander.shishkin@linux.intel.com;
> mingo@redhat.com; james.clark@arm.com; namhyung@kernel.org;
> jolsa@redhat.com; linux-arm-kernel@lists.infradead.org;
> robin.murphy@arm.com; Sudeep Holla <sudeep.holla@arm.com>
> Subject: Re: [PATCH RFC 0/7] perf pmu-events: Support event aliasing for
> system PMUs
> 
> On 19/02/2020 01:55, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Mark Rutland <mark.rutland@arm.com>
> >> Sent: 2020年2月19日 2:14
> >> To: John Garry <john.garry@huawei.com>
> >> Cc: ak@linux.intel.com; Joakim Zhang <qiangqing.zhang@nxp.com>;
> >> suzuki.poulose@arm.com; peterz@infradead.org; Will Deacon
> >> <will@kernel.org>; Linuxarm <linuxarm@huawei.com>; acme@kernel.org;
> >> linux-kernel@vger.kernel.org; Zhangshaokun
> >> <zhangshaokun@hisilicon.com>; alexander.shishkin@linux.intel.com;
> >> mingo@redhat.com; james.clark@arm.com; namhyung@kernel.org;
> >> jolsa@redhat.com; linux-arm-kernel@lists.infradead.org;
> >> robin.murphy@arm.com; Sudeep Holla <sudeep.holla@arm.com>
> >> Subject: Re: [PATCH RFC 0/7] perf pmu-events: Support event aliasing
> >> for system PMUs
> >
> > [...]
> >>> And typically most PMU HW would have no ID reg, so where to even get
> >>> this identification info? Joakim Zhang seems to have this problem
> >>> for the imx8 DDRC PMU driver.
> >>
> >> For imx8, the DT compat string or additional properties on the DDRC
> >> node could be used to imply the id.
> >
> > Hi Mark,
> >
> > Yes, actually we can expose something like DDRC_ID to indicate each specific
> DDR controller, to point out the filter feature.
> > But, even the SoCs integrated the same DDRC_ID, just say that they have the
> same DDRC controller.
> >
> >  From user space, the usage is different, for example:
> >
> > i.MX8MM and i.MX8MN, they use the same driver(DDRC_ID) and cortex-a53
> integrated.
> >
> > If we want to monitor VPU, their *master id* is different from SoCs.
> > On i.MX8MM, event is imx8_ddr0/axid-read,axi_id=0x08/ On i.MX8MN,
> > event is imx8_ddr0/axid-read,axi_id=0x12/
> >
> > I try to write a JSON file to use these events, for now, I only can
> > locate the file at the directory:
> > tools/perf/pmu-events/arch/arm64/arm/cortex-a53/
> >
> > Perf tool loads all events when CPUID matched, which is now unreasonable,
> we want related events are loaded for specific SoC.
> 
> so we could have a folder like .../arch/arm64/nxp/system for these JSONs. The
> perf tool can be updated to handle CPU and system events in separate folders.
> 
> >
> > All events will also be loaded if we use DDRC_ID to match in the future, this
> seems to not be a good ideal.
> 
> The important part is knowing which events are supported per implementation.
> Is there any method in the driver of knowing the specific implementation, like
> any DT compat string? Least preferred option would be DT machine ID.

I think, NO, master id could be different even they use the same DT compatible string.

Best Regards,
Joakim Zhang
> >
> 
> John
John Garry Feb. 19, 2020, 2:28 p.m. UTC | #14
>>> i.MX8MM and i.MX8MN, they use the same driver(DDRC_ID) and cortex-a53
>> integrated.
>>>
>>> If we want to monitor VPU, their *master id* is different from SoCs.
>>> On i.MX8MM, event is imx8_ddr0/axid-read,axi_id=0x08/ On i.MX8MN,
>>> event is imx8_ddr0/axid-read,axi_id=0x12/
>>>

So it seems that this master id and the axi_id are the same, which is 
some filtering key. Indeed, the actual event number is the same between 
SoC implementations.

And metric groups do support filtering, AFAIU.

>>> I try to write a JSON file to use these events, for now, I only can
>>> locate the file at the directory:
>>> tools/perf/pmu-events/arch/arm64/arm/cortex-a53/
>>>
>>> Perf tool loads all events when CPUID matched, which is now unreasonable,
>> we want related events are loaded for specific SoC.
>>
>> so we could have a folder like .../arch/arm64/nxp/system for these JSONs. The
>> perf tool can be updated to handle CPU and system events in separate folders.
>>
>>>
>>> All events will also be loaded if we use DDRC_ID to match in the future, this
>> seems to not be a good ideal.
>>
>> The important part is knowing which events are supported per implementation.
>> Is there any method in the driver of knowing the specific implementation, like
>> any DT compat string? Least preferred option would be DT machine ID.
> 
> I think, NO, master id could be different even they use the same DT compatible string.

Are you sure? Checking the dts files for your SoCs, I see this:

~/kernel-dev6/arch/arm64$ git grep  "fsl,imx8m-ddr-pmu"
boot/dts/freescale/imx8mm.dtsi:                 compatible = 
"fsl,imx8mm-ddr-pmu", "fsl,imx8m-ddr-pmu";
boot/dts/freescale/imx8mn.dtsi:                 compatible = 
"fsl,imx8mn-ddr-pmu", "fsl,imx8m-ddr-pmu";
boot/dts/freescale/imx8mq.dtsi:                 compatible = 
"fsl,imx8mq-ddr-pmu", "fsl,imx8m-ddr-pmu";

So it seems that you could use "fsl,imx8mm-ddr-pmu" vs 
"fsl,imx8mn-ddr-pmu" to differentiate, right?

John