Rework the emulator and its users to handle failure scenarios entirely
within the emulator.
{x86,kvm}_emulate_instruction() currently return a tri-state value to
indicate success/continue, userspace exit needed, and failure. The
intent of returning EMULATE_FAIL is to let the caller handle failure in
a manner that is appropriate for the current context. In practice,
the emulator has ended up with a mixture of failure handling, i.e.
whether or not the emulator takes action on failure is dependent on the
specific flavor of emulation.
The mixed handling has proven to be rather fragile, e.g. many flows
incorrectly assume their specific flavor of emulation cannot fail or
that the emulator sets state to report the failure back to userspace.
Move everything inside the emulator, piece by piece, so that the
emulation routines can return '0' for exit to userspace and '1' for
resume the guest, just like every other VM-Exit handler.
Paolo/Radim/Vitaly, apologies for the resend, forgot to cc kvm, lkml
and reviewers.
Sean Christopherson (13):
KVM: x86: Relocate MMIO exit stats counting
KVM: x86: Clean up handle_emulation_failure()
KVM: x86: Refactor kvm_vcpu_do_singlestep() to remove out param
KVM: x86: Drop EMULTYPE_NO_UD_ON_FAIL as a standalone type
KVM: x86: Don't attempt VMWare emulation on #GP with non-zero error
code
KVM: x86: Move #GP injection for VMware into x86_emulate_instruction()
KVM: x86: Add explicit flag for forced emulation on #UD
KVM: x86: Move #UD injection for failed emulation into emulation code
KVM: x86: Exit to userspace on emulation skip failure
KVM: x86: Handle emulation failure directly in kvm_task_switch()
KVM: x86: Move triple fault request into RM int injection
KVM: VMX: Remove EMULATE_FAIL handling in handle_invalid_guest_state()
KVM: x86: Remove emulation_result enums, EMULATE_{DONE,FAIL,USER_EXIT}
arch/x86/include/asm/kvm_host.h | 10 +--
arch/x86/kvm/mmu.c | 16 +---
arch/x86/kvm/svm.c | 41 ++++-------
arch/x86/kvm/vmx/vmx.c | 92 +++++++++--------------
arch/x86/kvm/x86.c | 127 +++++++++++++++++---------------
arch/x86/kvm/x86.h | 2 +-
6 files changed, 121 insertions(+), 167 deletions(-)
--
2.22.0
Immediately inject a #GP when VMware emulation fails and return
EMULATE_DONE instead of propagating EMULATE_FAIL up the stack. This
helps pave the way for removing EMULATE_FAIL altogether.
Rename EMULTYPE_VMWARE to EMULTYPE_VMWARE_GP to help document why a #GP
is injected on emulation failure.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/svm.c | 9 ++-------
arch/x86/kvm/vmx/vmx.c | 9 ++-------
arch/x86/kvm/x86.c | 14 +++++++++-----
4 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index dd6bd9ed0839..d1d5b5ca1195 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1318,7 +1318,7 @@ enum emulation_result {
#define EMULTYPE_TRAP_UD (1 << 1)
#define EMULTYPE_SKIP (1 << 2)
#define EMULTYPE_ALLOW_RETRY (1 << 3)
-#define EMULTYPE_VMWARE (1 << 5)
+#define EMULTYPE_VMWARE_GP (1 << 5)
int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
void *insn, int insn_len);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index b96a119690f4..97562c2c8b7b 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2768,7 +2768,6 @@ static int gp_interception(struct vcpu_svm *svm)
{
struct kvm_vcpu *vcpu = &svm->vcpu;
u32 error_code = svm->vmcb->control.exit_info_1;
- int er;
WARN_ON_ONCE(!enable_vmware_backdoor);
@@ -2776,12 +2775,8 @@ static int gp_interception(struct vcpu_svm *svm)
kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
return 1;
}
- er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
- if (er == EMULATE_USER_EXIT)
- return 0;
- else if (er != EMULATE_DONE)
- kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
- return 1;
+ return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
+ EMULATE_USER_EXIT;
}
static bool is_erratum_383(void)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 3ee0dd304bc7..25410c58c758 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4492,7 +4492,6 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
u32 intr_info, ex_no, error_code;
unsigned long cr2, rip, dr6;
u32 vect_info;
- enum emulation_result er;
vect_info = vmx->idt_vectoring_info;
intr_info = vmx->exit_intr_info;
@@ -4514,12 +4513,8 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
return 1;
}
- er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
- if (er == EMULATE_USER_EXIT)
- return 0;
- else if (er != EMULATE_DONE)
- kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
- return 1;
+ return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
+ EMULATE_USER_EXIT;
}
/*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e0f0e14d8fac..228ca71d5b01 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6210,8 +6210,10 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
++vcpu->stat.insn_emulation_fail;
trace_kvm_emulate_insn_failed(vcpu);
- if (emulation_type & EMULTYPE_VMWARE)
- return EMULATE_FAIL;
+ if (emulation_type & EMULTYPE_VMWARE_GP) {
+ kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
+ return EMULATE_DONE;
+ }
kvm_queue_exception(vcpu, UD_VECTOR);
@@ -6543,9 +6545,11 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
}
}
- if ((emulation_type & EMULTYPE_VMWARE) &&
- !is_vmware_backdoor_opcode(ctxt))
- return EMULATE_FAIL;
+ if ((emulation_type & EMULTYPE_VMWARE_GP) &&
+ !is_vmware_backdoor_opcode(ctxt)) {
+ kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
+ return EMULATE_DONE;
+ }
if (emulation_type & EMULTYPE_SKIP) {
kvm_rip_write(vcpu, ctxt->_eip);
--
2.22.0
Immediately inject a #UD and return EMULATE done if emulation fails when
handling an intercepted #UD. This helps pave the way for removing
EMULATE_FAIL altogether.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a1f9e36b2d58..bff3320aa78e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5328,7 +5328,6 @@ EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
int handle_ud(struct kvm_vcpu *vcpu)
{
int emul_type = EMULTYPE_TRAP_UD;
- enum emulation_result er;
char sig[5]; /* ud2; .ascii "kvm" */
struct x86_exception e;
@@ -5340,12 +5339,7 @@ int handle_ud(struct kvm_vcpu *vcpu)
emul_type = EMULTYPE_TRAP_UD_FORCED;
}
- er = kvm_emulate_instruction(vcpu, emul_type);
- if (er == EMULATE_USER_EXIT)
- return 0;
- if (er != EMULATE_DONE)
- kvm_queue_exception(vcpu, UD_VECTOR);
- return 1;
+ return kvm_emulate_instruction(vcpu, emul_type) != EMULATE_USER_EXIT;
}
EXPORT_SYMBOL_GPL(handle_ud);
@@ -6533,8 +6527,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
++vcpu->stat.insn_emulation;
if (r != EMULATION_OK) {
if ((emulation_type & EMULTYPE_TRAP_UD) ||
- (emulation_type & EMULTYPE_TRAP_UD_FORCED))
- return EMULATE_FAIL;
+ (emulation_type & EMULTYPE_TRAP_UD_FORCED)) {
+ kvm_queue_exception(vcpu, UD_VECTOR);
+ return EMULATE_DONE;
+ }
if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
emulation_type))
return EMULATE_DONE;
--
2.22.0
Return the single-step emulation result directly instead of via an out
param. Presumably at some point in the past kvm_vcpu_do_singlestep()
could be called with *r==EMULATE_USER_EXIT, but that is no longer the
case, i.e. all callers are happy to overwrite their own return variable.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c6de5bc4fa5e..fe847f8eb947 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6377,7 +6377,7 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
return dr6;
}
-static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
+static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu)
{
struct kvm_run *kvm_run = vcpu->run;
@@ -6386,10 +6386,10 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
kvm_run->debug.arch.exception = DB_VECTOR;
kvm_run->exit_reason = KVM_EXIT_DEBUG;
- *r = EMULATE_USER_EXIT;
- } else {
- kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
+ return EMULATE_USER_EXIT;
}
+ kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
+ return EMULATE_DONE;
}
int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
@@ -6410,7 +6410,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
* that sets the TF flag".
*/
if (unlikely(rflags & X86_EFLAGS_TF))
- kvm_vcpu_do_singlestep(vcpu, &r);
+ r = kvm_vcpu_do_singlestep(vcpu);
return r == EMULATE_DONE;
}
EXPORT_SYMBOL_GPL(kvm_skip_emulated_instruction);
@@ -6613,7 +6613,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
kvm_rip_write(vcpu, ctxt->eip);
if (r == EMULATE_DONE && ctxt->tf)
- kvm_vcpu_do_singlestep(vcpu, &r);
+ r = kvm_vcpu_do_singlestep(vcpu);
if (!ctxt->have_exception ||
exception_type(ctxt->exception.vector) == EXCPT_TRAP)
__kvm_set_rflags(vcpu, ctxt->eflags);
--
2.22.0
The VMware backdoor hooks #GP faults on IN{S}, OUT{S}, and RDPMC, none
of which generate a non-zero error code for their #GP. Re-injecting #GP
instead of attempting emulation on a non-zero error code will allow a
future patch to move #GP injection (for emulation failure) into
kvm_emulate_instruction() without having to plumb in the error code.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm.c | 6 +++++-
arch/x86/kvm/vmx/vmx.c | 7 ++++++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5a42f9c70014..b96a119690f4 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2772,11 +2772,15 @@ static int gp_interception(struct vcpu_svm *svm)
WARN_ON_ONCE(!enable_vmware_backdoor);
+ if (error_code) {
+ kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
+ return 1;
+ }
er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
if (er == EMULATE_USER_EXIT)
return 0;
else if (er != EMULATE_DONE)
- kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
+ kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
return 1;
}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6ecf773825e2..3ee0dd304bc7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4509,11 +4509,16 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
WARN_ON_ONCE(!enable_vmware_backdoor);
+
+ if (error_code) {
+ kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
+ return 1;
+ }
er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
if (er == EMULATE_USER_EXIT)
return 0;
else if (er != EMULATE_DONE)
- kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
+ kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
return 1;
}
--
2.22.0
Move the stat.mmio_exits update into x86_emulate_instruction(). This is
both a bug fix, e.g. the current update flows will incorrectly increment
mmio_exits on emulation failure, and a preparatory change to set the
stage for eliminating EMULATE_DONE and company.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/mmu.c | 2 --
arch/x86/kvm/vmx/vmx.c | 1 -
arch/x86/kvm/x86.c | 2 ++
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 4c45ff0cfbd0..845e39d8a970 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -5437,8 +5437,6 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
case EMULATE_DONE:
return 1;
case EMULATE_USER_EXIT:
- ++vcpu->stat.mmio_exits;
- /* fall through */
case EMULATE_FAIL:
return 0;
default:
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 570a233e272b..18286e5b5983 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5200,7 +5200,6 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
err = kvm_emulate_instruction(vcpu, 0);
if (err == EMULATE_USER_EXIT) {
- ++vcpu->stat.mmio_exits;
ret = 0;
goto out;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b4cfd786d0b6..cd425f54096a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6598,6 +6598,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
}
r = EMULATE_USER_EXIT;
} else if (vcpu->mmio_needed) {
+ ++vcpu->stat.mmio_exits;
+
if (!vcpu->mmio_is_write)
writeback = false;
r = EMULATE_USER_EXIT;
--
2.22.0
Sean Christopherson <[email protected]> writes:
> Return the single-step emulation result directly instead of via an out
> param. Presumably at some point in the past kvm_vcpu_do_singlestep()
> could be called with *r==EMULATE_USER_EXIT, but that is no longer the
> case, i.e. all callers are happy to overwrite their own return variable.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/x86.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c6de5bc4fa5e..fe847f8eb947 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6377,7 +6377,7 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
> return dr6;
> }
>
> -static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
> +static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu)
> {
> struct kvm_run *kvm_run = vcpu->run;
>
> @@ -6386,10 +6386,10 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
> kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
> kvm_run->debug.arch.exception = DB_VECTOR;
> kvm_run->exit_reason = KVM_EXIT_DEBUG;
> - *r = EMULATE_USER_EXIT;
> - } else {
> - kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
> + return EMULATE_USER_EXIT;
> }
> + kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
> + return EMULATE_DONE;
> }
>
> int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
> @@ -6410,7 +6410,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
> * that sets the TF flag".
> */
> if (unlikely(rflags & X86_EFLAGS_TF))
> - kvm_vcpu_do_singlestep(vcpu, &r);
> + r = kvm_vcpu_do_singlestep(vcpu);
> return r == EMULATE_DONE;
> }
> EXPORT_SYMBOL_GPL(kvm_skip_emulated_instruction);
> @@ -6613,7 +6613,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
> kvm_rip_write(vcpu, ctxt->eip);
> if (r == EMULATE_DONE && ctxt->tf)
> - kvm_vcpu_do_singlestep(vcpu, &r);
> + r = kvm_vcpu_do_singlestep(vcpu);
> if (!ctxt->have_exception ||
> exception_type(ctxt->exception.vector) == EXCPT_TRAP)
> __kvm_set_rflags(vcpu, ctxt->eflags);
Reviewed-by: Vitaly Kuznetsov <[email protected]>
--
Vitaly
The "no #UD on fail" is used only in the VMWare case, and for the VMWare
scenario it really means "#GP instead of #UD on fail". Remove the flag
in preparation for moving all fault injection into the emulation flow
itself, which in turn will allow eliminating EMULATE_DONE and company.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 -
arch/x86/kvm/svm.c | 3 +--
arch/x86/kvm/vmx/vmx.c | 3 +--
arch/x86/kvm/x86.c | 2 +-
4 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 44a5ce57a905..dd6bd9ed0839 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1318,7 +1318,6 @@ enum emulation_result {
#define EMULTYPE_TRAP_UD (1 << 1)
#define EMULTYPE_SKIP (1 << 2)
#define EMULTYPE_ALLOW_RETRY (1 << 3)
-#define EMULTYPE_NO_UD_ON_FAIL (1 << 4)
#define EMULTYPE_VMWARE (1 << 5)
int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 1f220a85514f..5a42f9c70014 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2772,8 +2772,7 @@ static int gp_interception(struct vcpu_svm *svm)
WARN_ON_ONCE(!enable_vmware_backdoor);
- er = kvm_emulate_instruction(vcpu,
- EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
+ er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
if (er == EMULATE_USER_EXIT)
return 0;
else if (er != EMULATE_DONE)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 18286e5b5983..6ecf773825e2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4509,8 +4509,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
WARN_ON_ONCE(!enable_vmware_backdoor);
- er = kvm_emulate_instruction(vcpu,
- EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
+ er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
if (er == EMULATE_USER_EXIT)
return 0;
else if (er != EMULATE_DONE)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fe847f8eb947..e0f0e14d8fac 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6210,7 +6210,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
++vcpu->stat.insn_emulation_fail;
trace_kvm_emulate_insn_failed(vcpu);
- if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
+ if (emulation_type & EMULTYPE_VMWARE)
return EMULATE_FAIL;
kvm_queue_exception(vcpu, UD_VECTOR);
--
2.22.0
Request triple fault in kvm_inject_realmode_interrupt() instead of
returning EMULATE_FAIL and deferring to the caller. All existing
callers request triple fault and it's highly unlikely Real Mode is
going to acquire new features. While this consolidates a small amount
of code, the real goal is to remove the last reference to EMULATE_FAIL.
No functional change intended.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 9 +++------
arch/x86/kvm/x86.c | 17 ++++++++---------
arch/x86/kvm/x86.h | 2 +-
3 files changed, 12 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 52d5705ff7dc..efc2f913ca19 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1527,8 +1527,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu)
int inc_eip = 0;
if (kvm_exception_is_soft(nr))
inc_eip = vcpu->arch.event_exit_inst_len;
- if (kvm_inject_realmode_interrupt(vcpu, nr, inc_eip) != EMULATE_DONE)
- kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
+ kvm_inject_realmode_interrupt(vcpu, nr, inc_eip);
return;
}
@@ -4276,8 +4275,7 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
int inc_eip = 0;
if (vcpu->arch.interrupt.soft)
inc_eip = vcpu->arch.event_exit_inst_len;
- if (kvm_inject_realmode_interrupt(vcpu, irq, inc_eip) != EMULATE_DONE)
- kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
+ kvm_inject_realmode_interrupt(vcpu, irq, inc_eip);
return;
}
intr = irq | INTR_INFO_VALID_MASK;
@@ -4313,8 +4311,7 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
vmx->loaded_vmcs->nmi_known_unmasked = false;
if (vmx->rmode.vm86_active) {
- if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR, 0) != EMULATE_DONE)
- kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
+ kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR, 0);
return;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 83b3c7e9fce7..e457363622e5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6176,7 +6176,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
vcpu->arch.emulate_regs_need_sync_from_vcpu = false;
}
-int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
+void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
{
struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
int ret;
@@ -6188,14 +6188,13 @@ int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
ctxt->_eip = ctxt->eip + inc_eip;
ret = emulate_int_real(ctxt, irq);
- if (ret != X86EMUL_CONTINUE)
- return EMULATE_FAIL;
-
- ctxt->eip = ctxt->_eip;
- kvm_rip_write(vcpu, ctxt->eip);
- kvm_set_rflags(vcpu, ctxt->eflags);
-
- return EMULATE_DONE;
+ if (ret != X86EMUL_CONTINUE) {
+ kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
+ } else {
+ ctxt->eip = ctxt->_eip;
+ kvm_rip_write(vcpu, ctxt->eip);
+ kvm_set_rflags(vcpu, ctxt->eflags);
+ }
}
EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index b5274e2a53cf..dbf7442a822b 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -261,7 +261,7 @@ static inline bool kvm_check_has_quirk(struct kvm *kvm, u64 quirk)
}
void kvm_set_pending_timer(struct kvm_vcpu *vcpu);
-int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
+void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr);
u64 get_kvmclock_ns(struct kvm *kvm);
--
2.22.0
Add an explicit emulation type for forced #UD emulation and use it to
detect that KVM should unconditionally inject a #UD instead of falling
into its standard emulation failure handling.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/x86.c | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d1d5b5ca1195..a38c93362945 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1318,6 +1318,7 @@ enum emulation_result {
#define EMULTYPE_TRAP_UD (1 << 1)
#define EMULTYPE_SKIP (1 << 2)
#define EMULTYPE_ALLOW_RETRY (1 << 3)
+#define EMULTYPE_TRAP_UD_FORCED (1 << 4)
#define EMULTYPE_VMWARE_GP (1 << 5)
int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 228ca71d5b01..a1f9e36b2d58 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5337,7 +5337,7 @@ int handle_ud(struct kvm_vcpu *vcpu)
sig, sizeof(sig), &e) == 0 &&
memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
- emul_type = 0;
+ emul_type = EMULTYPE_TRAP_UD_FORCED;
}
er = kvm_emulate_instruction(vcpu, emul_type);
@@ -6532,7 +6532,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
trace_kvm_emulate_insn_start(vcpu);
++vcpu->stat.insn_emulation;
if (r != EMULATION_OK) {
- if (emulation_type & EMULTYPE_TRAP_UD)
+ if ((emulation_type & EMULTYPE_TRAP_UD) ||
+ (emulation_type & EMULTYPE_TRAP_UD_FORCED))
return EMULATE_FAIL;
if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
emulation_type))
--
2.22.0
Deferring emulation failure handling (in some cases) to the caller of
x86_emulate_instruction() has proven fragile, e.g. multiple instances of
KVM not setting run->exit_reason on EMULATE_FAIL, largely due to it
being difficult to discern what emulation types can return what result,
and which combination of types and results are handled where.
Now that x86_emulate_instruction() always handles emulation failure,
i.e. EMULATION_FAIL is only referenced in callers, remove the
emulation_result enums entirely. Per KVM's existing exit handling
conventions, return '0' and '1' for "exit to userspace" and "resume
guest" respectively. Doing so cleans up many callers, e.g. they can
return kvm_emulate_instruction() directly instead of having to interpret
its result.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 6 ----
arch/x86/kvm/mmu.c | 14 ++------
arch/x86/kvm/svm.c | 22 ++++++-------
arch/x86/kvm/vmx/vmx.c | 28 ++++++----------
arch/x86/kvm/x86.c | 57 ++++++++++++++++-----------------
5 files changed, 49 insertions(+), 78 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a38c93362945..3f524cea20ad 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1308,12 +1308,6 @@ extern u64 kvm_default_tsc_scaling_ratio;
extern u64 kvm_mce_cap_supported;
-enum emulation_result {
- EMULATE_DONE, /* no further processing */
- EMULATE_USER_EXIT, /* kvm_run ready for userspace exit */
- EMULATE_FAIL, /* can't emulate this instruction */
-};
-
#define EMULTYPE_NO_DECODE (1 << 0)
#define EMULTYPE_TRAP_UD (1 << 1)
#define EMULTYPE_SKIP (1 << 2)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 845e39d8a970..22322f61f794 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -5364,7 +5364,6 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
void *insn, int insn_len)
{
int r, emulation_type = 0;
- enum emulation_result er;
bool direct = vcpu->arch.mmu->direct_map;
/* With shadow page tables, fault_address contains a GVA or nGPA. */
@@ -5431,17 +5430,8 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
return 1;
}
- er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len);
-
- switch (er) {
- case EMULATE_DONE:
- return 1;
- case EMULATE_USER_EXIT:
- case EMULATE_FAIL:
- return 0;
- default:
- BUG();
- }
+ return x86_emulate_instruction(vcpu, cr2, emulation_type, insn,
+ insn_len);
}
EXPORT_SYMBOL_GPL(kvm_mmu_page_fault);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index d9d88cecaba6..f374f11358b7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -787,7 +787,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
kvm_rip_write(vcpu, svm->next_rip);
svm_set_interrupt_shadow(vcpu, 0);
- return EMULATE_DONE;
+ return 1;
}
static void svm_queue_exception(struct kvm_vcpu *vcpu)
@@ -2775,8 +2775,7 @@ static int gp_interception(struct vcpu_svm *svm)
kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
return 1;
}
- return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
- EMULATE_USER_EXIT;
+ return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP);
}
static bool is_erratum_383(void)
@@ -2874,7 +2873,7 @@ static int io_interception(struct vcpu_svm *svm)
string = (io_info & SVM_IOIO_STR_MASK) != 0;
in = (io_info & SVM_IOIO_TYPE_MASK) != 0;
if (string)
- return kvm_emulate_instruction(vcpu, 0) == EMULATE_DONE;
+ return kvm_emulate_instruction(vcpu, 0);
port = io_info >> 16;
size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
@@ -3881,17 +3880,15 @@ static int task_switch_interception(struct vcpu_svm *svm)
int_type == SVM_EXITINTINFO_TYPE_SOFT ||
(int_type == SVM_EXITINTINFO_TYPE_EXEPT &&
(int_vec == OF_VECTOR || int_vec == BP_VECTOR))) {
- if (skip_emulated_instruction(&svm->vcpu) == EMULATE_USER_EXIT)
+ if (!skip_emulated_instruction(&svm->vcpu))
return 0;
}
if (int_type != SVM_EXITINTINFO_TYPE_SOFT)
int_vec = -1;
-
-
return kvm_task_switch(&svm->vcpu, tss_selector, int_vec, reason,
- has_error_code, error_code) != EMULATE_USER_EXIT;
+ has_error_code, error_code);
}
static int cpuid_interception(struct vcpu_svm *svm)
@@ -3912,7 +3909,7 @@ static int iret_interception(struct vcpu_svm *svm)
static int invlpg_interception(struct vcpu_svm *svm)
{
if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
- return kvm_emulate_instruction(&svm->vcpu, 0) == EMULATE_DONE;
+ return kvm_emulate_instruction(&svm->vcpu, 0);
kvm_mmu_invlpg(&svm->vcpu, svm->vmcb->control.exit_info_1);
return kvm_skip_emulated_instruction(&svm->vcpu);
@@ -3920,13 +3917,12 @@ static int invlpg_interception(struct vcpu_svm *svm)
static int emulate_on_interception(struct vcpu_svm *svm)
{
- return kvm_emulate_instruction(&svm->vcpu, 0) == EMULATE_DONE;
+ return kvm_emulate_instruction(&svm->vcpu, 0);
}
static int rsm_interception(struct vcpu_svm *svm)
{
- return kvm_emulate_instruction_from_buffer(&svm->vcpu,
- rsm_ins_bytes, 2) == EMULATE_DONE;
+ return kvm_emulate_instruction_from_buffer(&svm->vcpu, rsm_ins_bytes, 2);
}
static int rdpmc_interception(struct vcpu_svm *svm)
@@ -4745,7 +4741,7 @@ static int avic_unaccelerated_access_interception(struct vcpu_svm *svm)
ret = avic_unaccel_trap_write(svm);
} else {
/* Handling Fault */
- ret = (kvm_emulate_instruction(&svm->vcpu, 0) == EMULATE_DONE);
+ ret = kvm_emulate_instruction(&svm->vcpu, 0);
}
return ret;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 5b5f622cc092..44d868c49301 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1487,7 +1487,7 @@ static int __skip_emulated_instruction(struct kvm_vcpu *vcpu)
/* skipping an emulated instruction also counts */
vmx_set_interrupt_shadow(vcpu, 0);
- return EMULATE_DONE;
+ return 1;
}
static inline void skip_emulated_instruction(struct kvm_vcpu *vcpu)
@@ -4438,7 +4438,7 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
* Cause the #SS fault with 0 error code in VM86 mode.
*/
if (((vec == GP_VECTOR) || (vec == SS_VECTOR)) && err_code == 0) {
- if (kvm_emulate_instruction(vcpu, 0) == EMULATE_DONE) {
+ if (kvm_emulate_instruction(vcpu, 0)) {
if (vcpu->arch.halt_request) {
vcpu->arch.halt_request = 0;
return kvm_vcpu_halt(vcpu);
@@ -4510,8 +4510,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
return 1;
}
- return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
- EMULATE_USER_EXIT;
+ return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP);
}
/*
@@ -4608,7 +4607,7 @@ static int handle_io(struct kvm_vcpu *vcpu)
++vcpu->stat.io_exits;
if (string)
- return kvm_emulate_instruction(vcpu, 0) == EMULATE_DONE;
+ return kvm_emulate_instruction(vcpu, 0);
port = exit_qualification >> 16;
size = (exit_qualification & 7) + 1;
@@ -4682,7 +4681,7 @@ static int handle_set_cr4(struct kvm_vcpu *vcpu, unsigned long val)
static int handle_desc(struct kvm_vcpu *vcpu)
{
WARN_ON(!(vcpu->arch.cr4 & X86_CR4_UMIP));
- return kvm_emulate_instruction(vcpu, 0) == EMULATE_DONE;
+ return kvm_emulate_instruction(vcpu, 0);
}
static int handle_cr(struct kvm_vcpu *vcpu)
@@ -4927,7 +4926,7 @@ static int handle_vmcall(struct kvm_vcpu *vcpu)
static int handle_invd(struct kvm_vcpu *vcpu)
{
- return kvm_emulate_instruction(vcpu, 0) == EMULATE_DONE;
+ return kvm_emulate_instruction(vcpu, 0);
}
static int handle_invlpg(struct kvm_vcpu *vcpu)
@@ -4994,7 +4993,7 @@ static int handle_apic_access(struct kvm_vcpu *vcpu)
return kvm_skip_emulated_instruction(vcpu);
}
}
- return kvm_emulate_instruction(vcpu, 0) == EMULATE_DONE;
+ return kvm_emulate_instruction(vcpu, 0);
}
static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
@@ -5071,7 +5070,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
*/
return kvm_task_switch(vcpu, tss_selector,
type == INTR_TYPE_SOFT_INTR ? idt_index : -1,
- reason, has_error_code, error_code) != EMULATE_USER_EXIT;
+ reason, has_error_code, error_code);
}
static int handle_ept_violation(struct kvm_vcpu *vcpu)
@@ -5143,8 +5142,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
return kvm_skip_emulated_instruction(vcpu);
else
- return kvm_emulate_instruction(vcpu, EMULTYPE_SKIP) ==
- EMULATE_DONE;
+ return kvm_emulate_instruction(vcpu, EMULTYPE_SKIP);
}
return kvm_mmu_page_fault(vcpu, gpa, PFERR_RSVD_MASK, NULL, 0);
@@ -5163,7 +5161,6 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu)
static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
- enum emulation_result err = EMULATE_DONE;
bool intr_window_requested;
unsigned count = 130;
@@ -5184,14 +5181,9 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
if (kvm_test_request(KVM_REQ_EVENT, vcpu))
return 1;
- err = kvm_emulate_instruction(vcpu, 0);
-
- if (err == EMULATE_USER_EXIT)
+ if (!kvm_emulate_instruction(vcpu, 0))
return 0;
- if (WARN_ON_ONCE(err == EMULATE_FAIL))
- return 1;
-
if (vmx->emulation_required && !vmx->rmode.vm86_active &&
vcpu->arch.exception.pending) {
vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e457363622e5..53a16eb2aba8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5339,7 +5339,7 @@ int handle_ud(struct kvm_vcpu *vcpu)
emul_type = EMULTYPE_TRAP_UD_FORCED;
}
- return kvm_emulate_instruction(vcpu, emul_type) != EMULATE_USER_EXIT;
+ return kvm_emulate_instruction(vcpu, emul_type);
}
EXPORT_SYMBOL_GPL(handle_ud);
@@ -6205,14 +6205,14 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
if (emulation_type & EMULTYPE_VMWARE_GP) {
kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
- return EMULATE_DONE;
+ return 1;
}
if (emulation_type & EMULTYPE_SKIP) {
vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
vcpu->run->internal.ndata = 0;
- return EMULATE_USER_EXIT;
+ return 0;
}
kvm_queue_exception(vcpu, UD_VECTOR);
@@ -6221,10 +6221,10 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
vcpu->run->internal.ndata = 0;
- return EMULATE_USER_EXIT;
+ return 0;
}
- return EMULATE_DONE;
+ return 1;
}
static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t cr2,
@@ -6388,10 +6388,10 @@ static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu)
kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
kvm_run->debug.arch.exception = DB_VECTOR;
kvm_run->exit_reason = KVM_EXIT_DEBUG;
- return EMULATE_USER_EXIT;
+ return 0;
}
kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
- return EMULATE_DONE;
+ return 1;
}
int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
@@ -6400,7 +6400,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
int r;
r = kvm_x86_ops->skip_emulated_instruction(vcpu);
- if (unlikely(r != EMULATE_DONE))
+ if (unlikely(!r))
return 0;
/*
@@ -6413,7 +6413,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
*/
if (unlikely(rflags & X86_EFLAGS_TF))
r = kvm_vcpu_do_singlestep(vcpu);
- return r == EMULATE_DONE;
+ return r;
}
EXPORT_SYMBOL_GPL(kvm_skip_emulated_instruction);
@@ -6432,7 +6432,7 @@ static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
kvm_run->debug.arch.pc = eip;
kvm_run->debug.arch.exception = DB_VECTOR;
kvm_run->exit_reason = KVM_EXIT_DEBUG;
- *r = EMULATE_USER_EXIT;
+ *r = 0;
return true;
}
}
@@ -6448,7 +6448,7 @@ static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
vcpu->arch.dr6 &= ~DR_TRAP_BITS;
vcpu->arch.dr6 |= dr6 | DR6_RTM;
kvm_queue_exception(vcpu, DB_VECTOR);
- *r = EMULATE_DONE;
+ *r = 1;
return true;
}
}
@@ -6535,13 +6535,13 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
if ((emulation_type & EMULTYPE_TRAP_UD) ||
(emulation_type & EMULTYPE_TRAP_UD_FORCED)) {
kvm_queue_exception(vcpu, UD_VECTOR);
- return EMULATE_DONE;
+ return 1;
}
if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
emulation_type))
- return EMULATE_DONE;
+ return 1;
if (ctxt->have_exception && inject_emulated_exception(vcpu))
- return EMULATE_DONE;
+ return 1;
return handle_emulation_failure(vcpu, emulation_type);
}
}
@@ -6549,7 +6549,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
if ((emulation_type & EMULTYPE_VMWARE_GP) &&
!is_vmware_backdoor_opcode(ctxt)) {
kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
- return EMULATE_DONE;
+ return 1;
}
if (emulation_type & EMULTYPE_SKIP) {
@@ -6557,11 +6557,11 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
if (ctxt->eflags & X86_EFLAGS_RF)
kvm_set_rflags(vcpu, ctxt->eflags & ~X86_EFLAGS_RF);
kvm_x86_ops->set_interrupt_shadow(vcpu, 0);
- return EMULATE_DONE;
+ return 1;
}
if (retry_instruction(ctxt, cr2, emulation_type))
- return EMULATE_DONE;
+ return 1;
/* this is needed for vmware backdoor interface to work since it
changes registers values during IO operation */
@@ -6577,18 +6577,18 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
r = x86_emulate_insn(ctxt);
if (r == EMULATION_INTERCEPTED)
- return EMULATE_DONE;
+ return 1;
if (r == EMULATION_FAILED) {
if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
emulation_type))
- return EMULATE_DONE;
+ return 1;
return handle_emulation_failure(vcpu, emulation_type);
}
if (ctxt->have_exception) {
- r = EMULATE_DONE;
+ r = 1;
if (inject_emulated_exception(vcpu))
return r;
} else if (vcpu->arch.pio.count) {
@@ -6599,25 +6599,25 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
writeback = false;
vcpu->arch.complete_userspace_io = complete_emulated_pio;
}
- r = EMULATE_USER_EXIT;
+ r = 0;
} else if (vcpu->mmio_needed) {
++vcpu->stat.mmio_exits;
if (!vcpu->mmio_is_write)
writeback = false;
- r = EMULATE_USER_EXIT;
+ r = 0;
vcpu->arch.complete_userspace_io = complete_emulated_mmio;
} else if (r == EMULATION_RESTART)
goto restart;
else
- r = EMULATE_DONE;
+ r = 1;
if (writeback) {
unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
toggle_interruptibility(vcpu, ctxt->interruptibility);
vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
kvm_rip_write(vcpu, ctxt->eip);
- if (r == EMULATE_DONE && ctxt->tf)
+ if (r && ctxt->tf)
r = kvm_vcpu_do_singlestep(vcpu);
if (!ctxt->have_exception ||
exception_type(ctxt->exception.vector) == EXCPT_TRAP)
@@ -8213,12 +8213,11 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
static inline int complete_emulated_io(struct kvm_vcpu *vcpu)
{
int r;
+
vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
r = kvm_emulate_instruction(vcpu, EMULTYPE_NO_DECODE);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
- if (r != EMULATE_DONE)
- return 0;
- return 1;
+ return r;
}
static int complete_emulated_pio(struct kvm_vcpu *vcpu)
@@ -8590,13 +8589,13 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
vcpu->run->internal.ndata = 0;
- return EMULATE_USER_EXIT;
+ return 0;
}
kvm_rip_write(vcpu, ctxt->eip);
kvm_set_rflags(vcpu, ctxt->eflags);
kvm_make_request(KVM_REQ_EVENT, vcpu);
- return EMULATE_DONE;
+ return 1;
}
EXPORT_SYMBOL_GPL(kvm_task_switch);
--
2.22.0
Now that EMULATE_FAIL is completely unused, remove the last remaning
usage where KVM does something functional in response to EMULATE_FAIL.
Leave the check in place as a WARN_ON_ONCE to provide a better paper
trail when EMULATE_{DONE,FAIL,USER_EXIT} are completely removed.
Opportunistically remove the gotos in handle_invalid_guest_state().
With the EMULATE_FAIL handling gone there is no need to have a common
handler for emulation failure and the gotos only complicate things,
e.g. the signal_pending() check always returns '1', but this is far
from obvious when glancing through the code.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index efc2f913ca19..5b5f622cc092 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5164,7 +5164,6 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
enum emulation_result err = EMULATE_DONE;
- int ret = 1;
bool intr_window_requested;
unsigned count = 130;
@@ -5187,38 +5186,38 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
err = kvm_emulate_instruction(vcpu, 0);
- if (err == EMULATE_USER_EXIT) {
- ret = 0;
- goto out;
- }
+ if (err == EMULATE_USER_EXIT)
+ return 0;
- if (err != EMULATE_DONE)
- goto emulation_error;
+ if (WARN_ON_ONCE(err == EMULATE_FAIL))
+ return 1;
if (vmx->emulation_required && !vmx->rmode.vm86_active &&
- vcpu->arch.exception.pending)
- goto emulation_error;
+ vcpu->arch.exception.pending) {
+ vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+ vcpu->run->internal.suberror =
+ KVM_INTERNAL_ERROR_EMULATION;
+ vcpu->run->internal.ndata = 0;
+ return 0;
+ }
if (vcpu->arch.halt_request) {
vcpu->arch.halt_request = 0;
- ret = kvm_vcpu_halt(vcpu);
- goto out;
+ return kvm_vcpu_halt(vcpu);
}
+ /*
+ * Note, return 1 and not 0, vcpu_run() is responsible for
+ * morphing the pending signal into the proper return code.
+ */
if (signal_pending(current))
- goto out;
+ return 1;
+
if (need_resched())
schedule();
}
-out:
- return ret;
-
-emulation_error:
- vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
- vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
- vcpu->run->internal.ndata = 0;
- return 0;
+ return 1;
}
static void grow_ple_window(struct kvm_vcpu *vcpu)
--
2.22.0
Consolidate the reporting of emulation failure into kvm_task_switch()
so that it can return EMULATE_USER_EXIT. This helps pave the way for
removing EMULATE_FAIL altogether.
This also fixes a theoretical bug where task switch interception could
suppress an EMULATE_USER_EXIT return.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm.c | 11 ++---------
arch/x86/kvm/vmx/vmx.c | 14 +++-----------
arch/x86/kvm/x86.c | 9 ++++++---
3 files changed, 11 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index c8e3bef2d586..d9d88cecaba6 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3888,17 +3888,10 @@ static int task_switch_interception(struct vcpu_svm *svm)
if (int_type != SVM_EXITINTINFO_TYPE_SOFT)
int_vec = -1;
- if (kvm_task_switch(&svm->vcpu, tss_selector, int_vec, reason,
- has_error_code, error_code) == EMULATE_FAIL)
- goto fail;
- return 1;
-fail:
- svm->vcpu.run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
- svm->vcpu.run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
- svm->vcpu.run->internal.ndata = 0;
- return 0;
+ return kvm_task_switch(&svm->vcpu, tss_selector, int_vec, reason,
+ has_error_code, error_code) != EMULATE_USER_EXIT;
}
static int cpuid_interception(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 25410c58c758..52d5705ff7dc 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5068,21 +5068,13 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
type != INTR_TYPE_NMI_INTR))
skip_emulated_instruction(vcpu);
- if (kvm_task_switch(vcpu, tss_selector,
- type == INTR_TYPE_SOFT_INTR ? idt_index : -1, reason,
- has_error_code, error_code) == EMULATE_FAIL) {
- vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
- vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
- vcpu->run->internal.ndata = 0;
- return 0;
- }
-
/*
* TODO: What about debug traps on tss switch?
* Are we supposed to inject them and update dr6?
*/
-
- return 1;
+ return kvm_task_switch(vcpu, tss_selector,
+ type == INTR_TYPE_SOFT_INTR ? idt_index : -1,
+ reason, has_error_code, error_code) != EMULATE_USER_EXIT;
}
static int handle_ept_violation(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1a886ec6957d..83b3c7e9fce7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8587,9 +8587,12 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
has_error_code, error_code);
-
- if (ret)
- return EMULATE_FAIL;
+ if (ret) {
+ vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+ vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+ vcpu->run->internal.ndata = 0;
+ return EMULATE_USER_EXIT;
+ }
kvm_rip_write(vcpu, ctxt->eip);
kvm_set_rflags(vcpu, ctxt->eflags);
--
2.22.0
Kill a few birds with one stone by forcing an exit to userspace on skip
emulation failure. This removes a reference to EMULATE_FAIL, fixes a
bug in handle_ept_misconfig() where it would exit to userspace without
setting run->exit_reason, and fixes a theoretical bug in SVM's
task_switch_interception() where it would overwrite run->exit_reason on
a return of EMULATE_USER_EXIT.
Note, this technically doesn't fully fix task_switch_interception()
as it now incorrectly handles EMULATE_FAIL, but in practice there is no
bug as EMULATE_FAIL will never be returned for EMULTYPE_SKIP.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm.c | 4 ++--
arch/x86/kvm/x86.c | 9 +++++++--
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 97562c2c8b7b..c8e3bef2d586 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3881,8 +3881,8 @@ static int task_switch_interception(struct vcpu_svm *svm)
int_type == SVM_EXITINTINFO_TYPE_SOFT ||
(int_type == SVM_EXITINTINFO_TYPE_EXEPT &&
(int_vec == OF_VECTOR || int_vec == BP_VECTOR))) {
- if (skip_emulated_instruction(&svm->vcpu) != EMULATE_DONE)
- goto fail;
+ if (skip_emulated_instruction(&svm->vcpu) == EMULATE_USER_EXIT)
+ return 0;
}
if (int_type != SVM_EXITINTINFO_TYPE_SOFT)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bff3320aa78e..1a886ec6957d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6209,6 +6209,13 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
return EMULATE_DONE;
}
+ if (emulation_type & EMULTYPE_SKIP) {
+ vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+ vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+ vcpu->run->internal.ndata = 0;
+ return EMULATE_USER_EXIT;
+ }
+
kvm_queue_exception(vcpu, UD_VECTOR);
if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
@@ -6536,8 +6543,6 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
return EMULATE_DONE;
if (ctxt->have_exception && inject_emulated_exception(vcpu))
return EMULATE_DONE;
- if (emulation_type & EMULTYPE_SKIP)
- return EMULATE_FAIL;
return handle_emulation_failure(vcpu, emulation_type);
}
}
--
2.22.0
Sean Christopherson <[email protected]> writes:
> Move the stat.mmio_exits update into x86_emulate_instruction(). This is
> both a bug fix, e.g. the current update flows will incorrectly increment
> mmio_exits on emulation failure, and a preparatory change to set the
> stage for eliminating EMULATE_DONE and company.
>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Vitaly Kuznetsov <[email protected]>
This, however, makes me wonder why this is handled in x86-specific code
in the first place, can we just count KVM_EXIT_MMIO exits when handling
KVM_RUN?
> ---
> arch/x86/kvm/mmu.c | 2 --
> arch/x86/kvm/vmx/vmx.c | 1 -
> arch/x86/kvm/x86.c | 2 ++
> 3 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 4c45ff0cfbd0..845e39d8a970 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -5437,8 +5437,6 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
> case EMULATE_DONE:
> return 1;
> case EMULATE_USER_EXIT:
> - ++vcpu->stat.mmio_exits;
> - /* fall through */
> case EMULATE_FAIL:
> return 0;
> default:
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 570a233e272b..18286e5b5983 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5200,7 +5200,6 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
> err = kvm_emulate_instruction(vcpu, 0);
>
> if (err == EMULATE_USER_EXIT) {
> - ++vcpu->stat.mmio_exits;
> ret = 0;
> goto out;
> }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b4cfd786d0b6..cd425f54096a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6598,6 +6598,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> }
> r = EMULATE_USER_EXIT;
> } else if (vcpu->mmio_needed) {
> + ++vcpu->stat.mmio_exits;
> +
> if (!vcpu->mmio_is_write)
> writeback = false;
> r = EMULATE_USER_EXIT;
--
Vitaly
Sean Christopherson <[email protected]> writes:
> The VMware backdoor hooks #GP faults on IN{S}, OUT{S}, and RDPMC, none
> of which generate a non-zero error code for their #GP. Re-injecting #GP
> instead of attempting emulation on a non-zero error code will allow a
> future patch to move #GP injection (for emulation failure) into
> kvm_emulate_instruction() without having to plumb in the error code.
>
> Signed-off-by: Sean Christopherson <[email protected]>
(I just need to get this off my chest)
There was a long-standing issue with #GP interception: in case the
exception has nothing to do with VMware we were getting into infinite
loop of #GPs (and not #GP -> #DF -> #TF), e.g. here is a trace of
platform_info selftest:
<...>-43752 [001] 3615.602298: kvm_exit: reason EXIT_MSR rip 0x4015c2 info 0 0
<...>-43752 [001] 3615.602299: kvm_msr: msr_read ce = 0x0 (#GP)
<...>-43752 [001] 3615.602300: kvm_inj_exception: #GP (0x0)
<...>-43752 [001] 3615.602301: kvm_entry: vcpu 0
<...>-43752 [001] 3615.602302: kvm_exit: reason EXIT_EXCP_GP rip 0x4015c2 info 6a 0
<...>-43752 [001] 3615.602308: kvm_emulate_insn: 0:4015c2: 0f 32
<...>-43752 [001] 3615.602308: kvm_inj_exception: #GP (0x6a)
<...>-43752 [001] 3615.602309: kvm_entry: vcpu 0
<...>-43752 [001] 3615.602310: kvm_exit: reason EXIT_EXCP_GP rip 0x4015c2 info 6a 0
<...>-43752 [001] 3615.602312: kvm_emulate_insn: 0:4015c2: 0f 32
<...>-43752 [001] 3615.602312: kvm_inj_exception: #GP (0x6a)
<...>-43752 [001] 3615.602313: kvm_entry: vcpu 0
and so on.
This commit fixes the issue as the second #GP has error code:
<...>-52213 [006] 3740.739495: kvm_entry: vcpu 0
<...>-52213 [006] 3740.739496: kvm_exit: reason EXIT_MSR rip 0x4015c2 info 0 0
<...>-52213 [006] 3740.739497: kvm_msr: msr_read ce = 0x0 (#GP)
<...>-52213 [006] 3740.739502: kvm_inj_exception: #GP (0x0)
<...>-52213 [006] 3740.739503: kvm_entry: vcpu 0
<...>-52213 [006] 3740.739504: kvm_exit: reason EXIT_EXCP_GP rip 0x4015c2 info 6a 0
<...>-52213 [006] 3740.739505: kvm_inj_exception: #DF (0x0)
<...>-52213 [006] 3740.739506: kvm_entry: vcpu 0
<...>-52213 [006] 3740.739507: kvm_exit: reason EXIT_EXCP_GP rip 0x4015c2 info 42 0
<...>-52213 [006] 3740.739508: kvm_fpu: unload
<...>-52213 [006] 3740.739510: kvm_userspace_exit: reason KVM_EXIT_SHUTDOWN (8)
I'm not exactly sure this covers all possible cases as there might be
other cases when error code is not set but this is definitely an
improvement.
Reviewed-and-tested-by: Vitaly Kuznetsov <[email protected]>
> ---
> arch/x86/kvm/svm.c | 6 +++++-
> arch/x86/kvm/vmx/vmx.c | 7 ++++++-
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5a42f9c70014..b96a119690f4 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2772,11 +2772,15 @@ static int gp_interception(struct vcpu_svm *svm)
>
> WARN_ON_ONCE(!enable_vmware_backdoor);
>
In case you'll be respinning for whatever reason, could you please add a
short comment here (and vmx) saying something like "#GP interception for
VMware backdoor emulation only handles IN{S}, OUT{S}, and RDPMC and none
of these have a non-zero error code set" (I don't like the fact that
we'll need to have two copies of it but I can't think of a better place
for it).
> + if (error_code) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + return 1;
> + }
> er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> if (er == EMULATE_USER_EXIT)
> return 0;
> else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> return 1;
> }
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6ecf773825e2..3ee0dd304bc7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4509,11 +4509,16 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>
> if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
> WARN_ON_ONCE(!enable_vmware_backdoor);
> +
> + if (error_code) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + return 1;
> + }
> er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> if (er == EMULATE_USER_EXIT)
> return 0;
> else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> return 1;
> }
--
Vitaly
Sean Christopherson <[email protected]> writes:
> Immediately inject a #GP when VMware emulation fails and return
> EMULATE_DONE instead of propagating EMULATE_FAIL up the stack. This
> helps pave the way for removing EMULATE_FAIL altogether.
>
> Rename EMULTYPE_VMWARE to EMULTYPE_VMWARE_GP to help document why a #GP
> is injected on emulation failure.
Hm, maybe it's just me but I would've assumed _GP suffix means 'called
from #GP intercept' and not '#GP is injected on failure'. This is no
different here because we only do #GP intercepts for vmware backdoor
emulation and this emulation only happens on #GP.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/svm.c | 9 ++-------
> arch/x86/kvm/vmx/vmx.c | 9 ++-------
> arch/x86/kvm/x86.c | 14 +++++++++-----
> 4 files changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index dd6bd9ed0839..d1d5b5ca1195 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,7 +1318,7 @@ enum emulation_result {
> #define EMULTYPE_TRAP_UD (1 << 1)
> #define EMULTYPE_SKIP (1 << 2)
> #define EMULTYPE_ALLOW_RETRY (1 << 3)
> -#define EMULTYPE_VMWARE (1 << 5)
> +#define EMULTYPE_VMWARE_GP (1 << 5)
> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> void *insn, int insn_len);
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index b96a119690f4..97562c2c8b7b 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2768,7 +2768,6 @@ static int gp_interception(struct vcpu_svm *svm)
> {
> struct kvm_vcpu *vcpu = &svm->vcpu;
> u32 error_code = svm->vmcb->control.exit_info_1;
> - int er;
>
> WARN_ON_ONCE(!enable_vmware_backdoor);
>
> @@ -2776,12 +2775,8 @@ static int gp_interception(struct vcpu_svm *svm)
> kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> return 1;
> }
> - er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> - return 1;
> + return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
> + EMULATE_USER_EXIT;
> }
>
> static bool is_erratum_383(void)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 3ee0dd304bc7..25410c58c758 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4492,7 +4492,6 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> u32 intr_info, ex_no, error_code;
> unsigned long cr2, rip, dr6;
> u32 vect_info;
> - enum emulation_result er;
>
> vect_info = vmx->idt_vectoring_info;
> intr_info = vmx->exit_intr_info;
> @@ -4514,12 +4513,8 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> return 1;
> }
> - er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> - return 1;
> + return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
> + EMULATE_USER_EXIT;
> }
>
> /*
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e0f0e14d8fac..228ca71d5b01 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6210,8 +6210,10 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> ++vcpu->stat.insn_emulation_fail;
> trace_kvm_emulate_insn_failed(vcpu);
>
> - if (emulation_type & EMULTYPE_VMWARE)
> - return EMULATE_FAIL;
> + if (emulation_type & EMULTYPE_VMWARE_GP) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> + return EMULATE_DONE;
> + }
>
> kvm_queue_exception(vcpu, UD_VECTOR);
>
> @@ -6543,9 +6545,11 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> }
> }
>
> - if ((emulation_type & EMULTYPE_VMWARE) &&
> - !is_vmware_backdoor_opcode(ctxt))
> - return EMULATE_FAIL;
> + if ((emulation_type & EMULTYPE_VMWARE_GP) &&
> + !is_vmware_backdoor_opcode(ctxt)) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> + return EMULATE_DONE;
> + }
>
> if (emulation_type & EMULTYPE_SKIP) {
> kvm_rip_write(vcpu, ctxt->_eip);
Reviewed-by: Vitaly Kuznetsov <[email protected]>
--
Vitaly
> On 23 Aug 2019, at 4:06, Sean Christopherson <[email protected]> wrote:
>
> Return the single-step emulation result directly instead of via an out
> param. Presumably at some point in the past kvm_vcpu_do_singlestep()
> could be called with *r==EMULATE_USER_EXIT, but that is no longer the
> case, i.e. all callers are happy to overwrite their own return variable.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/x86.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c6de5bc4fa5e..fe847f8eb947 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6377,7 +6377,7 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
> return dr6;
> }
>
> -static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
> +static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu)
> {
> struct kvm_run *kvm_run = vcpu->run;
>
> @@ -6386,10 +6386,10 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
> kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
> kvm_run->debug.arch.exception = DB_VECTOR;
> kvm_run->exit_reason = KVM_EXIT_DEBUG;
> - *r = EMULATE_USER_EXIT;
> - } else {
> - kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
> + return EMULATE_USER_EXIT;
> }
> + kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
> + return EMULATE_DONE;
> }
>
> int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
> @@ -6410,7 +6410,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
> * that sets the TF flag".
> */
> if (unlikely(rflags & X86_EFLAGS_TF))
> - kvm_vcpu_do_singlestep(vcpu, &r);
> + r = kvm_vcpu_do_singlestep(vcpu);
> return r == EMULATE_DONE;
> }
> EXPORT_SYMBOL_GPL(kvm_skip_emulated_instruction);
> @@ -6613,7 +6613,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
> kvm_rip_write(vcpu, ctxt->eip);
> if (r == EMULATE_DONE && ctxt->tf)
> - kvm_vcpu_do_singlestep(vcpu, &r);
> + r = kvm_vcpu_do_singlestep(vcpu);
> if (!ctxt->have_exception ||
> exception_type(ctxt->exception.vector) == EXCPT_TRAP)
> __kvm_set_rflags(vcpu, ctxt->eflags);
> --
> 2.22.0
>
Reviewed-by: Liran Alon <[email protected]>
-Liran
> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
>
> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
> scenario it really means "#GP instead of #UD on fail". Remove the flag
> in preparation for moving all fault injection into the emulation flow
> itself, which in turn will allow eliminating EMULATE_DONE and company.
>
> Signed-off-by: Sean Christopherson <[email protected]>
When I created the commit which introduced this
e23661712005 ("KVM: x86: Add emulation_type to not raise #UD on emulation failure")
I intentionally introduced a new flag to emulation_type instead of using EMULTYPE_VMWARE
as I thought it’s weird to couple this behaviour specifically with VMware emulation.
As it made sense to me that there could be more scenarios in which some VMExit handler
would like to use the x86 emulator but in case of failure want to decide what would be
the failure handling from the outside. I also didn’t want the x86 emulator to be aware
of VMware interception internals.
Having said that, one could argue that the x86 emulator already knows about the VMware
interception internals because of how x86_emulate_instruction() use is_vmware_backdoor_opcode()
and from the mere existence of EMULTYPE_VMWARE. So I think it’s legit to decide
that we will just move all the VMware interception logic into the x86 emulator. Including
handling emulation failures. But then, I would make this patch of yours to also
modify handle_emulation_failure() to queue #GP to guest directly instead of #GP intercept
in VMX/SVM to do so.
I see you do it in a later patch "KVM: x86: Move #GP injection for VMware into x86_emulate_instruction()"
but I think this should just be squashed with this patch to make sense.
To sum-up, I agree with your approach but I recommend you squash this patch and patch 6 of the series to one
and change commit message to explain that you just move entire handling of VMware interception into
the x86 emulator. Instead of providing explanations such as VMware emulation is the only one that use
“no #UD on fail”.
The diff itself looks fine to me, therefore:
Reviewed-by: Liran Alon <[email protected]>
-Liran
> ---
> arch/x86/include/asm/kvm_host.h | 1 -
> arch/x86/kvm/svm.c | 3 +--
> arch/x86/kvm/vmx/vmx.c | 3 +--
> arch/x86/kvm/x86.c | 2 +-
> 4 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 44a5ce57a905..dd6bd9ed0839 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,7 +1318,6 @@ enum emulation_result {
> #define EMULTYPE_TRAP_UD (1 << 1)
> #define EMULTYPE_SKIP (1 << 2)
> #define EMULTYPE_ALLOW_RETRY (1 << 3)
> -#define EMULTYPE_NO_UD_ON_FAIL (1 << 4)
> #define EMULTYPE_VMWARE (1 << 5)
> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1f220a85514f..5a42f9c70014 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2772,8 +2772,7 @@ static int gp_interception(struct vcpu_svm *svm)
>
> WARN_ON_ONCE(!enable_vmware_backdoor);
>
> - er = kvm_emulate_instruction(vcpu,
> - EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> + er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> if (er == EMULATE_USER_EXIT)
> return 0;
> else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 18286e5b5983..6ecf773825e2 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4509,8 +4509,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>
> if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
> WARN_ON_ONCE(!enable_vmware_backdoor);
> - er = kvm_emulate_instruction(vcpu,
> - EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> + er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> if (er == EMULATE_USER_EXIT)
> return 0;
> else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fe847f8eb947..e0f0e14d8fac 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6210,7 +6210,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> ++vcpu->stat.insn_emulation_fail;
> trace_kvm_emulate_insn_failed(vcpu);
>
> - if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
> + if (emulation_type & EMULTYPE_VMWARE)
> return EMULATE_FAIL;
>
> kvm_queue_exception(vcpu, UD_VECTOR);
> --
> 2.22.0
>
> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
>
> Immediately inject a #GP when VMware emulation fails and return
> EMULATE_DONE instead of propagating EMULATE_FAIL up the stack. This
> helps pave the way for removing EMULATE_FAIL altogether.
>
> Rename EMULTYPE_VMWARE to EMULTYPE_VMWARE_GP to help document why a #GP
> is injected on emulation failure.
I would rephrase to say that this rename is in order to document that the x86 emulator is called to handle
VMware #GP interception. In theory, VMware could have also added weird behaviour
to #UD interception as-well. :P
Besides minor comments inline below:
Reviewed-by: Liran Alon <[email protected]>
-Liran
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/svm.c | 9 ++-------
> arch/x86/kvm/vmx/vmx.c | 9 ++-------
> arch/x86/kvm/x86.c | 14 +++++++++-----
> 4 files changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index dd6bd9ed0839..d1d5b5ca1195 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,7 +1318,7 @@ enum emulation_result {
> #define EMULTYPE_TRAP_UD (1 << 1)
> #define EMULTYPE_SKIP (1 << 2)
> #define EMULTYPE_ALLOW_RETRY (1 << 3)
> -#define EMULTYPE_VMWARE (1 << 5)
> +#define EMULTYPE_VMWARE_GP (1 << 5)
> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> void *insn, int insn_len);
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index b96a119690f4..97562c2c8b7b 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2768,7 +2768,6 @@ static int gp_interception(struct vcpu_svm *svm)
> {
> struct kvm_vcpu *vcpu = &svm->vcpu;
> u32 error_code = svm->vmcb->control.exit_info_1;
> - int er;
>
> WARN_ON_ONCE(!enable_vmware_backdoor);
>
> @@ -2776,12 +2775,8 @@ static int gp_interception(struct vcpu_svm *svm)
> kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> return 1;
> }
> - er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> - return 1;
> + return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
> + EMULATE_USER_EXIT;
> }
>
> static bool is_erratum_383(void)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 3ee0dd304bc7..25410c58c758 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4492,7 +4492,6 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> u32 intr_info, ex_no, error_code;
> unsigned long cr2, rip, dr6;
> u32 vect_info;
> - enum emulation_result er;
>
> vect_info = vmx->idt_vectoring_info;
> intr_info = vmx->exit_intr_info;
> @@ -4514,12 +4513,8 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> return 1;
> }
> - er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> - return 1;
> + return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP) !=
> + EMULATE_USER_EXIT;
> }
>
> /*
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e0f0e14d8fac..228ca71d5b01 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6210,8 +6210,10 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> ++vcpu->stat.insn_emulation_fail;
> trace_kvm_emulate_insn_failed(vcpu);
>
> - if (emulation_type & EMULTYPE_VMWARE)
> - return EMULATE_FAIL;
> + if (emulation_type & EMULTYPE_VMWARE_GP) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
I would add here a comment explaining why you can assume #GP error-code is 0.
i.e. Explain that’s because VMware #GP interception is only related to IN{S}, OUT{S} and RDPMC
instructions which all of them raise #GP with error-code of 0.
> + return EMULATE_DONE;
> + }
>
> kvm_queue_exception(vcpu, UD_VECTOR);
>
> @@ -6543,9 +6545,11 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> }
> }
>
> - if ((emulation_type & EMULTYPE_VMWARE) &&
> - !is_vmware_backdoor_opcode(ctxt))
> - return EMULATE_FAIL;
> + if ((emulation_type & EMULTYPE_VMWARE_GP) &&
> + !is_vmware_backdoor_opcode(ctxt)) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
Same here.
> + return EMULATE_DONE;
> + }
>
> if (emulation_type & EMULTYPE_SKIP) {
> kvm_rip_write(vcpu, ctxt->_eip);
> --
> 2.22.0
>
> On 23 Aug 2019, at 16:21, Liran Alon <[email protected]> wrote:
>
>
>
>> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
>>
>> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
>> scenario it really means "#GP instead of #UD on fail". Remove the flag
>> in preparation for moving all fault injection into the emulation flow
>> itself, which in turn will allow eliminating EMULATE_DONE and company.
>>
>> Signed-off-by: Sean Christopherson <[email protected]>
>
> When I created the commit which introduced this
> e23661712005 ("KVM: x86: Add emulation_type to not raise #UD on emulation failure")
> I intentionally introduced a new flag to emulation_type instead of using EMULTYPE_VMWARE
> as I thought it’s weird to couple this behaviour specifically with VMware emulation.
> As it made sense to me that there could be more scenarios in which some VMExit handler
> would like to use the x86 emulator but in case of failure want to decide what would be
> the failure handling from the outside. I also didn’t want the x86 emulator to be aware
> of VMware interception internals.
>
> Having said that, one could argue that the x86 emulator already knows about the VMware
> interception internals because of how x86_emulate_instruction() use is_vmware_backdoor_opcode()
> and from the mere existence of EMULTYPE_VMWARE. So I think it’s legit to decide
> that we will just move all the VMware interception logic into the x86 emulator. Including
> handling emulation failures. But then, I would make this patch of yours to also
> modify handle_emulation_failure() to queue #GP to guest directly instead of #GP intercept
> in VMX/SVM to do so.
> I see you do it in a later patch "KVM: x86: Move #GP injection for VMware into x86_emulate_instruction()"
> but I think this should just be squashed with this patch to make sense.
>
> To sum-up, I agree with your approach but I recommend you squash this patch and patch 6 of the series to one
> and change commit message to explain that you just move entire handling of VMware interception into
> the x86 emulator. Instead of providing explanations such as VMware emulation is the only one that use
> “no #UD on fail”.
After reading patch 5 as-well, I would recommend to first apply patch 5 (filter out #GP with error-code != 0)
and only then apply 4+6.
-Liran
>
> The diff itself looks fine to me, therefore:
> Reviewed-by: Liran Alon <[email protected]>
>
> -Liran
>
>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 -
>> arch/x86/kvm/svm.c | 3 +--
>> arch/x86/kvm/vmx/vmx.c | 3 +--
>> arch/x86/kvm/x86.c | 2 +-
>> 4 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 44a5ce57a905..dd6bd9ed0839 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1318,7 +1318,6 @@ enum emulation_result {
>> #define EMULTYPE_TRAP_UD (1 << 1)
>> #define EMULTYPE_SKIP (1 << 2)
>> #define EMULTYPE_ALLOW_RETRY (1 << 3)
>> -#define EMULTYPE_NO_UD_ON_FAIL (1 << 4)
>> #define EMULTYPE_VMWARE (1 << 5)
>> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
>> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 1f220a85514f..5a42f9c70014 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2772,8 +2772,7 @@ static int gp_interception(struct vcpu_svm *svm)
>>
>> WARN_ON_ONCE(!enable_vmware_backdoor);
>>
>> - er = kvm_emulate_instruction(vcpu,
>> - EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
>> + er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
>> if (er == EMULATE_USER_EXIT)
>> return 0;
>> else if (er != EMULATE_DONE)
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 18286e5b5983..6ecf773825e2 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -4509,8 +4509,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>>
>> if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
>> WARN_ON_ONCE(!enable_vmware_backdoor);
>> - er = kvm_emulate_instruction(vcpu,
>> - EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
>> + er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
>> if (er == EMULATE_USER_EXIT)
>> return 0;
>> else if (er != EMULATE_DONE)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index fe847f8eb947..e0f0e14d8fac 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6210,7 +6210,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>> ++vcpu->stat.insn_emulation_fail;
>> trace_kvm_emulate_insn_failed(vcpu);
>>
>> - if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
>> + if (emulation_type & EMULTYPE_VMWARE)
>> return EMULATE_FAIL;
>>
>> kvm_queue_exception(vcpu, UD_VECTOR);
>> --
>> 2.22.0
>>
>
> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
>
> The VMware backdoor hooks #GP faults on IN{S}, OUT{S}, and RDPMC, none
> of which generate a non-zero error code for their #GP. Re-injecting #GP
> instead of attempting emulation on a non-zero error code will allow a
> future patch to move #GP injection (for emulation failure) into
> kvm_emulate_instruction() without having to plumb in the error code.
>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Liran Alon <[email protected]>
-Liran
> ---
> arch/x86/kvm/svm.c | 6 +++++-
> arch/x86/kvm/vmx/vmx.c | 7 ++++++-
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5a42f9c70014..b96a119690f4 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2772,11 +2772,15 @@ static int gp_interception(struct vcpu_svm *svm)
>
> WARN_ON_ONCE(!enable_vmware_backdoor);
>
> + if (error_code) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + return 1;
> + }
> er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> if (er == EMULATE_USER_EXIT)
> return 0;
> else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> return 1;
> }
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6ecf773825e2..3ee0dd304bc7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4509,11 +4509,16 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>
> if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
> WARN_ON_ONCE(!enable_vmware_backdoor);
> +
> + if (error_code) {
> + kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + return 1;
> + }
> er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> if (er == EMULATE_USER_EXIT)
> return 0;
> else if (er != EMULATE_DONE)
> - kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> + kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> return 1;
> }
>
> --
> 2.22.0
>
> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
>
> Add an explicit emulation type for forced #UD emulation and use it to
> detect that KVM should unconditionally inject a #UD instead of falling
> into its standard emulation failure handling.
>
> Signed-off-by: Sean Christopherson <[email protected]>
The name "forced emulation on #UD" is not clear to me.
If I understand correctly, EMULTYPE_TRAP_UD is currently used to indicate
that in case the x86 emulator fails to decode instruction, the caller would like
the x86 emulator to fail early such that it can handle this condition properly.
Thus, I would rename it EMULTYPE_TRAP_DECODE_FAILURE.
But this new flag seems to do the same. So I’m left confused.
I’m probably missing something trivial here.
-Liran
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/x86.c | 5 +++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index d1d5b5ca1195..a38c93362945 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,6 +1318,7 @@ enum emulation_result {
> #define EMULTYPE_TRAP_UD (1 << 1)
> #define EMULTYPE_SKIP (1 << 2)
> #define EMULTYPE_ALLOW_RETRY (1 << 3)
> +#define EMULTYPE_TRAP_UD_FORCED (1 << 4)
> #define EMULTYPE_VMWARE_GP (1 << 5)
> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 228ca71d5b01..a1f9e36b2d58 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5337,7 +5337,7 @@ int handle_ud(struct kvm_vcpu *vcpu)
> sig, sizeof(sig), &e) == 0 &&
> memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
> kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
> - emul_type = 0;
> + emul_type = EMULTYPE_TRAP_UD_FORCED;
> }
>
> er = kvm_emulate_instruction(vcpu, emul_type);
> @@ -6532,7 +6532,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> trace_kvm_emulate_insn_start(vcpu);
> ++vcpu->stat.insn_emulation;
> if (r != EMULATION_OK) {
> - if (emulation_type & EMULTYPE_TRAP_UD)
> + if ((emulation_type & EMULTYPE_TRAP_UD) ||
> + (emulation_type & EMULTYPE_TRAP_UD_FORCED))
> return EMULATE_FAIL;
> if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
> emulation_type))
> --
> 2.22.0
>
> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
>
> Immediately inject a #UD and return EMULATE done if emulation fails when
> handling an intercepted #UD. This helps pave the way for removing
> EMULATE_FAIL altogether.
>
> Signed-off-by: Sean Christopherson <[email protected]>
I suggest squashing this commit which previous one.
-Liran
> ---
> arch/x86/kvm/x86.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a1f9e36b2d58..bff3320aa78e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5328,7 +5328,6 @@ EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
> int handle_ud(struct kvm_vcpu *vcpu)
> {
> int emul_type = EMULTYPE_TRAP_UD;
> - enum emulation_result er;
> char sig[5]; /* ud2; .ascii "kvm" */
> struct x86_exception e;
>
> @@ -5340,12 +5339,7 @@ int handle_ud(struct kvm_vcpu *vcpu)
> emul_type = EMULTYPE_TRAP_UD_FORCED;
> }
>
> - er = kvm_emulate_instruction(vcpu, emul_type);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - if (er != EMULATE_DONE)
> - kvm_queue_exception(vcpu, UD_VECTOR);
> - return 1;
> + return kvm_emulate_instruction(vcpu, emul_type) != EMULATE_USER_EXIT;
> }
> EXPORT_SYMBOL_GPL(handle_ud);
>
> @@ -6533,8 +6527,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> ++vcpu->stat.insn_emulation;
> if (r != EMULATION_OK) {
> if ((emulation_type & EMULTYPE_TRAP_UD) ||
> - (emulation_type & EMULTYPE_TRAP_UD_FORCED))
> - return EMULATE_FAIL;
> + (emulation_type & EMULTYPE_TRAP_UD_FORCED)) {
> + kvm_queue_exception(vcpu, UD_VECTOR);
> + return EMULATE_DONE;
> + }
> if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
> emulation_type))
> return EMULATE_DONE;
> --
> 2.22.0
>
On Fri, Aug 23, 2019 at 04:47:14PM +0300, Liran Alon wrote:
>
>
> > On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
> >
> > Add an explicit emulation type for forced #UD emulation and use it to
> > detect that KVM should unconditionally inject a #UD instead of falling
> > into its standard emulation failure handling.
> >
> > Signed-off-by: Sean Christopherson <[email protected]>
>
> The name "forced emulation on #UD" is not clear to me.
>
> If I understand correctly, EMULTYPE_TRAP_UD is currently used to indicate
> that in case the x86 emulator fails to decode instruction, the caller would
> like the x86 emulator to fail early such that it can handle this condition
> properly. Thus, I would rename it EMULTYPE_TRAP_DECODE_FAILURE.
EMULTYPE_TRAP_UD is used when KVM intercepts a #UD from hardware. KVM
only emulates select instructions in this case in order to minmize the
emulator attack surface, e.g.:
if (unlikely(ctxt->ud) && likely(!(ctxt->d & EmulateOnUD)))
return EMULATION_FAILED;
To enable testing of the emulator, KVM recognizes a special "opcode" that
triggers full emulation on #UD, e.g. ctxt->ud is false when the #UD was
triggered with the magic prefix. The prefix is only recognized when the
module param force_emulation_prefix is toggled on, hence the name
EMULTYPE_TRAP_UD_FORCED.
> But this new flag seems to do the same. So I’m left confused. I’m probably
> missing something trivial here.
On Fri, Aug 23, 2019 at 11:15:18AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <[email protected]> writes:
>
> > Move the stat.mmio_exits update into x86_emulate_instruction(). This is
> > both a bug fix, e.g. the current update flows will incorrectly increment
> > mmio_exits on emulation failure, and a preparatory change to set the
> > stage for eliminating EMULATE_DONE and company.
> >
> > Signed-off-by: Sean Christopherson <[email protected]>
>
> Reviewed-by: Vitaly Kuznetsov <[email protected]>
>
> This, however, makes me wonder why this is handled in x86-specific code
> in the first place, can we just count KVM_EXIT_MMIO exits when handling
> KVM_RUN?
struct kvm_vcpu_stat is arch specific. At a glance, everyone is counting
similar things, but often in slightly different ways. E.g. PowerPC and
ARM count MMIO exits, but PowerPC counts all exits, ARM has separate
counters for in-kernel vs. userspace, and x86 counts only userspace.
> On 23 Aug 2019, at 17:44, Sean Christopherson <[email protected]> wrote:
>
> On Fri, Aug 23, 2019 at 04:47:14PM +0300, Liran Alon wrote:
>>
>>
>>> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
>>>
>>> Add an explicit emulation type for forced #UD emulation and use it to
>>> detect that KVM should unconditionally inject a #UD instead of falling
>>> into its standard emulation failure handling.
>>>
>>> Signed-off-by: Sean Christopherson <[email protected]>
>>
>> The name "forced emulation on #UD" is not clear to me.
>>
>> If I understand correctly, EMULTYPE_TRAP_UD is currently used to indicate
>> that in case the x86 emulator fails to decode instruction, the caller would
>> like the x86 emulator to fail early such that it can handle this condition
>> properly. Thus, I would rename it EMULTYPE_TRAP_DECODE_FAILURE.
>
> EMULTYPE_TRAP_UD is used when KVM intercepts a #UD from hardware. KVM
> only emulates select instructions in this case in order to minmize the
> emulator attack surface, e.g.:
>
> if (unlikely(ctxt->ud) && likely(!(ctxt->d & EmulateOnUD)))
> return EMULATION_FAILED;
>
> To enable testing of the emulator, KVM recognizes a special "opcode" that
> triggers full emulation on #UD, e.g. ctxt->ud is false when the #UD was
> triggered with the magic prefix. The prefix is only recognized when the
> module param force_emulation_prefix is toggled on, hence the name
> EMULTYPE_TRAP_UD_FORCED.
Ah-ha. This makes sense. Thanks for the explanation.
I would say it’s worth putting a comment in it in code…
>
>> But this new flag seems to do the same. So I’m left confused. I’m probably
>> missing something trivial here.
On Fri, Aug 23, 2019 at 04:32:05PM +0300, Liran Alon wrote:
>
> > On 23 Aug 2019, at 16:21, Liran Alon <[email protected]> wrote:
> >
> >> On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
> >>
> >> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
> >> scenario it really means "#GP instead of #UD on fail". Remove the flag
> >> in preparation for moving all fault injection into the emulation flow
> >> itself, which in turn will allow eliminating EMULATE_DONE and company.
> >>
> >> Signed-off-by: Sean Christopherson <[email protected]>
> >
> > When I created the commit which introduced this e23661712005 ("KVM: x86:
> > Add emulation_type to not raise #UD on emulation failure") I intentionally
> > introduced a new flag to emulation_type instead of using EMULTYPE_VMWARE as
> > I thought it’s weird to couple this behaviour specifically with VMware
> > emulation. As it made sense to me that there could be more scenarios in
> > which some VMExit handler would like to use the x86 emulator but in case of
> > failure want to decide what would be the failure handling from the outside.
> > I also didn’t want the x86 emulator to be aware of VMware interception
> > internals.
> >
> > Having said that, one could argue that the x86 emulator already knows about
> > the VMware interception internals because of how x86_emulate_instruction()
> > use is_vmware_backdoor_opcode() and from the mere existence of
> > EMULTYPE_VMWARE. So I think it’s legit to decide that we will just move all
> > the VMware interception logic into the x86 emulator. Including handling
> > emulation failures. But then, I would make this patch of yours to also
> > modify handle_emulation_failure() to queue #GP to guest directly instead of
> > #GP intercept in VMX/SVM to do so. I see you do it in a later patch "KVM:
> > x86: Move #GP injection for VMware into x86_emulate_instruction()" but I
> > think this should just be squashed with this patch to make sense.
> >
> > To sum-up, I agree with your approach but I recommend you squash this patch
> > and patch 6 of the series to one and change commit message to explain that
> > you just move entire handling of VMware interception into the x86 emulator.
> > Instead of providing explanations such as VMware emulation is the only one
> > that use “no #UD on fail”.
>
> After reading patch 5 as-well, I would recommend to first apply patch 5
> (filter out #GP with error-code != 0) and only then apply 4+6.
Works for me.
On Fri, Aug 23, 2019 at 04:48:16PM +0300, Liran Alon wrote:
>
>
> > On 23 Aug 2019, at 4:07, Sean Christopherson <[email protected]> wrote:
> >
> > Immediately inject a #UD and return EMULATE done if emulation fails when
> > handling an intercepted #UD. This helps pave the way for removing
> > EMULATE_FAIL altogether.
> >
> > Signed-off-by: Sean Christopherson <[email protected]>
>
> I suggest squashing this commit which previous one.
Missed this comment first time around...
I'd like to keep the two patches separate in this case. Adding the
EMULTYPE_TRAP_UD_FORCED flag is a functional change, whereas this patch
is purely a refactor.