[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

When a CPU is brought up after we have finalised the system
wide capabilities (i.e, features and errata), we make sure the
new CPU doesn't need a new errata work around which has not been
detected already. However we don't run enable() method on the new
CPU for the errata work arounds already detected. This could
cause the new CPU running without potential work arounds.
It is upto the "enable()" method to decide if this CPU should
do something about the errata.

Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Martin <dave.martin@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpu_errata.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 90a9e465339c..54e41dfe41f6 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
 {
 	const struct arm64_cpu_capabilities *caps = arm64_errata;
 
-	for (; caps->matches; caps++)
-		if (!cpus_have_cap(caps->capability) &&
-			caps->matches(caps, SCOPE_LOCAL_CPU)) {
+	for (; caps->matches; caps++) {
+		if (cpus_have_cap(caps->capability)) {
+			if (caps->enable)
+				caps->enable((void *)caps);
+		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {
 			pr_crit("CPU%d: Requires work around for %s, not detected"
 					" at boot time\n",
 				smp_processor_id(),
 				caps->desc ? : "an erratum");
 			cpu_die_early();
 		}
+	}
 }
 
 void update_cpu_errata_workarounds(void)
-- 
2.13.6

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

When a CPU is brought up after we have finalised the system
wide capabilities (i.e, features and errata), we make sure the
new CPU doesn't need a new errata work around which has not been
detected already. However we don't run enable() method on the new
CPU for the errata work arounds already detected. This could
cause the new CPU running without potential work arounds.
It is upto the "enable()" method to decide if this CPU should
do something about the errata.

Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Martin <dave.martin@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpu_errata.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 90a9e465339c..54e41dfe41f6 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
 {
 	const struct arm64_cpu_capabilities *caps = arm64_errata;
 
-	for (; caps->matches; caps++)
-		if (!cpus_have_cap(caps->capability) &&
-			caps->matches(caps, SCOPE_LOCAL_CPU)) {
+	for (; caps->matches; caps++) {
+		if (cpus_have_cap(caps->capability)) {
+			if (caps->enable)
+				caps->enable((void *)caps);
+		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {
 			pr_crit("CPU%d: Requires work around for %s, not detected"
 					" at boot time\n",
 				smp_processor_id(),
 				caps->desc ? : "an erratum");
 			cpu_die_early();
 		}
+	}
 }
 
 void update_cpu_errata_workarounds(void)
-- 
2.13.6

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Dave Martin]

On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
> When a CPU is brought up after we have finalised the system
> wide capabilities (i.e, features and errata), we make sure the
> new CPU doesn't need a new errata work around which has not been
> detected already. However we don't run enable() method on the new
> CPU for the errata work arounds already detected. This could
> cause the new CPU running without potential work arounds.
> It is upto the "enable()" method to decide if this CPU should
> do something about the errata.
> 
> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dave Martin <dave.martin@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 90a9e465339c..54e41dfe41f6 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>  {
>  	const struct arm64_cpu_capabilities *caps = arm64_errata;
>  
> -	for (; caps->matches; caps++)
> -		if (!cpus_have_cap(caps->capability) &&
> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
> +	for (; caps->matches; caps++) {
> +		if (cpus_have_cap(caps->capability)) {
> +			if (caps->enable)
> +				caps->enable((void *)caps);

Do we really need this cast?

Can enable() fail, or do we already guarantee that it succeeds (by
having detected the cap in the first place)?

> +		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {

[...]

Cheers
---Dave

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Dave Martin]

On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
> When a CPU is brought up after we have finalised the system
> wide capabilities (i.e, features and errata), we make sure the
> new CPU doesn't need a new errata work around which has not been
> detected already. However we don't run enable() method on the new
> CPU for the errata work arounds already detected. This could
> cause the new CPU running without potential work arounds.
> It is upto the "enable()" method to decide if this CPU should
> do something about the errata.
> 
> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dave Martin <dave.martin@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 90a9e465339c..54e41dfe41f6 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>  {
>  	const struct arm64_cpu_capabilities *caps = arm64_errata;
>  
> -	for (; caps->matches; caps++)
> -		if (!cpus_have_cap(caps->capability) &&
> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
> +	for (; caps->matches; caps++) {
> +		if (cpus_have_cap(caps->capability)) {
> +			if (caps->enable)
> +				caps->enable((void *)caps);

Do we really need this cast?

Can enable() fail, or do we already guarantee that it succeeds (by
having detected the cap in the first place)?

> +		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {

[...]

Cheers
---Dave

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Robin Murphy]

On 17/01/18 12:25, Dave Martin wrote:
> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>> When a CPU is brought up after we have finalised the system
>> wide capabilities (i.e, features and errata), we make sure the
>> new CPU doesn't need a new errata work around which has not been
>> detected already. However we don't run enable() method on the new
>> CPU for the errata work arounds already detected. This could
>> cause the new CPU running without potential work arounds.
>> It is upto the "enable()" method to decide if this CPU should
>> do something about the errata.
>>
>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Dave Martin <dave.martin@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index 90a9e465339c..54e41dfe41f6 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>   {
>>   	const struct arm64_cpu_capabilities *caps = arm64_errata;
>>   
>> -	for (; caps->matches; caps++)
>> -		if (!cpus_have_cap(caps->capability) &&
>> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
>> +	for (; caps->matches; caps++) {
>> +		if (cpus_have_cap(caps->capability)) {
>> +			if (caps->enable)
>> +				caps->enable((void *)caps);
> 
> Do we really need this cast?

Seems to me like the prototype for .enable needs updating. If any 
existing callback was actually using the (non-const) void* for some 
purpose (thankfully nothing seems to be), then passing the capability 
pointer into that would be unlikely to end well anyway.

We probably want to replace or append that with a proper const struct 
arm64_cpu_capabilities* argument, to make it more of a method call.

Robin.

> Can enable() fail, or do we already guarantee that it succeeds (by
> having detected the cap in the first place)?
> 
>> +		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {
> 
> [...]
> 
> Cheers
> ---Dave
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Robin Murphy]

On 17/01/18 12:25, Dave Martin wrote:
> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>> When a CPU is brought up after we have finalised the system
>> wide capabilities (i.e, features and errata), we make sure the
>> new CPU doesn't need a new errata work around which has not been
>> detected already. However we don't run enable() method on the new
>> CPU for the errata work arounds already detected. This could
>> cause the new CPU running without potential work arounds.
>> It is upto the "enable()" method to decide if this CPU should
>> do something about the errata.
>>
>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Dave Martin <dave.martin@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index 90a9e465339c..54e41dfe41f6 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>   {
>>   	const struct arm64_cpu_capabilities *caps = arm64_errata;
>>   
>> -	for (; caps->matches; caps++)
>> -		if (!cpus_have_cap(caps->capability) &&
>> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
>> +	for (; caps->matches; caps++) {
>> +		if (cpus_have_cap(caps->capability)) {
>> +			if (caps->enable)
>> +				caps->enable((void *)caps);
> 
> Do we really need this cast?

Seems to me like the prototype for .enable needs updating. If any 
existing callback was actually using the (non-const) void* for some 
purpose (thankfully nothing seems to be), then passing the capability 
pointer into that would be unlikely to end well anyway.

We probably want to replace or append that with a proper const struct 
arm64_cpu_capabilities* argument, to make it more of a method call.

Robin.

> Can enable() fail, or do we already guarantee that it succeeds (by
> having detected the cap in the first place)?
> 
>> +		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {
> 
> [...]
> 
> Cheers
> ---Dave
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 13:20, Robin Murphy wrote:
> On 17/01/18 12:25, Dave Martin wrote:
>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>> When a CPU is brought up after we have finalised the system
>>> wide capabilities (i.e, features and errata), we make sure the
>>> new CPU doesn't need a new errata work around which has not been
>>> detected already. However we don't run enable() method on the new
>>> CPU for the errata work arounds already detected. This could
>>> cause the new CPU running without potential work arounds.
>>> It is upto the "enable()" method to decide if this CPU should
>>> do something about the errata.
>>>
>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Dave Martin <dave.martin@arm.com>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>>> index 90a9e465339c..54e41dfe41f6 100644
>>> --- a/arch/arm64/kernel/cpu_errata.c
>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>   {
>>>       const struct arm64_cpu_capabilities *caps = arm64_errata;
>>> -    for (; caps->matches; caps++)
>>> -        if (!cpus_have_cap(caps->capability) &&
>>> -            caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>> +    for (; caps->matches; caps++) {
>>> +        if (cpus_have_cap(caps->capability)) {
>>> +            if (caps->enable)
>>> +                caps->enable((void *)caps);
>>
>> Do we really need this cast?
> 
> Seems to me like the prototype for .enable needs updating. If any existing callback was actually using the (non-const) void* for some purpose (thankfully nothing seems to be), then passing the capability pointer into that would be unlikely to end well anyway.

I agree. This was initially written such that we could call it via on_each_cpu().
But then we later switched to stop_machine(). And we weren't using the argument until
very recently with the introduction of multiple entries for the same capability.

I will try to clean this up in a separate series, which would involve cleaning up
all the enable(), quite invasive. I would like this to go in for 4.16, as it is
needed for things like KPTI and some of the existing caps.

Cheers
Suzuki

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 13:20, Robin Murphy wrote:
> On 17/01/18 12:25, Dave Martin wrote:
>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>> When a CPU is brought up after we have finalised the system
>>> wide capabilities (i.e, features and errata), we make sure the
>>> new CPU doesn't need a new errata work around which has not been
>>> detected already. However we don't run enable() method on the new
>>> CPU for the errata work arounds already detected. This could
>>> cause the new CPU running without potential work arounds.
>>> It is upto the "enable()" method to decide if this CPU should
>>> do something about the errata.
>>>
>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Dave Martin <dave.martin@arm.com>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>> ? arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>> ? 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>>> index 90a9e465339c..54e41dfe41f6 100644
>>> --- a/arch/arm64/kernel/cpu_errata.c
>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>> ? {
>>> ????? const struct arm64_cpu_capabilities *caps = arm64_errata;
>>> -??? for (; caps->matches; caps++)
>>> -??????? if (!cpus_have_cap(caps->capability) &&
>>> -??????????? caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>> +??? for (; caps->matches; caps++) {
>>> +??????? if (cpus_have_cap(caps->capability)) {
>>> +??????????? if (caps->enable)
>>> +??????????????? caps->enable((void *)caps);
>>
>> Do we really need this cast?
> 
> Seems to me like the prototype for .enable needs updating. If any existing callback was actually using the (non-const) void* for some purpose (thankfully nothing seems to be), then passing the capability pointer into that would be unlikely to end well anyway.

I agree. This was initially written such that we could call it via on_each_cpu().
But then we later switched to stop_machine(). And we weren't using the argument until
very recently with the introduction of multiple entries for the same capability.

I will try to clean this up in a separate series, which would involve cleaning up
all the enable(), quite invasive. I would like this to go in for 4.16, as it is
needed for things like KPTI and some of the existing caps.

Cheers
Suzuki

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Robin Murphy]

On 17/01/18 13:31, Suzuki K Poulose wrote:
> On 17/01/18 13:20, Robin Murphy wrote:
>> On 17/01/18 12:25, Dave Martin wrote:
>>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>>> When a CPU is brought up after we have finalised the system
>>>> wide capabilities (i.e, features and errata), we make sure the
>>>> new CPU doesn't need a new errata work around which has not been
>>>> detected already. However we don't run enable() method on the new
>>>> CPU for the errata work arounds already detected. This could
>>>> cause the new CPU running without potential work arounds.
>>>> It is upto the "enable()" method to decide if this CPU should
>>>> do something about the errata.
>>>>
>>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work 
>>>> arounds on hotplugged CPU")
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Dave Martin <dave.martin@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/cpu_errata.c 
>>>> b/arch/arm64/kernel/cpu_errata.c
>>>> index 90a9e465339c..54e41dfe41f6 100644
>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>>   {
>>>>       const struct arm64_cpu_capabilities *caps = arm64_errata;
>>>> -    for (; caps->matches; caps++)
>>>> -        if (!cpus_have_cap(caps->capability) &&
>>>> -            caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>>> +    for (; caps->matches; caps++) {
>>>> +        if (cpus_have_cap(caps->capability)) {
>>>> +            if (caps->enable)
>>>> +                caps->enable((void *)caps);
>>>
>>> Do we really need this cast?
>>
>> Seems to me like the prototype for .enable needs updating. If any 
>> existing callback was actually using the (non-const) void* for some 
>> purpose (thankfully nothing seems to be), then passing the capability 
>> pointer into that would be unlikely to end well anyway.
> 
> I agree. This was initially written such that we could call it via 
> on_each_cpu().
> But then we later switched to stop_machine(). And we weren't using the 
> argument until
> very recently with the introduction of multiple entries for the same 
> capability.
> 
> I will try to clean this up in a separate series, which would involve 
> cleaning up
> all the enable(), quite invasive. I would like this to go in for 4.16, 
> as it is
> needed for things like KPTI and some of the existing caps.

OK, sounds good. For the sake of the immediate fix, perhaps it's cleaner 
to just pass NULL here if the current callbacks ignore it?

Robin.

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Robin Murphy]

On 17/01/18 13:31, Suzuki K Poulose wrote:
> On 17/01/18 13:20, Robin Murphy wrote:
>> On 17/01/18 12:25, Dave Martin wrote:
>>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>>> When a CPU is brought up after we have finalised the system
>>>> wide capabilities (i.e, features and errata), we make sure the
>>>> new CPU doesn't need a new errata work around which has not been
>>>> detected already. However we don't run enable() method on the new
>>>> CPU for the errata work arounds already detected. This could
>>>> cause the new CPU running without potential work arounds.
>>>> It is upto the "enable()" method to decide if this CPU should
>>>> do something about the errata.
>>>>
>>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work 
>>>> arounds on hotplugged CPU")
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Dave Martin <dave.martin@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>> ? arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>> ? 1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/cpu_errata.c 
>>>> b/arch/arm64/kernel/cpu_errata.c
>>>> index 90a9e465339c..54e41dfe41f6 100644
>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>> ? {
>>>> ????? const struct arm64_cpu_capabilities *caps = arm64_errata;
>>>> -??? for (; caps->matches; caps++)
>>>> -??????? if (!cpus_have_cap(caps->capability) &&
>>>> -??????????? caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>>> +??? for (; caps->matches; caps++) {
>>>> +??????? if (cpus_have_cap(caps->capability)) {
>>>> +??????????? if (caps->enable)
>>>> +??????????????? caps->enable((void *)caps);
>>>
>>> Do we really need this cast?
>>
>> Seems to me like the prototype for .enable needs updating. If any 
>> existing callback was actually using the (non-const) void* for some 
>> purpose (thankfully nothing seems to be), then passing the capability 
>> pointer into that would be unlikely to end well anyway.
> 
> I agree. This was initially written such that we could call it via 
> on_each_cpu().
> But then we later switched to stop_machine(). And we weren't using the 
> argument until
> very recently with the introduction of multiple entries for the same 
> capability.
> 
> I will try to clean this up in a separate series, which would involve 
> cleaning up
> all the enable(), quite invasive. I would like this to go in for 4.16, 
> as it is
> needed for things like KPTI and some of the existing caps.

OK, sounds good. For the sake of the immediate fix, perhaps it's cleaner 
to just pass NULL here if the current callbacks ignore it?

Robin.

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 13:43, Robin Murphy wrote:
> On 17/01/18 13:31, Suzuki K Poulose wrote:
>> On 17/01/18 13:20, Robin Murphy wrote:
>>> On 17/01/18 12:25, Dave Martin wrote:
>>>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>>>> When a CPU is brought up after we have finalised the system
>>>>> wide capabilities (i.e, features and errata), we make sure the
>>>>> new CPU doesn't need a new errata work around which has not been
>>>>> detected already. However we don't run enable() method on the new
>>>>> CPU for the errata work arounds already detected. This could
>>>>> cause the new CPU running without potential work arounds.
>>>>> It is upto the "enable()" method to decide if this CPU should
>>>>> do something about the errata.
>>>>>
>>>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>> Cc: Dave Martin <dave.martin@arm.com>
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> ---
>>>>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>>>>> index 90a9e465339c..54e41dfe41f6 100644
>>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>>>   {
>>>>>       const struct arm64_cpu_capabilities *caps = arm64_errata;
>>>>> -    for (; caps->matches; caps++)
>>>>> -        if (!cpus_have_cap(caps->capability) &&
>>>>> -            caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>>>> +    for (; caps->matches; caps++) {
>>>>> +        if (cpus_have_cap(caps->capability)) {
>>>>> +            if (caps->enable)
>>>>> +                caps->enable((void *)caps);
>>>>
>>>> Do we really need this cast?
>>>
>>> Seems to me like the prototype for .enable needs updating. If any existing callback was actually using the (non-const) void* for some purpose (thankfully nothing seems to be), then passing the capability pointer into that would be unlikely to end well anyway.
>>
>> I agree. This was initially written such that we could call it via on_each_cpu().
>> But then we later switched to stop_machine(). And we weren't using the argument until
>> very recently with the introduction of multiple entries for the same capability.
>>
>> I will try to clean this up in a separate series, which would involve cleaning up
>> all the enable(), quite invasive. I would like this to go in for 4.16, as it is>> needed for things like KPTI and some of the existing caps.

Correction, s/KPTI/bp hardening/

> 
> OK, sounds good. For the sake of the immediate fix, perhaps it's cleaner to just pass NULL here if the current callbacks ignore it?

As I said above, we have some users at the moment, so we cant do that.

Suzuki

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 13:43, Robin Murphy wrote:
> On 17/01/18 13:31, Suzuki K Poulose wrote:
>> On 17/01/18 13:20, Robin Murphy wrote:
>>> On 17/01/18 12:25, Dave Martin wrote:
>>>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>>>> When a CPU is brought up after we have finalised the system
>>>>> wide capabilities (i.e, features and errata), we make sure the
>>>>> new CPU doesn't need a new errata work around which has not been
>>>>> detected already. However we don't run enable() method on the new
>>>>> CPU for the errata work arounds already detected. This could
>>>>> cause the new CPU running without potential work arounds.
>>>>> It is upto the "enable()" method to decide if this CPU should
>>>>> do something about the errata.
>>>>>
>>>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>> Cc: Dave Martin <dave.martin@arm.com>
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> ---
>>>>> ? arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>>> ? 1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>>>>> index 90a9e465339c..54e41dfe41f6 100644
>>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>>> ? {
>>>>> ????? const struct arm64_cpu_capabilities *caps = arm64_errata;
>>>>> -??? for (; caps->matches; caps++)
>>>>> -??????? if (!cpus_have_cap(caps->capability) &&
>>>>> -??????????? caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>>>> +??? for (; caps->matches; caps++) {
>>>>> +??????? if (cpus_have_cap(caps->capability)) {
>>>>> +??????????? if (caps->enable)
>>>>> +??????????????? caps->enable((void *)caps);
>>>>
>>>> Do we really need this cast?
>>>
>>> Seems to me like the prototype for .enable needs updating. If any existing callback was actually using the (non-const) void* for some purpose (thankfully nothing seems to be), then passing the capability pointer into that would be unlikely to end well anyway.
>>
>> I agree. This was initially written such that we could call it via on_each_cpu().
>> But then we later switched to stop_machine(). And we weren't using the argument until
>> very recently with the introduction of multiple entries for the same capability.
>>
>> I will try to clean this up in a separate series, which would involve cleaning up
>> all the enable(), quite invasive. I would like this to go in for 4.16, as it is>> needed for things like KPTI and some of the existing caps.

Correction, s/KPTI/bp hardening/

> 
> OK, sounds good. For the sake of the immediate fix, perhaps it's cleaner to just pass NULL here if the current callbacks ignore it?

As I said above, we have some users at the moment, so we cant do that.

Suzuki

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 12:25, Dave Martin wrote:
> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>> When a CPU is brought up after we have finalised the system
>> wide capabilities (i.e, features and errata), we make sure the
>> new CPU doesn't need a new errata work around which has not been
>> detected already. However we don't run enable() method on the new
>> CPU for the errata work arounds already detected. This could
>> cause the new CPU running without potential work arounds.
>> It is upto the "enable()" method to decide if this CPU should
>> do something about the errata.
>>
>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Dave Martin <dave.martin@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index 90a9e465339c..54e41dfe41f6 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>   {
>>   	const struct arm64_cpu_capabilities *caps = arm64_errata;
>>   
>> -	for (; caps->matches; caps++)
>> -		if (!cpus_have_cap(caps->capability) &&
>> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
>> +	for (; caps->matches; caps++) {
>> +		if (cpus_have_cap(caps->capability)) {
>> +			if (caps->enable)
>> +				caps->enable((void *)caps);
> 
> Do we really need this cast?

Yes, otherwise we would be passing a "const *" where a "void *" is expected,
and the compiler warns. Or we could simply change the prototype of the
enable() method to accept a const capability ptr.

> 
> Can enable() fail, or do we already guarantee that it succeeds (by
> having detected the cap in the first place)?

enable() can't fail, since as you said the cap is already detected
by the scope of the capability. It is just the matter of enable() deciding
to do some action on the calling CPU depending on the type of the
work around (e.g, enable SCTLR bit or enable trapping etc). It is left
to the capability to decide whether the calling CPU needs any action
or not (e.g, bp hardening).

Cheers
Suzuki

> 
>> +		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {
> 
> [...]
> 
> Cheers
> ---Dave
> 

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 12:25, Dave Martin wrote:
> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>> When a CPU is brought up after we have finalised the system
>> wide capabilities (i.e, features and errata), we make sure the
>> new CPU doesn't need a new errata work around which has not been
>> detected already. However we don't run enable() method on the new
>> CPU for the errata work arounds already detected. This could
>> cause the new CPU running without potential work arounds.
>> It is upto the "enable()" method to decide if this CPU should
>> do something about the errata.
>>
>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Dave Martin <dave.martin@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index 90a9e465339c..54e41dfe41f6 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>   {
>>   	const struct arm64_cpu_capabilities *caps = arm64_errata;
>>   
>> -	for (; caps->matches; caps++)
>> -		if (!cpus_have_cap(caps->capability) &&
>> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
>> +	for (; caps->matches; caps++) {
>> +		if (cpus_have_cap(caps->capability)) {
>> +			if (caps->enable)
>> +				caps->enable((void *)caps);
> 
> Do we really need this cast?

Yes, otherwise we would be passing a "const *" where a "void *" is expected,
and the compiler warns. Or we could simply change the prototype of the
enable() method to accept a const capability ptr.

> 
> Can enable() fail, or do we already guarantee that it succeeds (by
> having detected the cap in the first place)?

enable() can't fail, since as you said the cap is already detected
by the scope of the capability. It is just the matter of enable() deciding
to do some action on the calling CPU depending on the type of the
work around (e.g, enable SCTLR bit or enable trapping etc). It is left
to the capability to decide whether the calling CPU needs any action
or not (e.g, bp hardening).

Cheers
Suzuki

> 
>> +		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {
> 
> [...]
> 
> Cheers
> ---Dave
> 

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Dave Martin]

On Wed, Jan 17, 2018 at 01:22:19PM +0000, Suzuki K Poulose wrote:
> On 17/01/18 12:25, Dave Martin wrote:
> >On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
> >>When a CPU is brought up after we have finalised the system
> >>wide capabilities (i.e, features and errata), we make sure the
> >>new CPU doesn't need a new errata work around which has not been
> >>detected already. However we don't run enable() method on the new
> >>CPU for the errata work arounds already detected. This could
> >>cause the new CPU running without potential work arounds.
> >>It is upto the "enable()" method to decide if this CPU should
> >>do something about the errata.
> >>
> >>Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
> >>Cc: Will Deacon <will.deacon@arm.com>
> >>Cc: Mark Rutland <mark.rutland@arm.com>
> >>Cc: Andre Przywara <andre.przywara@arm.com>
> >>Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>Cc: Dave Martin <dave.martin@arm.com>
> >>Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> >>---
> >>  arch/arm64/kernel/cpu_errata.c | 9 ++++++---
> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> >>index 90a9e465339c..54e41dfe41f6 100644
> >>--- a/arch/arm64/kernel/cpu_errata.c
> >>+++ b/arch/arm64/kernel/cpu_errata.c
> >>@@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
> >>  {
> >>  	const struct arm64_cpu_capabilities *caps = arm64_errata;
> >>-	for (; caps->matches; caps++)
> >>-		if (!cpus_have_cap(caps->capability) &&
> >>-			caps->matches(caps, SCOPE_LOCAL_CPU)) {
> >>+	for (; caps->matches; caps++) {
> >>+		if (cpus_have_cap(caps->capability)) {
> >>+			if (caps->enable)
> >>+				caps->enable((void *)caps);
> >
> >Do we really need this cast?
> 
> Yes, otherwise we would be passing a "const *" where a "void *" is expected,
> and the compiler warns. Or we could simply change the prototype of the
> enable() method to accept a const capability ptr.

Hmmm, what is this argument for exactly?  cpufeature.h doesn't explain
what it is.

Does any enable method use this for anything other than a struct
arm64_cpu_capabilities const * ?  If not, it would be better to
specifiy that.

Cheers
---Dave

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Dave Martin]

On Wed, Jan 17, 2018 at 01:22:19PM +0000, Suzuki K Poulose wrote:
> On 17/01/18 12:25, Dave Martin wrote:
> >On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
> >>When a CPU is brought up after we have finalised the system
> >>wide capabilities (i.e, features and errata), we make sure the
> >>new CPU doesn't need a new errata work around which has not been
> >>detected already. However we don't run enable() method on the new
> >>CPU for the errata work arounds already detected. This could
> >>cause the new CPU running without potential work arounds.
> >>It is upto the "enable()" method to decide if this CPU should
> >>do something about the errata.
> >>
> >>Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
> >>Cc: Will Deacon <will.deacon@arm.com>
> >>Cc: Mark Rutland <mark.rutland@arm.com>
> >>Cc: Andre Przywara <andre.przywara@arm.com>
> >>Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>Cc: Dave Martin <dave.martin@arm.com>
> >>Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> >>---
> >>  arch/arm64/kernel/cpu_errata.c | 9 ++++++---
> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> >>index 90a9e465339c..54e41dfe41f6 100644
> >>--- a/arch/arm64/kernel/cpu_errata.c
> >>+++ b/arch/arm64/kernel/cpu_errata.c
> >>@@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
> >>  {
> >>  	const struct arm64_cpu_capabilities *caps = arm64_errata;
> >>-	for (; caps->matches; caps++)
> >>-		if (!cpus_have_cap(caps->capability) &&
> >>-			caps->matches(caps, SCOPE_LOCAL_CPU)) {
> >>+	for (; caps->matches; caps++) {
> >>+		if (cpus_have_cap(caps->capability)) {
> >>+			if (caps->enable)
> >>+				caps->enable((void *)caps);
> >
> >Do we really need this cast?
> 
> Yes, otherwise we would be passing a "const *" where a "void *" is expected,
> and the compiler warns. Or we could simply change the prototype of the
> enable() method to accept a const capability ptr.

Hmmm, what is this argument for exactly?  cpufeature.h doesn't explain
what it is.

Does any enable method use this for anything other than a struct
arm64_cpu_capabilities const * ?  If not, it would be better to
specifiy that.

Cheers
---Dave

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 14:38, Dave Martin wrote:
> On Wed, Jan 17, 2018 at 01:22:19PM +0000, Suzuki K Poulose wrote:
>> On 17/01/18 12:25, Dave Martin wrote:
>>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>>> When a CPU is brought up after we have finalised the system
>>>> wide capabilities (i.e, features and errata), we make sure the
>>>> new CPU doesn't need a new errata work around which has not been
>>>> detected already. However we don't run enable() method on the new
>>>> CPU for the errata work arounds already detected. This could
>>>> cause the new CPU running without potential work arounds.
>>>> It is upto the "enable()" method to decide if this CPU should
>>>> do something about the errata.
>>>>
>>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Dave Martin <dave.martin@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>>>> index 90a9e465339c..54e41dfe41f6 100644
>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>>   {
>>>>   	const struct arm64_cpu_capabilities *caps = arm64_errata;
>>>> -	for (; caps->matches; caps++)
>>>> -		if (!cpus_have_cap(caps->capability) &&
>>>> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>>> +	for (; caps->matches; caps++) {
>>>> +		if (cpus_have_cap(caps->capability)) {
>>>> +			if (caps->enable)
>>>> +				caps->enable((void *)caps);
>>>
>>> Do we really need this cast?
>>
>> Yes, otherwise we would be passing a "const *" where a "void *" is expected,
>> and the compiler warns. Or we could simply change the prototype of the
>> enable() method to accept a const capability ptr.
> 
> Hmmm, what is this argument for exactly?  cpufeature.h doesn't explain
> what it is.

This was introduced by commit 0a0d111d40fd1 ("arm64: cpufeature: Pass capability
structure to ->enable callback").

The idea is to enable multiple entries in the table for a single capability.
Some capabilities (read errata) could be detected in multiple ways. e.g, different
MIDR ranges. (e.g, ARM64_HARDEN_BRANCH_PREDICTOR, ARM64_WORKAROUND_QCOM_FALKOR_E1003.
Now, even though the errata is the same, there might be different work arounds
for them in each "matching" cases. So, we need the "caps" passed on to the
enable() method to see if the "specific work around" should be applied
to the system/CPU (since CPU hwcap could be set by one of the cpu_capabilities
entry.) (as we invoke enable() for all "available" capabilities.)

Passing the caps information makes it easier to decide, by using the caps->matches().

> 
> Does any enable method use this for anything other than a struct
> arm64_cpu_capabilities const * ?

No, we use it only for const struct arm64_cpu_capability *.

> If not, it would be better to specifiy that.

Yes, that could be done.

Cheers
Suzuki

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 14:38, Dave Martin wrote:
> On Wed, Jan 17, 2018 at 01:22:19PM +0000, Suzuki K Poulose wrote:
>> On 17/01/18 12:25, Dave Martin wrote:
>>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>>> When a CPU is brought up after we have finalised the system
>>>> wide capabilities (i.e, features and errata), we make sure the
>>>> new CPU doesn't need a new errata work around which has not been
>>>> detected already. However we don't run enable() method on the new
>>>> CPU for the errata work arounds already detected. This could
>>>> cause the new CPU running without potential work arounds.
>>>> It is upto the "enable()" method to decide if this CPU should
>>>> do something about the errata.
>>>>
>>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Dave Martin <dave.martin@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>>>> index 90a9e465339c..54e41dfe41f6 100644
>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>>   {
>>>>   	const struct arm64_cpu_capabilities *caps = arm64_errata;
>>>> -	for (; caps->matches; caps++)
>>>> -		if (!cpus_have_cap(caps->capability) &&
>>>> -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>>> +	for (; caps->matches; caps++) {
>>>> +		if (cpus_have_cap(caps->capability)) {
>>>> +			if (caps->enable)
>>>> +				caps->enable((void *)caps);
>>>
>>> Do we really need this cast?
>>
>> Yes, otherwise we would be passing a "const *" where a "void *" is expected,
>> and the compiler warns. Or we could simply change the prototype of the
>> enable() method to accept a const capability ptr.
> 
> Hmmm, what is this argument for exactly?  cpufeature.h doesn't explain
> what it is.

This was introduced by commit 0a0d111d40fd1 ("arm64: cpufeature: Pass capability
structure to ->enable callback").

The idea is to enable multiple entries in the table for a single capability.
Some capabilities (read errata) could be detected in multiple ways. e.g, different
MIDR ranges. (e.g, ARM64_HARDEN_BRANCH_PREDICTOR, ARM64_WORKAROUND_QCOM_FALKOR_E1003.
Now, even though the errata is the same, there might be different work arounds
for them in each "matching" cases. So, we need the "caps" passed on to the
enable() method to see if the "specific work around" should be applied
to the system/CPU (since CPU hwcap could be set by one of the cpu_capabilities
entry.) (as we invoke enable() for all "available" capabilities.)

Passing the caps information makes it easier to decide, by using the caps->matches().

> 
> Does any enable method use this for anything other than a struct
> arm64_cpu_capabilities const * ?

No, we use it only for const struct arm64_cpu_capability *.

> If not, it would be better to specifiy that.

Yes, that could be done.

Cheers
Suzuki

Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Dave Martin]

On Wed, Jan 17, 2018 at 02:52:09PM +0000, Suzuki K Poulose wrote:
> On 17/01/18 14:38, Dave Martin wrote:
> >On Wed, Jan 17, 2018 at 01:22:19PM +0000, Suzuki K Poulose wrote:
> >>On 17/01/18 12:25, Dave Martin wrote:
> >>>On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:

[...]

> >>>>+	for (; caps->matches; caps++) {
> >>>>+		if (cpus_have_cap(caps->capability)) {
> >>>>+			if (caps->enable)
> >>>>+				caps->enable((void *)caps);
> >>>
> >>>Do we really need this cast?
> >>
> >>Yes, otherwise we would be passing a "const *" where a "void *" is expected,
> >>and the compiler warns. Or we could simply change the prototype of the
> >>enable() method to accept a const capability ptr.
> >
> >Hmmm, what is this argument for exactly?  cpufeature.h doesn't explain
> >what it is.
> 
> This was introduced by commit 0a0d111d40fd1 ("arm64: cpufeature: Pass capability
> structure to ->enable callback").
> 
> The idea is to enable multiple entries in the table for a single capability.
> Some capabilities (read errata) could be detected in multiple ways. e.g, different
> MIDR ranges. (e.g, ARM64_HARDEN_BRANCH_PREDICTOR, ARM64_WORKAROUND_QCOM_FALKOR_E1003.
> Now, even though the errata is the same, there might be different work arounds
> for them in each "matching" cases. So, we need the "caps" passed on to the
> enable() method to see if the "specific work around" should be applied
> to the system/CPU (since CPU hwcap could be set by one of the cpu_capabilities
> entry.) (as we invoke enable() for all "available" capabilities.)
> 
> Passing the caps information makes it easier to decide, by using the caps->matches().

This makes sense (it's a common OOP idiom anyway:
object->method(object, ...))

> >
> >Does any enable method use this for anything other than a struct
> >arm64_cpu_capabilities const * ?
> 
> No, we use it only for const struct arm64_cpu_capability *.
> 
> >If not, it would be better to specifiy that.
> 
> Yes, that could be done.

OK, I vote for doing that.

Cheers
---Dave

[PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Dave Martin]

On Wed, Jan 17, 2018 at 02:52:09PM +0000, Suzuki K Poulose wrote:
> On 17/01/18 14:38, Dave Martin wrote:
> >On Wed, Jan 17, 2018 at 01:22:19PM +0000, Suzuki K Poulose wrote:
> >>On 17/01/18 12:25, Dave Martin wrote:
> >>>On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:

[...]

> >>>>+	for (; caps->matches; caps++) {
> >>>>+		if (cpus_have_cap(caps->capability)) {
> >>>>+			if (caps->enable)
> >>>>+				caps->enable((void *)caps);
> >>>
> >>>Do we really need this cast?
> >>
> >>Yes, otherwise we would be passing a "const *" where a "void *" is expected,
> >>and the compiler warns. Or we could simply change the prototype of the
> >>enable() method to accept a const capability ptr.
> >
> >Hmmm, what is this argument for exactly?  cpufeature.h doesn't explain
> >what it is.
> 
> This was introduced by commit 0a0d111d40fd1 ("arm64: cpufeature: Pass capability
> structure to ->enable callback").
> 
> The idea is to enable multiple entries in the table for a single capability.
> Some capabilities (read errata) could be detected in multiple ways. e.g, different
> MIDR ranges. (e.g, ARM64_HARDEN_BRANCH_PREDICTOR, ARM64_WORKAROUND_QCOM_FALKOR_E1003.
> Now, even though the errata is the same, there might be different work arounds
> for them in each "matching" cases. So, we need the "caps" passed on to the
> enable() method to see if the "specific work around" should be applied
> to the system/CPU (since CPU hwcap could be set by one of the cpu_capabilities
> entry.) (as we invoke enable() for all "available" capabilities.)
> 
> Passing the caps information makes it easier to decide, by using the caps->matches().

This makes sense (it's a common OOP idiom anyway:
object->method(object, ...))

> >
> >Does any enable method use this for anything other than a struct
> >arm64_cpu_capabilities const * ?
> 
> No, we use it only for const struct arm64_cpu_capability *.
> 
> >If not, it would be better to specifiy that.
> 
> Yes, that could be done.

OK, I vote for doing that.

Cheers
---Dave