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

[Ricardo Koller <ricarkol@google.com>]

On Tue, Aug 30, 2022 at 10:49:11PM +0000, Sean Christopherson wrote:
> 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.

Sounds good, will try '0', and will make sure that not defining the
number of pages results in a clear error.

> 
> 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.

Thank you both for the reviews. I think this is ready for a v6, so
sending one soon.