On Fri, Jul 22, 2022 at 06:20:07PM +0000, Sean Christopherson wrote: > On Fri, Jul 22, 2022, Ricardo Koller wrote: > > On Thu, Jul 21, 2022 at 01:29:34AM +0000, Sean Christopherson wrote: > > > If we don't care, and maybe even if we do, then my preference would be to > > > enhance the __vm_create family of helpers to allow for specifying what > > > backing type should be used for page tables, i.e. associate the info the VM > > > instead of passing it around the stack. > > > > > > One idea would be to do something like David Matlack suggested a while back > > > and replace extra_mem_pages with a struct, e.g. struct kvm_vm_mem_params > > > That struct can then provide the necessary knobs to control how memory is > > > allocated. And then the lib can provide a global > > > > > > struct kvm_vm_mem_params kvm_default_vm_mem_params; > > > > > > > I like this idea, passing the info at vm creation. > > > > 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. 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). Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
