[PATCH v6 00/11] Add arm64/signal initial kselftest support

[PATCH v6 00/11] Add arm64/signal initial kselftest support

[Cristian Marussi]

Hi

this patchset aims to add the initial arch-specific arm64 support to
kselftest starting with signals-related test-cases.

This series is based on arm64/for-next/core [1]:

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

A common internal test-case layout is proposed for signal tests and it is
wired-up to the toplevel kselftest Makefile, so that it should be possible
at the end to run it on an arm64 target in the usual way with KSFT.

~/linux# make TARGETS=arm64 kselftest

New KSFT arm64 testcases live inside tools/testing/selftests/arm64 grouped
by family inside subdirectories: arm64/signal is the first family proposed
with this series.

This series converts also to this subdirectory scheme the pre-existing
(already queued on arm64/for-next/core) KSFT arm64 tags tests, moving them
into arm64/tags.

Thanks

Cristian

[1] git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core

Notes:
-----
- further details in the included READMEs

- more tests still to be written (current strategy is going through the related
  Kernel signal-handling code and write a test for each possible and sensible code-path)
  A few ideas for more TODO testcases:
	- fake_sigreturn_unmapped_sp: SP into unmapped addrs
	- fake_sigreturn_kernelspace_sp: SP into kernel addrs
	- fake_sigreturn_sve_bad_extra_context: SVE extra context badly formed
	- fake_sigreturn_misaligned_sp_4: misaligned SP by 4 (i.e., __alignof__(struct _aarch64_ctx))
	- fake_sigreturn_misaligned_sp_8: misaligned SP by 8 (i.e., sizeof(struct _aarch64_ctx))
	- fake_sigreturn_bad_size_non_aligned: a size that doesn't overflow __reserved[], but is not a multiple of 16
	- fake_sigreturn_bad_size_tiny: a size that is less than 16
	- fake_sigreturn_bad_size_overflow_tiny: a size that does overflow __reserved[], but by less than 16 bytes?
	- mangle_sve_invalid_extra_context: SVE extra_context invalid

- SVE signal testcases and special handling will be part of an additional patch
  still to be released

- KSFT arm64 tags test patch
  https://lore.kernel.org/linux-arm-kernel/c1e6aad230658bc175b42d92daeff2e30050302a.1563904656.git.andreyknvl@google.com/
  is relocated into its own directory under tools/testing/selftests/arm64/tags


Changes:
--------
 v5-->v6:
 - added arm64 toplevel Makefile SUBTARGETS env var to be able to selectively
   build only some arm64/ tests subdirectories
 - removed unneed toplevel Makefile exports and fixed Copyright
 - better checks for supported features and features names helpers
 - converted some run-time critical assert() to abort() to avoid
   issues when -NDEBUG is set
 - default_handler() signal handler refactored and split
 - using SIGTRAP for get_current_context()
 - use volatile where proper
 - refactor and relocate test_init() invocation
 - review usage of MRS SSBS instructions depending on HW_SSBS
 - cleanup fake_sigreturn trampoline
 - cleanup get_starting_header helper
 - avoiding timeout test failures wherever possible (fail immediately
   if possible)

 v4-->v5:
 - rebased on arm64/for-next-core merging 01/11 with KSFT tags tests:
   commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")
 - moved .gitignore up on elevel
 - moved kernel header search mechanism into KSFT arm64 toplevel Makefile
   so that it can be used easily also by each arm64 KSFT subsystem inside
   subdirs of arm64

 v3-->v4:
 - rebased on v5.3-rc6
 - added test descriptions
 - fixed commit messages (imperative mood)
 - added missing includes and removed unneeded ones
 - added/used new get_starting_head() helper
 - fixed/simplified signal.S::fakke_sigreturn()
 - added set_regval() macro and .init initialization func
 - better synchonization in get_current_context()
 - macroization of mangle_pstate_invalid_mode_el
 - split mangle_pstate_invalid_mode_el h/t
 - removed standalone mode
 - simplified CPU features checks
 - fixed/refactored get_header() and validation routines
 - simplfied docs


 v2-->v3:
 - rebased on v5.3-rc2
 - better test result characterization looking for
   SEGV_ACCERR in si_code on SIGSEGV
 - using KSFT Framework macros for retvalues
 - removed SAFE_WRITE()/dump_uc: buggy, un-needed and unused
 - reviewed generation process of test_arm64_signals.sh runner script
 - re-added a fixed fake_sigreturn_misaligned_sp testcase and a properly
   extended fake_sigreturn() helper
 - added tests' TODO notes


 v1-->v2:
- rebased on 5.2-rc7
- various makefile's cleanups
- mixed READMEs fixes
- fixed test_arm64_signals.sh runner script
- cleaned up assembly code in signal.S
- improved get_current_context() logic
- fixed SAFE_WRITE()
- common support code split into more chunks, each one introduced when
  needed by some new testcases
- fixed some headers validation routines in testcases.c
- removed some still broken/immature tests:
  + fake_sigreturn_misaligned
  + fake_sigreturn_overflow_reserved
  + mangle_pc_invalid
  + mangle_sp_misaligned
- fixed some other testcases:
  + mangle_pstate_ssbs_regs: better checks of SSBS bit when feature unsupported
  + mangle_pstate_invalid_compat_toggle: name fix
  + mangle_pstate_invalid_mode_el[1-3]: precautionary zeroing PSTATE.MODE
  + fake_sigreturn_bad_magic, fake_sigreturn_bad_size,
    fake_sigreturn_bad_size_for_magic0:
    - accounting for available space...dropping extra when needed
    - keeping alignent
- new testcases on FPSMID context:
  + fake_sigreturn_missing_fpsimd
  + fake_sigreturn_duplicated_fpsimd


Cristian Marussi (11):
  kselftest: arm64: extend toplevel skeleton Makefile
  kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils
  kselftest: arm64: mangle_pstate_invalid_daif_bits
  kselftest: arm64: mangle_pstate_invalid_mode_el[123][ht]
  kselftest: arm64: mangle_pstate_ssbs_regs
  kselftest: arm64: fake_sigreturn_bad_magic
  kselftest: arm64: fake_sigreturn_bad_size_for_magic0
  kselftest: arm64: fake_sigreturn_missing_fpsimd
  kselftest: arm64: fake_sigreturn_duplicated_fpsimd
  kselftest: arm64: fake_sigreturn_bad_size
  kselftest: arm64: fake_sigreturn_misaligned_sp

 tools/testing/selftests/Makefile              |   1 +
 tools/testing/selftests/arm64/Makefile        |  63 +++-
 tools/testing/selftests/arm64/README          |  25 ++
 .../testing/selftests/arm64/signal/.gitignore |   3 +
 tools/testing/selftests/arm64/signal/Makefile |  32 ++
 tools/testing/selftests/arm64/signal/README   |  59 ++++
 .../testing/selftests/arm64/signal/signals.S  |  64 ++++
 .../selftests/arm64/signal/test_signals.c     |  29 ++
 .../selftests/arm64/signal/test_signals.h     | 125 +++++++
 .../arm64/signal/test_signals_utils.c         | 331 ++++++++++++++++++
 .../arm64/signal/test_signals_utils.h         | 120 +++++++
 .../testcases/fake_sigreturn_bad_magic.c      |  52 +++
 .../testcases/fake_sigreturn_bad_size.c       |  77 ++++
 .../fake_sigreturn_bad_size_for_magic0.c      |  46 +++
 .../fake_sigreturn_duplicated_fpsimd.c        |  50 +++
 .../testcases/fake_sigreturn_misaligned_sp.c  |  37 ++
 .../testcases/fake_sigreturn_missing_fpsimd.c |  50 +++
 .../mangle_pstate_invalid_compat_toggle.c     |  31 ++
 .../mangle_pstate_invalid_daif_bits.c         |  35 ++
 .../mangle_pstate_invalid_mode_el1h.c         |  15 +
 .../mangle_pstate_invalid_mode_el1t.c         |  15 +
 .../mangle_pstate_invalid_mode_el2h.c         |  15 +
 .../mangle_pstate_invalid_mode_el2t.c         |  15 +
 .../mangle_pstate_invalid_mode_el3h.c         |  15 +
 .../mangle_pstate_invalid_mode_el3t.c         |  15 +
 .../mangle_pstate_invalid_mode_template.h     |  28 ++
 .../testcases/mangle_pstate_ssbs_regs.c       |  88 +++++
 .../arm64/signal/testcases/testcases.c        | 196 +++++++++++
 .../arm64/signal/testcases/testcases.h        | 104 ++++++
 tools/testing/selftests/arm64/tags/Makefile   |   6 +
 .../arm64/{ => tags}/run_tags_test.sh         |   0
 .../selftests/arm64/{ => tags}/tags_test.c    |   0
 32 files changed, 1738 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/arm64/README
 create mode 100644 tools/testing/selftests/arm64/signal/.gitignore
 create mode 100644 tools/testing/selftests/arm64/signal/Makefile
 create mode 100644 tools/testing/selftests/arm64/signal/README
 create mode 100644 tools/testing/selftests/arm64/signal/signals.S
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals.c
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals.h
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.c
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.h
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_misaligned_sp.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.h
 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%)

-- 
2.17.1

[PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Cristian Marussi]

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
+TEST_PROGS := run_tags_test.sh
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/tags/run_tags_test.sh
similarity index 100%
rename from tools/testing/selftests/arm64/run_tags_test.sh
rename to tools/testing/selftests/arm64/tags/run_tags_test.sh
diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags/tags_test.c
similarity index 100%
rename from tools/testing/selftests/arm64/tags_test.c
rename to tools/testing/selftests/arm64/tags/tags_test.c
-- 
2.17.1

[PATCH v6 02/11] kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils

[Cristian Marussi]

Add some arm64/signal specific boilerplate and utility code to help
further testcases' development.

Introduce also one simple testcase mangle_pstate_invalid_compat_toggle
and some related helpers: it is a simple mangle testcase which messes
with the ucontext_t from within the signal handler, trying to toggle
PSTATE state bits to switch the system between 32bit/64bit execution
state. Expects SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v5 --> v6
- fix commit msg
- feat_names is char const *const
- better supported options check and reporting
- removed critical asserts to avoid issues with NDEBUG
- more robust get_header
- fix validation for ESR_CONTEXT size
- add more explicit comment in GET_RESV_NEXT_HEAD() macro
- refactored default_handler()
- feats_ok() now public
- call always test_results() no matter the outcome of test_run()
v4 --> v5
- moved kernel headers include search to top level KSFT arm64 Makefile
- removed warning about kernel headers not found
- moved testcases/.gitignore up one level
v3 --> v4
- removed standalone mode
- fixed arm64/signal/README
- add file level comments: test layout / test description
- reduced verbosity
- removed spurious headers includes
- reviewed ID_AA64MMFR[1,2]_EL1 macros
- removed unused feats_ok
- simplified CPU features gathering
- reviewed included headers
- fixed/refactored get_header() and validation routines
- added test description
---
 tools/testing/selftests/arm64/Makefile        |   2 +-
 .../testing/selftests/arm64/signal/.gitignore |   3 +
 tools/testing/selftests/arm64/signal/Makefile |  32 ++
 tools/testing/selftests/arm64/signal/README   |  59 ++++
 .../selftests/arm64/signal/test_signals.c     |  29 ++
 .../selftests/arm64/signal/test_signals.h     | 111 +++++++
 .../arm64/signal/test_signals_utils.c         | 300 ++++++++++++++++++
 .../arm64/signal/test_signals_utils.h         |  19 ++
 .../mangle_pstate_invalid_compat_toggle.c     |  31 ++
 .../arm64/signal/testcases/testcases.c        | 150 +++++++++
 .../arm64/signal/testcases/testcases.h        | 100 ++++++
 11 files changed, 835 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/signal/.gitignore
 create mode 100644 tools/testing/selftests/arm64/signal/Makefile
 create mode 100644 tools/testing/selftests/arm64/signal/README
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals.c
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals.h
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.c
 create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.h
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.h

diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
index cbb2a5a9e3fc..892163106137 100644
--- a/tools/testing/selftests/arm64/Makefile
+++ b/tools/testing/selftests/arm64/Makefile
@@ -4,7 +4,7 @@
 ARCH ?= $(shell uname -m 2>/dev/null || echo not)
 
 ifneq (,$(filter $(ARCH),aarch64 arm64))
-SUBTARGETS ?= tags
+SUBTARGETS ?= tags signal
 else
 SUBTARGETS :=
 endif
diff --git a/tools/testing/selftests/arm64/signal/.gitignore b/tools/testing/selftests/arm64/signal/.gitignore
new file mode 100644
index 000000000000..e5aeae45febb
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/.gitignore
@@ -0,0 +1,3 @@
+!*.[ch]
+mangle_*
+fake_sigreturn_*
diff --git a/tools/testing/selftests/arm64/signal/Makefile b/tools/testing/selftests/arm64/signal/Makefile
new file mode 100644
index 000000000000..f78f5190e3d4
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/Makefile
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2019 ARM Limited
+
+# Additional include paths needed by kselftest.h and local headers
+CFLAGS += -D_GNU_SOURCE -std=gnu99 -I.
+
+SRCS := $(filter-out testcases/testcases.c,$(wildcard testcases/*.c))
+PROGS := $(patsubst %.c,%,$(SRCS))
+
+# Generated binaries to be installed by top KSFT script
+TEST_GEN_PROGS := $(notdir $(PROGS))
+
+# Get Kernel headers installed and use them.
+KSFT_KHDR_INSTALL := 1
+
+# Including KSFT lib.mk here will also mangle the TEST_GEN_PROGS list
+# to account for any OUTPUT target-dirs optionally provided by
+# the toplevel makefile
+include ../../lib.mk
+
+$(TEST_GEN_PROGS): $(PROGS)
+	cp $(PROGS) $(OUTPUT)/
+
+clean:
+	$(CLEAN)
+	rm -f $(PROGS)
+
+# Common test-unit targets to build common-layout test-cases executables
+# Needs secondary expansion to properly include the testcase c-file in pre-reqs
+.SECONDEXPANSION:
+$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c $$@.c test_signals.h test_signals_utils.h testcases/testcases.h
+	$(CC) $(CFLAGS) $^ -o $@
diff --git a/tools/testing/selftests/arm64/signal/README b/tools/testing/selftests/arm64/signal/README
new file mode 100644
index 000000000000..967a531b245c
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/README
@@ -0,0 +1,59 @@
+KSelfTest arm64/signal/
+=======================
+
+Signals Tests
++++++++++++++
+
+- Tests are built around a common main compilation unit: such shared main
+  enforces a standard sequence of operations needed to perform a single
+  signal-test (setup/trigger/run/result/cleanup)
+
+- The above mentioned ops are configurable on a test-by-test basis: each test
+  is described (and configured) using the descriptor signals.h::struct tdescr
+
+- Each signal testcase is compiled into its own executable: a separate
+  executable is used for each test since many tests complete successfully
+  by receiving some kind of fatal signal from the Kernel, so it's safer
+  to run each test unit in its own standalone process, so as to start each
+  test from a clean slate.
+
+- New tests can be simply defined in testcases/ dir providing a proper struct
+  tdescr overriding all the defaults we wish to change (as of now providing a
+  custom run method is mandatory though)
+
+- Signals' test-cases hereafter defined belong currently to two
+  principal families:
+
+  - 'mangle_' tests: a real signal (SIGUSR1) is raised and used as a trigger
+    and then the test case code modifies the signal frame from inside the
+    signal handler itself.
+
+  - 'fake_sigreturn_' tests: a brand new custom artificial sigframe structure
+    is placed on the stack and a sigreturn syscall is called to simulate a
+    real signal return. This kind of tests does not use a trigger usually and
+    they are just fired using some simple included assembly trampoline code.
+
+ - Most of these tests are successfully passing if the process gets killed by
+   some fatal signal: usually SIGSEGV or SIGBUS. Since while writing this
+   kind of tests it is extremely easy in fact to end-up injecting other
+   unrelated SEGV bugs in the testcases, it becomes extremely tricky to
+   be really sure that the tests are really addressing what they are meant
+   to address and they are not instead falling apart due to unplanned bugs
+   in the test code.
+   In order to alleviate the misery of the life of such test-developer, a few
+   helpers are provided:
+
+   - a couple of ASSERT_BAD/GOOD_CONTEXT() macros to easily parse a ucontext_t
+     and verify if it is indeed GOOD or BAD (depending on what we were
+     expecting), using the same logic/perspective as in the arm64 Kernel signals
+     routines.
+
+   - a sanity mechanism to be used in 'fake_sigreturn_'-alike tests: enabled by
+     default it takes care to verify that the test-execution had at least
+     successfully progressed up to the stage of triggering the fake sigreturn
+     call.
+
+  In both cases test results are expected in terms of:
+   - some fatal signal sent by the Kernel to the test process
+  or
+  - analyzing some final regs state
diff --git a/tools/testing/selftests/arm64/signal/test_signals.c b/tools/testing/selftests/arm64/signal/test_signals.c
new file mode 100644
index 000000000000..cb970346b280
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/test_signals.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Generic test wrapper for arm64 signal tests.
+ *
+ * Each test provides its own tde struct tdescr descriptor to link with
+ * this wrapper. Framework provides common helpers.
+ */
+#include <kselftest.h>
+
+#include "test_signals.h"
+#include "test_signals_utils.h"
+
+struct tdescr *current;
+
+int main(int argc, char *argv[])
+{
+	current = &tde;
+
+	ksft_print_msg("%s :: %s\n", current->name, current->descr);
+	if (test_setup(current)) {
+		test_run(current);
+		test_result(current);
+		test_cleanup(current);
+	}
+
+	return current->pass ? KSFT_PASS : KSFT_FAIL;
+}
diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h
new file mode 100644
index 000000000000..0dd71700ff67
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/test_signals.h
@@ -0,0 +1,111 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2019 ARM Limited */
+
+#ifndef __TEST_SIGNALS_H__
+#define __TEST_SIGNALS_H__
+
+#include <signal.h>
+#include <stdbool.h>
+#include <ucontext.h>
+
+/*
+ * Using ARCH specific and sanitized Kernel headers installed by KSFT
+ * framework since we asked for it by setting flag KSFT_KHDR_INSTALL
+ * in our Makefile.
+ */
+#include <asm/ptrace.h>
+#include <asm/hwcap.h>
+
+#define __stringify_1(x...)	#x
+#define __stringify(x...)	__stringify_1(x)
+
+#define get_regval(regname, out)			\
+{							\
+	asm volatile("mrs %0, " __stringify(regname)	\
+	: "=r" (out)					\
+	:						\
+	: "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 */
+#define ID_AA64MMFR1_PAN_SHIFT	20
+#define ID_AA64MMFR2_UAO_SHIFT	4
+
+/* Local Helpers */
+#define ID_AA64MMFR1_EL1_PAN_SUPPORTED(val) \
+	(!!((val) & (0xfUL << ID_AA64MMFR1_PAN_SHIFT)))
+#define ID_AA64MMFR2_EL1_UAO_SUPPORTED(val) \
+	(!!((val) & (0xfUL << ID_AA64MMFR2_UAO_SHIFT)))
+
+#define SSBS_SYSREG		S3_3_C4_C2_6	/* EL0 supported */
+
+/*
+ * Feature flags used in tdescr.feats_required to specify
+ * any feature by the test
+ */
+enum {
+	FSSBS_BIT,
+	FPAN_BIT,
+	FUAO_BIT,
+	FMAX_END
+};
+
+#define FEAT_SSBS		(1UL << FSSBS_BIT)
+#define FEAT_PAN		(1UL << FPAN_BIT)
+#define FEAT_UAO		(1UL << FUAO_BIT)
+
+/*
+ * A descriptor used to describe and configure a test case.
+ * Fields with a non-trivial meaning are described inline in the following.
+ */
+struct tdescr {
+	/* KEEP THIS FIELD FIRST for easier lookup from assembly */
+	void		*token;
+	/* when disabled token based sanity checking is skipped in handler */
+	bool		sanity_disabled;
+	/* just a name for the test-case; manadatory field */
+	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;
+	/* signum used as a test trigger. Zero if no trigger-signal is used */
+	int		sig_trig;
+	/*
+	 * signum considered as a successful test completion.
+	 * Zero when no signal is expected on success
+	 */
+	int		sig_ok;
+	/* signum expected on unsupported CPU features. */
+	int		sig_unsupp;
+	/* a timeout in second for test completion */
+	unsigned int	timeout;
+	bool		triggered;
+	bool		pass;
+	/* optional sa_flags for the installed handler */
+	int		sa_flags;
+	ucontext_t	saved_uc;
+
+	/* a custom setup function to be called before test starts */
+	int (*setup)(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 */
+	int (*trigger)(struct tdescr *td);
+	/*
+	 * the actual test-core: invoked differently depending on the
+	 * presence of the trigger function above; this is mandatory
+	 */
+	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;
+#endif
diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
new file mode 100644
index 000000000000..7324e8a7f47f
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c
@@ -0,0 +1,300 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2019 ARM Limited */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <string.h>
+#include <unistd.h>
+#include <assert.h>
+#include <sys/auxv.h>
+#include <linux/auxvec.h>
+#include <ucontext.h>
+
+#include "test_signals.h"
+#include "test_signals_utils.h"
+#include "testcases/testcases.h"
+
+extern struct tdescr *current;
+
+static char const *const feats_names[FMAX_END] = {
+	" SSBS ",
+	" PAN ",
+	" UAO ",
+};
+
+#define MAX_FEATS_SZ	128
+static char feats_string[MAX_FEATS_SZ];
+
+static inline char *feats_to_string(unsigned long feats)
+{
+	size_t flen = MAX_FEATS_SZ - 1;
+
+	for (int i = 0; i < FMAX_END; i++) {
+		if (feats & (1UL << i)) {
+			size_t tlen = strlen(feats_names[i]);
+
+			assert(flen > tlen);
+			flen -= tlen;
+			strncat(feats_string, feats_names[i], flen);
+		}
+	}
+
+	return feats_string;
+}
+
+static void unblock_signal(int signum)
+{
+	sigset_t sset;
+
+	sigemptyset(&sset);
+	sigaddset(&sset, signum);
+	sigprocmask(SIG_UNBLOCK, &sset, NULL);
+}
+
+static void default_result(struct tdescr *td, bool force_exit)
+{
+	if (td->pass)
+		fprintf(stderr, "==>> completed. PASS(1)\n");
+	else
+		fprintf(stdout, "==>> completed. FAIL(0)\n");
+	if (force_exit)
+		exit(td->pass ? EXIT_SUCCESS : EXIT_FAILURE);
+}
+
+/*
+ * The following handle_signal_* helpers are used by main default_handler
+ * and are meant to return true when signal is handled successfully:
+ * when false is returned instead, it means that the signal was somehow
+ * unexpected in that context and it was NOT handled; default_handler will
+ * take care of such unexpected situations.
+ */
+
+static bool handle_signal_unsupported(struct tdescr *td,
+				      siginfo_t *si, void *uc)
+{
+	if (feats_ok(td))
+		return false;
+
+	/* Mangling PC to avoid loops on original SIGILL */
+	((ucontext_t *)uc)->uc_mcontext.pc += 4;
+
+	if (!td->initialized) {
+		fprintf(stderr,
+			"Got SIG_UNSUPP @test_init. Ignore.\n");
+	} else {
+		fprintf(stderr,
+			"-- RX SIG_UNSUPP on unsupported feat...OK\n");
+		td->pass = 1;
+		default_result(current, 1);
+	}
+
+	return true;
+}
+
+static bool handle_signal_trigger(struct tdescr *td,
+				  siginfo_t *si, void *uc)
+{
+	td->triggered = 1;
+	/* ->run was asserted NON-NULL in test_setup() already */
+	td->run(td, si, uc);
+
+	return true;
+}
+
+static bool handle_signal_ok(struct tdescr *td,
+			     siginfo_t *si, void *uc)
+{
+	/*
+	 * it's a bug in the test code when this assert fail:
+	 * if sig_trig was defined, it must have been used before getting here.
+	 */
+	assert(!td->sig_trig || td->triggered);
+	fprintf(stderr,
+		"SIG_OK -- SP:0x%llX  si_addr@:%p  si_code:%d  token@:%p  offset:%ld\n",
+		((ucontext_t *)uc)->uc_mcontext.sp,
+		si->si_addr, si->si_code, td->token, td->token - si->si_addr);
+	/*
+	 * fake_sigreturn tests, which have sanity_enabled=1, set, at the very
+	 * last time, the token field to the SP address used to place the fake
+	 * sigframe: so token==0 means we never made it to the end,
+	 * segfaulting well-before, and the test is possibly broken.
+	 */
+	if (!td->sanity_disabled && !td->token) {
+		fprintf(stdout,
+			"current->token ZEROED...test is probably broken!\n");
+		abort();
+	}
+	/*
+	 * Trying to narrow down the SEGV to the ones generated by Kernel itself
+	 * via arm64_notify_segfault(). This is a best-effort check anyway, and
+	 * the si_code check may need to change if this aspect of the kernel
+	 * ABI changes.
+	 */
+	if (td->sig_ok == SIGSEGV && si->si_code != SEGV_ACCERR) {
+		fprintf(stdout,
+			"si_code != SEGV_ACCERR...test is probably broken!\n");
+		abort();
+	}
+	td->pass = 1;
+	/*
+	 * Some tests can lead to SEGV loops: in such a case we want to
+	 * terminate immediately exiting straight away; some others are not
+	 * supposed to outlive the signal handler code, due to the content of
+	 * the fake sigframe which caused the signal itself.
+	 */
+	default_result(current, 1);
+
+	return true;
+}
+
+static void default_handler(int signum, siginfo_t *si, void *uc)
+{
+	if (current->sig_unsupp && signum == current->sig_unsupp &&
+	    handle_signal_unsupported(current, si, uc)) {
+		fprintf(stderr, "Handled SIG_UNSUPP\n");
+	} else if (current->sig_trig && signum == current->sig_trig &&
+		   handle_signal_trigger(current, si, uc)) {
+		fprintf(stderr, "Handled SIG_TRIG\n");
+	} else if (current->sig_ok && signum == current->sig_ok &&
+		   handle_signal_ok(current, si, uc)) {
+		fprintf(stderr, "Handled SIG_OK\n");
+	} else {
+		if (signum == SIGALRM && current->timeout) {
+			fprintf(stderr, "-- Timeout !\n");
+		} else {
+			fprintf(stderr,
+				"-- RX UNEXPECTED SIGNAL: %d\n", signum);
+		}
+		default_result(current, 1);
+	}
+}
+
+static int default_setup(struct tdescr *td)
+{
+	struct sigaction sa;
+
+	sa.sa_sigaction = default_handler;
+	sa.sa_flags = SA_SIGINFO | SA_RESTART;
+	sa.sa_flags |= td->sa_flags;
+	sigemptyset(&sa.sa_mask);
+	/* uncatchable signals naturally skipped ... */
+	for (int sig = 1; sig < 32; sig++)
+		sigaction(sig, &sa, NULL);
+	/*
+	 * RT Signals default disposition is Term but they cannot be
+	 * generated by the Kernel in response to our tests; so just catch
+	 * them all and report them as UNEXPECTED signals.
+	 */
+	for (int sig = SIGRTMIN; sig <= SIGRTMAX; sig++)
+		sigaction(sig, &sa, NULL);
+
+	/* just in case...unblock explicitly all we need */
+	if (td->sig_trig)
+		unblock_signal(td->sig_trig);
+	if (td->sig_ok)
+		unblock_signal(td->sig_ok);
+	if (td->sig_unsupp)
+		unblock_signal(td->sig_unsupp);
+
+	if (td->timeout) {
+		unblock_signal(SIGALRM);
+		alarm(td->timeout);
+	}
+	fprintf(stderr, "Registered handlers for all signals.\n");
+
+	return 1;
+}
+
+static inline int default_trigger(struct tdescr *td)
+{
+	return !raise(td->sig_trig);
+}
+
+static int test_init(struct tdescr *td)
+{
+	td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
+	if (!td->minsigstksz)
+		td->minsigstksz = MINSIGSTKSZ;
+	fprintf(stderr, "Detected MINSTKSIGSZ:%d\n", td->minsigstksz);
+
+	if (td->feats_required) {
+		td->feats_supported = 0;
+		/*
+		 * Checking for CPU required features using both the
+		 * auxval and the arm64 MRS Emulation to read sysregs.
+		 */
+		if (getauxval(AT_HWCAP) & HWCAP_SSBS)
+			td->feats_supported |= FEAT_SSBS;
+		if (getauxval(AT_HWCAP) & HWCAP_CPUID) {
+			uint64_t val = 0;
+
+			/* Uses MRS emulation to check capability */
+			get_regval(SYS_ID_AA64MMFR1_EL1, val);
+			if (ID_AA64MMFR1_EL1_PAN_SUPPORTED(val))
+				td->feats_supported |= FEAT_PAN;
+			/* Uses MRS emulation to check capability */
+			get_regval(SYS_ID_AA64MMFR2_EL1, val);
+			if (ID_AA64MMFR2_EL1_UAO_SUPPORTED(val))
+				td->feats_supported |= FEAT_UAO;
+		} else {
+			fprintf(stderr,
+				"HWCAP_CPUID NOT available. Mark ALL feats UNSUPPORTED.\n");
+		}
+		if (feats_ok(td))
+			fprintf(stderr,
+				"Required Features: [%s] supported\n",
+				feats_to_string(td->feats_required &
+						td->feats_supported));
+		else
+			fprintf(stderr,
+				"Required Features: [%s] NOT supported\n",
+				feats_to_string(td->feats_required &
+						~td->feats_supported));
+	}
+
+	td->initialized = 1;
+	return 1;
+}
+
+int test_setup(struct tdescr *td)
+{
+	/* assert core invariants symptom of a rotten testcase */
+	assert(current);
+	assert(td);
+	assert(td->name);
+	assert(td->run);
+
+	if (!test_init(td))
+		return 0;
+
+	if (td->setup)
+		return td->setup(td);
+	else
+		return default_setup(td);
+}
+
+int test_run(struct tdescr *td)
+{
+	if (td->sig_trig) {
+		if (td->trigger)
+			return td->trigger(td);
+		else
+			return default_trigger(td);
+	} else {
+		return td->run(td, NULL, NULL);
+	}
+}
+
+void test_result(struct tdescr *td)
+{
+	if (td->check_result)
+		td->check_result(td);
+	default_result(td, 0);
+}
+
+void test_cleanup(struct tdescr *td)
+{
+	if (td->cleanup)
+		td->cleanup(td);
+}
diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
new file mode 100644
index 000000000000..47a7592b7c53
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2019 ARM Limited */
+
+#ifndef __TEST_SIGNALS_UTILS_H__
+#define __TEST_SIGNALS_UTILS_H__
+
+#include "test_signals.h"
+
+int test_setup(struct tdescr *td);
+void test_cleanup(struct tdescr *td);
+int test_run(struct tdescr *td);
+void test_result(struct tdescr *td);
+
+static inline bool feats_ok(struct tdescr *td)
+{
+	return (td->feats_required & td->feats_supported) == td->feats_required;
+}
+
+#endif
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c
new file mode 100644
index 000000000000..2cb118b0ba05
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, toggling
+ * the execution state bit: this attempt must be spotted by Kernel and
+ * the test case is expected to be terminated via SEGV.
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+static int mangle_invalid_pstate_run(struct tdescr *td, siginfo_t *si,
+				     ucontext_t *uc)
+{
+	ASSERT_GOOD_CONTEXT(uc);
+
+	/* This config should trigger a SIGSEGV by Kernel */
+	uc->uc_mcontext.pstate ^= PSR_MODE32_BIT;
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.sanity_disabled = true,
+		.name = "MANGLE_PSTATE_INVALID_STATE_TOGGLE",
+		.descr = "Mangling uc_mcontext with INVALID STATE_TOGGLE",
+		.sig_trig = SIGUSR1,
+		.sig_ok = SIGSEGV,
+		.run = mangle_invalid_pstate_run,
+};
diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c
new file mode 100644
index 000000000000..1914a01222a1
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2019 ARM Limited */
+#include "testcases.h"
+
+struct _aarch64_ctx *get_header(struct _aarch64_ctx *head, uint32_t magic,
+				size_t resv_sz, size_t *offset)
+{
+	size_t offs = 0;
+	struct _aarch64_ctx *found = NULL;
+
+	if (!head || resv_sz < HDR_SZ)
+		return found;
+
+	while (offs <= resv_sz - HDR_SZ &&
+	       head->magic != magic && head->magic) {
+		offs += head->size;
+		head = GET_RESV_NEXT_HEAD(head);
+	}
+	if (head->magic == magic) {
+		found = head;
+		if (offset)
+			*offset = offs;
+	}
+
+	return found;
+}
+
+bool validate_extra_context(struct extra_context *extra, char **err)
+{
+	struct _aarch64_ctx *term;
+
+	if (!extra || !err)
+		return false;
+
+	fprintf(stderr, "Validating EXTRA...\n");
+	term = GET_RESV_NEXT_HEAD(extra);
+	if (!term || term->magic || term->size) {
+		*err = "Missing terminator after EXTRA context";
+		return false;
+	}
+	if (extra->datap & 0x0fUL)
+		*err = "Extra DATAP misaligned";
+	else if (extra->size & 0x0fUL)
+		*err = "Extra SIZE misaligned";
+	else if (extra->datap != (uint64_t)term + sizeof(*term))
+		*err = "Extra DATAP misplaced (not contiguos)";
+	if (*err)
+		return false;
+
+	return true;
+}
+
+bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err)
+{
+	bool terminated = false;
+	size_t offs = 0;
+	int flags = 0;
+	struct extra_context *extra = NULL;
+	struct _aarch64_ctx *head =
+		(struct _aarch64_ctx *)uc->uc_mcontext.__reserved;
+
+	if (!err)
+		return false;
+	/* Walk till the end terminator verifying __reserved contents */
+	while (head && !terminated && offs < resv_sz) {
+		if ((uint64_t)head & 0x0fUL) {
+			*err = "Misaligned HEAD";
+			return false;
+		}
+
+		switch (head->magic) {
+		case 0:
+			if (head->size)
+				*err = "Bad size for terminator";
+			else
+				terminated = true;
+			break;
+		case FPSIMD_MAGIC:
+			if (flags & FPSIMD_CTX)
+				*err = "Multiple FPSIMD_MAGIC";
+			else if (head->size !=
+				 sizeof(struct fpsimd_context))
+				*err = "Bad size for fpsimd_context";
+			flags |= FPSIMD_CTX;
+			break;
+		case ESR_MAGIC:
+			if (head->size != sizeof(struct esr_context))
+				*err = "Bad size for esr_context";
+			break;
+		case SVE_MAGIC:
+			if (flags & SVE_CTX)
+				*err = "Multiple SVE_MAGIC";
+			else if (head->size !=
+				 sizeof(struct sve_context))
+				*err = "Bad size for sve_context";
+			flags |= SVE_CTX;
+			break;
+		case EXTRA_MAGIC:
+			if (flags & EXTRA_CTX)
+				*err = "Multiple EXTRA_MAGIC";
+			else if (head->size !=
+				 sizeof(struct extra_context))
+				*err = "Bad size for extra_context";
+			flags |= EXTRA_CTX;
+			extra = (struct extra_context *)head;
+			break;
+		case KSFT_BAD_MAGIC:
+			/*
+			 * This is a BAD magic header defined
+			 * artificially by a testcase and surely
+			 * unknown to the Kernel parse_user_sigframe().
+			 * It MUST cause a Kernel induced SEGV
+			 */
+			*err = "BAD MAGIC !";
+			break;
+		default:
+			/*
+			 * A still unknown Magic: potentially freshly added
+			 * to the Kernel code and still unknown to the
+			 * tests.
+			 */
+			fprintf(stdout,
+				"SKIP Unknown MAGIC: 0x%X - Is KSFT arm64/signal up to date ?\n",
+				head->magic);
+			break;
+		}
+
+		if (*err)
+			return false;
+
+		offs += head->size;
+		if (resv_sz < offs + sizeof(*head)) {
+			*err = "HEAD Overrun";
+			return false;
+		}
+
+		if (flags & EXTRA_CTX)
+			if (!validate_extra_context(extra, err))
+				return false;
+
+		head = GET_RESV_NEXT_HEAD(head);
+	}
+
+	if (terminated && !(flags & FPSIMD_CTX)) {
+		*err = "Missing FPSIMD";
+		return false;
+	}
+
+	return true;
+}
diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.h b/tools/testing/selftests/arm64/signal/testcases/testcases.h
new file mode 100644
index 000000000000..04987f7870bc
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/testcases.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2019 ARM Limited */
+#ifndef __TESTCASES_H__
+#define __TESTCASES_H__
+
+#include <stddef.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <ucontext.h>
+#include <signal.h>
+
+/* Architecture specific sigframe definitions */
+#include <asm/sigcontext.h>
+
+#define FPSIMD_CTX	(1 << 0)
+#define SVE_CTX		(1 << 1)
+#define EXTRA_CTX	(1 << 2)
+
+#define KSFT_BAD_MAGIC	0xdeadbeef
+
+#define HDR_SZ \
+	sizeof(struct _aarch64_ctx)
+
+#define GET_SF_RESV_HEAD(sf) \
+	(struct _aarch64_ctx *)(&(sf).uc.uc_mcontext.__reserved)
+
+#define GET_SF_RESV_SIZE(sf) \
+	sizeof((sf).uc.uc_mcontext.__reserved)
+
+#define GET_UCP_RESV_SIZE(ucp) \
+	sizeof((ucp)->uc_mcontext.__reserved)
+
+#define ASSERT_BAD_CONTEXT(uc) do {					\
+	char *err = NULL;						\
+	if (!validate_reserved((uc), GET_UCP_RESV_SIZE((uc)), &err)) {	\
+		if (err)						\
+			fprintf(stderr,					\
+				"Using badly built context - ERR: %s\n",\
+				err);					\
+	} else {							\
+		abort();						\
+	}								\
+} while (0)
+
+#define ASSERT_GOOD_CONTEXT(uc) do {					 \
+	char *err = NULL;						 \
+	if (!validate_reserved((uc), GET_UCP_RESV_SIZE((uc)), &err)) {	 \
+		if (err)						 \
+			fprintf(stderr,					 \
+				"Detected BAD context - ERR: %s\n", err);\
+		abort();						 \
+	} else {							 \
+		fprintf(stderr, "uc context validated.\n");		 \
+	}								 \
+} while (0)
+
+/*
+ * A simple record-walker for __reserved area: it walks through assuming
+ * only to find a proper struct __aarch64_ctx header descriptor.
+ *
+ * Instead it makes no assumptions on the content and ordering of the
+ * records, any needed bounds checking must be enforced by the caller
+ * if wanted: this way can be used by caller on any maliciously built bad
+ * contexts.
+ *
+ * head->size accounts both for payload and header _aarch64_ctx size !
+ */
+#define GET_RESV_NEXT_HEAD(h) \
+	(struct _aarch64_ctx *)((char *)(h) + (h)->size)
+
+struct fake_sigframe {
+	siginfo_t	info;
+	ucontext_t	uc;
+};
+
+
+bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err);
+
+bool validate_extra_context(struct extra_context *extra, char **err);
+
+struct _aarch64_ctx *get_header(struct _aarch64_ctx *head, uint32_t magic,
+				size_t resv_sz, size_t *offset);
+
+static inline struct _aarch64_ctx *get_terminator(struct _aarch64_ctx *head,
+						  size_t resv_sz,
+						  size_t *offset)
+{
+	return get_header(head, 0, resv_sz, offset);
+}
+
+static inline void write_terminator_record(struct _aarch64_ctx *tail)
+{
+	if (tail) {
+		tail->magic = 0;
+		tail->size = 0;
+	}
+}
+#endif
-- 
2.17.1

[PATCH v6 03/11] kselftest: arm64: mangle_pstate_invalid_daif_bits

[Cristian Marussi]

Add a simple mangle testcase which messes with the ucontext_t from within
the signal handler, trying to set PSTATE DAIF bits to an invalid value
(masking everything). Expects SIGSEGV on test PASS.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v3 --> v4
- fixed commit message
- added testcase comment description
---
 .../mangle_pstate_invalid_daif_bits.c         | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c
new file mode 100644
index 000000000000..434b82597007
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, mangling the
+ * DAIF bits in an illegal manner: this attempt must be spotted by Kernel
+ * and the test case is expected to be terminated via SEGV.
+ *
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+static int mangle_invalid_pstate_run(struct tdescr *td, siginfo_t *si,
+				     ucontext_t *uc)
+{
+	ASSERT_GOOD_CONTEXT(uc);
+
+	/*
+	 * This config should trigger a SIGSEGV by Kernel when it checks
+	 * the sigframe consistency in valid_user_regs() routine.
+	 */
+	uc->uc_mcontext.pstate |= PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT;
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.sanity_disabled = true,
+		.name = "MANGLE_PSTATE_INVALID_DAIF_BITS",
+		.descr = "Mangling uc_mcontext with INVALID DAIF_BITS",
+		.sig_trig = SIGUSR1,
+		.sig_ok = SIGSEGV,
+		.run = mangle_invalid_pstate_run,
+};
-- 
2.17.1

[PATCH v6 04/11] kselftest: arm64: mangle_pstate_invalid_mode_el[123][ht]

[Cristian Marussi]

Add 6 simple mangle testcases that mess with the ucontext_t from within
the signal handler, trying to toggle PSTATE mode bits to trick the system
into switching to EL1/EL2/EL3 using both SP_EL0(t) and SP_ELx(h).
Expects SIGSEGV on test PASS.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v3 --> v4
- fixed commit message
- macroization
- splitted into 6 macro-ised testcases to address t/h SP selection modes
- added test description
---
 .../mangle_pstate_invalid_mode_el1h.c         | 15 ++++++++++
 .../mangle_pstate_invalid_mode_el1t.c         | 15 ++++++++++
 .../mangle_pstate_invalid_mode_el2h.c         | 15 ++++++++++
 .../mangle_pstate_invalid_mode_el2t.c         | 15 ++++++++++
 .../mangle_pstate_invalid_mode_el3h.c         | 15 ++++++++++
 .../mangle_pstate_invalid_mode_el3t.c         | 15 ++++++++++
 .../mangle_pstate_invalid_mode_template.h     | 28 +++++++++++++++++++
 7 files changed, 118 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h

diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c
new file mode 100644
index 000000000000..95f821abdf46
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, toggling
+ * the mode bit to escalate exception level: this attempt must be spotted
+ * by Kernel and the test case is expected to be termninated via SEGV.
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+#include "mangle_pstate_invalid_mode_template.h"
+
+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(1h);
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c
new file mode 100644
index 000000000000..cc222d8a618a
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, toggling
+ * the mode bit to escalate exception level: this attempt must be spotted
+ * by Kernel and the test case is expected to be termninated via SEGV.
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+#include "mangle_pstate_invalid_mode_template.h"
+
+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(1t);
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c
new file mode 100644
index 000000000000..2188add7d28c
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, toggling
+ * the mode bit to escalate exception level: this attempt must be spotted
+ * by Kernel and the test case is expected to be termninated via SEGV.
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+#include "mangle_pstate_invalid_mode_template.h"
+
+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(2h);
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c
new file mode 100644
index 000000000000..df32dd5a479c
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, toggling
+ * the mode bit to escalate exception level: this attempt must be spotted
+ * by Kernel and the test case is expected to be termninated via SEGV.
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+#include "mangle_pstate_invalid_mode_template.h"
+
+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(2t);
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c
new file mode 100644
index 000000000000..9e6829b7e5db
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, toggling
+ * the mode bit to escalate exception level: this attempt must be spotted
+ * by Kernel and the test case is expected to be termninated via SEGV.
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+#include "mangle_pstate_invalid_mode_template.h"
+
+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(3h);
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c
new file mode 100644
index 000000000000..5685a4f10d06
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Try to mangle the ucontext from inside a signal handler, toggling
+ * the mode bit to escalate exception level: this attempt must be spotted
+ * by Kernel and the test case is expected to be termninated via SEGV.
+ */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+#include "mangle_pstate_invalid_mode_template.h"
+
+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(3t);
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h
new file mode 100644
index 000000000000..f5bf1804d858
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Utility macro to ease definition of testcases toggling mode EL
+ */
+
+#define DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(_mode)		\
+									\
+static int mangle_invalid_pstate_run(struct tdescr *td, siginfo_t *si,	\
+				     ucontext_t *uc)			\
+{									\
+	ASSERT_GOOD_CONTEXT(uc);					\
+									\
+	uc->uc_mcontext.pstate &= ~PSR_MODE_MASK;			\
+	uc->uc_mcontext.pstate |= PSR_MODE_EL ## _mode;			\
+									\
+	return 1;							\
+}									\
+									\
+struct tdescr tde = {							\
+		.sanity_disabled = true,				\
+		.name = "MANGLE_PSTATE_INVALID_MODE_EL"#_mode,		\
+		.descr = "Mangling uc_mcontext INVALID MODE EL"#_mode,	\
+		.sig_trig = SIGUSR1,					\
+		.sig_ok = SIGSEGV,					\
+		.run = mangle_invalid_pstate_run,			\
+}
-- 
2.17.1

[PATCH v6 05/11] kselftest: arm64: mangle_pstate_ssbs_regs

[Cristian Marussi]

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;
 	/* 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>
 #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;
+	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
+	 *   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)
+		      :
+		      : "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.
+	 */
+	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");
+
+	/* 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);
+}
+
+struct tdescr tde = {
+		.sanity_disabled = true,
+		.name = "MANGLE_PSTATE_SSBS_REGS",
+		.descr = "Mangle uc_mcontext setting SSBS bit.(MUST PRESERVE)",
+		.feats_required = FEAT_SSBS,
+		.sig_trig = SIGUSR1,
+		.sig_unsupp = SIGILL,
+		.init = mangle_invalid_pstate_ssbs_init,
+		.run = mangle_invalid_pstate_ssbs_run,
+		.check_result = pstate_ssbs_bit_checks,
+};
-- 
2.17.1

[PATCH v6 06/11] kselftest: arm64: fake_sigreturn_bad_magic

[Cristian Marussi]

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
v3 --> v4
- fix commit
- fix signal.S, handle misalign requests too
- remove unneeded comments
- add signal.h include
- added get_starting_head() helper
- added test description
---
 tools/testing/selftests/arm64/signal/Makefile |  2 +-
 .../testing/selftests/arm64/signal/signals.S  | 64 +++++++++++++++++++
 .../arm64/signal/test_signals_utils.h         |  2 +
 .../testcases/fake_sigreturn_bad_magic.c      | 52 +++++++++++++++
 .../arm64/signal/testcases/testcases.c        | 46 +++++++++++++
 .../arm64/signal/testcases/testcases.h        |  4 ++
 6 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/signal/signals.S
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c

diff --git a/tools/testing/selftests/arm64/signal/Makefile b/tools/testing/selftests/arm64/signal/Makefile
index f78f5190e3d4..b497cfea4643 100644
--- a/tools/testing/selftests/arm64/signal/Makefile
+++ b/tools/testing/selftests/arm64/signal/Makefile
@@ -28,5 +28,5 @@ clean:
 # Common test-unit targets to build common-layout test-cases executables
 # Needs secondary expansion to properly include the testcase c-file in pre-reqs
 .SECONDEXPANSION:
-$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c $$@.c test_signals.h test_signals_utils.h testcases/testcases.h
+$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c signals.S $$@.c test_signals.h test_signals_utils.h testcases/testcases.h
 	$(CC) $(CFLAGS) $^ -o $@
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) */
+	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/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
index 3ad062af1fa7..3320a69fb2a7 100644
--- a/tools/testing/selftests/arm64/signal/test_signals_utils.h
+++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
@@ -115,4 +115,6 @@ static __always_inline bool get_current_context(struct tdescr *td,
 
 	return td->live_uc_valid;
 }
+
+int fake_sigreturn(void *sigframe, size_t sz, int misalign_bytes);
 #endif
diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
new file mode 100644
index 000000000000..8dc600a7d4fd
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Place a fake sigframe on the stack including a BAD Unknown magic
+ * record: on sigreturn Kernel must spot this attempt and the test
+ * case is expected to be terminated via SEGV.
+ */
+
+#include <signal.h>
+#include <ucontext.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+struct fake_sigframe sf;
+
+static int fake_sigreturn_bad_magic_run(struct tdescr *td,
+					siginfo_t *si, ucontext_t *uc)
+{
+	struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
+
+	/* just to fill the ucontext_t with something real */
+	if (!get_current_context(td, &sf.uc))
+		return 1;
+
+	/* need at least 2*HDR_SZ space: KSFT_BAD_MAGIC + terminator. */
+	head = get_starting_head(shead, HDR_SZ * 2, GET_SF_RESV_SIZE(sf), NULL);
+	if (!head)
+		return 0;
+
+	/*
+	 * use a well known NON existent bad magic...something
+	 * we should pretty sure won't be ever defined in Kernel
+	 */
+	head->magic = KSFT_BAD_MAGIC;
+	head->size = HDR_SZ;
+	write_terminator_record(GET_RESV_NEXT_HEAD(head));
+
+	ASSERT_BAD_CONTEXT(&sf.uc);
+	fake_sigreturn(&sf, sizeof(sf), 0);
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.name = "FAKE_SIGRETURN_BAD_MAGIC",
+		.descr = "Trigger a sigreturn with a sigframe with a bad magic",
+		.sig_ok = SIGSEGV,
+		.timeout = 3,
+		.run = fake_sigreturn_bad_magic_run,
+};
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.
+ */
+struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,
+				       size_t need_sz, size_t resv_sz,
+				       size_t *offset)
+{
+	size_t offs = 0;
+	struct _aarch64_ctx *head;
+
+	head = get_terminator(shead, resv_sz, &offs);
+	/* not found a terminator...no need to update offset if any */
+	if (!head)
+		return head;
+	if (resv_sz - offs < need_sz) {
+		fprintf(stderr, "Low on space:%zd. Discarding extra_context.\n",
+			resv_sz - offs);
+		head = get_header(shead, EXTRA_MAGIC, resv_sz, &offs);
+		if (!head || resv_sz - offs < need_sz) {
+			fprintf(stderr,
+				"Failed to reclaim space on sigframe.\n");
+			return NULL;
+		}
+	}
+
+	fprintf(stderr, "Available space:%zd\n", resv_sz - offs);
+	if (offset)
+		*offset = offs;
+	return head;
+}
diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.h b/tools/testing/selftests/arm64/signal/testcases/testcases.h
index 04987f7870bc..ad884c135314 100644
--- a/tools/testing/selftests/arm64/signal/testcases/testcases.h
+++ b/tools/testing/selftests/arm64/signal/testcases/testcases.h
@@ -97,4 +97,8 @@ static inline void write_terminator_record(struct _aarch64_ctx *tail)
 		tail->size = 0;
 	}
 }
+
+struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,
+				       size_t need_sz, size_t resv_sz,
+				       size_t *offset);
 #endif
-- 
2.17.1

[PATCH v6 07/11] kselftest: arm64: fake_sigreturn_bad_size_for_magic0

[Cristian Marussi]

Add a simple fake_sigreturn testcase which builds a ucontext_t with a
badly sized terminator record and place it onto the stack.
Expects a SIGSEGV on test PASS.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v5 --> v6
- removed unneeded locals
- removed unneeded timeout on failure
v3 --> v4
- fix commit
- add signal.h include
- using new get_starting_head() helper
- added test description
---
 .../fake_sigreturn_bad_size_for_magic0.c      | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c
new file mode 100644
index 000000000000..a44b88bfc81a
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Place a fake sigframe on the stack including a badly sized terminator
+ * record: on sigreturn Kernel must spot this attempt and the test case
+ * is expected to be terminated via SEGV.
+ */
+
+#include <signal.h>
+#include <ucontext.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+struct fake_sigframe sf;
+
+static int fake_sigreturn_bad_size_for_magic0_run(struct tdescr *td,
+						  siginfo_t *si, ucontext_t *uc)
+{
+	struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
+
+	/* just to fill the ucontext_t with something real */
+	if (!get_current_context(td, &sf.uc))
+		return 1;
+
+	/* at least HDR_SZ for the badly sized terminator. */
+	head = get_starting_head(shead, HDR_SZ, GET_SF_RESV_SIZE(sf), NULL);
+	if (!head)
+		return 0;
+
+	head->magic = 0;
+	head->size = HDR_SZ;
+	ASSERT_BAD_CONTEXT(&sf.uc);
+	fake_sigreturn(&sf, sizeof(sf), 0);
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.name = "FAKE_SIGRETURN_BAD_SIZE_FOR_TERMINATOR",
+		.descr = "Trigger a sigreturn using non-zero size terminator",
+		.sig_ok = SIGSEGV,
+		.timeout = 3,
+		.run = fake_sigreturn_bad_size_for_magic0_run,
+};
-- 
2.17.1

[PATCH v6 08/11] kselftest: arm64: fake_sigreturn_missing_fpsimd

[Cristian Marussi]

Add a simple fake_sigreturn testcase which builds a ucontext_t without
the required fpsimd_context and place it onto the stack.
Expects a SIGSEGV on test PASS.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v3 --> v4
- fix commit
- added signal.h
- added test description
---
 .../testcases/fake_sigreturn_missing_fpsimd.c | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c
new file mode 100644
index 000000000000..08ecd8073a1a
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Place a fake sigframe on the stack missing the mandatory FPSIMD
+ * record: on sigreturn Kernel must spot this attempt and the test
+ * case is expected to be terminated via SEGV.
+ */
+
+#include <stdio.h>
+#include <signal.h>
+#include <ucontext.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+struct fake_sigframe sf;
+
+static int fake_sigreturn_missing_fpsimd_run(struct tdescr *td,
+					     siginfo_t *si, ucontext_t *uc)
+{
+	size_t resv_sz, offset;
+	struct _aarch64_ctx *head = GET_SF_RESV_HEAD(sf);
+
+	/* just to fill the ucontext_t with something real */
+	if (!get_current_context(td, &sf.uc))
+		return 1;
+
+	resv_sz = GET_SF_RESV_SIZE(sf);
+	head = get_header(head, FPSIMD_MAGIC, resv_sz, &offset);
+	if (head && resv_sz - offset >= HDR_SZ) {
+		fprintf(stderr, "Mangling template header. Spare space:%zd\n",
+			resv_sz - offset);
+		/* Just overwrite fpsmid_context */
+		write_terminator_record(head);
+
+		ASSERT_BAD_CONTEXT(&sf.uc);
+		fake_sigreturn(&sf, sizeof(sf), 0);
+	}
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.name = "FAKE_SIGRETURN_MISSING_FPSIMD",
+		.descr = "Triggers a sigreturn with a missing fpsimd_context",
+		.sig_ok = SIGSEGV,
+		.timeout = 3,
+		.run = fake_sigreturn_missing_fpsimd_run,
+};
-- 
2.17.1

[PATCH v6 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd

[Cristian Marussi]

Add a simple fake_sigreturn testcase which builds a ucontext_t with
an anomalous additional fpsimd_context and place it onto the stack.
Expects a SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v5 --> v6
- removed unneeded locals
- avoid timeout on failure
v3 --> v4
- fix commit
- missing include
- using new get_starting_head() helper
- added test description
---
 .../fake_sigreturn_duplicated_fpsimd.c        | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
new file mode 100644
index 000000000000..f0cc34dac47c
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Place a fake sigframe on the stack including an additional FPSIMD
+ * record: on sigreturn Kernel must spot this attempt and the test
+ * case is expected to be terminated via SEGV.
+ */
+
+#include <signal.h>
+#include <ucontext.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+struct fake_sigframe sf;
+
+static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,
+						siginfo_t *si, ucontext_t *uc)
+{
+	struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
+
+	/* just to fill the ucontext_t with something real */
+	if (!get_current_context(td, &sf.uc))
+		return 1;
+
+	head = get_starting_head(shead, sizeof(struct fpsimd_context) + HDR_SZ,
+				 GET_SF_RESV_SIZE(sf), NULL);
+	if (!head)
+		return 0;
+
+	/* Add a spurios fpsimd_context */
+	head->magic = FPSIMD_MAGIC;
+	head->size = sizeof(struct fpsimd_context);
+	/* and terminate */
+	write_terminator_record(GET_RESV_NEXT_HEAD(head));
+
+	ASSERT_BAD_CONTEXT(&sf.uc);
+	fake_sigreturn(&sf, sizeof(sf), 0);
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.name = "FAKE_SIGRETURN_DUPLICATED_FPSIMD",
+		.descr = "Triggers a sigreturn including two fpsimd_context",
+		.sig_ok = SIGSEGV,
+		.timeout = 3,
+		.run = fake_sigreturn_duplicated_fpsimd_run,
+};
-- 
2.17.1

[PATCH v6 10/11] kselftest: arm64: fake_sigreturn_bad_size

[Cristian Marussi]

Add a simple fake_sigreturn testcase which builds a ucontext_t with a
badly sized header that causes a overrun in the __reserved area and
place it onto the stack. Expects a SIGSEGV on test PASS.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v5 --> v6
- avoid timeout on failure
v3 --> v4
- fix commit
- missing include
- using new get_starting_head() helper
- added test description
---
 .../testcases/fake_sigreturn_bad_size.c       | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c
new file mode 100644
index 000000000000..b3c362100666
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Place a fake sigframe on the stack including a bad record overflowing
+ * the __reserved space: on sigreturn Kernel must spot this attempt and
+ * the test case is expected to be terminated via SEGV.
+ */
+
+#include <signal.h>
+#include <ucontext.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+struct fake_sigframe sf;
+
+#define MIN_SZ_ALIGN	16
+
+static int fake_sigreturn_bad_size_run(struct tdescr *td,
+				       siginfo_t *si, ucontext_t *uc)
+{
+	size_t resv_sz, need_sz, offset;
+	struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
+
+	/* just to fill the ucontext_t with something real */
+	if (!get_current_context(td, &sf.uc))
+		return 1;
+
+	resv_sz = GET_SF_RESV_SIZE(sf);
+	/* at least HDR_SZ + bad sized esr_context needed */
+	need_sz = sizeof(struct esr_context) + HDR_SZ;
+	head = get_starting_head(shead, need_sz, resv_sz, &offset);
+	if (!head)
+		return 0;
+
+	/*
+	 * Use an esr_context to build a fake header with a
+	 * size greater then the free __reserved area minus HDR_SZ;
+	 * using ESR_MAGIC here since it is not checked for size nor
+	 * is limited to one instance.
+	 *
+	 * At first inject an additional normal esr_context
+	 */
+	head->magic = ESR_MAGIC;
+	head->size = sizeof(struct esr_context);
+	/* and terminate properly */
+	write_terminator_record(GET_RESV_NEXT_HEAD(head));
+	ASSERT_GOOD_CONTEXT(&sf.uc);
+
+	/*
+	 * now mess with fake esr_context size: leaving less space than
+	 * needed while keeping size value 16-aligned
+	 *
+	 * It must trigger a SEGV from Kernel on:
+	 *
+	 *	resv_sz - offset < sizeof(*head)
+	 */
+	/* at first set the maximum good 16-aligned size */
+	head->size = (resv_sz - offset - need_sz + MIN_SZ_ALIGN) & ~0xfUL;
+	/* plus a bit more of 16-aligned sized stuff */
+	head->size += MIN_SZ_ALIGN;
+	/* and terminate properly */
+	write_terminator_record(GET_RESV_NEXT_HEAD(head));
+	ASSERT_BAD_CONTEXT(&sf.uc);
+	fake_sigreturn(&sf, sizeof(sf), 0);
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.name = "FAKE_SIGRETURN_BAD_SIZE",
+		.descr = "Triggers a sigreturn with a overrun __reserved area",
+		.sig_ok = SIGSEGV,
+		.timeout = 3,
+		.run = fake_sigreturn_bad_size_run,
+};
-- 
2.17.1

[PATCH v6 11/11] kselftest: arm64: fake_sigreturn_misaligned_sp

[Cristian Marussi]

Add a simple fake_sigreturn testcase which places a valid sigframe on a
non-16 bytes aligned SP. Expects a SIGSEGV on test PASS.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v3 --> v4
- fix commit
- use new fake_sigreturn misalig_bytes params
- removed TODO
- added test description
---
 .../testcases/fake_sigreturn_misaligned_sp.c  | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_misaligned_sp.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_misaligned_sp.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_misaligned_sp.c
new file mode 100644
index 000000000000..1e089e66f9f3
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_misaligned_sp.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 ARM Limited
+ *
+ * Place a fake sigframe on the stack at a misaligned SP: on sigreturn
+ * Kernel must spot this attempt and the test case is expected to be
+ * terminated via SEGV.
+ */
+
+#include <signal.h>
+#include <ucontext.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+struct fake_sigframe sf;
+
+static int fake_sigreturn_misaligned_run(struct tdescr *td,
+					 siginfo_t *si, ucontext_t *uc)
+{
+	/* just to fill the ucontext_t with something real */
+	if (!get_current_context(td, &sf.uc))
+		return 1;
+
+	/* Forcing sigframe on misaligned SP (16 + 3) */
+	fake_sigreturn(&sf, sizeof(sf), 3);
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.name = "FAKE_SIGRETURN_MISALIGNED_SP",
+		.descr = "Triggers a sigreturn with a misaligned sigframe",
+		.sig_ok = SIGSEGV,
+		.timeout = 3,
+		.run = fake_sigreturn_misaligned_run,
+};
-- 
2.17.1

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Anders Roxell]

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

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Cristian Marussi]

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.

Cheers

Cristian


> Cheers,
> Anders
> 

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[shuah]

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

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Cristian Marussi]

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/0999c80cd639b78ae27c0674069d552833227564.1561386715.git.andreyknvl@google.com/

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.

Cheers

Cristian

> 
> thanks,
> -- Shuah
> 

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Dave Martin]

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).

[...]

Cheers
---Dave

Re: [PATCH v6 02/11] kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils

[Dave Martin]

On Tue, Sep 10, 2019 at 01:31:02pm +0100, Cristian Marussi wrote:
> Add some arm64/signal specific boilerplate and utility code to help
> further testcases' development.
> 
> Introduce also one simple testcase mangle_pstate_invalid_compat_toggle
> and some related helpers: it is a simple mangle testcase which messes
> with the ucontext_t from within the signal handler, trying to toggle
> PSTATE state bits to switch the system between 32bit/64bit execution
> state. Expects SIGSEGV on test PASS.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v5 --> v6
> - fix commit msg
> - feat_names is char const *const
> - better supported options check and reporting
> - removed critical asserts to avoid issues with NDEBUG
> - more robust get_header
> - fix validation for ESR_CONTEXT size
> - add more explicit comment in GET_RESV_NEXT_HEAD() macro
> - refactored default_handler()
> - feats_ok() now public
> - call always test_results() no matter the outcome of test_run()

[...]

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

[...]

> +static int test_init(struct tdescr *td)
> +{
> +	td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
> +	if (!td->minsigstksz)
> +		td->minsigstksz = MINSIGSTKSZ;
> +	fprintf(stderr, "Detected MINSTKSIGSZ:%d\n", td->minsigstksz);
> +
> +	if (td->feats_required) {
> +		td->feats_supported = 0;
> +		/*
> +		 * Checking for CPU required features using both the
> +		 * auxval and the arm64 MRS Emulation to read sysregs.
> +		 */
> +		if (getauxval(AT_HWCAP) & HWCAP_SSBS)
> +			td->feats_supported |= FEAT_SSBS;
> +		if (getauxval(AT_HWCAP) & HWCAP_CPUID) {
> +			uint64_t val = 0;
> +
> +			/* Uses MRS emulation to check capability */
> +			get_regval(SYS_ID_AA64MMFR1_EL1, val);
> +			if (ID_AA64MMFR1_EL1_PAN_SUPPORTED(val))
> +				td->feats_supported |= FEAT_PAN;
> +			/* Uses MRS emulation to check capability */
> +			get_regval(SYS_ID_AA64MMFR2_EL1, val);
> +			if (ID_AA64MMFR2_EL1_UAO_SUPPORTED(val))
> +				td->feats_supported |= FEAT_UAO;
> +		} else {
> +			fprintf(stderr,
> +				"HWCAP_CPUID NOT available. Mark ALL feats UNSUPPORTED.\n");

Nit: this message isn't strictly correct now: SSBS may still be detected
even if HWCAP_CPUID isn't present.

For simplicity I suggest to drop this fprintf() (and the containing
else { }, which is otherwise empty).

The following code reports what features are supported in any case, so
the user will be able to see what was detected.


The rest looks reasonable to me now, so with the above nit fixed:

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

[...]

Cheers
---Dave

Re: [PATCH v6 05/11] kselftest: arm64: mangle_pstate_ssbs_regs

[Dave Martin]

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

Re: [PATCH v6 06/11] kselftest: arm64: fake_sigreturn_bad_magic

[Dave Martin]

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

Re: [PATCH v6 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd

[Dave Martin]

On Tue, Sep 10, 2019 at 01:31:09pm +0100, Cristian Marussi wrote:
> Add a simple fake_sigreturn testcase which builds a ucontext_t with
> an anomalous additional fpsimd_context and place it onto the stack.
> Expects a SIGSEGV on test PASS.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v5 --> v6
> - removed unneeded locals
> - avoid timeout on failure
> v3 --> v4
> - fix commit
> - missing include
> - using new get_starting_head() helper
> - added test description
> ---
>  .../fake_sigreturn_duplicated_fpsimd.c        | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> 
> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> new file mode 100644
> index 000000000000..f0cc34dac47c
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 ARM Limited
> + *
> + * Place a fake sigframe on the stack including an additional FPSIMD
> + * record: on sigreturn Kernel must spot this attempt and the test
> + * case is expected to be terminated via SEGV.
> + */
> +
> +#include <signal.h>
> +#include <ucontext.h>
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +struct fake_sigframe sf;
> +
> +static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,
> +						siginfo_t *si, ucontext_t *uc)
> +{
> +	struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
> +
> +	/* just to fill the ucontext_t with something real */
> +	if (!get_current_context(td, &sf.uc))
> +		return 1;
> +
> +	head = get_starting_head(shead, sizeof(struct fpsimd_context) + HDR_SZ,
> +				 GET_SF_RESV_SIZE(sf), NULL);
> +	if (!head)
> +		return 0;
> +
> +	/* Add a spurios fpsimd_context */

Nit: spurious

With that,

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

[...]

Cheers
---Dave

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[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/0999c80cd639b78ae27c0674069d552833227564.1561386715.git.andreyknvl@google.com/
> 
> 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

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[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

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Dave Martin]

On Tue, Sep 17, 2019 at 10:18:55AM -0600, 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

Thanks!

From the arm64 side, I'd say we're just down to minor issues at this
point.

Cheers
---Dave

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Cristian Marussi]

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
> 

Re: [PATCH v6 02/11] kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils

[Cristian Marussi]

On 17/09/2019 17:05, Dave Martin wrote:
> On Tue, Sep 10, 2019 at 01:31:02pm +0100, Cristian Marussi wrote:
>> Add some arm64/signal specific boilerplate and utility code to help
>> further testcases' development.
>>
>> Introduce also one simple testcase mangle_pstate_invalid_compat_toggle
>> and some related helpers: it is a simple mangle testcase which messes
>> with the ucontext_t from within the signal handler, trying to toggle
>> PSTATE state bits to switch the system between 32bit/64bit execution
>> state. Expects SIGSEGV on test PASS.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v5 --> v6
>> - fix commit msg
>> - feat_names is char const *const
>> - better supported options check and reporting
>> - removed critical asserts to avoid issues with NDEBUG
>> - more robust get_header
>> - fix validation for ESR_CONTEXT size
>> - add more explicit comment in GET_RESV_NEXT_HEAD() macro
>> - refactored default_handler()
>> - feats_ok() now public
>> - call always test_results() no matter the outcome of test_run()
> 
> [...]
> 
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
> 
> [...]
> 
>> +static int test_init(struct tdescr *td)
>> +{
>> +	td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
>> +	if (!td->minsigstksz)
>> +		td->minsigstksz = MINSIGSTKSZ;
>> +	fprintf(stderr, "Detected MINSTKSIGSZ:%d\n", td->minsigstksz);
>> +
>> +	if (td->feats_required) {
>> +		td->feats_supported = 0;
>> +		/*
>> +		 * Checking for CPU required features using both the
>> +		 * auxval and the arm64 MRS Emulation to read sysregs.
>> +		 */
>> +		if (getauxval(AT_HWCAP) & HWCAP_SSBS)
>> +			td->feats_supported |= FEAT_SSBS;
>> +		if (getauxval(AT_HWCAP) & HWCAP_CPUID) {
>> +			uint64_t val = 0;
>> +
>> +			/* Uses MRS emulation to check capability */
>> +			get_regval(SYS_ID_AA64MMFR1_EL1, val);
>> +			if (ID_AA64MMFR1_EL1_PAN_SUPPORTED(val))
>> +				td->feats_supported |= FEAT_PAN;
>> +			/* Uses MRS emulation to check capability */
>> +			get_regval(SYS_ID_AA64MMFR2_EL1, val);
>> +			if (ID_AA64MMFR2_EL1_UAO_SUPPORTED(val))
>> +				td->feats_supported |= FEAT_UAO;
>> +		} else {
>> +			fprintf(stderr,
>> +				"HWCAP_CPUID NOT available. Mark ALL feats UNSUPPORTED.\n");
> 
> Nit: this message isn't strictly correct now: SSBS may still be detected
> even if HWCAP_CPUID isn't present.
> 
> For simplicity I suggest to drop this fprintf() (and the containing
> else { }, which is otherwise empty).
> 
> The following code reports what features are supported in any case, so
> the user will be able to see what was detected.
> 
> 
> The rest looks reasonable to me now, so with the above nit fixed:
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>

Thanks I'll do the above fixes in v7.

Cristian
> 
> [...]
> 
> Cheers
> ---Dave
> 

Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile

[Cristian Marussi]

Hi

On 17/09/2019 17:05, 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?
> 

I've left this bit as it is in v7 (fixing some other minor stuff) as we discussed,
since I'm still waiting for some Shuah's comments.

>> +
>> +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 (?)
> 
> 

Ok I'll fix in v7

> 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'll wait for Shuah's comments as said.

Thanks

Cristian
> [...]
> 
> Cheers
> ---Dave
> 

Re: [PATCH v6 05/11] kselftest: arm64: mangle_pstate_ssbs_regs

[Cristian Marussi]

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
>> 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.)
> 

Ok. I'll fix it in v7


>>  	/* 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().
> 

Yes intentional and wrong. I'll fix it in v7.

>>  #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?
> 

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.

>> +	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.


>> +	/* 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...)

> Cheers
> ---Dave
> 

Thanks

Cristian

Re: [PATCH v6 06/11] kselftest: arm64: fake_sigreturn_bad_magic

[Cristian Marussi]

On 17/09/2019 17:06, Dave Martin wrote:
> 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.
> 

Well the reason if that I was equally (but wrongly on my side) convinced
to have to sum +16 before clearing the lower 0b1111.

My bad. I'm fixing in v7.


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

Cheers

Cristian

Re: [PATCH v6 05/11] kselftest: arm64: mangle_pstate_ssbs_regs

[Dave Martin]

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