Re: [PATCH 3/8] perf: pmuv3: Add common defines for the PMU version

From: Marc Zyngier <maz@kernel.org>
To: Zaid Al-Bassam <zalbassam@google.com>
Cc: Jesus Sanchez-Palencia <jesussanp@google.com>,
	Russell King <linux@armlinux.org.uk>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Zenghui Yu <yuzenghui@huawei.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	kvmarm@lists.linux.dev, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 3/8] perf: pmuv3: Add common defines for the PMU version
Date: Wed, 08 Feb 2023 11:20:16 +0000	[thread overview]
Message-ID: <86mt5oz9n3.wl-maz@kernel.org> (raw)
In-Reply-To: <20230126204444.2204061-4-zalbassam@google.com>

On Thu, 26 Jan 2023 20:44:39 +0000,
Zaid Al-Bassam <zalbassam@google.com> wrote:
> 
> The current PMU version defines are available for arm64 only,
> As we want to add PMUv3 support to arm (32-bit), this patch makes
> these defines available for both arm/arm64 by defining them in
> the common arm_pmuv3.h header.
> 
> Signed-off-by: Zaid Al-Bassam <zalbassam@google.com>
> ---
>  drivers/perf/arm_pmuv3.c       | 8 ++++----
>  include/linux/perf/arm_pmuv3.h | 6 ++++++
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 94e4098b662d..505f0758260c 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -392,7 +392,7 @@ static const struct attribute_group armv8_pmuv3_caps_attr_group = {
>   */
>  static bool armv8pmu_has_long_event(struct arm_pmu *cpu_pmu)
>  {
> -	return (cpu_pmu->pmuver >= ID_AA64DFR0_EL1_PMUVer_V3P5);
> +	return (cpu_pmu->pmuver >= ARMV8_PMU_DFR_VER_V3P5);

This doesn't really makes any sense. As per the architecture spec,
PMUv3p5 on AArch32 cannot expose the top 32 bits (DDI0487I.a, G8.4.10
"PMEVCNTR<n>, Performance Monitors Event Count Registers, n = 0 -
30"):

<quote>
There is no means to access bits [63:32] directly from AArch32 state.
</quote>

So on AArch32, this should always return false, no ifs, no buts.

Also, turning the architectural symbols (ID_AA64DFR0_EL1_*) into
custom stuff is a total non-starter. We generate these names and want
to use them everywhere.

Either you abstract them in the architecture specific headers, or you
define the AArch64 name in the AArch32 code.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.