[PATCH v2 0/5] x86: KVM: svm: get rid of hardcoded instructions lengths

[PATCH v2 0/5] x86: KVM: svm: get rid of hardcoded instructions lengths

[Vitaly Kuznetsov]

Changes since v1:
- Clear interrupt shadow in x86_emulate_instruction() instead of SVM's
  skip_emulated_instruction() to generalize the fix [Sean Christopherson]

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 (5):
  x86: KVM: svm: don't pretend to advance RIP in case
    wrmsr_interception() results in #GP
  x86: KVM: svm: avoid flooding logs when skip_emulated_instruction()
    fails
  x86: KVM: clear interrupt shadow on EMULTYPE_SKIP
  x86: KVM: add xsetbv to the emulator
  x86: KVM: svm: remove hardcoded instruction length from intercepts

 arch/x86/include/asm/kvm_emulate.h |  3 ++-
 arch/x86/kvm/emulate.c             | 23 ++++++++++++++++++++++-
 arch/x86/kvm/svm.c                 | 19 +++++--------------
 arch/x86/kvm/x86.c                 |  7 +++++++
 4 files changed, 36 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP

[Vitaly Kuznetsov]

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>
---
 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

Re: [PATCH v2 1/5] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP

[Sean Christopherson]

On Tue, Aug 06, 2019 at 08:01:46AM +0200, Vitaly Kuznetsov wrote:
> 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>

[PATCH v2 2/5] x86: KVM: svm: avoid flooding logs when skip_emulated_instruction() fails

[Vitaly Kuznetsov]

When we're unable to skip instruction with kvm_emulate_instruction() we
will not advance RIP and most likely the guest will get stuck as
consequitive attempts to execute the same instruction will likely result
in the same behavior.

As we're not supposed to see these messages under normal conditions, switch
to pr_err_once().

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 7e843b340490..80f576e05112 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -782,7 +782,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	if (!svm->next_rip) {
 		if (kvm_emulate_instruction(vcpu, EMULTYPE_SKIP) !=
 				EMULATE_DONE)
-			printk(KERN_DEBUG "%s: NOP\n", __func__);
+			pr_err_once("KVM: %s: unable to skip instruction\n",
+				    __func__);
 		return;
 	}
 	if (svm->next_rip - kvm_rip_read(vcpu) > MAX_INST_SIZE)
-- 
2.20.1

Re: [PATCH v2 2/5] x86: KVM: svm: avoid flooding logs when skip_emulated_instruction() fails

[Sean Christopherson]

On Tue, Aug 06, 2019 at 08:01:47AM +0200, Vitaly Kuznetsov wrote:
> When we're unable to skip instruction with kvm_emulate_instruction() we
> will not advance RIP and most likely the guest will get stuck as
> consequitive attempts to execute the same instruction will likely result
> in the same behavior.
> 
> As we're not supposed to see these messages under normal conditions, switch
> to pr_err_once().
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/svm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 7e843b340490..80f576e05112 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -782,7 +782,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	if (!svm->next_rip) {
>  		if (kvm_emulate_instruction(vcpu, EMULTYPE_SKIP) !=
>  				EMULATE_DONE)
> -			printk(KERN_DEBUG "%s: NOP\n", __func__);
> +			pr_err_once("KVM: %s: unable to skip instruction\n",
> +				    __func__);

IMO the proper fix would be to change skip_emulated_instruction() to
return an int so that emulation failure can be reported back up the stack.
It's a relatively minor change as there are a limited number of call sites
to skip_emulated_instruction() in SVM and VMX.

>  		return;
>  	}
>  	if (svm->next_rip - kvm_rip_read(vcpu) > MAX_INST_SIZE)
> -- 
> 2.20.1
> 

Re: [PATCH v2 2/5] x86: KVM: svm: avoid flooding logs when skip_emulated_instruction() fails

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> On Tue, Aug 06, 2019 at 08:01:47AM +0200, Vitaly Kuznetsov wrote:
>> When we're unable to skip instruction with kvm_emulate_instruction() we
>> will not advance RIP and most likely the guest will get stuck as
>> consequitive attempts to execute the same instruction will likely result
>> in the same behavior.
>> 
>> As we're not supposed to see these messages under normal conditions, switch
>> to pr_err_once().
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Reviewed-by: Jim Mattson <jmattson@google.com>
>> ---
>>  arch/x86/kvm/svm.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 7e843b340490..80f576e05112 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -782,7 +782,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>>  	if (!svm->next_rip) {
>>  		if (kvm_emulate_instruction(vcpu, EMULTYPE_SKIP) !=
>>  				EMULATE_DONE)
>> -			printk(KERN_DEBUG "%s: NOP\n", __func__);
>> +			pr_err_once("KVM: %s: unable to skip instruction\n",
>> +				    __func__);
>
> IMO the proper fix would be to change skip_emulated_instruction() to
> return an int so that emulation failure can be reported back up the stack.
> It's a relatively minor change as there are a limited number of call sites
> to skip_emulated_instruction() in SVM and VMX.
>

(I'm always wondering when is the right time to add "plus a bunch of
miscellaneous fixes all over" to the PATCH0's Subject line :-)

Will do in the next version, thanks!

>>  		return;
>>  	}
>>  	if (svm->next_rip - kvm_rip_read(vcpu) > MAX_INST_SIZE)
>> -- 
>> 2.20.1
>> 

-- 
Vitaly

[PATCH v2 3/5] x86: KVM: clear interrupt shadow on EMULTYPE_SKIP

[Vitaly Kuznetsov]

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) we forget to clear
  interrupt shadow.

Suggested-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 c6d951cbd76c..eac8253d84d2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6537,6 +6537,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

Re: [PATCH v2 3/5] x86: KVM: clear interrupt shadow on EMULTYPE_SKIP

[Sean Christopherson]

On Tue, Aug 06, 2019 at 08:01:48AM +0200, Vitaly Kuznetsov wrote:
> 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) we forget to clear

Redundant 'we'.  Might be worth adding a blurb in the changelog to note
that this intentionally doesn't handle "MOV/POP SS" as skip-emulation of
those instructions can only occur if the guest is doing something silly.

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

>   interrupt shadow.
> 
> Suggested-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 c6d951cbd76c..eac8253d84d2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6537,6 +6537,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
> 

[PATCH v2 4/5] x86: KVM: add xsetbv to the emulator

[Vitaly Kuznetsov]

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 80f576e05112..793a60461abe 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -6064,6 +6064,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 eac8253d84d2..b902b4735ad1 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

[PATCH v2 5/5] x86: KVM: svm: remove hardcoded instruction length from intercepts

[Vitaly Kuznetsov]

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.

Reported-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/svm.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 793a60461abe..dce215250d1f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2905,13 +2905,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);
 }
 
@@ -3699,7 +3697,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);
@@ -3726,7 +3723,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);
@@ -3740,8 +3736,8 @@ 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 (!kvm_skip_emulated_instruction(&svm->vcpu))
+		return 1;
 
 	if (!nested_svm_vmrun(svm))
 		return 1;
@@ -3777,7 +3773,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);
 
@@ -3793,7 +3788,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);
@@ -3818,7 +3812,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);
 }
 
@@ -3841,7 +3834,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);
 	}
 
@@ -3918,7 +3910,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);
 }
 
@@ -4248,7 +4239,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);
 	}
 }
@@ -4454,7 +4444,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

Re: [PATCH v2 5/5] x86: KVM: svm: remove hardcoded instruction length from intercepts

[Sean Christopherson]

On Tue, Aug 06, 2019 at 08:01:50AM +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.
> 
> Reported-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/svm.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 793a60461abe..dce215250d1f 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2905,13 +2905,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);
>  }
>  
> @@ -3699,7 +3697,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);
> @@ -3726,7 +3723,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);
> @@ -3740,8 +3736,8 @@ 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 (!kvm_skip_emulated_instruction(&svm->vcpu))
> +		return 1;

This is broken, e.g. KVM shouldn't resume the guest on single-step strap,
nor should it skip the meat of VMRUN emulation.  There are also several
pre-existing bugs, e.g. #GP can be injected due to invalid vmcb_gpa after
%rip is incremented and single-step #DB can be suppressed on failed
VMRUN (assuming that's not architectural behavior?).

Calling kvm_skip_emulated_instruction() after nested_svm_vmrun() looks like
it'd cause all kinds of problems, so I think the correct fix is to put the
call to kvm_skip_emulated_instruction() inside nested_svm_vmrun(), after
it maps the vmcb_gpa, e.g. something like this in the end (optionally
moving nested_svm_vmrun_msrpm() inside nested_svm_run() to eliminate the
weird goto handling).

static int nested_svm_vmrun(struct vcpu_svm *svm)
{
	int rc, ret;
	struct vmcb *nested_vmcb;
	struct vmcb *hsave = svm->nested.hsave;
	struct vmcb *vmcb = svm->vmcb;
	struct kvm_host_map map;
	u64 vmcb_gpa;

	vmcb_gpa = svm->vmcb->save.rax;

	rc = kvm_vcpu_map(&svm->vcpu, gfn_to_gpa(vmcb_gpa), &map);
	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)) {
		nested_vmcb->control.exit_code    = SVM_EXIT_ERR;
		nested_vmcb->control.exit_code_hi = 0;
		nested_vmcb->control.exit_info_1  = 0;
		nested_vmcb->control.exit_info_2  = 0;

		kvm_vcpu_unmap(&svm->vcpu, &map, true);

		return ret;
	}

	<  ... more existing code ... >

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

static int vmrun_interception(struct vcpu_svm *svm)
{
	if (nested_svm_check_permissions(svm))
		return 1;

	return nested_svm_vmrun(svm)
}

>  
>  	if (!nested_svm_vmrun(svm))
>  		return 1;
> @@ -3777,7 +3773,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);
>  
> @@ -3793,7 +3788,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);
> @@ -3818,7 +3812,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);
>  }
>  
> @@ -3841,7 +3834,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);
>  	}
>  
> @@ -3918,7 +3910,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);
>  }
>  
> @@ -4248,7 +4239,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);
>  	}
>  }
> @@ -4454,7 +4444,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
> 

Re: [PATCH v2 5/5] x86: KVM: svm: remove hardcoded instruction length from intercepts

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> On Tue, Aug 06, 2019 at 08:01:50AM +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.
>> 
>> Reported-by: Jim Mattson <jmattson@google.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/kvm/svm.c | 15 ++-------------
>>  1 file changed, 2 insertions(+), 13 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 793a60461abe..dce215250d1f 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2905,13 +2905,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);
>>  }
>>  
>> @@ -3699,7 +3697,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);
>> @@ -3726,7 +3723,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);
>> @@ -3740,8 +3736,8 @@ 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 (!kvm_skip_emulated_instruction(&svm->vcpu))
>> +		return 1;
>
> This is broken, e.g. KVM shouldn't resume the guest on single-step strap,
> nor should it skip the meat of VMRUN emulation.  There are also several
> pre-existing bugs, e.g. #GP can be injected due to invalid vmcb_gpa after
> %rip is incremented and single-step #DB can be suppressed on failed
> VMRUN (assuming that's not architectural behavior?).
>
> Calling kvm_skip_emulated_instruction() after nested_svm_vmrun() looks like
> it'd cause all kinds of problems, so I think the correct fix is to put the
> call to kvm_skip_emulated_instruction() inside nested_svm_vmrun(), after
> it maps the vmcb_gpa, e.g. something like this in the end (optionally
> moving nested_svm_vmrun_msrpm() inside nested_svm_run() to eliminate the
> weird goto handling).

I see, thank you for the suggestion, will do in the next version. I'll
split vmrun fix from removing hardcoded instruction lengths from other
intercepts.

>
> static int nested_svm_vmrun(struct vcpu_svm *svm)
> {
> 	int rc, ret;
> 	struct vmcb *nested_vmcb;
> 	struct vmcb *hsave = svm->nested.hsave;
> 	struct vmcb *vmcb = svm->vmcb;
> 	struct kvm_host_map map;
> 	u64 vmcb_gpa;
>
> 	vmcb_gpa = svm->vmcb->save.rax;
>
> 	rc = kvm_vcpu_map(&svm->vcpu, gfn_to_gpa(vmcb_gpa), &map);
> 	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)) {
> 		nested_vmcb->control.exit_code    = SVM_EXIT_ERR;
> 		nested_vmcb->control.exit_code_hi = 0;
> 		nested_vmcb->control.exit_info_1  = 0;
> 		nested_vmcb->control.exit_info_2  = 0;
>
> 		kvm_vcpu_unmap(&svm->vcpu, &map, true);
>
> 		return ret;
> 	}
>
> 	<  ... more existing code ... >
>
> 	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 ret;
> }
>
> static int vmrun_interception(struct vcpu_svm *svm)
> {
> 	if (nested_svm_check_permissions(svm))
> 		return 1;
>
> 	return nested_svm_vmrun(svm)
> }
>
>>  
>>  	if (!nested_svm_vmrun(svm))
>>  		return 1;
>> @@ -3777,7 +3773,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);
>>  
>> @@ -3793,7 +3788,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);
>> @@ -3818,7 +3812,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);
>>  }
>>  
>> @@ -3841,7 +3834,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);
>>  	}
>>  
>> @@ -3918,7 +3910,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);
>>  }
>>  
>> @@ -4248,7 +4239,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);
>>  	}
>>  }
>> @@ -4454,7 +4444,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
>> 

-- 
Vitaly