On 12/02/18 17:17, Dave Martin wrote:
On Fri, Feb 09, 2018 at 05:54:57PM +0000, Suzuki K Poulose wrote:
The kernel detects and uses some of the features based on the boot
CPU and expects that all the following CPUs conform to it. e.g,
with VHE and the boot CPU running at EL2, the kernel decides to
keep the kernel running at EL2. If another CPU is brought up without
this capability, we use custom hooks (via check_early_cpu_features())
to handle it. To handle such capabilities add support for detecting
and enabling capabilities based on the boot CPU.


diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 383c69c95f23..5f56a8342065 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -104,7 +104,7 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
   *    some checks at runtime. This could be, e.g, checking the value of a 
field
   *    in CPU ID feature register or checking the cpu model. The capability
   *    provides a call back ( @matches() ) to perform the check.
- *    Scope defines how the checks should be performed. There are two cases:
+ *    Scope defines how the checks should be performed. There are three cases:
   *
   *     a) SCOPE_LOCAL_CPU: check all the CPUs and "detect" if at least one
   *        matches. This implies, we have to run the check on all the booting
@@ -117,6 +117,11 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
   *        field in one of the CPU ID feature registers, we use the sanitised
   *        value of the register from the CPU feature infrastructure to make
   *        the decision.
+ *             Or
+ *     c) SCOPE_BOOT_CPU: Check only on the primary boot CPU to detect the 
feature.
+ *        This category is for features that are "finalised" (or used) by the 
kernel
+ *        very early even before the SMP cpus are brought up.
+ *

Nit: the overlong lines bring no benefit here.  Please wrap them if
possible -- but to avoid patch churn only bother for lines actually
changed/added by this patch.

Sure

  static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
@@ -1277,12 +1277,21 @@ __enable_cpu_capabilities(const struct 
arm64_cpu_capabilities *caps, u16 scope_m
if (caps->cpu_enable) {
                        /*
-                        * Use stop_machine() as it schedules the work allowing
-                        * us to modify PSTATE, instead of on_each_cpu() which
-                        * uses an IPI, giving us a PSTATE that disappears when
-                        * we return.
+                        * If we are dealing with a boot CPU capability, we
+                        * have to enable this only on the Boot CPU, where it
+                        * is detected. All the secondaries enable it via
+                        * check_local_cpu_capabilities().

I found this confusing to read, because it's not 100% clear whether the
"If we are dealing with a boot CPU capability" applies to the second
sentence as well.

Maybe this would be clearer as:

"Capabilities with SCOPE_BOOT_CPU are finalised before any secondary
CPU boots.  Thus, each secondary will enable the capability as
appropriate via check_local_cpu_capabilities().  The only exception is
the boot CPU, for which the capability must be enabled here.  This
approach avoids costly stop_machine() calls for this case."

Thoughts?

Definitely better, will change it.


@@ -1362,6 +1371,12 @@ static void check_early_cpu_features(void)
  {
        verify_cpu_run_el();
        verify_cpu_asid_bits();
+       /*
+        * Early features are used by the kernel already. If there
+        * is a conflict, we cannot proceed further.
+        */
+       if (!verify_local_cpu_caps(SCOPE_BOOT_CPU))
+               cpu_panic_kernel();
  }
static void
@@ -1403,9 +1418,8 @@ static void verify_sve_features(void)
   */
  static void verify_local_cpu_capabilities(void)
  {

Nit: Maybe add a comment saying where SCOPE_BOOT_CPU capabilities are
checked.

Ok


-       if (!verify_local_cpu_caps(SCOPE_ALL))
+       if (!verify_local_cpu_caps(SCOPE_ALL & ~SCOPE_BOOT_CPU))
                cpu_die_early();
-

Nit: keep blank line?

Otherwise it looks like the if() falls through, where really
cpu_die_early() does not return.


ok

  void __init setup_cpu_features(void)
--
2.14.3

With fair consideration given to the nits above:

Reviewed-by: Dave Martin <dave.mar...@arm.com>

Cheers
Suzuki

Reply via email to