diff mbox series

[v2,1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL

Message ID 20210728163318.51492-2-broonie@kernel.org (mailing list archive)
State New
Headers show
Series kselftest/arm64: Vector length configuration tests | expand

Commit Message

Mark Brown July 28, 2021, 4:33 p.m. UTC
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

Comments

Dave Martin July 29, 2021, 9:52 a.m. UTC | #1
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
Mark Brown July 29, 2021, 11:22 a.m. UTC | #2
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).
Dave Martin July 29, 2021, 1:27 p.m. UTC | #3
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
Mark Brown July 29, 2021, 4:03 p.m. UTC | #4
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.
Dave Martin July 29, 2021, 4:17 p.m. UTC | #5
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
diff mbox series

Patch

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