Re: [PATCH v7 07/13] KVM: selftests: Add vm->memslots[] and enum kvm_mem_region_type
On Tue, Sep 20, 2022 at 05:39:19PM +, 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_wa8fyd5ilbnxjo...@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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 07/13] KVM: selftests: Add vm->memslots[] and enum kvm_mem_region_type
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_wa8fyd5ilbnxjo...@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
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 07/13] KVM: selftests: Add vm->memslots[] and enum kvm_mem_region_type
On Tue, Sep 20, 2022 at 04:25:45AM +, 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 > 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. > > Fix the above issues by having selftests specify how they want memory to be > laid out. Start by changing vm_create() to not create memslot #0; a future > commit will add test that specifies all memslots used by the VM. Then, add > the > vm->memslots[] array to specify the right memslot for different memory > allocators, e.g.,: lib/elf should use the vm->[MEM_REGION_CODE] memslot. This > will be used as a way to specify the page-tables memslots (to be backed by > huge > pages for example). > > There is no functional change intended. The current commit lays out memory > exactly as before. The next commit will change the allocators to get the > region > they should be using, e.g.,: like the page table allocators using the pt > memslot. > > Cc: Sean Christopherson > Cc: Andrew Jones > Signed-off-by: Ricardo Koller > --- > .../selftests/kvm/include/kvm_util_base.h | 25 +-- > tools/testing/selftests/kvm/lib/kvm_util.c| 18 +++-- > 2 files changed, 33 insertions(+), 10 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v7 07/13] KVM: selftests: Add vm->memslots[] and enum kvm_mem_region_type
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
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.
Fix the above issues by having selftests specify how they want memory to be
laid out. Start by changing vm_create() to not create memslot #0; a future
commit will add test that specifies all memslots used by the VM. Then, add the
vm->memslots[] array to specify the right memslot for different memory
allocators, e.g.,: lib/elf should use the vm->[MEM_REGION_CODE] memslot. This
will be used as a way to specify the page-tables memslots (to be backed by huge
pages for example).
There is no functional change intended. The current commit lays out memory
exactly as before. The next commit will change the allocators to get the region
they should be using, e.g.,: like the page table allocators using the pt
memslot.
Cc: Sean Christopherson
Cc: Andrew Jones
Signed-off-by: Ricardo Koller
---
.../selftests/kvm/include/kvm_util_base.h | 25 +--
tools/testing/selftests/kvm/lib/kvm_util.c| 18 +++--
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h
b/tools/testing/selftests/kvm/include/kvm_util_base.h
index b2dbe253d4d0..26187b8a66c6 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -65,6 +65,13 @@ struct userspace_mem_regions {
DECLARE_HASHTABLE(slot_hash, 9);
};
+enum kvm_mem_region_type {
+ MEM_REGION_CODE,
+ MEM_REGION_PT,
+ MEM_REGION_DATA,
+ NR_MEM_REGIONS,
+};
+
struct kvm_vm {
int mode;
unsigned long type;
@@ -93,6 +100,13 @@ struct kvm_vm {
int stats_fd;
struct kvm_stats_header stats_header;
struct kvm_stats_desc *stats_desc;
+
+ /*
+* KVM region slots. These are the default memslots used by page
+* allocators, e.g., lib/elf uses the memslots[MEM_REGION_CODE]
+* memslot.
+*/
+ uint32_t memslots[NR_MEM_REGIONS];
};
@@ -105,6 +119,13 @@ struct kvm_vm {
struct userspace_mem_region *
memslot2region(struct kvm_vm *vm, uint32_t memslot);
+inline struct userspace_mem_region *
+vm_get_mem_region(struct kvm_vm *vm, enum kvm_mem_region_type mrt)
+{
+ assert(mrt < NR_MEM_REGIONS);
+ return memslot2region(vm, vm->memslots[mrt]);
+}
+
/* Minimum allocated guest virtual and physical addresses */
#define KVM_UTIL_MIN_VADDR 0x2000
#define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x18
@@ -643,13 +664,13 @@ vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm);
* __vm_create() does NOT create vCPUs, @nr_runnable_vcpus is used purely to
* calculate the amount of memory needed for per-vCPU data, e.g. stacks.
*/
-struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t nr_pages);
+struct kvm_vm *vm_create(enum vm_guest_mode mode);
struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus,
uint64_t nr_extra_pages);
static inline struct kvm_vm *vm_create_barebones(void)
{
- return vm_create(VM_MODE_DEFAULT, 0);
+ return vm_create(VM_MODE_DEFAULT);
}
static inline struct kvm_vm *vm_create(uint32_t nr_runnable_vcpus)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c
b/tools/testing/selftests/kvm/lib/kvm_util.c
index 5a9f080ff888..2e36d6323518 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -143,13 +143,10 @@ const struct vm_guest_mode_params vm_guest_mode_params[]
= {
_Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct
vm_guest_mode_params) == NUM_VM_MODES,
"Missing new mode params?");
-struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t nr_pages)
+struct kvm_vm *vm_create(enum vm_guest_mode mode)
{
struct kvm_vm *vm;
- pr_debug("%s: mode='%s' pages='%ld'\n", __func__,
-vm_guest_mode_string(mode), nr_pages);
-
vm = calloc(1, sizeof(*vm));
TEST_ASSERT(vm != NULL, "Insufficient Memory");
@@ -245,9 +242,6 @@ struct kvm_vm *vm_create(enum vm_guest_mode mode,
uint64_t nr_pages)
/* Allocate and setup memory for guest. */
vm->vpages_mapped = sparsebit_alloc();
- if (nr_pages != 0)
- vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
- 0, 0, nr_pages, 0);
return vm;
}
@@ -293,8 +287,16 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode,
uint32_t nr_runnable_vcpus,
uint64_t
