On 2019-09-10 13:31, Cristian Marussi wrote:

Modify KSFT arm64 toplevel Makefile to maintain arm64 kselftests organized by subsystem, keeping them into distinct subdirectories under arm64 custom KSFT directory: tools/testing/selftests/arm64/

Add to such toplevel Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

Fit existing arm64 tags kselftest into this new schema moving them into their own subdirectory (arm64/tags).

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

Based on: commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

---

v5 --> v6

• using realpath to avoid passing down relative paths
• fix commit msg & Copyright
• removed unneded Makefile export
• added SUBTARGETS specification, to allow building specific only some arm64 test subsystems

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile
• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 63 +++++++++++++++++-- tools/testing/selftests/arm64/README | 25 ++++++++ tools/testing/selftests/arm64/tags/Makefile | 6 ++ .../arm64/{ => tags}/run_tags_test.sh | 0 .../selftests/arm64/{ => tags}/tags_test.c | 0 6 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/tags/Makefile rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%) rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c2b15..1722dae9381a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 TARGETS = android +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..cbb2a5a9e3fc 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,66 @@ # SPDX-License-Identifier: GPL-2.0 -# ARCH can be overridden by the user for cross compiling +# When ARCH not overridden for crosscompiling, lookup machine ARCH ?= $(shell uname -m 2>/dev/null || echo not) ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +SUBTARGETS ?= tags +else +SUBTARGETS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = $(realpath ../../../../)

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

+CFLAGS += -I$(khdr_dir)

+export CFLAGS +export top_srcdir

+all:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• mkdir -p $$BUILD_TARGET;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+install: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+run_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+# Avoid any output on non arm64 on emit_tests +emit_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+clean:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+.PHONY: all clean install run_tests emit_tests diff --git a/tools/testing/selftests/arm64/README b/tools/testing/selftests/arm64/README new file mode 100644 index 000000000000..cc1e51796fee --- /dev/null +++ b/tools/testing/selftests/arm64/README @@ -0,0 +1,25 @@ +KSelfTest ARM64 +===============

+- These tests are arm64 specific and so not built or run but just skipped

• completely when env-variable ARCH is found to be different than 'arm64'
• and `uname -m` reports other than 'aarch64'.

+- Holding true the above, ARM64 KSFT tests can be run within the KSelfTest

• framework using standard Linux top-level-makefile targets:

•  $ make TARGETS=arm64 kselftest-clean
  

•  $ make TARGETS=arm64 kselftest
  

•  or
  

•  $ make -C tools/testing/selftests TARGETS=arm64 \
  

• INSTALL_PATH=<your-installation-path> install
  

•  or, alternatively, only specific arm64/ subtargets can be picked:
  

•  $ make -C tools/testing/selftests TARGETS=arm64 SUBTARGETS="tags signal" \
  

• INSTALL_PATH=<your-installation-path> install
  

• Further details on building and running KFST can be found in:

• Documentation/dev-tools/kselftest.rst
  

diff --git a/tools/testing/selftests/arm64/tags/Makefile b/tools/testing/selftests/arm64/tags/Makefile new file mode 100644 index 000000000000..dcc8b0467b68 --- /dev/null +++ b/tools/testing/selftests/arm64/tags/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0

+TEST_GEN_PROGS := tags_test

This should be TEST_GEN_FILES, since its used by run_tags_test.sh. If its TEST_GEN_PROGS it will be added to the script run_kselftest.sh, and I don't think thats the intent, even though it looked like that before.

Cheers, Anders

On 9/17/19 9:17 AM, Cristian Marussi wrote:

Hi Anders

thanks for the review.

On 17/09/2019 14:42, Anders Roxell wrote:

...

On 2019-09-10 13:31, Cristian Marussi wrote:

...

Modify KSFT arm64 toplevel Makefile to maintain arm64 kselftests organized by subsystem, keeping them into distinct subdirectories under arm64 custom KSFT directory: tools/testing/selftests/arm64/

Add to such toplevel Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

Fit existing arm64 tags kselftest into this new schema moving them into their own subdirectory (arm64/tags).

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

Based on: commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

---

v5 --> v6

• using realpath to avoid passing down relative paths
• fix commit msg & Copyright
• removed unneded Makefile export
• added SUBTARGETS specification, to allow building specific only some arm64 test subsystems

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile
• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 63 +++++++++++++++++-- tools/testing/selftests/arm64/README | 25 ++++++++ tools/testing/selftests/arm64/tags/Makefile | 6 ++ .../arm64/{ => tags}/run_tags_test.sh | 0 .../selftests/arm64/{ => tags}/tags_test.c | 0 6 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/tags/Makefile rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%) rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c2b15..1722dae9381a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 TARGETS = android +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..cbb2a5a9e3fc 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,66 @@ # SPDX-License-Identifier: GPL-2.0 -# ARCH can be overridden by the user for cross compiling +# When ARCH not overridden for crosscompiling, lookup machine ARCH ?= $(shell uname -m 2>/dev/null || echo not) ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +SUBTARGETS ?= tags +else +SUBTARGETS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = $(realpath ../../../../)

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

+CFLAGS += -I$(khdr_dir)

+export CFLAGS +export top_srcdir

+all:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• mkdir -p $$BUILD_TARGET;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+install: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+run_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+# Avoid any output on non arm64 on emit_tests +emit_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+clean:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+.PHONY: all clean install run_tests emit_tests diff --git a/tools/testing/selftests/arm64/README b/tools/testing/selftests/arm64/README new file mode 100644 index 000000000000..cc1e51796fee --- /dev/null +++ b/tools/testing/selftests/arm64/README @@ -0,0 +1,25 @@ +KSelfTest ARM64 +===============

+- These tests are arm64 specific and so not built or run but just skipped

• completely when env-variable ARCH is found to be different than 'arm64'
• and `uname -m` reports other than 'aarch64'.

+- Holding true the above, ARM64 KSFT tests can be run within the KSelfTest

• framework using standard Linux top-level-makefile targets:

•  $ make TARGETS=arm64 kselftest-clean
  

•  $ make TARGETS=arm64 kselftest
  

•  or
  

•  $ make -C tools/testing/selftests TARGETS=arm64 \
  

• INSTALL_PATH=<your-installation-path> install
  

•  or, alternatively, only specific arm64/ subtargets can be picked:
  

•  $ make -C tools/testing/selftests TARGETS=arm64 SUBTARGETS="tags signal" \
  

• INSTALL_PATH=<your-installation-path> install
  

• Further details on building and running KFST can be found in:

• Documentation/dev-tools/kselftest.rst
  

diff --git a/tools/testing/selftests/arm64/tags/Makefile b/tools/testing/selftests/arm64/tags/Makefile new file mode 100644 index 000000000000..dcc8b0467b68 --- /dev/null +++ b/tools/testing/selftests/arm64/tags/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0

+TEST_GEN_PROGS := tags_test

This should be TEST_GEN_FILES, since its used by run_tags_test.sh. If its TEST_GEN_PROGS it will be added to the script run_kselftest.sh, and I don't think thats the intent, even though it looked like that before.

In fact I saw the tags tests running twice (via ./tags_test and via ./run_tags_test.sh) when called via run_kselftest.sh....but since it was already like that in the original patch so I did not want to fix it in the context of this series (where tags tests are simply relocated into their own directory)

I could add a separate fix on top of this series if it could make sense.

We are still in review phase I would think. It would make sense to fix the original patch and not as a separate fix patch.

thanks, -- Shuah

On 9/17/19 9:58 AM, Cristian Marussi wrote:

On 17/09/2019 16:29, shuah wrote:

...

On 9/17/19 9:17 AM, Cristian Marussi wrote:

...

Hi Anders

thanks for the review.

On 17/09/2019 14:42, Anders Roxell wrote:

...

On 2019-09-10 13:31, Cristian Marussi wrote:

...

Modify KSFT arm64 toplevel Makefile to maintain arm64 kselftests organized by subsystem, keeping them into distinct subdirectories under arm64 custom KSFT directory: tools/testing/selftests/arm64/

Add to such toplevel Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

Fit existing arm64 tags kselftest into this new schema moving them into their own subdirectory (arm64/tags).

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

Based on: commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

---

v5 --> v6

• using realpath to avoid passing down relative paths
• fix commit msg & Copyright
• removed unneded Makefile export
• added SUBTARGETS specification, to allow building specific only some arm64 test subsystems

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile
• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 63 +++++++++++++++++-- tools/testing/selftests/arm64/README | 25 ++++++++ tools/testing/selftests/arm64/tags/Makefile | 6 ++ .../arm64/{ => tags}/run_tags_test.sh | 0 .../selftests/arm64/{ => tags}/tags_test.c | 0 6 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/tags/Makefile rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%) rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c2b15..1722dae9381a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 TARGETS = android +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..cbb2a5a9e3fc 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,66 @@ # SPDX-License-Identifier: GPL-2.0 -# ARCH can be overridden by the user for cross compiling +# When ARCH not overridden for crosscompiling, lookup machine ARCH ?= $(shell uname -m 2>/dev/null || echo not) ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +SUBTARGETS ?= tags +else +SUBTARGETS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = $(realpath ../../../../)

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

+CFLAGS += -I$(khdr_dir)

+export CFLAGS +export top_srcdir

+all:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• mkdir -p $$BUILD_TARGET;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+install: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+run_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+# Avoid any output on non arm64 on emit_tests +emit_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+clean:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+.PHONY: all clean install run_tests emit_tests diff --git a/tools/testing/selftests/arm64/README b/tools/testing/selftests/arm64/README new file mode 100644 index 000000000000..cc1e51796fee --- /dev/null +++ b/tools/testing/selftests/arm64/README @@ -0,0 +1,25 @@ +KSelfTest ARM64 +===============

+- These tests are arm64 specific and so not built or run but just skipped

• completely when env-variable ARCH is found to be different than 'arm64'
• and `uname -m` reports other than 'aarch64'.

+- Holding true the above, ARM64 KSFT tests can be run within the KSelfTest

• framework using standard Linux top-level-makefile targets:

•  $ make TARGETS=arm64 kselftest-clean
  

•  $ make TARGETS=arm64 kselftest
  

•  or
  

•  $ make -C tools/testing/selftests TARGETS=arm64 \
  

• INSTALL_PATH=<your-installation-path> install
  

•  or, alternatively, only specific arm64/ subtargets can be picked:
  

•  $ make -C tools/testing/selftests TARGETS=arm64 SUBTARGETS="tags signal" \
  

• INSTALL_PATH=<your-installation-path> install
  

• Further details on building and running KFST can be found in:

• Documentation/dev-tools/kselftest.rst
  

diff --git a/tools/testing/selftests/arm64/tags/Makefile b/tools/testing/selftests/arm64/tags/Makefile new file mode 100644 index 000000000000..dcc8b0467b68 --- /dev/null +++ b/tools/testing/selftests/arm64/tags/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0

+TEST_GEN_PROGS := tags_test

This should be TEST_GEN_FILES, since its used by run_tags_test.sh. If its TEST_GEN_PROGS it will be added to the script run_kselftest.sh, and I don't think thats the intent, even though it looked like that before.

In fact I saw the tags tests running twice (via ./tags_test and via ./run_tags_test.sh) when called via run_kselftest.sh....but since it was already like that in the original patch so I did not want to fix it in the context of this series (where tags tests are simply relocated into their own directory)

I could add a separate fix on top of this series if it could make sense.

We are still in review phase I would think. It would make sense to fix the original patch and not as a separate fix patch.

The original code for:

...

...

...

...

+++ b/tools/testing/selftests/arm64/tags/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0

+TEST_GEN_PROGS := tags_test

has not been introduced in this series (under review), and that's merged already I think:

https://lore.kernel.org/linux-kselftest/0999c80cd639b78ae27c0674069d55283322...

This patch only moves the original tags tests (introduced with the above commit) from arm64/ into their own arm64/tags/ directory and integrate with the new arm64/signal tests by this series.

Here I have just moved down the original code including the bug, that's why I'm saying I could push a fix on top of this series.

I thought I had to keep the two series distinct give that I'm integrating someone else commit (and eventually fix later): but if not I can alternatively fix the above tags tests issue in the next v7 02/11 within this series.

Thanks. Yeah if it is already in a tree then, let's fix it in a separate patch and add Fixes tag to make it easier to track them.

thanks, -- Shuah

On 9/17/19 10:05 AM, Dave Martin wrote:

On Tue, Sep 10, 2019 at 01:31:01pm +0100, Cristian Marussi wrote:

...

Modify KSFT arm64 toplevel Makefile to maintain arm64 kselftests organized by subsystem, keeping them into distinct subdirectories under arm64 custom KSFT directory: tools/testing/selftests/arm64/

Add to such toplevel Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

Fit existing arm64 tags kselftest into this new schema moving them into their own subdirectory (arm64/tags).

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

Based on: commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

---

v5 --> v6

• using realpath to avoid passing down relative paths
• fix commit msg & Copyright
• removed unneded Makefile export
• added SUBTARGETS specification, to allow building specific only some arm64 test subsystems

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile
• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 63 +++++++++++++++++-- tools/testing/selftests/arm64/README | 25 ++++++++ tools/testing/selftests/arm64/tags/Makefile | 6 ++ .../arm64/{ => tags}/run_tags_test.sh | 0 .../selftests/arm64/{ => tags}/tags_test.c | 0 6 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/tags/Makefile rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%) rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c2b15..1722dae9381a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 TARGETS = android +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..cbb2a5a9e3fc 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,66 @@ # SPDX-License-Identifier: GPL-2.0 -# ARCH can be overridden by the user for cross compiling +# When ARCH not overridden for crosscompiling, lookup machine ARCH ?= $(shell uname -m 2>/dev/null || echo not) ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +SUBTARGETS ?= tags +else +SUBTARGETS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = $(realpath ../../../../)

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

I still tend to think that for now we should just do what all the other tests do.

Most tests use

CFLAGS += -I../../../../usr/include/

in their Makefiles.

For us, the test Makefiles are nested one level deeper, so I guess we would put

CFLAGS += -I../../../../../usr/include/

in each.

This will break in some cases, but only in the same cases where kselftest is already broken.

Ideally we would fix this globally, but can that instead be done independently of this series?

Fixing only arm64, by pasting some arbitrary logic from selftests/Makefile doesn't seem like a future-proof approach.

Or did I miss something?

...

+CFLAGS += -I$(khdr_dir)

+export CFLAGS +export top_srcdir

+all:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• mkdir -p $$BUILD_TARGET;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+install: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+run_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+# Avoid any output on non arm64 on emit_tests

This comment can be dropped: the whole file does nothing for non-arm64, and it achieves it in the same way as other arch-specific Makefiles, so it's odd to have the comment here specifically (?)

With or without the above changes, I'm happy to give

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

(but Shuah or someone will need to give a view on how this integrates with kselftest overall).

I am reviewing the series this week. I will provide comments in a day or two.

thanks, -- Shuah

On 17/09/2019 17:18, shuah wrote:

On 9/17/19 10:05 AM, Dave Martin wrote:

...

On Tue, Sep 10, 2019 at 01:31:01pm +0100, Cristian Marussi wrote:

...

Modify KSFT arm64 toplevel Makefile to maintain arm64 kselftests organized by subsystem, keeping them into distinct subdirectories under arm64 custom KSFT directory: tools/testing/selftests/arm64/

Add to such toplevel Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

Fit existing arm64 tags kselftest into this new schema moving them into their own subdirectory (arm64/tags).

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

Based on: commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

---

v5 --> v6

• using realpath to avoid passing down relative paths
• fix commit msg & Copyright
• removed unneded Makefile export
• added SUBTARGETS specification, to allow building specific only some arm64 test subsystems

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile
• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 63 +++++++++++++++++-- tools/testing/selftests/arm64/README | 25 ++++++++ tools/testing/selftests/arm64/tags/Makefile | 6 ++ .../arm64/{ => tags}/run_tags_test.sh | 0 .../selftests/arm64/{ => tags}/tags_test.c | 0 6 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/tags/Makefile rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%) rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c2b15..1722dae9381a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 TARGETS = android +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..cbb2a5a9e3fc 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,66 @@ # SPDX-License-Identifier: GPL-2.0 -# ARCH can be overridden by the user for cross compiling +# When ARCH not overridden for crosscompiling, lookup machine ARCH ?= $(shell uname -m 2>/dev/null || echo not) ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +SUBTARGETS ?= tags +else +SUBTARGETS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = $(realpath ../../../../)

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

I still tend to think that for now we should just do what all the other tests do.

Most tests use

CFLAGS += -I../../../../usr/include/

in their Makefiles.

For us, the test Makefiles are nested one level deeper, so I guess we would put

CFLAGS += -I../../../../../usr/include/

in each.

This will break in some cases, but only in the same cases where kselftest is already broken.

Ideally we would fix this globally, but can that instead be done independently of this series?

Fixing only arm64, by pasting some arbitrary logic from selftests/Makefile doesn't seem like a future-proof approach.

Or did I miss something?

...

+CFLAGS += -I$(khdr_dir)

+export CFLAGS +export top_srcdir

+all:

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• mkdir -p $$BUILD_TARGET;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+install: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+run_tests: all

• @for DIR in $(SUBTARGETS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+# Avoid any output on non arm64 on emit_tests

This comment can be dropped: the whole file does nothing for non-arm64, and it achieves it in the same way as other arch-specific Makefiles, so it's odd to have the comment here specifically (?)

With or without the above changes, I'm happy to give

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

(but Shuah or someone will need to give a view on how this integrates with kselftest overall).

I am reviewing the series this week. I will provide comments in a day or two.

Thanks Shuah.

As a side question... should I rebase next v7 (after your feedback) on latest kselftest branch (linux-kselftest-5.4-rc1 or whatever)

Cristian

thanks, -- Shuah

On Tue, Sep 10, 2019 at 01:31:05pm +0100, Cristian Marussi wrote:

Add a simple mangle testcase which messes with the ucontext_t from within the signal handler, trying to set the PSTATE SSBS bit and verify that SSBS bit set is preserved across sigreturn. When available, use MRS SBSS support to set/get SSBS bit, otherwise lookup PSTATE.SSBS directly.

Additionally, in order to support this test specific needs:

• extend signal testing framework to allow the definition of a custom per test initialization function to be run at the end of test setup and before test run routine.
• introduce a set_regval() helper to set system register values in a toolchain independent way.
• introduce also a new common utility function: get_current_context() which can be used to grab a ucontext without the help of libc, and detect if such ucontext has been actively used to jump back.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v5 --> v6

• using SIGTRAP as sig_copyctx for get_current_context()
• get_current_context() is now __always_inline
• last minute check for SSBS cleared
• restore volatile usage, dropping useless DSB
• output clobber on *dest_uc
• no abort() on SSSB not cleared
• refactored/relocated test_init() call to be after test_setup() [to catch early SIGILL while initializing]
• avoid MRS SSBS when !feats_ok()...use instead PSTATE.SSBS
• refactored SIG_COPYCTX usage to fit new splitted-by-signal layout

v3 --> v4

• fix commit message
• missing include signal.h
• added .init per-test init-func
• added set_regval() helper
• added SSBS clear to 0 custom .init function
• removed volatile qualifier associated with sig_atomic_t data
• added dsb inside handler to ensure the writes related to the grabbed ucontext have completed
• added test description

---

.../selftests/arm64/signal/test_signals.c | 2 +- .../selftests/arm64/signal/test_signals.h | 54 ++++++---- .../arm64/signal/test_signals_utils.c | 41 +++++++- .../arm64/signal/test_signals_utils.h | 99 +++++++++++++++++++ .../testcases/mangle_pstate_ssbs_regs.c | 88 +++++++++++++++++ 5 files changed, 258 insertions(+), 26 deletions(-) create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c

diff --git a/tools/testing/selftests/arm64/signal/test_signals.c b/tools/testing/selftests/arm64/signal/test_signals.c index cb970346b280..6e4da9c920d4 100644 --- a/tools/testing/selftests/arm64/signal/test_signals.c +++ b/tools/testing/selftests/arm64/signal/test_signals.c @@ -19,7 +19,7 @@ int main(int argc, char *argv[]) current = &tde; ksft_print_msg("%s :: %s\n", current->name, current->descr);

• if (test_setup(current)) {

• if (test_setup(current) && test_init(current)) { test_run(current); test_result(current); test_cleanup(current);

diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h index 0dd71700ff67..753fe64cbe11 100644 --- a/tools/testing/selftests/arm64/signal/test_signals.h +++ b/tools/testing/selftests/arm64/signal/test_signals.h @@ -27,6 +27,14 @@ : "memory"); \ } +#define set_regval(regname, in) \ +{ \

• asm volatile("msr " __stringify(regname) ", %0" \
• : \
• : "r" (in) \
• : "memory"); \

+}

/* Regs encoding and masks naming copied in from sysreg.h */ #define SYS_ID_AA64MMFR1_EL1 S3_0_C0_C7_1 /* MRS Emulated */ #define SYS_ID_AA64MMFR2_EL1 S3_0_C0_C7_2 /* MRS Emulated */ @@ -62,39 +70,47 @@ enum { */ struct tdescr { /* KEEP THIS FIELD FIRST for easier lookup from assembly */

• void *token;

• void *token;

All the whitespace changes here make it difficult to review this patch.

Can you please fold the indentation changes into the patches that added the lines in the first place -- patches 2 and 5, maybe others?

(Assuming you make no other changes, feel free to keep my Reviewed-by on those patches.)

/* when disabled token based sanity checking is skipped in handler */

• bool sanity_disabled;

• bool sanity_disabled; /* just a name for the test-case; manadatory field */

• char *name;
• char *descr;
• unsigned long feats_required;

• char *name;
• char *descr;
• unsigned long feats_required; /* bitmask of effectively supported feats: populated at run-time */

• unsigned long feats_supported;
• bool initialized;
• unsigned int minsigstksz;

• unsigned long feats_supported;
• bool initialized;
• unsigned int minsigstksz; /* signum used as a test trigger. Zero if no trigger-signal is used */

• int sig_trig;

• int sig_trig; /*
  • signum considered as a successful test completion.
  • Zero when no signal is expected on success
  */

• int sig_ok;

• int sig_ok; /* signum expected on unsupported CPU features. */

• int sig_unsupp;

• int sig_unsupp; /* a timeout in second for test completion */

• unsigned int timeout;
• bool triggered;
• bool pass;

• unsigned int timeout;
• bool triggered;
• bool pass; /* optional sa_flags for the installed handler */

• int sa_flags;
• ucontext_t saved_uc;

• int sa_flags;
• ucontext_t saved_uc;
• /* used by get_current_ctx() */
• size_t live_sz;
• ucontext_t *live_uc;

• /* a custom setup function to be called before test starts */

• volatile sig_atomic_t live_uc_valid;
• /* optional test private data */
• void *priv;
• /* a custom setup: called alternatively to default_setup */ int (*setup)(struct tdescr *td);
• /* a custom init: called by default test init after test_setup */
• void (*init)(struct tdescr *td); /* a custom cleanup function called before test exits */ void (*cleanup)(struct tdescr *td);

• /* an optional function to be used as a trigger for test starting */

• /* an optional function to be used as a trigger for starting test */ int (*trigger)(struct tdescr *td); /*
  • the actual test-core: invoked differently depending on the

@@ -103,8 +119,6 @@ struct tdescr { int (*run)(struct tdescr *td, siginfo_t *si, ucontext_t *uc); /* an optional function for custom results' processing */ void (*check_result)(struct tdescr *td);

• void *priv;

}; extern struct tdescr tde; diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c index 7324e8a7f47f..133105755683 100644 --- a/tools/testing/selftests/arm64/signal/test_signals_utils.c +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c @@ -4,19 +4,22 @@ #include <stdio.h> #include <stdlib.h> #include <signal.h> -#include <string.h>

Is this intentional? This file still uses memcpy().

#include <unistd.h> #include <assert.h> #include <sys/auxv.h> #include <linux/auxvec.h> #include <ucontext.h> +#include <asm/unistd.h>

#include "test_signals.h" #include "test_signals_utils.h" #include "testcases/testcases.h" extern struct tdescr *current; +static int sig_copyctx = SIGTRAP;

static char const *const feats_names[FMAX_END] = { " SSBS ", " PAN ", @@ -148,6 +151,20 @@ static bool handle_signal_ok(struct tdescr *td, return true; } +static bool handle_signal_copyctx(struct tdescr *td,

• 		  siginfo_t *si, void *uc)
  

+{

• /* Mangling PC to avoid loops on original BRK instr */
• ((ucontext_t *)uc)->uc_mcontext.pc += 4;
• memcpy(td->live_uc, uc, td->live_sz);
• ASSERT_GOOD_CONTEXT(td->live_uc);
• td->live_uc_valid = 1;
• fprintf(stderr,

• "GOOD CONTEXT grabbed from sig_copyctx handler\n");
  

• return true;

+}

static void default_handler(int signum, siginfo_t *si, void *uc) { if (current->sig_unsupp && signum == current->sig_unsupp && @@ -159,6 +176,9 @@ static void default_handler(int signum, siginfo_t *si, void *uc) } else if (current->sig_ok && signum == current->sig_ok && handle_signal_ok(current, si, uc)) { fprintf(stderr, "Handled SIG_OK\n");

• } else if (signum == sig_copyctx && current->live_uc &&

•    handle_signal_copyctx(current, si, uc)) {
  

• fprintf(stderr, "Handled SIG_COPYCTX\n");
  

  } else { if (signum == SIGALRM && current->timeout) { fprintf(stderr, "-- Timeout !\n");

@@ -211,8 +231,17 @@ static inline int default_trigger(struct tdescr *td) return !raise(td->sig_trig); } -static int test_init(struct tdescr *td) +int test_init(struct tdescr *td) {

• if (td->sig_trig == sig_copyctx) {

• fprintf(stdout,
  

• 	"Signal %d is RESERVED, cannot be used as a trigger. Aborting\n",
  

• 	sig_copyctx);
  

• return 0;
  

• }
• /* just in case */
• unblock_signal(sig_copyctx);
• td->minsigstksz = getauxval(AT_MINSIGSTKSZ); if (!td->minsigstksz) td->minsigstksz = MINSIGSTKSZ;

@@ -253,7 +282,12 @@ static int test_init(struct tdescr *td) ~td->feats_supported)); }

• /* Perform test specific additional initialization */
• if (td->init)

• td->init(td);
  

  td->initialized = 1;
• fprintf(stderr, "Testcase initialized.\n");
• return 1;

} @@ -265,9 +299,6 @@ int test_setup(struct tdescr *td) assert(td->name); assert(td->run);

• if (!test_init(td))

• return 0;
  

• if (td->setup) return td->setup(td); else

diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h index 47a7592b7c53..3ad062af1fa7 100644 --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h @@ -4,8 +4,13 @@ #ifndef __TEST_SIGNALS_UTILS_H__ #define __TEST_SIGNALS_UTILS_H__ +#include <assert.h> +#include <stdio.h> +#include <string.h>

#include "test_signals.h" +int test_init(struct tdescr *td); int test_setup(struct tdescr *td); void test_cleanup(struct tdescr *td); int test_run(struct tdescr *td); @@ -16,4 +21,98 @@ static inline bool feats_ok(struct tdescr *td) return (td->feats_required & td->feats_supported) == td->feats_required; } +/*

• • Obtaining a valid and full-blown ucontext_t from userspace is tricky:
• • libc getcontext does() not save all the regs and messes with some of
• • them (pstate value in particular is not reliable).
• • Here we use a service signal to grab the ucontext_t from inside a
• • dedicated signal handler, since there, it is populated by Kernel
• • itself in setup_sigframe(). The grabbed context is then stored and
• • made available in td->live_uc.
• • As service-signal is used a SIGTRAP induced by a 'brk' instruction,
• • because here we have to avoid syscalls to trigger the signal since
• • they would cause any SVE sigframe content (if any) to be removed.
• • Anyway this function really serves a dual purpose:
• • 1. grab a valid sigcontext into td->live_uc for result analysis: in
• • such case it returns 1.
• • 2. detect if, somehow, a previously grabbed live_uc context has been
• • used actively with a sigreturn: in such a case the execution would have
• • magically resumed in the middle of this function itself (seen_already==1):
• • in such a case return 0, since in fact we have not just simply grabbed
• • the context.
• • This latter case is useful to detect when a fake_sigreturn test-case has
• • unexpectedly survived without hitting a SEGV.
• • Note that the case of runtime dynamically sized sigframes (like in SVE
• • context) is still NOT addressed: sigframe size is supposed to be fixed
• • at sizeof(ucontext_t).
• */

+static __always_inline bool get_current_context(struct tdescr *td,

• 				ucontext_t *dest_uc)
  

+{

• static volatile bool seen_already;
• assert(td && dest_uc);
• /* it's a genuine invocation..reinit */
• seen_already = 0;

Nit: can we have "= 0" as an initializer in the declaration above?

• td->live_uc_valid = 0;
• td->live_sz = sizeof(*dest_uc);
• memset(dest_uc, 0x00, td->live_sz);
• td->live_uc = dest_uc;
• /*

• * Grab ucontext_t triggering a SIGTRAP.
  

• *
  

• * Note that:
  

• * - live_uc_valid is declared volatile sig_atomic_t in
  

• *   struct tdescr since it will be changed inside the
  

• *   sig_copyctx handler
  

• * - the additional 'memory' clobber is there to avoid possible
  

• *   compiler's assumption on live_uc_valid, seen-already and
  

I could be wrong about this, but I'm not sure that the "memory" clobber is sufficient for for seen_already, because of the way that variable is scoped to this function; however, you declare seen_already volatile anyway which should be sufficient.

I suggest you just omit seen_already from this comment.

• *   the content pointed by dest_uc, which are all changed inside
  

• *   the signal handler
  

• * - BRK causes a debug exception which is handled by the Kernel
  

• *   and finally causes the SIGTRAP signal to be delivered to this
  

• *   test thread. Since such delivery happens on the ret_to_user()
  

• *   /do_notify_resume() debug exception return-path, we are sure
  

• *   that the registered SIGTRAP handler has been run to completion
  

• *   before the execution path is restored here: as a consequence
  

• *   we can be sure that the volatile sig_atomic_t live_uc_valid
  

• *   carries a meaningful result. Being in a single thread context
  

• *   we'll also be sure that any access to memory modified by the
  

• *   handler (namely ucontext_t) will be visible once returned.
  

• * - note that since we are using a breakpoint instruction here
  

• *   to cause a SIGTRAP, the ucontext_t grabbed from the signal
  

• *   handler would naturally contain a PC pointing exactly to this
  

• *   BRK line, which means that, on return from the signal handler,
  

• *   or if we place the ucontext_t on the stack to fake a sigreturn,
  

• *   we'll end up in an infinite loop of BRK-SIGTRAP-handler.
  

• *   For this reason we take care to artificially move forward the
  

• *   PC to the next instruction while inside the signal handler.
  

• */
  

• asm volatile ("brk #666"

•       : "=m" (*dest_uc)
  

Make this an input-output argument ("+m")? The memset() needs to take effect happen before the asm.

•       :
  

•       : "memory");
  

• /*

• * If we get here with seen_already==1 it implies the td->live_uc
  

• * context has been used to get back here....this probably means
  

• * a test has failed to cause a SEGV...anyway live_uc does not
  

• * point to a just acquired copy of ucontext_t...so return 0
  

• */
  

• if (seen_already) {

• fprintf(stdout,
  

• 	"Unexpected successful sigreturn detected: live_uc is stale !\n");
  

• return 0;
  

• }
• seen_already = 1;
• return td->live_uc_valid;

+} #endif diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c new file mode 100644 index 000000000000..e2b87ea4c11f --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, setting the
• • SSBS bit to 1 and veryfing that such modification is preserved.
• */

+#include <stdio.h> +#include <stdlib.h> +#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+static void mangle_invalid_pstate_ssbs_init(struct tdescr *td) +{

• if (feats_ok(td)) {

• fprintf(stderr, "Clearing SSBS to 0\n");
  

• set_regval(SSBS_SYSREG, 0);
  

• }

+}

+static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,

• 			  siginfo_t *si, ucontext_t *uc)
  

+{

• ASSERT_GOOD_CONTEXT(uc);
• /*

• * If HW_SSBS is supported but we weren't able to clear SSBS during
  

• * test_init, or if something has reset SSBS in the meantime, abort.
  

What is "HW_SSBS" ?

• */
  

• if (feats_ok(td) && (uc->uc_mcontext.pstate & PSR_SSBS_BIT)) {

• fprintf(stderr,
  

• 	"SSBS unexpectedly NOT zeroed ! Something's wrong. Abort\n");
  

• abort();
  

• }
• /* set bit value ... should NOT be cleared by Kernel on sigreturn */
• uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
• fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",

• uc->uc_mcontext.pstate);
  

• /* Save after mangling...it should be preserved */
• td->saved_uc = *uc;
• return 1;

+}

+static void pstate_ssbs_bit_checks(struct tdescr *td) +{

• uint64_t val = 0;
• ucontext_t uc;
• /* This check reports some result even if MRS SSBS unsupported */
• if (get_current_context(td, &uc))

• fprintf(stderr,
  

• 	"INFO: live_uc - got PSTATE: 0x%016llX -> SSBS %s\n",
  

• 	uc.uc_mcontext.pstate,
  

• 	(td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT) ==
  

• 	(uc.uc_mcontext.pstate & PSR_SSBS_BIT) ?
  

• 	"PRESERVED" : "CLEARED");
  

Don't we subsequently refer to uc even if get_current_context() failed here?

• /* Choose check method depending of supported features */
• if (feats_ok(td)) {

• fprintf(stderr, "Checking with MRS SSBS...\n");
  

• get_regval(SSBS_SYSREG, val);
  

• fprintf(stderr, "INFO: MRS SSBS - got: 0x%016lX\n", val);
  

• } else {

• fprintf(stderr, "Checking with PSTATE.SSBS...\n");
  

• val = uc.uc_mcontext.pstate;
  

• }
• /* pass when preserved */
• td->pass = (val & PSR_SSBS_BIT) ==

•    (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT);
  

Does this mean the test fails when SSBS isn't supported at all? That doesn't seem right.

I think trying to handle both levels of SSBS support within the same test is complicating things here. The purpose of this is to check that sigreturn doesn't mask out the SSBS bit when it shouldn't: we don't care whether the SSBS architecture feature actually works.

So, would it be simpler to drop the MSR/MRS direct access to the SSBS bit?

Instead, we could predicate this test on whether ID_AA64PFR1_EL1.SSBS

= 1 instead of HWCAP_SSBS, and we could just check that run() can

successfully _toggle_ uc->uc_mcontext.pstate ^= PSR_SSBS_BIT, with the change checked via a subsequent get_current_context().

In other words, we no longer try to initialise SSBS to a particular value. Instead, we just check the we can change the bit.

Does that make sense?

Cheers ---Dave

On Mon, Oct 07, 2019 at 07:23:19PM +0100, Cristian Marussi wrote:

Hi

finally back on this series.

On 17/09/2019 17:05, Dave Martin wrote:

...

On Tue, Sep 10, 2019 at 01:31:05pm +0100, Cristian Marussi wrote:

...

Add a simple mangle testcase which messes with the ucontext_t from within the signal handler, trying to set the PSTATE SSBS bit and verify that SSBS bit set is preserved across sigreturn. When available, use MRS SBSS support to set/get SSBS bit, otherwise lookup PSTATE.SSBS directly.

Additionally, in order to support this test specific needs:

• extend signal testing framework to allow the definition of a custom per test initialization function to be run at the end of test setup and before test run routine.
• introduce a set_regval() helper to set system register values in a toolchain independent way.
• introduce also a new common utility function: get_current_context() which can be used to grab a ucontext without the help of libc, and detect if such ucontext has been actively used to jump back.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v5 --> v6

• using SIGTRAP as sig_copyctx for get_current_context()
• get_current_context() is now __always_inline
• last minute check for SSBS cleared
• restore volatile usage, dropping useless DSB
• output clobber on *dest_uc
• no abort() on SSSB not cleared
• refactored/relocated test_init() call to be after test_setup() [to catch early SIGILL while initializing]
• avoid MRS SSBS when !feats_ok()...use instead PSTATE.SSBS
• refactored SIG_COPYCTX usage to fit new splitted-by-signal layout

[...]

...

...

diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h

[...]

...

...

+static __always_inline bool get_current_context(struct tdescr *td,

• 				ucontext_t *dest_uc)
  

+{

• static volatile bool seen_already;
• assert(td && dest_uc);
• /* it's a genuine invocation..reinit */
• seen_already = 0;

Nit: can we have "= 0" as an initializer in the declaration above?

Not sure if you mean to add a zero initialization to the static declaration or to fold this apparently redundant

...

...

• seen_already = 0;

into the above.

If you mean the latter folding I think I cannot for the following reasons:

the static seen_already is placed out of the stack and automatically initialized to 0 once for all at program initialization.

...

...

• static volatile bool seen_already;

After that, seen_already is set to 1 after the context has been grabbed using the brk/SIGTRAP trick in order to signify that context has been grabbed successfully. But is is set to 1 only after having been checked for ZERO in order to detect if we happened to arrive here in the middle of this function as the unexpected result of a fake_sigreturn using this context.

...

...

• if (seen_already) {

• fprintf(stdout,
  

• 	"Unexpected successful sigreturn detected: live_uc is stale !\n");
  

• return 0;
  

• }
• seen_already = 1;

So get_current_context returns 1 on success when a good context has been grabbed, while returns 0 when in fact returning from a sigreturn using a previously grabbed context (in a highly corrupted stack frame...so you'll probably want to exit straight away in the caller at that point.)

Now answering finally your (possible) question about the apparently redundant

...

...

seen_already = 0;

what happens if I call regularly two times in sequence this utility function in the same program ?

say at first to grab a context to analyze the PSTATE reported there, and maybe again later to grab another context to put onto the stack, or to recheck the PSTATE ?

I would expect NOT to fail and obtain both times a new valid context, but my understanding is that in order to be able to do so, I'll have to re-initialize to 0 the seen_already flag explicitly on each invocation.

My bad -- I'd confused myself and missed the significance of "static" here. As you point out, seen_already needs to be static for other reasons, and reinitialised each time we call this function.

So you were right to ignore this nitpick :)

...

...

• td->live_uc_valid = 0;
• td->live_sz = sizeof(*dest_uc);
• memset(dest_uc, 0x00, td->live_sz);
• td->live_uc = dest_uc;
• /*

• * Grab ucontext_t triggering a SIGTRAP.
  

• *
  

• * Note that:
  

• * - live_uc_valid is declared volatile sig_atomic_t in
  

• *   struct tdescr since it will be changed inside the
  

• *   sig_copyctx handler
  

• * - the additional 'memory' clobber is there to avoid possible
  

• *   compiler's assumption on live_uc_valid, seen-already and
  

I could be wrong about this, but I'm not sure that the "memory" clobber is sufficient for for seen_already, because of the way that variable is scoped to this function; however, you declare seen_already volatile anyway which should be sufficient.

I suggest you just omit seen_already from this comment.

Ok

...

...

• *   the content pointed by dest_uc, which are all changed inside
  

• *   the signal handler
  

• * - BRK causes a debug exception which is handled by the Kernel
  

• *   and finally causes the SIGTRAP signal to be delivered to this
  

• *   test thread. Since such delivery happens on the ret_to_user()
  

• *   /do_notify_resume() debug exception return-path, we are sure
  

• *   that the registered SIGTRAP handler has been run to completion
  

• *   before the execution path is restored here: as a consequence
  

• *   we can be sure that the volatile sig_atomic_t live_uc_valid
  

• *   carries a meaningful result. Being in a single thread context
  

• *   we'll also be sure that any access to memory modified by the
  

• *   handler (namely ucontext_t) will be visible once returned.
  

• * - note that since we are using a breakpoint instruction here
  

• *   to cause a SIGTRAP, the ucontext_t grabbed from the signal
  

• *   handler would naturally contain a PC pointing exactly to this
  

• *   BRK line, which means that, on return from the signal handler,
  

• *   or if we place the ucontext_t on the stack to fake a sigreturn,
  

• *   we'll end up in an infinite loop of BRK-SIGTRAP-handler.
  

• *   For this reason we take care to artificially move forward the
  

• *   PC to the next instruction while inside the signal handler.
  

• */
  

• asm volatile ("brk #666"

•       : "=m" (*dest_uc)
  

Make this an input-output argument ("+m")? The memset() needs to take effect happen before the asm.

Ok

...

...

•       :
  

•       : "memory");
  

• /*

• * If we get here with seen_already==1 it implies the td->live_uc
  

• * context has been used to get back here....this probably means
  

• * a test has failed to cause a SEGV...anyway live_uc does not
  

• * point to a just acquired copy of ucontext_t...so return 0
  

• */
  

• if (seen_already) {

• fprintf(stdout,
  

• 	"Unexpected successful sigreturn detected: live_uc is stale !\n");
  

• return 0;
  

• }
• seen_already = 1;
• return td->live_uc_valid;

+} #endif diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c new file mode 100644 index 000000000000..e2b87ea4c11f --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, setting the
• • SSBS bit to 1 and veryfing that such modification is preserved.
• */

+#include <stdio.h> +#include <stdlib.h> +#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+static void mangle_invalid_pstate_ssbs_init(struct tdescr *td) +{

• if (feats_ok(td)) {

• fprintf(stderr, "Clearing SSBS to 0\n");
  

• set_regval(SSBS_SYSREG, 0);
  

• }

+}

+static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,

• 			  siginfo_t *si, ucontext_t *uc)
  

+{

• ASSERT_GOOD_CONTEXT(uc);
• /*

• * If HW_SSBS is supported but we weren't able to clear SSBS during
  

• * test_init, or if something has reset SSBS in the meantime, abort.
  

What is "HW_SSBS" ?

HWCAP_SSBS, I'll fix

...

...

• */
  

• if (feats_ok(td) && (uc->uc_mcontext.pstate & PSR_SSBS_BIT)) {

• fprintf(stderr,
  

• 	"SSBS unexpectedly NOT zeroed ! Something's wrong. Abort\n");
  

• abort();
  

• }
• /* set bit value ... should NOT be cleared by Kernel on sigreturn */
• uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
• fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",

• uc->uc_mcontext.pstate);
  

• /* Save after mangling...it should be preserved */
• td->saved_uc = *uc;
• return 1;

+}

+static void pstate_ssbs_bit_checks(struct tdescr *td) +{

• uint64_t val = 0;
• ucontext_t uc;
• /* This check reports some result even if MRS SSBS unsupported */
• if (get_current_context(td, &uc))

• fprintf(stderr,
  

• 	"INFO: live_uc - got PSTATE: 0x%016llX -> SSBS %s\n",
  

• 	uc.uc_mcontext.pstate,
  

• 	(td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT) ==
  

• 	(uc.uc_mcontext.pstate & PSR_SSBS_BIT) ?
  

• 	"PRESERVED" : "CLEARED");
  

Don't we subsequently refer to uc even if get_current_context() failed here?

Yes, but in fact in this invocation it cannot fail, it can fail only once a sigreturn with a previously grabbed context uses it and unexpectedly returns. Even though the if is redundant in fact here, it is misleading as used now, I'll fix checking the retvalue and bailing out with abort() in case get_curent_context unexpectedly failed in this conetxt.

OK, that sounds like a cleaner approach, even though I guess you're correct that the failure is impossible here.

...

...

• /* Choose check method depending of supported features */
• if (feats_ok(td)) {

• fprintf(stderr, "Checking with MRS SSBS...\n");
  

• get_regval(SSBS_SYSREG, val);
  

• fprintf(stderr, "INFO: MRS SSBS - got: 0x%016lX\n", val);
  

• } else {

• fprintf(stderr, "Checking with PSTATE.SSBS...\n");
  

• val = uc.uc_mcontext.pstate;
  

• }
• /* pass when preserved */
• td->pass = (val & PSR_SSBS_BIT) ==

•    (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT);
  

Does this mean the test fails when SSBS isn't supported at all? That doesn't seem right.

I think trying to handle both levels of SSBS support within the same test is complicating things here. The purpose of this is to check that sigreturn doesn't mask out the SSBS bit when it shouldn't: we don't care whether the SSBS architecture feature actually works.

So, would it be simpler to drop the MSR/MRS direct access to the SSBS bit?

Instead, we could predicate this test on whether ID_AA64PFR1_EL1.SSBS

...

= 1 instead of HWCAP_SSBS, and we could just check that run() can

successfully _toggle_ uc->uc_mcontext.pstate ^= PSR_SSBS_BIT, with the change checked via a subsequent get_current_context().

Ok I'll check on ID_AA64PFR1_EL1.SSBS >= 1 instead of HWCAP_SSBS,since it is all I need to be able to use PSTATE.SSBS (with .SSBS=2 I've got also MRS/MSR which does NOT hurt)

I'll SKIP if support is missing returning KSFT_SKIP (extending test_init related logic for this)

...

In other words, we no longer try to initialise SSBS to a particular value. Instead, we just check the we can change the bit.

Does that make sense?

Yes but I'll have anyway to test by setting to SSBS in the ucontext (only when SSBS supported as said), and verify that is NOT cleared, because if I toggle the bit 1-->0 then I would have nothing to check really. (I made this error already in a previous iteration...)

Hmmm, OK, I'll take a another look when reviewing v7.

Cheers ---Dave

On Tue, Sep 10, 2019 at 01:31:06pm +0100, Cristian Marussi wrote:

Add a simple fake_sigreturn testcase which builds a ucontext_t with a bad magic header and place it onto the stack. Expects a SIGSEGV on test PASS.

Introduce a common utility assembly trampoline function to invoke a sigreturn while placing the provided sigframe at wanted alignment and also an helper to make space when needed inside the sigframe reserved area.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v5 --> v6

• fake_sigreturn: cleaned up, avoiding excessive SP misalignments
• fake_sigreturn: better formatting and prologue
• get_starting_header: cleand up and commented
• avoid timeout on failure

[...]

diff --git a/tools/testing/selftests/arm64/signal/signals.S b/tools/testing/selftests/arm64/signal/signals.S new file mode 100644 index 000000000000..e670f8f2c8de --- /dev/null +++ b/tools/testing/selftests/arm64/signal/signals.S @@ -0,0 +1,64 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (C) 2019 ARM Limited */

+#include <asm/unistd.h>

+.section .rodata, "a" +call_fmt:

• .asciz "Calling sigreturn with fake sigframe sized:%zd at SP @%08lX\n"

+.text

+.globl fake_sigreturn

+/* fake_sigreturn x0:&sigframe, x1:sigframe_size, x2:misalign_bytes */ +fake_sigreturn:

• stp x29, x30, [sp, #-16]!
• mov x29, sp
• mov x20, x0
• mov x21, x1
• mov x22, x2
• /* create space on the stack for fake sigframe 16 bytes-aligned */
• add x0, x21, x22
• add x0, x0, #16
• bic x0, x0, #15 /* round_up(sigframe_size + misalign_bytes, 16) */

If I've figured this out right, x0 as computed here actually looks like round_up(sigframe_size + misalign_bytes + 1, 16) - 1.

(n + (m - 1)) & ~(m - 1) is the classic way to round up when m is a power of 2.

That's why I originally suggested to add 15. Your code works, but I think it always allocates at least one byte more than needed (?)

This is not a huge deal, and better than allocating one byte to few, but it would be good to understand whether this behaviour was intentional or not.

• sub sp, sp, x0
• add x23, sp, x22 /* new sigframe base with misaligment if any */
• ldr x0, =call_fmt
• mov x1, x21
• mov x2, x23
• bl printf
• /* memcpy the provided content, while still keeping SP aligned */
• mov x0, x23
• mov x1, x20
• mov x2, x21
• bl memcpy
• /*

• * Here saving a last minute SP to current->token acts as a marker:
  

• * if we got here, we are successfully faking a sigreturn; in other
  

• * words we are sure no bad fatal signal has been raised till now
  

• * for unrelated reasons, so we should consider the possibly observed
  

• * fatal signal like SEGV coming from Kernel restore_sigframe() and
  

• * triggered as expected from our test-case.
  

• * For simplicity this assumes that current field 'token' is laid out
  

• * as first in struct tdescr
  

• */
  

• ldr x0, current
• str x23, [x0]
• /* finally move SP to misaligned address...if any requested */
• mov sp, x23
• mov x8, #__NR_rt_sigreturn
• svc #0
• /*

• * Above sigreturn should not return...looping here leads to a timeout
  

• * and ensure proper and clean test failure, instead of jumping around
  

• * on a potentially corrupted stack.
  

• */
  

• b .

[...]

diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c index 1914a01222a1..e3521949b800 100644 --- a/tools/testing/selftests/arm64/signal/testcases/testcases.c +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c @@ -148,3 +148,49 @@ bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err) return true; }

+/*

• • This function walks through the records inside the provided reserved area
• • trying to find enough space to fit @need_sz bytes: if not enough space is
• • available and an extra_context record is present, it throws away the
• • extra_context record.
• • It returns a pointer to a new header where it is possible to start storing
• • our need_sz bytes.
• • @shead: points to the start of reserved area
• • @need_sz: needed bytes
• • @resv_sz: reserved area size in bytes
• • @offset: if not null, this will be filled with the offset of the return

• •    head pointer from @shead
    

• • @return: pointer to a new head where to start storing need_sz bytes, or

• •    NULL if space could not be made available.
    

• */

That's much clearer now, thanks for that.

+struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,

• 		       size_t need_sz, size_t resv_sz,
  

• 		       size_t *offset)
  

+{

[...]

Cheers ---Dave