diff mbox series

[3/4] kvm-unit-test: nVMX: __enter_guest() should not set "launched" state when VM-entry fails

Message ID 20190829205635.20189-4-krish.sadhukhan@oracle.com (mailing list archive)
State New, archived
Headers show
Series KVM: nVMX: Check GUEST_DEBUGCTL and GUEST_DR7 on vmentry of nested guests | expand

Commit Message

Krish Sadhukhan Aug. 29, 2019, 8:56 p.m. UTC 
Bit# 31 in VM-exit reason is set by hardware in both cases of early VM-entry
failures and VM-entry failures due to invalid guest state. Whenever VM-entry
fails, the nested VMCS is not in "launched" state any more. Hence,
__enter_guest() should not set the "launched" state when a VM-entry fails.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Sean Christopherson Sept. 4, 2019, 3:42 p.m. UTC | #1 
On Thu, Aug 29, 2019 at 04:56:34PM -0400, Krish Sadhukhan wrote:
> Bit# 31 in VM-exit reason is set by hardware in both cases of early VM-entry
> failures and VM-entry failures due to invalid guest state.

This is incorrect, VMCS.EXIT_REASON is not written on a VM-Fail.  If the
tests are passing, you're probably consuming a stale EXIT_REASON.

> Whenever VM-entry
> fails, the nested VMCS is not in "launched" state any more. Hence,
> __enter_guest() should not set the "launched" state when a VM-entry fails.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  x86/vmx.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 872ba11..183d11b 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -1805,6 +1805,8 @@ static void check_for_guest_termination(void)
>   */
>  static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
>  {
> +	bool vm_entry_failure;
> +
>  	TEST_ASSERT_MSG(v2_guest_main,
>  			"Never called test_set_guest_func!");
>  
> @@ -1812,15 +1814,14 @@ static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
>  			"Called enter_guest() after guest returned.");
>  
>  	vmx_enter_guest(failure);
> +	vm_entry_failure = vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE;

Rather than duplicating the code in vmx_run(), what if we move this check
into vmx_enter_guest() and rework struct vmentry_failure?  The code was
originally designed to handle only VM-Fail conditions, we should clean it
up instead of bolting more stuff on top.  E.g.:

struct vmentry_status {
	/* Did we attempt VMLAUNCH or VMRESUME */ 
	bool vmlaunch;
	/* Instruction mnemonic (for convenience). */
	const char *instr;
	/* VM-Enter passed all consistency checks, i.e. did not fail. */
	bool succeeded;
	/* VM-Enter failed before loading guest state, i.e. VM-Fail. */
	bool vm_fail;
	/* Contents of RFLAGS on VM-Fail, EXIT_REASON on VM-Exit.  */
	union {
		unsigned long vm_fail_flags;
		unsigned long vm_exit_reason;
	};
};

static void vmx_enter_guest(struct vmentry_status *status)
{
	status->vm_fail = 0;

	in_guest = 1;
	asm volatile (
		"mov %[HOST_RSP], %%rdi\n\t"
		"vmwrite %%rsp, %%rdi\n\t"
		LOAD_GPR_C
		"cmpb $0, %[launched]\n\t"
		"jne 1f\n\t"
		"vmlaunch\n\t"
		"jmp 2f\n\t"
		"1: "
		"vmresume\n\t"
		"2: "
		SAVE_GPR_C
		"pushf\n\t"
		"pop %%rdi\n\t"
		"mov %%rdi, %[vm_fail_flags]\n\t"
		"movl $1, %[vm_fail]\n\t"
		"jmp 3f\n\t"
		"vmx_return:\n\t"
		SAVE_GPR_C
		"3: \n\t"
		: [vm_fail]"+m"(status->vm_fail),
		  [vm_fail_flags]"=m"(status->vm_fail_flags)
		: [launched]"m"(launched), [HOST_RSP]"i"(HOST_RSP)
		: "rdi", "memory", "cc"
	);
	in_guest = 0;

	if (!status->vm_fail)
		status->vm_exit_reason = vmcs_read(EXI_REASON);
		
	status->succeeded = !status->vm_fail &&
			    !(status->vm_exit_reason & VMX_ENTRY_FAILURE);

	status->vmlaunch = !launched;
	status->instr = launched ? "vmresume" : "vmlaunch";

	if (status->succeeded)
		launched = 1;
}

>  	if ((abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL && failure->early) ||
> -	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE &&
> -	    vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
> -
> +	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE && vm_entry_failure)) {
>  		print_vmentry_failure_info(failure);
>  		abort();
>  	}
>  
> -	if (!failure->early) {
> +	if (!vm_entry_failure) {
>  		launched = 1;
>  		check_for_guest_termination();
>  	}
> -- 
> 2.20.1
>
Krish Sadhukhan Sept. 13, 2019, 8:37 p.m. UTC | #2 
On 9/4/19 8:42 AM, Sean Christopherson wrote:
> On Thu, Aug 29, 2019 at 04:56:34PM -0400, Krish Sadhukhan wrote:
>> Bit# 31 in VM-exit reason is set by hardware in both cases of early VM-entry
>> failures and VM-entry failures due to invalid guest state.
> This is incorrect, VMCS.EXIT_REASON is not written on a VM-Fail.  If the
> tests are passing, you're probably consuming a stale EXIT_REASON.

In vmx_vcpu_run(),

         if (vmx->fail || (vmx->exit_reason & 
VMX_EXIT_REASONS_FAILED_VMENTRY))
                 return;

         vmx->loaded_vmcs->launched = 1;

we return without setting "launched" whenever bit# 31 is set in Exit 
Reason. If VM-entry fails due to invalid guest state or due to errors in 
VM-entry MSR-loading area, bit#31 is set.  As a result, L2 is not in 
"launched" state when we return to L1.  Tests that want to VMRESUME L2 
after fixing the bad guest state or the bad MSR-loading area, fail with 
VM-Instruction Error 5,

         "Early vmresume failure: error number is 5. See Intel 30.4."

>
>> Whenever VM-entry
>> fails, the nested VMCS is not in "launched" state any more. Hence,
>> __enter_guest() should not set the "launched" state when a VM-entry fails.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> ---
>>   x86/vmx.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/x86/vmx.c b/x86/vmx.c
>> index 872ba11..183d11b 100644
>> --- a/x86/vmx.c
>> +++ b/x86/vmx.c
>> @@ -1805,6 +1805,8 @@ static void check_for_guest_termination(void)
>>    */
>>   static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
>>   {
>> +	bool vm_entry_failure;
>> +
>>   	TEST_ASSERT_MSG(v2_guest_main,
>>   			"Never called test_set_guest_func!");
>>   
>> @@ -1812,15 +1814,14 @@ static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
>>   			"Called enter_guest() after guest returned.");
>>   
>>   	vmx_enter_guest(failure);
>> +	vm_entry_failure = vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE;
> Rather than duplicating the code in vmx_run(), what if we move this check
> into vmx_enter_guest() and rework struct vmentry_failure?  The code was
> originally designed to handle only VM-Fail conditions, we should clean it
> up instead of bolting more stuff on top.  E.g.:
>
> struct vmentry_status {
> 	/* Did we attempt VMLAUNCH or VMRESUME */
> 	bool vmlaunch;
> 	/* Instruction mnemonic (for convenience). */
> 	const char *instr;
> 	/* VM-Enter passed all consistency checks, i.e. did not fail. */
> 	bool succeeded;
> 	/* VM-Enter failed before loading guest state, i.e. VM-Fail. */
> 	bool vm_fail;
> 	/* Contents of RFLAGS on VM-Fail, EXIT_REASON on VM-Exit.  */
> 	union {
> 		unsigned long vm_fail_flags;
> 		unsigned long vm_exit_reason;
> 	};
> };
>
> static void vmx_enter_guest(struct vmentry_status *status)
> {
> 	status->vm_fail = 0;
>
> 	in_guest = 1;
> 	asm volatile (
> 		"mov %[HOST_RSP], %%rdi\n\t"
> 		"vmwrite %%rsp, %%rdi\n\t"
> 		LOAD_GPR_C
> 		"cmpb $0, %[launched]\n\t"
> 		"jne 1f\n\t"
> 		"vmlaunch\n\t"
> 		"jmp 2f\n\t"
> 		"1: "
> 		"vmresume\n\t"
> 		"2: "
> 		SAVE_GPR_C
> 		"pushf\n\t"
> 		"pop %%rdi\n\t"
> 		"mov %%rdi, %[vm_fail_flags]\n\t"
> 		"movl $1, %[vm_fail]\n\t"
> 		"jmp 3f\n\t"
> 		"vmx_return:\n\t"
> 		SAVE_GPR_C
> 		"3: \n\t"
> 		: [vm_fail]"+m"(status->vm_fail),
> 		  [vm_fail_flags]"=m"(status->vm_fail_flags)
> 		: [launched]"m"(launched), [HOST_RSP]"i"(HOST_RSP)
> 		: "rdi", "memory", "cc"
> 	);
> 	in_guest = 0;
>
> 	if (!status->vm_fail)
> 		status->vm_exit_reason = vmcs_read(EXI_REASON);
> 		
> 	status->succeeded = !status->vm_fail &&
> 			    !(status->vm_exit_reason & VMX_ENTRY_FAILURE);
>
> 	status->vmlaunch = !launched;
> 	status->instr = launched ? "vmresume" : "vmlaunch";
>
> 	if (status->succeeded)
> 		launched = 1;
> }


This looks good. Do you want to send a patch or you want me to add it to 
the current set ?


>
>>   	if ((abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL && failure->early) ||
>> -	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE &&
>> -	    vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
>> -
>> +	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE && vm_entry_failure)) {
>>   		print_vmentry_failure_info(failure);
>>   		abort();
>>   	}
>>   
>> -	if (!failure->early) {
>> +	if (!vm_entry_failure) {
>>   		launched = 1;
>>   		check_for_guest_termination();
>>   	}
>> -- 
>> 2.20.1
>>
Sean Christopherson Sept. 13, 2019, 9:06 p.m. UTC | #3 
On Fri, Sep 13, 2019 at 01:37:55PM -0700, Krish Sadhukhan wrote:
> 
> On 9/4/19 8:42 AM, Sean Christopherson wrote:
> >On Thu, Aug 29, 2019 at 04:56:34PM -0400, Krish Sadhukhan wrote:
> >>Bit# 31 in VM-exit reason is set by hardware in both cases of early VM-entry
> >>failures and VM-entry failures due to invalid guest state.
> >This is incorrect, VMCS.EXIT_REASON is not written on a VM-Fail.  If the
> >tests are passing, you're probably consuming a stale EXIT_REASON.
> 
> In vmx_vcpu_run(),
> 
>         if (vmx->fail || (vmx->exit_reason &
> VMX_EXIT_REASONS_FAILED_VMENTRY))
>                 return;
> 
>         vmx->loaded_vmcs->launched = 1;
> 
> we return without setting "launched" whenever bit# 31 is set in Exit Reason.
> If VM-entry fails due to invalid guest state or due to errors in VM-entry
> MSR-loading area, bit#31 is set.  As a result, L2 is not in "launched" state
> when we return to L1.  Tests that want to VMRESUME L2 after fixing the bad
> guest state or the bad MSR-loading area, fail with VM-Instruction Error 5,
> 
>         "Early vmresume failure: error number is 5. See Intel 30.4."

Yes, a VMCS isn't marked launched if it generates a VM-Exit due to a
failed consistency check.  But as that code shows, a failed consistency
check results in said VM-Exit *or* a VM-Fail.  Cosnsitency checks that
fail early, i.e. before loading guest state, generate VM-Fail, any check
that fails after the CPU has started loading guest state manifests as a
VM-Exit.  VMCS.EXIT_REASON isn't touched in the VM-Fail case.

E.g. in CHECKS ON VMX CONTROLS AND HOST-STATE AREA, the SDM states:

  VM entry fails if any of these checks fail.  When such failures occur,
  control is passed to the next instruction, RFLAGS.ZF is set to 1 to
  indicate the failure, and the VM-instruction error field is loaded with
  an error number that indicates whether the failure was due to the
  controls or the host-state area (see Chapter 30).
Krish Sadhukhan Sept. 16, 2019, 7:12 p.m. UTC | #4 
On 09/13/2019 02:06 PM, Sean Christopherson wrote:
> On Fri, Sep 13, 2019 at 01:37:55PM -0700, Krish Sadhukhan wrote:
>> On 9/4/19 8:42 AM, Sean Christopherson wrote:
>>> On Thu, Aug 29, 2019 at 04:56:34PM -0400, Krish Sadhukhan wrote:
>>>> Bit# 31 in VM-exit reason is set by hardware in both cases of early VM-entry
>>>> failures and VM-entry failures due to invalid guest state.
>>> This is incorrect, VMCS.EXIT_REASON is not written on a VM-Fail.  If the
>>> tests are passing, you're probably consuming a stale EXIT_REASON.
>> In vmx_vcpu_run(),
>>
>>          if (vmx->fail || (vmx->exit_reason &
>> VMX_EXIT_REASONS_FAILED_VMENTRY))
>>                  return;
>>
>>          vmx->loaded_vmcs->launched = 1;
>>
>> we return without setting "launched" whenever bit# 31 is set in Exit Reason.
>> If VM-entry fails due to invalid guest state or due to errors in VM-entry
>> MSR-loading area, bit#31 is set.  As a result, L2 is not in "launched" state
>> when we return to L1.  Tests that want to VMRESUME L2 after fixing the bad
>> guest state or the bad MSR-loading area, fail with VM-Instruction Error 5,
>>
>>          "Early vmresume failure: error number is 5. See Intel 30.4."
> Yes, a VMCS isn't marked launched if it generates a VM-Exit due to a
> failed consistency check.  But as that code shows, a failed consistency
> check results in said VM-Exit *or* a VM-Fail.  Cosnsitency checks that
> fail early, i.e. before loading guest state, generate VM-Fail, any check
> that fails after the CPU has started loading guest state manifests as a
> VM-Exit.  VMCS.EXIT_REASON isn't touched in the VM-Fail case.
>
> E.g. in CHECKS ON VMX CONTROLS AND HOST-STATE AREA, the SDM states:
>
>    VM entry fails if any of these checks fail.  When such failures occur,
>    control is passed to the next instruction, RFLAGS.ZF is set to 1 to
>    indicate the failure, and the VM-instruction error field is loaded with
>    an error number that indicates whether the failure was due to the
>    controls or the host-state area (see Chapter 30).

The fix done by Marc Orr in

         "[kvm-unit-tests PATCH v2] x86: nvmx: test max atomic switch MSRs"

fixes this problem.
diff mbox series

Patch

diff --git a/x86/vmx.c b/x86/vmx.c
index 872ba11..183d11b 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -1805,6 +1805,8 @@  static void check_for_guest_termination(void)
  */
 static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
 {
+	bool vm_entry_failure;
+
 	TEST_ASSERT_MSG(v2_guest_main,
 			"Never called test_set_guest_func!");
 
@@ -1812,15 +1814,14 @@  static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
 			"Called enter_guest() after guest returned.");
 
 	vmx_enter_guest(failure);
+	vm_entry_failure = vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE;
 	if ((abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL && failure->early) ||
-	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE &&
-	    vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
-
+	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE && vm_entry_failure)) {
 		print_vmentry_failure_info(failure);
 		abort();
 	}
 
-	if (!failure->early) {
+	if (!vm_entry_failure) {
 		launched = 1;
 		check_for_guest_termination();
 	}