On Tue, Jul 26, 2022 at 06:15:31PM +0000, Sean Christopherson wrote: > On Sat, Jul 23, 2022, Andrew Jones wrote: > > On Fri, Jul 22, 2022 at 06:20:07PM +0000, Sean Christopherson wrote: > > > On Fri, Jul 22, 2022, Ricardo Koller wrote: > > > > What about dividing the changes in two. > > > > > > > > 1. Will add the struct to "__vm_create()" as part of this > > > > series, and then use it in this commit. There's only one user > > > > > > > > dirty_log_test.c: vm = __vm_create(mode, 1, extra_mem_pages); > > > > > > > > so that would avoid having to touch every test as part of this patchset. > > > > > > > > 2. I can then send another series to add support for all the other > > > > vm_create() functions. > > > > > > > > Alternatively, I can send a new series that does 1 and 2 afterwards. > > > > WDYT? > > > > > > Don't do #2, ever. :-) The intent of having vm_create() versus is __vm_create() > > > is so that tests that don't care about things like backing pages don't have to > > > pass in extra params. I very much want to keep that behavior, i.e. I don't want > > > to extend vm_create() at all. IMO, adding _anything_ is a slippery slope, e.g. > > > why are the backing types special enough to get a param, but thing XYZ isn't? > > > > > > Thinking more, the struct idea probably isn't going to work all that well. It > > > again puts the selftests into a state where it becomes difficult to control one > > > setting and ignore the rest, e.g. the dirty_log_test and anything else with extra > > > pages suddenly has to care about the backing type for page tables and code. > > > > > > Rather than adding a struct, what about extending the @mode param? We already > > > have vm_mem_backing_src_type, we just need a way to splice things together. There > > > are a total of four things we can control: primary mode, and then code, data, and > > > page tables backing types. > > > > > > So, turn @mode into a uint32_t and carve out 8 bits for each of those four "modes". > > > The defaults Just Work because VM_MEM_SRC_ANONYMOUS==0. > > > > Hi Sean, > > > > How about merging both proposals and turn @mode into a struct and pass > > around a pointer to it? Then, when calling something that requires @mode, > > if @mode is NULL, the called function would use vm_arch_default_mode() > > to get a pointer to the arch-specific default mode struct. > > One tweak: rather that use @NULL as a magic param, #define VM_MODE_DEFAULT to > point at a global struct, similar to what is already done for __aarch64__. > > E.g. > > __vm_create(VM_MODE_DEFAULT, nr_runnable_vcpus, 0); > > does a much better job of self-documenting its behavior than this: > > __vm_create(NULL, nr_runnable_vcpus, 0); > > > If a test needs to modify the parameters then it can construct a mode struct > > from scratch or start with a copy of the default. As long as all members of > > the struct representing parameters, such as backing type, have defaults > > mapped to zero for the struct members, then we shouldn't be adding any burden > > to users that don't care about other parameters (other than ensuring their > > @mode struct was zero initialized). > > I was hoping to avoid forcing tests to build a struct, but looking at all the > existing users, they either use for_each_guest_mode() or just pass VM_MODE_DEFAULT, > so in practice it's a complete non-issue. > > The page fault usage will likely be similar, e.g. programatically generate the set > of combinations to test. > > So yeah, let's try the struct approach. Thank you both for the suggestions. About to send v5 with the suggested changes, with a slight modification. V5 adds "struct kvm_vm_mem_params" which includes a "guest mode" field. The suggestion was to overload "guest mode". What I have doesn't change the meaning of "guest mode", and just keeps everything dealing with "guest mode" the same (like guest_mode.c). The main changes are: 1. adding a struct kvm_vm_mem_params that defines the memory layout: -struct kvm_vm *____vm_create(enum vm_guest_mode mode, uint64_t nr_pages); +struct kvm_vm *____vm_create(struct kvm_vm_mem_params *mem_params); 2. adding memslot vm->memslot.[code|pt|data] and change all allocators to use the right memslot, e.g.,: lib/elf should use the code memslot. 3. change the new page_fault_test.c setup_memslot() accordingly (much nicer). Let me know what you think. Thanks! Ricardo
