[PATCH v2 00/11] KVM: SVM: Fix soft int/ex re-injection

[PATCH v2 00/11] KVM: SVM: Fix soft int/ex re-injection

[Sean Christopherson]

Fix soft interrupt/exception reinjection on SVM.

The underlying issue is that SVM simply retries INT* instructions instead
of reinjecting the soft interupt/exception if an exception VM-Exit occurred
during vectoring.  Lack of reinjection breaks nested virtualization if
the injected event came from L1 and the VM-Exit is not forwarded to L1,
as there is no instruction to retry.  More fundamentally, retrying the
instruction is wrong as it can produce side effects that shouldn't occur,
e.g. code #DBs.

VMX has been fixed since commit 66fd3f7f901f ("KVM: Do not re-execute
INTn instruction."), but SVM was left behind.  Probably because fixing
SVM is a mess due to NRIPS not being supported on all architectures, and
due to it being poorly implemented (with respect to soft events) when it
is supported.

Opportunistically clean up related tracepoints to make debugging related
issues less painful in the future.

The last patch is not-signed-off-by as I think it needs broader review
and feedback before KVM drops support for CPUs that are old, but not
thaaaaat old.

The tracepoint output looks like:

    kvm_inj_exception: #GP (0x0) 
    kvm_inj_exception: #UD       
    kvm_inj_exception: #DE       
    kvm_inj_exception: #DE [reinjected]
    kvm_inj_exception: #BP [reinjected]
    kvm_inj_exception: #NP (0x18) [reinjected]

and for "irqs":

    kvm_inj_virq: Soft/INTn 0x20 [reinjected]
    kvm_inj_virq: Soft/INTn 0x19 [reinjected]
    kvm_inj_virq: IRQ 0x20
    kvm_inj_virq: IRQ 0xf1

v2:
  - Collect reviews. [Maxim]
  - Drop a stale comment midway through. [Paolo]
  - Correctly handle (at least as correctly as SVM allows) the scenario
    where an injected soft interrupt/exception has no backing insn. [Maxim]
  - Tag reinjected exceptions in the tracepoint. [Maxim]
  - Use the correct L2 RIP (hopefully) in svm_set_nested_state. [Maciej]
  - Fix a BUG that can be triggered by userspace.
  - Fix the error code FIXME in the exception tracepoint.
  - Differentiate soft vs. hard "IRQ" injection in tracepoint.
  - Assert that the first soft int is injected on the correct RIP in
    the selftest.

v1:
  https://lore.kernel.org/all/20220402010903.727604-1-seanjc@google.com

Maciej's original series:
  https://lore.kernel.org/all/cover.1646944472.git.maciej.szmigiero@oracle.com

Maciej S. Szmigiero (3):
  KVM: nSVM: Sync next_rip field from vmcb12 to vmcb02
  KVM: SVM: Don't BUG if userspace injects a soft interrupt with GIF=0
  KVM: selftests: nSVM: Add svm_nested_soft_inject_test

Sean Christopherson (8):
  KVM: SVM: Unwind "speculative" RIP advancement if INTn injection
    "fails"
  KVM: SVM: Stuff next_rip on emulated INT3 injection if NRIPS is
    supported
  KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction
  KVM: SVM: Re-inject INTn instead of retrying the insn on "failure"
  KVM: x86: Trace re-injected exceptions
  KVM: x86: Print error code in exception injection tracepoint iff valid
  KVM: x86: Differentiate Soft vs. Hard IRQs vs. reinjected in
    tracepoint
  KVM: SVM: Drop support for CPUs without NRIPS (NextRIP Save) support

 arch/x86/include/asm/kvm_host.h               |   2 +-
 arch/x86/kvm/svm/nested.c                     |  46 ++++-
 arch/x86/kvm/svm/svm.c                        | 169 ++++++++++++------
 arch/x86/kvm/svm/svm.h                        |   7 +-
 arch/x86/kvm/trace.h                          |  31 ++--
 arch/x86/kvm/vmx/vmx.c                        |   4 +-
 arch/x86/kvm/x86.c                            |  20 ++-
 tools/testing/selftests/kvm/.gitignore        |   3 +-
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/svm_util.h   |   2 +
 .../kvm/x86_64/svm_nested_soft_inject_test.c  | 149 +++++++++++++++
 11 files changed, 351 insertions(+), 83 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c


base-commit: 2a39d8b39bffdaf1a4223d0d22f07baee154c8f3
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 01/11] KVM: nSVM: Sync next_rip field from vmcb12 to vmcb02

[Sean Christopherson]

From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

The next_rip field of a VMCB is *not* an output-only field for a VMRUN.
This field value (instead of the saved guest RIP) in used by the CPU for
the return address pushed on stack when injecting a software interrupt or
INT3 or INTO exception.

Make sure this field gets synced from vmcb12 to vmcb02 when entering L2 or
loading a nested state and NRIPS is exposed to L1.  If NRIPS is supported
in hardware but not exposed to L1 (nrips=0 or hidden by userspace), stuff
vmcb02's next_rip from the new L2 RIP to emulate a !NRIPS CPU (which
saves RIP on the stack as-is).

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/nested.c | 22 +++++++++++++++++++---
 arch/x86/kvm/svm/svm.h    |  1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index bed5e1692cef..461c5f247801 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -371,6 +371,7 @@ void __nested_copy_vmcb_control_to_cache(struct kvm_vcpu *vcpu,
 	to->nested_ctl          = from->nested_ctl;
 	to->event_inj           = from->event_inj;
 	to->event_inj_err       = from->event_inj_err;
+	to->next_rip            = from->next_rip;
 	to->nested_cr3          = from->nested_cr3;
 	to->virt_ext            = from->virt_ext;
 	to->pause_filter_count  = from->pause_filter_count;
@@ -608,7 +609,8 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 	}
 }
 
-static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
+static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
+					  unsigned long vmcb12_rip)
 {
 	u32 int_ctl_vmcb01_bits = V_INTR_MASKING_MASK;
 	u32 int_ctl_vmcb12_bits = V_TPR_MASK | V_IRQ_INJECTION_BITS_MASK;
@@ -662,6 +664,19 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
 	vmcb02->control.event_inj           = svm->nested.ctl.event_inj;
 	vmcb02->control.event_inj_err       = svm->nested.ctl.event_inj_err;
 
+	/*
+	 * next_rip is consumed on VMRUN as the return address pushed on the
+	 * stack for injected soft exceptions/interrupts.  If nrips is exposed
+	 * to L1, take it verbatim from vmcb12.  If nrips is supported in
+	 * hardware but not exposed to L1, stuff the actual L2 RIP to emulate
+	 * what a nrips=0 CPU would do (L1 is responsible for advancing RIP
+	 * prior to injecting the event).
+	 */
+	if (svm->nrips_enabled)
+		vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
+	else if (boot_cpu_has(X86_FEATURE_NRIPS))
+		vmcb02->control.next_rip    = vmcb12_rip;
+
 	vmcb02->control.virt_ext            = vmcb01->control.virt_ext &
 					      LBR_CTL_ENABLE_MASK;
 	if (svm->lbrv_enabled)
@@ -745,7 +760,7 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa,
 	nested_svm_copy_common_state(svm->vmcb01.ptr, svm->nested.vmcb02.ptr);
 
 	svm_switch_vmcb(svm, &svm->nested.vmcb02);
-	nested_vmcb02_prepare_control(svm);
+	nested_vmcb02_prepare_control(svm, vmcb12->save.rip);
 	nested_vmcb02_prepare_save(svm, vmcb12);
 
 	ret = nested_svm_load_cr3(&svm->vcpu, svm->nested.save.cr3,
@@ -1418,6 +1433,7 @@ static void nested_copy_vmcb_cache_to_control(struct vmcb_control_area *dst,
 	dst->nested_ctl           = from->nested_ctl;
 	dst->event_inj            = from->event_inj;
 	dst->event_inj_err        = from->event_inj_err;
+	dst->next_rip             = from->next_rip;
 	dst->nested_cr3           = from->nested_cr3;
 	dst->virt_ext              = from->virt_ext;
 	dst->pause_filter_count   = from->pause_filter_count;
@@ -1602,7 +1618,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	nested_copy_vmcb_control_to_cache(svm, ctl);
 
 	svm_switch_vmcb(svm, &svm->nested.vmcb02);
-	nested_vmcb02_prepare_control(svm);
+	nested_vmcb02_prepare_control(svm, save->rip);
 
 	/*
 	 * While the nested guest CR3 is already checked and set by
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 32220a1b0ea2..7d97e4d18c8b 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -139,6 +139,7 @@ struct vmcb_ctrl_area_cached {
 	u64 nested_ctl;
 	u32 event_inj;
 	u32 event_inj_err;
+	u64 next_rip;
 	u64 nested_cr3;
 	u64 virt_ext;
 	u32 clean;
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 02/11] KVM: SVM: Don't BUG if userspace injects a soft interrupt with GIF=0

[Sean Christopherson]

From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Don't BUG/WARN on interrupt injection due to GIF being cleared if the
injected event is a soft interrupt, which are not actually IRQs and thus
not subject to IRQ blocking conditions.  KVM doesn't currently use event
injection to handle incomplete soft interrupts, but it's trivial for
userspace to force the situation via KVM_SET_VCPU_EVENTS.

Opportunistically downgrade the BUG_ON() to WARN_ON(), there's no need to
bring down the whole host just because there might be some issue with
respect to guest GIF handling in KVM, or as evidenced here, an egregious
oversight with respect to KVM's uAPI.

  kernel BUG at arch/x86/kvm/svm/svm.c:3386!
  invalid opcode: 0000 [#1] SMP
  CPU: 15 PID: 926 Comm: smm_test Not tainted 5.17.0-rc3+ #264
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0010:svm_inject_irq+0xab/0xb0 [kvm_amd]
  Code: <0f> 0b 0f 1f 00 0f 1f 44 00 00 80 3d ac b3 01 00 00 55 48 89 f5 53
  RSP: 0018:ffffc90000b37d88 EFLAGS: 00010246
  RAX: 0000000000000000 RBX: ffff88810a234ac0 RCX: 0000000000000006
  RDX: 0000000000000000 RSI: ffffc90000b37df7 RDI: ffff88810a234ac0
  RBP: ffffc90000b37df7 R08: ffff88810a1fa410 R09: 0000000000000000
  R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
  R13: ffff888109571000 R14: ffff88810a234ac0 R15: 0000000000000000
  FS:  0000000001821380(0000) GS:ffff88846fdc0000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00007f74fc550008 CR3: 000000010a6fe000 CR4: 0000000000350ea0
  Call Trace:
   <TASK>
   inject_pending_event+0x2f7/0x4c0 [kvm]
   kvm_arch_vcpu_ioctl_run+0x791/0x17a0 [kvm]
   kvm_vcpu_ioctl+0x26d/0x650 [kvm]
   __x64_sys_ioctl+0x82/0xb0
   do_syscall_64+0x3b/0xc0
   entry_SYSCALL_64_after_hwframe+0x44/0xae
   </TASK>

Fixes: 219b65dcf6c0 ("KVM: SVM: Improve nested interrupt injection")
Cc: stable@vger.kernel.org
Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 75b4f3ac8b1a..151fba0b405f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3384,7 +3384,7 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	BUG_ON(!(gif_set(svm)));
+	WARN_ON(!vcpu->arch.interrupt.soft && !gif_set(svm));
 
 	trace_kvm_inj_virq(vcpu->arch.interrupt.nr);
 	++vcpu->stat.irq_injections;
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 03/11] KVM: SVM: Unwind "speculative" RIP advancement if INTn injection "fails"

[Sean Christopherson]

Unwind the RIP advancement done by svm_queue_exception() when injecting
an INT3 ultimately "fails" due to the CPU encountering a VM-Exit while
vectoring the injected event, even if the exception reported by the CPU
isn't the same event that was injected.  If vectoring INT3 encounters an
exception, e.g. #NP, and vectoring the #NP encounters an intercepted
exception, e.g. #PF when KVM is using shadow paging, then the #NP will
be reported as the event that was in-progress.

Note, this is still imperfect, as it will get a false positive if the
INT3 is cleanly injected, no VM-Exit occurs before the IRET from the INT3
handler in the guest, the instruction following the INT3 generates an
exception (directly or indirectly), _and_ vectoring that exception
encounters an exception that is intercepted by KVM.  The false positives
could theoretically be solved by further analyzing the vectoring event,
e.g. by comparing the error code against the expected error code were an
exception to occur when vectoring the original injected exception, but
SVM without NRIPS is a complete disaster, trying to make it 100% correct
is a waste of time.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Fixes: 66b7138f9136 ("KVM: SVM: Emulate nRIP feature when reinjecting INT3")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 151fba0b405f..82175a13c668 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3700,6 +3700,18 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
 	vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
 	type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
 
+	/*
+	 * If NextRIP isn't enabled, KVM must manually advance RIP prior to
+	 * injecting the soft exception/interrupt.  That advancement needs to
+	 * be unwound if vectoring didn't complete.  Note, the _new_ event may
+	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
+	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
+	 * be the reported vectored event, but RIP still needs to be unwound.
+	 */
+	if (int3_injected && type == SVM_EXITINTINFO_TYPE_EXEPT &&
+	   kvm_is_linear_rip(vcpu, svm->int3_rip))
+		kvm_rip_write(vcpu, kvm_rip_read(vcpu) - int3_injected);
+
 	switch (type) {
 	case SVM_EXITINTINFO_TYPE_NMI:
 		vcpu->arch.nmi_injected = true;
@@ -3713,16 +3725,11 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
 
 		/*
 		 * In case of software exceptions, do not reinject the vector,
-		 * but re-execute the instruction instead. Rewind RIP first
-		 * if we emulated INT3 before.
+		 * but re-execute the instruction instead.
 		 */
-		if (kvm_exception_is_soft(vector)) {
-			if (vector == BP_VECTOR && int3_injected &&
-			    kvm_is_linear_rip(vcpu, svm->int3_rip))
-				kvm_rip_write(vcpu,
-					      kvm_rip_read(vcpu) - int3_injected);
+		if (kvm_exception_is_soft(vector))
 			break;
-		}
+
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
 			u32 err = svm->vmcb->control.exit_int_info_err;
 			kvm_requeue_exception_e(vcpu, vector, err);
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 04/11] KVM: SVM: Stuff next_rip on emulated INT3 injection if NRIPS is supported

[Sean Christopherson]

If NRIPS is supported in hardware but disabled in KVM, set next_rip to
the next RIP when advancing RIP as part of emulating INT3 injection.
There is no flag to tell the CPU that KVM isn't using next_rip, and so
leaving next_rip is left as is will result in the CPU pushing garbage
onto the stack when vectoring the injected event.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Fixes: 66b7138f9136 ("KVM: SVM: Emulate nRIP feature when reinjecting INT3")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 82175a13c668..14bc4e87437b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -392,6 +392,10 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu)
 		 */
 		(void)svm_skip_emulated_instruction(vcpu);
 		rip = kvm_rip_read(vcpu);
+
+		if (boot_cpu_has(X86_FEATURE_NRIPS))
+			svm->vmcb->control.next_rip = rip;
+
 		svm->int3_rip = rip + svm->vmcb->save.cs.base;
 		svm->int3_injected = rip - old_rip;
 	}
@@ -3703,7 +3707,7 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
 	/*
 	 * If NextRIP isn't enabled, KVM must manually advance RIP prior to
 	 * injecting the soft exception/interrupt.  That advancement needs to
-	 * be unwound if vectoring didn't complete.  Note, the _new_ event may
+	 * be unwound if vectoring didn't complete.  Note, the new event may
 	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
 	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
 	 * be the reported vectored event, but RIP still needs to be unwound.
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

[Sean Christopherson]

Re-inject INT3/INTO instead of retrying the instruction if the CPU
encountered an intercepted exception while vectoring the software
exception, e.g. if vectoring INT3 encounters a #PF and KVM is using
shadow paging.  Retrying the instruction is architecturally wrong, e.g.
will result in a spurious #DB if there's a code breakpoint on the INT3/O,
and lack of re-injection also breaks nested virtualization, e.g. if L1
injects a software exception and vectoring the injected exception
encounters an exception that is intercepted by L0 but not L1.

Due to, ahem, deficiencies in the SVM architecture, acquiring the next
RIP may require flowing through the emulator even if NRIPS is supported,
as the CPU clears next_rip if the VM-Exit is due to an exception other
than "exceptions caused by the INT3, INTO, and BOUND instructions".  To
deal with this, "skip" the instruction to calculate next_rip (if it's
not already known), and then unwind the RIP write and any side effects
(RFLAGS updates).

Save the computed next_rip and use it to re-stuff next_rip if injection
doesn't complete.  This allows KVM to do the right thing if next_rip was
known prior to injection, e.g. if L1 injects a soft event into L2, and
there is no backing INTn instruction, e.g. if L1 is injecting an
arbitrary event.

Note, it's impossible to guarantee architectural correctness given SVM's
architectural flaws.  E.g. if the guest executes INTn (no KVM injection),
an exit occurs while vectoring the INTn, and the guest modifies the code
stream while the exit is being handled, KVM will compute the incorrect
next_rip due to "skipping" the wrong instruction.  A future enhancement
to make this less awful would be for KVM to detect that the decoded
instruction is not the correct INTn and drop the to-be-injected soft
event (retrying is a lesser evil compared to shoving the wrong RIP on the
exception stack).

Reported-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/nested.c |  28 +++++++-
 arch/x86/kvm/svm/svm.c    | 140 +++++++++++++++++++++++++++-----------
 arch/x86/kvm/svm/svm.h    |   6 +-
 3 files changed, 130 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 461c5f247801..0163238aa198 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -609,6 +609,21 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 	}
 }
 
+static inline bool is_evtinj_soft(u32 evtinj)
+{
+	u32 type = evtinj & SVM_EVTINJ_TYPE_MASK;
+	u8 vector = evtinj & SVM_EVTINJ_VEC_MASK;
+
+	if (!(evtinj & SVM_EVTINJ_VALID))
+		return false;
+
+	/*
+	 * Intentionally return false for SOFT events, SVM doesn't yet support
+	 * re-injecting soft interrupts.
+	 */
+	return type == SVM_EVTINJ_TYPE_EXEPT && kvm_exception_is_soft(vector);
+}
+
 static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 					  unsigned long vmcb12_rip)
 {
@@ -677,6 +692,16 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 	else if (boot_cpu_has(X86_FEATURE_NRIPS))
 		vmcb02->control.next_rip    = vmcb12_rip;
 
+	if (is_evtinj_soft(vmcb02->control.event_inj)) {
+		svm->soft_int_injected = true;
+		svm->soft_int_csbase = svm->vmcb->save.cs.base;
+		svm->soft_int_old_rip = vmcb12_rip;
+		if (svm->nrips_enabled)
+			svm->soft_int_next_rip = svm->nested.ctl.next_rip;
+		else
+			svm->soft_int_next_rip = vmcb12_rip;
+	}
+
 	vmcb02->control.virt_ext            = vmcb01->control.virt_ext &
 					      LBR_CTL_ENABLE_MASK;
 	if (svm->lbrv_enabled)
@@ -849,6 +874,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 
 out_exit_err:
 	svm->nested.nested_run_pending = 0;
+	svm->soft_int_injected = false;
 
 	svm->vmcb->control.exit_code    = SVM_EXIT_ERR;
 	svm->vmcb->control.exit_code_hi = 0;
@@ -1618,7 +1644,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	nested_copy_vmcb_control_to_cache(svm, ctl);
 
 	svm_switch_vmcb(svm, &svm->nested.vmcb02);
-	nested_vmcb02_prepare_control(svm, save->rip);
+	nested_vmcb02_prepare_control(svm, svm->vmcb->save.rip);
 
 	/*
 	 * While the nested guest CR3 is already checked and set by
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 14bc4e87437b..8321f9ce5e35 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -342,9 +342,11 @@ static void svm_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
 
 }
 
-static int svm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
+static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
+					   bool commit_side_effects)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
+	unsigned long old_rflags;
 
 	/*
 	 * SEV-ES does not expose the next RIP. The RIP update is controlled by
@@ -359,18 +361,75 @@ static int svm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	}
 
 	if (!svm->next_rip) {
+		if (unlikely(!commit_side_effects))
+			old_rflags = svm->vmcb->save.rflags;
+
 		if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP))
 			return 0;
+
+		if (unlikely(!commit_side_effects))
+			svm->vmcb->save.rflags = old_rflags;
 	} else {
 		kvm_rip_write(vcpu, svm->next_rip);
 	}
 
 done:
-	svm_set_interrupt_shadow(vcpu, 0);
+	if (likely(commit_side_effects))
+		svm_set_interrupt_shadow(vcpu, 0);
 
 	return 1;
 }
 
+static int svm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
+{
+	return __svm_skip_emulated_instruction(vcpu, true);
+}
+
+static int svm_update_soft_interrupt_rip(struct kvm_vcpu *vcpu)
+{
+	unsigned long rip, old_rip = kvm_rip_read(vcpu);
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	/*
+	 * Due to architectural shortcomings, the CPU doesn't always provide
+	 * NextRIP, e.g. if KVM intercepted an exception that occurred while
+	 * the CPU was vectoring an INTO/INT3 in the guest.  Temporarily skip
+	 * the instruction even if NextRIP is supported to acquire the next
+	 * RIP so that it can be shoved into the NextRIP field, otherwise
+	 * hardware will fail to advance guest RIP during event injection.
+	 * Drop the exception/interrupt if emulation fails and effectively
+	 * retry the instruction, it's the least awful option.  If NRIPS is
+	 * in use, the skip must not commit any side effects such as clearing
+	 * the interrupt shadow or RFLAGS.RF.
+	 */
+	if (!__svm_skip_emulated_instruction(vcpu, !nrips))
+		return -EIO;
+
+	rip = kvm_rip_read(vcpu);
+
+	/*
+	 * Save the injection information, even when using next_rip, as the
+	 * VMCB's next_rip will be lost (cleared on VM-Exit) if the injection
+	 * doesn't complete due to a VM-Exit occurring while the CPU is
+	 * vectoring the event.   Decoding the instruction isn't guaranteed to
+	 * work as there may be no backing instruction, e.g. if the event is
+	 * being injected by L1 for L2, or if the guest is patching INT3 into
+	 * a different instruction.
+	 */
+	svm->soft_int_injected = true;
+	svm->soft_int_csbase = svm->vmcb->save.cs.base;
+	svm->soft_int_old_rip = old_rip;
+	svm->soft_int_next_rip = rip;
+
+	if (nrips)
+		kvm_rip_write(vcpu, old_rip);
+
+	if (static_cpu_has(X86_FEATURE_NRIPS))
+		svm->vmcb->control.next_rip = rip;
+
+	return 0;
+}
+
 static void svm_queue_exception(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -380,25 +439,9 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu)
 
 	kvm_deliver_exception_payload(vcpu);
 
-	if (nr == BP_VECTOR && !nrips) {
-		unsigned long rip, old_rip = kvm_rip_read(vcpu);
-
-		/*
-		 * For guest debugging where we have to reinject #BP if some
-		 * INT3 is guest-owned:
-		 * Emulate nRIP by moving RIP forward. Will fail if injection
-		 * raises a fault that is not intercepted. Still better than
-		 * failing in all cases.
-		 */
-		(void)svm_skip_emulated_instruction(vcpu);
-		rip = kvm_rip_read(vcpu);
-
-		if (boot_cpu_has(X86_FEATURE_NRIPS))
-			svm->vmcb->control.next_rip = rip;
-
-		svm->int3_rip = rip + svm->vmcb->save.cs.base;
-		svm->int3_injected = rip - old_rip;
-	}
+	if (kvm_exception_is_soft(nr) &&
+	    svm_update_soft_interrupt_rip(vcpu))
+		return;
 
 	svm->vmcb->control.event_inj = nr
 		| SVM_EVTINJ_VALID
@@ -3671,15 +3714,46 @@ static inline void sync_lapic_to_cr8(struct kvm_vcpu *vcpu)
 	svm->vmcb->control.int_ctl |= cr8 & V_TPR_MASK;
 }
 
+static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,
+					int type)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	/*
+	 * If NRIPS is enabled, KVM must snapshot the pre-VMRUN next_rip that's
+	 * associated with the original soft exception/interrupt.  next_rip is
+	 * cleared on all exits that can occur while vectoring an event, so KVM
+	 * needs to manually set next_rip for re-injection.  Unlike the !nrips
+	 * case below, this needs to be done if and only if KVM is re-injecting
+	 * the same event, i.e. if the event is a soft exception/interrupt,
+	 * otherwise next_rip is unused on VMRUN.
+	 */
+	if (nrips && type == SVM_EXITINTINFO_TYPE_EXEPT &&
+	    kvm_exception_is_soft(vector) &&
+	    kvm_is_linear_rip(vcpu, svm->soft_int_old_rip + svm->soft_int_csbase))
+		svm->vmcb->control.next_rip = svm->soft_int_next_rip;
+	/*
+	 * If NRIPS isn't enabled, KVM must manually advance RIP prior to
+	 * injecting the soft exception/interrupt.  That advancement needs to
+	 * be unwound if vectoring didn't complete.  Note, the new event may
+	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
+	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
+	 * be the reported vectored event, but RIP still needs to be unwound.
+	 */
+	else if (!nrips && type == SVM_EXITINTINFO_TYPE_EXEPT &&
+		 kvm_is_linear_rip(vcpu, svm->soft_int_next_rip + svm->soft_int_csbase))
+		kvm_rip_write(vcpu, svm->soft_int_old_rip);
+}
+
 static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	u8 vector;
 	int type;
 	u32 exitintinfo = svm->vmcb->control.exit_int_info;
-	unsigned int3_injected = svm->int3_injected;
+	bool soft_int_injected = svm->soft_int_injected;
 
-	svm->int3_injected = 0;
+	svm->soft_int_injected = false;
 
 	/*
 	 * If we've made progress since setting HF_IRET_MASK, we've
@@ -3704,17 +3778,8 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
 	vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
 	type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
 
-	/*
-	 * If NextRIP isn't enabled, KVM must manually advance RIP prior to
-	 * injecting the soft exception/interrupt.  That advancement needs to
-	 * be unwound if vectoring didn't complete.  Note, the new event may
-	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
-	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
-	 * be the reported vectored event, but RIP still needs to be unwound.
-	 */
-	if (int3_injected && type == SVM_EXITINTINFO_TYPE_EXEPT &&
-	   kvm_is_linear_rip(vcpu, svm->int3_rip))
-		kvm_rip_write(vcpu, kvm_rip_read(vcpu) - int3_injected);
+	if (soft_int_injected)
+		svm_complete_soft_interrupt(vcpu, vector, type);
 
 	switch (type) {
 	case SVM_EXITINTINFO_TYPE_NMI:
@@ -3727,13 +3792,6 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
 		if (vector == X86_TRAP_VC)
 			break;
 
-		/*
-		 * In case of software exceptions, do not reinject the vector,
-		 * but re-execute the instruction instead.
-		 */
-		if (kvm_exception_is_soft(vector))
-			break;
-
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
 			u32 err = svm->vmcb->control.exit_int_info_err;
 			kvm_requeue_exception_e(vcpu, vector, err);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 7d97e4d18c8b..6acb494e3598 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -230,8 +230,10 @@ struct vcpu_svm {
 	bool nmi_singlestep;
 	u64 nmi_singlestep_guest_rflags;
 
-	unsigned int3_injected;
-	unsigned long int3_rip;
+	unsigned long soft_int_csbase;
+	unsigned long soft_int_old_rip;
+	unsigned long soft_int_next_rip;
+	bool soft_int_injected;
 
 	/* optional nested SVM features that are enabled for this guest  */
 	bool nrips_enabled                : 1;
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 06/11] KVM: SVM: Re-inject INTn instead of retrying the insn on "failure"

[Sean Christopherson]

Re-inject INTn software interrupts instead of retrying the instruction if
the CPU encountered an intercepted exception while vectoring the INTn,
e.g. if KVM intercepted a #PF when utilizing shadow paging.  Retrying the
instruction is architecturally wrong e.g. will result in a spurious #DB
if there's a code breakpoint on the INT3/O, and lack of re-injection also
breaks nested virtualization, e.g. if L1 injects a software interrupt and
vectoring the injected interrupt encounters an exception that is
intercepted by L0 but not L1.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/nested.c |  7 +++----
 arch/x86/kvm/svm/svm.c    | 24 +++++++++++++++++++-----
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 0163238aa198..a83e367ade54 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -617,10 +617,9 @@ static inline bool is_evtinj_soft(u32 evtinj)
 	if (!(evtinj & SVM_EVTINJ_VALID))
 		return false;
 
-	/*
-	 * Intentionally return false for SOFT events, SVM doesn't yet support
-	 * re-injecting soft interrupts.
-	 */
+	if (type == SVM_EVTINJ_TYPE_SOFT)
+		return true;
+
 	return type == SVM_EVTINJ_TYPE_EXEPT && kvm_exception_is_soft(vector);
 }
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8321f9ce5e35..b8fb07eeeca5 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3430,14 +3430,23 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
 static void svm_inject_irq(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
+	u32 type;
 
-	WARN_ON(!vcpu->arch.interrupt.soft && !gif_set(svm));
+	if (vcpu->arch.interrupt.soft) {
+		if (svm_update_soft_interrupt_rip(vcpu))
+			return;
+
+		type = SVM_EVTINJ_TYPE_SOFT;
+	} else {
+		WARN_ON(!gif_set(svm));
+		type = SVM_EVTINJ_TYPE_INTR;
+	}
 
 	trace_kvm_inj_virq(vcpu->arch.interrupt.nr);
 	++vcpu->stat.irq_injections;
 
 	svm->vmcb->control.event_inj = vcpu->arch.interrupt.nr |
-		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
+				       SVM_EVTINJ_VALID | type;
 }
 
 void svm_complete_interrupt_delivery(struct kvm_vcpu *vcpu, int delivery_mode,
@@ -3717,6 +3726,8 @@ static inline void sync_lapic_to_cr8(struct kvm_vcpu *vcpu)
 static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,
 					int type)
 {
+	bool is_exception = (type == SVM_EXITINTINFO_TYPE_EXEPT);
+	bool is_soft = (type == SVM_EXITINTINFO_TYPE_SOFT);
 	struct vcpu_svm *svm = to_svm(vcpu);
 
 	/*
@@ -3728,8 +3739,7 @@ static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,
 	 * the same event, i.e. if the event is a soft exception/interrupt,
 	 * otherwise next_rip is unused on VMRUN.
 	 */
-	if (nrips && type == SVM_EXITINTINFO_TYPE_EXEPT &&
-	    kvm_exception_is_soft(vector) &&
+	if (nrips && (is_soft || (is_exception && kvm_exception_is_soft(vector))) &&
 	    kvm_is_linear_rip(vcpu, svm->soft_int_old_rip + svm->soft_int_csbase))
 		svm->vmcb->control.next_rip = svm->soft_int_next_rip;
 	/*
@@ -3740,7 +3750,7 @@ static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,
 	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
 	 * be the reported vectored event, but RIP still needs to be unwound.
 	 */
-	else if (!nrips && type == SVM_EXITINTINFO_TYPE_EXEPT &&
+	else if (!nrips && (is_soft || is_exception) &&
 		 kvm_is_linear_rip(vcpu, svm->soft_int_next_rip + svm->soft_int_csbase))
 		kvm_rip_write(vcpu, svm->soft_int_old_rip);
 }
@@ -3802,9 +3812,13 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
 	case SVM_EXITINTINFO_TYPE_INTR:
 		kvm_queue_interrupt(vcpu, vector, false);
 		break;
+	case SVM_EXITINTINFO_TYPE_SOFT:
+		kvm_queue_interrupt(vcpu, vector, true);
+		break;
 	default:
 		break;
 	}
+
 }
 
 static void svm_cancel_injection(struct kvm_vcpu *vcpu)
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 07/11] KVM: x86: Trace re-injected exceptions

[Sean Christopherson]

Trace exceptions that are re-injected, not just those that KVM is
injecting for the first time.  Debugging re-injection bugs is painful
enough as is, not having visibility into what KVM is doing only makes
things worse.

Delay propagating pending=>injected in the non-reinjection path so that
the tracing can properly identify reinjected exceptions.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/trace.h | 12 ++++++++----
 arch/x86/kvm/x86.c   | 16 +++++++++-------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index de4762517569..d07428e660e3 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -358,25 +358,29 @@ TRACE_EVENT(kvm_inj_virq,
  * Tracepoint for kvm interrupt injection:
  */
 TRACE_EVENT(kvm_inj_exception,
-	TP_PROTO(unsigned exception, bool has_error, unsigned error_code),
-	TP_ARGS(exception, has_error, error_code),
+	TP_PROTO(unsigned exception, bool has_error, unsigned error_code,
+		 bool reinjected),
+	TP_ARGS(exception, has_error, error_code, reinjected),
 
 	TP_STRUCT__entry(
 		__field(	u8,	exception	)
 		__field(	u8,	has_error	)
 		__field(	u32,	error_code	)
+		__field(	bool,	reinjected	)
 	),
 
 	TP_fast_assign(
 		__entry->exception	= exception;
 		__entry->has_error	= has_error;
 		__entry->error_code	= error_code;
+		__entry->reinjected	= reinjected;
 	),
 
-	TP_printk("%s (0x%x)",
+	TP_printk("%s (0x%x)%s",
 		  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
 		  /* FIXME: don't print error_code if not present */
-		  __entry->has_error ? __entry->error_code : 0)
+		  __entry->has_error ? __entry->error_code : 0,
+		  __entry->reinjected ? " [reinjected]" : "")
 );
 
 /*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 951d0a78ccda..c3ee8dc00d3a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9393,6 +9393,11 @@ int kvm_check_nested_events(struct kvm_vcpu *vcpu)
 
 static void kvm_inject_exception(struct kvm_vcpu *vcpu)
 {
+	trace_kvm_inj_exception(vcpu->arch.exception.nr,
+				vcpu->arch.exception.has_error_code,
+				vcpu->arch.exception.error_code,
+				vcpu->arch.exception.injected);
+
 	if (vcpu->arch.exception.error_code && !is_protmode(vcpu))
 		vcpu->arch.exception.error_code = false;
 	static_call(kvm_x86_queue_exception)(vcpu);
@@ -9450,13 +9455,6 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool *req_immediate_exit)
 
 	/* try to inject new event if pending */
 	if (vcpu->arch.exception.pending) {
-		trace_kvm_inj_exception(vcpu->arch.exception.nr,
-					vcpu->arch.exception.has_error_code,
-					vcpu->arch.exception.error_code);
-
-		vcpu->arch.exception.pending = false;
-		vcpu->arch.exception.injected = true;
-
 		if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
 			__kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
 					     X86_EFLAGS_RF);
@@ -9470,6 +9468,10 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool *req_immediate_exit)
 		}
 
 		kvm_inject_exception(vcpu);
+
+		vcpu->arch.exception.pending = false;
+		vcpu->arch.exception.injected = true;
+
 		can_inject = false;
 	}
 
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 08/11] KVM: x86: Print error code in exception injection tracepoint iff valid

[Sean Christopherson]

Print the error code in the exception injection tracepoint if and only if
the exception has an error code.  Define the entire error code sequence
as a set of formatted strings, print empty strings if there's no error
code, and abuse __print_symbolic() by passing it an empty array to coerce
it into printing the error code as a hex string.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/trace.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index d07428e660e3..385436d12024 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -376,10 +376,11 @@ TRACE_EVENT(kvm_inj_exception,
 		__entry->reinjected	= reinjected;
 	),
 
-	TP_printk("%s (0x%x)%s",
+	TP_printk("%s%s%s%s%s",
 		  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
-		  /* FIXME: don't print error_code if not present */
-		  __entry->has_error ? __entry->error_code : 0,
+		  !__entry->has_error ? "" : " (",
+		  !__entry->has_error ? "" : __print_symbolic(__entry->error_code, { }),
+		  !__entry->has_error ? "" : ")",
 		  __entry->reinjected ? " [reinjected]" : "")
 );
 
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 09/11] KVM: x86: Differentiate Soft vs. Hard IRQs vs. reinjected in tracepoint

[Sean Christopherson]

In the IRQ injection tracepoint, differentiate between Hard IRQs and Soft
"IRQs", i.e. interrupts that are reinjected after incomplete delivery of
a software interrupt from an INTn instruction.  Tag reinjected interrupts
as such, even though the information is usually redundant since soft
interrupts are only ever reinjected by KVM.  Though rare in practice, a
hard IRQ can be reinjected.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/svm/svm.c          |  5 +++--
 arch/x86/kvm/trace.h            | 16 +++++++++++-----
 arch/x86/kvm/vmx/vmx.c          |  4 ++--
 arch/x86/kvm/x86.c              |  4 ++--
 5 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f164c6c1514a..ae088c6fb287 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1400,7 +1400,7 @@ struct kvm_x86_ops {
 	u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu);
 	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
 				unsigned char *hypercall_addr);
-	void (*inject_irq)(struct kvm_vcpu *vcpu);
+	void (*inject_irq)(struct kvm_vcpu *vcpu, bool reinjected);
 	void (*inject_nmi)(struct kvm_vcpu *vcpu);
 	void (*queue_exception)(struct kvm_vcpu *vcpu);
 	void (*cancel_injection)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b8fb07eeeca5..4a912623b961 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3427,7 +3427,7 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
 	++vcpu->stat.nmi_injections;
 }
 
-static void svm_inject_irq(struct kvm_vcpu *vcpu)
+static void svm_inject_irq(struct kvm_vcpu *vcpu, bool reinjected)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	u32 type;
@@ -3442,7 +3442,8 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu)
 		type = SVM_EVTINJ_TYPE_INTR;
 	}
 
-	trace_kvm_inj_virq(vcpu->arch.interrupt.nr);
+	trace_kvm_inj_virq(vcpu->arch.interrupt.nr,
+			   vcpu->arch.interrupt.soft, reinjected);
 	++vcpu->stat.irq_injections;
 
 	svm->vmcb->control.event_inj = vcpu->arch.interrupt.nr |
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 385436d12024..e1b089285c55 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -333,18 +333,24 @@ TRACE_EVENT_KVM_EXIT(kvm_exit);
  * Tracepoint for kvm interrupt injection:
  */
 TRACE_EVENT(kvm_inj_virq,
-	TP_PROTO(unsigned int irq),
-	TP_ARGS(irq),
+	TP_PROTO(unsigned int vector, bool soft, bool reinjected),
+	TP_ARGS(vector, soft, reinjected),
 
 	TP_STRUCT__entry(
-		__field(	unsigned int,	irq		)
+		__field(	unsigned int,	vector		)
+		__field(	bool,		soft		)
+		__field(	unsigned int,	reinjected	)
 	),
 
 	TP_fast_assign(
-		__entry->irq		= irq;
+		__entry->vector		= vector;
+		__entry->soft		= soft;
+		__entry->reinjected	= reinjected;
 	),
 
-	TP_printk("irq %u", __entry->irq)
+	TP_printk("%s 0x%x%s",
+		  __entry->soft ? "Soft/INTn" : "IRQ", __entry->vector,
+		  __entry->reinjected ? " [reinjected]" : "")
 );
 
 #define EXS(x) { x##_VECTOR, "#" #x }
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index cf8581978bce..a0083464682d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4566,13 +4566,13 @@ static void vmx_enable_nmi_window(struct kvm_vcpu *vcpu)
 	exec_controls_setbit(to_vmx(vcpu), CPU_BASED_NMI_WINDOW_EXITING);
 }
 
-static void vmx_inject_irq(struct kvm_vcpu *vcpu)
+static void vmx_inject_irq(struct kvm_vcpu *vcpu, bool reinjected)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	uint32_t intr;
 	int irq = vcpu->arch.interrupt.nr;
 
-	trace_kvm_inj_virq(irq);
+	trace_kvm_inj_virq(irq, vcpu->arch.interrupt.soft, reinjected);
 
 	++vcpu->stat.irq_injections;
 	if (vmx->rmode.vm86_active) {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c3ee8dc00d3a..0a154b54b8aa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9433,7 +9433,7 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool *req_immediate_exit)
 			static_call(kvm_x86_inject_nmi)(vcpu);
 			can_inject = false;
 		} else if (vcpu->arch.interrupt.injected) {
-			static_call(kvm_x86_inject_irq)(vcpu);
+			static_call(kvm_x86_inject_irq)(vcpu, true);
 			can_inject = false;
 		}
 	}
@@ -9524,7 +9524,7 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool *req_immediate_exit)
 			goto out;
 		if (r) {
 			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu), false);
-			static_call(kvm_x86_inject_irq)(vcpu);
+			static_call(kvm_x86_inject_irq)(vcpu, false);
 			WARN_ON(static_call(kvm_x86_interrupt_allowed)(vcpu, true) < 0);
 		}
 		if (kvm_cpu_has_injectable_intr(vcpu))
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 10/11] KVM: selftests: nSVM: Add svm_nested_soft_inject_test

[Sean Christopherson]

From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Add a KVM self-test that checks whether a nSVM L1 is able to successfully
inject a software interrupt and a soft exception into its L2 guest.

In practice, this tests both the next_rip field consistency and
L1-injected event with intervening L0 VMEXIT during its delivery:
the first nested VMRUN (that's also trying to inject a software interrupt)
will immediately trigger a L0 NPF.
This L0 NPF will have zero in its CPU-returned next_rip field, which if
incorrectly reused by KVM will trigger a #PF when trying to return to
such address 0 from the interrupt handler.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
[sean: check exact L2 RIP on first soft interrupt]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   3 +-
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/svm_util.h   |   2 +
 .../kvm/x86_64/svm_nested_soft_inject_test.c  | 151 ++++++++++++++++++
 4 files changed, 156 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 56140068b763..74f3099f0ad3 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -35,9 +35,10 @@
 /x86_64/state_test
 /x86_64/svm_vmcall_test
 /x86_64/svm_int_ctl_test
-/x86_64/tsc_scaling_sync
+/x86_64/svm_nested_soft_inject_test
 /x86_64/sync_regs_test
 /x86_64/tsc_msrs_test
+/x86_64/tsc_scaling_sync
 /x86_64/userspace_io_test
 /x86_64/userspace_msr_exit_test
 /x86_64/vmx_apic_access_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index af582d168621..eedf6bf713d3 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -66,6 +66,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/state_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_preemption_timer_test
 TEST_GEN_PROGS_x86_64 += x86_64/svm_vmcall_test
 TEST_GEN_PROGS_x86_64 += x86_64/svm_int_ctl_test
+TEST_GEN_PROGS_x86_64 += x86_64/svm_nested_soft_inject_test
 TEST_GEN_PROGS_x86_64 += x86_64/tsc_scaling_sync
 TEST_GEN_PROGS_x86_64 += x86_64/sync_regs_test
 TEST_GEN_PROGS_x86_64 += x86_64/userspace_io_test
diff --git a/tools/testing/selftests/kvm/include/x86_64/svm_util.h b/tools/testing/selftests/kvm/include/x86_64/svm_util.h
index a25aabd8f5e7..d49f7c9b4564 100644
--- a/tools/testing/selftests/kvm/include/x86_64/svm_util.h
+++ b/tools/testing/selftests/kvm/include/x86_64/svm_util.h
@@ -16,6 +16,8 @@
 #define CPUID_SVM_BIT		2
 #define CPUID_SVM		BIT_ULL(CPUID_SVM_BIT)
 
+#define SVM_EXIT_EXCP_BASE	0x040
+#define SVM_EXIT_HLT		0x078
 #define SVM_EXIT_MSR		0x07c
 #define SVM_EXIT_VMMCALL	0x081
 
diff --git a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
new file mode 100644
index 000000000000..257aa2280b5c
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Oracle and/or its affiliates.
+ *
+ * Based on:
+ *   svm_int_ctl_test
+ *
+ *   Copyright (C) 2021, Red Hat, Inc.
+ *
+ */
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+#include "svm_util.h"
+
+#define VCPU_ID		0
+#define INT_NR			0x20
+#define X86_FEATURE_NRIPS	BIT(3)
+
+#define vmcall()		\
+	__asm__ __volatile__(	\
+		"vmmcall\n"	\
+		)
+
+#define ud2()			\
+	__asm__ __volatile__(	\
+		"ud2\n"	\
+		)
+
+#define hlt()			\
+	__asm__ __volatile__(	\
+		"hlt\n"	\
+		)
+
+static unsigned int bp_fired;
+static void guest_bp_handler(struct ex_regs *regs)
+{
+	bp_fired++;
+}
+
+static unsigned int int_fired;
+static void l2_guest_code(void);
+
+static void guest_int_handler(struct ex_regs *regs)
+{
+	int_fired++;
+	GUEST_ASSERT_2(regs->rip == (unsigned long)l2_guest_code,
+		       regs->rip, (unsigned long)l2_guest_code);
+}
+
+static void l2_guest_code(void)
+{
+	GUEST_ASSERT(int_fired == 1);
+	vmcall();
+	ud2();
+
+	GUEST_ASSERT(bp_fired == 1);
+	hlt();
+}
+
+static void l1_guest_code(struct svm_test_data *svm)
+{
+	#define L2_GUEST_STACK_SIZE 64
+	unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
+	struct vmcb *vmcb = svm->vmcb;
+
+	/* Prepare for L2 execution. */
+	generic_svm_setup(svm, l2_guest_code,
+			  &l2_guest_stack[L2_GUEST_STACK_SIZE]);
+
+	vmcb->control.intercept_exceptions |= BIT(PF_VECTOR) | BIT(UD_VECTOR);
+	vmcb->control.intercept |= BIT(INTERCEPT_HLT);
+
+	vmcb->control.event_inj = INT_NR | SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_SOFT;
+	/* The return address pushed on stack */
+	vmcb->control.next_rip = vmcb->save.rip;
+
+	run_guest(vmcb, svm->vmcb_gpa);
+	GUEST_ASSERT_3(vmcb->control.exit_code == SVM_EXIT_VMMCALL,
+		       vmcb->control.exit_code,
+		       vmcb->control.exit_info_1, vmcb->control.exit_info_2);
+
+	/* Skip over VMCALL */
+	vmcb->save.rip += 3;
+
+	vmcb->control.event_inj = BP_VECTOR | SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_EXEPT;
+	/* The return address pushed on stack, skip over UD2 */
+	vmcb->control.next_rip = vmcb->save.rip + 2;
+
+	run_guest(vmcb, svm->vmcb_gpa);
+	GUEST_ASSERT_3(vmcb->control.exit_code == SVM_EXIT_HLT,
+		       vmcb->control.exit_code,
+		       vmcb->control.exit_info_1, vmcb->control.exit_info_2);
+
+	GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_cpuid_entry2 *cpuid;
+	struct kvm_vm *vm;
+	vm_vaddr_t svm_gva;
+	struct kvm_guest_debug debug;
+
+	nested_svm_check_supported();
+
+	cpuid = kvm_get_supported_cpuid_entry(0x8000000a);
+	if (!(cpuid->edx & X86_FEATURE_NRIPS)) {
+		print_skip("nRIP Save unavailable");
+		exit(KSFT_SKIP);
+	}
+
+	vm = vm_create_default(VCPU_ID, 0, (void *) l1_guest_code);
+
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(vm, VCPU_ID);
+
+	vm_install_exception_handler(vm, BP_VECTOR, guest_bp_handler);
+	vm_install_exception_handler(vm, INT_NR, guest_int_handler);
+
+	vcpu_alloc_svm(vm, &svm_gva);
+	vcpu_args_set(vm, VCPU_ID, 1, svm_gva);
+
+	memset(&debug, 0, sizeof(debug));
+	vcpu_set_guest_debug(vm, VCPU_ID, &debug);
+
+	struct kvm_run *run = vcpu_state(vm, VCPU_ID);
+	struct ucall uc;
+
+	vcpu_run(vm, VCPU_ID);
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+		    "Got exit_reason other than KVM_EXIT_IO: %u (%s)\n",
+		    run->exit_reason,
+		    exit_reason_str(run->exit_reason));
+
+	switch (get_ucall(vm, VCPU_ID, &uc)) {
+	case UCALL_ABORT:
+		TEST_FAIL("%s at %s:%ld, vals = 0x%lx 0x%lx 0x%lx", (const char *)uc.args[0],
+			  __FILE__, uc.args[1], uc.args[2], uc.args[3], uc.args[4]);
+		break;
+		/* NOT REACHED */
+	case UCALL_DONE:
+		goto done;
+	default:
+		TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
+	}
+done:
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v2 11/11] KVM: SVM: Drop support for CPUs without NRIPS (NextRIP Save) support

[Sean Christopherson]

Drop support for CPUs without NRIPS along with the associated module
param.  Requiring NRIPS simplifies a handful of paths in KVM, especially
paths where KVM has to do silly things when nrips=false but supported in
hardware as there is no way to tell the CPU _not_ to use NRIPS.

NRIPS was introduced in 2009, i.e. every AMD-based CPU released in the
last decade should support NRIPS.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Not-signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/nested.c                     |  9 +--
 arch/x86/kvm/svm/svm.c                        | 77 +++++++------------
 .../kvm/x86_64/svm_nested_soft_inject_test.c  |  6 +-
 3 files changed, 32 insertions(+), 60 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index a83e367ade54..f39c958c77f5 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -681,14 +681,13 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 	/*
 	 * next_rip is consumed on VMRUN as the return address pushed on the
 	 * stack for injected soft exceptions/interrupts.  If nrips is exposed
-	 * to L1, take it verbatim from vmcb12.  If nrips is supported in
-	 * hardware but not exposed to L1, stuff the actual L2 RIP to emulate
-	 * what a nrips=0 CPU would do (L1 is responsible for advancing RIP
-	 * prior to injecting the event).
+	 * to L1, take it verbatim from vmcb12.  If nrips is not exposed to L1,
+	 * stuff the actual L2 RIP to emulate what an nrips=0 CPU would do (L1
+	 * is responsible for advancing RIP prior to injecting the event).
 	 */
 	if (svm->nrips_enabled)
 		vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
-	else if (boot_cpu_has(X86_FEATURE_NRIPS))
+	else
 		vmcb02->control.next_rip    = vmcb12_rip;
 
 	if (is_evtinj_soft(vmcb02->control.event_inj)) {
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 4a912623b961..6e6530c01e34 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -162,10 +162,6 @@ module_param_named(npt, npt_enabled, bool, 0444);
 static int nested = true;
 module_param(nested, int, S_IRUGO);
 
-/* enable/disable Next RIP Save */
-static int nrips = true;
-module_param(nrips, int, 0444);
-
 /* enable/disable Virtual VMLOAD VMSAVE */
 static int vls = true;
 module_param(vls, int, 0444);
@@ -355,10 +351,8 @@ static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
 	if (sev_es_guest(vcpu->kvm))
 		goto done;
 
-	if (nrips && svm->vmcb->control.next_rip != 0) {
-		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
+	if (svm->vmcb->control.next_rip != 0)
 		svm->next_rip = svm->vmcb->control.next_rip;
-	}
 
 	if (!svm->next_rip) {
 		if (unlikely(!commit_side_effects))
@@ -394,15 +388,14 @@ static int svm_update_soft_interrupt_rip(struct kvm_vcpu *vcpu)
 	 * Due to architectural shortcomings, the CPU doesn't always provide
 	 * NextRIP, e.g. if KVM intercepted an exception that occurred while
 	 * the CPU was vectoring an INTO/INT3 in the guest.  Temporarily skip
-	 * the instruction even if NextRIP is supported to acquire the next
-	 * RIP so that it can be shoved into the NextRIP field, otherwise
-	 * hardware will fail to advance guest RIP during event injection.
-	 * Drop the exception/interrupt if emulation fails and effectively
-	 * retry the instruction, it's the least awful option.  If NRIPS is
-	 * in use, the skip must not commit any side effects such as clearing
-	 * the interrupt shadow or RFLAGS.RF.
+	 * the instruction to acquire the next RIP so that it can be shoved
+	 * into the NextRIP field, otherwise hardware will fail to advance
+	 * guest RIP during event injection.  Drop the exception/interrupt if
+	 * emulation fails and effectively retry the instruction, it's the
+	 * least awful option.  The skip must not commit any side effects such
+	 * as clearing the interrupt shadow or RFLAGS.RF.
 	 */
-	if (!__svm_skip_emulated_instruction(vcpu, !nrips))
+	if (!__svm_skip_emulated_instruction(vcpu, false))
 		return -EIO;
 
 	rip = kvm_rip_read(vcpu);
@@ -421,11 +414,9 @@ static int svm_update_soft_interrupt_rip(struct kvm_vcpu *vcpu)
 	svm->soft_int_old_rip = old_rip;
 	svm->soft_int_next_rip = rip;
 
-	if (nrips)
-		kvm_rip_write(vcpu, old_rip);
+	kvm_rip_write(vcpu, old_rip);
 
-	if (static_cpu_has(X86_FEATURE_NRIPS))
-		svm->vmcb->control.next_rip = rip;
+	svm->vmcb->control.next_rip = rip;
 
 	return 0;
 }
@@ -3732,28 +3723,16 @@ static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,
 	struct vcpu_svm *svm = to_svm(vcpu);
 
 	/*
-	 * If NRIPS is enabled, KVM must snapshot the pre-VMRUN next_rip that's
-	 * associated with the original soft exception/interrupt.  next_rip is
-	 * cleared on all exits that can occur while vectoring an event, so KVM
-	 * needs to manually set next_rip for re-injection.  Unlike the !nrips
-	 * case below, this needs to be done if and only if KVM is re-injecting
-	 * the same event, i.e. if the event is a soft exception/interrupt,
-	 * otherwise next_rip is unused on VMRUN.
+	 * KVM must snapshot the pre-VMRUN next_rip that's associated with the
+	 * original soft exception/interrupt.  next_rip is cleared on all exits
+	 * that can occur while vectoring an event, so KVM needs to manually
+	 * set next_rip for re-injection.  This needs to be done if and only if
+	 * KVM is re-injecting the same event, i.e. if the event is a soft
+	 * exception/interrupt, otherwise next_rip is unused on VMRUN.
 	 */
-	if (nrips && (is_soft || (is_exception && kvm_exception_is_soft(vector))) &&
+	if ((is_soft || (is_exception && kvm_exception_is_soft(vector))) &&
 	    kvm_is_linear_rip(vcpu, svm->soft_int_old_rip + svm->soft_int_csbase))
 		svm->vmcb->control.next_rip = svm->soft_int_next_rip;
-	/*
-	 * If NRIPS isn't enabled, KVM must manually advance RIP prior to
-	 * injecting the soft exception/interrupt.  That advancement needs to
-	 * be unwound if vectoring didn't complete.  Note, the new event may
-	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
-	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
-	 * be the reported vectored event, but RIP still needs to be unwound.
-	 */
-	else if (!nrips && (is_soft || is_exception) &&
-		 kvm_is_linear_rip(vcpu, svm->soft_int_next_rip + svm->soft_int_csbase))
-		kvm_rip_write(vcpu, svm->soft_int_old_rip);
 }
 
 static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
@@ -4112,8 +4091,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 				    boot_cpu_has(X86_FEATURE_XSAVES);
 
 	/* Update nrips enabled cache */
-	svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
-			     guest_cpuid_has(vcpu, X86_FEATURE_NRIPS);
+	svm->nrips_enabled = guest_cpuid_has(vcpu, X86_FEATURE_NRIPS);
 
 	svm->tsc_scaling_enabled = tsc_scaling && guest_cpuid_has(vcpu, X86_FEATURE_TSCRATEMSR);
 	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
@@ -4324,9 +4302,7 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
 		break;
 	}
 
-	/* TODO: Advertise NRIPS to guest hypervisor unconditionally */
-	if (static_cpu_has(X86_FEATURE_NRIPS))
-		vmcb->control.next_rip  = info->next_rip;
+	vmcb->control.next_rip  = info->next_rip;
 	vmcb->control.exit_code = icpt_info.exit_code;
 	vmexit = nested_svm_exit_handled(svm);
 
@@ -4859,9 +4835,7 @@ static __init void svm_set_cpu_caps(void)
 	if (nested) {
 		kvm_cpu_cap_set(X86_FEATURE_SVM);
 		kvm_cpu_cap_set(X86_FEATURE_VMCBCLEAN);
-
-		if (nrips)
-			kvm_cpu_cap_set(X86_FEATURE_NRIPS);
+		kvm_cpu_cap_set(X86_FEATURE_NRIPS);
 
 		if (npt_enabled)
 			kvm_cpu_cap_set(X86_FEATURE_NPT);
@@ -4908,6 +4882,12 @@ static __init int svm_hardware_setup(void)
 	int r;
 	unsigned int order = get_order(IOPM_SIZE);
 
+	/* KVM no longer supports CPUs without NextRIP Save support. */
+	if (!boot_cpu_has(X86_FEATURE_NRIPS)) {
+		pr_err_ratelimited("NRIPS (NextRIP Save) not supported\n");
+		return -EOPNOTSUPP;
+	}
+
 	/*
 	 * NX is required for shadow paging and for NPT if the NX huge pages
 	 * mitigation is enabled.
@@ -4989,11 +4969,6 @@ static __init int svm_hardware_setup(void)
 			goto err;
 	}
 
-	if (nrips) {
-		if (!boot_cpu_has(X86_FEATURE_NRIPS))
-			nrips = false;
-	}
-
 	enable_apicv = avic = avic && npt_enabled && (boot_cpu_has(X86_FEATURE_AVIC) || force_avic);
 
 	if (enable_apicv) {
diff --git a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
index 257aa2280b5c..39a6569715fd 100644
--- a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
+++ b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
@@ -106,10 +106,8 @@ int main(int argc, char *argv[])
 	nested_svm_check_supported();
 
 	cpuid = kvm_get_supported_cpuid_entry(0x8000000a);
-	if (!(cpuid->edx & X86_FEATURE_NRIPS)) {
-		print_skip("nRIP Save unavailable");
-		exit(KSFT_SKIP);
-	}
+	TEST_ASSERT(cpuid->edx & X86_FEATURE_NRIPS,
+		    "KVM is supposed to unconditionally advertise nRIP Save\n");
 
 	vm = vm_create_default(VCPU_ID, 0, (void *) l1_guest_code);
 
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [PATCH v2 11/11] KVM: SVM: Drop support for CPUs without NRIPS (NextRIP Save) support

[Maxim Levitsky]

On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> Drop support for CPUs without NRIPS along with the associated module
> param.  Requiring NRIPS simplifies a handful of paths in KVM, especially
> paths where KVM has to do silly things when nrips=false but supported in
> hardware as there is no way to tell the CPU _not_ to use NRIPS.
> 
> NRIPS was introduced in 2009, i.e. every AMD-based CPU released in the
> last decade should support NRIPS.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Not-signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/nested.c                     |  9 +--
>  arch/x86/kvm/svm/svm.c                        | 77 +++++++------------
>  .../kvm/x86_64/svm_nested_soft_inject_test.c  |  6 +-
>  3 files changed, 32 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index a83e367ade54..f39c958c77f5 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -681,14 +681,13 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  	/*
>  	 * next_rip is consumed on VMRUN as the return address pushed on the
>  	 * stack for injected soft exceptions/interrupts.  If nrips is exposed
> -	 * to L1, take it verbatim from vmcb12.  If nrips is supported in
> -	 * hardware but not exposed to L1, stuff the actual L2 RIP to emulate
> -	 * what a nrips=0 CPU would do (L1 is responsible for advancing RIP
> -	 * prior to injecting the event).
> +	 * to L1, take it verbatim from vmcb12.  If nrips is not exposed to L1,
> +	 * stuff the actual L2 RIP to emulate what an nrips=0 CPU would do (L1
> +	 * is responsible for advancing RIP prior to injecting the event).
>  	 */
>  	if (svm->nrips_enabled)
>  		vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
> -	else if (boot_cpu_has(X86_FEATURE_NRIPS))
> +	else
>  		vmcb02->control.next_rip    = vmcb12_rip;
>  
>  	if (is_evtinj_soft(vmcb02->control.event_inj)) {
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 4a912623b961..6e6530c01e34 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -162,10 +162,6 @@ module_param_named(npt, npt_enabled, bool, 0444);
>  static int nested = true;
>  module_param(nested, int, S_IRUGO);
>  
> -/* enable/disable Next RIP Save */
> -static int nrips = true;
> -module_param(nrips, int, 0444);
> -
>  /* enable/disable Virtual VMLOAD VMSAVE */
>  static int vls = true;
>  module_param(vls, int, 0444);
> @@ -355,10 +351,8 @@ static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
>  	if (sev_es_guest(vcpu->kvm))
>  		goto done;
>  
> -	if (nrips && svm->vmcb->control.next_rip != 0) {
> -		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
> +	if (svm->vmcb->control.next_rip != 0)
>  		svm->next_rip = svm->vmcb->control.next_rip;
> -	}
>  
>  	if (!svm->next_rip) {
>  		if (unlikely(!commit_side_effects))
> @@ -394,15 +388,14 @@ static int svm_update_soft_interrupt_rip(struct kvm_vcpu *vcpu)
>  	 * Due to architectural shortcomings, the CPU doesn't always provide
>  	 * NextRIP, e.g. if KVM intercepted an exception that occurred while
>  	 * the CPU was vectoring an INTO/INT3 in the guest.  Temporarily skip
> -	 * the instruction even if NextRIP is supported to acquire the next
> -	 * RIP so that it can be shoved into the NextRIP field, otherwise
> -	 * hardware will fail to advance guest RIP during event injection.
> -	 * Drop the exception/interrupt if emulation fails and effectively
> -	 * retry the instruction, it's the least awful option.  If NRIPS is
> -	 * in use, the skip must not commit any side effects such as clearing
> -	 * the interrupt shadow or RFLAGS.RF.
> +	 * the instruction to acquire the next RIP so that it can be shoved
> +	 * into the NextRIP field, otherwise hardware will fail to advance
> +	 * guest RIP during event injection.  Drop the exception/interrupt if
> +	 * emulation fails and effectively retry the instruction, it's the
> +	 * least awful option.  The skip must not commit any side effects such
> +	 * as clearing the interrupt shadow or RFLAGS.RF.
>  	 */
> -	if (!__svm_skip_emulated_instruction(vcpu, !nrips))
> +	if (!__svm_skip_emulated_instruction(vcpu, false))
>  		return -EIO;
>  
>  	rip = kvm_rip_read(vcpu);
> @@ -421,11 +414,9 @@ static int svm_update_soft_interrupt_rip(struct kvm_vcpu *vcpu)
>  	svm->soft_int_old_rip = old_rip;
>  	svm->soft_int_next_rip = rip;
>  
> -	if (nrips)
> -		kvm_rip_write(vcpu, old_rip);
> +	kvm_rip_write(vcpu, old_rip);
>  
> -	if (static_cpu_has(X86_FEATURE_NRIPS))
> -		svm->vmcb->control.next_rip = rip;
> +	svm->vmcb->control.next_rip = rip;
>  
>  	return 0;
>  }
> @@ -3732,28 +3723,16 @@ static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
>  	/*
> -	 * If NRIPS is enabled, KVM must snapshot the pre-VMRUN next_rip that's
> -	 * associated with the original soft exception/interrupt.  next_rip is
> -	 * cleared on all exits that can occur while vectoring an event, so KVM
> -	 * needs to manually set next_rip for re-injection.  Unlike the !nrips
> -	 * case below, this needs to be done if and only if KVM is re-injecting
> -	 * the same event, i.e. if the event is a soft exception/interrupt,
> -	 * otherwise next_rip is unused on VMRUN.
> +	 * KVM must snapshot the pre-VMRUN next_rip that's associated with the
> +	 * original soft exception/interrupt.  next_rip is cleared on all exits
> +	 * that can occur while vectoring an event, so KVM needs to manually
> +	 * set next_rip for re-injection.  This needs to be done if and only if
> +	 * KVM is re-injecting the same event, i.e. if the event is a soft
> +	 * exception/interrupt, otherwise next_rip is unused on VMRUN.
>  	 */
> -	if (nrips && (is_soft || (is_exception && kvm_exception_is_soft(vector))) &&
> +	if ((is_soft || (is_exception && kvm_exception_is_soft(vector))) &&
>  	    kvm_is_linear_rip(vcpu, svm->soft_int_old_rip + svm->soft_int_csbase))
>  		svm->vmcb->control.next_rip = svm->soft_int_next_rip;
> -	/*
> -	 * If NRIPS isn't enabled, KVM must manually advance RIP prior to
> -	 * injecting the soft exception/interrupt.  That advancement needs to
> -	 * be unwound if vectoring didn't complete.  Note, the new event may
> -	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
> -	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
> -	 * be the reported vectored event, but RIP still needs to be unwound.
> -	 */
> -	else if (!nrips && (is_soft || is_exception) &&
> -		 kvm_is_linear_rip(vcpu, svm->soft_int_next_rip + svm->soft_int_csbase))
> -		kvm_rip_write(vcpu, svm->soft_int_old_rip);
>  }
>  
>  static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
> @@ -4112,8 +4091,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  				    boot_cpu_has(X86_FEATURE_XSAVES);
>  
>  	/* Update nrips enabled cache */
> -	svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
> -			     guest_cpuid_has(vcpu, X86_FEATURE_NRIPS);
> +	svm->nrips_enabled = guest_cpuid_has(vcpu, X86_FEATURE_NRIPS);
>  
>  	svm->tsc_scaling_enabled = tsc_scaling && guest_cpuid_has(vcpu, X86_FEATURE_TSCRATEMSR);
>  	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
> @@ -4324,9 +4302,7 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
>  		break;
>  	}
>  
> -	/* TODO: Advertise NRIPS to guest hypervisor unconditionally */
> -	if (static_cpu_has(X86_FEATURE_NRIPS))
> -		vmcb->control.next_rip  = info->next_rip;
> +	vmcb->control.next_rip  = info->next_rip;
>  	vmcb->control.exit_code = icpt_info.exit_code;
>  	vmexit = nested_svm_exit_handled(svm);
>  
> @@ -4859,9 +4835,7 @@ static __init void svm_set_cpu_caps(void)
>  	if (nested) {
>  		kvm_cpu_cap_set(X86_FEATURE_SVM);
>  		kvm_cpu_cap_set(X86_FEATURE_VMCBCLEAN);
> -
> -		if (nrips)
> -			kvm_cpu_cap_set(X86_FEATURE_NRIPS);
> +		kvm_cpu_cap_set(X86_FEATURE_NRIPS);
>  
>  		if (npt_enabled)
>  			kvm_cpu_cap_set(X86_FEATURE_NPT);
> @@ -4908,6 +4882,12 @@ static __init int svm_hardware_setup(void)
>  	int r;
>  	unsigned int order = get_order(IOPM_SIZE);
>  
> +	/* KVM no longer supports CPUs without NextRIP Save support. */
> +	if (!boot_cpu_has(X86_FEATURE_NRIPS)) {
> +		pr_err_ratelimited("NRIPS (NextRIP Save) not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	/*
>  	 * NX is required for shadow paging and for NPT if the NX huge pages
>  	 * mitigation is enabled.
> @@ -4989,11 +4969,6 @@ static __init int svm_hardware_setup(void)
>  			goto err;
>  	}
>  
> -	if (nrips) {
> -		if (!boot_cpu_has(X86_FEATURE_NRIPS))
> -			nrips = false;
> -	}
> -
>  	enable_apicv = avic = avic && npt_enabled && (boot_cpu_has(X86_FEATURE_AVIC) || force_avic);
>  
>  	if (enable_apicv) {
> diff --git a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
> index 257aa2280b5c..39a6569715fd 100644
> --- a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
> @@ -106,10 +106,8 @@ int main(int argc, char *argv[])
>  	nested_svm_check_supported();
>  
>  	cpuid = kvm_get_supported_cpuid_entry(0x8000000a);
> -	if (!(cpuid->edx & X86_FEATURE_NRIPS)) {
> -		print_skip("nRIP Save unavailable");
> -		exit(KSFT_SKIP);
> -	}
> +	TEST_ASSERT(cpuid->edx & X86_FEATURE_NRIPS,
> +		    "KVM is supposed to unconditionally advertise nRIP Save\n");
>  
>  	vm = vm_create_default(VCPU_ID, 0, (void *) l1_guest_code);
>  


The only issue would be IMHO that if (for testing/whatever) you boot a guest without NRIPS,
then the guest won't be able to use SVM
Or in other words, its true that every sane AMD cpu supports NRIPs, but a "nested AMD cpu"
in theory might not.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

[Maciej S. Szmigiero]

On 23.04.2022 04:14, Sean Christopherson wrote:
> Re-inject INT3/INTO instead of retrying the instruction if the CPU
> encountered an intercepted exception while vectoring the software
> exception, e.g. if vectoring INT3 encounters a #PF and KVM is using
> shadow paging.  Retrying the instruction is architecturally wrong, e.g.
> will result in a spurious #DB if there's a code breakpoint on the INT3/O,
> and lack of re-injection also breaks nested virtualization, e.g. if L1
> injects a software exception and vectoring the injected exception
> encounters an exception that is intercepted by L0 but not L1.
> 
> Due to, ahem, deficiencies in the SVM architecture, acquiring the next
> RIP may require flowing through the emulator even if NRIPS is supported,
> as the CPU clears next_rip if the VM-Exit is due to an exception other
> than "exceptions caused by the INT3, INTO, and BOUND instructions".  To
> deal with this, "skip" the instruction to calculate next_rip (if it's
> not already known), and then unwind the RIP write and any side effects
> (RFLAGS updates).
> 
> Save the computed next_rip and use it to re-stuff next_rip if injection
> doesn't complete.  This allows KVM to do the right thing if next_rip was
> known prior to injection, e.g. if L1 injects a soft event into L2, and
> there is no backing INTn instruction, e.g. if L1 is injecting an
> arbitrary event.
> 
> Note, it's impossible to guarantee architectural correctness given SVM's
> architectural flaws.  E.g. if the guest executes INTn (no KVM injection),
> an exit occurs while vectoring the INTn, and the guest modifies the code
> stream while the exit is being handled, KVM will compute the incorrect
> next_rip due to "skipping" the wrong instruction.  A future enhancement
> to make this less awful would be for KVM to detect that the decoded
> instruction is not the correct INTn and drop the to-be-injected soft
> event (retrying is a lesser evil compared to shoving the wrong RIP on the
> exception stack).
> 
> Reported-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/nested.c |  28 +++++++-
>   arch/x86/kvm/svm/svm.c    | 140 +++++++++++++++++++++++++++-----------
>   arch/x86/kvm/svm/svm.h    |   6 +-
>   3 files changed, 130 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 461c5f247801..0163238aa198 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -609,6 +609,21 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>   	}
>   }
>   
> +static inline bool is_evtinj_soft(u32 evtinj)
> +{
> +	u32 type = evtinj & SVM_EVTINJ_TYPE_MASK;
> +	u8 vector = evtinj & SVM_EVTINJ_VEC_MASK;
> +
> +	if (!(evtinj & SVM_EVTINJ_VALID))
> +		return false;
> +
> +	/*
> +	 * Intentionally return false for SOFT events, SVM doesn't yet support
> +	 * re-injecting soft interrupts.
> +	 */
> +	return type == SVM_EVTINJ_TYPE_EXEPT && kvm_exception_is_soft(vector);
> +}
> +
>   static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>   					  unsigned long vmcb12_rip)
>   {
> @@ -677,6 +692,16 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>   	else if (boot_cpu_has(X86_FEATURE_NRIPS))
>   		vmcb02->control.next_rip    = vmcb12_rip;
>   
> +	if (is_evtinj_soft(vmcb02->control.event_inj)) {
> +		svm->soft_int_injected = true;
> +		svm->soft_int_csbase = svm->vmcb->save.cs.base;
> +		svm->soft_int_old_rip = vmcb12_rip;
> +		if (svm->nrips_enabled)
> +			svm->soft_int_next_rip = svm->nested.ctl.next_rip;
> +		else
> +			svm->soft_int_next_rip = vmcb12_rip;

The above branch can be actually reduced to simply
"svm->soft_int_next_rip = vmcb02->control.next_rip" if NRIPS is required
for (at least) nSVM.

Thanks,
Maciej

Re: [PATCH v2 09/11] KVM: x86: Differentiate Soft vs. Hard IRQs vs. reinjected in tracepoint

[Maciej S. Szmigiero]

On 23.04.2022 04:14, Sean Christopherson wrote:
> In the IRQ injection tracepoint, differentiate between Hard IRQs and Soft
> "IRQs", i.e. interrupts that are reinjected after incomplete delivery of
> a software interrupt from an INTn instruction.  Tag reinjected interrupts
> as such, even though the information is usually redundant since soft
> interrupts are only ever reinjected by KVM.  Though rare in practice, a
> hard IRQ can be reinjected.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/kvm_host.h |  2 +-
>   arch/x86/kvm/svm/svm.c          |  5 +++--
>   arch/x86/kvm/trace.h            | 16 +++++++++++-----
>   arch/x86/kvm/vmx/vmx.c          |  4 ++--
>   arch/x86/kvm/x86.c              |  4 ++--
>   5 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index f164c6c1514a..ae088c6fb287 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1400,7 +1400,7 @@ struct kvm_x86_ops {
>   	u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu);
>   	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
>   				unsigned char *hypercall_addr);
> -	void (*inject_irq)(struct kvm_vcpu *vcpu);
> +	void (*inject_irq)(struct kvm_vcpu *vcpu, bool reinjected);
>   	void (*inject_nmi)(struct kvm_vcpu *vcpu);
>   	void (*queue_exception)(struct kvm_vcpu *vcpu);
>   	void (*cancel_injection)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b8fb07eeeca5..4a912623b961 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3427,7 +3427,7 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>   	++vcpu->stat.nmi_injections;
>   }
>   
> -static void svm_inject_irq(struct kvm_vcpu *vcpu)
> +static void svm_inject_irq(struct kvm_vcpu *vcpu, bool reinjected)
>   {
>   	struct vcpu_svm *svm = to_svm(vcpu);
>   	u32 type;
> @@ -3442,7 +3442,8 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu)
>   		type = SVM_EVTINJ_TYPE_INTR;
>   	}
>   
> -	trace_kvm_inj_virq(vcpu->arch.interrupt.nr);
> +	trace_kvm_inj_virq(vcpu->arch.interrupt.nr,
> +			   vcpu->arch.interrupt.soft, reinjected);
>   	++vcpu->stat.irq_injections;
>   
>   	svm->vmcb->control.event_inj = vcpu->arch.interrupt.nr |
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 385436d12024..e1b089285c55 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -333,18 +333,24 @@ TRACE_EVENT_KVM_EXIT(kvm_exit);
>    * Tracepoint for kvm interrupt injection:
>    */
>   TRACE_EVENT(kvm_inj_virq,
> -	TP_PROTO(unsigned int irq),
> -	TP_ARGS(irq),
> +	TP_PROTO(unsigned int vector, bool soft, bool reinjected),
> +	TP_ARGS(vector, soft, reinjected),
>   
>   	TP_STRUCT__entry(
> -		__field(	unsigned int,	irq		)
> +		__field(	unsigned int,	vector		)
> +		__field(	bool,		soft		)
> +		__field(	unsigned int,	reinjected	)

The "reinjected" field was probably supposed to be bool, just like
in the trace function prototype.

Thanks,
Maciej

Re: [PATCH v2 10/11] KVM: selftests: nSVM: Add svm_nested_soft_inject_test

[Maciej S. Szmigiero]

On 23.04.2022 04:14, Sean Christopherson wrote:
> From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> Add a KVM self-test that checks whether a nSVM L1 is able to successfully
> inject a software interrupt and a soft exception into its L2 guest.
> 
> In practice, this tests both the next_rip field consistency and
> L1-injected event with intervening L0 VMEXIT during its delivery:
> the first nested VMRUN (that's also trying to inject a software interrupt)
> will immediately trigger a L0 NPF.
> This L0 NPF will have zero in its CPU-returned next_rip field, which if
> incorrectly reused by KVM will trigger a #PF when trying to return to
> such address 0 from the interrupt handler.
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> [sean: check exact L2 RIP on first soft interrupt]
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Looks like this version doesn't integrate the changes that Maxim has suggested [1].
Will provide an updated version after I test the patch set.

Thanks,
Maciej

[1]: https://lore.kernel.org/kvm/2401bf729beab6d9348fda18f55e90ed9c1f7583.camel@redhat.com/

Re: [PATCH v2 11/11] KVM: SVM: Drop support for CPUs without NRIPS (NextRIP Save) support

[Maciej S. Szmigiero]

On 23.04.2022 04:14, Sean Christopherson wrote:
> Drop support for CPUs without NRIPS along with the associated module
> param.  Requiring NRIPS simplifies a handful of paths in KVM, especially
> paths where KVM has to do silly things when nrips=false but supported in
> hardware as there is no way to tell the CPU _not_ to use NRIPS.
> 
> NRIPS was introduced in 2009, i.e. every AMD-based CPU released in the
> last decade should support NRIPS.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Not-signed-off-by: Sean Christopherson <seanjc@google.com>

To be honest, I think completely removing KVM support (rather than just nSVM)
for these older AMD CPUs is a bit too much.
I totally envision complaints coming after this change reaches distro kernels.

After all, even older Yonah parts remain supported on the VMX side.

Thanks,
Maciej

Re: [PATCH v2 00/11] KVM: SVM: Fix soft int/ex re-injection

[Maciej S. Szmigiero]

On 23.04.2022 04:14, Sean Christopherson wrote:
> Fix soft interrupt/exception reinjection on SVM.
> 

Thanks for the patch set Sean, I can't see anything being obviously wrong
during a static code review - just small nits.

Will test it practically tomorrow and report the results.

Thanks,
Maciej

Re: [PATCH v2 00/11] KVM: SVM: Fix soft int/ex re-injection

[Maciej S. Szmigiero]

On 26.04.2022 01:01, Maciej S. Szmigiero wrote:
> On 23.04.2022 04:14, Sean Christopherson wrote:
>> Fix soft interrupt/exception reinjection on SVM.
>>
> 
> Thanks for the patch set Sean, I can't see anything being obviously wrong
> during a static code review - just small nits.
> 
> Will test it practically tomorrow and report the results.

I've tested this patch set and it seems to work fine with respect
to soft {exception,interrupt} re-injection and next_rip field consistency.

I have prepared a draft of an updated version at [1] with the following
further changes:
* "Downgraded" the commit affecting !nrips CPUs to just drop nested SVM
support for such parts instead of SVM support in general,

* Added a fix for L1/L2 NMI state confusion during L1 -> L2 NMI re-injection,

* Updated the new KVM self-test to also check for the NMI injection
scenario being fixed (that was found causing issues with a real guest),

* Changed "kvm_inj_virq" trace event "reinjected" field type to bool.

Will post a v3 patch set (with proper SoBs, etc.) if there are no further
comments or objections.

Thanks,
Maciej

[1]: https://github.com/maciejsszmigiero/linux/commits/svm_next_rip-sc

Re: [PATCH v2 02/11] KVM: SVM: Don't BUG if userspace injects a soft interrupt with GIF=0

[Maxim Levitsky]

On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> Don't BUG/WARN on interrupt injection due to GIF being cleared if the
> injected event is a soft interrupt, which are not actually IRQs and thus

Are any injected events subject to GIF set? I think that EVENTINJ just injects
unconditionaly whatever hypervisor puts in it.

Best regards,
	Maxim Levitsky

> not subject to IRQ blocking conditions.  KVM doesn't currently use event
> injection to handle incomplete soft interrupts, but it's trivial for
> userspace to force the situation via KVM_SET_VCPU_EVENTS.
> 
> Opportunistically downgrade the BUG_ON() to WARN_ON(), there's no need to
> bring down the whole host just because there might be some issue with
> respect to guest GIF handling in KVM, or as evidenced here, an egregious
> oversight with respect to KVM's uAPI.
> 
>   kernel BUG at arch/x86/kvm/svm/svm.c:3386!
>   invalid opcode: 0000 [#1] SMP
>   CPU: 15 PID: 926 Comm: smm_test Not tainted 5.17.0-rc3+ #264
>   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
>   RIP: 0010:svm_inject_irq+0xab/0xb0 [kvm_amd]
>   Code: <0f> 0b 0f 1f 00 0f 1f 44 00 00 80 3d ac b3 01 00 00 55 48 89 f5 53
>   RSP: 0018:ffffc90000b37d88 EFLAGS: 00010246
>   RAX: 0000000000000000 RBX: ffff88810a234ac0 RCX: 0000000000000006
>   RDX: 0000000000000000 RSI: ffffc90000b37df7 RDI: ffff88810a234ac0
>   RBP: ffffc90000b37df7 R08: ffff88810a1fa410 R09: 0000000000000000
>   R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
>   R13: ffff888109571000 R14: ffff88810a234ac0 R15: 0000000000000000
>   FS:  0000000001821380(0000) GS:ffff88846fdc0000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 00007f74fc550008 CR3: 000000010a6fe000 CR4: 0000000000350ea0
>   Call Trace:
>    <TASK>
>    inject_pending_event+0x2f7/0x4c0 [kvm]
>    kvm_arch_vcpu_ioctl_run+0x791/0x17a0 [kvm]
>    kvm_vcpu_ioctl+0x26d/0x650 [kvm]
>    __x64_sys_ioctl+0x82/0xb0
>    do_syscall_64+0x3b/0xc0
>    entry_SYSCALL_64_after_hwframe+0x44/0xae
>    </TASK>
> 
> Fixes: 219b65dcf6c0 ("KVM: SVM: Improve nested interrupt injection")
> Cc: stable@vger.kernel.org
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 75b4f3ac8b1a..151fba0b405f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3384,7 +3384,7 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> -	BUG_ON(!(gif_set(svm)));
> +	WARN_ON(!vcpu->arch.interrupt.soft && !gif_set(svm));
>  
>  	trace_kvm_inj_virq(vcpu->arch.interrupt.nr);
>  	++vcpu->stat.irq_injections;

Re: [PATCH v2 01/11] KVM: nSVM: Sync next_rip field from vmcb12 to vmcb02

[Maxim Levitsky]

On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> The next_rip field of a VMCB is *not* an output-only field for a VMRUN.
> This field value (instead of the saved guest RIP) in used by the CPU for
> the return address pushed on stack when injecting a software interrupt or
> INT3 or INTO exception.
> 
> Make sure this field gets synced from vmcb12 to vmcb02 when entering L2 or
> loading a nested state and NRIPS is exposed to L1.  If NRIPS is supported
> in hardware but not exposed to L1 (nrips=0 or hidden by userspace), stuff
> vmcb02's next_rip from the new L2 RIP to emulate a !NRIPS CPU (which
> saves RIP on the stack as-is).
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/nested.c | 22 +++++++++++++++++++---
>  arch/x86/kvm/svm/svm.h    |  1 +
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index bed5e1692cef..461c5f247801 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -371,6 +371,7 @@ void __nested_copy_vmcb_control_to_cache(struct kvm_vcpu *vcpu,
>  	to->nested_ctl          = from->nested_ctl;
>  	to->event_inj           = from->event_inj;
>  	to->event_inj_err       = from->event_inj_err;
> +	to->next_rip            = from->next_rip;
>  	to->nested_cr3          = from->nested_cr3;
>  	to->virt_ext            = from->virt_ext;
>  	to->pause_filter_count  = from->pause_filter_count;
> @@ -608,7 +609,8 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>  	}
>  }
>  
> -static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
> +static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
> +					  unsigned long vmcb12_rip)

I know that I already reviewed this, but why do we need to pass an extra
parameter to nested_vmcb02_prepare_control.
Lets just put that value in the cache to be consistent with the rest?

Best regards,
	Maxim Levitsky


>  {
>  	u32 int_ctl_vmcb01_bits = V_INTR_MASKING_MASK;
>  	u32 int_ctl_vmcb12_bits = V_TPR_MASK | V_IRQ_INJECTION_BITS_MASK;
> @@ -662,6 +664,19 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>  	vmcb02->control.event_inj           = svm->nested.ctl.event_inj;
>  	vmcb02->control.event_inj_err       = svm->nested.ctl.event_inj_err;
>  
> +	/*
> +	 * next_rip is consumed on VMRUN as the return address pushed on the
> +	 * stack for injected soft exceptions/interrupts.  If nrips is exposed
> +	 * to L1, take it verbatim from vmcb12.  If nrips is supported in
> +	 * hardware but not exposed to L1, stuff the actual L2 RIP to emulate
> +	 * what a nrips=0 CPU would do (L1 is responsible for advancing RIP
> +	 * prior to injecting the event).
> +	 */
> +	if (svm->nrips_enabled)
> +		vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
> +	else if (boot_cpu_has(X86_FEATURE_NRIPS))
> +		vmcb02->control.next_rip    = vmcb12_rip;
> +
>  	vmcb02->control.virt_ext            = vmcb01->control.virt_ext &
>  					      LBR_CTL_ENABLE_MASK;
>  	if (svm->lbrv_enabled)
> @@ -745,7 +760,7 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa,
>  	nested_svm_copy_common_state(svm->vmcb01.ptr, svm->nested.vmcb02.ptr);
>  
>  	svm_switch_vmcb(svm, &svm->nested.vmcb02);
> -	nested_vmcb02_prepare_control(svm);
> +	nested_vmcb02_prepare_control(svm, vmcb12->save.rip);
>  	nested_vmcb02_prepare_save(svm, vmcb12);
>  
>  	ret = nested_svm_load_cr3(&svm->vcpu, svm->nested.save.cr3,
> @@ -1418,6 +1433,7 @@ static void nested_copy_vmcb_cache_to_control(struct vmcb_control_area *dst,
>  	dst->nested_ctl           = from->nested_ctl;
>  	dst->event_inj            = from->event_inj;
>  	dst->event_inj_err        = from->event_inj_err;
> +	dst->next_rip             = from->next_rip;
>  	dst->nested_cr3           = from->nested_cr3;
>  	dst->virt_ext              = from->virt_ext;
>  	dst->pause_filter_count   = from->pause_filter_count;
> @@ -1602,7 +1618,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	nested_copy_vmcb_control_to_cache(svm, ctl);
>  
>  	svm_switch_vmcb(svm, &svm->nested.vmcb02);
> -	nested_vmcb02_prepare_control(svm);
> +	nested_vmcb02_prepare_control(svm, save->rip);
>  
>  	/*
>  	 * While the nested guest CR3 is already checked and set by
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 32220a1b0ea2..7d97e4d18c8b 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -139,6 +139,7 @@ struct vmcb_ctrl_area_cached {
>  	u64 nested_ctl;
>  	u32 event_inj;
>  	u32 event_inj_err;
> +	u64 next_rip;
>  	u64 nested_cr3;
>  	u64 virt_ext;
>  	u32 clean;

Re: [PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

[Maxim Levitsky]

On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> Re-inject INT3/INTO instead of retrying the instruction if the CPU
> encountered an intercepted exception while vectoring the software
> exception, e.g. if vectoring INT3 encounters a #PF and KVM is using
> shadow paging.  Retrying the instruction is architecturally wrong, e.g.
> will result in a spurious #DB if there's a code breakpoint on the INT3/O,
> and lack of re-injection also breaks nested virtualization, e.g. if L1
> injects a software exception and vectoring the injected exception
> encounters an exception that is intercepted by L0 but not L1.
> 
> Due to, ahem, deficiencies in the SVM architecture, acquiring the next
> RIP may require flowing through the emulator even if NRIPS is supported,
> as the CPU clears next_rip if the VM-Exit is due to an exception other
> than "exceptions caused by the INT3, INTO, and BOUND instructions".  To
> deal with this, "skip" the instruction to calculate next_rip (if it's
> not already known), and then unwind the RIP write and any side effects
> (RFLAGS updates).
> 
> Save the computed next_rip and use it to re-stuff next_rip if injection
> doesn't complete.  This allows KVM to do the right thing if next_rip was
> known prior to injection, e.g. if L1 injects a soft event into L2, and
> there is no backing INTn instruction, e.g. if L1 is injecting an
> arbitrary event.
> 
> Note, it's impossible to guarantee architectural correctness given SVM's
> architectural flaws.  E.g. if the guest executes INTn (no KVM injection),
> an exit occurs while vectoring the INTn, and the guest modifies the code
> stream while the exit is being handled, KVM will compute the incorrect
> next_rip due to "skipping" the wrong instruction.  A future enhancement
> to make this less awful would be for KVM to detect that the decoded
> instruction is not the correct INTn and drop the to-be-injected soft
> event (retrying is a lesser evil compared to shoving the wrong RIP on the
> exception stack).
> 
> Reported-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/nested.c |  28 +++++++-
>  arch/x86/kvm/svm/svm.c    | 140 +++++++++++++++++++++++++++-----------
>  arch/x86/kvm/svm/svm.h    |   6 +-
>  3 files changed, 130 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 461c5f247801..0163238aa198 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -609,6 +609,21 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>  	}
>  }
>  
> +static inline bool is_evtinj_soft(u32 evtinj)
> +{
> +	u32 type = evtinj & SVM_EVTINJ_TYPE_MASK;
> +	u8 vector = evtinj & SVM_EVTINJ_VEC_MASK;
> +
> +	if (!(evtinj & SVM_EVTINJ_VALID))
> +		return false;
> +
> +	/*
> +	 * Intentionally return false for SOFT events, SVM doesn't yet support
> +	 * re-injecting soft interrupts.
> +	 */
> +	return type == SVM_EVTINJ_TYPE_EXEPT && kvm_exception_is_soft(vector);
> +}
> +
>  static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  					  unsigned long vmcb12_rip)
>  {
> @@ -677,6 +692,16 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  	else if (boot_cpu_has(X86_FEATURE_NRIPS))
>  		vmcb02->control.next_rip    = vmcb12_rip;
>  
> +	if (is_evtinj_soft(vmcb02->control.event_inj)) {
> +		svm->soft_int_injected = true;
> +		svm->soft_int_csbase = svm->vmcb->save.cs.base;
> +		svm->soft_int_old_rip = vmcb12_rip;
> +		if (svm->nrips_enabled)
> +			svm->soft_int_next_rip = svm->nested.ctl.next_rip;
> +		else
> +			svm->soft_int_next_rip = vmcb12_rip;
> +	}
> +
>  	vmcb02->control.virt_ext            = vmcb01->control.virt_ext &
>  					      LBR_CTL_ENABLE_MASK;
>  	if (svm->lbrv_enabled)
> @@ -849,6 +874,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>  
>  out_exit_err:
>  	svm->nested.nested_run_pending = 0;
> +	svm->soft_int_injected = false;
>  
>  	svm->vmcb->control.exit_code    = SVM_EXIT_ERR;
>  	svm->vmcb->control.exit_code_hi = 0;
> @@ -1618,7 +1644,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	nested_copy_vmcb_control_to_cache(svm, ctl);
>  
>  	svm_switch_vmcb(svm, &svm->nested.vmcb02);
> -	nested_vmcb02_prepare_control(svm, save->rip);
> +	nested_vmcb02_prepare_control(svm, svm->vmcb->save.rip);

Is this change intentional?

>  
>  	/*
>  	 * While the nested guest CR3 is already checked and set by
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 14bc4e87437b..8321f9ce5e35 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -342,9 +342,11 @@ static void svm_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
>  
>  }
>  
> -static int svm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
> +static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
> +					   bool commit_side_effects)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> +	unsigned long old_rflags;
>  
>  	/*
>  	 * SEV-ES does not expose the next RIP. The RIP update is controlled by
> @@ -359,18 +361,75 @@ static int svm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	}
>  
>  	if (!svm->next_rip) {
> +		if (unlikely(!commit_side_effects))
> +			old_rflags = svm->vmcb->save.rflags;
> +
>  		if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP))
>  			return 0;
> +
> +		if (unlikely(!commit_side_effects))
> +			svm->vmcb->save.rflags = old_rflags;
>  	} else {
>  		kvm_rip_write(vcpu, svm->next_rip);
>  	}
>  
>  done:
> -	svm_set_interrupt_shadow(vcpu, 0);
> +	if (likely(commit_side_effects))
> +		svm_set_interrupt_shadow(vcpu, 0);
>  
>  	return 1;
>  }
>  
> +static int svm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
> +{
> +	return __svm_skip_emulated_instruction(vcpu, true);
> +}
> +
> +static int svm_update_soft_interrupt_rip(struct kvm_vcpu *vcpu)
> +{
> +	unsigned long rip, old_rip = kvm_rip_read(vcpu);
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	/*
> +	 * Due to architectural shortcomings, the CPU doesn't always provide
> +	 * NextRIP, e.g. if KVM intercepted an exception that occurred while
> +	 * the CPU was vectoring an INTO/INT3 in the guest.  Temporarily skip
> +	 * the instruction even if NextRIP is supported to acquire the next
> +	 * RIP so that it can be shoved into the NextRIP field, otherwise
> +	 * hardware will fail to advance guest RIP during event injection.
> +	 * Drop the exception/interrupt if emulation fails and effectively
> +	 * retry the instruction, it's the least awful option.  If NRIPS is
> +	 * in use, the skip must not commit any side effects such as clearing
> +	 * the interrupt shadow or RFLAGS.RF.
> +	 */
> +	if (!__svm_skip_emulated_instruction(vcpu, !nrips))
> +		return -EIO;
> +
> +	rip = kvm_rip_read(vcpu);
> +
> +	/*
> +	 * Save the injection information, even when using next_rip, as the
> +	 * VMCB's next_rip will be lost (cleared on VM-Exit) if the injection
> +	 * doesn't complete due to a VM-Exit occurring while the CPU is
> +	 * vectoring the event.   Decoding the instruction isn't guaranteed to
> +	 * work as there may be no backing instruction, e.g. if the event is
> +	 * being injected by L1 for L2, or if the guest is patching INT3 into
> +	 * a different instruction.
> +	 */
> +	svm->soft_int_injected = true;
> +	svm->soft_int_csbase = svm->vmcb->save.cs.base;
> +	svm->soft_int_old_rip = old_rip;
> +	svm->soft_int_next_rip = rip;
> +
> +	if (nrips)
> +		kvm_rip_write(vcpu, old_rip);
> +
> +	if (static_cpu_has(X86_FEATURE_NRIPS))
> +		svm->vmcb->control.next_rip = rip;
> +
> +	return 0;
> +}
> +
>  static void svm_queue_exception(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -380,25 +439,9 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu)
>  
>  	kvm_deliver_exception_payload(vcpu);
>  
> -	if (nr == BP_VECTOR && !nrips) {
> -		unsigned long rip, old_rip = kvm_rip_read(vcpu);
> -
> -		/*
> -		 * For guest debugging where we have to reinject #BP if some
> -		 * INT3 is guest-owned:
> -		 * Emulate nRIP by moving RIP forward. Will fail if injection
> -		 * raises a fault that is not intercepted. Still better than
> -		 * failing in all cases.
> -		 */
> -		(void)svm_skip_emulated_instruction(vcpu);
> -		rip = kvm_rip_read(vcpu);
> -
> -		if (boot_cpu_has(X86_FEATURE_NRIPS))
> -			svm->vmcb->control.next_rip = rip;
> -
> -		svm->int3_rip = rip + svm->vmcb->save.cs.base;
> -		svm->int3_injected = rip - old_rip;
> -	}
> +	if (kvm_exception_is_soft(nr) &&
> +	    svm_update_soft_interrupt_rip(vcpu))
> +		return;
>  
>  	svm->vmcb->control.event_inj = nr
>  		| SVM_EVTINJ_VALID
> @@ -3671,15 +3714,46 @@ static inline void sync_lapic_to_cr8(struct kvm_vcpu *vcpu)
>  	svm->vmcb->control.int_ctl |= cr8 & V_TPR_MASK;
>  }
>  
> +static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,
> +					int type)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	/*
> +	 * If NRIPS is enabled, KVM must snapshot the pre-VMRUN next_rip that's
> +	 * associated with the original soft exception/interrupt.  next_rip is
> +	 * cleared on all exits that can occur while vectoring an event, so KVM
> +	 * needs to manually set next_rip for re-injection.  Unlike the !nrips
> +	 * case below, this needs to be done if and only if KVM is re-injecting
> +	 * the same event, i.e. if the event is a soft exception/interrupt,
> +	 * otherwise next_rip is unused on VMRUN.
> +	 */
> +	if (nrips && type == SVM_EXITINTINFO_TYPE_EXEPT &&
> +	    kvm_exception_is_soft(vector) &&
> +	    kvm_is_linear_rip(vcpu, svm->soft_int_old_rip + svm->soft_int_csbase))
> +		svm->vmcb->control.next_rip = svm->soft_int_next_rip;
> +	/*
> +	 * If NRIPS isn't enabled, KVM must manually advance RIP prior to
> +	 * injecting the soft exception/interrupt.  That advancement needs to
> +	 * be unwound if vectoring didn't complete.  Note, the new event may
> +	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
> +	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
> +	 * be the reported vectored event, but RIP still needs to be unwound.
> +	 */
> +	else if (!nrips && type == SVM_EXITINTINFO_TYPE_EXEPT &&
> +		 kvm_is_linear_rip(vcpu, svm->soft_int_next_rip + svm->soft_int_csbase))
> +		kvm_rip_write(vcpu, svm->soft_int_old_rip);
> +}
> +
>  static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u8 vector;
>  	int type;
>  	u32 exitintinfo = svm->vmcb->control.exit_int_info;
> -	unsigned int3_injected = svm->int3_injected;
> +	bool soft_int_injected = svm->soft_int_injected;
>  
> -	svm->int3_injected = 0;
> +	svm->soft_int_injected = false;
>  
>  	/*
>  	 * If we've made progress since setting HF_IRET_MASK, we've
> @@ -3704,17 +3778,8 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>  	vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
>  	type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
>  
> -	/*
> -	 * If NextRIP isn't enabled, KVM must manually advance RIP prior to
> -	 * injecting the soft exception/interrupt.  That advancement needs to
> -	 * be unwound if vectoring didn't complete.  Note, the new event may
> -	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
> -	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
> -	 * be the reported vectored event, but RIP still needs to be unwound.
> -	 */
> -	if (int3_injected && type == SVM_EXITINTINFO_TYPE_EXEPT &&
> -	   kvm_is_linear_rip(vcpu, svm->int3_rip))
> -		kvm_rip_write(vcpu, kvm_rip_read(vcpu) - int3_injected);
> +	if (soft_int_injected)
> +		svm_complete_soft_interrupt(vcpu, vector, type);
>  
>  	switch (type) {
>  	case SVM_EXITINTINFO_TYPE_NMI:
> @@ -3727,13 +3792,6 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>  		if (vector == X86_TRAP_VC)
>  			break;
>  
> -		/*
> -		 * In case of software exceptions, do not reinject the vector,
> -		 * but re-execute the instruction instead.
> -		 */
> -		if (kvm_exception_is_soft(vector))
> -			break;
> -
>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>  			u32 err = svm->vmcb->control.exit_int_info_err;
>  			kvm_requeue_exception_e(vcpu, vector, err);
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 7d97e4d18c8b..6acb494e3598 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -230,8 +230,10 @@ struct vcpu_svm {
>  	bool nmi_singlestep;
>  	u64 nmi_singlestep_guest_rflags;
>  
> -	unsigned int3_injected;
> -	unsigned long int3_rip;
> +	unsigned long soft_int_csbase;
> +	unsigned long soft_int_old_rip;
> +	unsigned long soft_int_next_rip;
> +	bool soft_int_injected;
>  
>  	/* optional nested SVM features that are enabled for this guest  */
>  	bool nrips_enabled                : 1;


Overall looks good, but I need to give it a bit more though to be sure,
I'll wait for next version.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 07/11] KVM: x86: Trace re-injected exceptions

[Maxim Levitsky]

On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> Trace exceptions that are re-injected, not just those that KVM is
> injecting for the first time.  Debugging re-injection bugs is painful
> enough as is, not having visibility into what KVM is doing only makes
> things worse.
> 
> Delay propagating pending=>injected in the non-reinjection path so that
> the tracing can properly identify reinjected exceptions.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/trace.h | 12 ++++++++----
>  arch/x86/kvm/x86.c   | 16 +++++++++-------
>  2 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index de4762517569..d07428e660e3 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -358,25 +358,29 @@ TRACE_EVENT(kvm_inj_virq,
>   * Tracepoint for kvm interrupt injection:
>   */
>  TRACE_EVENT(kvm_inj_exception,
> -	TP_PROTO(unsigned exception, bool has_error, unsigned error_code),
> -	TP_ARGS(exception, has_error, error_code),
> +	TP_PROTO(unsigned exception, bool has_error, unsigned error_code,
> +		 bool reinjected),
> +	TP_ARGS(exception, has_error, error_code, reinjected),
>  
>  	TP_STRUCT__entry(
>  		__field(	u8,	exception	)
>  		__field(	u8,	has_error	)
>  		__field(	u32,	error_code	)
> +		__field(	bool,	reinjected	)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->exception	= exception;
>  		__entry->has_error	= has_error;
>  		__entry->error_code	= error_code;
> +		__entry->reinjected	= reinjected;
>  	),
>  
> -	TP_printk("%s (0x%x)",
> +	TP_printk("%s (0x%x)%s",
>  		  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
>  		  /* FIXME: don't print error_code if not present */
> -		  __entry->has_error ? __entry->error_code : 0)
> +		  __entry->has_error ? __entry->error_code : 0,
> +		  __entry->reinjected ? " [reinjected]" : "")
>  );
>  
>  /*
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 951d0a78ccda..c3ee8dc00d3a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9393,6 +9393,11 @@ int kvm_check_nested_events(struct kvm_vcpu *vcpu)
>  
>  static void kvm_inject_exception(struct kvm_vcpu *vcpu)
>  {
> +	trace_kvm_inj_exception(vcpu->arch.exception.nr,
> +				vcpu->arch.exception.has_error_code,
> +				vcpu->arch.exception.error_code,
> +				vcpu->arch.exception.injected);
> +
>  	if (vcpu->arch.exception.error_code && !is_protmode(vcpu))
>  		vcpu->arch.exception.error_code = false;
>  	static_call(kvm_x86_queue_exception)(vcpu);
> @@ -9450,13 +9455,6 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool *req_immediate_exit)
>  
>  	/* try to inject new event if pending */
>  	if (vcpu->arch.exception.pending) {
> -		trace_kvm_inj_exception(vcpu->arch.exception.nr,
> -					vcpu->arch.exception.has_error_code,
> -					vcpu->arch.exception.error_code);
> -
> -		vcpu->arch.exception.pending = false;
> -		vcpu->arch.exception.injected = true;
> -
>  		if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
>  			__kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
>  					     X86_EFLAGS_RF);
> @@ -9470,6 +9468,10 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool *req_immediate_exit)
>  		}
>  
>  		kvm_inject_exception(vcpu);
> +
> +		vcpu->arch.exception.pending = false;
> +		vcpu->arch.exception.injected = true;
> +
>  		can_inject = false;
>  	}
>  

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH v2 08/11] KVM: x86: Print error code in exception injection tracepoint iff valid

[Maxim Levitsky]

On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> Print the error code in the exception injection tracepoint if and only if
> the exception has an error code.  Define the entire error code sequence
> as a set of formatted strings, print empty strings if there's no error
> code, and abuse __print_symbolic() by passing it an empty array to coerce
> it into printing the error code as a hex string.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/trace.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index d07428e660e3..385436d12024 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -376,10 +376,11 @@ TRACE_EVENT(kvm_inj_exception,
>  		__entry->reinjected	= reinjected;
>  	),
>  
> -	TP_printk("%s (0x%x)%s",
> +	TP_printk("%s%s%s%s%s",
>  		  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
> -		  /* FIXME: don't print error_code if not present */
> -		  __entry->has_error ? __entry->error_code : 0,
> +		  !__entry->has_error ? "" : " (",
> +		  !__entry->has_error ? "" : __print_symbolic(__entry->error_code, { }),
> +		  !__entry->has_error ? "" : ")",
>  		  __entry->reinjected ? " [reinjected]" : "")
>  );
>  



Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Good idea to do it in few more places, I'll keep that in mind.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 02/11] KVM: SVM: Don't BUG if userspace injects a soft interrupt with GIF=0

[Maciej S. Szmigiero]

On 28.04.2022 09:35, Maxim Levitsky wrote:
> On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
>> From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>
>> Don't BUG/WARN on interrupt injection due to GIF being cleared if the
>> injected event is a soft interrupt, which are not actually IRQs and thus
> 
> Are any injected events subject to GIF set? I think that EVENTINJ just injects
> unconditionaly whatever hypervisor puts in it.

That's right, EVENTINJ will pretty much always inject, even when the CPU
is in a 'wrong' state (like for example, injecting a hardware interrupt
or a NMI with GIF masked).

But KVM as a L0 is not supposed to inject a hardware interrupt into guest
with GIF unset since the guest is obviously not expecting it then.
Hence this WARN_ON().

> Best regards,
> 	Maxim Levitsky

Thanks,
Maciej

Re: [PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

[Maciej S. Szmigiero]

On 28.04.2022 11:37, Maxim Levitsky wrote:
> On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
>> Re-inject INT3/INTO instead of retrying the instruction if the CPU
>> encountered an intercepted exception while vectoring the software
>> exception, e.g. if vectoring INT3 encounters a #PF and KVM is using
>> shadow paging.  Retrying the instruction is architecturally wrong, e.g.
>> will result in a spurious #DB if there's a code breakpoint on the INT3/O,
>> and lack of re-injection also breaks nested virtualization, e.g. if L1
>> injects a software exception and vectoring the injected exception
>> encounters an exception that is intercepted by L0 but not L1.
>>
>> Due to, ahem, deficiencies in the SVM architecture, acquiring the next
>> RIP may require flowing through the emulator even if NRIPS is supported,
>> as the CPU clears next_rip if the VM-Exit is due to an exception other
>> than "exceptions caused by the INT3, INTO, and BOUND instructions".  To
>> deal with this, "skip" the instruction to calculate next_rip (if it's
>> not already known), and then unwind the RIP write and any side effects
>> (RFLAGS updates).
>>
>> Save the computed next_rip and use it to re-stuff next_rip if injection
>> doesn't complete.  This allows KVM to do the right thing if next_rip was
>> known prior to injection, e.g. if L1 injects a soft event into L2, and
>> there is no backing INTn instruction, e.g. if L1 is injecting an
>> arbitrary event.
>>
>> Note, it's impossible to guarantee architectural correctness given SVM's
>> architectural flaws.  E.g. if the guest executes INTn (no KVM injection),
>> an exit occurs while vectoring the INTn, and the guest modifies the code
>> stream while the exit is being handled, KVM will compute the incorrect
>> next_rip due to "skipping" the wrong instruction.  A future enhancement
>> to make this less awful would be for KVM to detect that the decoded
>> instruction is not the correct INTn and drop the to-be-injected soft
>> event (retrying is a lesser evil compared to shoving the wrong RIP on the
>> exception stack).
>>
>> Reported-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> ---
>>   arch/x86/kvm/svm/nested.c |  28 +++++++-
>>   arch/x86/kvm/svm/svm.c    | 140 +++++++++++++++++++++++++++-----------
>>   arch/x86/kvm/svm/svm.h    |   6 +-
>>   3 files changed, 130 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 461c5f247801..0163238aa198 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -609,6 +609,21 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>>   	}
>>   }
>>   
>> +static inline bool is_evtinj_soft(u32 evtinj)
>> +{
>> +	u32 type = evtinj & SVM_EVTINJ_TYPE_MASK;
>> +	u8 vector = evtinj & SVM_EVTINJ_VEC_MASK;
>> +
>> +	if (!(evtinj & SVM_EVTINJ_VALID))
>> +		return false;
>> +
>> +	/*
>> +	 * Intentionally return false for SOFT events, SVM doesn't yet support
>> +	 * re-injecting soft interrupts.
>> +	 */
>> +	return type == SVM_EVTINJ_TYPE_EXEPT && kvm_exception_is_soft(vector);
>> +}
>> +
>>   static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>>   					  unsigned long vmcb12_rip)
>>   {
>> @@ -677,6 +692,16 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>>   	else if (boot_cpu_has(X86_FEATURE_NRIPS))
>>   		vmcb02->control.next_rip    = vmcb12_rip;
>>   
>> +	if (is_evtinj_soft(vmcb02->control.event_inj)) {
>> +		svm->soft_int_injected = true;
>> +		svm->soft_int_csbase = svm->vmcb->save.cs.base;
>> +		svm->soft_int_old_rip = vmcb12_rip;
>> +		if (svm->nrips_enabled)
>> +			svm->soft_int_next_rip = svm->nested.ctl.next_rip;
>> +		else
>> +			svm->soft_int_next_rip = vmcb12_rip;
>> +	}
>> +
>>   	vmcb02->control.virt_ext            = vmcb01->control.virt_ext &
>>   					      LBR_CTL_ENABLE_MASK;
>>   	if (svm->lbrv_enabled)
>> @@ -849,6 +874,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>>   
>>   out_exit_err:
>>   	svm->nested.nested_run_pending = 0;
>> +	svm->soft_int_injected = false;
>>   
>>   	svm->vmcb->control.exit_code    = SVM_EXIT_ERR;
>>   	svm->vmcb->control.exit_code_hi = 0;
>> @@ -1618,7 +1644,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>>   	nested_copy_vmcb_control_to_cache(svm, ctl);
>>   
>>   	svm_switch_vmcb(svm, &svm->nested.vmcb02);
>> -	nested_vmcb02_prepare_control(svm, save->rip);
>> +	nested_vmcb02_prepare_control(svm, svm->vmcb->save.rip);
> 
> Is this change intentional?

It looks to me the final code is correct since "svm->vmcb->save"
contains L2 register save, while "save" has L1 register save.

It was the patch 1 from this series that was incorrect in
using "save->rip" here instead.

Thanks,
Maciej

Re: [PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

[Sean Christopherson]

On Thu, Apr 28, 2022, Maciej S. Szmigiero wrote:
> On 28.04.2022 11:37, Maxim Levitsky wrote:
> > On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> > > @@ -1618,7 +1644,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> > >   	nested_copy_vmcb_control_to_cache(svm, ctl);
> > >   	svm_switch_vmcb(svm, &svm->nested.vmcb02);
> > > -	nested_vmcb02_prepare_control(svm, save->rip);
> > > +	nested_vmcb02_prepare_control(svm, svm->vmcb->save.rip);
> > 
> > Is this change intentional?
> 
> It looks to me the final code is correct since "svm->vmcb->save"
> contains L2 register save, while "save" has L1 register save.
> 
> It was the patch 1 from this series that was incorrect in
> using "save->rip" here instead.

Yeah, I botched the fixup.

Re: [PATCH v2 02/11] KVM: SVM: Don't BUG if userspace injects a soft interrupt with GIF=0

[Maxim Levitsky]

On Thu, 2022-04-28 at 15:27 +0200, Maciej S. Szmigiero wrote:
> On 28.04.2022 09:35, Maxim Levitsky wrote:
> > On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> > > From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > 
> > > Don't BUG/WARN on interrupt injection due to GIF being cleared if the
> > > injected event is a soft interrupt, which are not actually IRQs and thus
> > 
> > Are any injected events subject to GIF set? I think that EVENTINJ just injects
> > unconditionaly whatever hypervisor puts in it.
> 
> That's right, EVENTINJ will pretty much always inject, even when the CPU
> is in a 'wrong' state (like for example, injecting a hardware interrupt
> or a NMI with GIF masked).
> 
> But KVM as a L0 is not supposed to inject a hardware interrupt into guest
> with GIF unset since the guest is obviously not expecting it then.
> Hence this WARN_ON().

If you mean L0->L1 injection, that sure, but if L1 injects interrupt to L2,
then it should always be allowed to do so.


I am not sure that I am right here, just noticed something odd. I'll take
a better look at this next week.


Best regards,
	Maxim levitsky

> 
> > Best regards,
> > 	Maxim Levitsky
> 
> Thanks,
> Maciej
> 

Re: [PATCH v2 02/11] KVM: SVM: Don't BUG if userspace injects a soft interrupt with GIF=0

[Sean Christopherson]

On Thu, Apr 28, 2022, Maxim Levitsky wrote:
> On Thu, 2022-04-28 at 15:27 +0200, Maciej S. Szmigiero wrote:
> > On 28.04.2022 09:35, Maxim Levitsky wrote:
> > > On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> > > > From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > > 
> > > > Don't BUG/WARN on interrupt injection due to GIF being cleared if the
> > > > injected event is a soft interrupt, which are not actually IRQs and thus
> > > 
> > > Are any injected events subject to GIF set? I think that EVENTINJ just injects
> > > unconditionaly whatever hypervisor puts in it.
> > 
> > That's right, EVENTINJ will pretty much always inject, even when the CPU
> > is in a 'wrong' state (like for example, injecting a hardware interrupt
> > or a NMI with GIF masked).
> > 
> > But KVM as a L0 is not supposed to inject a hardware interrupt into guest
> > with GIF unset since the guest is obviously not expecting it then.
> > Hence this WARN_ON().
> 
> If you mean L0->L1 injection, that sure, but if L1 injects interrupt to L2,
> then it should always be allowed to do so.

Yes, L1 can inject whatever it wants, whenever it wants.

I kept the WARN_ON() under the assumption that KVM would refuse to inject IRQs
stuffed by userspace if GIF is disabled, but looking at the code again, I have
no idea why I thought that.  KVM_SET_VCPU_EVENTS blindly takes whatever userspace
provides, I don't see anything that would prevent userspace from shoving in a
hardware IRQ.

Re: [PATCH v2 02/11] KVM: SVM: Don't BUG if userspace injects a soft interrupt with GIF=0

[Maciej S. Szmigiero]

On 28.04.2022 17:04, Sean Christopherson wrote:
> On Thu, Apr 28, 2022, Maxim Levitsky wrote:
>> On Thu, 2022-04-28 at 15:27 +0200, Maciej S. Szmigiero wrote:
>>> On 28.04.2022 09:35, Maxim Levitsky wrote:
>>>> On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
>>>>> From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>>
>>>>> Don't BUG/WARN on interrupt injection due to GIF being cleared if the
>>>>> injected event is a soft interrupt, which are not actually IRQs and thus
>>>>
>>>> Are any injected events subject to GIF set? I think that EVENTINJ just injects
>>>> unconditionaly whatever hypervisor puts in it.
>>>
>>> That's right, EVENTINJ will pretty much always inject, even when the CPU
>>> is in a 'wrong' state (like for example, injecting a hardware interrupt
>>> or a NMI with GIF masked).
>>>
>>> But KVM as a L0 is not supposed to inject a hardware interrupt into guest
>>> with GIF unset since the guest is obviously not expecting it then.
>>> Hence this WARN_ON().
>>
>> If you mean L0->L1 injection, that sure, but if L1 injects interrupt to L2,
>> then it should always be allowed to do so.
> 
> Yes, L1 can inject whatever it wants, whenever it wants.
> 
> I kept the WARN_ON() under the assumption that KVM would refuse to inject IRQs
> stuffed by userspace if GIF is disabled, but looking at the code again, I have
> no idea why I thought that.  KVM_SET_VCPU_EVENTS blindly takes whatever userspace
> provides, I don't see anything that would prevent userspace from shoving in a
> hardware IRQ.

You both are right, while KVM itself would not inject IRQ with GIF masked and
a nested VMRUN would enable GIF unconditionally, userspace via KVM_SET_VCPU_EVENTS
does not have this restriction.

Will remove this WARN_ON() then.

Thanks,
Maciej