[kvm-unit-tests PATCH 0/2] x86: nVMX: Bug fixes

[kvm-unit-tests PATCH 0/2] x86: nVMX: Bug fixes

[Nadav Amit]

Two bug fixes that were found while running on bare-metal.

The first one caused the second bug not to be found until now.

The second bug is an SDM bug, and requires KVM to be fixed as well
(consider it as a bug report on behalf of VMware).

Cc: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Cc: Marc Orr <marcorr@google.com>

Nadav Amit (2):
  x86: nVMX: Do not use test_skip() when multiple tests are run
  x86: nVMX: Fix wrong reserved bits of error-code

 x86/vmx_tests.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

-- 
2.17.1

[kvm-unit-tests PATCH 1/2] x86: nVMX: Do not use test_skip() when multiple tests are run

[Nadav Amit]

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(-)

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

[kvm-unit-tests PATCH 2/2] x86: nVMX: Fix wrong reserved bits of error-code

[Nadav Amit]

The SDM indeed says that "If deliver-error-code is 1, bits 31:15 of the
VM-entry exception error-code field are 0." However, the SDM is wrong,
and bits that need to be zeroed are 31:16.

Our engineers confirmed that the SDM is wrong with Intel. Fix the test.

Note that KVM should be fixed as well.

Fixes: 8d2cdb35a07a ("x86: Add test for nested VM entry prereqs")
Cc: Marc Orr <marcorr@google.com>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 x86/vmx_tests.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 4ff1570..37c56df 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4306,15 +4306,15 @@ skip_unrestricted_guest:
 
 	/*
 	 * If deliver-error-code is 1
-	 * bits 31:15 of the VM-entry exception error-code field are 0.
+	 * bits 31:16 of the VM-entry exception error-code field are 0.
 	 */
 	ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK |
 			INTR_TYPE_HARD_EXCEPTION | GP_VECTOR;
 	report_prefix_pushf("%s, VM-entry intr info=0x%x",
-			    "VM-entry exception error code[31:15] clear",
+			    "VM-entry exception error code[31:16] clear",
 			    ent_intr_info);
 	vmcs_write(ENT_INTR_INFO, ent_intr_info);
-	for (cnt = 15; cnt <= 31; cnt++) {
+	for (cnt = 16; cnt <= 31; cnt++) {
 		ent_intr_err = 1U << cnt;
 		report_prefix_pushf("VM-entry intr error=0x%x [-]",
 				    ent_intr_err);
-- 
2.17.1

Re: [kvm-unit-tests PATCH 2/2] x86: nVMX: Fix wrong reserved bits of error-code

[Sean Christopherson]

On Fri, Aug 30, 2019 at 01:40:31PM -0700, Nadav Amit wrote:
> The SDM indeed says that "If deliver-error-code is 1, bits 31:15 of the
> VM-entry exception error-code field are 0." However, the SDM is wrong,
> and bits that need to be zeroed are 31:16.
> 
> Our engineers confirmed that the SDM is wrong with Intel. Fix the test.
> 
> Note that KVM should be fixed as well.
> 
> Fixes: 8d2cdb35a07a ("x86: Add test for nested VM entry prereqs")
> Cc: Marc Orr <marcorr@google.com>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

Re: [kvm-unit-tests PATCH 1/2] x86: nVMX: Do not use test_skip() when multiple tests are run

[Sean Christopherson]

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
> 

Re: [kvm-unit-tests PATCH 1/2] x86: nVMX: Do not use test_skip() when multiple tests are run

[Nadav Amit]

> 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.

Re: [kvm-unit-tests PATCH 1/2] x86: nVMX: Do not use test_skip() when multiple tests are run

[Sean Christopherson]

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.

Re: [kvm-unit-tests PATCH 0/2] x86: nVMX: Bug fixes

[Paolo Bonzini]

On 30/08/19 22:40, Nadav Amit wrote:
> Two bug fixes that were found while running on bare-metal.
> 
> The first one caused the second bug not to be found until now.
> 
> The second bug is an SDM bug, and requires KVM to be fixed as well
> (consider it as a bug report on behalf of VMware).
> 
> Cc: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Cc: Marc Orr <marcorr@google.com>
> 
> Nadav Amit (2):
>   x86: nVMX: Do not use test_skip() when multiple tests are run
>   x86: nVMX: Fix wrong reserved bits of error-code
> 
>  x86/vmx_tests.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 

Queued, thanks.

Paolo