diff mbox series

[2/2,kvm-unit-test] nVMX x86: Check VMX-preemption timer controls on vmentry of L2 guests

Message ID 20181101052159.32695-3-krish.sadhukhan@oracle.com (mailing list archive)
State New, archived
Headers show
Series [1/2] nVMX x86: Check VMX-preemption timer controls on vmentry of L2 guests | expand

Commit Message

Krish Sadhukhan Nov. 1, 2018, 5:21 a.m. UTC 
According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
following check needs to be enforced on vmentry of L2 guests:

    If the "activate VMX-preemption timer" VM-execution control is 0, the
    the "save VMX-preemption timer value" VM-exit control must also be 0.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 x86/vmx_tests.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Paolo Bonzini Nov. 26, 2018, 4:32 p.m. UTC | #1 
On 01/11/18 06:21, Krish Sadhukhan wrote:
> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
> following check needs to be enforced on vmentry of L2 guests:
> 
>     If the "activate VMX-preemption timer" VM-execution control is 0, the
>     the "save VMX-preemption timer value" VM-exit control must also be 0.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  x86/vmx_tests.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index b105b23..7f49048 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4721,6 +4721,55 @@ static void test_pml(void)
>  	vmcs_write(CPU_EXEC_CTRL1, secondary_saved);
>  }
>  
> + /*
> + * If the "activate VMX-preemption timer" VM-execution control is 0, the
> + * the "save VMX-preemption timer value" VM-exit control must also be 0.
> + *
> + *  [Intel SDM]
> + */
> +static void test_vmx_preemption_timer(void)
> +{
> +	u32 saved_pin = vmcs_read(PIN_CONTROLS);
> +	u32 saved_exit = vmcs_read(EXI_CONTROLS);
> +	u32 pin = saved_pin;
> +	u32 exit = saved_exit;
> +
> +	if (!((ctrl_exit_rev.clr & EXI_SAVE_PREEMPT) ||
> +	    (ctrl_pin_rev.clr & PIN_PREEMPT))) {
> +		printf("\"Save-VMX-preemption-timer\" control and/or \"Enable-VMX-preemption-timer\" control is not supported\n");
> +		return;
> +	}
> +
> +	pin |= PIN_PREEMPT;
> +	vmcs_write(PIN_CONTROLS, pin);
> +	exit &= ~EXI_SAVE_PREEMPT;
> +	vmcs_write(EXI_CONTROLS, exit);
> +	report_prefix_pushf("enable-VMX-preemption-timer enabled, save-VMX-preemption-timer disabled");
> +	test_vmx_controls(true, false);
> +	report_prefix_pop();
> +
> +	exit |= EXI_SAVE_PREEMPT;
> +	vmcs_write(EXI_CONTROLS, exit);
> +	report_prefix_pushf("enable-VMX-preemption-timer enabled, save-VMX-preemption-timer enabled");
> +	test_vmx_controls(true, false);
> +	report_prefix_pop();
> +
> +	pin &= ~PIN_PREEMPT;
> +	vmcs_write(PIN_CONTROLS, pin);
> +	report_prefix_pushf("enable-VMX-preemption-timer disabled, save-VMX-preemption-timer enabled");
> +	test_vmx_controls(false, false);
> +	report_prefix_pop();
> +
> +	exit &= ~EXI_SAVE_PREEMPT;
> +	vmcs_write(EXI_CONTROLS, exit);
> +	report_prefix_pushf("enable-VMX-preemption-timer disabled, save-VMX-preemption-timer disabled");
> +	test_vmx_controls(true, false);
> +	report_prefix_pop();
> +
> +	vmcs_write(PIN_CONTROLS, saved_pin);
> +	vmcs_write(EXI_CONTROLS, saved_exit);
> +}
> +
>  /*
>   * Check that the virtual CPU checks all of the VMX controls as
>   * documented in the Intel SDM.
> @@ -4747,6 +4796,7 @@ static void vmx_controls_test(void)
>  	test_invalid_event_injection();
>  	test_vpid();
>  	test_eptp();
> +	test_vmx_preemption_timer();
>  }
>  
>  static bool valid_vmcs_for_vmentry(void)
> 

The following is needed too:

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index fe187c5..7a8d6ce 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -183,6 +183,8 @@ static int preemption_timer_exit_handler(void)
 			vmx_set_test_stage(4);
 			vmcs_write(PIN_CONTROLS,
 				   vmcs_read(PIN_CONTROLS) & ~PIN_PREEMPT);
+			vmcs_write(EXI_CONTROLS,
+				   vmcs_read(EXI_CONTROLS) & ~EXI_SAVE_PREEMPT);
 			vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE);
 			return VMX_TEST_RESUME;
 		case 4:

Paolo
Krish Sadhukhan Dec. 1, 2018, 2:12 a.m. UTC | #2 
On 11/26/2018 08:32 AM, Paolo Bonzini wrote:
> On 01/11/18 06:21, Krish Sadhukhan wrote:
>> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
>> following check needs to be enforced on vmentry of L2 guests:
>>
>>      If the "activate VMX-preemption timer" VM-execution control is 0, the
>>      the "save VMX-preemption timer value" VM-exit control must also be 0.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
>> ---
>>   x86/vmx_tests.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index b105b23..7f49048 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -4721,6 +4721,55 @@ static void test_pml(void)
>>   	vmcs_write(CPU_EXEC_CTRL1, secondary_saved);
>>   }
>>   
>> + /*
>> + * If the "activate VMX-preemption timer" VM-execution control is 0, the
>> + * the "save VMX-preemption timer value" VM-exit control must also be 0.
>> + *
>> + *  [Intel SDM]
>> + */
>> +static void test_vmx_preemption_timer(void)
>> +{
>> +	u32 saved_pin = vmcs_read(PIN_CONTROLS);
>> +	u32 saved_exit = vmcs_read(EXI_CONTROLS);
>> +	u32 pin = saved_pin;
>> +	u32 exit = saved_exit;
>> +
>> +	if (!((ctrl_exit_rev.clr & EXI_SAVE_PREEMPT) ||
>> +	    (ctrl_pin_rev.clr & PIN_PREEMPT))) {
>> +		printf("\"Save-VMX-preemption-timer\" control and/or \"Enable-VMX-preemption-timer\" control is not supported\n");
>> +		return;
>> +	}
>> +
>> +	pin |= PIN_PREEMPT;
>> +	vmcs_write(PIN_CONTROLS, pin);
>> +	exit &= ~EXI_SAVE_PREEMPT;
>> +	vmcs_write(EXI_CONTROLS, exit);
>> +	report_prefix_pushf("enable-VMX-preemption-timer enabled, save-VMX-preemption-timer disabled");
>> +	test_vmx_controls(true, false);
>> +	report_prefix_pop();
>> +
>> +	exit |= EXI_SAVE_PREEMPT;
>> +	vmcs_write(EXI_CONTROLS, exit);
>> +	report_prefix_pushf("enable-VMX-preemption-timer enabled, save-VMX-preemption-timer enabled");
>> +	test_vmx_controls(true, false);
>> +	report_prefix_pop();
>> +
>> +	pin &= ~PIN_PREEMPT;
>> +	vmcs_write(PIN_CONTROLS, pin);
>> +	report_prefix_pushf("enable-VMX-preemption-timer disabled, save-VMX-preemption-timer enabled");
>> +	test_vmx_controls(false, false);
>> +	report_prefix_pop();
>> +
>> +	exit &= ~EXI_SAVE_PREEMPT;
>> +	vmcs_write(EXI_CONTROLS, exit);
>> +	report_prefix_pushf("enable-VMX-preemption-timer disabled, save-VMX-preemption-timer disabled");
>> +	test_vmx_controls(true, false);
>> +	report_prefix_pop();
>> +
>> +	vmcs_write(PIN_CONTROLS, saved_pin);
>> +	vmcs_write(EXI_CONTROLS, saved_exit);
>> +}
>> +
>>   /*
>>    * Check that the virtual CPU checks all of the VMX controls as
>>    * documented in the Intel SDM.
>> @@ -4747,6 +4796,7 @@ static void vmx_controls_test(void)
>>   	test_invalid_event_injection();
>>   	test_vpid();
>>   	test_eptp();
>> +	test_vmx_preemption_timer();
>>   }
>>   
>>   static bool valid_vmcs_for_vmentry(void)
>>
> The following is needed too:
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index fe187c5..7a8d6ce 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -183,6 +183,8 @@ static int preemption_timer_exit_handler(void)
>   			vmx_set_test_stage(4);
>   			vmcs_write(PIN_CONTROLS,
>   				   vmcs_read(PIN_CONTROLS) & ~PIN_PREEMPT);
> +			vmcs_write(EXI_CONTROLS,
> +				   vmcs_read(EXI_CONTROLS) & ~EXI_SAVE_PREEMPT);
>   			vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE);
>   			return VMX_TEST_RESUME;
>   		case 4:
>
> Paolo

I missed that. Thanks !

For adhering to the rule, we should also disable the control at the end 
of preemption_timer_exit_handler():

...
      vmcs_write(PIN_CONTROLS, vmcs_read(PIN_CONTROLS) & ~PIN_PREEMPT);
+  vmcs_write(EXI_CONTROLS,
+                     vmcs_read(EXI_CONTROLS) & ~EXI_SAVE_PREEMPT);
            return VMX_TEST_VMEXIT;



Should I send out a revised patch or you will apply these changes ?
Paolo Bonzini Dec. 6, 2018, 8:26 p.m. UTC | #3 
On 01/12/18 03:12, Krish Sadhukhan wrote:
> For adhering to the rule, we should also disable the control at the end
> of preemption_timer_exit_handler():
> 
> ...
>      vmcs_write(PIN_CONTROLS, vmcs_read(PIN_CONTROLS) & ~PIN_PREEMPT);
> +  vmcs_write(EXI_CONTROLS,
> +                     vmcs_read(EXI_CONTROLS) & ~EXI_SAVE_PREEMPT);
>            return VMX_TEST_VMEXIT;
> 

Or also remove the other vmcs_write, since the VMCS is not reused across
tests.

> Should I send out a revised patch or you will apply these changes ?

I have already applied the patch.

Paolo
diff mbox series

Patch

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index b105b23..7f49048 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4721,6 +4721,55 @@  static void test_pml(void)
 	vmcs_write(CPU_EXEC_CTRL1, secondary_saved);
 }
 
+ /*
+ * If the "activate VMX-preemption timer" VM-execution control is 0, the
+ * the "save VMX-preemption timer value" VM-exit control must also be 0.
+ *
+ *  [Intel SDM]
+ */
+static void test_vmx_preemption_timer(void)
+{
+	u32 saved_pin = vmcs_read(PIN_CONTROLS);
+	u32 saved_exit = vmcs_read(EXI_CONTROLS);
+	u32 pin = saved_pin;
+	u32 exit = saved_exit;
+
+	if (!((ctrl_exit_rev.clr & EXI_SAVE_PREEMPT) ||
+	    (ctrl_pin_rev.clr & PIN_PREEMPT))) {
+		printf("\"Save-VMX-preemption-timer\" control and/or \"Enable-VMX-preemption-timer\" control is not supported\n");
+		return;
+	}
+
+	pin |= PIN_PREEMPT;
+	vmcs_write(PIN_CONTROLS, pin);
+	exit &= ~EXI_SAVE_PREEMPT;
+	vmcs_write(EXI_CONTROLS, exit);
+	report_prefix_pushf("enable-VMX-preemption-timer enabled, save-VMX-preemption-timer disabled");
+	test_vmx_controls(true, false);
+	report_prefix_pop();
+
+	exit |= EXI_SAVE_PREEMPT;
+	vmcs_write(EXI_CONTROLS, exit);
+	report_prefix_pushf("enable-VMX-preemption-timer enabled, save-VMX-preemption-timer enabled");
+	test_vmx_controls(true, false);
+	report_prefix_pop();
+
+	pin &= ~PIN_PREEMPT;
+	vmcs_write(PIN_CONTROLS, pin);
+	report_prefix_pushf("enable-VMX-preemption-timer disabled, save-VMX-preemption-timer enabled");
+	test_vmx_controls(false, false);
+	report_prefix_pop();
+
+	exit &= ~EXI_SAVE_PREEMPT;
+	vmcs_write(EXI_CONTROLS, exit);
+	report_prefix_pushf("enable-VMX-preemption-timer disabled, save-VMX-preemption-timer disabled");
+	test_vmx_controls(true, false);
+	report_prefix_pop();
+
+	vmcs_write(PIN_CONTROLS, saved_pin);
+	vmcs_write(EXI_CONTROLS, saved_exit);
+}
+
 /*
  * Check that the virtual CPU checks all of the VMX controls as
  * documented in the Intel SDM.
@@ -4747,6 +4796,7 @@  static void vmx_controls_test(void)
 	test_invalid_event_injection();
 	test_vpid();
 	test_eptp();
+	test_vmx_preemption_timer();
 }
 
 static bool valid_vmcs_for_vmentry(void)