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 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.

> Runtime slowdown is no argument here, as that's only a whopping two
> emulated accesses.
> 
> What was the reason to exclude them? Less test coverage can't be it,
> right? ;)

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.

Defining a @force_emulation but then ignoring it for a one-off test violates the
principle of least suprise.

Plumbing a second param/flag into check_toggle_cr0_wp() would, IMO, unnecessarily
increase the maintenance cost.  Ditto for creating a more complex param.

As for test coverage side, I doubt that honoring @force_emulation reduces test
coverage in practice.  As above, most developers likely do not test with FEP.  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.