Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

From: Suzuki K Poulose
Date: Wed Jan 17 2018 - 09:52:19 EST


On 17/01/18 14:38, Dave Martin wrote:
On Wed, Jan 17, 2018 at 01:22:19PM +0000, Suzuki K Poulose wrote:
On 17/01/18 12:25, Dave Martin wrote:
On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
When a CPU is brought up after we have finalised the system
wide capabilities (i.e, features and errata), we make sure the
new CPU doesn't need a new errata work around which has not been
detected already. However we don't run enable() method on the new
CPU for the errata work arounds already detected. This could
cause the new CPU running without potential work arounds.
It is upto the "enable()" method to decide if this CPU should
do something about the errata.

Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
Cc: Will Deacon <will.deacon@xxxxxxx>
Cc: Mark Rutland <mark.rutland@xxxxxxx>
Cc: Andre Przywara <andre.przywara@xxxxxxx>
Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
Cc: Dave Martin <dave.martin@xxxxxxx>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
---
arch/arm64/kernel/cpu_errata.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 90a9e465339c..54e41dfe41f6 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
{
const struct arm64_cpu_capabilities *caps = arm64_errata;
- for (; caps->matches; caps++)
- if (!cpus_have_cap(caps->capability) &&
- caps->matches(caps, SCOPE_LOCAL_CPU)) {
+ for (; caps->matches; caps++) {
+ if (cpus_have_cap(caps->capability)) {
+ if (caps->enable)
+ caps->enable((void *)caps);

Do we really need this cast?

Yes, otherwise we would be passing a "const *" where a "void *" is expected,
and the compiler warns. Or we could simply change the prototype of the
enable() method to accept a const capability ptr.

Hmmm, what is this argument for exactly? cpufeature.h doesn't explain
what it is.

This was introduced by commit 0a0d111d40fd1 ("arm64: cpufeature: Pass capability
structure to ->enable callback").

The idea is to enable multiple entries in the table for a single capability.
Some capabilities (read errata) could be detected in multiple ways. e.g, different
MIDR ranges. (e.g, ARM64_HARDEN_BRANCH_PREDICTOR, ARM64_WORKAROUND_QCOM_FALKOR_E1003.
Now, even though the errata is the same, there might be different work arounds
for them in each "matching" cases. So, we need the "caps" passed on to the
enable() method to see if the "specific work around" should be applied
to the system/CPU (since CPU hwcap could be set by one of the cpu_capabilities
entry.) (as we invoke enable() for all "available" capabilities.)

Passing the caps information makes it easier to decide, by using the caps->matches().


Does any enable method use this for anything other than a struct
arm64_cpu_capabilities const * ?

No, we use it only for const struct arm64_cpu_capability *.

If not, it would be better to specifiy that.

Yes, that could be done.

Cheers
Suzuki