diff mbox series

[v1,2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls

Message ID 20210727180649.12943-3-broonie@kernel.org
State Superseded
Headers show
Series kselftest/arm64: Vector length configuration tests | expand

Commit Message

Mark Brown July 27, 2021, 6:06 p.m. UTC 
Currently sve-probe-vls does not verify that the vector lengths reported
by the prctl() interface are actually what is reported by the architecture,
use the rdvl_sve() helper to validate this.

Signed-off-by: Mark Brown <broonie@kernel.org>

---
 tools/testing/selftests/arm64/fp/Makefile        | 2 +-
 tools/testing/selftests/arm64/fp/sve-probe-vls.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.20.1

Comments

Dave Martin July 28, 2021, 9:41 a.m. UTC | #1 
On Tue, Jul 27, 2021 at 07:06:48PM +0100, Mark Brown wrote:
> Currently sve-probe-vls does not verify that the vector lengths reported

> by the prctl() interface are actually what is reported by the architecture,

> use the rdvl_sve() helper to validate this.

> 

> Signed-off-by: Mark Brown <broonie@kernel.org>

> ---

>  tools/testing/selftests/arm64/fp/Makefile        | 2 +-

>  tools/testing/selftests/arm64/fp/sve-probe-vls.c | 5 +++++

>  2 files changed, 6 insertions(+), 1 deletion(-)

> 

> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile

> index ed62e7003b96..fa3a0167db2d 100644

> --- a/tools/testing/selftests/arm64/fp/Makefile

> +++ b/tools/testing/selftests/arm64/fp/Makefile

> @@ -13,7 +13,7 @@ fpsimd-test: fpsimd-test.o

>  	$(CC) -nostdlib $^ -o $@

>  rdvl-sve: rdvl-sve.o rdvl.o

>  sve-ptrace: sve-ptrace.o sve-ptrace-asm.o

> -sve-probe-vls: sve-probe-vls.o

> +sve-probe-vls: sve-probe-vls.o rdvl.o

>  sve-test: sve-test.o

>  	$(CC) -nostdlib $^ -o $@

>  vlset: vlset.o

> diff --git a/tools/testing/selftests/arm64/fp/sve-probe-vls.c b/tools/testing/selftests/arm64/fp/sve-probe-vls.c

> index 76e138525d55..a6cd1bd6e515 100644

> --- a/tools/testing/selftests/arm64/fp/sve-probe-vls.c

> +++ b/tools/testing/selftests/arm64/fp/sve-probe-vls.c

> @@ -13,6 +13,7 @@

>  #include <asm/sigcontext.h>

>  

>  #include "../../kselftest.h"

> +#include "rdvl.h"

>  

>  int main(int argc, char **argv)

>  {


This test was originally a diagnostic tool, so the way VLs are probed
aims to be efficient, rather than being defensive against the kernel
doing weird stuff.

If the kernel returns a vl > than the one we tried to set, we'll end
up in an infinite loop because of the way the loop deliberately uses the
kernel's return value to skip unsupported VLs.  So, it might be better
to probe every single architecturally possible VL and sanity check
everything instead.

While taking this opportunity, a check that the resulting vl is <= the
requested VL might be good too.

Also, if the kernel rounds any given vl down to some vl_actual, then it
should also round every possible other_vl down to vl_actual, where
vl_actual <= other_vl < vl.  It's not OK for different vls to be rounded
down to different actual values that have nothing to do with each other.

Since this test is our example of how to do this probing, it would be
good to preserve the "efficient" version though, and check that the
probed set of vls matches what is obtained by exhaustive probing.
Using comments and well-chosen function names, we can hopefully steer
people to use the efficient version that trusts the kernel in their own
code.

> @@ -38,6 +39,10 @@ int main(int argc, char **argv)

>  

>  		vl &= PR_SVE_VL_LEN_MASK;

>  

> +		if (rdvl_sve() != vl)

> +			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",

> +					   vl, rdvl_sve());

> +


If this fails, the VL vl wasn't "Set" at all.  I found this a bit
confusing from just looking at this hunk.

Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?

[...]

Thinking about it, due to the way these tests do inline vector length
changes we should also be making sure that they are not built with SVE
code gen by default, since inline VL changes could cause bad things to
happen in that case.

This isn't a problem for now, but as time goes on the toolchain may
start to be configured to compile for SVE by default.

I'm not sure of the correct option for forcing SVE off against the
compiler default -- check with the tools folks.  If there isn't a
proper way to do this, it's a toolchain defect so we should flag it up,
but -mgeneral-regs-only might work for us even though it's a bit of a
sledgehammer.

Compiler options aside, I think we _probably_ get away with a
native-SVE libc, since if everything in libc is VL-agnostic then it
shouldn't care what VL we call in with.  If being paranoid, we could
always restore the VL before doing anything other then prctl() and
RVDL, but we should probably wait for something to break first.


If we wanted to be super-robust, we could make the business end of each
test a pure-asm binary and control them via ptrace.  But that's probably
too much pain for now, even if the test harness were reusable.

Cheers
---Dave
Mark Brown July 28, 2021, 11:07 a.m. UTC | #2 
On Wed, Jul 28, 2021 at 10:41:51AM +0100, Dave Martin wrote:

> This test was originally a diagnostic tool, so the way VLs are probed

> aims to be efficient, rather than being defensive against the kernel

> doing weird stuff.


Yeah, the whole probing thing doesn't really fit into kselftest's idea
of what a test is - I just put this in here since it seemed like a cheap
extra validation that we could add in with little bother rather than
because it's a complete and thorough validation of every possible thing
that could go wrong.  I'd be just as happy to not modify this at all but
since it does try the intermediate VLs it didn't seem like a terrible
idea to throw in some validation while we're at it.

> If the kernel returns a vl > than the one we tried to set, we'll end

> up in an infinite loop because of the way the loop deliberately uses the

> kernel's return value to skip unsupported VLs.  So, it might be better

> to probe every single architecturally possible VL and sanity check

> everything instead.


Yup, that'd obviously be a complete rewrite though.  We'd not only need
to probe every possible vector length, but also validate that any
unsupported vector lengths report the expected vector length instead
when we try them.

> > +		if (rdvl_sve() != vl)

> > +			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",

> > +					   vl, rdvl_sve());


> If this fails, the VL vl wasn't "Set" at all.  I found this a bit

> confusing from just looking at this hunk.


> Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?


Sure.

> I'm not sure of the correct option for forcing SVE off against the

> compiler default -- check with the tools folks.  If there isn't a

> proper way to do this, it's a toolchain defect so we should flag it up,

> but -mgeneral-regs-only might work for us even though it's a bit of a

> sledgehammer.


-march should do the trick (if it doesn't the base kernel already has
issues).  This is a separate issue that affects all the arm64 selftests
I think.
Dave Martin July 28, 2021, 11:35 a.m. UTC | #3 
On Wed, Jul 28, 2021 at 12:07:01PM +0100, Mark Brown wrote:
> On Wed, Jul 28, 2021 at 10:41:51AM +0100, Dave Martin wrote:

> 

> > This test was originally a diagnostic tool, so the way VLs are probed

> > aims to be efficient, rather than being defensive against the kernel

> > doing weird stuff.

> 

> Yeah, the whole probing thing doesn't really fit into kselftest's idea

> of what a test is - I just put this in here since it seemed like a cheap

> extra validation that we could add in with little bother rather than

> because it's a complete and thorough validation of every possible thing

> that could go wrong.  I'd be just as happy to not modify this at all but

> since it does try the intermediate VLs it didn't seem like a terrible

> idea to throw in some validation while we're at it.

> 

> > If the kernel returns a vl > than the one we tried to set, we'll end

> > up in an infinite loop because of the way the loop deliberately uses the

> > kernel's return value to skip unsupported VLs.  So, it might be better

> > to probe every single architecturally possible VL and sanity check

> > everything instead.

> 

> Yup, that'd obviously be a complete rewrite though.  We'd not only need

> to probe every possible vector length, but also validate that any

> unsupported vector lengths report the expected vector length instead

> when we try them.


Fair enough, but is it worth dropping a comment in to clarify what this
does and doesn't test?  This could be an area for future improvement.

> > > +		if (rdvl_sve() != vl)

> > > +			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",

> > > +					   vl, rdvl_sve());

> 

> > If this fails, the VL vl wasn't "Set" at all.  I found this a bit

> > confusing from just looking at this hunk.

> 

> > Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?

> 

> Sure.

> 

> > I'm not sure of the correct option for forcing SVE off against the

> > compiler default -- check with the tools folks.  If there isn't a

> > proper way to do this, it's a toolchain defect so we should flag it up,

> > but -mgeneral-regs-only might work for us even though it's a bit of a

> > sledgehammer.

> 

> -march should do the trick (if it doesn't the base kernel already has

> issues).  This is a separate issue that affects all the arm64 selftests

> I think.


OK, if there's already precedent then I guess we can go with that.

Cheers
---Dave
diff mbox series

Patch

diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index ed62e7003b96..fa3a0167db2d 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -13,7 +13,7 @@  fpsimd-test: fpsimd-test.o
 	$(CC) -nostdlib $^ -o $@
 rdvl-sve: rdvl-sve.o rdvl.o
 sve-ptrace: sve-ptrace.o sve-ptrace-asm.o
-sve-probe-vls: sve-probe-vls.o
+sve-probe-vls: sve-probe-vls.o rdvl.o
 sve-test: sve-test.o
 	$(CC) -nostdlib $^ -o $@
 vlset: vlset.o
diff --git a/tools/testing/selftests/arm64/fp/sve-probe-vls.c b/tools/testing/selftests/arm64/fp/sve-probe-vls.c
index 76e138525d55..a6cd1bd6e515 100644
--- a/tools/testing/selftests/arm64/fp/sve-probe-vls.c
+++ b/tools/testing/selftests/arm64/fp/sve-probe-vls.c
@@ -13,6 +13,7 @@ 
 #include <asm/sigcontext.h>
 
 #include "../../kselftest.h"
+#include "rdvl.h"
 
 int main(int argc, char **argv)
 {
@@ -38,6 +39,10 @@  int main(int argc, char **argv)
 
 		vl &= PR_SVE_VL_LEN_MASK;
 
+		if (rdvl_sve() != vl)
+			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",
+					   vl, rdvl_sve());
+
 		if (!sve_vl_valid(vl))
 			ksft_exit_fail_msg("VL %d invalid\n", vl);
 		vq = sve_vq_from_vl(vl);