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

[Sean Christopherson <seanjc@google.com>]

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.