Message ID | 20190806170043.35588-1-broonie@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | arm64: Clarify when cpu_enable() is called | expand |
[+Suzuki] On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote: > Strengthen the wording in the documentation for cpu_enable() to make it > more obvious to readers not already familiar with the code when the core > will call this callback and that this is intentional. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/include/asm/cpufeature.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index cf65a47ee6b4..3d8afcf687d9 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -289,9 +289,9 @@ struct arm64_cpu_capabilities { > u16 type; > bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope); > /* > - * Take the appropriate actions to enable this capability for this CPU. > - * For each successfully booted CPU, this method is called for each > - * globally detected capability. > + * Take the appropriate actions to configure this capability for this > + * CPU. This will be called on all CPUs in the system if the > + * capability is detected anywhere in the system. > */ > void (*cpu_enable)(const struct arm64_cpu_capabilities *cap); > union { That's not quite right though either, is it? We need to take into account the scope of the capability/erratum as well, since we don't /always/ call this function for everybody. Suzuki, are there any cases where ->cpu_enable() may be called on a CPU without the feature outside of ARM64_CPUCAP_LOCAL_CPU_ERRATUM or ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE? Will
On Wed, Aug 07, 2019 at 05:01:08PM +0100, Will Deacon wrote: > On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote: > > - * Take the appropriate actions to enable this capability for this CPU. > > - * For each successfully booted CPU, this method is called for each > > - * globally detected capability. > > + * Take the appropriate actions to configure this capability for this > > + * CPU. This will be called on all CPUs in the system if the > > + * capability is detected anywhere in the system. > That's not quite right though either, is it? We need to take into account > the scope of the capability/erratum as well, since we don't /always/ call > this function for everybody. I guess you're thinking of the ARM64_CPUCAP_SYSTEM_FEATURE case where we match the feature on all CPUs so we could see the feature on some CPUs but not detect it as we're requiring a match on all? Possibly the above should be "detected and enabled" rather than just "detected"? I can see how that might not be 100% clear, I was thinking of detection as passing all the match requirements including cross-CPU requirements but that could be more explicit. Perhaps something like: If this is called for any CPU in the system then it will be called for all of them. might cover it? > Suzuki, are there any cases where ->cpu_enable() may be called on a CPU > without the feature outside of ARM64_CPUCAP_LOCAL_CPU_ERRATUM or > ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE? There's at least ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, that was what caused me to notice what was happening (and get confused about why cpu_enable() was being called on non-matching CPUs). I don't see where we limit where cpu_enable() is called after we start calling it. When we're looping through in cpu_enable_non_boot_scope() we skip SCOPE_BOOT_CPU but those get cpu_enable() called in enable_cpu_capabilites() or verify_local_cpu_capabilities() depending on if it's the boot CPU or not. It's possible I'm missing something though.
On 07/08/2019 17:01, Will Deacon wrote: > [+Suzuki] > > On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote: >> Strengthen the wording in the documentation for cpu_enable() to make it >> more obvious to readers not already familiar with the code when the core >> will call this callback and that this is intentional. >> >> Signed-off-by: Mark Brown <broonie@kernel.org> >> --- >> arch/arm64/include/asm/cpufeature.h | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h >> index cf65a47ee6b4..3d8afcf687d9 100644 >> --- a/arch/arm64/include/asm/cpufeature.h >> +++ b/arch/arm64/include/asm/cpufeature.h >> @@ -289,9 +289,9 @@ struct arm64_cpu_capabilities { >> u16 type; >> bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope); >> /* >> - * Take the appropriate actions to enable this capability for this CPU. >> - * For each successfully booted CPU, this method is called for each >> - * globally detected capability. >> + * Take the appropriate actions to configure this capability for this >> + * CPU. This will be called on all CPUs in the system if the >> + * capability is detected anywhere in the system. >> */ >> void (*cpu_enable)(const struct arm64_cpu_capabilities *cap); >> union { > > That's not quite right though either, is it? We need to take into account > the scope of the capability/erratum as well, Exactly. Each capability is detected based on the "SCOPE" of the capability. So, the above statement is clearly misleading (i.e, mentioning the case for the LOCAL_CPU scope capabilities) and is wrong for SYSTEM scope. For that matter, one should not talk about the "where" it is detected, as long as he understands the "scope" of the capability. since we don't /always/ call > this function for everybody. > > Suzuki, are there any cases where ->cpu_enable() may be called on a CPU > without the feature outside of ARM64_CPUCAP_LOCAL_CPU_ERRATUM or > ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE? The callback can be issued for any "capability" with LOCAL_CPU scope, provided the capability is detected before the system finalizes the list. So, it applies for both the above and the ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE. Cheers Suzuki
On 07/08/2019 17:51, Mark Brown wrote: > On Wed, Aug 07, 2019 at 05:01:08PM +0100, Will Deacon wrote: >> On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote: > >>> - * Take the appropriate actions to enable this capability for this CPU. >>> - * For each successfully booted CPU, this method is called for each >>> - * globally detected capability. >>> + * Take the appropriate actions to configure this capability for this >>> + * CPU. This will be called on all CPUs in the system if the >>> + * capability is detected anywhere in the system. > >> That's not quite right though either, is it? We need to take into account >> the scope of the capability/erratum as well, since we don't /always/ call >> this function for everybody. > > I guess you're thinking of the ARM64_CPUCAP_SYSTEM_FEATURE case where we > match the feature on all CPUs so we could see the feature on some CPUs > but not detect it as we're requiring a match on all? We don't run the "match" check (i.e, detect) on all CPUs for SYSTEM scoped features. Instead, we use sanitised feature set to detect the system features. > Possibly the above > should be "detected and enabled" rather than just "detected"? I can see > how that might not be 100% clear, I was thinking of detection as passing > all the match requirements including cross-CPU requirements but that > could be more explicit. Perhaps something like: > > If this is called for any CPU in the system then it will be > called for all of them. > > might cover it? * Take the appropriate actions to configure this capability for the * current CPU. If this capability is detected by the kernel, this will * called on all the CPUs in the system, including the hotplugged * CPUs. */ > >> Suzuki, are there any cases where ->cpu_enable() may be called on a CPU >> without the feature outside of ARM64_CPUCAP_LOCAL_CPU_ERRATUM or >> ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE? > > There's at least ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, that was what > caused me to notice what was happening (and get confused about why > cpu_enable() was being called on non-matching CPUs). Well, this is true for the ERRATUM too. In a nutshell, any LOCAL_CPU scoped capability can trigger this on the CPUs. > > I don't see where we limit where cpu_enable() is called after we start > calling it. When we're looping through in cpu_enable_non_boot_scope() > we skip SCOPE_BOOT_CPU but those get cpu_enable() called in > enable_cpu_capabilites() or verify_local_cpu_capabilities() depending on > if it's the boot CPU or not. It's possible I'm missing something > though. There are two phases for any capability: 1) Detection - The kernel runs the matches() for the capability and marks the cap available when it returns true. Depending on the scope of the capability this could be done : a) BOOT_CPU scope - Only on boot CPU. This must be only used for features that are enabled very early in the kernel and get used right away by the boot CPU. The checks use the raw CPU registers. e.g, VHE - kernel runs in EL2, GIC NMI via Priority masking. b) LOCAL_SCOPE - On boot CPU and all the secondary CPUs booted by the kernel during smp bring up (before smp_cpus_done()) and before the userspace gets control. The checks use the raw CPU registers. e.g, CPU erratums c) SYSTEM_SCOPE - After all the smp CPUs are booted by the kernel, (i.e, from smp_cpus_done()). The checks are usually run with sanitised feature register set, and gives you the system wide state of the features. The kernel finalizes the capabilities at this point and cannot be changed. Thus, the hotplugged CPUs cannot affect the feature state and must comply to the now advertised set of capabilities/features. 2) Enabling a capability - Once the kernel has detected the capability and has been advertised (via cpu_hwcaps), each capability can take some action using the cpu_enable() callback. This could include changing system/control register configurations (e.g, trapping access to registers). The cpu_enable() is called on all online CPUs in the system. Depending on the SCOPE of the capabilities, the "cpu_enable" operation is triggered. For all CPUs that are brought online after the capability was enabled (e.g hotplugged CPUs), this gets called via check_local_cpu_capabilities()->verify_local_cpu_capabilities(). a) BOOT_CPU - The BOOT scope capabilities are enabled after the boot CPU detects the features. i.e, from setup_boot_cpu_capabilities() and is applied directly on the boot CPU (because there are no other online CPUs). Thus, all other CPUs (including the secondaries brought up by the kernel) trigger the cpu_enable() for BOOT scope CPUs via check_local_cpu_capabilities(). b) All the other capabilities trigger the "cpu_enable" on all the online CPUs after the system establishes the feature set in setup_system_capabilities(). And the cpu_enable() operations are batched via stop_machine() with cpu_enable_non_boot_scope_capabilities() [ The boot CPU scope capabilities are already enabled by now on all online CPUs]. Any new CPU that the kernel brings up must be triggered by the userspace and they trigger the cpu_enable() operation as they are brought up individually via check_local_cpu_capabilities()->verify_local_cpu_capabilities(). Does this help ? Cheers Suzuki
On Thu, Aug 08, 2019 at 12:05:02PM +0100, Suzuki K Poulose wrote: > On 07/08/2019 17:51, Mark Brown wrote: > > On Wed, Aug 07, 2019 at 05:01:08PM +0100, Will Deacon wrote: > > > On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote: > > I guess you're thinking of the ARM64_CPUCAP_SYSTEM_FEATURE case where we > > match the feature on all CPUs so we could see the feature on some CPUs > > but not detect it as we're requiring a match on all? > We don't run the "match" check (i.e, detect) on all CPUs for SYSTEM scoped > features. Instead, we use sanitised feature set to detect the system features. Right, but the sanitised feature set involves merging the capabilities of all the CPUs. > > If this is called for any CPU in the system then it will be > > called for all of them. > > might cover it? > * current CPU. If this capability is detected by the kernel, this will > * called on all the CPUs in the system, including the hotplugged > * CPUs. > */ How about adding ", regardless of if the capability was detected on that specific CPU" at the end? The above is *accurate* but it's still easy to insert an "on that CPU" in there when reading especially with the awkward phrasing. Or possibly just drop the first comma. The reason I said "If this is called" rather than "if this is detected" is to make it as clear as possible that the calls don't depend on detection without being overly verbose. If this capability is detected by the kernel this will called on all the CPUs in the system, including the hotplugged CPUs, regardless of if the capability was detected on that specific CPU. > > I don't see where we limit where cpu_enable() is called after we start > > calling it. When we're looping through in cpu_enable_non_boot_scope() > > we skip SCOPE_BOOT_CPU but those get cpu_enable() called in > > enable_cpu_capabilites() or verify_local_cpu_capabilities() depending on > > if it's the boot CPU or not. It's possible I'm missing something > > though. ... > Does this help ? I think you just confirmed that when I said we don't limit where we call cpu_enable() that's accurate?
On Thu, Aug 08, 2019 at 10:20:12AM +0100, Suzuki K Poulose wrote: > On 07/08/2019 17:01, Will Deacon wrote: > > On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote: > > > + * Take the appropriate actions to configure this capability for this > > > + * CPU. This will be called on all CPUs in the system if the > > > + * capability is detected anywhere in the system. > > That's not quite right though either, is it? We need to take into account > > the scope of the capability/erratum as well, > Exactly. Each capability is detected based on the "SCOPE" of the capability. > So, the above statement is clearly misleading (i.e, mentioning the case for the > LOCAL_CPU scope capabilities) and is wrong for SYSTEM scope. For that matter, > one should not talk about the "where" it is detected, as long as he understands > the "scope" of the capability. My goal with using "anywhere" was to cover all scopes, including the system scope and also all more local scopes.
On 08/08/2019 13:19, Mark Brown wrote: > On Thu, Aug 08, 2019 at 12:05:02PM +0100, Suzuki K Poulose wrote: >> On 07/08/2019 17:51, Mark Brown wrote: >>> On Wed, Aug 07, 2019 at 05:01:08PM +0100, Will Deacon wrote: >>>> On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote: > >>> I guess you're thinking of the ARM64_CPUCAP_SYSTEM_FEATURE case where we >>> match the feature on all CPUs so we could see the feature on some CPUs >>> but not detect it as we're requiring a match on all? > >> We don't run the "match" check (i.e, detect) on all CPUs for SYSTEM scoped >> features. Instead, we use sanitised feature set to detect the system features. > > Right, but the sanitised feature set involves merging the capabilities > of all the CPUs. > >>> If this is called for any CPU in the system then it will be >>> called for all of them. > >>> might cover it? > >> * current CPU. If this capability is detected by the kernel, this will >> * called on all the CPUs in the system, including the hotplugged >> * CPUs. >> */ > > How about adding ", regardless of if the capability was detected on that > specific CPU" at the end? The above is *accurate* but it's still easy to > insert an "on that CPU" in there when reading especially with the > awkward phrasing. Or possibly just drop the first comma. The reason I > said "If this is called" rather than "if this is detected" is to make it > as clear as possible that the calls don't depend on detection without > being overly verbose. > > If this capability is detected by the kernel > this will called on all the CPUs in the system, including > the hotplugged CPUs, regardless of if the capability was > detected on that specific CPU. I think the only issue with this, as also with the original statement, is that you are overloading "detected" for the "specific CPU" case. In the first use, the "detect" is dependent on the SCOPE of the capability and in the latter one is strictly "LOCAL" scope. If you replace the second "detected" with say, "not available" or even "not matched", it makes it less confusing. How about: If the capability is detected by the kernel this will be called on all the CPUs in the system, including the hotplugged CPUs, regardless of if the capability was *available* on that specific CPU. This is useful for some capabilities (e.g, working around CPU errata), where all the CPUs must take some action (e.g, changing system control/configuration). Thus, if an action is required only if the CPU has the capability, then the routine must check it before taking any action. Suzuki
On Thu, Aug 08, 2019 at 02:21:42PM +0100, Suzuki K Poulose wrote: > On 08/08/2019 13:19, Mark Brown wrote: > > > > If this is called for any CPU in the system then it will be > > > > called for all of them. > > > > might cover it? > > > * current CPU. If this capability is detected by the kernel, this will > > > * called on all the CPUs in the system, including the hotplugged > > > * CPUs. > > > */ > > If this capability is detected by the kernel > > this will called on all the CPUs in the system, including > > the hotplugged CPUs, regardless of if the capability was > > detected on that specific CPU. > I think the only issue with this, as also with the original statement, is that > you are overloading "detected" for the "specific CPU" case. In the first > use, the "detect" is dependent on the SCOPE of the capability and in the > latter one That's not quite what I'm trying to get over here - what I'm trying to get over is that the enable does not have the same scope as the detection, I think it's fairly natural to assume that that is the case. That is to say that the behaviour for the system scope detection case is expected but for anything that's CPU local it's a surprise. > is strictly "LOCAL" scope. If you replace the second "detected" with say, "not > available" or even "not matched", it makes it less confusing. > If the capability is detected by the kernel this will be called on all > the CPUs in the system, including the hotplugged CPUs, regardless of if > the capability was *available* on that specific CPU. This is useful for > some capabilities (e.g, working around CPU errata), where all the CPUs > must take some action (e.g, changing system control/configuration). > Thus, if an action is required only if the CPU has the capability, then > the routine must check it before taking any action. That's a bit verbose but I think it's sufficiently unambiguous. I'm still confused about how this differs from what I originally proposed :/
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index cf65a47ee6b4..3d8afcf687d9 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -289,9 +289,9 @@ struct arm64_cpu_capabilities { u16 type; bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope); /* - * Take the appropriate actions to enable this capability for this CPU. - * For each successfully booted CPU, this method is called for each - * globally detected capability. + * Take the appropriate actions to configure this capability for this + * CPU. This will be called on all CPUs in the system if the + * capability is detected anywhere in the system. */ void (*cpu_enable)(const struct arm64_cpu_capabilities *cap); union {
Strengthen the wording in the documentation for cpu_enable() to make it more obvious to readers not already familiar with the code when the core will call this callback and that this is intentional. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/cpufeature.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)