Re: [PATCH 1/4] drivers/perf: Add support for ARMv8.3-SPE

From: Leo Yan <leo.yan@linaro.org>
To: "liwei (GF)" <liwei391@huawei.com>, Al Grant <Al.Grant@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Jiri Olsa <jolsa@redhat.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	linux-kernel@vger.kernel.org, zhangshaokun@hisilicon.com,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, James Clark <james.clark@arm.com>,
	guohanjun@huawei.com, Namhyung Kim <namhyung@kernel.org>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/4] drivers/perf: Add support for ARMv8.3-SPE
Date: Wed, 29 Jul 2020 15:08:55 +0800	[thread overview]
Message-ID: <20200729070855.GG4343@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <c2ce17a9-bd3a-e828-fe76-769c9feabece@huawei.com>

On Tue, Jul 28, 2020 at 09:24:42PM +0800, liwei (GF) wrote:

[...]

> >> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> >> index e51ddb6d63ed..5ec7ee0c8fa1 100644
> >> --- a/drivers/perf/arm_spe_pmu.c
> >> +++ b/drivers/perf/arm_spe_pmu.c
> >> @@ -54,7 +54,7 @@ struct arm_spe_pmu {
> >>  	struct hlist_node			hotplug_node;
> >>  
> >>  	int					irq; /* PPI */
> >> -
> >> +	int					pmuver;
> > 
> > Since the version number is only 4 bits width, 'u16' would be enough
> > to record SPE version number.
> 
> Sounds reasonable, i can change it to 'u16' if you insist.
> 
> >>  	u16					min_period;
> >>  	u16					counter_sz;
> >>  
> >> @@ -80,6 +80,15 @@ struct arm_spe_pmu {
> >>  /* Keep track of our dynamic hotplug state */
> >>  static enum cpuhp_state arm_spe_pmu_online;
> >>  
> >> +static u64 sys_pmsevfr_el1_mask[] = {
> >> +	[ID_AA64DFR0_PMSVER_8_2] = GENMASK_ULL(63, 48) | GENMASK_ULL(31, 24) |
> >> +		GENMASK_ULL(15, 12) | BIT_ULL(7) | BIT_ULL(5) | BIT_ULL(3) |
> >> +		BIT_ULL(1),
> >> +	[ID_AA64DFR0_PMSVER_8_3] = GENMASK_ULL(63, 48) | GENMASK_ULL(31, 24) |
> >> +		GENMASK_ULL(18, 17) | GENMASK_ULL(15, 11) | BIT_ULL(7) |
> >> +		BIT_ULL(5) | BIT_ULL(3) | BIT_ULL(1),
> >> +};
> > 
> > Seems to me, the definitions for Aarch64 system registers should be
> > placed into the file 'arch/arm64/include/asm/sysreg.h'.  Like below
> > two macros:
> > 
> >   #define SYS_PMSEVFR_EL1_RES0_8_2		0x0000ffff00ff0f55UL
> >   #define SYS_PMSEVFR_EL1_RES0_8_3		...
> 
> I really think using GENMASK_ULL() to generate the mask is better than a definition
> with magic number. It is beneficial to be reviewed and extended later.

Understand.  Here I just want to remind, you could see the ARMv8's
system registers definition usually are placed into the global header
sysreg.h rather than define them in separate source files.

You could define the bit mask with GENMASK_ULL() for the two macros
in sysreg.h.

> > Let's wait for Will or Mark Rutland's comments for this, in case I
> > mislead for this.
> >> +
> >>  enum arm_spe_pmu_buf_fault_action {
> >>  	SPE_PMU_BUF_FAULT_ACT_SPURIOUS,
> >>  	SPE_PMU_BUF_FAULT_ACT_FATAL,
> >> @@ -670,7 +679,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> >>  	    !cpumask_test_cpu(event->cpu, &spe_pmu->supported_cpus))
> >>  		return -ENOENT;
> >>  
> >> -	if (arm_spe_event_to_pmsevfr(event) & SYS_PMSEVFR_EL1_RES0)
> >> +	if (arm_spe_event_to_pmsevfr(event) & ~sys_pmsevfr_el1_mask[spe_pmu->pmuver])
> >>  		return -EOPNOTSUPP;
> >>  
> >>  	if (attr->exclude_idle)
> >> @@ -937,6 +946,7 @@ static void __arm_spe_pmu_dev_probe(void *info)
> >>  			fld, smp_processor_id());
> >>  		return;
> >>  	}
> >> +	spe_pmu->pmuver = fld;
> >>  
> >>  	/* Read PMBIDR first to determine whether or not we have access */
> >>  	reg = read_sysreg_s(SYS_PMBIDR_EL1);
> >> @@ -1027,8 +1037,8 @@ static void __arm_spe_pmu_dev_probe(void *info)
> >>  	}
> >>  
> >>  	dev_info(dev,
> >> -		 "probed for CPUs %*pbl [max_record_sz %u, align %u, features 0x%llx]\n",
> >> -		 cpumask_pr_args(&spe_pmu->supported_cpus),
> >> +		 "v%d probed for CPUs %*pbl [max_record_sz %u, align %u, features 0x%llx]\n",
> > 
> > Let's output explict info, like:
> > 
> >   "probed for CPUs %*pbl [pmuver %d, max_record_sz %u, align %u, features 0x%llx]\n",
> > 
> 
> Agree, and i have a little question here:
> Currently, the of_compatible of SPE PMU is "arm,statistical-profiling-extension-v1", and
> the platform_device name is "arm,spe-v1". So this message looks weird when supporting
> ARMv8.3-SPE because the pmuver is 2.

I think here we need to distinguish two things: SPE (as an IP) and
ARMv8.2/ARMv8.3 (as CPU architectures).  From my understanding, now we
are working on SPE-v1, but it needs to support ARMv8 variants, e.g.
ARMv8.2 and ARMv8.3 with SVE extension.

I am not the best person to clarify the version number for SPE, if Arm
colleagues disagree with this, very welcome to correct me.

Also loop in Al for this.

> As the version of SPE can be probed by reading 'ID_AA64DFR0_EL1.PMSVer', can we remove
> the version hint in of_compatible and platform_device name?

No, for device tree, usually we need to keep back compability for the
DT binding, so we cannot remove compatible string.

Thanks,
Leo

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