Re: [PATCH v3 1/3] arm64: perf: Add support caps in sysfs

[Will Deacon <will@kernel.org>]

On Thu, Jun 18, 2020 at 09:35:42PM +0800, Shaokun Zhang wrote:
> ARMv8.4-PMU introduces the PMMIR_EL1 registers and some new PMU events,
> like STALL_SLOT etc, are related to it. Let's add a caps directory to
> /sys/bus/event_source/devices/armv8_pmuv3_0/ and support slots from
> PMMIR_EL1 registers in this entry. The user programs can get the slots
> from sysfs directly.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> ---
> ChangeLog in v3:
>     * Fix one typo in patch3
> 
> ChangeLog in v2:
>     * Add caps entry in sysfs
>     * Fix the PMU events typos
>     * Add one new patch to correct event ID in sysfs
> 
>  arch/arm64/include/asm/sysreg.h |  2 +
>  arch/arm64/kernel/perf_event.c  | 87 +++++++++++++++++++++++++++++++----------
>  include/linux/perf/arm_pmu.h    |  1 +
>  3 files changed, 69 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 463175f80341..56c45a9207c7 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -321,6 +321,8 @@
>  #define SYS_PMINTENSET_EL1		sys_reg(3, 0, 9, 14, 1)
>  #define SYS_PMINTENCLR_EL1		sys_reg(3, 0, 9, 14, 2)
>  
> +#define SYS_PMMIR_EL1			sys_reg(3, 0, 9, 14, 6)
> +
>  #define SYS_MAIR_EL1			sys_reg(3, 0, 10, 2, 0)
>  #define SYS_AMAIR_EL1			sys_reg(3, 0, 10, 3, 0)
>  
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 4d7879484cec..5f2ac87e4b91 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -277,6 +277,51 @@ static struct attribute_group armv8_pmuv3_format_attr_group = {
>  	.attrs = armv8_pmuv3_format_attrs,
>  };
>  
> +static inline int armv8pmu_get_pmu_version(void)
> +{

No need for 'inline' here.

> +	int pmuver;
> +	u64 dfr0;
> +
> +	dfr0 = read_sysreg(id_aa64dfr0_el1);
> +	pmuver = cpuid_feature_extract_unsigned_field(dfr0,
> +			ID_AA64DFR0_PMUVER_SHIFT);
> +
> +	return pmuver;
> +}
> +
> +static umode_t
> +armv8pmu_caps_attr_is_visible(struct kobject *kobj, struct attribute *attr,
> +			      int unused)
> +{
> +	int pmuver = armv8pmu_get_pmu_version();
> +
> +	if (pmuver >= ID_AA64DFR0_PMUVER_8_4)
> +		return attr->mode;

Is this sufficient? I'm a bit confused by the text in the Arm ARM that says:

  | If ARMv8.4-PMU is implemented:
  | * If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether
  |   the PMMIR System registers are implemented.
  | * If STALL_SLOT is implemented, then the PMMIR System registers are
  |   implemented.

whereas the register description for PMMIR_EL1 says:

  | This register is present only when ARMv8.4-PMU is implemented.

Mark -- please could you clarify whether or not we need to check STALL_SLOT
as well as the PMUVer?

> +
> +	return 0;
> +}
> +
> +static ssize_t slots_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int slots = read_sysreg_s(SYS_PMMIR_EL1) & 0xFF;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", slots);
> +}
> +
> +static DEVICE_ATTR_RO(slots);
> +
> +static struct attribute *armv8_pmuv3_caps_attrs[] = {
> +	&dev_attr_slots.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group armv8_pmuv3_caps_attr_group = {
> +	.name = "caps",
> +	.attrs = armv8_pmuv3_caps_attrs,
> +	.is_visible = armv8pmu_caps_attr_is_visible,
> +};
> +
>  /*
>   * Perf Events' indices
>   */
> @@ -940,14 +985,11 @@ static void __armv8pmu_probe_pmu(void *info)
>  {
>  	struct armv8pmu_probe_info *probe = info;
>  	struct arm_pmu *cpu_pmu = probe->pmu;
> -	u64 dfr0;
>  	u64 pmceid_raw[2];
>  	u32 pmceid[2];
>  	int pmuver;
>  
> -	dfr0 = read_sysreg(id_aa64dfr0_el1);
> -	pmuver = cpuid_feature_extract_unsigned_field(dfr0,
> -			ID_AA64DFR0_PMUVER_SHIFT);
> +	pmuver = armv8pmu_get_pmu_version();
>  	if (pmuver == 0xf || pmuver == 0)
>  		return;
>  
> @@ -994,7 +1036,8 @@ static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu)
>  static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>  			  int (*map_event)(struct perf_event *event),
>  			  const struct attribute_group *events,
> -			  const struct attribute_group *format)
> +			  const struct attribute_group *format,
> +			  const struct attribute_group *caps)
>  {
>  	int ret = armv8pmu_probe_pmu(cpu_pmu);
>  	if (ret)
> @@ -1019,6 +1062,8 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>  			events : &armv8_pmuv3_events_attr_group;
>  	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] = format ?
>  			format : &armv8_pmuv3_format_attr_group;
> +	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_CAPS] = caps ?
> +			caps : &armv8_pmuv3_caps_attr_group;
>  
>  	return 0;
>  }
> @@ -1026,97 +1071,97 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>  static int armv8_pmuv3_init(struct arm_pmu *cpu_pmu)
>  {
>  	return armv8_pmu_init(cpu_pmu, "armv8_pmuv3",
> -			      armv8_pmuv3_map_event, NULL, NULL);
> +			      armv8_pmuv3_map_event, NULL, NULL, NULL);

Maybe we should add:

static int armv8_pmu_init_nogroups(struct arm_pmu *cpu_pmu, char *name,
				   int (*map_event)(struct perf_event *event))
{
	return armv8_pmu_init(cpu_pmu, name, map_event, NULL, NULL, NULL);
}

and then update all these CPU initialisers to use that instead?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel