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@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;
}
> 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
>
