| Message ID | 20190830204031.3100-2-namit@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: nVMX: Bug fixes | expand |
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@oracle.com> > Signed-off-by: Nadav Amit <namit@vmware.com> invvpid_test_v2() also has a bunch of bad calls to test_skip(). 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. > --- > x86/vmx_tests.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index f035f24..4ff1570 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -4040,7 +4040,7 @@ static void test_vpid(void) > > if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) && > (ctrl_cpu_rev[1].clr & CPU_VPID))) { > - test_skip("Secondary controls and/or VPID not supported"); > + printf("Secondary controls and/or VPID not supported\n"); > return; > } > > @@ -4544,7 +4544,7 @@ static void test_nmi_ctrls(void) > > if ((ctrl_pin_rev.clr & (PIN_NMI | PIN_VIRT_NMI)) != > (PIN_NMI | PIN_VIRT_NMI)) { > - test_skip("NMI exiting and Virtual NMIs are not supported !"); > + printf("NMI exiting and Virtual NMIs are not supported !\n"); > return; > } > > @@ -4657,7 +4657,7 @@ static void test_ept_eptp(void) > > if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) && > (ctrl_cpu_rev[1].clr & CPU_EPT))) { > - test_skip("\"CPU secondary\" and/or \"enable EPT\" execution controls are not supported !"); > + printf("\"CPU secondary\" and/or \"enable EPT\" execution controls are not supported !\n"); > return; > } > > @@ -4844,7 +4844,7 @@ static void test_pml(void) > > if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) && > (ctrl_cpu_rev[1].clr & CPU_EPT) && (ctrl_cpu_rev[1].clr & CPU_PML))) { > - test_skip("\"Secondary execution\" control or \"enable EPT\" control or \"enable PML\" control is not supported !"); > + printf("\"Secondary execution\" control or \"enable EPT\" control or \"enable PML\" control is not supported !\n"); > return; > } > > -- > 2.17.1 >
> On Sep 3, 2019, at 10:28 AM, Sean Christopherson <sean.j.christopherson@intel.com> 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@oracle.com> >> Signed-off-by: Nadav Amit <namit@vmware.com> > > 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. > 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). Having said that, the fact that it uses HYPERCALL_VMABORT does seem wrong. I think it should be a separate change though.
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@intel.com> 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@oracle.com> > >> Signed-off-by: Nadav Amit <namit@vmware.com> > > > > 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.
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index f035f24..4ff1570 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -4040,7 +4040,7 @@ static void test_vpid(void) if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) && (ctrl_cpu_rev[1].clr & CPU_VPID))) { - test_skip("Secondary controls and/or VPID not supported"); + printf("Secondary controls and/or VPID not supported\n"); return; } @@ -4544,7 +4544,7 @@ static void test_nmi_ctrls(void) if ((ctrl_pin_rev.clr & (PIN_NMI | PIN_VIRT_NMI)) != (PIN_NMI | PIN_VIRT_NMI)) { - test_skip("NMI exiting and Virtual NMIs are not supported !"); + printf("NMI exiting and Virtual NMIs are not supported !\n"); return; } @@ -4657,7 +4657,7 @@ static void test_ept_eptp(void) if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) && (ctrl_cpu_rev[1].clr & CPU_EPT))) { - test_skip("\"CPU secondary\" and/or \"enable EPT\" execution controls are not supported !"); + printf("\"CPU secondary\" and/or \"enable EPT\" execution controls are not supported !\n"); return; } @@ -4844,7 +4844,7 @@ static void test_pml(void) if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) && (ctrl_cpu_rev[1].clr & CPU_EPT) && (ctrl_cpu_rev[1].clr & CPU_PML))) { - test_skip("\"Secondary execution\" control or \"enable EPT\" control or \"enable PML\" control is not supported !"); + printf("\"Secondary execution\" control or \"enable EPT\" control or \"enable PML\" control is not supported !\n"); return; }
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@oracle.com> Signed-off-by: Nadav Amit <namit@vmware.com> --- x86/vmx_tests.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)