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

[Sean Christopherson <seanjc@google.com>]

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
>