Re: [kvm-unit-tests PATCH v4 6/9] x86/access: Try forced emulation for CR0.WP test as well

[Sean Christopherson <seanjc@google.com>]

On Wed, Apr 05, 2023, Mathias Krause wrote:
> On 05.04.23 16:29, Sean Christopherson wrote:
> > On Wed, Apr 05, 2023, Mathias Krause wrote:
> >> On 04.04.23 18:53, Sean Christopherson wrote:
> >>> @@ -1127,6 +1128,10 @@ static int check_toggle_cr0_wp(ac_pt_env_t *pt_env)
> >>>  
> >>>  	err += do_cr0_wp_access(&at, 0);
> >>>  	err += do_cr0_wp_access(&at, AC_CPU_CR0_WP_MASK);
> >>
> >>> +	if (!(invalid_mask & AC_FEP_MASK)) {
> >>
> >> Can we *please* change this back to 'if (is_fep_available()) {'...? I
> >> really would like to get these tests exercised by default if possible.
> > 
> > "by default" is a bit misleading IMO.  The vast majority of developers almost
> > certainly do not do testing with FEP enabled.
> 
> Fair enough. But with "by default if possible" I meant, if kvm.ko was
> already loaded with force_emulation_prefix=1, the CR0.WP access tests
> should automatically make use of it -- much like it's done in other
> tests, like x86/emulator.c, x86/emulator64.c and x86/pmu.c. Or do you
> want to change these tests to get a new "force_emulation" parameter as
> well and disable the automatic detection and usage of FEP support in
> tests completely?

In an ideal world, the access test would use FEP if it's available, i.e. not make
it a separate test case.  The unfortunate reality is that the runtime is simply
too long to do that for the test as a whole.

> > The goal is to reach a balance between the cost of maintenance, principle of least
> > surprise, and test coverage.  Ease of debugging also factors in (if the FEP version
> > fails but the non-FEP versions does not), but that's largely a bonus.
> 
> It's a bonus on the test coverage side, IMHO. If the FEP version fails
> but the non-FEP one doesn't, apparently something is broken somewhere
> and should be fixed.

For sure.  What I'm saying is that on my end, I can skip a non-trivial amount of
triage/debug if "access" passes but "access_fep" fails.  That info _should_ also
be captured in the log, but not all CI setups actually do that, e.g. the "official"
gitlab based CI doesn't capture logs (and it's a giant pain).  But again, this is
not a primary motivator, it's just a happy bonus.

> > Defining a @force_emulation but then ignoring it for a one-off test violates the
> > principle of least suprise.
> 
> Do we need additional parameters for PKU / SMEP / SMAP / LA57 as well or
> leave the automatic detection in place? </rhetorical question>
> 
> We only need the "force_emulation" parameter because the ac_test_bump()
> loop is so much slower with forced emulation. That's the only reason for
> it to exists. We can rename it to "full" and do the force emulation
> tests for ac_test_exec() if FEP is available. But just excluding some
> (cheap) tests because some command line argument wasn't provided would
> be surprising to me. Tests should be simple to use, IMO.

Look at it from the perspective of someone who has never run these tests and has
no clue what the test does, let alone the gory details of FEP.  The most straightforward
interpretation of force_emulation=false is that the test does not force emulation,
thus having a one-off testcase that forces emulation anyways would be surprising.

E.g. imagine being the person that discovered the VMX #PF test failed because of
the KVM bug where the emulator barfs on a NOP for L2.  Before they even get near
the KVM bug itself, the person would be wondering why on earth a NOP is getting a
#UD.  They would likely then discover FEP and ask the obvious question of why the
test is trying to use FEP even though forced emulation is allegedly disabled.

That can obviously be solved by comments to explain what "full" means, but as below,
I would prefer to avoid that if possible.

> > Plumbing a second param/flag into check_toggle_cr0_wp() would, IMO, unnecessarily
> > increase the maintenance cost.  Ditto for creating a more complex param.
> 
> Fully agree, no need for additional parameters. The existing one should
> simply be renamed to "full" and just control ac_test_exec()'s behavior.
>
> > I doubt most CI setups that run KUT enable FEP either.  And if
> > CI/developers do automatically enable FEP, I would be shocked/saddened if
> > adding an additional configuration is more difficult than overiding a
> > module param.  E.g. I will soon be modifying my scripts to do both.
> 
> Well, the force emulation access tests take a significant amount of time
> to run, so will likely be disabled for CI systems that run on a free
> tier basis. But do we need to disable the possibility to run the corner
> case test as well? I don't think so. If some CI system already takes the
> effort to manually load kvm.ko with force_emulation_prefix=1, it should
> get these additional cheap tests automatically instead of having the
> need to carry additional patches to get them.

If that ends up being the case then I'm totally fine tweaking the param to gate
only the main loop, but my preference is to wait until it's actually a real
problem.  I.e. take on more complexity, even though it is minor, if and only if
we really need to.