Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework
On Thu, Apr 16, 2020 at 03:59:39PM +0100, Suzuki K Poulose wrote: > On 04/14/2020 10:31 PM, Will Deacon wrote: > > Now that Suzuki isn't within throwing distance, I thought I'd better add > > a rough overview comment to cpufeature.c so that it doesn't take me days > > to remember how it works next time. > > > > Signed-off-by: Will Deacon > > --- > > arch/arm64/kernel/cpufeature.c | 43 ++ > > 1 file changed, 43 insertions(+) > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 680a453ca8c4..421ca99dc8fc 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -3,6 +3,49 @@ > >* Contains CPU feature definitions > >* > >* Copyright (C) 2015 ARM Ltd. > > + * > > + * A note for the weary kernel hacker: the code here is confusing and hard > > to > > + * follow! That's partly because it's solving a nasty problem, but also > > because > > + * there's a little bit of over-abstraction that tends to obscure what's > > going > > + * on behind a maze of helper functions and macros. > > Thanks for writing this up ! It's purely a selfish thing ;) > > + * The basic problem is that hardware folks have started gluing together > > CPUs > > + * with distinct architectural features; in some cases even creating SoCs > > where > > + * user-visible instructions are available only on a subset of the > > available > > + * cores. We try to address this by snapshotting the feature registers of > > the > > + * boot CPU and comparing these with the feature registers of each > > secondary > > + * CPU when bringing them up. If there is a mismatch, then we update the > > + * snapshot state to indicate the lowest-common denominator of the feature, > > + * known as the "safe" value. This snapshot state can be queried to view > > the > > I am not sure if the following is implied above. > > 1) Against the "snapshot" state, where mismatches triggers updating > the "snapshot" state to reflect the "safe" value. > > 2) Compared against the CPU feature registers of *the boot CPU* for > "FTR_STRICT" fields and any mismatch triggers TAINT_CPU_OUT_OF_SPEC. > This makes sure that warning is generated for each OUT_OF_SPEC > secondary CPU. I was trying to avoid talking about the consequences of a mismatch in that paragraph, and instead cover them below: > > + * The sanitised register values are used to decide which capabilities we > > + * have in the system. These may be in the form of traditional "hwcaps" > > + * advertised to userspace or internal "cpucaps" which are used to > > configure > > + * things like alternative patching and static keys. While a feature > > mismatch > > + * may result in a TAINT_CPU_OUT_OF_SPEC kernel taint, a capability > > mismatch > > + * may prevent a CPU from being onlined at all. Do you think something is missing here? > > + * > > + * Some implementation details worth remembering: > > + * > > + * - Mismatched features are *always* sanitised to a "safe" value, which > > + * usually indicates that the feature is not supported. > > + * > > + * - A mismatched feature marked with FTR_STRICT will cause a "SANITY > > CHECK" > > + * warning when onlining an offending CPU and the kernel will be tainted > > + * with TAINT_CPU_OUT_OF_SPEC. > > As mentioned above, this check is against that of the "boot CPU" > register state, which may not be implicit from the statement. Hmm, I'm trying to figure out if this matters. I suppose this means you get a SANITY CHECK warning for every mismatching secondary CPU, but that's implied by the above. Is there something else I'm missing? > > + * > > + * - Features marked as FTR_VISIBLE have their sanitised value visible to > > + * userspace. FTR_VISIBLE features in registers that are only visible > > + * to EL0 by trapping *must* have a corresponding HWCAP so that late > > + * onlining of CPUs cannot lead to features disappearing at runtime. > > + * > > As you mentioned in the other response we could add information about > the guest view, something like : > > - KVM exposes the sanitised value of the feature registers to the > guests and is not affected by the FTR_VISIBLE. However, > depending on the individual feature support in the hypervisor, > some of the fields may be capped/limited. In light of Marc's comment, I'll add something here along the lines of: "KVM exposes its own view of the feature registers to guest operating systems regardless of FTR_VISIBLE. This is typically driven from the sanitised register values to allow virtual CPUs to be migrated between arbitrary physical CPUs, but some features not present on the host are also advertised and emulated. Look at sys_reg_descs[] for the gory details." Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework
On 2020-04-16 15:59, Suzuki K Poulose wrote: Hi Suzuki, [...] As you mentioned in the other response we could add information about the guest view, something like : - KVM exposes the sanitised value of the feature registers to the guests and is not affected by the FTR_VISIBLE. However, depending on the individual feature support in the hypervisor, some of the fields may be capped/limited. Although in most cases, what KVM exposes is indeed a strict subset of the host's features, there is a few corner cases where we expose features that do not necessarily exist on the host. For example ARMv8.5-GTG and ARMv8.4-TTL get exposed by the NV patches even if they don't exist on the host, as KVM will actually emulate them. Not a big deal, but I just wanted to outline that it isn't as clear-cut as it may seem... Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework
Hi Will, On 04/14/2020 10:31 PM, Will Deacon wrote: Now that Suzuki isn't within throwing distance, I thought I'd better add a rough overview comment to cpufeature.c so that it doesn't take me days to remember how it works next time. Signed-off-by: Will Deacon --- arch/arm64/kernel/cpufeature.c | 43 ++ 1 file changed, 43 insertions(+) diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 680a453ca8c4..421ca99dc8fc 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -3,6 +3,49 @@ * Contains CPU feature definitions * * Copyright (C) 2015 ARM Ltd. + * + * A note for the weary kernel hacker: the code here is confusing and hard to + * follow! That's partly because it's solving a nasty problem, but also because + * there's a little bit of over-abstraction that tends to obscure what's going + * on behind a maze of helper functions and macros. Thanks for writing this up ! + * + * The basic problem is that hardware folks have started gluing together CPUs + * with distinct architectural features; in some cases even creating SoCs where + * user-visible instructions are available only on a subset of the available + * cores. We try to address this by snapshotting the feature registers of the + * boot CPU and comparing these with the feature registers of each secondary + * CPU when bringing them up. If there is a mismatch, then we update the + * snapshot state to indicate the lowest-common denominator of the feature, + * known as the "safe" value. This snapshot state can be queried to view the I am not sure if the following is implied above. 1) Against the "snapshot" state, where mismatches triggers updating the "snapshot" state to reflect the "safe" value. 2) Compared against the CPU feature registers of *the boot CPU* for "FTR_STRICT" fields and any mismatch triggers TAINT_CPU_OUT_OF_SPEC. This makes sure that warning is generated for each OUT_OF_SPEC secondary CPU. + * "sanitised" value of a feature register. + * + * The sanitised register values are used to decide which capabilities we + * have in the system. These may be in the form of traditional "hwcaps" + * advertised to userspace or internal "cpucaps" which are used to configure + * things like alternative patching and static keys. While a feature mismatch + * may result in a TAINT_CPU_OUT_OF_SPEC kernel taint, a capability mismatch + * may prevent a CPU from being onlined at all. + * + * Some implementation details worth remembering: + * + * - Mismatched features are *always* sanitised to a "safe" value, which + * usually indicates that the feature is not supported. + * + * - A mismatched feature marked with FTR_STRICT will cause a "SANITY CHECK" + * warning when onlining an offending CPU and the kernel will be tainted + * with TAINT_CPU_OUT_OF_SPEC. As mentioned above, this check is against that of the "boot CPU" register state, which may not be implicit from the statement. + * + * - Features marked as FTR_VISIBLE have their sanitised value visible to + * userspace. FTR_VISIBLE features in registers that are only visible + * to EL0 by trapping *must* have a corresponding HWCAP so that late + * onlining of CPUs cannot lead to features disappearing at runtime. + * As you mentioned in the other response we could add information about the guest view, something like : - KVM exposes the sanitised value of the feature registers to the guests and is not affected by the FTR_VISIBLE. However, depending on the individual feature support in the hypervisor, some of the fields may be capped/limited. Cheers Suzuki ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework
Hi Suzuki, On Tue, Apr 14, 2020 at 10:31:14PM +0100, Will Deacon wrote: > Now that Suzuki isn't within throwing distance, I thought I'd better add > a rough overview comment to cpufeature.c so that it doesn't take me days > to remember how it works next time. > > Signed-off-by: Will Deacon > --- > arch/arm64/kernel/cpufeature.c | 43 ++ > 1 file changed, 43 insertions(+) Any chance you can look at this one, please? I don't trust myself to get all of the details right here! I'm also wondering whether we should mention something about KVM and the guest view of the registers. What do you think? Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework
Now that Suzuki isn't within throwing distance, I thought I'd better add
a rough overview comment to cpufeature.c so that it doesn't take me days
to remember how it works next time.
Signed-off-by: Will Deacon
---
arch/arm64/kernel/cpufeature.c | 43 ++
1 file changed, 43 insertions(+)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 680a453ca8c4..421ca99dc8fc 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -3,6 +3,49 @@
* Contains CPU feature definitions
*
* Copyright (C) 2015 ARM Ltd.
+ *
+ * A note for the weary kernel hacker: the code here is confusing and hard to
+ * follow! That's partly because it's solving a nasty problem, but also because
+ * there's a little bit of over-abstraction that tends to obscure what's going
+ * on behind a maze of helper functions and macros.
+ *
+ * The basic problem is that hardware folks have started gluing together CPUs
+ * with distinct architectural features; in some cases even creating SoCs where
+ * user-visible instructions are available only on a subset of the available
+ * cores. We try to address this by snapshotting the feature registers of the
+ * boot CPU and comparing these with the feature registers of each secondary
+ * CPU when bringing them up. If there is a mismatch, then we update the
+ * snapshot state to indicate the lowest-common denominator of the feature,
+ * known as the "safe" value. This snapshot state can be queried to view the
+ * "sanitised" value of a feature register.
+ *
+ * The sanitised register values are used to decide which capabilities we
+ * have in the system. These may be in the form of traditional "hwcaps"
+ * advertised to userspace or internal "cpucaps" which are used to configure
+ * things like alternative patching and static keys. While a feature mismatch
+ * may result in a TAINT_CPU_OUT_OF_SPEC kernel taint, a capability mismatch
+ * may prevent a CPU from being onlined at all.
+ *
+ * Some implementation details worth remembering:
+ *
+ * - Mismatched features are *always* sanitised to a "safe" value, which
+ * usually indicates that the feature is not supported.
+ *
+ * - A mismatched feature marked with FTR_STRICT will cause a "SANITY CHECK"
+ * warning when onlining an offending CPU and the kernel will be tainted
+ * with TAINT_CPU_OUT_OF_SPEC.
+ *
+ * - Features marked as FTR_VISIBLE have their sanitised value visible to
+ * userspace. FTR_VISIBLE features in registers that are only visible
+ * to EL0 by trapping *must* have a corresponding HWCAP so that late
+ * onlining of CPUs cannot lead to features disappearing at runtime.
+ *
+ * - A "feature" is typically a 4-bit register field. A "capability" is the
+ * high-level description derived from the sanitised field value.
+ *
+ * - Read the Arm ARM (DDI 0487F.a) section D13.1.3 ("Principles of the ID
+ * scheme for fields in ID registers") to understand when feature fields
+ * may be signed or unsigned (FTR_SIGNED and FTR_UNSIGNED accordingly).
*/
#define pr_fmt(fmt) "CPU features: " fmt
--
2.26.0.110.g2183baf09c-goog
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
