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

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

[Vitaly Kuznetsov]

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.

Early RFC. Unfortunately, I got distracted by some other problems so
sending it out half-baked.

TODO:
- Get rid of hardcoded '+ 3' in vmrun_interception().
- Test.

P.S. If you'd like to test the series you'll have to have a CPU without
NRIP_SAVE feature or forcefully disable it, something like:

index 8d4e50428b68..93c7eaad7915 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -922,6 +922,9 @@ static void init_amd(struct cpuinfo_x86 *c)
        /* AMD CPUs don't reset SS attributes on SYSRET, Xen does. */
        if (!cpu_has(c, X86_FEATURE_XENPV))
                set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
+
+       /* No nrips */
+       clear_cpu_cap(c, X86_FEATURE_NRIPS);
 }
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5beca1030c9a..5b2ea34bc9f2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -773,11 +773,11 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
        struct vcpu_svm *svm = to_svm(vcpu);
 
-       if (svm->vmcb->control.next_rip != 0) {
+/*     if (svm->vmcb->control.next_rip != 0) {
                WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
                svm->next_rip = svm->vmcb->control.next_rip;
        }
-
+*/
        if (!svm->next_rip) {
                if (kvm_emulate_instruction(vcpu, EMULTYPE_SKIP) !=
                                EMULATE_DONE)

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: svm: clear interrupt shadow on all paths in
    skip_emulated_instruction()
  x86: KVM: add xsetbv to the emulator
  x86: KVM: svm: remove hardcoded instruction length from intercepts

 arch/x86/include/asm/kvm_emulate.h |  1 +
 arch/x86/kvm/emulate.c             |  9 ++++++++-
 arch/x86/kvm/svm.c                 | 19 ++++++-------------
 3 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH RFC 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.

By the way, rdmsr_interception() has it right already.

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 5b2ea34bc9f2..982c6b9bfc90 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4430,13 +4430,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

[PATCH RFC 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>
---
 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 982c6b9bfc90..68f1f0218c95 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -781,7 +781,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

[PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Vitaly Kuznetsov]

Regardless of the way how we skip instruction, interrupt shadow needs to be
cleared.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 68f1f0218c95..f980fc43372d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -783,13 +783,15 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 				EMULATE_DONE)
 			pr_err_once("KVM: %s: unable to skip instruction\n",
 				    __func__);
-		return;
+		goto clear_int_shadow;
 	}
 	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);
+
+clear_int_shadow:
 	svm_set_interrupt_shadow(vcpu, 0);
 }
 
-- 
2.20.1

[PATCH RFC 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 | 1 +
 arch/x86/kvm/emulate.c             | 9 ++++++++-
 arch/x86/kvm/svm.c                 | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index feab24cac610..478f76b0122d 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -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 d0d5dd44b4f4..ff25d94df684 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4393,6 +4393,12 @@ static const struct opcode group7_rm1[] = {
 	N, N, N, N, N, N,
 };
 
+static const struct opcode group7_rm2[] = {
+	N,
+	DI(SrcNone | Priv, 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),
@@ -4482,7 +4488,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 f980fc43372d..39e61029f401 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -6041,6 +6041,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
-- 
2.20.1

[PATCH RFC 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 | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 39e61029f401..4c29859fecde 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2890,13 +2890,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);
 }
 
@@ -3684,7 +3682,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);
@@ -3711,7 +3708,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);
@@ -3762,7 +3758,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);
 
@@ -3778,7 +3773,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);
@@ -3803,7 +3797,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);
 }
 
@@ -3826,7 +3819,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);
 	}
 
@@ -3903,7 +3895,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);
 }
 
@@ -4233,7 +4224,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);
 	}
 }
@@ -4439,7 +4429,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 RFC 0/5] x86/KVM/svm: get rid of hardcoded instructions lengths

[Paolo Bonzini]

On 20/06/19 13:02, Vitaly Kuznetsov wrote:
> 
> P.S. If you'd like to test the series you'll have to have a CPU without
> NRIP_SAVE feature or forcefully disable it, something like:
> 
> index 8d4e50428b68..93c7eaad7915 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -922,6 +922,9 @@ static void init_amd(struct cpuinfo_x86 *c)
>         /* AMD CPUs don't reset SS attributes on SYSRET, Xen does. */
>         if (!cpu_has(c, X86_FEATURE_XENPV))
>                 set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
> +
> +       /* No nrips */
> +       clear_cpu_cap(c, X86_FEATURE_NRIPS);
>  }
>  
>  #ifdef CONFIG_X86_32

Let's add a module parameter instead.  Patch sent (forgot to Cc you).

Paolo

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

[Paolo Bonzini]

On 20/06/19 13:02, Vitaly Kuznetsov wrote:
> To avoid hardcoding xsetbv length to '3' we need to support decoding it in
> the emulator.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Can you also emulate it properly?  The code from QEMU's
target/i386/fpu_helper.c can help. :)

Paolo

> ---
>  arch/x86/include/asm/kvm_emulate.h | 1 +
>  arch/x86/kvm/emulate.c             | 9 ++++++++-
>  arch/x86/kvm/svm.c                 | 1 +
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index feab24cac610..478f76b0122d 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -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 d0d5dd44b4f4..ff25d94df684 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4393,6 +4393,12 @@ static const struct opcode group7_rm1[] = {
>  	N, N, N, N, N, N,
>  };
>  
> +static const struct opcode group7_rm2[] = {
> +	N,
> +	DI(SrcNone | Priv, 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),
> @@ -4482,7 +4488,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 f980fc43372d..39e61029f401 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -6041,6 +6041,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
> 

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

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/06/19 13:02, Vitaly Kuznetsov wrote:
>> 
>> P.S. If you'd like to test the series you'll have to have a CPU without
>> NRIP_SAVE feature or forcefully disable it, something like:
>> 
>> index 8d4e50428b68..93c7eaad7915 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -922,6 +922,9 @@ static void init_amd(struct cpuinfo_x86 *c)
>>         /* AMD CPUs don't reset SS attributes on SYSRET, Xen does. */
>>         if (!cpu_has(c, X86_FEATURE_XENPV))
>>                 set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
>> +
>> +       /* No nrips */
>> +       clear_cpu_cap(c, X86_FEATURE_NRIPS);
>>  }
>>  
>>  #ifdef CONFIG_X86_32
>
> Let's add a module parameter instead.  Patch sent (forgot to Cc you).
>

Sure, I thought I'm the only interested person around but if there's
hope for more this definitely sounds like a good idea)

-- 
Vitaly

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

[Jim Mattson]

On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> 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>
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Jim Mattson]

On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Regardless of the way how we skip instruction, interrupt shadow needs to be
> cleared.

This change is definitely an improvement, but the existing code seems
to assume that we never call skip_emulated_instruction on a
POP-SS/MOV-to-SS/STI. Is that enforced anywhere?

Reviewed-by: Jim Mattson <jmattson@google.com>

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

[Jim Mattson]

On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> 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>

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

[Jim Mattson]

On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> 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.
>
> By the way, rdmsr_interception() has it right already.

I think I actually prefer the current placement, because this allows
the code that's common to both kvm-amd.ko and kvm-intel.ko to be
hoisted into the vendor-agnostic kvm module. Also, this hard-coded '2'
should be going away, right?

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

[Vitaly Kuznetsov]

Jim Mattson <jmattson@google.com> writes:

> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> 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.
>>
>> By the way, rdmsr_interception() has it right already.
>
> I think I actually prefer the current placement, because this allows
> the code that's common to both kvm-amd.ko and kvm-intel.ko to be
> hoisted into the vendor-agnostic kvm module. Also, this hard-coded '2'
> should be going away, right?

This whole change goes away in PATCH5 (with hardcoded '+2'), I added
this patch just to make it clear that RIP advancement we're doing here
is only being used by kvm_skip_emulated_instruction() and
kvm_inject_gp() branch is not affected.

We can throw this patch away from the series.

-- 
Vitaly

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Vitaly Kuznetsov]

Jim Mattson <jmattson@google.com> writes:

> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Regardless of the way how we skip instruction, interrupt shadow needs to be
>> cleared.
>
> This change is definitely an improvement, but the existing code seems
> to assume that we never call skip_emulated_instruction on a
> POP-SS/MOV-to-SS/STI. Is that enforced anywhere?

Hm, good question. I'll try to check before v1.

-- 
Vitaly

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

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/06/19 13:02, Vitaly Kuznetsov wrote:
>> To avoid hardcoding xsetbv length to '3' we need to support decoding it in
>> the emulator.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Can you also emulate it properly?  The code from QEMU's
> target/i386/fpu_helper.c can help. :)
>

(Had a chance to get back to this just now, sorry)

Assuming __kvm_set_xcr() is also a correct implementation, would the
code below do the job? (Just trying to figure out why you suggested
me to take a look at QEMU's variant):

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/x86.c b/arch/x86/kvm/x86.c
index c6d951cbd76c..9512cc38dfe9 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)

-- 
Vitaly

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

[Paolo Bonzini]

On 31/07/19 15:07, Vitaly Kuznetsov wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 20/06/19 13:02, Vitaly Kuznetsov wrote:
>>> To avoid hardcoding xsetbv length to '3' we need to support decoding it in
>>> the emulator.
>>>
>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>
>> Can you also emulate it properly?  The code from QEMU's
>> target/i386/fpu_helper.c can help. :)
>>
> 
> (Had a chance to get back to this just now, sorry)
> 
> Assuming __kvm_set_xcr() is also a correct implementation, would the
> code below do the job? (Just trying to figure out why you suggested
> me to take a look at QEMU's variant):
> 

Yes, I didn't remember __kvm_set_xcr.

Paolo

> 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/x86.c b/arch/x86/kvm/x86.c
> index c6d951cbd76c..9512cc38dfe9 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)
> 

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Vitaly Kuznetsov]

Jim Mattson <jmattson@google.com> writes:

> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Regardless of the way how we skip instruction, interrupt shadow needs to be
>> cleared.
>
> This change is definitely an improvement, but the existing code seems
> to assume that we never call skip_emulated_instruction on a
> POP-SS/MOV-to-SS/STI. Is that enforced anywhere?

(before I send v1 of the series) I looked at the current code and I
don't think it is enforced, however, VMX version does the same and
honestly I can't think of a situation when we would be doing 'skip' for
such an instruction.... and there's nothing we can easily enforce from
skip_emulated_instruction() as we have no idea what the instruction
is... 

I can of course be totally wrong and would appreciate if someone more
knowledgeable chimes in :-)

-- 
Vitaly

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Paolo Bonzini]

On 31/07/19 15:50, Vitaly Kuznetsov wrote:
> Jim Mattson <jmattson@google.com> writes:
> 
>> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>>
>>> Regardless of the way how we skip instruction, interrupt shadow needs to be
>>> cleared.
>>
>> This change is definitely an improvement, but the existing code seems
>> to assume that we never call skip_emulated_instruction on a
>> POP-SS/MOV-to-SS/STI. Is that enforced anywhere?
> 
> (before I send v1 of the series) I looked at the current code and I
> don't think it is enforced, however, VMX version does the same and
> honestly I can't think of a situation when we would be doing 'skip' for
> such an instruction.... and there's nothing we can easily enforce from
> skip_emulated_instruction() as we have no idea what the instruction
> is... 

I agree, I think a comment is worthwhile but we can live with the
limitation.

Paolo

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Jim Mattson]

On Wed, Jul 31, 2019 at 9:37 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 31/07/19 15:50, Vitaly Kuznetsov wrote:
> > Jim Mattson <jmattson@google.com> writes:
> >
> >> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >>>
> >>> Regardless of the way how we skip instruction, interrupt shadow needs to be
> >>> cleared.
> >>
> >> This change is definitely an improvement, but the existing code seems
> >> to assume that we never call skip_emulated_instruction on a
> >> POP-SS/MOV-to-SS/STI. Is that enforced anywhere?
> >
> > (before I send v1 of the series) I looked at the current code and I
> > don't think it is enforced, however, VMX version does the same and
> > honestly I can't think of a situation when we would be doing 'skip' for
> > such an instruction.... and there's nothing we can easily enforce from
> > skip_emulated_instruction() as we have no idea what the instruction
> > is...

Can't we still coerce kvm into emulating any instruction by leveraging
a stale ITLB entry? The 'emulator' kvm-unit-test did this before the
KVM forced emulation prefix was introduced, but I haven't checked to
see if the original (admittedly fragile) approach still works. Also,
for POP-SS, you could always force emulation by mapping the %rsp
address beyond guest physical memory. The hypervisor would then have
to emulate the instruction to provide bus-error semantics.

> I agree, I think a comment is worthwhile but we can live with the
> limitation.

I think we can live with the limitation, but I'd really prefer to see
a KVM exit with KVM_INTERNAL_ERROR_EMULATION for an instruction that
kvm doesn't emulate properly. That seems better than just a comment
that the virtual CPU doesn't behave as architected. (I realize that I
am probably in the minority here.)

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Sean Christopherson]

On Wed, Jul 31, 2019 at 01:27:53PM -0700, Jim Mattson wrote:
> On Wed, Jul 31, 2019 at 9:37 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 31/07/19 15:50, Vitaly Kuznetsov wrote:
> > > Jim Mattson <jmattson@google.com> writes:
> > >
> > >> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> > >>>
> > >>> Regardless of the way how we skip instruction, interrupt shadow needs to be
> > >>> cleared.
> > >>
> > >> This change is definitely an improvement, but the existing code seems
> > >> to assume that we never call skip_emulated_instruction on a
> > >> POP-SS/MOV-to-SS/STI. Is that enforced anywhere?
> > >
> > > (before I send v1 of the series) I looked at the current code and I
> > > don't think it is enforced, however, VMX version does the same and
> > > honestly I can't think of a situation when we would be doing 'skip' for
> > > such an instruction.... and there's nothing we can easily enforce from
> > > skip_emulated_instruction() as we have no idea what the instruction
> > > is...
> 
> Can't we still coerce kvm into emulating any instruction by leveraging
> a stale ITLB entry? The 'emulator' kvm-unit-test did this before the
> KVM forced emulation prefix was introduced, but I haven't checked to
> see if the original (admittedly fragile) approach still works. Also,
> for POP-SS, you could always force emulation by mapping the %rsp
> address beyond guest physical memory. The hypervisor would then have
> to emulate the instruction to provide bus-error semantics.
> 
> > I agree, I think a comment is worthwhile but we can live with the
> > limitation.
> 
> I think we can live with the limitation, but I'd really prefer to see
> a KVM exit with KVM_INTERNAL_ERROR_EMULATION for an instruction that
> kvm doesn't emulate properly. That seems better than just a comment
> that the virtual CPU doesn't behave as architected. (I realize that I
> am probably in the minority here.)

At a glance, the full emulator models behavior correctly, e.g. see
toggle_interruptibility() and setters of ctxt->interruptibility.

I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI
fast paths as the only (VMX) path that would incorrectly handle a
MOV/POP SS.  Reading the guest's instruction stream to detect MOV/POP SS
would defeat the whole "fast path" thing, not to mention both paths aren't
exactly architecturally compliant in the first place.

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Jim Mattson]

On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:

> At a glance, the full emulator models behavior correctly, e.g. see
> toggle_interruptibility() and setters of ctxt->interruptibility.
>
> I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI
> fast paths as the only (VMX) path that would incorrectly handle a
> MOV/POP SS.  Reading the guest's instruction stream to detect MOV/POP SS
> would defeat the whole "fast path" thing, not to mention both paths aren't
> exactly architecturally compliant in the first place.

The proposed patch clears the interrupt shadow in the VMCB on all
paths through svm's skip_emulated_instruction. If this happens at the
tail end of emulation, it doesn't matter if the full emulator does the
right thing.

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Sean Christopherson]

On Wed, Jul 31, 2019 at 04:45:21PM -0700, Jim Mattson wrote:
> On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> 
> > At a glance, the full emulator models behavior correctly, e.g. see
> > toggle_interruptibility() and setters of ctxt->interruptibility.
> >
> > I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI
> > fast paths as the only (VMX) path that would incorrectly handle a
> > MOV/POP SS.  Reading the guest's instruction stream to detect MOV/POP SS
> > would defeat the whole "fast path" thing, not to mention both paths aren't
> > exactly architecturally compliant in the first place.
> 
> The proposed patch clears the interrupt shadow in the VMCB on all
> paths through svm's skip_emulated_instruction. If this happens at the
> tail end of emulation, it doesn't matter if the full emulator does the
> right thing.

Unless I'm missing something, skip_emulated_instruction() isn't called in
the emulation case, x86_emulate_instruction() updates %rip directly, e.g.:

	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)
			kvm_vcpu_do_singlestep(vcpu, &r);
		if (!ctxt->have_exception ||
		    exception_type(ctxt->exception.vector) == EXCPT_TRAP)
			__kvm_set_rflags(vcpu, ctxt->eflags);

		/*
		 * For STI, interrupts are shadowed; so KVM_REQ_EVENT will
		 * do nothing, and it will be requested again as soon as
		 * the shadow expires.  But we still need to check here,
		 * because POPF has no interrupt shadow.
		 */
		if (unlikely((ctxt->eflags & ~rflags) & X86_EFLAGS_IF))
			kvm_make_request(KVM_REQ_EVENT, vcpu);
	}

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Paolo Bonzini]

On 01/08/19 01:56, Sean Christopherson wrote:
> On Wed, Jul 31, 2019 at 04:45:21PM -0700, Jim Mattson wrote:
>> On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson
>> <sean.j.christopherson@intel.com> wrote:
>>
>>> At a glance, the full emulator models behavior correctly, e.g. see
>>> toggle_interruptibility() and setters of ctxt->interruptibility.
>>>
>>> I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI
>>> fast paths as the only (VMX) path that would incorrectly handle a
>>> MOV/POP SS.  Reading the guest's instruction stream to detect MOV/POP SS
>>> would defeat the whole "fast path" thing, not to mention both paths aren't
>>> exactly architecturally compliant in the first place.
>>
>> The proposed patch clears the interrupt shadow in the VMCB on all
>> paths through svm's skip_emulated_instruction. If this happens at the
>> tail end of emulation, it doesn't matter if the full emulator does the
>> right thing.
> 
> Unless I'm missing something, skip_emulated_instruction() isn't called in
> the emulation case, x86_emulate_instruction() updates %rip directly, e.g.:

Indeed.  skip_emulated_instruction() is only used when the vmexit code
takes care of emulation directly.

Paolo

> 	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)
> 			kvm_vcpu_do_singlestep(vcpu, &r);
> 		if (!ctxt->have_exception ||
> 		    exception_type(ctxt->exception.vector) == EXCPT_TRAP)
> 			__kvm_set_rflags(vcpu, ctxt->eflags);
> 
> 		/*
> 		 * For STI, interrupts are shadowed; so KVM_REQ_EVENT will
> 		 * do nothing, and it will be requested again as soon as
> 		 * the shadow expires.  But we still need to check here,
> 		 * because POPF has no interrupt shadow.
> 		 */
> 		if (unlikely((ctxt->eflags & ~rflags) & X86_EFLAGS_IF))
> 			kvm_make_request(KVM_REQ_EVENT, vcpu);
> 	}
> 

Re: [PATCH RFC 3/5] x86: KVM: svm: clear interrupt shadow on all paths in skip_emulated_instruction()

[Jim Mattson]

On Wed, Jul 31, 2019 at 5:13 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 01/08/19 01:56, Sean Christopherson wrote:
> > On Wed, Jul 31, 2019 at 04:45:21PM -0700, Jim Mattson wrote:
> >> On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson
> >> <sean.j.christopherson@intel.com> wrote:
> >>
> >>> At a glance, the full emulator models behavior correctly, e.g. see
> >>> toggle_interruptibility() and setters of ctxt->interruptibility.
> >>>
> >>> I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI
> >>> fast paths as the only (VMX) path that would incorrectly handle a
> >>> MOV/POP SS.  Reading the guest's instruction stream to detect MOV/POP SS
> >>> would defeat the whole "fast path" thing, not to mention both paths aren't
> >>> exactly architecturally compliant in the first place.
> >>
> >> The proposed patch clears the interrupt shadow in the VMCB on all
> >> paths through svm's skip_emulated_instruction. If this happens at the
> >> tail end of emulation, it doesn't matter if the full emulator does the
> >> right thing.
> >
> > Unless I'm missing something, skip_emulated_instruction() isn't called in
> > the emulation case, x86_emulate_instruction() updates %rip directly, e.g.:
>
> Indeed.  skip_emulated_instruction() is only used when the vmexit code
> takes care of emulation directly.

Mea culpa. I had incorrectly assumed that "skip_emulated_instruction"
was used when an instruction was emulated. I retract my objection.
Having now been twice bitten by misleading function names, I'll be
more careful in the future.