Re: [kvm-unit-tests v2 PATCH] vmx: Fix EPT accessed and dirty flag test

[Sean Christopherson <seanjc@google.com>]

On Wed, Feb 02, 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

Heh, the changelog never actually provides info on how it fixes things.

  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.

> Signed-off-by: Cathy Avery <cavery@redhat.com>
> ---
> 
> * Changes in v2:
> 
> - Initialize vmcs EPTP to good values for page walk len
>   and ept memory type.
> - Restore eptp to known good values from eptp_saved
> - Cleanup test_vmx_vmlaunch to generate clearer and
>   more consolidated test reports.
>   New format suggested by seanjc@google.com
> ---
>  x86/vmx_tests.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 3d57ed6..1269829 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);
>  }

The changes to test_vmx_vmlaunch() need to be a separate patch.  The addition of
setup_dummy_ept() would ideally be separate as well, though I don't care terribly
about that one.

With this split in two (or three) and an updated changelog,

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