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@xxxxxxxxxx>
Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx>
---
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
