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 |
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 >
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 >>
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).
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 --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(); }