On 26/01/18 10:00, Dave Martin wrote:

On Thu, Jan 25, 2018 at 05:56:02PM +0000, Suzuki K Poulose wrote:

quoted

On 25/01/18 17:33, Dave Martin wrote:

quoted

On Tue, Jan 23, 2018 at 12:27:57PM +0000, Suzuki K Poulose wrote:

quoted

We use arm64_cpu_capabilities to represent CPU ELF HWCAPs exposed
to the userspace and the CPU hwcaps used by the kernel, which
include cpu features and CPU errata work arounds.

At the moment we have the following restricions:

  a) CPU feature hwcaps (arm64_features) and ELF HWCAPs (arm64_elf_hwcap)
    - Detected mostly on system wide CPU feature register. But
      there are some which really uses a local CPU's value to
      decide the availability (e.g, availability of hardware
      prefetch). So, we run the check only once, after all the
      boot-time active CPUs are turned on.

[ARM64_HAS_NO_HW_PREFETCH is kinda broken, but we also get away with it
presumably because the only systems to which it applies are homogeneous,
and anyway it's only an optimisation IIUC.

This could be a separate category, but as a one-off that may be a bit
pointless.

I understand and was planning to fix this back when it was introduced.
But then it was pointless at that time, given that it was always
guaranteed to be a homogeneous system. We do something about it in
Patch 9.

This was just on observation than something that needs to be fixed,
but it it's been cleaned up then so much the better :)

I'll take a look.

quoted

quoted

.def_scope == SCOPE_SYSTEM appears anomalous there, but it's also
unused in that case.]

quoted

    - Any late CPU which doesn't posses all the established features
      is killed.

Does "established feature" above ...

quoted

    - Any late CPU which possess a feature *not* already available
      is allowed to boot.

mean the same as "feature already available" here?

Yes, its the same. I should have been more consistent.

quoted

quoted

  b) CPU Errata work arounds (arm64_errata)
    - Detected mostly based on a local CPU's feature register.
      The checks are run on each boot time activated CPUs.
    - Any late CPU which doesn't have any of the established errata
      work around capabilities is ignored and is allowed to boot.
    - Any late CPU which has an errata work around not already available
      is killed.

However there are some exceptions to the cases above.

1) KPTI is a feature that we need to enable when at least one CPU needs it.
    And any late CPU that has this "feature" should be killed.

Should that be "If KPTI is not enabled during system boot, then any late
CPU that has this "feature" should be killed."

Yes.

quoted

quoted

2) Hardware DBM feature is a non-conflicting capability which could be
    enabled on CPUs which has it without any issues, even if the CPU is

have

quoted

quoted

    brought up late.

So this calls for a lot more fine grained behavior for each capability.
And if we define all the attributes to control their behavior properly,
we may be able to use a single table for the CPU hwcaps (not the
ELF HWCAPs, which cover errata and features). This is a prepartory step
to get there. We define type for a capability, which for now encodes the
scope of the check. i.e, whether it should be checked system wide or on
each local CPU. We define two types :

   1) ARM64_CPUCAP_BOOT_SYSTEM_FEATURE - Implies (a) as described above.
   1) ARM64_CPUCAP_STRICT_CPU_LOCAL_ERRATUM - Implies (b) as described above.

2)

Meaning you've got 1) twice above (in case you didn't spot it).

Yes, you're right.

quoted

quoted

quoted

As such there is no change in how the capabilities are treated.

OK, I think I finally have my head around this, more or less.

Mechanism (operations on architectural feature regs) and policy (kernel
runtime configuration) seem to be rather mixed together.  This works
fairly naturally for things like deriving the sanitised feature regs
seen by userspace and determining the ELF hwcaps; but not so naturally
for errata workarounds and other anomalous things like
ARM64_HAS_NO_HW_PREFETCH.

Right. We are stuck with "cpu_hwcaps" for both erratum and features,
based on which we make some decisions to change the kernel behavior,
as it is tied to alternative patching.

quoted

I'm not sure that there is a better approach though -- anyway, that
would be out of scope for this series.

quoted

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
  arch/arm64/include/asm/cpufeature.h | 24 +++++++++++++++++------
  arch/arm64/kernel/cpu_errata.c      |  8 ++++----
  arch/arm64/kernel/cpufeature.c      | 38 ++++++++++++++++++-------------------
  3 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index a23c0d4f27e9..4fd5de8ef33e 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h

@@ -86,16 +86,23 @@ struct arm64_ftr_reg {
  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
-/* scope of capability check */
-enum {
-	SCOPE_SYSTEM,
-	SCOPE_LOCAL_CPU,
-};
+
+/* Decide how the capability is detected. On a local CPU vs System wide */
+#define ARM64_CPUCAP_SCOPE_MASK			0x3
+#define ARM64_CPUCAP_SCOPE_LOCAL_CPU		BIT(0)
+#define ARM64_CPUCAP_SCOPE_SYSTEM		BIT(1)
+#define SCOPE_SYSTEM				ARM64_CPUCAP_SCOPE_SYSTEM
+#define SCOPE_LOCAL_CPU				ARM64_CPUCAP_SCOPE_LOCAL_CPU

Are these really orthogonal?  What would be meant by (LOCAL_CPU | SYSTEM)?

It is an unsupported configuration.

Surely meaningless, not just unsupported?

Yep.

quoted

quoted

Otherwise, perhaps they should be 0 and 1, not BIT(0), BIT(1).

It is a bit tricky. I chose separate bits to allow filter an entry in a table
of capabilities based on the bits. e.g, we want to

1) Process only the local scope (e.g detecting CPU local capabilities, where
we are not yet ready to run the system wide checks)

2) Process all the entries including local/system. (e.g, verifying all the
capabilities for a late CPU).

OK, so LOCAL_CPU and SYSTEM are mutually exclusive for the cap type, but
by making them separate bits in a bitmask then (LOCAL_CPU | SYSTEM) as a
match value will match on either.

quoted

Things get further complicated by the addition of "EARLY", where we want to
filter entries based on "EARLY" bit. So, we need to pass on a mask of bits
to the helpers, which are not just the binary scope. See Patch 7 for more
info.

quoted

quoted

+
+/* CPU errata detected at boot time based on feature of one or more CPUs */
+#define ARM64_CPUCAP_STRICT_CPU_LOCAL_ERRATUM	(ARM64_CPUCAP_SCOPE_LOCAL_CPU)

quoted

+/* CPU feature detected at boot time based on system-wide value of a feature */

I'm still not overly keen on these names, but I do at least see where
they come from now.

I will try to make this patch a bit more simpler, by not doing a forward reference
of the conflict "behavior" we introduce in the next patch and, keeping it just to
changing the field name.

Thanks a lot for the feedback.

Cheers
Suzuki