* [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths
@ 2019-08-08 17:30 Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 1/7] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP Vitaly Kuznetsov
` (6 more replies)
0 siblings, 7 replies; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
Changes since v2 [Sean Christopherson]:
- Add Reviewed-by tags:
- PATCH2 replaced with the suggested "x86: kvm: svm: propagate errors from
skip_emulated_instruction()" approach.
- PATCH5 split into three separating vmrun_interception() from others and
implementing the suggested solution.
Original description:
Jim rightfully complains that hardcoding instuctions lengths is not always
correct: additional (redundant) prefixes can be used. Luckily, the ugliness
is mostly harmless: modern AMD CPUs support NRIP_SAVE feature but I'd like
to clean things up and sacrifice speed in favor of correctness.
Vitaly Kuznetsov (7):
x86: KVM: svm: don't pretend to advance RIP in case
wrmsr_interception() results in #GP
x86: kvm: svm: propagate errors from skip_emulated_instruction()
x86: KVM: clear interrupt shadow on EMULTYPE_SKIP
x86: KVM: add xsetbv to the emulator
x86: KVM: svm: remove hardcoded instruction length from intercepts
x86: KVM: svm: eliminate weird goto from vmrun_interception()
x86: KVM: svm: eliminate hardcoded RIP advancement from
vmrun_interception()
arch/x86/include/asm/kvm_emulate.h | 3 +-
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/emulate.c | 23 ++++++-
arch/x86/kvm/svm.c | 98 +++++++++++++-----------------
arch/x86/kvm/vmx/vmx.c | 8 ++-
arch/x86/kvm/x86.c | 13 +++-
6 files changed, 83 insertions(+), 64 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/7] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
@ 2019-08-08 17:30 ` Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 2/7] x86: kvm: svm: propagate errors from skip_emulated_instruction() Vitaly Kuznetsov
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
svm->next_rip is only used by skip_emulated_instruction() and in case
kvm_set_msr() fails we rightfully don't do that. Move svm->next_rip
advancement to 'else' branch to avoid creating false impression that
it's always advanced (and make it look like rdmsr_interception()).
This is a preparatory change to removing hardcoded RIP advancement
from instruction intercepts, no functional change.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/svm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 7eafc6907861..7e843b340490 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4447,13 +4447,13 @@ static int wrmsr_interception(struct vcpu_svm *svm)
msr.index = ecx;
msr.host_initiated = false;
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
if (kvm_set_msr(&svm->vcpu, &msr)) {
trace_kvm_msr_write_ex(ecx, data);
kvm_inject_gp(&svm->vcpu, 0);
return 1;
} else {
trace_kvm_msr_write(ecx, data);
+ svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
return kvm_skip_emulated_instruction(&svm->vcpu);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/7] x86: kvm: svm: propagate errors from skip_emulated_instruction()
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 1/7] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP Vitaly Kuznetsov
@ 2019-08-08 17:30 ` Vitaly Kuznetsov
2019-08-09 18:31 ` Sean Christopherson
2019-08-08 17:30 ` [PATCH v3 3/7] x86: KVM: clear interrupt shadow on EMULTYPE_SKIP Vitaly Kuznetsov
` (4 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
On AMD, kvm_x86_ops->skip_emulated_instruction(vcpu) can, in theory,
fail: in !nrips case we call kvm_emulate_instruction(EMULTYPE_SKIP).
Currently, we only do printk(KERN_DEBUG) when this happens and this
is not ideal. Propagate the error up the stack.
On VMX, skip_emulated_instruction() doesn't fail, we have two call
sites calling it explicitly: handle_exception_nmi() and
handle_task_switch(), we can just ignore the result.
On SVM, we also have two explicit call sites:
svm_queue_exception() and it seems we don't need to do anything there as
we check if RIP was advanced or not. In task_switch_interception(),
however, we are better off not proceeding to kvm_task_switch() in case
skip_emulated_instruction() failed.
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/svm.c | 36 ++++++++++++++++++---------------
arch/x86/kvm/vmx/vmx.c | 8 +++++---
arch/x86/kvm/x86.c | 6 ++++--
4 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 7b0a4ee77313..f9e6d0b0f581 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1068,7 +1068,7 @@ struct kvm_x86_ops {
void (*run)(struct kvm_vcpu *vcpu);
int (*handle_exit)(struct kvm_vcpu *vcpu);
- void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
+ int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu, int mask);
u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu);
void (*patch_hypercall)(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 7e843b340490..8299b0de06e2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -770,7 +770,7 @@ static void svm_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
}
-static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
+static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
@@ -779,18 +779,17 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
svm->next_rip = svm->vmcb->control.next_rip;
}
- if (!svm->next_rip) {
- if (kvm_emulate_instruction(vcpu, EMULTYPE_SKIP) !=
- EMULATE_DONE)
- printk(KERN_DEBUG "%s: NOP\n", __func__);
- return;
- }
+ if (!svm->next_rip)
+ return kvm_emulate_instruction(vcpu, EMULTYPE_SKIP);
+
if (svm->next_rip - kvm_rip_read(vcpu) > MAX_INST_SIZE)
printk(KERN_ERR "%s: ip 0x%lx next 0x%llx\n",
__func__, kvm_rip_read(vcpu), svm->next_rip);
kvm_rip_write(vcpu, svm->next_rip);
svm_set_interrupt_shadow(vcpu, 0);
+
+ return EMULATE_DONE;
}
static void svm_queue_exception(struct kvm_vcpu *vcpu)
@@ -821,7 +820,7 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu)
* raises a fault that is not intercepted. Still better than
* failing in all cases.
*/
- skip_emulated_instruction(&svm->vcpu);
+ (void)skip_emulated_instruction(&svm->vcpu);
rip = kvm_rip_read(&svm->vcpu);
svm->int3_rip = rip + svm->vmcb->save.cs.base;
svm->int3_injected = rip - old_rip;
@@ -3899,20 +3898,25 @@ static int task_switch_interception(struct vcpu_svm *svm)
if (reason != TASK_SWITCH_GATE ||
int_type == SVM_EXITINTINFO_TYPE_SOFT ||
(int_type == SVM_EXITINTINFO_TYPE_EXEPT &&
- (int_vec == OF_VECTOR || int_vec == BP_VECTOR)))
- skip_emulated_instruction(&svm->vcpu);
+ (int_vec == OF_VECTOR || int_vec == BP_VECTOR))) {
+ if (skip_emulated_instruction(&svm->vcpu) != EMULATE_DONE)
+ goto fail;
+ }
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) {
- 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;
- }
+ 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;
}
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 074385c86c09..2579e7a6d59d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1473,7 +1473,7 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data)
}
-static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
+static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
{
unsigned long rip;
@@ -1483,6 +1483,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
/* skipping an emulated instruction also counts */
vmx_set_interrupt_shadow(vcpu, 0);
+
+ return EMULATE_DONE;
}
static void vmx_clear_hlt(struct kvm_vcpu *vcpu)
@@ -4547,7 +4549,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
vcpu->arch.dr6 &= ~DR_TRAP_BITS;
vcpu->arch.dr6 |= dr6 | DR6_RTM;
if (is_icebp(intr_info))
- skip_emulated_instruction(vcpu);
+ (void)skip_emulated_instruction(vcpu);
kvm_queue_exception(vcpu, DB_VECTOR);
return 1;
@@ -5057,7 +5059,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
if (!idt_v || (type != INTR_TYPE_HARD_EXCEPTION &&
type != INTR_TYPE_EXT_INTR &&
type != INTR_TYPE_NMI_INTR))
- skip_emulated_instruction(vcpu);
+ (void)skip_emulated_instruction(vcpu);
if (kvm_task_switch(vcpu, tss_selector,
type == INTR_TYPE_SOFT_INTR ? idt_index : -1, reason,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c6d951cbd76c..a97818b1111d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6383,9 +6383,11 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
{
unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
- int r = EMULATE_DONE;
+ int r;
- kvm_x86_ops->skip_emulated_instruction(vcpu);
+ r = kvm_x86_ops->skip_emulated_instruction(vcpu);
+ if (r != EMULATE_DONE)
+ return 0;
/*
* rflags is the old, "raw" value of the flags. The new value has
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/7] x86: KVM: clear interrupt shadow on EMULTYPE_SKIP
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 1/7] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 2/7] x86: kvm: svm: propagate errors from skip_emulated_instruction() Vitaly Kuznetsov
@ 2019-08-08 17:30 ` Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 4/7] x86: KVM: add xsetbv to the emulator Vitaly Kuznetsov
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
When doing x86_emulate_instruction(EMULTYPE_SKIP) interrupt shadow has to
be cleared if and only if the skipping is successful.
There are two immediate issues:
- In SVM skip_emulated_instruction() we are not zapping interrupt shadow
in case kvm_emulate_instruction(EMULTYPE_SKIP) is used to advance RIP
(!nrpip_save).
- In VMX handle_ept_misconfig() when running as a nested hypervisor we
(static_cpu_has(X86_FEATURE_HYPERVISOR) case) forget to clear interrupt
shadow.
Note that we intentionally don't handle the case when the skipped
instruction is supposed to prolong the interrupt shadow ("MOV/POP SS") as
skip-emulation of those instructions should not happen under normal
circumstances.
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/x86.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a97818b1111d..50e0b25092c7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6539,6 +6539,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
kvm_rip_write(vcpu, ctxt->_eip);
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;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 4/7] x86: KVM: add xsetbv to the emulator
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
` (2 preceding siblings ...)
2019-08-08 17:30 ` [PATCH v3 3/7] x86: KVM: clear interrupt shadow on EMULTYPE_SKIP Vitaly Kuznetsov
@ 2019-08-08 17:30 ` Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 5/7] x86: KVM: svm: remove hardcoded instruction length from intercepts Vitaly Kuznetsov
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
To avoid hardcoding xsetbv length to '3' we need to support decoding it in
the emulator.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/include/asm/kvm_emulate.h | 3 ++-
arch/x86/kvm/emulate.c | 23 ++++++++++++++++++++++-
arch/x86/kvm/svm.c | 1 +
arch/x86/kvm/x86.c | 6 ++++++
4 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index feab24cac610..77cf6c11f66b 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -229,7 +229,7 @@ struct x86_emulate_ops {
int (*pre_leave_smm)(struct x86_emulate_ctxt *ctxt,
const char *smstate);
void (*post_leave_smm)(struct x86_emulate_ctxt *ctxt);
-
+ int (*set_xcr)(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr);
};
typedef u32 __attribute__((vector_size(16))) sse128_t;
@@ -429,6 +429,7 @@ enum x86_intercept {
x86_intercept_ins,
x86_intercept_out,
x86_intercept_outs,
+ x86_intercept_xsetbv,
nr_x86_intercepts
};
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 718f7d9afedc..f9e843dd992a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4156,6 +4156,20 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
return rc;
}
+static int em_xsetbv(struct x86_emulate_ctxt *ctxt)
+{
+ u32 eax, ecx, edx;
+
+ eax = reg_read(ctxt, VCPU_REGS_RAX);
+ edx = reg_read(ctxt, VCPU_REGS_RDX);
+ ecx = reg_read(ctxt, VCPU_REGS_RCX);
+
+ if (ctxt->ops->set_xcr(ctxt, ecx, ((u64)edx << 32) | eax))
+ return emulate_gp(ctxt, 0);
+
+ return X86EMUL_CONTINUE;
+}
+
static bool valid_cr(int nr)
{
switch (nr) {
@@ -4409,6 +4423,12 @@ static const struct opcode group7_rm1[] = {
N, N, N, N, N, N,
};
+static const struct opcode group7_rm2[] = {
+ N,
+ II(ImplicitOps | Priv, em_xsetbv, xsetbv),
+ N, N, N, N, N, N,
+};
+
static const struct opcode group7_rm3[] = {
DIP(SrcNone | Prot | Priv, vmrun, check_svme_pa),
II(SrcNone | Prot | EmulateOnUD, em_hypercall, vmmcall),
@@ -4498,7 +4518,8 @@ static const struct group_dual group7 = { {
}, {
EXT(0, group7_rm0),
EXT(0, group7_rm1),
- N, EXT(0, group7_rm3),
+ EXT(0, group7_rm2),
+ EXT(0, group7_rm3),
II(SrcNone | DstMem | Mov, em_smsw, smsw), N,
II(SrcMem16 | Mov | Priv, em_lmsw, lmsw),
EXT(0, group7_rm7),
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8299b0de06e2..858feeac01a4 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -6067,6 +6067,7 @@ static const struct __x86_intercept {
[x86_intercept_ins] = POST_EX(SVM_EXIT_IOIO),
[x86_intercept_out] = POST_EX(SVM_EXIT_IOIO),
[x86_intercept_outs] = POST_EX(SVM_EXIT_IOIO),
+ [x86_intercept_xsetbv] = PRE_EX(SVM_EXIT_XSETBV),
};
#undef PRE_EX
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 50e0b25092c7..2f4a4d0967f3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6068,6 +6068,11 @@ static void emulator_post_leave_smm(struct x86_emulate_ctxt *ctxt)
kvm_smm_changed(emul_to_vcpu(ctxt));
}
+static int emulator_set_xcr(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr)
+{
+ return __kvm_set_xcr(emul_to_vcpu(ctxt), index, xcr);
+}
+
static const struct x86_emulate_ops emulate_ops = {
.read_gpr = emulator_read_gpr,
.write_gpr = emulator_write_gpr,
@@ -6109,6 +6114,7 @@ static const struct x86_emulate_ops emulate_ops = {
.set_hflags = emulator_set_hflags,
.pre_leave_smm = emulator_pre_leave_smm,
.post_leave_smm = emulator_post_leave_smm,
+ .set_xcr = emulator_set_xcr,
};
static void toggle_interruptibility(struct kvm_vcpu *vcpu, u32 mask)
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 5/7] x86: KVM: svm: remove hardcoded instruction length from intercepts
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
` (3 preceding siblings ...)
2019-08-08 17:30 ` [PATCH v3 4/7] x86: KVM: add xsetbv to the emulator Vitaly Kuznetsov
@ 2019-08-08 17:30 ` Vitaly Kuznetsov
2019-08-09 18:37 ` Sean Christopherson
2019-08-08 17:30 ` [PATCH v3 6/7] x86: KVM: svm: eliminate weird goto from vmrun_interception() Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 7/7] x86: KVM: svm: eliminate hardcoded RIP advancement " Vitaly Kuznetsov
6 siblings, 1 reply; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
Various intercepts hard-code the respective instruction lengths to optimize
skip_emulated_instruction(): when next_rip is pre-set we skip
kvm_emulate_instruction(vcpu, EMULTYPE_SKIP). The optimization is, however,
incorrect: different (redundant) prefixes could be used to enlarge the
instruction. We can't really avoid decoding.
svm->next_rip is not used when CPU supports 'nrips' (X86_FEATURE_NRIPS)
feature: next RIP is provided in VMCB. The feature is not really new
(Opteron G3s had it already) and the change should have zero affect.
Remove manual svm->next_rip setting with hard-coded instruction lengths.
The only case where we now use svm->next_rip is EXIT_IOIO: the instruction
length is provided to us by hardware.
Hardcoded RIP advancement remains in vmrun_interception(), this is going to
be taken care of separately.
Reported-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/svm.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 858feeac01a4..6d16d1898810 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2903,13 +2903,11 @@ static int nop_on_interception(struct vcpu_svm *svm)
static int halt_interception(struct vcpu_svm *svm)
{
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 1;
return kvm_emulate_halt(&svm->vcpu);
}
static int vmmcall_interception(struct vcpu_svm *svm)
{
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
return kvm_emulate_hypercall(&svm->vcpu);
}
@@ -3697,7 +3695,6 @@ static int vmload_interception(struct vcpu_svm *svm)
nested_vmcb = map.hva;
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
ret = kvm_skip_emulated_instruction(&svm->vcpu);
nested_svm_vmloadsave(nested_vmcb, svm->vmcb);
@@ -3724,7 +3721,6 @@ static int vmsave_interception(struct vcpu_svm *svm)
nested_vmcb = map.hva;
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
ret = kvm_skip_emulated_instruction(&svm->vcpu);
nested_svm_vmloadsave(svm->vmcb, nested_vmcb);
@@ -3775,7 +3771,6 @@ static int stgi_interception(struct vcpu_svm *svm)
if (vgif_enabled(svm))
clr_intercept(svm, INTERCEPT_STGI);
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
ret = kvm_skip_emulated_instruction(&svm->vcpu);
kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
@@ -3791,7 +3786,6 @@ static int clgi_interception(struct vcpu_svm *svm)
if (nested_svm_check_permissions(svm))
return 1;
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
ret = kvm_skip_emulated_instruction(&svm->vcpu);
disable_gif(svm);
@@ -3816,7 +3810,6 @@ static int invlpga_interception(struct vcpu_svm *svm)
/* Let's treat INVLPGA the same as INVLPG (can be optimized!) */
kvm_mmu_invlpg(vcpu, kvm_rax_read(&svm->vcpu));
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
return kvm_skip_emulated_instruction(&svm->vcpu);
}
@@ -3839,7 +3832,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
u32 index = kvm_rcx_read(&svm->vcpu);
if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
return kvm_skip_emulated_instruction(&svm->vcpu);
}
@@ -3921,7 +3913,6 @@ static int task_switch_interception(struct vcpu_svm *svm)
static int cpuid_interception(struct vcpu_svm *svm)
{
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
return kvm_emulate_cpuid(&svm->vcpu);
}
@@ -4251,7 +4242,6 @@ static int rdmsr_interception(struct vcpu_svm *svm)
kvm_rax_write(&svm->vcpu, msr_info.data & 0xffffffff);
kvm_rdx_write(&svm->vcpu, msr_info.data >> 32);
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
return kvm_skip_emulated_instruction(&svm->vcpu);
}
}
@@ -4457,7 +4447,6 @@ static int wrmsr_interception(struct vcpu_svm *svm)
return 1;
} else {
trace_kvm_msr_write(ecx, data);
- svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
return kvm_skip_emulated_instruction(&svm->vcpu);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 6/7] x86: KVM: svm: eliminate weird goto from vmrun_interception()
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
` (4 preceding siblings ...)
2019-08-08 17:30 ` [PATCH v3 5/7] x86: KVM: svm: remove hardcoded instruction length from intercepts Vitaly Kuznetsov
@ 2019-08-08 17:30 ` Vitaly Kuznetsov
2019-08-09 18:46 ` Sean Christopherson
2019-08-08 17:30 ` [PATCH v3 7/7] x86: KVM: svm: eliminate hardcoded RIP advancement " Vitaly Kuznetsov
6 siblings, 1 reply; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
Regardless of whether or not nested_svm_vmrun_msrpm() fails, we return 1
from vmrun_interception() so there's no point in doing goto. Also,
nested_svm_vmrun_msrpm() call can be made from nested_svm_vmrun() where
other nested launch issues are handled.
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/svm.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 6d16d1898810..43bc4a5e4948 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3658,6 +3658,15 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb, &map);
+ if (!nested_svm_vmrun_msrpm(svm)) {
+ svm->vmcb->control.exit_code = SVM_EXIT_ERR;
+ svm->vmcb->control.exit_code_hi = 0;
+ svm->vmcb->control.exit_info_1 = 0;
+ svm->vmcb->control.exit_info_2 = 0;
+
+ nested_svm_vmexit(svm);
+ }
+
return true;
}
@@ -3740,20 +3749,6 @@ static int vmrun_interception(struct vcpu_svm *svm)
if (!nested_svm_vmrun(svm))
return 1;
- if (!nested_svm_vmrun_msrpm(svm))
- goto failed;
-
- return 1;
-
-failed:
-
- svm->vmcb->control.exit_code = SVM_EXIT_ERR;
- svm->vmcb->control.exit_code_hi = 0;
- svm->vmcb->control.exit_info_1 = 0;
- svm->vmcb->control.exit_info_2 = 0;
-
- nested_svm_vmexit(svm);
-
return 1;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 7/7] x86: KVM: svm: eliminate hardcoded RIP advancement from vmrun_interception()
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
` (5 preceding siblings ...)
2019-08-08 17:30 ` [PATCH v3 6/7] x86: KVM: svm: eliminate weird goto from vmrun_interception() Vitaly Kuznetsov
@ 2019-08-08 17:30 ` Vitaly Kuznetsov
2019-08-09 18:55 ` Sean Christopherson
6 siblings, 1 reply; 12+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-08 17:30 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson, Sean Christopherson
Just like we do with other intercepts, in vmrun_interception() we should be
doing kvm_skip_emulated_instruction() and not just RIP += 3. Also, it is
wrong to increment RIP before nested_svm_vmrun() as it can result in
kvm_inject_gp().
We can't call kvm_skip_emulated_instruction() after nested_svm_vmrun() so
move it inside. To preserve the return value from it nested_svm_vmrun()
needs to start returning an int.
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/svm.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 43bc4a5e4948..6c4046eb26b3 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3586,9 +3586,9 @@ static void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
mark_all_dirty(svm->vmcb);
}
-static bool nested_svm_vmrun(struct vcpu_svm *svm)
+static int nested_svm_vmrun(struct vcpu_svm *svm)
{
- int rc;
+ int rc, ret;
struct vmcb *nested_vmcb;
struct vmcb *hsave = svm->nested.hsave;
struct vmcb *vmcb = svm->vmcb;
@@ -3598,12 +3598,15 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
vmcb_gpa = svm->vmcb->save.rax;
rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map);
- if (rc) {
- if (rc == -EINVAL)
- kvm_inject_gp(&svm->vcpu, 0);
- return false;
+ if (rc == -EINVAL) {
+ kvm_inject_gp(&svm->vcpu, 0);
+ return 1;
}
+ ret = kvm_skip_emulated_instruction(&svm->vcpu);
+ if (rc)
+ return ret;
+
nested_vmcb = map.hva;
if (!nested_vmcb_checks(nested_vmcb)) {
@@ -3614,7 +3617,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
kvm_vcpu_unmap(&svm->vcpu, &map, true);
- return false;
+ return ret;
}
trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
@@ -3667,7 +3670,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
nested_svm_vmexit(svm);
}
- return true;
+ return ret;
}
static void nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb)
@@ -3743,13 +3746,7 @@ static int vmrun_interception(struct vcpu_svm *svm)
if (nested_svm_check_permissions(svm))
return 1;
- /* Save rip after vmrun instruction */
- kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
-
- if (!nested_svm_vmrun(svm))
- return 1;
-
- return 1;
+ return nested_svm_vmrun(svm);
}
static int stgi_interception(struct vcpu_svm *svm)
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/7] x86: kvm: svm: propagate errors from skip_emulated_instruction()
2019-08-08 17:30 ` [PATCH v3 2/7] x86: kvm: svm: propagate errors from skip_emulated_instruction() Vitaly Kuznetsov
@ 2019-08-09 18:31 ` Sean Christopherson
0 siblings, 0 replies; 12+ messages in thread
From: Sean Christopherson @ 2019-08-09 18:31 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson
On Thu, Aug 08, 2019 at 07:30:46PM +0200, Vitaly Kuznetsov wrote:
> On AMD, kvm_x86_ops->skip_emulated_instruction(vcpu) can, in theory,
> fail: in !nrips case we call kvm_emulate_instruction(EMULTYPE_SKIP).
> Currently, we only do printk(KERN_DEBUG) when this happens and this
> is not ideal. Propagate the error up the stack.
>
> On VMX, skip_emulated_instruction() doesn't fail, we have two call
> sites calling it explicitly: handle_exception_nmi() and
> handle_task_switch(), we can just ignore the result.
>
> On SVM, we also have two explicit call sites:
> svm_queue_exception() and it seems we don't need to do anything there as
> we check if RIP was advanced or not. In task_switch_interception(),
> however, we are better off not proceeding to kvm_task_switch() in case
> skip_emulated_instruction() failed.
>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
...
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 074385c86c09..2579e7a6d59d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1473,7 +1473,7 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data)
> }
>
>
> -static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
> +static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
> {
> unsigned long rip;
>
> @@ -1483,6 +1483,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>
> /* skipping an emulated instruction also counts */
> vmx_set_interrupt_shadow(vcpu, 0);
> +
> + return EMULATE_DONE;
> }
>
> static void vmx_clear_hlt(struct kvm_vcpu *vcpu)
> @@ -4547,7 +4549,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> vcpu->arch.dr6 &= ~DR_TRAP_BITS;
> vcpu->arch.dr6 |= dr6 | DR6_RTM;
> if (is_icebp(intr_info))
> - skip_emulated_instruction(vcpu);
> + (void)skip_emulated_instruction(vcpu);
>
> kvm_queue_exception(vcpu, DB_VECTOR);
> return 1;
> @@ -5057,7 +5059,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
> if (!idt_v || (type != INTR_TYPE_HARD_EXCEPTION &&
> type != INTR_TYPE_EXT_INTR &&
> type != INTR_TYPE_NMI_INTR))
> - skip_emulated_instruction(vcpu);
> + (void)skip_emulated_instruction(vcpu);
Maybe a silly idea, but what if we squash the return value in a dedicated
helper, with a big "DO NOT USE" comment above the int-returning function, e.g.:
static int __skip_emulated_instruction(struct kvm_vcpu *vcpu)
{
unsigned long rip;
rip = kvm_rip_read(vcpu);
rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
kvm_rip_write(vcpu, rip);
/* skipping an emulated instruction also counts */
vmx_set_interrupt_shadow(vcpu, 0);
return EMULATE_DONE;
}
static inline void skip_emulated_instruction(struct kvm_vcpu *vcpu)
{
(void)__skip_emulated_instruction(vcpu);
}
Alternatively, the inner function could be void, but on my system that
adds an extra call in the wrapper, i.e. in the kvm_skip_emulated...()
path. The above approach generates the same code as your patch, e.g.
allows the compiler to decide whether or not to inline the meat of the
code.
> if (kvm_task_switch(vcpu, tss_selector,
> type == INTR_TYPE_SOFT_INTR ? idt_index : -1, reason,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c6d951cbd76c..a97818b1111d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6383,9 +6383,11 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
> int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
> {
> unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> - int r = EMULATE_DONE;
> + int r;
>
> - kvm_x86_ops->skip_emulated_instruction(vcpu);
> + r = kvm_x86_ops->skip_emulated_instruction(vcpu);
> + if (r != EMULATE_DONE)
This should probably be wrapped with unlikely.
> + return 0;
>
> /*
> * rflags is the old, "raw" value of the flags. The new value has
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 5/7] x86: KVM: svm: remove hardcoded instruction length from intercepts
2019-08-08 17:30 ` [PATCH v3 5/7] x86: KVM: svm: remove hardcoded instruction length from intercepts Vitaly Kuznetsov
@ 2019-08-09 18:37 ` Sean Christopherson
0 siblings, 0 replies; 12+ messages in thread
From: Sean Christopherson @ 2019-08-09 18:37 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson
On Thu, Aug 08, 2019 at 07:30:49PM +0200, Vitaly Kuznetsov wrote:
> Various intercepts hard-code the respective instruction lengths to optimize
> skip_emulated_instruction(): when next_rip is pre-set we skip
> kvm_emulate_instruction(vcpu, EMULTYPE_SKIP). The optimization is, however,
> incorrect: different (redundant) prefixes could be used to enlarge the
> instruction. We can't really avoid decoding.
>
> svm->next_rip is not used when CPU supports 'nrips' (X86_FEATURE_NRIPS)
> feature: next RIP is provided in VMCB. The feature is not really new
> (Opteron G3s had it already) and the change should have zero affect.
>
> Remove manual svm->next_rip setting with hard-coded instruction lengths.
> The only case where we now use svm->next_rip is EXIT_IOIO: the instruction
> length is provided to us by hardware.
>
> Hardcoded RIP advancement remains in vmrun_interception(), this is going to
> be taken care of separately.
>
> Reported-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 6/7] x86: KVM: svm: eliminate weird goto from vmrun_interception()
2019-08-08 17:30 ` [PATCH v3 6/7] x86: KVM: svm: eliminate weird goto from vmrun_interception() Vitaly Kuznetsov
@ 2019-08-09 18:46 ` Sean Christopherson
0 siblings, 0 replies; 12+ messages in thread
From: Sean Christopherson @ 2019-08-09 18:46 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson
On Thu, Aug 08, 2019 at 07:30:50PM +0200, Vitaly Kuznetsov wrote:
> Regardless of whether or not nested_svm_vmrun_msrpm() fails, we return 1
> from vmrun_interception() so there's no point in doing goto. Also,
> nested_svm_vmrun_msrpm() call can be made from nested_svm_vmrun() where
> other nested launch issues are handled.
>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> arch/x86/kvm/svm.c | 23 +++++++++--------------
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 6d16d1898810..43bc4a5e4948 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3658,6 +3658,15 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
>
> enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb, &map);
>
> + if (!nested_svm_vmrun_msrpm(svm)) {
> + svm->vmcb->control.exit_code = SVM_EXIT_ERR;
> + svm->vmcb->control.exit_code_hi = 0;
> + svm->vmcb->control.exit_info_1 = 0;
> + svm->vmcb->control.exit_info_2 = 0;
> +
> + nested_svm_vmexit(svm);
> + }
> +
> return true;
nested_svm_vmrun() no longer needs a return value, it just needs to return
early. But making it 'void' just to change it to 'int' in the next patch
is a bit gratuitous, so what about changing it to return an 'int' in this
patch?
That'd also eliminate the funky
if (!nested_svm_vmrun(svm))
return 1;
return 1;
chunk in vmrun_interception() that temporarily exists until patch 7/7.
> }
>
> @@ -3740,20 +3749,6 @@ static int vmrun_interception(struct vcpu_svm *svm)
> if (!nested_svm_vmrun(svm))
> return 1;
>
> - if (!nested_svm_vmrun_msrpm(svm))
> - goto failed;
> -
> - return 1;
> -
> -failed:
> -
> - svm->vmcb->control.exit_code = SVM_EXIT_ERR;
> - svm->vmcb->control.exit_code_hi = 0;
> - svm->vmcb->control.exit_info_1 = 0;
> - svm->vmcb->control.exit_info_2 = 0;
> -
> - nested_svm_vmexit(svm);
> -
> return 1;
> }
>
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 7/7] x86: KVM: svm: eliminate hardcoded RIP advancement from vmrun_interception()
2019-08-08 17:30 ` [PATCH v3 7/7] x86: KVM: svm: eliminate hardcoded RIP advancement " Vitaly Kuznetsov
@ 2019-08-09 18:55 ` Sean Christopherson
0 siblings, 0 replies; 12+ messages in thread
From: Sean Christopherson @ 2019-08-09 18:55 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, linux-kernel, Paolo Bonzini, Radim Krčmář,
Joerg Roedel, Jim Mattson
On Thu, Aug 08, 2019 at 07:30:51PM +0200, Vitaly Kuznetsov wrote:
> Just like we do with other intercepts, in vmrun_interception() we should be
> doing kvm_skip_emulated_instruction() and not just RIP += 3. Also, it is
> wrong to increment RIP before nested_svm_vmrun() as it can result in
> kvm_inject_gp().
>
> We can't call kvm_skip_emulated_instruction() after nested_svm_vmrun() so
> move it inside. To preserve the return value from it nested_svm_vmrun()
> needs to start returning an int.
>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> arch/x86/kvm/svm.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 43bc4a5e4948..6c4046eb26b3 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3586,9 +3586,9 @@ static void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
> mark_all_dirty(svm->vmcb);
> }
>
> -static bool nested_svm_vmrun(struct vcpu_svm *svm)
> +static int nested_svm_vmrun(struct vcpu_svm *svm)
> {
> - int rc;
> + int rc, ret;
> struct vmcb *nested_vmcb;
> struct vmcb *hsave = svm->nested.hsave;
> struct vmcb *vmcb = svm->vmcb;
> @@ -3598,12 +3598,15 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> vmcb_gpa = svm->vmcb->save.rax;
>
> rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map);
> - if (rc) {
> - if (rc == -EINVAL)
> - kvm_inject_gp(&svm->vcpu, 0);
> - return false;
> + if (rc == -EINVAL) {
> + kvm_inject_gp(&svm->vcpu, 0);
> + return 1;
> }
>
> + ret = kvm_skip_emulated_instruction(&svm->vcpu);
> + if (rc)
> + return ret;
This should probably have a comment, the 'if (rc)' looks so wrong at first
glance. Maybe not the best suggestion on my part...
Alternatively, this sequence is more obvious and at worst adds a few bytes
to the code footprint.
if (ret == EINVAL) {
kvm_inject_gp(&svm->vcpu, 0);
return 1;
} else if (ret) {
return kvm_skip_emulated_instruction(&svm->vcpu);
}
ret = kvm_skip_emulated_instruction(&svm->vcpu);
> +
> nested_vmcb = map.hva;
>
> if (!nested_vmcb_checks(nested_vmcb)) {
> @@ -3614,7 +3617,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
>
> kvm_vcpu_unmap(&svm->vcpu, &map, true);
>
> - return false;
> + return ret;
> }
>
> trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
> @@ -3667,7 +3670,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> nested_svm_vmexit(svm);
> }
>
> - return true;
> + return ret;
> }
>
> static void nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb)
> @@ -3743,13 +3746,7 @@ static int vmrun_interception(struct vcpu_svm *svm)
> if (nested_svm_check_permissions(svm))
> return 1;
>
> - /* Save rip after vmrun instruction */
> - kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
> -
> - if (!nested_svm_vmrun(svm))
> - return 1;
> -
> - return 1;
> + return nested_svm_vmrun(svm);
> }
>
> static int stgi_interception(struct vcpu_svm *svm)
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-08-09 18:55 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-08 17:30 [PATCH v3 0/7] x86: KVM: svm: get rid of hardcoded instructions lengths Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 1/7] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 2/7] x86: kvm: svm: propagate errors from skip_emulated_instruction() Vitaly Kuznetsov
2019-08-09 18:31 ` Sean Christopherson
2019-08-08 17:30 ` [PATCH v3 3/7] x86: KVM: clear interrupt shadow on EMULTYPE_SKIP Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 4/7] x86: KVM: add xsetbv to the emulator Vitaly Kuznetsov
2019-08-08 17:30 ` [PATCH v3 5/7] x86: KVM: svm: remove hardcoded instruction length from intercepts Vitaly Kuznetsov
2019-08-09 18:37 ` Sean Christopherson
2019-08-08 17:30 ` [PATCH v3 6/7] x86: KVM: svm: eliminate weird goto from vmrun_interception() Vitaly Kuznetsov
2019-08-09 18:46 ` Sean Christopherson
2019-08-08 17:30 ` [PATCH v3 7/7] x86: KVM: svm: eliminate hardcoded RIP advancement " Vitaly Kuznetsov
2019-08-09 18:55 ` Sean Christopherson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).