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.
