[ARM64,v4.4,V3,12/44] arm64: cpufeature: Test 'matches' pointer to find the end of the list

Commit Message

Viresh Kumar Aug. 29, 2019, 11:33 a.m. UTC

From: James Morse <[e-mail]>

commit 644c2ae198412c956700e55a2acf80b2541f6aa5 upstream.

CPU feature code uses the desc field as a test to find the end of the list,
this means every entry must have a description. This generates noise for
entries in the list that aren't really features, but combinations of them.
e.g.
> CPU features: detected feature: Privileged Access Never
> CPU features: detected feature: PAN and not UAO

These combination features are needed for corner cases with alternatives,
where cpu features interact.

Change all walkers of the arm64_features[] and arm64_hwcaps[] lists to test
'matches' not 'desc', and only print 'desc' if it is non-NULL.

Signed-off-by: James Morse <[e-mail]>
Reviewed-by : Suzuki K Poulose <[e-mail]>
Signed-off-by: Catalin Marinas <[e-mail]>
Signed-off-by: Viresh Kumar <[e-mail]>
---
 arch/arm64/kernel/cpufeature.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Mark Rutland Sept. 2, 2019, 2:27 p.m. UTC | #1

On Thu, Aug 29, 2019 at 05:03:57PM +0530, Viresh Kumar wrote:
> From: James Morse <[e-mail]>
> 
> commit 644c2ae198412c956700e55a2acf80b2541f6aa5 upstream.
> 
> CPU feature code uses the desc field as a test to find the end of the list,
> this means every entry must have a description. This generates noise for
> entries in the list that aren't really features, but combinations of them.
> e.g.
> > CPU features: detected feature: Privileged Access Never
> > CPU features: detected feature: PAN and not UAO
> 
> These combination features are needed for corner cases with alternatives,
> where cpu features interact.
> 
> Change all walkers of the arm64_features[] and arm64_hwcaps[] lists to test
> 'matches' not 'desc', and only print 'desc' if it is non-NULL.
> 
> Signed-off-by: James Morse <[e-mail]>
> Reviewed-by : Suzuki K Poulose <[e-mail]>
> Signed-off-by: Catalin Marinas <[e-mail]>
> Signed-off-by: Viresh Kumar <[e-mail]>
> ---
>  arch/arm64/kernel/cpufeature.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

From looking at my 4.9.y/{meltdown,spectre} banches on kernel.org [1,2],
and chasing the history v4.4..v4.9, there are a number of patches I'd
expect to have alongside this that I don't spot in this series:

* e3661b128e53ee281e1e7c589a5b647890bd6d7c ("arm64: Allow a capability to be checked on a single CPU")
* 8f4137588261d7504f4aa022dc9d1a1fd1940e8e ("arm64: Allow checking of a CPU-local erratum")
* 67948af41f2e6818edeeba5182811c704d484949 ("arm64: capabilities: Handle duplicate entries for a capability")
* edf298cfce47ab7279d03b5203ae2ef3a58e49db ("arm64: cpufeature: __this_cpu_has_cap() shouldn't stop early")

... which IIUC are necessary for big.LITTLE to work correctly.

Have you verified this for big.LITTLE?

Thanks,
Mark.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=stable/4.9.y/meltdown
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=stable/4.9.y/spectre

> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index c1eddc07d996..bdb4cd9ffccf 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -744,7 +744,7 @@ static void setup_cpu_hwcaps(void)
>  	int i;
>  	const struct arm64_cpu_capabilities *hwcaps = arm64_hwcaps;
>  
> -	for (i = 0; hwcaps[i].desc; i++)
> +	for (i = 0; hwcaps[i].matches; i++)
>  		if (hwcaps[i].matches(&hwcaps[i]))
>  			cap_set_hwcap(&hwcaps[i]);
>  }
> @@ -754,11 +754,11 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
>  {
>  	int i;
>  
> -	for (i = 0; caps[i].desc; i++) {
> +	for (i = 0; caps[i].matches; i++) {
>  		if (!caps[i].matches(&caps[i]))
>  			continue;
>  
> -		if (!cpus_have_cap(caps[i].capability))
> +		if (!cpus_have_cap(caps[i].capability) && caps[i].desc)
>  			pr_info("%s %s\n", info, caps[i].desc);
>  		cpus_set_cap(caps[i].capability);
>  	}
> @@ -772,7 +772,7 @@ static void enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps)
>  {
>  	int i;
>  
> -	for (i = 0; caps[i].desc; i++)
> +	for (i = 0; caps[i].matches; i++)
>  		if (caps[i].enable && cpus_have_cap(caps[i].capability))
>  			/*
>  			 * Use stop_machine() as it schedules the work allowing
> @@ -884,7 +884,7 @@ void verify_local_cpu_capabilities(void)
>  		return;
>  
>  	caps = arm64_features;
> -	for (i = 0; caps[i].desc; i++) {
> +	for (i = 0; caps[i].matches; i++) {
>  		if (!cpus_have_cap(caps[i].capability) || !caps[i].sys_reg)
>  			continue;
>  		/*
> @@ -897,7 +897,7 @@ void verify_local_cpu_capabilities(void)
>  			caps[i].enable(NULL);
>  	}
>  
> -	for (i = 0, caps = arm64_hwcaps; caps[i].desc; i++) {
> +	for (i = 0, caps = arm64_hwcaps; caps[i].matches; i++) {
>  		if (!cpus_have_hwcap(&caps[i]))
>  			continue;
>  		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))
> -- 
> 2.21.0.rc0.269.g1a574e7a288b
>

Viresh Kumar Sept. 5, 2019, 7:45 a.m. UTC | #2

On 02-09-19, 15:27, Mark Rutland wrote:
> On Thu, Aug 29, 2019 at 05:03:57PM +0530, Viresh Kumar wrote:
> > From: James Morse <[e-mail]>
> > 
> > commit 644c2ae198412c956700e55a2acf80b2541f6aa5 upstream.
> > 
> > CPU feature code uses the desc field as a test to find the end of the list,
> > this means every entry must have a description. This generates noise for
> > entries in the list that aren't really features, but combinations of them.
> > e.g.
> > > CPU features: detected feature: Privileged Access Never
> > > CPU features: detected feature: PAN and not UAO
> > 
> > These combination features are needed for corner cases with alternatives,
> > where cpu features interact.
> > 
> > Change all walkers of the arm64_features[] and arm64_hwcaps[] lists to test
> > 'matches' not 'desc', and only print 'desc' if it is non-NULL.
> > 
> > Signed-off-by: James Morse <[e-mail]>
> > Reviewed-by : Suzuki K Poulose <[e-mail]>
> > Signed-off-by: Catalin Marinas <[e-mail]>
> > Signed-off-by: Viresh Kumar <[e-mail]>
> > ---
> >  arch/arm64/kernel/cpufeature.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> >From looking at my 4.9.y/{meltdown,spectre} banches on kernel.org [1,2],
> and chasing the history v4.4..v4.9, there are a number of patches I'd
> expect to have alongside this that I don't spot in this series:
> 
> * e3661b128e53ee281e1e7c589a5b647890bd6d7c ("arm64: Allow a capability to be checked on a single CPU")
> * 8f4137588261d7504f4aa022dc9d1a1fd1940e8e ("arm64: Allow checking of a CPU-local erratum")
> * 67948af41f2e6818edeeba5182811c704d484949 ("arm64: capabilities: Handle duplicate entries for a capability")
> * edf298cfce47ab7279d03b5203ae2ef3a58e49db ("arm64: cpufeature: __this_cpu_has_cap() shouldn't stop early")

I also had to pick this one for cleaner rebase:

752835019c15 arm64: HWCAP: Split COMPAT HWCAP table entries

> 
> ... which IIUC are necessary for big.LITTLE to work correctly.

I have pushed the changes to my branch again with above 5 patches and
some more reordering to match 4.9 log.

> Have you verified this for big.LITTLE?

Not sure if we ever talked about this earlier, but here is the
situation which I explained to Julien earlier.

I don't have access to the test-suite to verify that these patches
indeed fix the spectre mitigations and I was asked to backport these
and then ask for help from ARM to get these tested through the
test-suite. I was expecting Julien to do that earlier.

Julien did ask me to verify few things earlier, which can be done
without the test suite and was about checking that the new code paths
are getting hit now or not, which I did.

I haven't tested these on big LITTLE, though I can get the branch
through LAVA to get it tested on big LITTLE but I have no clue on what
I should be looking for in results :)

If there is some testing that can be done on my side for this, I sure
can do it. But I would need help from you on that to know what exactly
I need to check.

Thanks for the reviews Mark.

Mark Rutland Sept. 6, 2019, 1:49 p.m. UTC | #3

On Thu, Sep 05, 2019 at 01:15:06PM +0530, Viresh Kumar wrote:
> On 02-09-19, 15:27, Mark Rutland wrote:
> > On Thu, Aug 29, 2019 at 05:03:57PM +0530, Viresh Kumar wrote:
> > > From: James Morse <[e-mail]>
> > > 
> > > commit 644c2ae198412c956700e55a2acf80b2541f6aa5 upstream.
> > > 
> > > CPU feature code uses the desc field as a test to find the end of the list,
> > > this means every entry must have a description. This generates noise for
> > > entries in the list that aren't really features, but combinations of them.
> > > e.g.
> > > > CPU features: detected feature: Privileged Access Never
> > > > CPU features: detected feature: PAN and not UAO
> > > 
> > > These combination features are needed for corner cases with alternatives,
> > > where cpu features interact.
> > > 
> > > Change all walkers of the arm64_features[] and arm64_hwcaps[] lists to test
> > > 'matches' not 'desc', and only print 'desc' if it is non-NULL.
> > > 
> > > Signed-off-by: James Morse <[e-mail]>
> > > Reviewed-by : Suzuki K Poulose <[e-mail]>
> > > Signed-off-by: Catalin Marinas <[e-mail]>
> > > Signed-off-by: Viresh Kumar <[e-mail]>
> > > ---
> > >  arch/arm64/kernel/cpufeature.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > >From looking at my 4.9.y/{meltdown,spectre} banches on kernel.org [1,2],
> > and chasing the history v4.4..v4.9, there are a number of patches I'd
> > expect to have alongside this that I don't spot in this series:
> > 
> > * e3661b128e53ee281e1e7c589a5b647890bd6d7c ("arm64: Allow a capability to be checked on a single CPU")
> > * 8f4137588261d7504f4aa022dc9d1a1fd1940e8e ("arm64: Allow checking of a CPU-local erratum")
> > * 67948af41f2e6818edeeba5182811c704d484949 ("arm64: capabilities: Handle duplicate entries for a capability")
> > * edf298cfce47ab7279d03b5203ae2ef3a58e49db ("arm64: cpufeature: __this_cpu_has_cap() shouldn't stop early")
> 
> I also had to pick this one for cleaner rebase:
> 
> 752835019c15 arm64: HWCAP: Split COMPAT HWCAP table entries
> 
> > 
> > ... which IIUC are necessary for big.LITTLE to work correctly.
> 
> I have pushed the changes to my branch again with above 5 patches and
> some more reordering to match 4.9 log.

Thanks for this!

> > Have you verified this for big.LITTLE?
> 
> Not sure if we ever talked about this earlier, but here is the
> situation which I explained to Julien earlier.
> 
> I don't have access to the test-suite to verify that these patches
> indeed fix the spectre mitigations and I was asked to backport these
> and then ask for help from ARM to get these tested through the
> test-suite. I was expecting Julien to do that earlier.

Ok, thanks for providing this context.

As a heads-up, I'll be at LPC next week. While I'm there I won't be able
to test things, and I'm unlikely to find time to review, but I'll try to
do so ASAP once I return.

> Julien did ask me to verify few things earlier, which can be done
> without the test suite and was about checking that the new code paths
> are getting hit now or not, which I did.
> 
> I haven't tested these on big LITTLE, though I can get the branch
> through LAVA to get it tested on big LITTLE but I have no clue on what
> I should be looking for in results :)

I think it would be worthwhile to do that ASAP to make sure there are no
boot-time or run-time regressions. We can look at the logs later (or
re-run with some additional logging) to verify things are working as
expected.

> If there is some testing that can be done on my side for this, I sure
> can do it. But I would need help from you on that to know what exactly
> I need to check.

Sure. I'll have to take another look over the series to figure that out,
and as above I might not be able to do so until after LPC -- sorry!

Thanks,
Mark.

Viresh Kumar Sept. 10, 2019, 9:35 a.m. UTC | #4

On 06-09-19, 14:49, Mark Rutland wrote:
> I think it would be worthwhile to do that ASAP to make sure there are no
> boot-time or run-time regressions. We can look at the logs later (or
> re-run with some additional logging) to verify things are working as
> expected.

Sure, so my branch already goes through some LAVA testing from Linaro and
kernel-ci as well. It also gets build tested by 0-day testing bot.

I will make sure it runs on some big.LITTLE stuff on LAVA. Thanks.

Viresh Kumar Oct. 11, 2019, 6:36 a.m. UTC | #5

On 06-09-19, 14:49, Mark Rutland wrote:
> Sure. I'll have to take another look over the series to figure that out,
> and as above I might not be able to do so until after LPC -- sorry!

Just wanted to check if someone was able to come back to this series
to give further reviews :)

Patch

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index c1eddc07d996..bdb4cd9ffccf 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -744,7 +744,7 @@  static void setup_cpu_hwcaps(void)
 	int i;
 	const struct arm64_cpu_capabilities *hwcaps = arm64_hwcaps;
 
-	for (i = 0; hwcaps[i].desc; i++)
+	for (i = 0; hwcaps[i].matches; i++)
 		if (hwcaps[i].matches(&hwcaps[i]))
 			cap_set_hwcap(&hwcaps[i]);
 }
@@ -754,11 +754,11 @@  void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
 {
 	int i;
 
-	for (i = 0; caps[i].desc; i++) {
+	for (i = 0; caps[i].matches; i++) {
 		if (!caps[i].matches(&caps[i]))
 			continue;
 
-		if (!cpus_have_cap(caps[i].capability))
+		if (!cpus_have_cap(caps[i].capability) && caps[i].desc)
 			pr_info("%s %s\n", info, caps[i].desc);
 		cpus_set_cap(caps[i].capability);
 	}
@@ -772,7 +772,7 @@  static void enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps)
 {
 	int i;
 
-	for (i = 0; caps[i].desc; i++)
+	for (i = 0; caps[i].matches; i++)
 		if (caps[i].enable && cpus_have_cap(caps[i].capability))
 			/*
 			 * Use stop_machine() as it schedules the work allowing
@@ -884,7 +884,7 @@  void verify_local_cpu_capabilities(void)
 		return;
 
 	caps = arm64_features;
-	for (i = 0; caps[i].desc; i++) {
+	for (i = 0; caps[i].matches; i++) {
 		if (!cpus_have_cap(caps[i].capability) || !caps[i].sys_reg)
 			continue;
 		/*
@@ -897,7 +897,7 @@  void verify_local_cpu_capabilities(void)
 			caps[i].enable(NULL);
 	}
 
-	for (i = 0, caps = arm64_hwcaps; caps[i].desc; i++) {
+	for (i = 0, caps = arm64_hwcaps; caps[i].matches; i++) {
 		if (!cpus_have_hwcap(&caps[i]))
 			continue;
 		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))