[PATCH 1/2] KVM: selftests: Add default testfiles for KVM selftests runner

[Vipin Sharma]

On 2025-04-30 10:01:29, Sean Christopherson wrote:
> On Fri, Feb 21, 2025, Vipin Sharma wrote:
> We definitely auto-generate the default testcases.  Relying on the user to remember
> to add a testcase, and on the maintainer to remember to check for that, isn't a
> winning strategy.

Good idea, this will help in at least automatically run the default tests.
Also, if tests are in the selftest kvm source directory then it will
give an hint to developer/maintainer that they missed writing a default
test file for a newly introduced test when they run git status.

> 
> But, I do think we should commit the default.test files to the repository.  If
> they're ephemeral, then several problems arise:
> 
>  1. For out-of-tree builds, the default.test files should arguably be placed in
>     the OUTPUT directory.  But if/when we add curated testcases/, then we'll either
>     end up with multiple testcases/ directories (source and output), or we'll have
>     to copy testcases/ from the source to the output on a normal build, which is
>     rather gross.  Or we'd need e.g. "make testcases", which is also gross, e.g.
>     I don't want to have to run yet more commands just to execute tests.
> 
>  2. Generating default.test could overwrite a user-defined file.  That's firmly
>     a user error, but at least if they default.test files are commited, the user
>     will get a hint or three that they're doing things wrong.
> 
>  3. If the files aren't committed, then they probably should removed on "clean",
>     which isn't the end of the world since they're trivially easy to generate,
>     but it's kinda funky. 
> 
> So, what if we add this to auto-generate the files?  It's obviously wasteful since
> the files will exist 99.9999999% of the time, but the overhead is completely
> negligible.  The only concern I have is if this will do the wrong thing for some
> build environments, i.e. shove the files in the wrong location.

We can get the current path of the Makefile.kvm by writing this at the top
of the Makefile.kvm:
	MAKEFILE_DIR := $(dir $(realpath $(lastword $(MAKEFILE_LIST))))

Then MAKEFILE_DIR will have the source directory of Makfile.kvm and
testcase will be in the same directory.

With this we can modify the below foreach you wrote by prefixing
MAKEFILE_DIR to "testcases".

Does this alleviate concern regaring build environment?

> 
> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index f62b0a5aba35..d94bb8330ad1 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -198,6 +198,13 @@ TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH))
>  TEST_GEN_PROGS_EXTENDED += $(TEST_GEN_PROGS_EXTENDED_$(ARCH))
>  LIBKVM += $(LIBKVM_$(ARCH))
>  
> +$(foreach tc, $(TEST_PROGS), $(shell mkdir -p testcases/$(patsubst %.sh,%,$(tc))))
> +$(foreach tc, $(TEST_GEN_PROGS), $(shell mkdir -p testcases/$(tc)))
> +$(foreach tc, $(TEST_PROGS), \
> +  $(shell echo $(tc) > $(patsubst %.sh,testcases/%/default.test,$(tc))))
> +$(foreach tc, $(TEST_GEN_PROGS), \
> +  $(shell echo $(tc) > $(patsubst %,testcases/%/default.test,$(tc))))
> +

This looks good.

> > diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> > index 1d41a046a7bf..550b7c2b4a0c 100644
> > --- a/tools/testing/selftests/kvm/.gitignore
> > +++ b/tools/testing/selftests/kvm/.gitignore
> > @@ -9,4 +9,5 @@
> >  !config
> >  !settings
> >  !Makefile
> > -!Makefile.kvm
> > \ No newline at end of file
> > +!Makefile.kvm
> > +!*.test
> 
> Let's keep the extension wildcards sorted alphabetically, i.e.:
> 
> # SPDX-License-Identifier: GPL-2.0-only
> *
> !/**/
> !*.c
> !*.h
> !*.py
> !*.S
> !*.sh
> !*.test
> !.gitignore
> !config
> !settings
> !Makefile
> !Makefile.kvm
> 

Okay.

> > diff --git a/tools/testing/selftests/kvm/testcases/aarch64/aarch32_id_regs/default.test b/tools/testing/selftests/kvm/testcases/aarch64/aarch32_id_regs/default.test
> > new file mode 100644
> > index 000000000000..5db8723f554f
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/testcases/aarch64/aarch32_id_regs/default.test
> > @@ -0,0 +1 @@
> > +./aarch64/aarch32_id_regs
> 
> I don't see any reason to make the paths directly consumable with a leading "./".
> Spoiler alert from the next patch: the location of the test executables needs to
> explicitly resolved by the test runner.

I will remove this.