On Tue, Sep 03, 2019 at 05:44:00PM +0000, Nadav Amit wrote:
> > On Sep 3, 2019, at 10:28 AM, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
> >
> > On Fri, Aug 30, 2019 at 01:40:30PM -0700, Nadav Amit wrote:
> >> Using test_skip() when multiple tests are run causes all the following
> >> tests to be skipped. Instead, just print a message and return.
> >>
> >> Fixes: 47cc3d85c2fe ("nVMX x86: Check PML and EPT on vmentry of L2 guests")
> >> Fixes: 7fd449f2ed2e ("nVMX x86: Check VPID value on vmentry of L2 guests")
> >> Fixes: 181219bfd76b ("x86: Add test for checking NMI controls on vmentry of L2 guests")
> >> Fixes: 1d70eb823e12 ("nVMX x86: Check EPTP on vmentry of L2 guests")
> >> Cc: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> >> Signed-off-by: Nadav Amit <namit@xxxxxxxxxx>
> >
> > invvpid_test_v2() also has a bunch of bad calls to test_skip().
>
> In the case of invvpid_test_v2() the use seems correct, as the call is not
> encapsulated within a group of tests. You want to skip all the tests if
> invvpid is not supported for some reason.
Ah, I misread the code, I was thinking the longjmp was headed out of the
loop on vmx_tests.
> > What about removing test_skip() entirely? The code for in_guest looks
> > suspect, e.g. at a glance it should use HYPERCALL_VMSKIP instead of
> > HYPERCALL_VMABORT. The only somewhat legit usage is the ept tests, and
> > only then because the ept tests are all at the end of the array.
> > Returning success/failure from ept_access_test_setup() seems like a
> > better solution than test_skip.
>
> I don’t know. test_skip() does seem “nice” in theory (as long as it is not
> used improperly).
Agreed after rereading the code.
> Having said that, the fact that it uses HYPERCALL_VMABORT
> does seem wrong. I think it should be a separate change though.
Definitely. I'll look at it when I get the chance.
