Date:   Wed, 08 Feb 2023 11:20:16 +0000
Message-ID: <86mt5oz9n3.wl-maz@kernel.org>
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
In-Reply-To: <20230126204444.2204061-4-zalbassam@google.com>
References: <20230126204444.2204061-1-zalbassam@google.com>
        <20230126204444.2204061-4-zalbassam@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.