Re: [PATCH v2 12/20] KVM: selftests: Add APIs to override memory region types with custom memslots

[Sean Christopherson]

On Thu, Sep 10, 2026, Ritesh Harjani wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> 
> > Add and use APIs to override memory region types instead of open coding the
> > logic in tests, partly to dedup test code, but mostly to add some amount of
> > protection against conflicting overrides.  This will allow converting more
> > tests to override memory regions without taking on too much risk of bugs in
> > the long term, e.g. due to silently overriding a region multiple times.
> >
> > Deliberately "unroll" the loops in __vm_create() and the CMMA test's
> > create_main_memslot() to avoid filling all memslots in anticipation of
> > adding an "extra" memslot that is for test use and *only* test use, i.e.
> > shouldn't be configured by core library code.
> >
> > Converting tests to overriding memory regions will allow taking the region
> > type instead of the raw memslot in the physical page allocator APIs.
> >
> 
> May I suggest few changes in the naming of these APIs:
> 
> static inline void vm_override_mem_region(struct kvm_vm *vm,
> 					      enum kvm_mem_region_type type,
> 					      u32 slot)
> {
> 	TEST_ASSERT(vm->memslots[type] == KVM_INVALID_MEMSLOT,
> 		    "Memory region type '%u' was already overridden with slot=%u",
> 		    type, vm->memslots[type]);
> 
> 	vm->memslots[type] = slot;
> }
> 
> static inline void vm_override_add_mem_region_flags(struct kvm_vm *vm,
> 					    enum kvm_mem_region_type type,
> 					    enum vm_mem_backing_src_type src_type,
> 					    gpa_t gpa, u32 slot, u64 npages,
> 					    u32 flags)
> {
> 	vm_override_mem_region(vm, type, slot);
> 	vm_userspace_mem_region_add(vm, src_type, gpa, slot, npages, flags);
> }
> 
> static inline void vm_override_add_mem_region(struct kvm_vm *vm,
> 					  enum kvm_mem_region_type type,
> 					  enum vm_mem_backing_src_type src_type,
> 					  gpa_t gpa, u32 slot, u64 npages)
> {
> 	vm_override_add_mem_region_flags(vm, type, src_type, gpa, slot, npages, 0);
> }
> 
> Those "_add_" and "_flags" in the function names easily gives away
> the difference in the APIs, rather than differentiating via "__".

I am strongly against postfixes like "_flags".  We do use postfixes for the VM
creation APIs, e.g. vm_create_barebones(), vm_create_with_vcpus(), etc., but only
because there are so many possible combinations that differentiating through
underscores is completely infeasible (unless we forced callers to regurgitate
huge amounts of boilerplace and/or had an absurd number of params), and because
the collection of APIs is tree-like, as opposed to a single linear chain of APIs.

For this, there is a much more finite set of possibilities, and the set of APIs
is a direct liner chain (no underscores => __ => ____).

I agree that not capturing that {,__}vm_override_mem_region() adds a userspace
memory region isn't ideal, but due to the term "memory region" already being
somewhat overloaded, I don't want to have "add" in the name as I think that will
make it harder to differentiate between the "enum kvm_mem_region_type" APIs and
the "userspace memory region" APIs.

And IMO it's totally fine for a function name to express what the API does at a
higher level, without capturing the exact operations in explicit detail.  E.g.
__vm_create() (and even ____vm_create()) obviously does a lot more than literally
KVM_VM_CREATE.