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.
