Re: [PATCH v4 09/13] KVM: selftests: aarch64: Add aarch64/page_fault_test

[Ricardo Koller <ricarkol@google.com>]

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