| Message ID | 20220310164532.1821490-1-bgardon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: x86: Add a cap to disable NX hugepages on a VM | expand |
On Thu, Mar 10, 2022, Ben Gardon wrote: > selftests: KVM: Wrap memslot IDs in a struct for readability > selftests: KVM: Add memslot parameter to VM vaddr allocation > selftests: KVM: Add memslot parameter to elf_load I really, really, don't want to go down this path of proliferating memslot crud throughout the virtual memory allocators. I would much rather we solve this by teaching the VM creation helpers to (optionally) use hugepages. The amount of churn required just so that one test can back code with hugepages is absurd, and there's bound to be tests in the future that want to force hugepages as well.
On Thu, Mar 10, 2022 at 11:58 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Mar 10, 2022, Ben Gardon wrote: > > selftests: KVM: Wrap memslot IDs in a struct for readability > > selftests: KVM: Add memslot parameter to VM vaddr allocation > > selftests: KVM: Add memslot parameter to elf_load > > I really, really, don't want to go down this path of proliferating memslot crud > throughout the virtual memory allocators. I would much rather we solve this by > teaching the VM creation helpers to (optionally) use hugepages. The amount of > churn required just so that one test can back code with hugepages is absurd, and > there's bound to be tests in the future that want to force hugepages as well. I agree that proliferating the memslots argument isn't strictly required for this test, but doing so makes it much easier to make assertions about hugepage counts and such because you don't have your stacks and page tables backed with hugepages. Those patches are a lot of churn, but at least to me, they make the code much more readable. Currently there are many functions which just pass along 0 for the memslot, and often have multiple other numerical arguments, which makes it hard to understand what the function is doing. I don't think explicitly specifying memslots really adds that much overhead to the tests, and I'd rather have control over that than implicitly cramming everything into memslot 0. If you have a better way to manage the memslots and create virtual mappings for / load code into other memslots, I'm open to it, but we should do something about it.
On Thu, Mar 10, 2022, Ben Gardon wrote: > Those patches are a lot of churn, but at least to me, they make the > code much more readable. Currently there are many functions which just > pass along 0 for the memslot, and often have multiple other numerical > arguments, which makes it hard to understand what the function is > doing. Yeah, my solution for that was to rip out all the params. E.g. the most used function I ended up with is static inline struct kvm_vm *vm_create_with_one_vcpu(struct kvm_vcpu **vcpu, void *guest_code) { return __vm_create_with_one_vcpu(vcpu, 0, guest_code); } and then the usage is vm = vm_create_with_one_vcpu(&vcpu, guest_main); supp_cpuid = kvm_get_supported_cpuid(); cpuid2 = vcpu_get_cpuid(vcpu); My overarching complaint with the selftests is that they make the hard things hard, and the easy things harder. If a test wants to be backed by hugepages, it shouldn't have to manually specify a memslot. Let me post my selftests rework as RFC (_very_ RFC at this point). I was hoping to do more than compile test before posting anything, but it's going to be multiple weeks before I'll get back to it. Hopefully it'll start a discussion on actually rewriting the framework so that writing new tests is less painful, and so that every new thing that comes along doesn't require poking at 50 different tests.
Okay, I'll hold off on the memslot refactor in v2, but if folks have feedback on the other aspects of the v1 patch series, I'd appreciate it. If not, I'll try to get a v2 sent out. I think that the commits adding utility functions for the binary stats interface to the binary stats test could be queued separately from the rest of this series and will be helpful for other folks working on new selftests. On Thu, Mar 10, 2022 at 8:19 PM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Mar 10, 2022, Ben Gardon wrote: > > Those patches are a lot of churn, but at least to me, they make the > > code much more readable. Currently there are many functions which just > > pass along 0 for the memslot, and often have multiple other numerical > > arguments, which makes it hard to understand what the function is > > doing. > > Yeah, my solution for that was to rip out all the params. E.g. the most used > function I ended up with is > > static inline struct kvm_vm *vm_create_with_one_vcpu(struct kvm_vcpu **vcpu, > void *guest_code) > { > return __vm_create_with_one_vcpu(vcpu, 0, guest_code); > } > > and then the usage is > > vm = vm_create_with_one_vcpu(&vcpu, guest_main); > > supp_cpuid = kvm_get_supported_cpuid(); > cpuid2 = vcpu_get_cpuid(vcpu); > > My overarching complaint with the selftests is that they make the hard things hard, > and the easy things harder. If a test wants to be backed by hugepages, it shouldn't > have to manually specify a memslot. > > Let me post my selftests rework as RFC (_very_ RFC at this point). I was hoping to > do more than compile test before posting anything, but it's going to be multiple > weeks before I'll get back to it. Hopefully it'll start a discussion on actually > rewriting the framework so that writing new tests is less painful, and so that every > new thing that comes along doesn't require poking at 50 different tests.