KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: kvm@vger.kernel.org, rkrcmar@redhat.com, pbonzini@redhat.com,
	jmattson@google.com
Subject: Re: [PATCH 3/4] kvm-unit-test: nVMX: __enter_guest() should not set "launched" state when VM-entry fails
Date: Fri, 13 Sep 2019 13:37:55 -0700
Message-ID: <a2268863-e554-4547-5196-3509bda3ace3@oracle.com> (raw)
In-Reply-To: <20190904154231.GB24079@linux.intel.com>


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
>>

  reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-29 20:56 [PATCH 0/4] KVM: nVMX: Check GUEST_DEBUGCTL and GUEST_DR7 on vmentry of nested guests Krish Sadhukhan
2019-08-29 20:56 ` [PATCH 1/4] KVM: nVMX: Check GUEST_DEBUGCTL " Krish Sadhukhan
2019-08-29 22:12   ` Jim Mattson
2019-08-30 23:26     ` Krish Sadhukhan
2019-09-01 23:55       ` Jim Mattson
2019-08-29 20:56 ` [PATCH 2/4] KVM: nVMX: Check GUEST_DR7 " Krish Sadhukhan
2019-08-29 22:26   ` Jim Mattson
2019-08-30 23:07     ` Krish Sadhukhan
2019-08-30 23:15       ` Jim Mattson
2019-09-02  0:33         ` Jim Mattson
     [not found]           ` <e229bea2-acb2-e268-6281-d8e467c3282e@oracle.com>
2019-09-04 16:44             ` Jim Mattson
2019-09-04 16:58               ` Sean Christopherson
2019-09-04 18:05               ` Krish Sadhukhan
2019-09-04 18:20                 ` Jim Mattson
2019-09-09  4:11                   ` Krish Sadhukhan
2019-09-09 15:56                     ` Jim Mattson
2019-09-04 17:14           ` Sean Christopherson
2019-08-29 20:56 ` [PATCH 3/4] kvm-unit-test: nVMX: __enter_guest() should not set "launched" state when VM-entry fails Krish Sadhukhan
2019-09-04 15:42   ` Sean Christopherson
2019-09-13 20:37     ` Krish Sadhukhan [this message]
2019-09-13 21:06       ` Sean Christopherson
2019-09-16 19:12         ` Krish Sadhukhan
2019-08-29 20:56 ` [PATCH 4/4] kvm-unit-test: nVMX: Check GUEST_DEBUGCTL and GUEST_DR7 on vmentry of nested guests Krish Sadhukhan
2019-08-29 23:17   ` Jim Mattson
2019-08-30  1:12     ` Nadav Amit

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a2268863-e554-4547-5196-3509bda3ace3@oracle.com \
    --to=krish.sadhukhan@oracle.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=rkrcmar@redhat.com \
    --cc=sean.j.christopherson@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org kvm@archiver.kernel.org
	public-inbox-index kvm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox