| Message ID | 20210210230625.550939-10-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VM: selftests: Hugepage fixes and cleanups | expand |
On Wed, Feb 10, 2021 at 3:06 PM Sean Christopherson <seanjc@google.com> wrote: > > Move the per-VM GPA into perf_test_args instead of storing it as a > separate global variable. It's not obvious that guest_test_phys_mem > holds a GPA, nor that it's connected/coupled with per_vcpu->gpa. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/include/perf_test_util.h | 8 +----- > .../selftests/kvm/lib/perf_test_util.c | 28 ++++++++----------- > .../kvm/memslot_modification_stress_test.c | 2 +- > 3 files changed, 13 insertions(+), 25 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h > index 4d53238b139f..cccf1c44bddb 100644 > --- a/tools/testing/selftests/kvm/include/perf_test_util.h > +++ b/tools/testing/selftests/kvm/include/perf_test_util.h > @@ -29,6 +29,7 @@ struct perf_test_vcpu_args { > struct perf_test_args { > struct kvm_vm *vm; > uint64_t host_page_size; > + uint64_t gpa; > uint64_t guest_page_size; > int wr_fract; > > @@ -37,13 +38,6 @@ struct perf_test_args { > > extern struct perf_test_args perf_test_args; > > -/* > - * Guest physical memory offset of the testing memory slot. > - * This will be set to the topmost valid physical address minus > - * the test memory size. > - */ > -extern uint64_t guest_test_phys_mem; > - > struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus, > uint64_t vcpu_memory_bytes, > enum vm_mem_backing_src_type backing_src); > diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c > index f22ce1836547..03f125236021 100644 > --- a/tools/testing/selftests/kvm/lib/perf_test_util.c > +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c > @@ -9,8 +9,6 @@ > > struct perf_test_args perf_test_args; > > -uint64_t guest_test_phys_mem; > - > /* > * Guest virtual memory offset of the testing memory slot. > * Must not conflict with identity mapped test code. > @@ -87,29 +85,25 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus, > TEST_ASSERT(guest_num_pages < vm_get_max_gfn(vm), > "Requested more guest memory than address space allows.\n" > " guest pages: %lx max gfn: %x vcpus: %d wss: %lx]\n", > - guest_num_pages, vm_get_max_gfn(vm), vcpus, > - vcpu_memory_bytes); > + guest_num_pages, vm_get_max_gfn(vm), vcpus, vcpu_memory_bytes); > > - guest_test_phys_mem = (vm_get_max_gfn(vm) - guest_num_pages) * > - pta->guest_page_size; > - guest_test_phys_mem &= ~(pta->host_page_size - 1); > + pta->gpa = (vm_get_max_gfn(vm) - guest_num_pages) * pta->guest_page_size; > + pta->gpa &= ~(pta->host_page_size - 1); Also not related to this patch, but another case for align. > if (backing_src == VM_MEM_SRC_ANONYMOUS_THP || > backing_src == VM_MEM_SRC_ANONYMOUS_HUGETLB) > - guest_test_phys_mem &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); > - > + pta->gpa &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); also align > #ifdef __s390x__ > /* Align to 1M (segment size) */ > - guest_test_phys_mem &= ~((1 << 20) - 1); > + pta->gpa &= ~((1 << 20) - 1); And here again (oof) > #endif > - pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem); > + pr_info("guest physical test memory offset: 0x%lx\n", pta->gpa); > > /* Add an extra memory slot for testing */ > - vm_userspace_mem_region_add(vm, backing_src, guest_test_phys_mem, > - PERF_TEST_MEM_SLOT_INDEX, > - guest_num_pages, 0); > + vm_userspace_mem_region_add(vm, backing_src, pta->gpa, > + PERF_TEST_MEM_SLOT_INDEX, guest_num_pages, 0); > > /* Do mapping for the demand paging memory slot */ > - virt_map(vm, guest_test_virt_mem, guest_test_phys_mem, guest_num_pages, 0); > + virt_map(vm, guest_test_virt_mem, pta->gpa, guest_num_pages, 0); > > ucall_init(vm, NULL); > > @@ -139,13 +133,13 @@ void perf_test_setup_vcpus(struct kvm_vm *vm, int vcpus, > (vcpu_id * vcpu_memory_bytes); > vcpu_args->pages = vcpu_memory_bytes / > pta->guest_page_size; > - vcpu_args->gpa = guest_test_phys_mem + > + vcpu_args->gpa = pta->gpa + > (vcpu_id * vcpu_memory_bytes); > } else { > vcpu_args->gva = guest_test_virt_mem; > vcpu_args->pages = (vcpus * vcpu_memory_bytes) / > pta->guest_page_size; > - vcpu_args->gpa = guest_test_phys_mem; > + vcpu_args->gpa = pta->gpa; > } > > pr_debug("Added VCPU %d with test mem gpa [%lx, %lx)\n", > diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c > index 6096bf0a5b34..569bb1f55bdf 100644 > --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c > +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c > @@ -121,7 +121,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) > > add_remove_memslot(vm, p->memslot_modification_delay, > p->nr_memslot_modifications, > - guest_test_phys_mem + > + perf_test_args.gpa + > (guest_percpu_mem_size * nr_vcpus) + > perf_test_args.host_page_size + > perf_test_args.guest_page_size); > -- > 2.30.0.478.g8a0d178c01-goog >
On Wed, Feb 10, 2021, Ben Gardon wrote: > > diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c > > index f22ce1836547..03f125236021 100644 > > --- a/tools/testing/selftests/kvm/lib/perf_test_util.c > > +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c > > @@ -9,8 +9,6 @@ > > > > struct perf_test_args perf_test_args; > > > > -uint64_t guest_test_phys_mem; > > - > > /* > > * Guest virtual memory offset of the testing memory slot. > > * Must not conflict with identity mapped test code. > > @@ -87,29 +85,25 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus, > > TEST_ASSERT(guest_num_pages < vm_get_max_gfn(vm), > > "Requested more guest memory than address space allows.\n" > > " guest pages: %lx max gfn: %x vcpus: %d wss: %lx]\n", > > - guest_num_pages, vm_get_max_gfn(vm), vcpus, > > - vcpu_memory_bytes); > > + guest_num_pages, vm_get_max_gfn(vm), vcpus, vcpu_memory_bytes); > > > > - guest_test_phys_mem = (vm_get_max_gfn(vm) - guest_num_pages) * > > - pta->guest_page_size; > > - guest_test_phys_mem &= ~(pta->host_page_size - 1); > > + pta->gpa = (vm_get_max_gfn(vm) - guest_num_pages) * pta->guest_page_size; > > + pta->gpa &= ~(pta->host_page_size - 1); > > Also not related to this patch, but another case for align. > > > if (backing_src == VM_MEM_SRC_ANONYMOUS_THP || > > backing_src == VM_MEM_SRC_ANONYMOUS_HUGETLB) > > - guest_test_phys_mem &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); > > - > > + pta->gpa &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); > > also align > > > #ifdef __s390x__ > > /* Align to 1M (segment size) */ > > - guest_test_phys_mem &= ~((1 << 20) - 1); > > + pta->gpa &= ~((1 << 20) - 1); > > And here again (oof) Yep, I'll fix all these and the align() comment in v2.
On 11/02/21 02:56, Sean Christopherson wrote: >>> + pta->gpa = (vm_get_max_gfn(vm) - guest_num_pages) * pta->guest_page_size; >>> + pta->gpa &= ~(pta->host_page_size - 1); >> Also not related to this patch, but another case for align. >> >>> if (backing_src == VM_MEM_SRC_ANONYMOUS_THP || >>> backing_src == VM_MEM_SRC_ANONYMOUS_HUGETLB) >>> - guest_test_phys_mem &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); >>> - >>> + pta->gpa &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); >> also align >> >>> #ifdef __s390x__ >>> /* Align to 1M (segment size) */ >>> - guest_test_phys_mem &= ~((1 << 20) - 1); >>> + pta->gpa &= ~((1 << 20) - 1); >> And here again (oof) > > Yep, I'll fix all these and the align() comment in v2. This is not exactly align in fact; it is x & ~y rather than (x + y) & ~y. Are you going to introduce a round-down macro or is it a bug? (I am lazy...). Paolo
On Thu, Feb 11, 2021, Paolo Bonzini wrote: > On 11/02/21 02:56, Sean Christopherson wrote: > > > > + pta->gpa = (vm_get_max_gfn(vm) - guest_num_pages) * pta->guest_page_size; > > > > + pta->gpa &= ~(pta->host_page_size - 1); > > > Also not related to this patch, but another case for align. > > > > > > > if (backing_src == VM_MEM_SRC_ANONYMOUS_THP || > > > > backing_src == VM_MEM_SRC_ANONYMOUS_HUGETLB) > > > > - guest_test_phys_mem &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); > > > > - > > > > + pta->gpa &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); > > > also align > > > > > > > #ifdef __s390x__ > > > > /* Align to 1M (segment size) */ > > > > - guest_test_phys_mem &= ~((1 << 20) - 1); > > > > + pta->gpa &= ~((1 << 20) - 1); > > > And here again (oof) > > > > Yep, I'll fix all these and the align() comment in v2. > > This is not exactly align in fact; it is x & ~y rather than (x + y) & ~y. > Are you going to introduce a round-down macro or is it a bug? (I am > lazy...). Good question. I, too, was lazy. I didn't look at the guts of align() when I moved it, and I didn't look closely at Ben's suggestion. I'll take a closer look today and make sure everything is doing what it's supposed to do.
On Thu, Feb 11, 2021 at 7:58 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Feb 11, 2021, Paolo Bonzini wrote: > > On 11/02/21 02:56, Sean Christopherson wrote: > > > > > + pta->gpa = (vm_get_max_gfn(vm) - guest_num_pages) * pta->guest_page_size; > > > > > + pta->gpa &= ~(pta->host_page_size - 1); > > > > Also not related to this patch, but another case for align. > > > > > > > > > if (backing_src == VM_MEM_SRC_ANONYMOUS_THP || > > > > > backing_src == VM_MEM_SRC_ANONYMOUS_HUGETLB) > > > > > - guest_test_phys_mem &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); > > > > > - > > > > > + pta->gpa &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); > > > > also align > > > > > > > > > #ifdef __s390x__ > > > > > /* Align to 1M (segment size) */ > > > > > - guest_test_phys_mem &= ~((1 << 20) - 1); > > > > > + pta->gpa &= ~((1 << 20) - 1); > > > > And here again (oof) > > > > > > Yep, I'll fix all these and the align() comment in v2. > > > > This is not exactly align in fact; it is x & ~y rather than (x + y) & ~y. > > Are you going to introduce a round-down macro or is it a bug? (I am > > lazy...). > > Good question. I, too, was lazy. I didn't look at the guts of align() when I > moved it, and I didn't look closely at Ben's suggestion. I'll take a closer > look today and make sure everything is doing what it's supposed to do. Ooh, great point Paolo, that helper is indeed rounding up. My comment in patch #2 was totally wrong. I forgot anyone would ever want to round up. :/ My misunderstanding and the above use cases are probably good evidence that it would be helpful to have both align_up and align_down helpers.
diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h index 4d53238b139f..cccf1c44bddb 100644 --- a/tools/testing/selftests/kvm/include/perf_test_util.h +++ b/tools/testing/selftests/kvm/include/perf_test_util.h @@ -29,6 +29,7 @@ struct perf_test_vcpu_args { struct perf_test_args { struct kvm_vm *vm; uint64_t host_page_size; + uint64_t gpa; uint64_t guest_page_size; int wr_fract; @@ -37,13 +38,6 @@ struct perf_test_args { extern struct perf_test_args perf_test_args; -/* - * Guest physical memory offset of the testing memory slot. - * This will be set to the topmost valid physical address minus - * the test memory size. - */ -extern uint64_t guest_test_phys_mem; - struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus, uint64_t vcpu_memory_bytes, enum vm_mem_backing_src_type backing_src); diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c index f22ce1836547..03f125236021 100644 --- a/tools/testing/selftests/kvm/lib/perf_test_util.c +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c @@ -9,8 +9,6 @@ struct perf_test_args perf_test_args; -uint64_t guest_test_phys_mem; - /* * Guest virtual memory offset of the testing memory slot. * Must not conflict with identity mapped test code. @@ -87,29 +85,25 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus, TEST_ASSERT(guest_num_pages < vm_get_max_gfn(vm), "Requested more guest memory than address space allows.\n" " guest pages: %lx max gfn: %x vcpus: %d wss: %lx]\n", - guest_num_pages, vm_get_max_gfn(vm), vcpus, - vcpu_memory_bytes); + guest_num_pages, vm_get_max_gfn(vm), vcpus, vcpu_memory_bytes); - guest_test_phys_mem = (vm_get_max_gfn(vm) - guest_num_pages) * - pta->guest_page_size; - guest_test_phys_mem &= ~(pta->host_page_size - 1); + pta->gpa = (vm_get_max_gfn(vm) - guest_num_pages) * pta->guest_page_size; + pta->gpa &= ~(pta->host_page_size - 1); if (backing_src == VM_MEM_SRC_ANONYMOUS_THP || backing_src == VM_MEM_SRC_ANONYMOUS_HUGETLB) - guest_test_phys_mem &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); - + pta->gpa &= ~(KVM_UTIL_HUGEPAGE_ALIGNMENT - 1); #ifdef __s390x__ /* Align to 1M (segment size) */ - guest_test_phys_mem &= ~((1 << 20) - 1); + pta->gpa &= ~((1 << 20) - 1); #endif - pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem); + pr_info("guest physical test memory offset: 0x%lx\n", pta->gpa); /* Add an extra memory slot for testing */ - vm_userspace_mem_region_add(vm, backing_src, guest_test_phys_mem, - PERF_TEST_MEM_SLOT_INDEX, - guest_num_pages, 0); + vm_userspace_mem_region_add(vm, backing_src, pta->gpa, + PERF_TEST_MEM_SLOT_INDEX, guest_num_pages, 0); /* Do mapping for the demand paging memory slot */ - virt_map(vm, guest_test_virt_mem, guest_test_phys_mem, guest_num_pages, 0); + virt_map(vm, guest_test_virt_mem, pta->gpa, guest_num_pages, 0); ucall_init(vm, NULL); @@ -139,13 +133,13 @@ void perf_test_setup_vcpus(struct kvm_vm *vm, int vcpus, (vcpu_id * vcpu_memory_bytes); vcpu_args->pages = vcpu_memory_bytes / pta->guest_page_size; - vcpu_args->gpa = guest_test_phys_mem + + vcpu_args->gpa = pta->gpa + (vcpu_id * vcpu_memory_bytes); } else { vcpu_args->gva = guest_test_virt_mem; vcpu_args->pages = (vcpus * vcpu_memory_bytes) / pta->guest_page_size; - vcpu_args->gpa = guest_test_phys_mem; + vcpu_args->gpa = pta->gpa; } pr_debug("Added VCPU %d with test mem gpa [%lx, %lx)\n", diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c index 6096bf0a5b34..569bb1f55bdf 100644 --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c @@ -121,7 +121,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) add_remove_memslot(vm, p->memslot_modification_delay, p->nr_memslot_modifications, - guest_test_phys_mem + + perf_test_args.gpa + (guest_percpu_mem_size * nr_vcpus) + perf_test_args.host_page_size + perf_test_args.guest_page_size);
Move the per-VM GPA into perf_test_args instead of storing it as a separate global variable. It's not obvious that guest_test_phys_mem holds a GPA, nor that it's connected/coupled with per_vcpu->gpa. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- .../selftests/kvm/include/perf_test_util.h | 8 +----- .../selftests/kvm/lib/perf_test_util.c | 28 ++++++++----------- .../kvm/memslot_modification_stress_test.c | 2 +- 3 files changed, 13 insertions(+), 25 deletions(-)