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 7930D18E25; Mon, 16 Dec 2024 14:31:46 +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=1734359506; cv=none; b=MVF9ehmRhaoE550DBgmJR011wNi6pkbE/R6S+4PYR+BpYe3X6TnYh7QTLIE6CGSEoGOj+seJZT7r80rcWyR0BaVn/O/2ptdRHTHjFiC66LUnKX8z6HFyI+0uuQNwOnb118sXeRLme/eYEgpCm+S0bpz1MgN23eFeDImKKnxYmZ0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734359506; c=relaxed/simple; bh=q//uXuHw5NNyHRrYJA+t5Do7tOiMP5mB0zY7GpysFuw=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=kp4YEf3R5CLjrVwez8WLZMZllgyDIZC5lOme62EF83vmPFXE3AVj1mQv0G7u9GRwc1oZkYxPHeBuWl9F5QeRDcnM0NB5sb+5e2Tp5y5RIRh4kSYBJChEKdiqVEWGKqWmp4ewMMl7QrzjVUS0aDlQeXlnFFL/kRQOZZXhulNGZH0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=avK1n6GR; 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="avK1n6GR" Received: by smtp.kernel.org (Postfix) with ESMTPSA id EE72EC4CED0; Mon, 16 Dec 2024 14:31:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734359506; bh=q//uXuHw5NNyHRrYJA+t5Do7tOiMP5mB0zY7GpysFuw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=avK1n6GRfEbGCiA5MGkHoGcE+AINRnA4Gm4Gxqm4ZzgfVfxrRshODochg9mUYzZoA EuaMvt+DZPrqbCSp17+I2Qt+UtkV5f7U2hMg47Xq8me29/Jo4ceCrd0mkFdqAdQ+K+ 0/4PjG18v2Wih8ks4QULfOjTjEiCgWZpwGFG6+dz7MSBwEi6bgyD/7ojs5jH8t8ngb dcL/rq39tKYYQPEFKw5HTN7UEAKAATnPq/+9hL7w3Gz70NEBiydO7aVqNTU54oXK4K wXAS49UjQ6842fF2nciDYfbnEmOJVId1jXhIujipWFqyR5Tf9UQIQlsoEtmCFm47x8 nZLFKKiHX7Gog== 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.95) (envelope-from ) id 1tNC8N-004CsH-N6; Mon, 16 Dec 2024 14:31:43 +0000 Date: Mon, 16 Dec 2024 14:31:43 +0000 Message-ID: <865xnjsnqo.wl-maz@kernel.org> From: Marc Zyngier To: Mark Rutland Cc: Mark Brown , Catalin Marinas , Will Deacon , Peter Collingbourne , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] arm64/sme: Move storage of reg_smidr to __cpuinfo_store_cpu() In-Reply-To: References: <20241214-arm64-fix-boot-cpu-smidr-v1-1-0745c40772dd@kernel.org> <87a5cysfci.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/29.4 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) X-TUID: 7+uNRFeFxaHZ Precedence: bulk X-Mailing-List: linux-kernel@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: mark.rutland@arm.com, broonie@kernel.org, catalin.marinas@arm.com, will@kernel.org, pcc@google.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, stable@vger.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 Mon, 16 Dec 2024 12:38:17 +0000, Mark Rutland wrote: > > On Sat, Dec 14, 2024 at 10:56:13AM +0000, Marc Zyngier wrote: > > > Why isn't the following a good enough fix? It makes it plain that > > boot_cpu_data is only a copy of CPU0's initial boot state. > > > > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > > index d79e88fccdfce..0cbb42fd48850 100644 > > --- a/arch/arm64/kernel/cpuinfo.c > > +++ b/arch/arm64/kernel/cpuinfo.c > > @@ -497,6 +497,6 @@ void __init cpuinfo_store_boot_cpu(void) > > struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0); > > __cpuinfo_store_cpu(info); > > > > + init_cpu_features(info); > > boot_cpu_data = *info; > > - init_cpu_features(&boot_cpu_data); > > } > > I think that change in isolation is fine, but I don't think that's the > right fix. > > I think that what we did in commit: > > 892f7237b3ff ("arm64: Delay initialisation of cpuinfo_arm64::reg_{zcr,smcr}") > > ... introduces an anti-pattern that'd be nice to avoid. That broke the > existing split of __cpuinfo_store_cpu() and init_cpu_features(), where > the former read the ID regs, and the latter set up the features > *without* altering the copy of the ID regs that was read. i.e. > init_cpu_features() shouldn't write to its info argument at all. > > I understand that we have to do something as a bodge for broken FW which > traps SME, but I'd much rather we did that within __cpuinfo_store_cpu(). Honestly, I'd rather revert that patch, together with b3000e2133d8 ("arm64: Add the arm64.nosme command line option"). I'm getting tired of the FW nonsense, and we are only allowing vendors to ship untested crap. Furthermore, given the state of SME in the kernel, I don't think this is makes any difference. So maybe this is the right time to reset everything to a sane state. > Can we add something to check whether SME was disabled on the command > line, and use that in __cpuinfo_store_cpu(), effectively reverting > 892f7237b3ff? Maybe, but that'd be before any sanitisation of the overrides, so it would have to severely limit its scope. Something like this, which I haven't tested: diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index d79e88fccdfce..9e9295e045009 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -492,10 +492,22 @@ void cpuinfo_store_cpu(void) update_cpu_features(smp_processor_id(), info, &boot_cpu_data); } +static void cpuinfo_apply_overrides(struct cpuinfo_arm64 *info) +{ + if (FIELD_GET(ID_AA64PFR0_EL1_SVE, id_aa64pfr0_override.mask) && + !FIELD_GET(ID_AA64PFR0_EL1_SVE, id_aa64pfr0_override.val)) + info->reg_id_aa64pfr0 &= ~ID_AA64PFR0_EL1_SVE; + + if (FIELD_GET(ID_AA64PFR1_EL1_SME, id_aa64pfr1_override.mask) && + !FIELD_GET(ID_AA64PFR1_EL1_SME, id_aa64pfr1_override.val)) + info->reg_id_aa64pfr1 &= ~ID_AA64PFR1_EL1_SME; +} + void __init cpuinfo_store_boot_cpu(void) { struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0); __cpuinfo_store_cpu(info); + cpuinfo_apply_overrides(info); boot_cpu_data = *info; init_cpu_features(&boot_cpu_data); But this will have ripple effects on the rest of the override code (the kernel messages are likely to be wrong). M. -- Without deviation from the norm, progress is not possible.