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@xxxxxxxxxxxxxx
>
>
> > (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
