Re: [PATCH v7 07/13] KVM: selftests: Add vm->memslots[] and enum kvm_mem_region_type

[Ricardo Koller <ricarkol@google.com>]

On Tue, Sep 20, 2022 at 05:39:19PM +0000, Sean Christopherson wrote:
> On Tue, Sep 20, 2022, Ricardo Koller wrote:
> > The vm_create() helpers are hardcoded to place most page types (code,
> > page-tables, stacks, etc) in the same memslot #0, and always backed with
> > anonymous 4K.  There are a couple of issues with that.  First, tests willing to
> 
> Preferred kernel style is to wrap changelogs at ~75 chars, e.g. so that `git show`
> stays under 80 chars.
> 
> And in general, please incorporate checkpatch into your workflow, e.g. there's
> also a spelling mistake below.
> 
>   WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
>   #9: 
>   anonymous 4K.  There are a couple of issues with that.  First, tests willing to
> 
>   WARNING: 'spreaded' may be misspelled - perhaps 'spread'?
>   #12: 
>   the hardcoded assumption of memslot #0 holding most things is spreaded
>                                                               ^^^^^^^^
> 
>   total: 0 errors, 2 warnings, 94 lines checked
> 
> > differ a bit, like placing page-tables in a different backing source type must
> > replicate much of what's already done by the vm_create() functions.  Second,
> > the hardcoded assumption of memslot #0 holding most things is spreaded
> > everywhere; this makes it very hard to change.
> 
> ...
> 
> > @@ -105,6 +119,13 @@ struct kvm_vm {
> >  struct userspace_mem_region *
> >  memslot2region(struct kvm_vm *vm, uint32_t memslot);
> >  
> > +inline struct userspace_mem_region *
> 
> Should be static inline.
> 
> > +vm_get_mem_region
> 
> Please don't insert newlines before the function name, it makes searching painful.
> Ignore existing patterns in KVM selfetsts, they're wrong.  ;-)  Linus has a nice
> explanation/rant on this[*].
> 
> The resulting declaration will run long, but at least for me, I'll take that any
> day over putting the function name on a new line.
> 
> [*] https://lore.kernel.org/all/CAHk-=wjoLAYG446ZNHfg=GhjSY6nFmuB_wA8fYd5iLBNXjo9Bw@mail.gmail.com
> 
> 
> > (struct kvm_vm *vm, enum kvm_mem_region_type mrt)
> 
> One last nit, what about "region" or "type" instead of "mrt"?  The acronym made me
> briefly pause to figure out what "mrt" meant, which is silly because the name really
> doesn't have much meaning.
> 
> > +{
> > +	assert(mrt < NR_MEM_REGIONS);
> > +	return memslot2region(vm, vm->memslots[mrt]);
> > +}
> 
> ...
> 
> > @@ -293,8 +287,16 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus,
> >  	uint64_t nr_pages = vm_nr_pages_required(mode, nr_runnable_vcpus,
> >  						 nr_extra_pages);
> >  	struct kvm_vm *vm;
> > +	int i;
> > +
> > +	pr_debug("%s: mode='%s' pages='%ld'\n", __func__,
> > +		 vm_guest_mode_string(mode), nr_pages);
> > +
> > +	vm = ____vm_create(mode);
> > +	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, 0);
> 
> The spacing is weird here.  Adding the region and stuffing vm->memslots are what
> should be bundled together, not creating the VM and adding the common region.  I.e.
> 
> 	pr_debug("%s: mode='%s' pages='%ld'\n", __func__,
> 		 vm_guest_mode_string(mode), nr_pages);
> 
> 	vm = ____vm_create(mode);
> 
> 	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, 0);
> 	for (i = 0; i < NR_MEM_REGIONS; i++)
> 		vm->memslots[i] = 0;
> 
> >  
> > -	vm = ____vm_create(mode, nr_pages);
> > +	for (i = 0; i < NR_MEM_REGIONS; i++)
> > +		vm->memslots[i] = 0;
> >  
> >  	kvm_vm_elf_load(vm, program_invocation_name);
> >  
> > -- 
> > 2.37.3.968.ga6b4b080e4-goog
> > 

Ack on all the above. Will send a v8 later today.

Thanks!
Ricardo