[PATCH] x86/svm: Fix the IRET instruction completion tracking

[Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>]

On the AMD platform without the vNMI support, the current implementation of the
NMI completion tracking, via the IRET instruction interception, entails
functional correctness corner cases. The IRET interception causes VMExit as the
first step before its execution. A corner case can happen where the IRET
instruction faults during its execution and before it retires. In such
a scenario, if a VMExit occurs in between, due to NPF exit, for example, the
Xen hypervisor will incorrectly inject a pending NMI. Additionally, the single
stepping implementation with the explicit setting of the interrupt shadow bit
and intercepting an injected virtual interrupt could entail unnecessary delays
until the RFLAGS.IF is set to 1. Ensure the IRET instruction is retired by
verifying the forward progress of the IRET's instruction pointer (RIP) before
injecting a pending NMI. Rather implement the single stepping with a debug trap.

Signed-off-by: Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>
---
 - CI tests:
https://gitlab.com/xen-project/people/aabdelsa/xen/-/pipelines/2407770975
---
 xen/arch/x86/hvm/svm/intr.c              | 17 +++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c               | 17 +++++++++--------
 xen/arch/x86/include/asm/hvm/svm-types.h |  3 +++
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 6453a46b85..c36d89e08b 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -112,6 +112,17 @@ static void svm_enable_intr_window(struct vcpu *v, struct 
hvm_intack intack)
          (general1_intercepts & GENERAL1_INTERCEPT_IRET) )
         return;
 
+    /*
+     * If there is a pending NMI but NMIs are blocked due to either the 
currently
+     * executing IRET is not yet retired or there is an interrupt shadow. 
Single
+     * step over the possible blocker.
+     */
+    if ( intack.source == hvm_intsrc_nmi )
+    {
+        vmcb->rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
+        return;
+    }
+
     intr = vmcb_get_vintr(vmcb);
     intr.fields.irq     = 1;
     intr.fields.vector  = 0;
@@ -136,6 +147,12 @@ void asmlinkage svm_intr_assist(void)
     /* Crank the handle on interrupt state. */
     pt_update_irq(v);
 
+    /*
+     * If there is an IRET instruction in-flight, ensure it is retired and
+     * it is accordingly safe to inject a pending NMI.
+     */
+    v->arch.hvm.svm.iret_mask &= ( guest_cpu_user_regs()->rip == 
v->arch.hvm.svm.nmi_iret_rip );
+
     do {
         intack = hvm_vcpu_has_pending_irq(v);
         if ( likely(intack.source == hvm_intsrc_none) )
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 243c41fb13..65da9ab777 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -548,7 +548,8 @@ static unsigned cf_check int 
svm_get_interrupt_shadow(struct vcpu *v)
     if ( vmcb->int_stat.intr_shadow )
         intr_shadow |= HVM_INTR_SHADOW_MOV_SS | HVM_INTR_SHADOW_STI;
 
-    if ( vmcb_get_general1_intercepts(vmcb) & GENERAL1_INTERCEPT_IRET )
+    if ( (vmcb_get_general1_intercepts(vmcb) & GENERAL1_INTERCEPT_IRET) ||
+         v->arch.hvm.svm.iret_mask )
         intr_shadow |= HVM_INTR_SHADOW_NMI;
 
     return intr_shadow;
@@ -3063,15 +3064,15 @@ void asmlinkage svm_vmexit_handler(void)
         u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
 
         /*
-         * IRET clears the NMI mask. However because we clear the mask
-         * /before/ executing IRET, we set the interrupt shadow to prevent
-         * a pending NMI from being injected immediately. This will work
-         * perfectly unless the IRET instruction faults: in that case we
-         * may inject an NMI before the NMI handler's IRET instruction is
-         * retired.
+         * IRET clears the NMI mask. However, The IRET instruction in the
+         * guest is not retired yet. We should ensure the IRET instruction
+         * retired / completed  before injecting another pending NMI, if
+         * there is a pending NMI. Store the current IP, so we can later
+         * verify, if we had any progress.
          */
         general1_intercepts &= ~GENERAL1_INTERCEPT_IRET;
-        vmcb->int_stat.intr_shadow = 1;
+        v->arch.hvm.svm.iret_mask = true;
+        v->arch.hvm.svm.nmi_iret_rip = guest_cpu_user_regs()->rip;
 
         vmcb_set_general1_intercepts(vmcb, general1_intercepts);
         break;
diff --git a/xen/arch/x86/include/asm/hvm/svm-types.h 
b/xen/arch/x86/include/asm/hvm/svm-types.h
index 051b235d8f..d2fbab6940 100644
--- a/xen/arch/x86/include/asm/hvm/svm-types.h
+++ b/xen/arch/x86/include/asm/hvm/svm-types.h
@@ -23,6 +23,9 @@ struct svm_vcpu {
     uint64_t vmcb_pa;
     int     launch_core;
 
+    bool iret_mask;
+    unsigned long nmi_iret_rip;
+
     uint8_t vmcb_sync_state; /* enum vmcb_sync_state */
 
     /* VMCB has a cached instruction from #PF/#NPF Decode Assist? */
-- 
2.53.0