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? That would be quite counter-intuitive to reach a good
test coverage goal.
>
>> 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.
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.
>
> 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.
>
> 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.
>
> 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.
Well, I do ;)
> 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.
Thanks,
Mathias
