On Wed, Feb 07, 2018 at 03:16:39PM +0000, Suzuki K Poulose wrote: > On 07/02/18 10:37, Dave Martin wrote: > >On Wed, Jan 31, 2018 at 06:27:51PM +0000, Suzuki K Poulose wrote:
[...] > >>As such there is no change in how the capabilities are treated. > >> > >>Cc: Dave Martin <[email protected]> > >>Signed-off-by: Suzuki K Poulose <[email protected]> > > > >A few minor nits in the documentation, otherwise > > > >Reviewed-by: Dave Martin <[email protected]> > > > >>--- > >> arch/arm64/include/asm/cpufeature.h | 90 > >> ++++++++++++++++++++++++++++++++++--- > >> arch/arm64/kernel/cpu_errata.c | 8 ++-- > >> arch/arm64/kernel/cpufeature.c | 38 ++++++++-------- > >> 3 files changed, 107 insertions(+), 29 deletions(-) > >> > >>diff --git a/arch/arm64/include/asm/cpufeature.h > >>b/arch/arm64/include/asm/cpufeature.h > >>index 7925e40c6ded..05da54f1b4c7 100644 > >>--- a/arch/arm64/include/asm/cpufeature.h > >>+++ b/arch/arm64/include/asm/cpufeature.h > >>@@ -86,16 +86,89 @@ struct arm64_ftr_reg { > >> extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; > >>-/* scope of capability check */ > >>-enum { > >>- SCOPE_SYSTEM, > >>- SCOPE_LOCAL_CPU, > >>-}; > >>+/* > >>+ * CPU capabilities: > >>+ * > >>+ * We use arm64_cpu_capabilities to represent system features, errata work > >>+ * arounds (both used internally by kernel and tracked in cpu_hwcaps) and > >>+ * ELF HWCAPs (which are exposed to user). > >>+ * > >>+ * To support systems with heterogeneous CPUs, we need to make sure that we > >>+ * detect the capabilities correctly on the system and take appropriate > >>+ * measures to ensure there are not incompatibilities. > >>+ * > >>+ * This comment tries to explain how we treat the capabilities. > >>+ * Each capability has the following list of attributes : > >>+ * > >>+ * 1) Scope of Detection : The system detects a given capability by > >>performing > >>+ * 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: > >>+ * > >>+ * 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 > >>+ * CPUs, until the system decides that state of the capability is > >>finalised. > >>+ * (See section 2 below) > >>+ * Or > >>+ * b) SCOPE_SYSTEM: check all the CPUs and "detect" if all the CPUs > >>matches. > >>+ * This implies, we run the check only once, when the system > >>decides to > >>+ * finalise the state of the capability. If the capability relies > >>on a > >>+ * 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. > >>+ * The process of detection is usually denoted by "update" capability > >>state > >>+ * in the code. > >>+ * > >>+ * 2) Finalise the state : The kernel should finalise the state of a > >>capability > >>+ * at some point during its execution and take necessary actions if > >>any. Usually, > >>+ * this is done, after all the boot-time enabled CPUs are brought up by > >>the > >>+ * kernel, so that it can make better decision based on the available > >>set > >>+ * of CPUs. However, there are some special cases, where the action is > >>taken > >>+ * during the early boot by the primary boot CPU. (e.g, running the > >>kernel at > >>+ * EL2 with Virtualisation Host Extensions). The kernel usually > >>disallows > >>+ * any changes to the state of a capability once it finalises the > >>capability > >>+ * and takes any action, as it may be impossible to execute the actions > >>safely. > >>+ * > >>+ * 3) Verification: When a CPU is brought online (e.g, by user or by the > >>kernel), > >>+ * the kernel should make sure that it is safe to use the CPU, by > >>verifying > >>+ * that the CPU is compliant with the state of the capabilities > >>established > > > >Nit: can we say "finalised" instead of "established"? > > > >There could be doubt about precisely what "established" means. > >"Finalised" is clearly defined in (2) -- I'm assuming that's the > >intended meaning here (?) > > You're right. It should be "Finalised". > > > > >>+ * already. This happens via : > >>+ * secondary_start_kernel()-> check_local_cpu_capabilities() -> > >>+ * check_early_cpu_features() && verify_local_cpu_capabilities() > > > >Nit: Maybe just say "via secondart_start_kernel()"? Too much detail > >about the exact code flow may become wrong in the future when someone > >refactors the code. > > Sure. We could say secondary_start_kernel-> check_local_cpu_capabilities(). Yes, that seems enough. > > > >>+ * > >>+ * As explained in (2) above, capabilities could be finalised at > >>different > >>+ * points in the execution. Each CPU is verified against the "finalised" > >>+ * capabilities and if there is a conflict, the kernel takes an action, > >>based > >>+ * on the severity (e.g, a CPU could be prevented from booting or cause > >>a > >>+ * kernel panic). The CPU is allowed to "affect" the state of the > >>capability, > >>+ * if it has not been finalised already. > >>+ * > >>+ * 4) Action: As mentioned in (2), the kernel can take an action for each > >>detected > >>+ * capability, on all CPUs on the system. This is always initiated only > >>after > > > >Nit: maybe clarify what an action is, e.g. > >"Appropriate actions include patching in alternatives, turning on an > >architectural feature or activating errata workarounds." > > See below. > > > > >Can we can that it is the job of the cpu_enable() method to perform the > >appropriate action, or is that not universally true? > > > > It is not completely true. e.g we don't patch in alternatives from "enable" > call back. > They are batched and performed after we have "taken actions" (i.e after > enable_cpu_capabilites() ). But all CPU control specific changes are > performed from > cpu_enable(). > > So we could say: > > "Appropriate actions include turning on an architectural feature or > changing the CPU control bits (e.g SCTLR or TCR). Patching in > alternatives for the capabilities are are -> is ? > batched and is performed separately" Ah, OK. Yes that seems fine. Happy to keep my Reviewed-by with those edits. Cheers ---Dave
