Re: [kvm-unit-tests PATCH v2 2/4] x86/access: CR0.WP toggling write to r/o data test

[Sean Christopherson <seanjc@google.com>]

On Mon, Apr 03, 2023, Mathias Krause wrote:
> On 31.03.23 18:20, Sean Christopherson wrote:
> > On Fri, Mar 31, 2023, Mathias Krause wrote:
> >> +	 * the access test and toggle EFER.NX to flush and rebuild the current
> >> +	 * MMU context based on that value.
> >> +	 */
> >> +
> >> +	set_cr0_wp(1);
> >> +	set_efer_nx(1);
> >> +	set_efer_nx(0);
> > 
> > Rather than copy+paste and end up with a superfluous for-loop, through the guts
> > of the test into a separate inner function, e.g.
> > 
> >   static int __check_toggle_cr0_wp(ac_test_t *at, bool cr0_wp_initially_set)
> > 
> > and then use @cr0_wp_initially_set to set/clear AC_CPU_CR0_WP_MASK.  And for the
> > printf(), check "at.flags & AC_CPU_CR0_WP_MASK" to determine whether the access
> > was expected to fault or succeed.  That should make it easy to test all the
> > combinations.
> 
> Well, I thought of a helper function too and folding the value of CR0.WP
> into the error message. But looking at other tests I got the impression,
> it's more important to make the test conditions more obvious than it is
> to write compact code. 

LOL, you're looking for reasoning/logic where there is none.  The sad truth is
that the vast majority of tests were "written" by copy+paste+tweak.

> This not only makes it easier to see what gets tested but also shows what the
> test was intended to do. If, instead, the error message is based on the value
> of AC_CPU_CR0_WP_MASK, one not only has to check what value it was set to but
> also look up what its meaning really is, like if set, does it mean CR0.WP=0
> or 1?

I generally agree with having self-documenting code and/or assertions, but that
only works to a certain point in tests, especially for tests that are verifying
architectural behavior.  Oftentimes I know what KUT code is doing, but it takes
far too much digging to understand what is actually expected to happen and why.
The architectural behavior cases are especially problematic because there are so
many details that aren't really captured in the test itself.

IMO, given how KUT is structured and what it is testing, documenting the exact
testscase is the role of the error message.  E.g. if the error message explicitly
states the test case, then it documents the code _and_ provides a helpful message
to allow users to quickly triage failures.  Of course, the vast majority of error
messages in KUT are awful IMO, and are a constant source of frustration, but one
can dream :-)