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@xxxxxxx>
> >>---
> >>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@xxxxxxx>
> >
> >(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
