Patchwork [3/4,v2,kvm-unit-test,nVMX] : Change 'test_vmx_controls' to 'test_vmlaunch' and add a parameter for expected error

login
register
mail settings
Submitter Krish Sadhukhan
Date Feb. 6, 2019, 2:19 a.m.
Message ID <ff9583f3-771b-47e9-2fb7-d56a029acc1b@oracle.com>
Download mbox | patch
Permalink /patch/719217/
State New
Headers show

Comments

Krish Sadhukhan - Feb. 6, 2019, 2:19 a.m.
On 02/05/2019 03:30 PM, Sean Christopherson wrote:
> On Tue, Feb 05, 2019 at 05:34:26PM -0500, Krish Sadhukhan wrote:
>> The function 'test_vmx_controls' has thus far been used to test the VMX
>> controls and has the expected error code (returned by KVM) hard-coded in it.
>> When vmlaunch fails due to a bad Host State Area, the error code returned by
>> KVM is different. So rename this function and add a new parameter so that it
>> can also be used for testing the Host State Area during vmlaunch of L2 guests.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>>   x86/vmx_tests.c | 164 +++++++++++++++++++++++++-----------------------
>>   1 file changed, 86 insertions(+), 78 deletions(-)
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index 9a3cdee..b69a7d9 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -3285,7 +3285,7 @@ success:
>>   /*
>>    * Try to launch the current VMCS.
>>    */
>> -static void test_vmx_controls(bool controls_valid, bool xfail)
>> +static void test_vmlaunch(bool controls_valid, bool xfail, u32 error_expected)
>>   {
>>   	bool success = vmlaunch_succeeds();
>>   	u32 vmx_inst_err;
>> @@ -3295,8 +3295,8 @@ static void test_vmx_controls(bool controls_valid, bool xfail)
>>   	if (!success) {
>>   		vmx_inst_err = vmcs_read(VMX_INST_ERROR);
>>   		report("VMX inst error is %d (actual %d)",
>> -		       vmx_inst_err == VMXERR_ENTRY_INVALID_CONTROL_FIELD,
>> -		       VMXERR_ENTRY_INVALID_CONTROL_FIELD, vmx_inst_err);
>> +		       vmx_inst_err == error_expected, error_expected,
>> +		       vmx_inst_err);
>>   	}
> That's a lot of call sites to change just to reuse a few lines of code.
> And the xfail stuff isn't needed for the new host state checks.  What
> about moving the VMX_INST_ERROR logic to a helper and reusing that?
>
>  From 01c9fe2e0d4e0b4593334396a0a880f5adfd3fb5 Mon Sep 17 00:00:00 2001
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> Date: Tue, 5 Feb 2019 15:25:24 -0800
> Subject: [PATCH] KVM: nVMX: Add helper to check VMX_INST_ERROR after failed
>   vmlaunch
>
> ...so that the code can be reused for failure due to reasons other than
> invalid controls, e.g. invalid host state.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   x86/vmx_tests.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 28bab81..97c2163 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -3285,22 +3285,26 @@ success:
>   	return true;
>   }
>   
> +static void check_vmx_instr_err(u32 expected)
> +{
> +	u32 vmx_inst_err;
> +
> +	vmx_inst_err = vmcs_read(VMX_INST_ERROR);
> +	report("VMX inst error is %d (actual %d)",
> +	       vmx_inst_err == expected, expected, vmx_inst_err);
> +}
> +
>   /*
>    * Try to launch the current VMCS.
>    */
>   static void test_vmx_controls(bool controls_valid, bool xfail)
>   {
>   	bool success = vmlaunch_succeeds();
> -	u32 vmx_inst_err;
>   
>   	report_xfail("vmlaunch %s", xfail, success == controls_valid,
>   		     controls_valid ? "succeeds" : "fails");
> -	if (!success) {
> -		vmx_inst_err = vmcs_read(VMX_INST_ERROR);
> -		report("VMX inst error is %d (actual %d)",
> -		       vmx_inst_err == VMXERR_ENTRY_INVALID_CONTROL_FIELD,
> -		       VMXERR_ENTRY_INVALID_CONTROL_FIELD, vmx_inst_err);
> -	}
> +	if (!success)
> +		check_vmx_instr_err(VMXERR_ENTRY_INVALID_CONTROL_FIELD);
>   }
>   
>   /*

First of all,  'test_vmx_controls' seems a misnomer to me. All that this 
function does is vmlaunch'es the current VMCS and checks the error. From 
that perspective, something like 'do_vmlaunch' seems a more appropriate 
name to me. Also, we need to encapsulate the vmlaunch call and the error 
checking within a function such that the function can be reused for any 
VMCS field and not just the VMX control fields.

With respect to your suggested changes in patch# 4, the following lines 
of code will have to be duplicated each time we test a Host State Area 
field (or any other non-VMX-control field):

         success = vmlaunch_succeeds();
         report("vmlaunch %s", !success, "fails");
         if (!success)
check_vmx_instr_err(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);


Even though the call sites need to be changed, how about having two 
different versions of 'do_vmlaunch' - one for 'xfail' cases and the 
other for regular cases - such that any of these 'do_vmlaunch' functions 
can be used for any type of VMCS field and not just the VMX controls ?

Patch

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ee0c9ff..336d0df 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3282,22 +3282,43 @@  success:
         return true;
  }

+static void check_vmx_instr_err(u32 expected)
+{
+       u32 vmx_inst_err;
+
+       vmx_inst_err = vmcs_read(VMX_INST_ERROR);
+       report("VMX inst error is %d (actual %d)",
+              vmx_inst_err == expected, expected, vmx_inst_err);
+}
+
  /*
   * Try to launch the current VMCS.
   */
-static void test_vmx_controls(bool controls_valid, bool xfail)
+static void do_vmlaunch(bool controls_valid, u32 error_expected)
+{
+       bool success = vmlaunch_succeeds();
+       u32 vmx_inst_err;
+
+       report("vmlaunch %s", success == controls_valid,
+                    controls_valid ? "succeeds" : "fails");
+       if (!success)
+               check_vmx_instr_err(error_expected);
+}
+
+
+/*
+ * Try to launch the current VMCS.
+ */
+static void do_vmlaunch_xfail(bool controls_valid, u32 error_expected,
+                               bool xfail)
  {
         bool success = vmlaunch_succeeds();
         u32 vmx_inst_err;

         report_xfail("vmlaunch %s", xfail, success == controls_valid,
                      controls_valid ? "succeeds" : "fails");
-       if (!success) {
-               vmx_inst_err = vmcs_read(VMX_INST_ERROR);
-               report("VMX inst error is %d (actual %d)",
-                      vmx_inst_err == VMXERR_ENTRY_INVALID_CONTROL_FIELD,
-                      VMXERR_ENTRY_INVALID_CONTROL_FIELD, vmx_inst_err);
-       }
+       if (!success)
+               check_vmx_instr_err(error_expected);
  }