Re: [PATCH v4 8/9] KVM: selftests: Add rule to generate default tests for KVM selftests runner

From: Vipin Sharma <vipinsh@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev,
	 kvm-riscv@lists.infradead.org, pbonzini@redhat.com,
	borntraeger@linux.ibm.com,  frankja@linux.ibm.com,
	imbrenda@linux.ibm.com, anup@brainfault.org,
	 atish.patra@linux.dev, zhaotianrui@loongson.cn,
	maobibo@loongson.cn,  chenhuacai@kernel.org, maz@kernel.org,
	oliver.upton@linux.dev, ajones@ventanamicro.com
Subject: Re: [PATCH v4 8/9] KVM: selftests: Add rule to generate default tests for KVM selftests runner
Date: Thu, 20 Aug 2026 16:29:22 -0700	[thread overview]
Message-ID: <20260820220340.GC618052.vipinsh@google.com> (raw)
In-Reply-To: <ampPd9szwRoGKwds@google.com>

On Wed, Jul 29, 2026 at 12:07:35PM -0700, Sean Christopherson wrote:
> On Tue, Mar 31, 2026, Vipin Sharma wrote:
> > +# Default testcases for KVM selftests runner will be generated in this directory.
> > +DEFAULT_TESTCASES = testcases_default_gen
> 
> That's a cumbersome name.  It was also written by Yoda :-) How about default_testcases?
> 

No fun, you are!

> > +
> >  EXTRA_CLEAN += $(GEN_HDRS) \
> >  	       $(LIBKVM_OBJS) \
> >  	       $(SPLIT_TEST_GEN_OBJ) \
> >  	       $(TEST_DEP_FILES) \
> >  	       $(TEST_GEN_OBJ) \
> > +	       $(OUTPUT)/$(DEFAULT_TESTCASES) \
> >  	       cscope.*
> >  
> >  $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c $(GEN_HDRS)
> > @@ -363,3 +367,23 @@ cscope:
> >  	find . -name '*.c' \
> >  		-exec realpath --relative-base=$(PWD) {} \;) | sort -u > cscope.files
> >  	cscope -b
> > +
> > +# Generate runner testcases in DEFAULT_TESTCASES directory.
> > +# $(OUTPUT) is either CWD or specified in the make command.
> > +tests_install: list_progs = $(patsubst $(OUTPUT)/%,%,$(TEST_GEN_PROGS))
> > +tests_install:
> 
> This is very much *not* an install.  In fact, install is broken, because the
> runner isn't added to TEST_FILES.

I think this will also be not a good approach if we are moving towards
opt-in. I added another option in your FIXUP below.

> 
> Another issue with install is that it flattens the directory structures, i.e.
> drops the $ARCH/ subdirectories.  That causes issues for the default.test testcases
> due to them using partially qualified paths.
> 
> And coming back to this with fresh eyes, I don't love generating the testcases
> for the "normal" build.  It necessitates adding default.test to .gitignore,
> effectively requires copying the runner to the output directory, and is pure
> noise for folks that don't want to utilize the runner.

Yes, this is true.

> 
> Rather than generate testcases for the default build, what if we make this 100%
> opt-in, and take a hard dependency on install?  That should obviate the need for
> ignoring default.test, because if someone is silly enough to install testcases
> in the source tree, they get to deal with the noise.
> 
> And we can commit to having the default testcases provide completely unqualified
> paths, so that the Just Work with the flattened output.  If someone wants to run
> with an unflattened tree, then we should update the runner itself to handle
> multiple paths, a la the actual PATH variable.
> 
> The other nice thing is that we can make the runner opt-in, without requiring the
> user to run multiple make commands, by chaining testcases => install => all.  If
> we try to generate testcases as an optional step to the normal build, then we'll
> end up being able to run install without having generated the testcases, which
> is "fine", but probably not what we want for people that are trying to use the
> runner.
> 
> The obvious downside is that it requires doing an install to get the default
> testcases, but IMO that's an acceptable tradeoff.
> 

I like the opt-in approach. However, there is a subtle difference when
using 'testcases: install' vs 'all: tests_install'

Test cases can only be generated with

  make -C tools/testing/selftests/kvm testcases

We will not be able to generate test cases from the selftest directory
or the source root.

New command will be:

  make -C tools/testing/selftests/kvm testscases INSTALL_PATH=<path>

I think this is fine, considering it is opt-in and user has to know how
to run it. One advantage is that currently when a user runs:

  make -C tools/testing/selftests/kvm install INSTALL_PATH=<path>

They don't get run_kselftest.sh to run those tests. With the new rule,
they will have a runner to work with.

> > +	$(foreach tc, $(TEST_PROGS), \
> > +		$(shell mkdir -p $(OUTPUT)/$(DEFAULT_TESTCASES)/$(patsubst %.sh,%,$(tc))))
> > +	$(foreach tc, $(TEST_PROGS), \
> > +		$(shell echo $(tc) > $(patsubst %.sh,$(OUTPUT)/$(DEFAULT_TESTCASES)/%/default.test,$(tc))))
> > +
> > +	$(foreach tc, $(list_progs), \
> > +		$(shell mkdir -p $(OUTPUT)/$(DEFAULT_TESTCASES)/$(tc)))
> > +	$(foreach tc, $(list_progs), \
> > +		$(shell echo $(tc) > $(patsubst %,$(OUTPUT)/$(DEFAULT_TESTCASES)/%/default.test,$(tc))))
> > +
> > +	@if [ ! -d $(OUTPUT)/runner ]; then             \
> 
> This is very wrong, as this will fail to pick up any changes made to the runner.

yeah, I think rsync would have been better.

> 
> All in all, this as fixup?
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 83aa2fe01bac..91d2b21d396f 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -8,7 +8,6 @@
>  !*.S
>  !*.sh
>  !*.test
> -default.test
>  !.gitignore
>  !config
>  !settings
> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index c5821d495ea6..fc9dc41fdbeb 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -1,8 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  include ../../../build/Build.include
>  
> -all: tests_install
> -
>  LIBKVM += lib/assert.c
>  LIBKVM += lib/elf.c
>  LIBKVM += lib/guest_modes.c
> @@ -347,15 +345,13 @@ $(SPLIT_TEST_GEN_PROGS): $(OUTPUT)/%: $(OUTPUT)/%.o $(OUTPUT)/$(ARCH)/%.o
>  $(SPLIT_TEST_GEN_OBJ): $(OUTPUT)/$(ARCH)/%.o: $(ARCH)/%.c
>         $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
>  
> -# Default testcases for KVM selftests runner will be generated in this directory.
> -DEFAULT_TESTCASES = testcases_default_gen
> +TEST_FILES := $(selfdir)/kvm/runner

Downside of this one is that if someone is not planning to use runner they
will still get this directory copied.

Should we instead add 'rsync -u' in 'testcases' rule below which can copy
runner only when it is updated or not present? This would be similar to
original patch but instead of 'cp' it will be 'rsync' command. This will
also copy runner when someone is explicitly using it.

I think we should also include 'tests' directory which we are using for
custom testcases and they also .

>  
>  EXTRA_CLEAN += $(GEN_HDRS) \
>                $(LIBKVM_OBJS) \
>                $(SPLIT_TEST_GEN_OBJ) \
>                $(TEST_DEP_FILES) \
>                $(TEST_GEN_OBJ) \
> -              $(OUTPUT)/$(DEFAULT_TESTCASES) \
>                cscope.*
>  
>  $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c $(GEN_HDRS)
> @@ -386,21 +382,18 @@ cscope:
>         cscope -b
>  
>  # Generate runner testcases in DEFAULT_TESTCASES directory.
> -# $(OUTPUT) is either CWD or specified in the make command.
> -tests_install: list_progs = $(patsubst $(OUTPUT)/%,%,$(TEST_GEN_PROGS))
> -tests_install:
> -       $(foreach tc, $(TEST_PROGS), \
> -               $(shell mkdir -p $(OUTPUT)/$(DEFAULT_TESTCASES)/$(patsubst %.sh,%,$(tc))))
> -       $(foreach tc, $(TEST_PROGS), \
> -               $(shell echo $(tc) > $(patsubst %.sh,$(OUTPUT)/$(DEFAULT_TESTCASES)/%/default.test,$(tc))))
> +DEFAULT_TESTCASES = default_testcases
> +
> +testcases: list_progs = $(notdir $(patsubst $(OUTPUT)/%,%,$(TEST_GEN_PROGS)))
> +testcases: install
> +       $(foreach tc, $(notdir $(TEST_PROGS)), \
> +               $(shell mkdir -p $(INSTALL_PATH)/$(DEFAULT_TESTCASES)/$(notdir $(patsubst %.sh,%,$(tc)))))
> +       $(foreach tc, $(notdir $(TEST_PROGS)), \
> +               $(shell echo $(tc) > $(patsubst %.sh,$(INSTALL_PATH)/$(DEFAULT_TESTCASES)/%/default.test,$(tc))))
>  
>         $(foreach tc, $(list_progs), \
> -               $(shell mkdir -p $(OUTPUT)/$(DEFAULT_TESTCASES)/$(tc)))
> +               $(shell mkdir -p $(INSTALL_PATH)/$(DEFAULT_TESTCASES)/$(tc)))
>         $(foreach tc, $(list_progs), \
> -               $(shell echo $(tc) > $(patsubst %,$(OUTPUT)/$(DEFAULT_TESTCASES)/%/default.test,$(tc))))
> -
> -       @if [ ! -d $(OUTPUT)/runner ]; then             \
> -               cp -r $(selfdir)/kvm/runner $(OUTPUT);  \
> -       fi
> +               $(shell echo $(tc) > $(patsubst %,$(INSTALL_PATH)/$(DEFAULT_TESTCASES)/%/default.test,$(tc))))

All looks good here.

Should I send a next version of this series with the changes you have
suggested or you wanna take the diff you created and apply before merge?