[kvm-unit-tests v3 PATCH 0/3] vmx: Fix EPT accessed and dirty flag test

[kvm-unit-tests v3 PATCH 0/3] vmx: Fix EPT accessed and dirty flag test

[Cathy Avery]

Fixes to EPT test plus improvements to test_vmx_vmlaunch reporting.

* Changes in v3:
 - Split fixes into 3 patches.
 - Provide more detailed patch descriptions. 
 - Improve change log entries.

* Changes in v2:
 - Use the saved EPTP to restore the EPTP after each sub-test instead of
   manually unwinding what was done by the sub-test, which is error prone
   and hard to follow
 - Explicitly setup a dummy EPTP, as calling the test in isolation will cause
   test failures due to lack a good starting EPTP 
 - Cleanup test_vmx_vmlaunch to generate clearer and
   more consolidated test reports.
   New format suggested by seanjc@google.com

Cathy Avery (3):
  vmx: Cleanup test_vmx_vmlaunch to generate clearer and more
    consolidated test reports
  vmx: Explicitly setup a dummy EPTP in EPT accessed and dirty flag test
  vmx: Correctly refresh EPTP value in EPT accessed and dirty flag test

 x86/vmx_tests.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

-- 
2.31.1

[kvm-unit-tests v3 PATCH 1/3] vmx: Cleanup test_vmx_vmlaunch to generate clearer and more consolidated test reports

[Cathy Avery]

In cases when xerror is 0 ( the test is not expected to error ) and
the test does error we get a confusing test result as the vmlaunch status
is based on !xerror:

FAIL: Enable-EPT enabled; EPT page walk length 24: vmlaunch succeeds

This patch also eliminates the double call to report per launch and
clarifies the failure messages. New format suggested by seanjc@google.com

Signed-off-by: Cathy Avery <cavery@redhat.com>
---
 x86/vmx_tests.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 3d57ed6..0dab98e 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3392,14 +3392,21 @@ static void test_vmx_vmlaunch(u32 xerror)
 	bool success = vmlaunch_succeeds();
 	u32 vmx_inst_err;
 
-	report(success == !xerror, "vmlaunch %s",
-	       !xerror ? "succeeds" : "fails");
-	if (!success && xerror) {
-		vmx_inst_err = vmcs_read(VMX_INST_ERROR);
+	if (!success)
+	vmx_inst_err = vmcs_read(VMX_INST_ERROR);
+
+	if (success && !xerror)
+		report_pass("VMLAUNCH succeeded as expected");
+	else if (success && xerror)
+		report_fail("VMLAUNCH succeeded unexpectedly, wanted VM-Fail with error code = %d",
+			    xerror);
+	else if (!success && !xerror)
+		report_fail("VMLAUNCH hit unexpected VM-Fail with error code = %d",
+			    vmx_inst_err);
+	else
 		report(vmx_inst_err == xerror,
-		       "VMX inst error is %d (actual %d)", xerror,
-		       vmx_inst_err);
-	}
+		       "VMLAUNCH hit VM-Fail as expected, wanted error code %d, got %d",
+		       xerror, vmx_inst_err);
 }
 
 /*
-- 
2.31.1

Re: [kvm-unit-tests v3 PATCH 1/3] vmx: Cleanup test_vmx_vmlaunch to generate clearer and more consolidated test reports

[Sean Christopherson]

On Wed, Feb 16, 2022, Cathy Avery wrote:
> In cases when xerror is 0 ( the test is not expected to error ) and
> the test does error we get a confusing test result as the vmlaunch status
> is based on !xerror:
> 
> FAIL: Enable-EPT enabled; EPT page walk length 24: vmlaunch succeeds
> 
> This patch also eliminates the double call to report per launch and
> clarifies the failure messages. New format suggested by seanjc@google.com
> 
> Signed-off-by: Cathy Avery <cavery@redhat.com>
> ---
>  x86/vmx_tests.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 3d57ed6..0dab98e 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -3392,14 +3392,21 @@ static void test_vmx_vmlaunch(u32 xerror)
>  	bool success = vmlaunch_succeeds();
>  	u32 vmx_inst_err;
>  
> -	report(success == !xerror, "vmlaunch %s",
> -	       !xerror ? "succeeds" : "fails");
> -	if (!success && xerror) {
> -		vmx_inst_err = vmcs_read(VMX_INST_ERROR);
> +	if (!success)
> +	vmx_inst_err = vmcs_read(VMX_INST_ERROR);

Needs to be indended.  With that fixed,

Reviewed-by: Sean Christopherson <seanjc@google.com>

[kvm-unit-tests v3 PATCH 2/3] vmx: Explicitly setup a dummy EPTP in EPT accessed and dirty flag test

[Cathy Avery]

test_ept_eptp is not explicitly calling setup_dummy_ept() to initialize
EPTP to a good starting value resulting in test failures when it is called
in isolation or when EPTP has been changed by some previous test.

Signed-off-by: Cathy Avery <cavery@redhat.com>
---
 x86/vmx_tests.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 0dab98e..617f9dd 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4714,12 +4714,11 @@ static void test_ept_eptp(void)
 {
 	u32 primary_saved = vmcs_read(CPU_EXEC_CTRL0);
 	u32 secondary_saved = vmcs_read(CPU_EXEC_CTRL1);
-	u64 eptp_saved = vmcs_read(EPTP);
 	u32 primary = primary_saved;
 	u32 secondary = secondary_saved;
-	u64 eptp = eptp_saved;
 	u32 i, maxphysaddr;
 	u64 j, resv_bits_mask = 0;
+	u64 eptp_saved, eptp;
 
 	if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) &&
 	    (ctrl_cpu_rev[1].clr & CPU_EPT))) {
@@ -4727,6 +4726,9 @@ static void test_ept_eptp(void)
 		return;
 	}
 
+	setup_dummy_ept();
+	eptp = eptp_saved = vmcs_read(EPTP);
+
 	/* Support for 4-level EPT is mandatory. */
 	report(is_4_level_ept_supported(), "4-level EPT support check");
 
-- 
2.31.1

Re: [kvm-unit-tests v3 PATCH 2/3] vmx: Explicitly setup a dummy EPTP in EPT accessed and dirty flag test

[Sean Christopherson]

On Wed, Feb 16, 2022, Cathy Avery wrote:
> test_ept_eptp is not explicitly calling setup_dummy_ept() to initialize
> EPTP to a good starting value resulting in test failures when it is called
> in isolation or when EPTP has been changed by some previous test.
> 
> Signed-off-by: Cathy Avery <cavery@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[kvm-unit-tests v3 PATCH 3/3] vmx: Correctly refresh EPTP value in EPT accessed and dirty flag test

[Cathy Avery]

If ept_ad is not supported by the processor or has been
turned off via kvm module param, test_ept_eptp() will
incorrectly leave EPTP_AD_FLAG set in variable eptp
causing the following failures of subsequent
test_vmx_valid_controls calls:

FAIL: Enable-EPT enabled; reserved bits [11:7] 0: vmlaunch succeeds
FAIL: Enable-EPT enabled; reserved bits [63:N] 0: vmlaunch succeeds

Use the saved EPTP to restore the EPTP after each sub-test instead of
manually unwinding what was done by the sub-test, which is error prone
and hard to follow.

Signed-off-by: Cathy Avery <cavery@redhat.com>
---
 x86/vmx_tests.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 617f9dd..1269829 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4751,8 +4751,7 @@ static void test_ept_eptp(void)
 			test_vmx_invalid_controls();
 		report_prefix_pop();
 	}
-
-	eptp = (eptp & ~EPT_MEM_TYPE_MASK) | 6ul;
+	eptp = eptp_saved;
 
 	/*
 	 * Page walk length (bits 5:3).  Note, the value in VMCS.EPTP "is 1
@@ -4771,9 +4770,7 @@ static void test_ept_eptp(void)
 			test_vmx_invalid_controls();
 		report_prefix_pop();
 	}
-
-	eptp = (eptp & ~EPTP_PG_WALK_LEN_MASK) |
-	    3ul << EPTP_PG_WALK_LEN_SHIFT;
+	eptp = eptp_saved;
 
 	/*
 	 * Accessed and dirty flag (bit 6)
@@ -4793,6 +4790,7 @@ static void test_ept_eptp(void)
 		eptp |= EPTP_AD_FLAG;
 		test_eptp_ad_bit(eptp, false);
 	}
+	eptp = eptp_saved;
 
 	/*
 	 * Reserved bits [11:7] and [63:N]
@@ -4811,8 +4809,7 @@ static void test_ept_eptp(void)
 			test_vmx_invalid_controls();
 		report_prefix_pop();
 	}
-
-	eptp = (eptp & ~(EPTP_RESERV_BITS_MASK << EPTP_RESERV_BITS_SHIFT));
+	eptp = eptp_saved;
 
 	maxphysaddr = cpuid_maxphyaddr();
 	for (i = 0; i < (63 - maxphysaddr + 1); i++) {
@@ -4831,6 +4828,7 @@ static void test_ept_eptp(void)
 			test_vmx_invalid_controls();
 		report_prefix_pop();
 	}
+	eptp = eptp_saved;
 
 	secondary &= ~(CPU_EPT | CPU_URG);
 	vmcs_write(CPU_EXEC_CTRL1, secondary);
-- 
2.31.1

Re: [kvm-unit-tests v3 PATCH 3/3] vmx: Correctly refresh EPTP value in EPT accessed and dirty flag test

[Sean Christopherson]

On Wed, Feb 16, 2022, Cathy Avery wrote:
> If ept_ad is not supported by the processor or has been
> turned off via kvm module param, test_ept_eptp() will
> incorrectly leave EPTP_AD_FLAG set in variable eptp
> causing the following failures of subsequent
> test_vmx_valid_controls calls:
> 
> FAIL: Enable-EPT enabled; reserved bits [11:7] 0: vmlaunch succeeds
> FAIL: Enable-EPT enabled; reserved bits [63:N] 0: vmlaunch succeeds
> 
> Use the saved EPTP to restore the EPTP after each sub-test instead of
> manually unwinding what was done by the sub-test, which is error prone
> and hard to follow.
> 
> Signed-off-by: Cathy Avery <cavery@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Thanks much for seeing this through!