[PATCH] arm64: Clarify when cpu_enable() is called

[PATCH] arm64: Clarify when cpu_enable() is called

[Mark Brown]

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 {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Will Deacon]

[+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2016 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Suzuki K Poulose]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2126 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Suzuki K Poulose]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2003 bytes --]

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 :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Suzuki K Poulose]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Clarify when cpu_enable() is called

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 979 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel