All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] kselftest/arm64: Vector length configuration tests
@ 2021-07-28 16:33 ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

Currently we don't have full automated tests for the vector length
configuation ABIs offered for SVE, we have a helper binary for setting
the vector length which can be used for manual tests and we use the
prctl() interface to enumerate the vector lengths but don't actually
verify that the vector lengths enumerated were set.

This patch series provides a small helper which allows us to get the
currently configured vector length using the RDVL instruction via either
a library call or stdout of a process and then uses this to both add
verification of enumerated vector lengths to our existing tests and also
add a new test program which exercises both the prctl() and sysfs
interfaces.

In preparation for the forthcomng support for the Scalable Matrix
Extension (SME) [1] which introduces a new vector length managed via a
very similar hardware interface the helper and new test program are
parameterised with the goal of allowing reuse for SME.

[1] https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/scalable-matrix-extension-armv9-a-architecture

v2:
 - Tweak log message on failure in sve-probe-vls.
 - Stylistic changes in vec-syscfg.
 - Flush stdout before forking in vec-syscfg.
 - Use EXIT_FAILURE.
 - Use fdopen() to get child output.
 - Replace a bunch of UNIX API usage with stdio.
 - Add a TODO list.
 - Verify that we're root before testing writes to /proc.

Mark Brown (4):
  kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  kselftest/arm64: Validate vector lengths are set in sve-probe-vls
  kselftest/arm64: Add tests for SVE vector configuration
  kselftest/arm64: Add a TODO list for floating point tests

 tools/testing/selftests/arm64/fp/.gitignore   |   2 +
 tools/testing/selftests/arm64/fp/Makefile     |  11 +-
 tools/testing/selftests/arm64/fp/TODO         |   3 +
 tools/testing/selftests/arm64/fp/rdvl-sve.c   |  14 +
 tools/testing/selftests/arm64/fp/rdvl.S       |   9 +
 tools/testing/selftests/arm64/fp/rdvl.h       |   8 +
 .../selftests/arm64/fp/sve-probe-vls.c        |   5 +
 tools/testing/selftests/arm64/fp/vec-syscfg.c | 580 ++++++++++++++++++
 8 files changed, 629 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/arm64/fp/TODO
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h
 create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c


base-commit: ff1176468d368232b684f75e82563369208bc371
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 0/4] kselftest/arm64: Vector length configuration tests
@ 2021-07-28 16:33 ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

Currently we don't have full automated tests for the vector length
configuation ABIs offered for SVE, we have a helper binary for setting
the vector length which can be used for manual tests and we use the
prctl() interface to enumerate the vector lengths but don't actually
verify that the vector lengths enumerated were set.

This patch series provides a small helper which allows us to get the
currently configured vector length using the RDVL instruction via either
a library call or stdout of a process and then uses this to both add
verification of enumerated vector lengths to our existing tests and also
add a new test program which exercises both the prctl() and sysfs
interfaces.

In preparation for the forthcomng support for the Scalable Matrix
Extension (SME) [1] which introduces a new vector length managed via a
very similar hardware interface the helper and new test program are
parameterised with the goal of allowing reuse for SME.

[1] https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/scalable-matrix-extension-armv9-a-architecture

v2:
 - Tweak log message on failure in sve-probe-vls.
 - Stylistic changes in vec-syscfg.
 - Flush stdout before forking in vec-syscfg.
 - Use EXIT_FAILURE.
 - Use fdopen() to get child output.
 - Replace a bunch of UNIX API usage with stdio.
 - Add a TODO list.
 - Verify that we're root before testing writes to /proc.

Mark Brown (4):
  kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  kselftest/arm64: Validate vector lengths are set in sve-probe-vls
  kselftest/arm64: Add tests for SVE vector configuration
  kselftest/arm64: Add a TODO list for floating point tests

 tools/testing/selftests/arm64/fp/.gitignore   |   2 +
 tools/testing/selftests/arm64/fp/Makefile     |  11 +-
 tools/testing/selftests/arm64/fp/TODO         |   3 +
 tools/testing/selftests/arm64/fp/rdvl-sve.c   |  14 +
 tools/testing/selftests/arm64/fp/rdvl.S       |   9 +
 tools/testing/selftests/arm64/fp/rdvl.h       |   8 +
 .../selftests/arm64/fp/sve-probe-vls.c        |   5 +
 tools/testing/selftests/arm64/fp/vec-syscfg.c | 580 ++++++++++++++++++
 8 files changed, 629 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/arm64/fp/TODO
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h
 create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c


base-commit: ff1176468d368232b684f75e82563369208bc371
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  2021-07-28 16:33 ` Mark Brown
@ 2021-07-28 16:33   ` Mark Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

SVE provides an instruction RDVL which reports the currently configured
vector length. In order to validate that our vector length configuration
interfaces are working correctly without having to build the C code for
our test programs with SVE enabled provide a trivial assembly library
with a C callable function that executes RDVL. Since these interfaces
also control behaviour on exec*() provide a trivial wrapper program which
reports the currently configured vector length on stdout, tests can use
this to verify that behaviour on exec*() is as expected.

In preparation for providing similar helper functionality for SME, the
Scalable Matrix Extension, which allows separately configured vector
lengths to be read back both the assembler function and wrapper binary
have SVE included in their name.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/.gitignore |  1 +
 tools/testing/selftests/arm64/fp/Makefile   |  6 +++++-
 tools/testing/selftests/arm64/fp/rdvl-sve.c | 14 ++++++++++++++
 tools/testing/selftests/arm64/fp/rdvl.S     |  9 +++++++++
 tools/testing/selftests/arm64/fp/rdvl.h     |  8 ++++++++
 5 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h

diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
index d66f76d2a650..6b53a7b60fee 100644
--- a/tools/testing/selftests/arm64/fp/.gitignore
+++ b/tools/testing/selftests/arm64/fp/.gitignore
@@ -1,4 +1,5 @@
 fpsimd-test
+rdvl-sve
 sve-probe-vls
 sve-ptrace
 sve-test
diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index a57009d3a0dc..ed62e7003b96 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -2,12 +2,16 @@
 
 CFLAGS += -I../../../../../usr/include/
 TEST_GEN_PROGS := sve-ptrace sve-probe-vls
-TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress sve-test sve-stress vlset
+TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
+	rdvl-sve \
+	sve-test sve-stress \
+	vlset
 
 all: $(TEST_GEN_PROGS) $(TEST_PROGS_EXTENDED)
 
 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-test: sve-test.o
diff --git a/tools/testing/selftests/arm64/fp/rdvl-sve.c b/tools/testing/selftests/arm64/fp/rdvl-sve.c
new file mode 100644
index 000000000000..7f8a13a18f5d
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl-sve.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <stdio.h>
+
+#include "rdvl.h"
+
+int main(void)
+{
+	int vl = rdvl_sve();
+
+	printf("%d\n", vl);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/arm64/fp/rdvl.S b/tools/testing/selftests/arm64/fp/rdvl.S
new file mode 100644
index 000000000000..6e76dd720b87
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl.S
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2021 ARM Limited.
+
+.arch_extension sve
+
+.globl rdvl_sve
+rdvl_sve:
+	rdvl	x0, #1
+	ret
diff --git a/tools/testing/selftests/arm64/fp/rdvl.h b/tools/testing/selftests/arm64/fp/rdvl.h
new file mode 100644
index 000000000000..7c9d953fc9e7
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef RDVL_H
+#define RDVL_H
+
+int rdvl_sve(void);
+
+#endif
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
@ 2021-07-28 16:33   ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

SVE provides an instruction RDVL which reports the currently configured
vector length. In order to validate that our vector length configuration
interfaces are working correctly without having to build the C code for
our test programs with SVE enabled provide a trivial assembly library
with a C callable function that executes RDVL. Since these interfaces
also control behaviour on exec*() provide a trivial wrapper program which
reports the currently configured vector length on stdout, tests can use
this to verify that behaviour on exec*() is as expected.

In preparation for providing similar helper functionality for SME, the
Scalable Matrix Extension, which allows separately configured vector
lengths to be read back both the assembler function and wrapper binary
have SVE included in their name.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/.gitignore |  1 +
 tools/testing/selftests/arm64/fp/Makefile   |  6 +++++-
 tools/testing/selftests/arm64/fp/rdvl-sve.c | 14 ++++++++++++++
 tools/testing/selftests/arm64/fp/rdvl.S     |  9 +++++++++
 tools/testing/selftests/arm64/fp/rdvl.h     |  8 ++++++++
 5 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h

diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
index d66f76d2a650..6b53a7b60fee 100644
--- a/tools/testing/selftests/arm64/fp/.gitignore
+++ b/tools/testing/selftests/arm64/fp/.gitignore
@@ -1,4 +1,5 @@
 fpsimd-test
+rdvl-sve
 sve-probe-vls
 sve-ptrace
 sve-test
diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index a57009d3a0dc..ed62e7003b96 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -2,12 +2,16 @@
 
 CFLAGS += -I../../../../../usr/include/
 TEST_GEN_PROGS := sve-ptrace sve-probe-vls
-TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress sve-test sve-stress vlset
+TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
+	rdvl-sve \
+	sve-test sve-stress \
+	vlset
 
 all: $(TEST_GEN_PROGS) $(TEST_PROGS_EXTENDED)
 
 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-test: sve-test.o
diff --git a/tools/testing/selftests/arm64/fp/rdvl-sve.c b/tools/testing/selftests/arm64/fp/rdvl-sve.c
new file mode 100644
index 000000000000..7f8a13a18f5d
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl-sve.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <stdio.h>
+
+#include "rdvl.h"
+
+int main(void)
+{
+	int vl = rdvl_sve();
+
+	printf("%d\n", vl);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/arm64/fp/rdvl.S b/tools/testing/selftests/arm64/fp/rdvl.S
new file mode 100644
index 000000000000..6e76dd720b87
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl.S
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2021 ARM Limited.
+
+.arch_extension sve
+
+.globl rdvl_sve
+rdvl_sve:
+	rdvl	x0, #1
+	ret
diff --git a/tools/testing/selftests/arm64/fp/rdvl.h b/tools/testing/selftests/arm64/fp/rdvl.h
new file mode 100644
index 000000000000..7c9d953fc9e7
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef RDVL_H
+#define RDVL_H
+
+int rdvl_sve(void);
+
+#endif
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 2/4] kselftest/arm64: Validate vector lengths are set in sve-probe-vls
  2021-07-28 16:33 ` Mark Brown
@ 2021-07-28 16:33   ` Mark Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

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..a24eca7a4ecb 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("PR_SVE_SET_VL reports %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);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 2/4] kselftest/arm64: Validate vector lengths are set in sve-probe-vls
@ 2021-07-28 16:33   ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

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..a24eca7a4ecb 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("PR_SVE_SET_VL reports %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);
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 3/4] kselftest/arm64: Add tests for SVE vector configuration
  2021-07-28 16:33 ` Mark Brown
@ 2021-07-28 16:33   ` Mark Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

We provide interfaces for configuring the SVE vector length seen by
processes using prctl and also via /proc for configuring the default
values. Provide tests that exercise all these interfaces and verify that
they take effect as expected, though at present no test fully enumerates
all the possible vector lengths.

A subset of this is already tested via sve-probe-vls but the /proc
interfaces are not currently covered at all.

In preparation for the forthcoming support for SME, the Scalable Matrix
Extension, which has separately but similarly configured vector lengths
which we expect to offer similar userspace interfaces for, all the actual
files and prctls used are parameterised and we don't validate that the
architectural minimum vector length is the minimum we see.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/.gitignore   |   1 +
 tools/testing/selftests/arm64/fp/Makefile     |   3 +-
 tools/testing/selftests/arm64/fp/vec-syscfg.c | 580 ++++++++++++++++++
 3 files changed, 583 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c

diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
index 6b53a7b60fee..b67395903b9b 100644
--- a/tools/testing/selftests/arm64/fp/.gitignore
+++ b/tools/testing/selftests/arm64/fp/.gitignore
@@ -3,4 +3,5 @@ rdvl-sve
 sve-probe-vls
 sve-ptrace
 sve-test
+vec-syscfg
 vlset
diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index fa3a0167db2d..f2abdd6ba12e 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 CFLAGS += -I../../../../../usr/include/
-TEST_GEN_PROGS := sve-ptrace sve-probe-vls
+TEST_GEN_PROGS := sve-ptrace sve-probe-vls vec-syscfg
 TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
 	rdvl-sve \
 	sve-test sve-stress \
@@ -16,6 +16,7 @@ sve-ptrace: sve-ptrace.o sve-ptrace-asm.o
 sve-probe-vls: sve-probe-vls.o rdvl.o
 sve-test: sve-test.o
 	$(CC) -nostdlib $^ -o $@
+vec-syscfg: vec-syscfg.o rdvl.o
 vlset: vlset.o
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
new file mode 100644
index 000000000000..63d080537d21
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
@@ -0,0 +1,580 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 ARM Limited.
+ * Original author: Mark Brown <broonie@kernel.org>
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/auxv.h>
+#include <sys/prctl.h>
+#include <sys/ptrace.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <asm/sigcontext.h>
+#include <asm/hwcap.h>
+
+#include "../../kselftest.h"
+#include "rdvl.h"
+
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
+
+#define ARCH_MIN_VL SVE_VL_MIN
+
+struct vec_data {
+	const char *name;
+	unsigned long hwcap_type;
+	unsigned long hwcap;
+	const char *rdvl_binary;
+	int (*rdvl)(void);
+
+	int prctl_get;
+	int prctl_set;
+	const char *default_vl_file;
+
+	int default_vl;
+	int min_vl;
+	int max_vl;
+};
+
+
+static struct vec_data vec_data[] = {
+	{
+		.name = "SVE",
+		.hwcap_type = AT_HWCAP,
+		.hwcap = HWCAP_SVE,
+		.rdvl = rdvl_sve,
+		.rdvl_binary = "./rdvl-sve",
+		.prctl_get = PR_SVE_GET_VL,
+		.prctl_set = PR_SVE_SET_VL,
+		.default_vl_file = "/proc/sys/abi/sve_default_vector_length",
+	},
+};
+
+/* Start a new process and return the vector length it sees */
+int get_child_rdvl(struct vec_data *data)
+{
+	FILE *out;
+	int pipefd[2];
+	pid_t pid, child;
+	int read_vl, ret;
+
+	ret = pipe(pipefd);
+	if (ret == -1) {
+		ksft_print_msg("pipe() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		return -1;
+	}
+
+	fflush(stdout);
+
+	child = fork();
+	if (child == -1) {
+		ksft_print_msg("fork() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		return -1;
+	}
+
+	/* Child: put vector length on the pipe */
+	if (child == 0) {
+		/*
+		 * Replace stdout with the pipe, errors to stderr from
+		 * here as kselftest prints to stdout.
+		 */
+		ret = dup2(pipefd[1], 1);
+		if (ret == -1) {
+			fprintf(stderr, "dup2() %d\n", errno);
+			exit(EXIT_FAILURE);
+		}
+
+		/* exec() a new binary which puts the VL on stdout */
+		ret = execl(data->rdvl_binary, data->rdvl_binary, NULL);
+		fprintf(stderr, "execl(%s) failed: %d\n",
+			data->rdvl_binary, errno, strerror(errno));
+
+		exit(EXIT_FAILURE);
+	}
+
+	close(pipefd[1]);
+
+	/* Parent; wait for the exit status from the child & verify it */
+	while (1) {
+		pid = wait(&ret);
+		if (pid == -1) {
+			ksft_print_msg("wait() failed: %d (%s)\n",
+				       errno, strerror(errno));
+			return -1;
+		}
+
+		if (pid != child)
+			continue;
+
+		if (!WIFEXITED(ret)) {
+			ksft_print_msg("child exited abnormally\n");
+			return -1;
+		}
+
+		if (WEXITSTATUS(ret) != 0) {
+			ksft_print_msg("child returned error %d\n",
+				       WEXITSTATUS(ret));
+			return -1;
+		}
+
+		out = fdopen(pipefd[0], "r");
+		if (!out) {
+			ksft_print_msg("failed to open child stdout\n");
+			return -1;
+		}
+
+		ret = fscanf(out, "%d", &read_vl);
+		fclose(out);
+		if (ret != 1) {
+			ksft_print_msg("failed to parse VL from '%s'\n",
+				       data->rdvl_binary);
+			return -1;
+		}
+
+		return read_vl;
+	}
+}
+
+int file_read_integer(const char *name, int *val)
+{
+	FILE *f;
+	int ret;
+
+	f = fopen(name, "r");
+	if (!f) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	ret = fscanf(f, "%d", val);
+	fclose(f);
+	if (ret != 1) {
+		ksft_test_result_fail("Failed to parse %s\n", name);
+		return -1;
+	}
+
+	return 0;
+}
+
+int file_write_integer(const char *name, int val)
+{
+	FILE *f;
+	int ret;
+
+	f = fopen(name, "w");
+	if (!f) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	fprintf(f, "%d", val);
+	fclose(f);
+	if (ret < 0) {
+		ksft_test_result_fail("Error writing %d to %s\n",
+				      val, name);
+		return -1;
+	}
+
+	return 0;
+}
+
+/*
+ * Verify that we can read the default VL via proc, checking that it
+ * is set in a freshly spawned child.
+ */
+void proc_read_default(struct vec_data *data)
+{
+	int default_vl, child_vl, ret;
+
+	ret = file_read_integer(data->default_vl_file, &default_vl);
+	if (ret != 0)
+		return;
+
+	/* Is this the actual default seen by new processes? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != default_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      default_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s default vector length %d\n", data->name,
+			      default_vl);
+	data->default_vl = default_vl;
+}
+
+/* Verify that we can write a minimum value and have it take effect */
+void proc_write_min(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	if (geteuid() != 0) {
+		ksft_test_result_skip("Need to be root to write to /proc\n");
+		return;
+	}
+
+	ret = file_write_integer(data->default_vl_file, ARCH_MIN_VL);
+	if (ret != 0)
+		return;
+
+	/* What was the new value? */
+	ret = file_read_integer(data->default_vl_file, &new_default);
+	if (ret != 0)
+		return;
+
+	/* Did it take effect in a new process? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != new_default) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      new_default, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s minimum vector length %d\n", data->name,
+			      new_default);
+	data->min_vl = new_default;
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* Verify that we can write a maximum value and have it take effect */
+void proc_write_max(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	if (geteuid() != 0) {
+		ksft_test_result_skip("Need to be root to write to /proc\n");
+		return;
+	}
+
+	/* -1 is accepted by the /proc interface as the maximum VL */
+	ret = file_write_integer(data->default_vl_file, -1);
+	if (ret != 0)
+		return;
+
+	/* What was the new value? */
+	ret = file_read_integer(data->default_vl_file, &new_default);
+	if (ret != 0)
+		return;
+
+	/* Did it take effect in a new process? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != new_default) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      new_default, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s maximum vector length %d\n", data->name,
+			      new_default);
+	data->max_vl = new_default;
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* Can we read back a VL from prctl? */
+void prctl_get(struct vec_data *data)
+{
+	int ret;
+
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	/* Mask out any flags */
+	ret &= PR_SVE_VL_LEN_MASK;
+
+	/* Is that what we can read back directly? */
+	if (ret == data->rdvl())
+		ksft_test_result_pass("%s current VL is %d\n",
+				      data->name, ret);
+	else
+		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
+				      data->name, ret, data->rdvl());
+}
+
+/* Does the prctl let us set the VL we already have? */
+void prctl_set_same(struct vec_data *data)
+{
+	int cur_vl = data->rdvl();
+	int ret;
+
+	ret = prctl(data->prctl_set, cur_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	if (cur_vl != data->rdvl())
+		ksft_test_result_pass("%s current VL is %d\n",
+				      data->name, ret);
+	else
+		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
+				      data->name, ret, data->rdvl());
+}
+
+/* Can we set a new VL for this process? */
+void prctl_set(struct vec_data *data)
+{
+	int ret;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	/* Try to set the minimum VL */
+	ret = prctl(data->prctl_set, data->min_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	if ((ret & PR_SVE_VL_LEN_MASK) != data->min_vl) {
+		ksft_test_result_fail("%s prctl set %d but return value is %d\n",
+				      data->name, data->min_vl, data->rdvl());
+		return;
+	}
+
+	if (data->rdvl() != data->min_vl) {
+		ksft_test_result_fail("%s set %d but RDVL is %d\n",
+				      data->name, data->min_vl, data->rdvl());
+		return;
+	}
+
+	/* Try to set the maximum VL */
+	ret = prctl(data->prctl_set, data->max_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->max_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	if ((ret & PR_SVE_VL_LEN_MASK) != data->max_vl) {
+		ksft_test_result_fail("%s prctl() set %d but return value is %d\n",
+				      data->name, data->max_vl, data->rdvl());
+		return;
+	}
+
+	/* The _INHERIT flag should not be present when we read the VL */
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	if (ret & PR_SVE_VL_INHERIT) {
+		ksft_test_result_fail("%s prctl() reports _INHERIT\n",
+				      data->name);
+		return;
+	}
+
+	ksft_test_result_pass("%s prctl() set min/max\n", data->name);
+}
+
+/* If we didn't request it a new VL shouldn't affect the child */
+void prctl_set_no_child(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	ret = prctl(data->prctl_set, data->min_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Ensure the default VL is different */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	/* Check that the child has the default we just set */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->max_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->max_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length used default\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* If we didn't request it a new VL shouldn't affect the child */
+void prctl_set_for_child(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_VL_INHERIT);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* The _INHERIT flag should be present when we read the VL */
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+	if (!(ret & PR_SVE_VL_INHERIT)) {
+		ksft_test_result_fail("%s prctl() does not report _INHERIT\n",
+				      data->name);
+		return;
+	}
+
+	/* Ensure the default VL is different */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	/* Check that the child inherited our VL */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->min_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->min_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length was inherited\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* _ONEXEC takes effect only in the child process */
+void prctl_set_onexec(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	/* Set a known value for the default and our current VL */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	ret = prctl(data->prctl_set, data->max_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Set a different value for the child to have on exec */
+	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_SET_VL_ONEXEC);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Our current VL should stay the same */
+	if (data->rdvl() != data->max_vl) {
+		ksft_test_result_fail("%s VL changed by _ONEXEC prctl()\n",
+				      data->name);
+		return;
+	}
+
+	/* Check that the child inherited our VL */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->min_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->min_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length set on exec\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+typedef void (*test_type)(struct vec_data *);
+
+test_type tests[] = {
+	/*
+	 * The default/min/max tests must be first and in this order
+	 * to provide data for other tests.
+	 */
+	proc_read_default,
+	proc_write_min,
+	proc_write_max,
+
+	prctl_get,
+	prctl_set,
+	prctl_set_no_child,
+	prctl_set_for_child,
+	prctl_set_onexec,
+};
+
+int main(void)
+{
+	int i, j;
+
+	ksft_print_header();
+	ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data));
+
+	for (i = 0; i < ARRAY_SIZE(vec_data); i++) {
+		struct vec_data *data = &vec_data[i];
+		int supported = getauxval(data->hwcap_type) & data->hwcap;
+
+		for (j = 0; j < ARRAY_SIZE(tests); j++) {
+			if (supported)
+				tests[j](data);
+			else
+				ksft_test_result_skip("%s not supported\n",
+						      data->name);
+		}
+	}
+
+	ksft_exit_pass();
+}
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 3/4] kselftest/arm64: Add tests for SVE vector configuration
@ 2021-07-28 16:33   ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

We provide interfaces for configuring the SVE vector length seen by
processes using prctl and also via /proc for configuring the default
values. Provide tests that exercise all these interfaces and verify that
they take effect as expected, though at present no test fully enumerates
all the possible vector lengths.

A subset of this is already tested via sve-probe-vls but the /proc
interfaces are not currently covered at all.

In preparation for the forthcoming support for SME, the Scalable Matrix
Extension, which has separately but similarly configured vector lengths
which we expect to offer similar userspace interfaces for, all the actual
files and prctls used are parameterised and we don't validate that the
architectural minimum vector length is the minimum we see.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/.gitignore   |   1 +
 tools/testing/selftests/arm64/fp/Makefile     |   3 +-
 tools/testing/selftests/arm64/fp/vec-syscfg.c | 580 ++++++++++++++++++
 3 files changed, 583 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c

diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
index 6b53a7b60fee..b67395903b9b 100644
--- a/tools/testing/selftests/arm64/fp/.gitignore
+++ b/tools/testing/selftests/arm64/fp/.gitignore
@@ -3,4 +3,5 @@ rdvl-sve
 sve-probe-vls
 sve-ptrace
 sve-test
+vec-syscfg
 vlset
diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index fa3a0167db2d..f2abdd6ba12e 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 CFLAGS += -I../../../../../usr/include/
-TEST_GEN_PROGS := sve-ptrace sve-probe-vls
+TEST_GEN_PROGS := sve-ptrace sve-probe-vls vec-syscfg
 TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
 	rdvl-sve \
 	sve-test sve-stress \
@@ -16,6 +16,7 @@ sve-ptrace: sve-ptrace.o sve-ptrace-asm.o
 sve-probe-vls: sve-probe-vls.o rdvl.o
 sve-test: sve-test.o
 	$(CC) -nostdlib $^ -o $@
+vec-syscfg: vec-syscfg.o rdvl.o
 vlset: vlset.o
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
new file mode 100644
index 000000000000..63d080537d21
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
@@ -0,0 +1,580 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 ARM Limited.
+ * Original author: Mark Brown <broonie@kernel.org>
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/auxv.h>
+#include <sys/prctl.h>
+#include <sys/ptrace.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <asm/sigcontext.h>
+#include <asm/hwcap.h>
+
+#include "../../kselftest.h"
+#include "rdvl.h"
+
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
+
+#define ARCH_MIN_VL SVE_VL_MIN
+
+struct vec_data {
+	const char *name;
+	unsigned long hwcap_type;
+	unsigned long hwcap;
+	const char *rdvl_binary;
+	int (*rdvl)(void);
+
+	int prctl_get;
+	int prctl_set;
+	const char *default_vl_file;
+
+	int default_vl;
+	int min_vl;
+	int max_vl;
+};
+
+
+static struct vec_data vec_data[] = {
+	{
+		.name = "SVE",
+		.hwcap_type = AT_HWCAP,
+		.hwcap = HWCAP_SVE,
+		.rdvl = rdvl_sve,
+		.rdvl_binary = "./rdvl-sve",
+		.prctl_get = PR_SVE_GET_VL,
+		.prctl_set = PR_SVE_SET_VL,
+		.default_vl_file = "/proc/sys/abi/sve_default_vector_length",
+	},
+};
+
+/* Start a new process and return the vector length it sees */
+int get_child_rdvl(struct vec_data *data)
+{
+	FILE *out;
+	int pipefd[2];
+	pid_t pid, child;
+	int read_vl, ret;
+
+	ret = pipe(pipefd);
+	if (ret == -1) {
+		ksft_print_msg("pipe() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		return -1;
+	}
+
+	fflush(stdout);
+
+	child = fork();
+	if (child == -1) {
+		ksft_print_msg("fork() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		return -1;
+	}
+
+	/* Child: put vector length on the pipe */
+	if (child == 0) {
+		/*
+		 * Replace stdout with the pipe, errors to stderr from
+		 * here as kselftest prints to stdout.
+		 */
+		ret = dup2(pipefd[1], 1);
+		if (ret == -1) {
+			fprintf(stderr, "dup2() %d\n", errno);
+			exit(EXIT_FAILURE);
+		}
+
+		/* exec() a new binary which puts the VL on stdout */
+		ret = execl(data->rdvl_binary, data->rdvl_binary, NULL);
+		fprintf(stderr, "execl(%s) failed: %d\n",
+			data->rdvl_binary, errno, strerror(errno));
+
+		exit(EXIT_FAILURE);
+	}
+
+	close(pipefd[1]);
+
+	/* Parent; wait for the exit status from the child & verify it */
+	while (1) {
+		pid = wait(&ret);
+		if (pid == -1) {
+			ksft_print_msg("wait() failed: %d (%s)\n",
+				       errno, strerror(errno));
+			return -1;
+		}
+
+		if (pid != child)
+			continue;
+
+		if (!WIFEXITED(ret)) {
+			ksft_print_msg("child exited abnormally\n");
+			return -1;
+		}
+
+		if (WEXITSTATUS(ret) != 0) {
+			ksft_print_msg("child returned error %d\n",
+				       WEXITSTATUS(ret));
+			return -1;
+		}
+
+		out = fdopen(pipefd[0], "r");
+		if (!out) {
+			ksft_print_msg("failed to open child stdout\n");
+			return -1;
+		}
+
+		ret = fscanf(out, "%d", &read_vl);
+		fclose(out);
+		if (ret != 1) {
+			ksft_print_msg("failed to parse VL from '%s'\n",
+				       data->rdvl_binary);
+			return -1;
+		}
+
+		return read_vl;
+	}
+}
+
+int file_read_integer(const char *name, int *val)
+{
+	FILE *f;
+	int ret;
+
+	f = fopen(name, "r");
+	if (!f) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	ret = fscanf(f, "%d", val);
+	fclose(f);
+	if (ret != 1) {
+		ksft_test_result_fail("Failed to parse %s\n", name);
+		return -1;
+	}
+
+	return 0;
+}
+
+int file_write_integer(const char *name, int val)
+{
+	FILE *f;
+	int ret;
+
+	f = fopen(name, "w");
+	if (!f) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	fprintf(f, "%d", val);
+	fclose(f);
+	if (ret < 0) {
+		ksft_test_result_fail("Error writing %d to %s\n",
+				      val, name);
+		return -1;
+	}
+
+	return 0;
+}
+
+/*
+ * Verify that we can read the default VL via proc, checking that it
+ * is set in a freshly spawned child.
+ */
+void proc_read_default(struct vec_data *data)
+{
+	int default_vl, child_vl, ret;
+
+	ret = file_read_integer(data->default_vl_file, &default_vl);
+	if (ret != 0)
+		return;
+
+	/* Is this the actual default seen by new processes? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != default_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      default_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s default vector length %d\n", data->name,
+			      default_vl);
+	data->default_vl = default_vl;
+}
+
+/* Verify that we can write a minimum value and have it take effect */
+void proc_write_min(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	if (geteuid() != 0) {
+		ksft_test_result_skip("Need to be root to write to /proc\n");
+		return;
+	}
+
+	ret = file_write_integer(data->default_vl_file, ARCH_MIN_VL);
+	if (ret != 0)
+		return;
+
+	/* What was the new value? */
+	ret = file_read_integer(data->default_vl_file, &new_default);
+	if (ret != 0)
+		return;
+
+	/* Did it take effect in a new process? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != new_default) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      new_default, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s minimum vector length %d\n", data->name,
+			      new_default);
+	data->min_vl = new_default;
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* Verify that we can write a maximum value and have it take effect */
+void proc_write_max(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	if (geteuid() != 0) {
+		ksft_test_result_skip("Need to be root to write to /proc\n");
+		return;
+	}
+
+	/* -1 is accepted by the /proc interface as the maximum VL */
+	ret = file_write_integer(data->default_vl_file, -1);
+	if (ret != 0)
+		return;
+
+	/* What was the new value? */
+	ret = file_read_integer(data->default_vl_file, &new_default);
+	if (ret != 0)
+		return;
+
+	/* Did it take effect in a new process? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != new_default) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      new_default, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s maximum vector length %d\n", data->name,
+			      new_default);
+	data->max_vl = new_default;
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* Can we read back a VL from prctl? */
+void prctl_get(struct vec_data *data)
+{
+	int ret;
+
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	/* Mask out any flags */
+	ret &= PR_SVE_VL_LEN_MASK;
+
+	/* Is that what we can read back directly? */
+	if (ret == data->rdvl())
+		ksft_test_result_pass("%s current VL is %d\n",
+				      data->name, ret);
+	else
+		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
+				      data->name, ret, data->rdvl());
+}
+
+/* Does the prctl let us set the VL we already have? */
+void prctl_set_same(struct vec_data *data)
+{
+	int cur_vl = data->rdvl();
+	int ret;
+
+	ret = prctl(data->prctl_set, cur_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	if (cur_vl != data->rdvl())
+		ksft_test_result_pass("%s current VL is %d\n",
+				      data->name, ret);
+	else
+		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
+				      data->name, ret, data->rdvl());
+}
+
+/* Can we set a new VL for this process? */
+void prctl_set(struct vec_data *data)
+{
+	int ret;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	/* Try to set the minimum VL */
+	ret = prctl(data->prctl_set, data->min_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	if ((ret & PR_SVE_VL_LEN_MASK) != data->min_vl) {
+		ksft_test_result_fail("%s prctl set %d but return value is %d\n",
+				      data->name, data->min_vl, data->rdvl());
+		return;
+	}
+
+	if (data->rdvl() != data->min_vl) {
+		ksft_test_result_fail("%s set %d but RDVL is %d\n",
+				      data->name, data->min_vl, data->rdvl());
+		return;
+	}
+
+	/* Try to set the maximum VL */
+	ret = prctl(data->prctl_set, data->max_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->max_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	if ((ret & PR_SVE_VL_LEN_MASK) != data->max_vl) {
+		ksft_test_result_fail("%s prctl() set %d but return value is %d\n",
+				      data->name, data->max_vl, data->rdvl());
+		return;
+	}
+
+	/* The _INHERIT flag should not be present when we read the VL */
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	if (ret & PR_SVE_VL_INHERIT) {
+		ksft_test_result_fail("%s prctl() reports _INHERIT\n",
+				      data->name);
+		return;
+	}
+
+	ksft_test_result_pass("%s prctl() set min/max\n", data->name);
+}
+
+/* If we didn't request it a new VL shouldn't affect the child */
+void prctl_set_no_child(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	ret = prctl(data->prctl_set, data->min_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Ensure the default VL is different */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	/* Check that the child has the default we just set */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->max_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->max_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length used default\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* If we didn't request it a new VL shouldn't affect the child */
+void prctl_set_for_child(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_VL_INHERIT);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* The _INHERIT flag should be present when we read the VL */
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+	if (!(ret & PR_SVE_VL_INHERIT)) {
+		ksft_test_result_fail("%s prctl() does not report _INHERIT\n",
+				      data->name);
+		return;
+	}
+
+	/* Ensure the default VL is different */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	/* Check that the child inherited our VL */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->min_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->min_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length was inherited\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* _ONEXEC takes effect only in the child process */
+void prctl_set_onexec(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	/* Set a known value for the default and our current VL */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	ret = prctl(data->prctl_set, data->max_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Set a different value for the child to have on exec */
+	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_SET_VL_ONEXEC);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Our current VL should stay the same */
+	if (data->rdvl() != data->max_vl) {
+		ksft_test_result_fail("%s VL changed by _ONEXEC prctl()\n",
+				      data->name);
+		return;
+	}
+
+	/* Check that the child inherited our VL */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->min_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->min_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length set on exec\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+typedef void (*test_type)(struct vec_data *);
+
+test_type tests[] = {
+	/*
+	 * The default/min/max tests must be first and in this order
+	 * to provide data for other tests.
+	 */
+	proc_read_default,
+	proc_write_min,
+	proc_write_max,
+
+	prctl_get,
+	prctl_set,
+	prctl_set_no_child,
+	prctl_set_for_child,
+	prctl_set_onexec,
+};
+
+int main(void)
+{
+	int i, j;
+
+	ksft_print_header();
+	ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data));
+
+	for (i = 0; i < ARRAY_SIZE(vec_data); i++) {
+		struct vec_data *data = &vec_data[i];
+		int supported = getauxval(data->hwcap_type) & data->hwcap;
+
+		for (j = 0; j < ARRAY_SIZE(tests); j++) {
+			if (supported)
+				tests[j](data);
+			else
+				ksft_test_result_skip("%s not supported\n",
+						      data->name);
+		}
+	}
+
+	ksft_exit_pass();
+}
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 4/4] kselftest/arm64: Add a TODO list for floating point tests
  2021-07-28 16:33 ` Mark Brown
@ 2021-07-28 16:33   ` Mark Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

Write down some ideas for additional coverage for floating point in case
someone feels inspired to look into them.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/TODO | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/fp/TODO

diff --git a/tools/testing/selftests/arm64/fp/TODO b/tools/testing/selftests/arm64/fp/TODO
new file mode 100644
index 000000000000..eada915227cf
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/TODO
@@ -0,0 +1,3 @@
+- Test unsupported values in the ABIs
+- More coverage for ptrace (eg, vector length conversions).
+- Coverage for signals.
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 4/4] kselftest/arm64: Add a TODO list for floating point tests
@ 2021-07-28 16:33   ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-28 16:33 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Shuah Khan
  Cc: Dave Martin, linux-arm-kernel, linux-kselftest, Mark Brown

Write down some ideas for additional coverage for floating point in case
someone feels inspired to look into them.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/TODO | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/fp/TODO

diff --git a/tools/testing/selftests/arm64/fp/TODO b/tools/testing/selftests/arm64/fp/TODO
new file mode 100644
index 000000000000..eada915227cf
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/TODO
@@ -0,0 +1,3 @@
+- Test unsupported values in the ABIs
+- More coverage for ptrace (eg, vector length conversions).
+- Coverage for signals.
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  2021-07-28 16:33   ` Mark Brown
@ 2021-07-29  9:52     ` Dave Martin
  -1 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:15PM +0100, Mark Brown wrote:
> SVE provides an instruction RDVL which reports the currently configured
> vector length. In order to validate that our vector length configuration
> interfaces are working correctly without having to build the C code for
> our test programs with SVE enabled provide a trivial assembly library
> with a C callable function that executes RDVL. Since these interfaces
> also control behaviour on exec*() provide a trivial wrapper program which
> reports the currently configured vector length on stdout, tests can use
> this to verify that behaviour on exec*() is as expected.
> 
> In preparation for providing similar helper functionality for SME, the
> Scalable Matrix Extension, which allows separately configured vector
> lengths to be read back both the assembler function and wrapper binary
> have SVE included in their name.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

(With or without a couple of trivial nits below.)

> ---
>  tools/testing/selftests/arm64/fp/.gitignore |  1 +
>  tools/testing/selftests/arm64/fp/Makefile   |  6 +++++-
>  tools/testing/selftests/arm64/fp/rdvl-sve.c | 14 ++++++++++++++
>  tools/testing/selftests/arm64/fp/rdvl.S     |  9 +++++++++
>  tools/testing/selftests/arm64/fp/rdvl.h     |  8 ++++++++
>  5 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h
> 
> diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
> index d66f76d2a650..6b53a7b60fee 100644
> --- a/tools/testing/selftests/arm64/fp/.gitignore
> +++ b/tools/testing/selftests/arm64/fp/.gitignore
> @@ -1,4 +1,5 @@
>  fpsimd-test
> +rdvl-sve
>  sve-probe-vls
>  sve-ptrace
>  sve-test
> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
> index a57009d3a0dc..ed62e7003b96 100644
> --- a/tools/testing/selftests/arm64/fp/Makefile
> +++ b/tools/testing/selftests/arm64/fp/Makefile
> @@ -2,12 +2,16 @@
>  
>  CFLAGS += -I../../../../../usr/include/
>  TEST_GEN_PROGS := sve-ptrace sve-probe-vls
> -TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress sve-test sve-stress vlset
> +TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
> +	rdvl-sve \
> +	sve-test sve-stress \
> +	vlset
>  
>  all: $(TEST_GEN_PROGS) $(TEST_PROGS_EXTENDED)
>  
>  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-test: sve-test.o
> diff --git a/tools/testing/selftests/arm64/fp/rdvl-sve.c b/tools/testing/selftests/arm64/fp/rdvl-sve.c
> new file mode 100644
> index 000000000000..7f8a13a18f5d
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl-sve.c
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <stdio.h>
> +
> +#include "rdvl.h"
> +
> +int main(void)
> +{
> +	int vl = rdvl_sve();
> +
> +	printf("%d\n", vl);
> +
> +	return 0;

For consistency with the changes in vec-syscfg, we could use
EXIT_SUCCESS here.

Although it's hard to see what could go wrong I/O-wise that doesn't
involve vec-syscfg itself having gone wrong, it's probably good
practice to do the final error check:

	if (ferror(stdout) || fclose(stdout))
		return EXIT_FAILURE;

	return EXIT_SUCCESS;

(In reality, people rarely seem to bother with this, so I'm not going
to lose sleep if we don't do it...)

> +}
> diff --git a/tools/testing/selftests/arm64/fp/rdvl.S b/tools/testing/selftests/arm64/fp/rdvl.S
> new file mode 100644
> index 000000000000..6e76dd720b87
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl.S
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (C) 2021 ARM Limited.
> +
> +.arch_extension sve
> +
> +.globl rdvl_sve

Should we stick a

	.type rdvl_sve, @function

here?  This may avoid surprises with future toolchain behaviours.
Probably doesn't matter, but I have bad memories of Thumb-2...

Lacking this annotation is widespread though, as well as being de facto
standard before awkward architectures came along.

If the selftests have access to the ENTRY() macro we could use that, but
I'm guessing that isn't exported for userspace.

> +rdvl_sve:
> +	rdvl	x0, #1
> +	ret
> diff --git a/tools/testing/selftests/arm64/fp/rdvl.h b/tools/testing/selftests/arm64/fp/rdvl.h
> new file mode 100644
> index 000000000000..7c9d953fc9e7
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef RDVL_H
> +#define RDVL_H
> +
> +int rdvl_sve(void);
> +
> +#endif
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
@ 2021-07-29  9:52     ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:15PM +0100, Mark Brown wrote:
> SVE provides an instruction RDVL which reports the currently configured
> vector length. In order to validate that our vector length configuration
> interfaces are working correctly without having to build the C code for
> our test programs with SVE enabled provide a trivial assembly library
> with a C callable function that executes RDVL. Since these interfaces
> also control behaviour on exec*() provide a trivial wrapper program which
> reports the currently configured vector length on stdout, tests can use
> this to verify that behaviour on exec*() is as expected.
> 
> In preparation for providing similar helper functionality for SME, the
> Scalable Matrix Extension, which allows separately configured vector
> lengths to be read back both the assembler function and wrapper binary
> have SVE included in their name.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

(With or without a couple of trivial nits below.)

> ---
>  tools/testing/selftests/arm64/fp/.gitignore |  1 +
>  tools/testing/selftests/arm64/fp/Makefile   |  6 +++++-
>  tools/testing/selftests/arm64/fp/rdvl-sve.c | 14 ++++++++++++++
>  tools/testing/selftests/arm64/fp/rdvl.S     |  9 +++++++++
>  tools/testing/selftests/arm64/fp/rdvl.h     |  8 ++++++++
>  5 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h
> 
> diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
> index d66f76d2a650..6b53a7b60fee 100644
> --- a/tools/testing/selftests/arm64/fp/.gitignore
> +++ b/tools/testing/selftests/arm64/fp/.gitignore
> @@ -1,4 +1,5 @@
>  fpsimd-test
> +rdvl-sve
>  sve-probe-vls
>  sve-ptrace
>  sve-test
> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
> index a57009d3a0dc..ed62e7003b96 100644
> --- a/tools/testing/selftests/arm64/fp/Makefile
> +++ b/tools/testing/selftests/arm64/fp/Makefile
> @@ -2,12 +2,16 @@
>  
>  CFLAGS += -I../../../../../usr/include/
>  TEST_GEN_PROGS := sve-ptrace sve-probe-vls
> -TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress sve-test sve-stress vlset
> +TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
> +	rdvl-sve \
> +	sve-test sve-stress \
> +	vlset
>  
>  all: $(TEST_GEN_PROGS) $(TEST_PROGS_EXTENDED)
>  
>  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-test: sve-test.o
> diff --git a/tools/testing/selftests/arm64/fp/rdvl-sve.c b/tools/testing/selftests/arm64/fp/rdvl-sve.c
> new file mode 100644
> index 000000000000..7f8a13a18f5d
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl-sve.c
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <stdio.h>
> +
> +#include "rdvl.h"
> +
> +int main(void)
> +{
> +	int vl = rdvl_sve();
> +
> +	printf("%d\n", vl);
> +
> +	return 0;

For consistency with the changes in vec-syscfg, we could use
EXIT_SUCCESS here.

Although it's hard to see what could go wrong I/O-wise that doesn't
involve vec-syscfg itself having gone wrong, it's probably good
practice to do the final error check:

	if (ferror(stdout) || fclose(stdout))
		return EXIT_FAILURE;

	return EXIT_SUCCESS;

(In reality, people rarely seem to bother with this, so I'm not going
to lose sleep if we don't do it...)

> +}
> diff --git a/tools/testing/selftests/arm64/fp/rdvl.S b/tools/testing/selftests/arm64/fp/rdvl.S
> new file mode 100644
> index 000000000000..6e76dd720b87
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl.S
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (C) 2021 ARM Limited.
> +
> +.arch_extension sve
> +
> +.globl rdvl_sve

Should we stick a

	.type rdvl_sve, @function

here?  This may avoid surprises with future toolchain behaviours.
Probably doesn't matter, but I have bad memories of Thumb-2...

Lacking this annotation is widespread though, as well as being de facto
standard before awkward architectures came along.

If the selftests have access to the ENTRY() macro we could use that, but
I'm guessing that isn't exported for userspace.

> +rdvl_sve:
> +	rdvl	x0, #1
> +	ret
> diff --git a/tools/testing/selftests/arm64/fp/rdvl.h b/tools/testing/selftests/arm64/fp/rdvl.h
> new file mode 100644
> index 000000000000..7c9d953fc9e7
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef RDVL_H
> +#define RDVL_H
> +
> +int rdvl_sve(void);
> +
> +#endif
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/4] kselftest/arm64: Validate vector lengths are set in sve-probe-vls
  2021-07-28 16:33   ` Mark Brown
@ 2021-07-29  9:52     ` Dave Martin
  -1 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:16PM +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>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  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..a24eca7a4ecb 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("PR_SVE_SET_VL reports %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);
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/4] kselftest/arm64: Validate vector lengths are set in sve-probe-vls
@ 2021-07-29  9:52     ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:16PM +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>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  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..a24eca7a4ecb 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("PR_SVE_SET_VL reports %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);
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 3/4] kselftest/arm64: Add tests for SVE vector configuration
  2021-07-28 16:33   ` Mark Brown
@ 2021-07-29  9:52     ` Dave Martin
  -1 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:17PM +0100, Mark Brown wrote:
> We provide interfaces for configuring the SVE vector length seen by
> processes using prctl and also via /proc for configuring the default
> values. Provide tests that exercise all these interfaces and verify that
> they take effect as expected, though at present no test fully enumerates
> all the possible vector lengths.
> 
> A subset of this is already tested via sve-probe-vls but the /proc
> interfaces are not currently covered at all.
> 
> In preparation for the forthcoming support for SME, the Scalable Matrix
> Extension, which has separately but similarly configured vector lengths
> which we expect to offer similar userspace interfaces for, all the actual
> files and prctls used are parameterised and we don't validate that the
> architectural minimum vector length is the minimum we see.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  tools/testing/selftests/arm64/fp/.gitignore   |   1 +
>  tools/testing/selftests/arm64/fp/Makefile     |   3 +-
>  tools/testing/selftests/arm64/fp/vec-syscfg.c | 580 ++++++++++++++++++
>  3 files changed, 583 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c
> 
> diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
> index 6b53a7b60fee..b67395903b9b 100644
> --- a/tools/testing/selftests/arm64/fp/.gitignore
> +++ b/tools/testing/selftests/arm64/fp/.gitignore
> @@ -3,4 +3,5 @@ rdvl-sve
>  sve-probe-vls
>  sve-ptrace
>  sve-test
> +vec-syscfg
>  vlset
> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
> index fa3a0167db2d..f2abdd6ba12e 100644
> --- a/tools/testing/selftests/arm64/fp/Makefile
> +++ b/tools/testing/selftests/arm64/fp/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  CFLAGS += -I../../../../../usr/include/
> -TEST_GEN_PROGS := sve-ptrace sve-probe-vls
> +TEST_GEN_PROGS := sve-ptrace sve-probe-vls vec-syscfg
>  TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
>  	rdvl-sve \
>  	sve-test sve-stress \
> @@ -16,6 +16,7 @@ sve-ptrace: sve-ptrace.o sve-ptrace-asm.o
>  sve-probe-vls: sve-probe-vls.o rdvl.o
>  sve-test: sve-test.o
>  	$(CC) -nostdlib $^ -o $@
> +vec-syscfg: vec-syscfg.o rdvl.o
>  vlset: vlset.o
>  
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
> new file mode 100644
> index 000000000000..63d080537d21
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
> @@ -0,0 +1,580 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 ARM Limited.
> + * Original author: Mark Brown <broonie@kernel.org>
> + */
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/auxv.h>
> +#include <sys/prctl.h>
> +#include <sys/ptrace.h>

Nit: ^ also still not used? (I think you caught <asm/ptrace.h> already.)

(Hmm, there must be a tool for weeding #includes somewhere...)

> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <asm/sigcontext.h>
> +#include <asm/hwcap.h>
> +
> +#include "../../kselftest.h"
> +#include "rdvl.h"
> +
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> +
> +#define ARCH_MIN_VL SVE_VL_MIN
> +
> +struct vec_data {
> +	const char *name;
> +	unsigned long hwcap_type;
> +	unsigned long hwcap;
> +	const char *rdvl_binary;
> +	int (*rdvl)(void);
> +
> +	int prctl_get;
> +	int prctl_set;
> +	const char *default_vl_file;
> +
> +	int default_vl;
> +	int min_vl;
> +	int max_vl;
> +};
> +
> +
> +static struct vec_data vec_data[] = {
> +	{
> +		.name = "SVE",
> +		.hwcap_type = AT_HWCAP,
> +		.hwcap = HWCAP_SVE,
> +		.rdvl = rdvl_sve,
> +		.rdvl_binary = "./rdvl-sve",
> +		.prctl_get = PR_SVE_GET_VL,
> +		.prctl_set = PR_SVE_SET_VL,
> +		.default_vl_file = "/proc/sys/abi/sve_default_vector_length",
> +	},
> +};
> +
> +/* Start a new process and return the vector length it sees */
> +int get_child_rdvl(struct vec_data *data)
> +{
> +	FILE *out;
> +	int pipefd[2];
> +	pid_t pid, child;
> +	int read_vl, ret;
> +
> +	ret = pipe(pipefd);
> +	if (ret == -1) {
> +		ksft_print_msg("pipe() failed: %d (%s)\n",
> +			       errno, strerror(errno));
> +		return -1;
> +	}
> +
> +	fflush(stdout);
> +
> +	child = fork();
> +	if (child == -1) {
> +		ksft_print_msg("fork() failed: %d (%s)\n",
> +			       errno, strerror(errno));
> +		return -1;

Leaks pipefd[0], pipefd[1]?

> +	}
> +
> +	/* Child: put vector length on the pipe */
> +	if (child == 0) {
> +		/*
> +		 * Replace stdout with the pipe, errors to stderr from
> +		 * here as kselftest prints to stdout.
> +		 */
> +		ret = dup2(pipefd[1], 1);
> +		if (ret == -1) {
> +			fprintf(stderr, "dup2() %d\n", errno);
> +			exit(EXIT_FAILURE);
> +		}
> +
> +		/* exec() a new binary which puts the VL on stdout */
> +		ret = execl(data->rdvl_binary, data->rdvl_binary, NULL);
> +		fprintf(stderr, "execl(%s) failed: %d\n",
> +			data->rdvl_binary, errno, strerror(errno));
> +
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	close(pipefd[1]);
> +
> +	/* Parent; wait for the exit status from the child & verify it */
> +	while (1) {
> +		pid = wait(&ret);
> +		if (pid == -1) {
> +			ksft_print_msg("wait() failed: %d (%s)\n",
> +				       errno, strerror(errno));
> +			return -1;

Leaks pipefd[0]?

> +		}
> +
> +		if (pid != child)
> +			continue;
> +
> +		if (!WIFEXITED(ret)) {
> +			ksft_print_msg("child exited abnormally\n");
> +			return -1;

Same from here up to the fclose().


Although this looks like it might have been based on code I wrote
previously(?), I guess it might be better to move all the
post-child-exit code out of the loop, since it really only executes
once, and any mishap is treated as fatal anyway.  It looks kind of
weird to have close() in the loop (even if hidden now in fclose())
with pipe() outside.

There's only one child anyway, and we treat !WIFEXITED() as an error
(without WUNTRACED or similar and since we are not a tracer,
WIFSIGNALED is the only other possbility IIUC.)

> +		}
> +
> +		if (WEXITSTATUS(ret) != 0) {
> +			ksft_print_msg("child returned error %d\n",
> +				       WEXITSTATUS(ret));
> +			return -1;
> +		}
> +
> +		out = fdopen(pipefd[0], "r");
> +		if (!out) {
> +			ksft_print_msg("failed to open child stdout\n");
> +			return -1;
> +		}
> +
> +		ret = fscanf(out, "%d", &read_vl);

Is it still worth slurping the newline here?  That's the only way to
tell the difference between complete line and truncation of the stream.

The "%d%*1[\n]*n" trick from my previous reply would probably work for
that.  Or maybe just checking for the '\n' with an fgetc() after the
fscanf() would work.  I don't remember offhand whether scanf is allowed
to silently consume trailing whitespace after a conversion, which would
be a problem for the fgetc() approach.  Maybe not though.

OTOH this is a pipe, and with my suggestion on the sve-rdvl patch,
the child should report nonzero exit status if something goes wrong on
its end.  So there's not a lot that can actually go wrong that we
wouldn't detect somehow.

Either way, this still looks a lot like file_read_integer() and the
error detection discussion applies to both.

Could the code be factored out as

int ffile_read_integer(int *val, FILE *f) {
	...
}

int file_read_integer(const char *name, int val) {
	fopen() ... ffile_read_integer() ... fclose()
}

> +		fclose(out);
> +		if (ret != 1) {
> +			ksft_print_msg("failed to parse VL from '%s'\n",
> +				       data->rdvl_binary);
> +			return -1;
> +		}
> +
> +		return read_vl;
> +	}
> +}
> +
> +int file_read_integer(const char *name, int *val)
> +{
> +	FILE *f;
> +	int ret;
> +
> +	f = fopen(name, "r");
> +	if (!f) {
> +		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
> +				      name, errno,
> +				      strerror(errno));
> +		return -1;
> +	}
> +
> +	ret = fscanf(f, "%d", val);
> +	fclose(f);
> +	if (ret != 1) {
> +		ksft_test_result_fail("Failed to parse %s\n", name);
> +		return -1;
> +	}
> +
> +	return 0;
> +}

[...]

> +typedef void (*test_type)(struct vec_data *);
> +
> +test_type tests[] = {
> +	/*
> +	 * The default/min/max tests must be first and in this order
> +	 * to provide data for other tests.
> +	 */
> +	proc_read_default,
> +	proc_write_min,
> +	proc_write_max,
> +
> +	prctl_get,
> +	prctl_set,
> +	prctl_set_no_child,
> +	prctl_set_for_child,
> +	prctl_set_onexec,
> +};

This can be static const too come to think of it, and every function
except main() can be static...

[...]

> +int main(void)
> +{
> +	int i, j;
> +
> +	ksft_print_header();
> +	ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data));
> +
> +	for (i = 0; i < ARRAY_SIZE(vec_data); i++) {
> +		struct vec_data *data = &vec_data[i];
> +		int supported = getauxval(data->hwcap_type) & data->hwcap;
> +
> +		for (j = 0; j < ARRAY_SIZE(tests); j++) {
> +			if (supported)

Did we commit to never defining hwcaps beyond bit 31?  It sort of looks
this way from arch/arm64/include/uapi/asm/hwcap.h, but I can't remember
the reason.  I'd say something to do with compat, only of course that
has its own hwcaps.

Since getauxval() returns an unsigned long, would it be a good idea to
make this:

		int supported = (getauxval(data->hwcap_type) & data->hwcap) != 0;

or

		unsigned long supported_hwcaps = getauxval(data->hwcap_type);

		/* ... */

			if (supported_hwcaps & data->hwcap)

just to avoid future surprises?

[...]

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 3/4] kselftest/arm64: Add tests for SVE vector configuration
@ 2021-07-29  9:52     ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:17PM +0100, Mark Brown wrote:
> We provide interfaces for configuring the SVE vector length seen by
> processes using prctl and also via /proc for configuring the default
> values. Provide tests that exercise all these interfaces and verify that
> they take effect as expected, though at present no test fully enumerates
> all the possible vector lengths.
> 
> A subset of this is already tested via sve-probe-vls but the /proc
> interfaces are not currently covered at all.
> 
> In preparation for the forthcoming support for SME, the Scalable Matrix
> Extension, which has separately but similarly configured vector lengths
> which we expect to offer similar userspace interfaces for, all the actual
> files and prctls used are parameterised and we don't validate that the
> architectural minimum vector length is the minimum we see.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  tools/testing/selftests/arm64/fp/.gitignore   |   1 +
>  tools/testing/selftests/arm64/fp/Makefile     |   3 +-
>  tools/testing/selftests/arm64/fp/vec-syscfg.c | 580 ++++++++++++++++++
>  3 files changed, 583 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c
> 
> diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
> index 6b53a7b60fee..b67395903b9b 100644
> --- a/tools/testing/selftests/arm64/fp/.gitignore
> +++ b/tools/testing/selftests/arm64/fp/.gitignore
> @@ -3,4 +3,5 @@ rdvl-sve
>  sve-probe-vls
>  sve-ptrace
>  sve-test
> +vec-syscfg
>  vlset
> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
> index fa3a0167db2d..f2abdd6ba12e 100644
> --- a/tools/testing/selftests/arm64/fp/Makefile
> +++ b/tools/testing/selftests/arm64/fp/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  CFLAGS += -I../../../../../usr/include/
> -TEST_GEN_PROGS := sve-ptrace sve-probe-vls
> +TEST_GEN_PROGS := sve-ptrace sve-probe-vls vec-syscfg
>  TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
>  	rdvl-sve \
>  	sve-test sve-stress \
> @@ -16,6 +16,7 @@ sve-ptrace: sve-ptrace.o sve-ptrace-asm.o
>  sve-probe-vls: sve-probe-vls.o rdvl.o
>  sve-test: sve-test.o
>  	$(CC) -nostdlib $^ -o $@
> +vec-syscfg: vec-syscfg.o rdvl.o
>  vlset: vlset.o
>  
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
> new file mode 100644
> index 000000000000..63d080537d21
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
> @@ -0,0 +1,580 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 ARM Limited.
> + * Original author: Mark Brown <broonie@kernel.org>
> + */
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/auxv.h>
> +#include <sys/prctl.h>
> +#include <sys/ptrace.h>

Nit: ^ also still not used? (I think you caught <asm/ptrace.h> already.)

(Hmm, there must be a tool for weeding #includes somewhere...)

> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <asm/sigcontext.h>
> +#include <asm/hwcap.h>
> +
> +#include "../../kselftest.h"
> +#include "rdvl.h"
> +
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> +
> +#define ARCH_MIN_VL SVE_VL_MIN
> +
> +struct vec_data {
> +	const char *name;
> +	unsigned long hwcap_type;
> +	unsigned long hwcap;
> +	const char *rdvl_binary;
> +	int (*rdvl)(void);
> +
> +	int prctl_get;
> +	int prctl_set;
> +	const char *default_vl_file;
> +
> +	int default_vl;
> +	int min_vl;
> +	int max_vl;
> +};
> +
> +
> +static struct vec_data vec_data[] = {
> +	{
> +		.name = "SVE",
> +		.hwcap_type = AT_HWCAP,
> +		.hwcap = HWCAP_SVE,
> +		.rdvl = rdvl_sve,
> +		.rdvl_binary = "./rdvl-sve",
> +		.prctl_get = PR_SVE_GET_VL,
> +		.prctl_set = PR_SVE_SET_VL,
> +		.default_vl_file = "/proc/sys/abi/sve_default_vector_length",
> +	},
> +};
> +
> +/* Start a new process and return the vector length it sees */
> +int get_child_rdvl(struct vec_data *data)
> +{
> +	FILE *out;
> +	int pipefd[2];
> +	pid_t pid, child;
> +	int read_vl, ret;
> +
> +	ret = pipe(pipefd);
> +	if (ret == -1) {
> +		ksft_print_msg("pipe() failed: %d (%s)\n",
> +			       errno, strerror(errno));
> +		return -1;
> +	}
> +
> +	fflush(stdout);
> +
> +	child = fork();
> +	if (child == -1) {
> +		ksft_print_msg("fork() failed: %d (%s)\n",
> +			       errno, strerror(errno));
> +		return -1;

Leaks pipefd[0], pipefd[1]?

> +	}
> +
> +	/* Child: put vector length on the pipe */
> +	if (child == 0) {
> +		/*
> +		 * Replace stdout with the pipe, errors to stderr from
> +		 * here as kselftest prints to stdout.
> +		 */
> +		ret = dup2(pipefd[1], 1);
> +		if (ret == -1) {
> +			fprintf(stderr, "dup2() %d\n", errno);
> +			exit(EXIT_FAILURE);
> +		}
> +
> +		/* exec() a new binary which puts the VL on stdout */
> +		ret = execl(data->rdvl_binary, data->rdvl_binary, NULL);
> +		fprintf(stderr, "execl(%s) failed: %d\n",
> +			data->rdvl_binary, errno, strerror(errno));
> +
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	close(pipefd[1]);
> +
> +	/* Parent; wait for the exit status from the child & verify it */
> +	while (1) {
> +		pid = wait(&ret);
> +		if (pid == -1) {
> +			ksft_print_msg("wait() failed: %d (%s)\n",
> +				       errno, strerror(errno));
> +			return -1;

Leaks pipefd[0]?

> +		}
> +
> +		if (pid != child)
> +			continue;
> +
> +		if (!WIFEXITED(ret)) {
> +			ksft_print_msg("child exited abnormally\n");
> +			return -1;

Same from here up to the fclose().


Although this looks like it might have been based on code I wrote
previously(?), I guess it might be better to move all the
post-child-exit code out of the loop, since it really only executes
once, and any mishap is treated as fatal anyway.  It looks kind of
weird to have close() in the loop (even if hidden now in fclose())
with pipe() outside.

There's only one child anyway, and we treat !WIFEXITED() as an error
(without WUNTRACED or similar and since we are not a tracer,
WIFSIGNALED is the only other possbility IIUC.)

> +		}
> +
> +		if (WEXITSTATUS(ret) != 0) {
> +			ksft_print_msg("child returned error %d\n",
> +				       WEXITSTATUS(ret));
> +			return -1;
> +		}
> +
> +		out = fdopen(pipefd[0], "r");
> +		if (!out) {
> +			ksft_print_msg("failed to open child stdout\n");
> +			return -1;
> +		}
> +
> +		ret = fscanf(out, "%d", &read_vl);

Is it still worth slurping the newline here?  That's the only way to
tell the difference between complete line and truncation of the stream.

The "%d%*1[\n]*n" trick from my previous reply would probably work for
that.  Or maybe just checking for the '\n' with an fgetc() after the
fscanf() would work.  I don't remember offhand whether scanf is allowed
to silently consume trailing whitespace after a conversion, which would
be a problem for the fgetc() approach.  Maybe not though.

OTOH this is a pipe, and with my suggestion on the sve-rdvl patch,
the child should report nonzero exit status if something goes wrong on
its end.  So there's not a lot that can actually go wrong that we
wouldn't detect somehow.

Either way, this still looks a lot like file_read_integer() and the
error detection discussion applies to both.

Could the code be factored out as

int ffile_read_integer(int *val, FILE *f) {
	...
}

int file_read_integer(const char *name, int val) {
	fopen() ... ffile_read_integer() ... fclose()
}

> +		fclose(out);
> +		if (ret != 1) {
> +			ksft_print_msg("failed to parse VL from '%s'\n",
> +				       data->rdvl_binary);
> +			return -1;
> +		}
> +
> +		return read_vl;
> +	}
> +}
> +
> +int file_read_integer(const char *name, int *val)
> +{
> +	FILE *f;
> +	int ret;
> +
> +	f = fopen(name, "r");
> +	if (!f) {
> +		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
> +				      name, errno,
> +				      strerror(errno));
> +		return -1;
> +	}
> +
> +	ret = fscanf(f, "%d", val);
> +	fclose(f);
> +	if (ret != 1) {
> +		ksft_test_result_fail("Failed to parse %s\n", name);
> +		return -1;
> +	}
> +
> +	return 0;
> +}

[...]

> +typedef void (*test_type)(struct vec_data *);
> +
> +test_type tests[] = {
> +	/*
> +	 * The default/min/max tests must be first and in this order
> +	 * to provide data for other tests.
> +	 */
> +	proc_read_default,
> +	proc_write_min,
> +	proc_write_max,
> +
> +	prctl_get,
> +	prctl_set,
> +	prctl_set_no_child,
> +	prctl_set_for_child,
> +	prctl_set_onexec,
> +};

This can be static const too come to think of it, and every function
except main() can be static...

[...]

> +int main(void)
> +{
> +	int i, j;
> +
> +	ksft_print_header();
> +	ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data));
> +
> +	for (i = 0; i < ARRAY_SIZE(vec_data); i++) {
> +		struct vec_data *data = &vec_data[i];
> +		int supported = getauxval(data->hwcap_type) & data->hwcap;
> +
> +		for (j = 0; j < ARRAY_SIZE(tests); j++) {
> +			if (supported)

Did we commit to never defining hwcaps beyond bit 31?  It sort of looks
this way from arch/arm64/include/uapi/asm/hwcap.h, but I can't remember
the reason.  I'd say something to do with compat, only of course that
has its own hwcaps.

Since getauxval() returns an unsigned long, would it be a good idea to
make this:

		int supported = (getauxval(data->hwcap_type) & data->hwcap) != 0;

or

		unsigned long supported_hwcaps = getauxval(data->hwcap_type);

		/* ... */

			if (supported_hwcaps & data->hwcap)

just to avoid future surprises?

[...]

Cheers
---Dave

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 4/4] kselftest/arm64: Add a TODO list for floating point tests
  2021-07-28 16:33   ` Mark Brown
@ 2021-07-29  9:52     ` Dave Martin
  -1 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:18PM +0100, Mark Brown wrote:
> Write down some ideas for additional coverage for floating point in case
> someone feels inspired to look into them.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Doesn't really matter exactly what we have in here at this point, but
it's good to have somewhere to collect this stuff.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  tools/testing/selftests/arm64/fp/TODO | 3 +++
>  1 file changed, 3 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/fp/TODO
> 
> diff --git a/tools/testing/selftests/arm64/fp/TODO b/tools/testing/selftests/arm64/fp/TODO
> new file mode 100644
> index 000000000000..eada915227cf
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/TODO
> @@ -0,0 +1,3 @@
> +- Test unsupported values in the ABIs
> +- More coverage for ptrace (eg, vector length conversions).
> +- Coverage for signals.
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 4/4] kselftest/arm64: Add a TODO list for floating point tests
@ 2021-07-29  9:52     ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29  9:52 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Wed, Jul 28, 2021 at 05:33:18PM +0100, Mark Brown wrote:
> Write down some ideas for additional coverage for floating point in case
> someone feels inspired to look into them.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Doesn't really matter exactly what we have in here at this point, but
it's good to have somewhere to collect this stuff.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  tools/testing/selftests/arm64/fp/TODO | 3 +++
>  1 file changed, 3 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/fp/TODO
> 
> diff --git a/tools/testing/selftests/arm64/fp/TODO b/tools/testing/selftests/arm64/fp/TODO
> new file mode 100644
> index 000000000000..eada915227cf
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/TODO
> @@ -0,0 +1,3 @@
> +- Test unsupported values in the ABIs
> +- More coverage for ptrace (eg, vector length conversions).
> +- Coverage for signals.
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  2021-07-29  9:52     ` Dave Martin
@ 2021-07-29 11:22       ` Mark Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-29 11:22 UTC (permalink / raw)
  To: Dave Martin
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

On Thu, Jul 29, 2021 at 10:52:24AM +0100, Dave Martin wrote:
> On Wed, Jul 28, 2021 at 05:33:15PM +0100, Mark Brown wrote:

> > +	return 0;

> For consistency with the changes in vec-syscfg, we could use
> EXIT_SUCCESS here.

0 and EXIT_SUCCESS are defined as equivalent (though they need not be
equal!) and 0 is much more idiomatic.

> Although it's hard to see what could go wrong I/O-wise that doesn't
> involve vec-syscfg itself having gone wrong, it's probably good
> practice to do the final error check:

> 	if (ferror(stdout) || fclose(stdout))
> 		return EXIT_FAILURE;

> 	return EXIT_SUCCESS;

> (In reality, people rarely seem to bother with this, so I'm not going
> to lose sleep if we don't do it...)

Yeah, I think this is one of those raising more questions than it
answers kind of things.

> > +.globl rdvl_sve

> Should we stick a

> 	.type rdvl_sve, @function

> here?  This may avoid surprises with future toolchain behaviours.
> Probably doesn't matter, but I have bad memories of Thumb-2...

> Lacking this annotation is widespread though, as well as being de facto
> standard before awkward architectures came along.

Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
asm code the kernel has.  I don't know if you think it's worth adding
that to SYM_FUNC_START now we have it though?

> If the selftests have access to the ENTRY() macro we could use that, but
> I'm guessing that isn't exported for userspace.

We don't use that any more anyway, it's SYM_FUNC_START() and friends not
that those are outside the kernel either.  We will have to do something
like that if anyone starts building userspace with BTI though (or I
might just shove a BTI C in there unconditionally, I'm sure we'll cope
with the overhead on older systems).

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
@ 2021-07-29 11:22       ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-29 11:22 UTC (permalink / raw)
  To: Dave Martin
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest


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

On Thu, Jul 29, 2021 at 10:52:24AM +0100, Dave Martin wrote:
> On Wed, Jul 28, 2021 at 05:33:15PM +0100, Mark Brown wrote:

> > +	return 0;

> For consistency with the changes in vec-syscfg, we could use
> EXIT_SUCCESS here.

0 and EXIT_SUCCESS are defined as equivalent (though they need not be
equal!) and 0 is much more idiomatic.

> Although it's hard to see what could go wrong I/O-wise that doesn't
> involve vec-syscfg itself having gone wrong, it's probably good
> practice to do the final error check:

> 	if (ferror(stdout) || fclose(stdout))
> 		return EXIT_FAILURE;

> 	return EXIT_SUCCESS;

> (In reality, people rarely seem to bother with this, so I'm not going
> to lose sleep if we don't do it...)

Yeah, I think this is one of those raising more questions than it
answers kind of things.

> > +.globl rdvl_sve

> Should we stick a

> 	.type rdvl_sve, @function

> here?  This may avoid surprises with future toolchain behaviours.
> Probably doesn't matter, but I have bad memories of Thumb-2...

> Lacking this annotation is widespread though, as well as being de facto
> standard before awkward architectures came along.

Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
asm code the kernel has.  I don't know if you think it's worth adding
that to SYM_FUNC_START now we have it though?

> If the selftests have access to the ENTRY() macro we could use that, but
> I'm guessing that isn't exported for userspace.

We don't use that any more anyway, it's SYM_FUNC_START() and friends not
that those are outside the kernel either.  We will have to do something
like that if anyone starts building userspace with BTI though (or I
might just shove a BTI C in there unconditionally, I'm sure we'll cope
with the overhead on older systems).

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  2021-07-29 11:22       ` Mark Brown
@ 2021-07-29 13:27         ` Dave P Martin
  -1 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2021-07-29 13:27 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Thu, Jul 29, 2021 at 12:22:17PM +0100, Mark Brown wrote:
> On Thu, Jul 29, 2021 at 10:52:24AM +0100, Dave Martin wrote:
> > On Wed, Jul 28, 2021 at 05:33:15PM +0100, Mark Brown wrote:
> 
> > > +	return 0;
> 
> > For consistency with the changes in vec-syscfg, we could use
> > EXIT_SUCCESS here.
> 
> 0 and EXIT_SUCCESS are defined as equivalent (though they need not be
> equal!) and 0 is much more idiomatic.

Well, it's a moot point, I guess.  It just seems a little inconsistent
now, but it's no big deal.

I would probably be equally happy if you called the child failure status
in vec-syscfg "1".

> 
> > Although it's hard to see what could go wrong I/O-wise that doesn't
> > involve vec-syscfg itself having gone wrong, it's probably good
> > practice to do the final error check:
> 
> > 	if (ferror(stdout) || fclose(stdout))
> > 		return EXIT_FAILURE;
> 
> > 	return EXIT_SUCCESS;
> 
> > (In reality, people rarely seem to bother with this, so I'm not going
> > to lose sleep if we don't do it...)
> 
> Yeah, I think this is one of those raising more questions than it
> answers kind of things.

Fair enough.  This is the kind of thing it may be better to do in the
library or framework, since even if people remember to do it, they are
unlikely to do consistent things...

> > > +.globl rdvl_sve
> 
> > Should we stick a
> 
> > 	.type rdvl_sve, @function
> 
> > here?  This may avoid surprises with future toolchain behaviours.
> > Probably doesn't matter, but I have bad memories of Thumb-2...
> 
> > Lacking this annotation is widespread though, as well as being de facto
> > standard before awkward architectures came along.
> 
> Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
> asm code the kernel has.  I don't know if you think it's worth adding
> that to SYM_FUNC_START now we have it though?

Actually, I think the core definition of SYM_FUNC_END() in
<linux/linkage.h> does this.

It would be good to pick up the common linkage macros; if we have to
sprinkle .type manually all over the tests people will likely make
mistakes, to that's probably not worth it.

If picking up the macros isn't trivial to do, I guess it's not that
important at this stage.

> 
> > If the selftests have access to the ENTRY() macro we could use that, but
> > I'm guessing that isn't exported for userspace.
> 
> We don't use that any more anyway, it's SYM_FUNC_START() and friends not

Yes, I was forgetting about the macros that replaced ENTRY().  It's a
while since I wrote much asm code in the kernel...

> that those are outside the kernel either.  We will have to do something
> like that if anyone starts building userspace with BTI though (or I
> might just shove a BTI C in there unconditionally, I'm sure we'll cope
> with the overhead on older systems).

I thought about that, but that .S file isn't annotated as supporting
BTI, so I guess there's no problem for now(?)

It would be harmless to add it, of course.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
@ 2021-07-29 13:27         ` Dave P Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2021-07-29 13:27 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Thu, Jul 29, 2021 at 12:22:17PM +0100, Mark Brown wrote:
> On Thu, Jul 29, 2021 at 10:52:24AM +0100, Dave Martin wrote:
> > On Wed, Jul 28, 2021 at 05:33:15PM +0100, Mark Brown wrote:
> 
> > > +	return 0;
> 
> > For consistency with the changes in vec-syscfg, we could use
> > EXIT_SUCCESS here.
> 
> 0 and EXIT_SUCCESS are defined as equivalent (though they need not be
> equal!) and 0 is much more idiomatic.

Well, it's a moot point, I guess.  It just seems a little inconsistent
now, but it's no big deal.

I would probably be equally happy if you called the child failure status
in vec-syscfg "1".

> 
> > Although it's hard to see what could go wrong I/O-wise that doesn't
> > involve vec-syscfg itself having gone wrong, it's probably good
> > practice to do the final error check:
> 
> > 	if (ferror(stdout) || fclose(stdout))
> > 		return EXIT_FAILURE;
> 
> > 	return EXIT_SUCCESS;
> 
> > (In reality, people rarely seem to bother with this, so I'm not going
> > to lose sleep if we don't do it...)
> 
> Yeah, I think this is one of those raising more questions than it
> answers kind of things.

Fair enough.  This is the kind of thing it may be better to do in the
library or framework, since even if people remember to do it, they are
unlikely to do consistent things...

> > > +.globl rdvl_sve
> 
> > Should we stick a
> 
> > 	.type rdvl_sve, @function
> 
> > here?  This may avoid surprises with future toolchain behaviours.
> > Probably doesn't matter, but I have bad memories of Thumb-2...
> 
> > Lacking this annotation is widespread though, as well as being de facto
> > standard before awkward architectures came along.
> 
> Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
> asm code the kernel has.  I don't know if you think it's worth adding
> that to SYM_FUNC_START now we have it though?

Actually, I think the core definition of SYM_FUNC_END() in
<linux/linkage.h> does this.

It would be good to pick up the common linkage macros; if we have to
sprinkle .type manually all over the tests people will likely make
mistakes, to that's probably not worth it.

If picking up the macros isn't trivial to do, I guess it's not that
important at this stage.

> 
> > If the selftests have access to the ENTRY() macro we could use that, but
> > I'm guessing that isn't exported for userspace.
> 
> We don't use that any more anyway, it's SYM_FUNC_START() and friends not

Yes, I was forgetting about the macros that replaced ENTRY().  It's a
while since I wrote much asm code in the kernel...

> that those are outside the kernel either.  We will have to do something
> like that if anyone starts building userspace with BTI though (or I
> might just shove a BTI C in there unconditionally, I'm sure we'll cope
> with the overhead on older systems).

I thought about that, but that .S file isn't annotated as supporting
BTI, so I guess there's no problem for now(?)

It would be harmless to add it, of course.

Cheers
---Dave

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  2021-07-29 13:27         ` Dave P Martin
@ 2021-07-29 16:03           ` Mark Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-29 16:03 UTC (permalink / raw)
  To: Dave P Martin
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

On Thu, Jul 29, 2021 at 02:27:04PM +0100, Dave P Martin wrote:
> On Thu, Jul 29, 2021 at 12:22:17PM +0100, Mark Brown wrote:

> > Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
> > asm code the kernel has.  I don't know if you think it's worth adding
> > that to SYM_FUNC_START now we have it though?

> Actually, I think the core definition of SYM_FUNC_END() in
> <linux/linkage.h> does this.

Ah, so it does.

> It would be good to pick up the common linkage macros; if we have to
> sprinkle .type manually all over the tests people will likely make
> mistakes, to that's probably not worth it.

> If picking up the macros isn't trivial to do, I guess it's not that
> important at this stage.

They're not exported from the kernel at all at the minute so that'd be a
whole new block of work that feels out of scope here, we already have a
stack of asm code in selftests.

> > that those are outside the kernel either.  We will have to do something
> > like that if anyone starts building userspace with BTI though (or I
> > might just shove a BTI C in there unconditionally, I'm sure we'll cope
> > with the overhead on older systems).

> I thought about that, but that .S file isn't annotated as supporting
> BTI, so I guess there's no problem for now(?)

True, we'll generate linker warnings but it should otherwise sort itself
out unless someone forced BTI mode.  The whole annotation thing really
isn't fun to deal with for assembly code, hopefully there'll be some
toolchain improvements in this area at some point.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
@ 2021-07-29 16:03           ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2021-07-29 16:03 UTC (permalink / raw)
  To: Dave P Martin
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest


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

On Thu, Jul 29, 2021 at 02:27:04PM +0100, Dave P Martin wrote:
> On Thu, Jul 29, 2021 at 12:22:17PM +0100, Mark Brown wrote:

> > Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
> > asm code the kernel has.  I don't know if you think it's worth adding
> > that to SYM_FUNC_START now we have it though?

> Actually, I think the core definition of SYM_FUNC_END() in
> <linux/linkage.h> does this.

Ah, so it does.

> It would be good to pick up the common linkage macros; if we have to
> sprinkle .type manually all over the tests people will likely make
> mistakes, to that's probably not worth it.

> If picking up the macros isn't trivial to do, I guess it's not that
> important at this stage.

They're not exported from the kernel at all at the minute so that'd be a
whole new block of work that feels out of scope here, we already have a
stack of asm code in selftests.

> > that those are outside the kernel either.  We will have to do something
> > like that if anyone starts building userspace with BTI though (or I
> > might just shove a BTI C in there unconditionally, I'm sure we'll cope
> > with the overhead on older systems).

> I thought about that, but that .S file isn't annotated as supporting
> BTI, so I guess there's no problem for now(?)

True, we'll generate linker warnings but it should otherwise sort itself
out unless someone forced BTI mode.  The whole annotation thing really
isn't fun to deal with for assembly code, hopefully there'll be some
toolchain improvements in this area at some point.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  2021-07-29 16:03           ` Mark Brown
@ 2021-07-29 16:17             ` Dave Martin
  -1 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29 16:17 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Thu, Jul 29, 2021 at 05:03:34PM +0100, Mark Brown wrote:
> On Thu, Jul 29, 2021 at 02:27:04PM +0100, Dave P Martin wrote:
> > On Thu, Jul 29, 2021 at 12:22:17PM +0100, Mark Brown wrote:
> 
> > > Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
> > > asm code the kernel has.  I don't know if you think it's worth adding
> > > that to SYM_FUNC_START now we have it though?
> 
> > Actually, I think the core definition of SYM_FUNC_END() in
> > <linux/linkage.h> does this.
> 
> Ah, so it does.
> 
> > It would be good to pick up the common linkage macros; if we have to
> > sprinkle .type manually all over the tests people will likely make
> > mistakes, to that's probably not worth it.
> 
> > If picking up the macros isn't trivial to do, I guess it's not that
> > important at this stage.
> 
> They're not exported from the kernel at all at the minute so that'd be a
> whole new block of work that feels out of scope here, we already have a
> stack of asm code in selftests.

Agreed.  Feels like it might be a good idea at some point, but it's
orthogonal to this series, and for now nothing breaks.

> > > that those are outside the kernel either.  We will have to do something
> > > like that if anyone starts building userspace with BTI though (or I
> > > might just shove a BTI C in there unconditionally, I'm sure we'll cope
> > > with the overhead on older systems).
> 
> > I thought about that, but that .S file isn't annotated as supporting
> > BTI, so I guess there's no problem for now(?)
> 
> True, we'll generate linker warnings but it should otherwise sort itself
> out unless someone forced BTI mode.  The whole annotation thing really
> isn't fun to deal with for assembly code, hopefully there'll be some
> toolchain improvements in this area at some point.

Ack

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
@ 2021-07-29 16:17             ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2021-07-29 16:17 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Shuah Khan, linux-arm-kernel,
	linux-kselftest

On Thu, Jul 29, 2021 at 05:03:34PM +0100, Mark Brown wrote:
> On Thu, Jul 29, 2021 at 02:27:04PM +0100, Dave P Martin wrote:
> > On Thu, Jul 29, 2021 at 12:22:17PM +0100, Mark Brown wrote:
> 
> > > Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
> > > asm code the kernel has.  I don't know if you think it's worth adding
> > > that to SYM_FUNC_START now we have it though?
> 
> > Actually, I think the core definition of SYM_FUNC_END() in
> > <linux/linkage.h> does this.
> 
> Ah, so it does.
> 
> > It would be good to pick up the common linkage macros; if we have to
> > sprinkle .type manually all over the tests people will likely make
> > mistakes, to that's probably not worth it.
> 
> > If picking up the macros isn't trivial to do, I guess it's not that
> > important at this stage.
> 
> They're not exported from the kernel at all at the minute so that'd be a
> whole new block of work that feels out of scope here, we already have a
> stack of asm code in selftests.

Agreed.  Feels like it might be a good idea at some point, but it's
orthogonal to this series, and for now nothing breaks.

> > > that those are outside the kernel either.  We will have to do something
> > > like that if anyone starts building userspace with BTI though (or I
> > > might just shove a BTI C in there unconditionally, I'm sure we'll cope
> > > with the overhead on older systems).
> 
> > I thought about that, but that .S file isn't annotated as supporting
> > BTI, so I guess there's no problem for now(?)
> 
> True, we'll generate linker warnings but it should otherwise sort itself
> out unless someone forced BTI mode.  The whole annotation thing really
> isn't fun to deal with for assembly code, hopefully there'll be some
> toolchain improvements in this area at some point.

Ack

Cheers
---Dave

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-07-29 16:21 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-28 16:33 [PATCH v2 0/4] kselftest/arm64: Vector length configuration tests Mark Brown
2021-07-28 16:33 ` Mark Brown
2021-07-28 16:33 ` [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL Mark Brown
2021-07-28 16:33   ` Mark Brown
2021-07-29  9:52   ` Dave Martin
2021-07-29  9:52     ` Dave Martin
2021-07-29 11:22     ` Mark Brown
2021-07-29 11:22       ` Mark Brown
2021-07-29 13:27       ` Dave P Martin
2021-07-29 13:27         ` Dave P Martin
2021-07-29 16:03         ` Mark Brown
2021-07-29 16:03           ` Mark Brown
2021-07-29 16:17           ` Dave Martin
2021-07-29 16:17             ` Dave Martin
2021-07-28 16:33 ` [PATCH v2 2/4] kselftest/arm64: Validate vector lengths are set in sve-probe-vls Mark Brown
2021-07-28 16:33   ` Mark Brown
2021-07-29  9:52   ` Dave Martin
2021-07-29  9:52     ` Dave Martin
2021-07-28 16:33 ` [PATCH v2 3/4] kselftest/arm64: Add tests for SVE vector configuration Mark Brown
2021-07-28 16:33   ` Mark Brown
2021-07-29  9:52   ` Dave Martin
2021-07-29  9:52     ` Dave Martin
2021-07-28 16:33 ` [PATCH v2 4/4] kselftest/arm64: Add a TODO list for floating point tests Mark Brown
2021-07-28 16:33   ` Mark Brown
2021-07-29  9:52   ` Dave Martin
2021-07-29  9:52     ` Dave Martin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.