Re: [PATCH v5 07/13] KVM: selftests: Change ____vm_create() to take struct kvm_vm_mem_params

[Sean Christopherson <seanjc@google.com>]

On Tue, Aug 30, 2022, Ricardo Koller wrote:
> On Mon, Aug 29, 2022 at 07:25:08PM +0200, Andrew Jones wrote:
> > On Tue, Aug 23, 2022 at 11:47:21PM +0000, Ricardo Koller wrote:
> > I feel we'll be revisiting this frequently when more and more region types
> > are desired. For example, Sean wants a read-only memory region for ucall
> > exits. How about putting a mem slot array in struct kvm_vm, defining an
> > enum to index it (which will expand), and then single helper function,
> > something like
> > 
> >  inline struct userspace_mem_region *
> >  vm_get_mem_region(struct kvm_vm *vm, enum memslot_type mst)
> >  {
> >     return memslot2region(vm, vm->memslots[mst]);
> >  }


> > 
> > > +
> > >  /* Minimum allocated guest virtual and physical addresses */
> > >  #define KVM_UTIL_MIN_VADDR		0x2000
> > >  #define KVM_GUEST_PAGE_TABLE_MIN_PADDR	0x180000
> > > @@ -637,19 +662,51 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> > >  			      vm_paddr_t paddr_min, uint32_t memslot);
> > >  vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm);
> > >  
> > > +#define MEM_PARAMS_MAX_MEMSLOTS 3
> > 
> > And this becomes MEMSLOT_MAX of the enum proposed above
> > 
> >  enum memslot_type {
> >      MEMSLOT_CODE,
> >      MEMSLOT_PT,
> >      MEMSLOT_DATA,

"memslot" is going to be confusing, e.g. MEMSLOT_MAX is some arbitrary selftests
constant that has no relationship to maximum number of memslots.

> >      MEMSLOT_MAX,

I dislike "max" because it's ambiguous, e.g. is it the maximum number of regions,
or is the max valid region?

Maybe something like this?

	enum kvm_mem_region_type {
		MEM_REGION_CODE
		...
		NR_MEM_REGIONS,
	}

> > > +#else
> > > +	.mode = VM_MODE_DEFAULT,
> > > +#endif
> > > +	.region[0] = {
> > > +		.src_type = VM_MEM_SRC_ANONYMOUS,
> > > +		.guest_paddr = 0,
> > > +		.slot = 0,
> > > +		/*
> > > +		 * 4mb when page size is 4kb. Note that vm_nr_pages_required(),
> > > +		 * the function used by most tests to calculate guest memory
> > > +		 * requirements uses around ~520 pages for more tests.
> > 
> > ...requirements, currently returns ~520 pages for the majority of tests.
> > 
> > > +		 */
> > > +		.npages = 1024,
> > 
> > And here we double it, but it's still fragile. I see we override this
> > in __vm_create() below though, so now I wonder why we set it at all.
> > 
> 
> I would prefer having a default that can be used by a test as-is. WDYT?
> or should we make it explicit that the default needs some updates?

In that case, the default should be '0'.  There are two users of ____vm_create().
__vm_create() takes the number of "extra" pages and calculates the "base" number
of pages.  vm_create_barebones() passes '0', i.e. can use default.

If '0' as a default is too weird, make it an illegal value and force the caller
to define the number of pages.

It's not a coincidence that those are the only two callers, ____vm_create() isn't
intended to be used directly.  If a test wants to create a VM just to create a VM,
then it shoulid use vm_create_barebones().  If a test wants to doing anything
remotely useful with the VM, it should use __vm_create() or something higher up
the food chain.