[PATCH] KVM: selftests: Depend on kernel headers when making selftests

[PATCH] KVM: selftests: Depend on kernel headers when making selftests

[Colton Lewis]

Add a makefile dependency for the kernel headers when building KVM
selftests. As the selftests do depend on the kernel headers, needing
to do things such as build selftests for a different architecture
requires rebuilding the headers. That is an extra annoying manual step
that should be handled by make.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
---

This change has been sitting in my local git repo as a stach for a
long time to make it easier to build selftests for multiple
architectures and I just realized I never sent it upstream. I don't
know if this is the best way to accomplish the goal, but it was the
only obvious one I found.

 tools/testing/selftests/kvm/Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 1750f91dd936..857c6f78e4da 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -221,12 +221,16 @@ LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ) $(LIBKVM_STRING_OBJ)
 EXTRA_CLEAN += $(LIBKVM_OBJS) cscope.*

 x := $(shell mkdir -p $(sort $(dir $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ))))
-$(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c
+$(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c headers
 	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@

 $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S
 	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@

+PHONY += headers
+headers:
+	$(MAKE) -C $(top_srcdir) headers
+
 # Compile the string overrides as freestanding to prevent the compiler from
 # generating self-referential code, e.g. without "freestanding" the compiler may
 # "optimize" memcmp() by invoking memcmp(), thus causing infinite recursion.
--
2.39.2.637.g21b0678d19-goog

Re: [PATCH] KVM: selftests: Depend on kernel headers when making selftests

[Sean Christopherson]

On Mon, Feb 20, 2023, Colton Lewis wrote:
> Add a makefile dependency for the kernel headers when building KVM
> selftests. As the selftests do depend on the kernel headers, needing
> to do things such as build selftests for a different architecture
> requires rebuilding the headers. That is an extra annoying manual step
> that should be handled by make.

I 100% agree that this is super annoying, but auto-installation of header was
deliberately removed[1][2] (this isn't the first time this type of change has been
proposed[3]) to play nice with an out-of-tree build directory.

Argh, digging a bit deeper, KVM selftests' makefile is all kinds of funky.  It
defines its own header path, LINUX_HDR_PATH, but then also grabs KHDR_INCLUDE
since commit 0cc5963b4cc3 ("selftests: kvm: Add the uapi headers include variable").
That's flawed because KVM selftests will pick up the in-tree headers first, i.e.
out-of-tree builds effectively rely on the in-tree headers not being built.  I'll
send the below as a formal patch.

As for auto-installing headers, for those of us that build directly from the KVM
selftests directory, the best option I've come up with is to add an alias+script
for building selftests and have that do the header installation.

---
 tools/testing/selftests/kvm/Makefile | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 844417601618..01ea083a2cd9 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -186,8 +186,6 @@ LIBKVM += $(LIBKVM_$(ARCH_DIR))
 # which causes the environment variable to override the makefile).
 include ../lib.mk
 
-INSTALL_HDR_PATH = $(top_srcdir)/usr
-LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/
 LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include
 ifeq ($(ARCH),x86_64)
 LINUX_TOOL_ARCH_INCLUDE = $(top_srcdir)/tools/arch/x86/include
@@ -198,9 +196,8 @@ CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \
 	-Wno-gnu-variable-sized-type-not-at-end \
 	-fno-builtin-memcmp -fno-builtin-memcpy -fno-builtin-memset \
 	-fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \
-	-I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \
-	-I$(<D) -Iinclude/$(ARCH_DIR) -I ../rseq -I.. $(EXTRA_CFLAGS) \
-	$(KHDR_INCLUDES)
+	-I$(LINUX_TOOL_ARCH_INCLUDE) -Iinclude -I$(<D) -Iinclude/$(ARCH_DIR) \
+	-I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
 
 no-pie-option := $(call try-run, echo 'int main(void) { return 0; }' | \
         $(CC) -Werror $(CFLAGS) -no-pie -x c - -o "$$TMP", -no-pie)
@@ -238,7 +235,7 @@ x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
 $(TEST_GEN_PROGS): $(LIBKVM_OBJS)
 $(TEST_GEN_PROGS_EXTENDED): $(LIBKVM_OBJS)
 
-cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib ..
+cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(top_srcdir)/usr/include include lib ..
 cscope:
 	$(RM) cscope.*
 	(find $(include_paths) -name '*.h' \

base-commit: c57c5ac42480250deb22b306ec6bcbdf0e6b5857
-- 

[1] https://lore.kernel.org/lkml/cover.1657296695.git.guillaume.tucker@collabora.com
[2] https://lore.kernel.org/all/b39b9e0b-45f3-1818-39fe-58921182d957@linuxfoundation.org
[3] https://lore.kernel.org/all/20221219095540.52208-1-likexu@tencent.com

Re: [PATCH] KVM: selftests: Depend on kernel headers when making selftests

[Shuah Khan]

On 2/20/23 13:35, Colton Lewis wrote:
> Add a makefile dependency for the kernel headers when building KVM
> selftests. As the selftests do depend on the kernel headers, needing
> to do things such as build selftests for a different architecture
> requires rebuilding the headers. That is an extra annoying manual step
> that should be handled by make.
> 

It does if you use the kselftest-* targets from the main Makefile:

# Kernel selftest

PHONY += kselftest
kselftest: headers
         $(Q)$(MAKE) -C $(srctree)/tools/testing/selftests run_tests

kselftest-%: headers FORCE
         $(Q)$(MAKE) -C $(srctree)/tools/testing/selftests $*

You can use the following commands to just build what you want to build:
make kselftest-all TARGETS=kvm

I would like to see the above used to make kselftest easier to maintain
and easier to add features.

If you choose to build from the test directory - then you do have
to manually run headers_install.


> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
> 
> This change has been sitting in my local git repo as a stach for a
> long time to make it easier to build selftests for multiple
> architectures and I just realized I never sent it upstream. I don't
> know if this is the best way to accomplish the goal, but it was the
> only obvious one I found.
> 
>   tools/testing/selftests/kvm/Makefile | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 1750f91dd936..857c6f78e4da 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -221,12 +221,16 @@ LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ) $(LIBKVM_STRING_OBJ)
>   EXTRA_CLEAN += $(LIBKVM_OBJS) cscope.*
> 
>   x := $(shell mkdir -p $(sort $(dir $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ))))
> -$(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c
> +$(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c headers
>   	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
> 
>   $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S
>   	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
> 
> +PHONY += headers
> +headers:
> +	$(MAKE) -C $(top_srcdir) headers
> +

This will install headers under the repo and doesn't work with out
of tree builds.

thanks,
-- Shuah