From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ECC8422DF99; Tue, 25 Nov 2025 15:01:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764082882; cv=none; b=hqEGLxmz7NJaI1sUyOCskmE96JwBe4KkEkegQI4qOVGy9E853aYC4eZUUiiqIOvBdt8kl2oX/WB1WsAEz1mC/x6bRGV+iPYAmkYsgrxMbno7qQo7Cv33DDg3kQdoLVsRvCRxLaQy3CmTNOFrPCA4WQmNTiBevq3t5G9Kdn5S+Yk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764082882; c=relaxed/simple; bh=WAkDYGjEjUD6loMdNqwRHWo0dXAwvjDZthFysfdybfY=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=fbd0l9oW1WCs9jGjJVrD2SGr3agfZgFhssYWPY84P1Z0Ps6VfakekPb4BEWEJWyaHdciBM5FqomqpRvwgtotOpGibznmovdCkk5a3vNDWjvrAcsLDk20ENoPO5WKt2BBPeOFXUKxNsoKceEOvVf0hg9DsQQXJ4Hu7y4ppQofOiM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=caV/PRL1; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="caV/PRL1" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A8095C4CEF1; Tue, 25 Nov 2025 15:01:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1764082881; bh=WAkDYGjEjUD6loMdNqwRHWo0dXAwvjDZthFysfdybfY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=caV/PRL14ot/Y+cOB89A7o7aOrIeRtUmTlEc56geEq6BoQfZp5mMAw8zTglT7wys+ U2qhHZ5kxWStK2UMaZ7R5t1EmyGzvWI+LITjvpjLDpIlFFwbgO4Bxldj0+38dpl47R bcmHcDHWDP7WQWPFpPMH978wyFQ8IlKrij6DC5p5vZ/HXDE5Aye7RWjhEA6X5db2dn SCEUImDkdAD2tBIIXXcu/xVG/hW+7G6rpsihcyNAMQINmQnwsYOXDPhyMUeNEoTlpj eccuQ4Jr/nxnPQak5x7GLkCGQoHO/xEWhRQSgD2noW7XsgUalXLP+qWnS3BZZxnMSn WNd3at1lXxvoQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1vNuXf-00000008Bo7-1jvm; Tue, 25 Nov 2025 15:01:19 +0000 Date: Tue, 25 Nov 2025 15:01:18 +0000 Message-ID: <86fra2qhq9.wl-maz@kernel.org> From: Marc Zyngier To: Suzuki K Poulose Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, Joey Gouly , Oliver Upton , Zenghui Yu , Christoffer Dall , Fuad Tabba , Mark Brown Subject: Re: [PATCH v4 06/49] KVM: arm64: GICv3: Detect and work around the lack of ICV_DIR_EL1 trapping In-Reply-To: References: <20251120172540.2267180-1-maz@kernel.org> <20251120172540.2267180-7-maz@kernel.org> <5df713d4-8b79-4456-8fd1-707ca89a61b6@arm.com> <86h5uiql4b.wl-maz@kernel.org> 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/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: suzuki.poulose@arm.com, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, joey.gouly@arm.com, oupton@kernel.org, yuzenghui@huawei.com, christoffer.dall@arm.com, tabba@google.com, broonie@kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Tue, 25 Nov 2025 14:14:25 +0000, Suzuki K Poulose wrote: > > On 25/11/2025 13:48, Marc Zyngier wrote: > > On Tue, 25 Nov 2025 11:26:10 +0000, > > Suzuki K Poulose wrote: > >> > >> On 20/11/2025 17:24, Marc Zyngier wrote: > >>> A long time ago, an unsuspecting architect forgot to add a trap > >>> bit for ICV_DIR_EL1 in ICH_HCR_EL2. Which was unfortunate, but > >>> what's a bit of spec between friends? Thankfully, this was fixed > >>> in a later revision, and ARM "deprecates" the lack of trapping > >>> ability. > >>> > >>> Unfortuantely, a few (billion) CPUs went out with that defect, > >>> anything ARMv8.0 from ARM, give or take. And on these CPUs, > >>> you can't trap DIR on its own, full stop. > >>> > >>> As the next best thing, we can trap everything in the common group, > >>> which is a tad expensive, but hey ho, that's what you get. You can > >>> otherwise recycle the HW in the neaby bin. > >>> > >>> Tested-by: Fuad Tabba > >>> Signed-off-by: Marc Zyngier > >>> --- > >>> arch/arm64/include/asm/virt.h | 7 ++++- > >>> arch/arm64/kernel/cpufeature.c | 52 ++++++++++++++++++++++++++++++++++ > >>> arch/arm64/kernel/hyp-stub.S | 5 ++++ > >>> arch/arm64/kvm/vgic/vgic-v3.c | 3 ++ > >>> arch/arm64/tools/cpucaps | 1 + > >>> 5 files changed, 67 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h > >>> index aa280f356b96a..8eb63d3294974 100644 > >>> --- a/arch/arm64/include/asm/virt.h > >>> +++ b/arch/arm64/include/asm/virt.h > >>> @@ -40,8 +40,13 @@ > >>> */ > >>> #define HVC_FINALISE_EL2 3 > >>> +/* > >>> + * HVC_GET_ICH_VTR_EL2 - Retrieve the ICH_VTR_EL2 value > >>> + */ > >>> +#define HVC_GET_ICH_VTR_EL2 4 > >>> + > >>> /* Max number of HYP stub hypercalls */ > >>> -#define HVC_STUB_HCALL_NR 4 > >>> +#define HVC_STUB_HCALL_NR 5 > >>> /* Error returned when an invalid stub number is passed into x0 > >>> */ > >>> #define HVC_STUB_ERR 0xbadca11 > >>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > >>> index 5ed401ff79e3e..5de51cb1b8fe2 100644 > >>> --- a/arch/arm64/kernel/cpufeature.c > >>> +++ b/arch/arm64/kernel/cpufeature.c > >>> @@ -2303,6 +2303,49 @@ static bool has_gic_prio_relaxed_sync(const struct arm64_cpu_capabilities *entry > >>> } > >>> #endif > >>> +static bool can_trap_icv_dir_el1(const struct > >>> arm64_cpu_capabilities *entry, > >>> + int scope) > >>> +{ > >>> + static const struct midr_range has_vgic_v3[] = { > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M1_ICESTORM), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M1_FIRESTORM), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M1_ICESTORM_PRO), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M1_FIRESTORM_PRO), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M1_ICESTORM_MAX), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M1_FIRESTORM_MAX), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M2_BLIZZARD), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M2_AVALANCHE), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M2_BLIZZARD_PRO), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M2_AVALANCHE_PRO), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M2_BLIZZARD_MAX), > >>> + MIDR_ALL_VERSIONS(MIDR_APPLE_M2_AVALANCHE_MAX), > >>> + {}, > >>> + }; > >>> + struct arm_smccc_res res = {}; > >>> + > >>> + BUILD_BUG_ON(ARM64_HAS_ICH_HCR_EL2_TDIR <= ARM64_HAS_GICV3_CPUIF); > >>> + BUILD_BUG_ON(ARM64_HAS_ICH_HCR_EL2_TDIR <= ARM64_HAS_GICV5_LEGACY); > >>> + if (!cpus_have_cap(ARM64_HAS_GICV3_CPUIF) && > >>> + !is_midr_in_range_list(has_vgic_v3)) > >>> + return false; > >>> + > >>> + if (!is_hyp_mode_available()) > >>> + return false; > >>> + > >>> + if (cpus_have_cap(ARM64_HAS_GICV5_LEGACY)) > >>> + return true; > >>> + > >>> + if (is_kernel_in_hyp_mode()) > >>> + res.a1 = read_sysreg_s(SYS_ICH_VTR_EL2); > >>> + else > >>> + arm_smccc_1_1_hvc(HVC_GET_ICH_VTR_EL2, &res); > >> > >> We are reading the register on the current CPU and this capability, > >> being a SYSTEM_FEATURE, relies on the "probing CPU". If there CPUs > >> with differing values (which I don't think is practical, but hey, > >> never say..). This is would better be a > >> ARM64_CPUCAP_EARLY_LOCAL_CPU_FEATURE, which would run through all > >> boot CPUs and would set the capability when it matches. > > > > While I agree that SYSTEM_FEATURE is most probably the wrong thing, I > > can't help but notice that > > > > - ARM64_HAS_GICV3_CPUIF, > > - ARM64_HAS_GIC_PRIO_MASKING > > - ARM64_HAS_GIC_PRIO_RELAXED_SYNC > > > > are all ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE. > > Which means, if GICV3_CPUIF is set any booting CPU must have them. > > > > > On the other ARM64_HAS_GICV5_LEGACY is ARM64_CPUCAP_EARLY_LOCAL_CPU_FEATURE. > > > > Given that ARM64_HAS_ICH_HCR_EL2_TDIR is dependent on both > > ARM64_HAS_GICV3_CPUIF and ARM64_HAS_GICV5_LEGACY, shouldn't these two > > (and their dependencies) be aligned to have the same behaviour? > > Yes, but it also depends on how you are detecting the feature. > > BOOT_CPU_FEATURES are finalized with BOOT CPU and thusanything that > boots up later must have it. So it is fine, as long > as a SYSTEM cap depends on it and can do its own additional checks > in a system wide safe manner. I had a quick go at it, and things are a bit murky. As it turns out, the GICv3 stuff is STRICT_BOOT_CPU_FEATURE because we need the GIC super early, before secondary CPUs can be brought up. However, virtualisation-specific features only get used way down the line, so both ICH_HCR_EL2_TDIR and GICV5_LEGACY can be delayed until all CPUs have booted. > But in your case, we a have SYSTEM cap, which only performs the check > on the "running CPU" and not considering a system wide safe value for > ICH_VTR_EL2_TDS. In order to do this, we need to switch to something > like the GICV5_LEGACY cap, which chooses to perform the check on all > booting CPUs (as we don't keep the system wide safe value for the > bit it is looking for). > > e.g., your cap may be incorrectly SET, when the BOOT cpu (which > mostly runs the SYSTEM scope), but another CPU didn't have the > ICH_VTR_EL2_TDS. Yup, that's the conclusion I also came to. > Does that help ? It does. I came up with the fix below, which I'll spin as a patch shortly. Thanks, M. diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index ff2b05f7226a9..c840a93b9ef95 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -2326,14 +2326,14 @@ static bool can_trap_icv_dir_el1(const struct arm64_cpu_capabilities *entry, BUILD_BUG_ON(ARM64_HAS_ICH_HCR_EL2_TDIR <= ARM64_HAS_GICV3_CPUIF); BUILD_BUG_ON(ARM64_HAS_ICH_HCR_EL2_TDIR <= ARM64_HAS_GICV5_LEGACY); - if (!cpus_have_cap(ARM64_HAS_GICV3_CPUIF) && + if (!this_cpu_has_cap(ARM64_HAS_GICV3_CPUIF) && !is_midr_in_range_list(has_vgic_v3)) return false; if (!is_hyp_mode_available()) return false; - if (cpus_have_cap(ARM64_HAS_GICV5_LEGACY)) + if (this_cpu_has_cap(ARM64_HAS_GICV5_LEGACY)) return true; if (is_kernel_in_hyp_mode()) @@ -2864,7 +2864,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { */ .desc = "ICV_DIR_EL1 trapping", .capability = ARM64_HAS_ICH_HCR_EL2_TDIR, - .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .type = ARM64_CPUCAP_EARLY_LOCAL_CPU_FEATURE, .matches = can_trap_icv_dir_el1, }, #ifdef CONFIG_ARM64_E0PD -- Without deviation from the norm, progress is not possible.