Message-ID: <diqzfrc41kns.fsf@google.com>
Date: Tue, 30 Sep 2025 07:09:27 +0000
From: Ackerley Tng <ackerleytng@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Christian Borntraeger <borntraeger@...ux.ibm.com>, 
	Janosch Frank <frankja@...ux.ibm.com>, Claudio Imbrenda <imbrenda@...ux.ibm.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, David Hildenbrand <david@...hat.com>, 
	Fuad Tabba <tabba@...gle.com>
Subject: Re: [PATCH 5/6] KVM: selftests: Add wrappers for mmap() and munmap()
 to assert success

Sean Christopherson <seanjc@...gle.com> writes:

> On Mon, Sep 29, 2025, Ackerley Tng wrote:
>> Sean Christopherson <seanjc@...gle.com> writes:
>> 
>> > Add and use wrappers for mmap() and munmap() that assert success to reduce
>> > a significant amount of boilerplate code, to ensure all tests assert on
>> > failure, and to provide consistent error messages on failure.
>> >
>> > No functional change intended.
>> >
>> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
>> > ---
>> >  .../testing/selftests/kvm/guest_memfd_test.c  | 21 +++------
>> >  .../testing/selftests/kvm/include/kvm_util.h  | 25 +++++++++++
>> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 44 +++++++------------
>> >  tools/testing/selftests/kvm/mmu_stress_test.c |  5 +--
>> >  .../selftests/kvm/s390/ucontrol_test.c        | 16 +++----
>> >  .../selftests/kvm/set_memory_region_test.c    | 17 ++++---
>> >  6 files changed, 64 insertions(+), 64 deletions(-)
>> >
>> > 
>> > [...snip...]
>> > 
>> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
>> > index 23a506d7eca3..1c68ff0fb3fb 100644
>> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
>> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
>> > @@ -278,6 +278,31 @@ static inline bool kvm_has_cap(long cap)
>> >  #define __KVM_SYSCALL_ERROR(_name, _ret) \
>> >  	"%s failed, rc: %i errno: %i (%s)", (_name), (_ret), errno, strerror(errno)
>> >  
>> > +static inline void *__kvm_mmap(size_t size, int prot, int flags, int fd,
>> > +			       off_t offset)
>> 
>> Do you have a policy/rationale for putting this in kvm_util.h as opposed
>> to test_util.h? I like the idea of this wrapper but I thought this is
>> less of a kvm thing and more of a test utility, and hence it belongs in
>> test_util.c and test_util.h.
>
> To be perfectly honest, I forgot test_util.h existed :-)
>

Merging/dropping one of kvm_util.h vs test_util.h is a good idea. The
distinction is not clear and it's already kind of messy between the two.

>> Also, the name kind of associates mmap with KVM too closely IMO, but
>> test_mmap() is not a great name either.
>
> Which file will hopefully be irrevelant, because ideally it'll be temporary (see
> below). But if someone has a strong opinion and/or better idea on the name prefix,
> I definitely want to settle on a name for syscall wrappers, because I want to go
> much further than just adding an mmap() wrapper.  I chose kvm_ because there's
> basically zero chance that will ever conflict with generic selftests functionality,
> and the wrappers utilize TEST_ASSERT(), which are unique to KVM selftests.
>
> As for why the current location will hopefully be temporary, and why I want to
> settle on a name, I have patches to add several more wrappers, along with
> infrastructure to make it super easy to add new wrappers.  When trying to sort
> out the libnuma stuff for Shivank's series[*], I discovered that KVM selftests
> already has a (very partial, very crappy) libnuma equivalent in
> tools/testing/selftests/kvm/include/numaif.h.
>
> Adding wrappers for NUMA syscalls became an exercise in frustration (so much
> uninteresting boilerplate, and I kept making silly mistakes), and so that combined
> with the desire for mmap() and munmap() wrappers motivated me to add a macro
> framework similar to the kernel's DEFINE_SYSCALL magic.
>
> So, I've got patches (that I'll post with the next version of the gmem NUMA
> series) that add tools/testing/selftests/kvm/include/kvm_syscalls.h, and
> __kvm_mmap() will be moved there (ideally it wouldn't move, but I want to land
> this small series in 6.18, and so wanted to keep the changes for 6.18 small-ish).
>
> For lack of a better namespace, and because we already have __KVM_SYSCALL_ERROR(),
> I picked KVM_SYSCALL_DEFINE() for the "standard" builder, e.g. libnuma equivalents,
> and then __KVM_SYSCALL_DEFINE() for a KVM selftests specific version to handle
> asserting success.
>

It's a common pattern in KVM selftests to have a syscall/ioctl wrapper
foo() that asserts defaults and a __foo() that doesn't assert anything
and allows tests to assert something else, but I have a contrary
opinion.

I think it's better that tests be explicit about what they're testing
for, so perhaps it's better to use macros like TEST_ASSERT_EQ() to
explicitly call a function and check the results.

Or perhaps it should be more explicit, like in the name, that an
assertion is made within this function?

In many cases a foo() exists without the corresponding __foo(), which
seems to be discouraging testing for error cases.

Also, I guess especially for vcpu_run(), tests would like to loop/take
different actions based on different errnos and then it gets a bit
unwieldy to have to avoid functions that have assertions within them.

I can see people forgetting to add TEST_ASSERT_EQ()s to check results of
setup/teardown functions but I think those errors would surface some
other way anyway.

Not a strongly-held opinion, and no major concerns on the naming
either. It's a selftest after all and IIUC we're okay to have selftest
interfaces change anyway?

> /* Define a kvm_<syscall>() API to assert success. */
> #define __KVM_SYSCALL_DEFINE(name, nr_args, args...)			\
> static inline void kvm_##name(DECLARE_ARGS(nr_args, args))		\
> {									\
> 	int r;								\
> 									\
> 	r = name(UNPACK_ARGS(nr_args, args));				\
> 	TEST_ASSERT(!r, __KVM_SYSCALL_ERROR(#name, r));			\
> }
>
> /*
>  * Macro to define syscall APIs, either because KVM selftests doesn't link to
>  * the standard library, e.g. libnuma, or because there is no library that yet
>  * provides the syscall.  These
>  */
> #define KVM_SYSCALL_DEFINE(name, nr_args, args...)			\
> static inline long name(DECLARE_ARGS(nr_args, args))			\
> {									\
> 	return syscall(__NR_##name, UNPACK_ARGS(nr_args, args));	\
> }									\
> __KVM_SYSCALL_DEFINE(name, nr_args, args)
>
>
> The usage looks like this (which is odd at first glance, but makes it trivially
> easy to copy+paste from the kernel SYSCALL_DEFINE invocations:
>
> KVM_SYSCALL_DEFINE(get_mempolicy, 5, int *, policy, const unsigned long *, nmask,
> 		   unsigned long, maxnode, void *, addr, int, flags);
>
> KVM_SYSCALL_DEFINE(set_mempolicy, 3, int, mode, const unsigned long *, nmask,
> 		   unsigned long, maxnode);
>
> KVM_SYSCALL_DEFINE(set_mempolicy_home_node, 4, unsigned long, start,
> 		   unsigned long, len, unsigned long, home_node,
> 		   unsigned long, flags);
>
> KVM_SYSCALL_DEFINE(migrate_pages, 4, int, pid, unsigned long, maxnode,
> 		   const unsigned long *, frommask, const unsigned long *, tomask);
>
> KVM_SYSCALL_DEFINE(move_pages, 6, int, pid, unsigned long, count, void *, pages,
> 		   const int *, nodes, int *, status, int, flags);
>
> KVM_SYSCALL_DEFINE(mbind, 6, void *, addr, unsigned long, size, int, mode,
> 		   const unsigned long *, nodemask, unsigned long, maxnode,
> 		   unsigned int, flags);
>
> __KVM_SYSCALL_DEFINE(munmap, 2, void *, mem, size_t, size);
> __KVM_SYSCALL_DEFINE(close, 1, int, fd);
> __KVM_SYSCALL_DEFINE(fallocate, 4, int, fd, int, mode, loff_t, offset, loff_t, len);
> __KVM_SYSCALL_DEFINE(ftruncate, 2, unsigned int, fd, off_t, length);
>
> [*] https://lore.kernel.org/all/0e986bdb-7d1b-4c14-932e-771a87532947@amd.com

Powered by blists - more mailing lists