x86/svm: Work around VMRUN's corruption of VMCB.INTR_SHADOW

I was asked to explain the comment beside STI in the svm_asm_do_resume() and,
in trying to do so, things unravelled.

The original fix inherited from KVM prevented the guests INTR_SHADOW being
changed from 0 to 1.  This avoid undue delay to interrupts, but it caused the
value to get changed from 1 to 0 if the guest was in an INTR_SHADOW.

Whether this matters for IDT guests is unknown; it depends on what else VMRUN
can do beside event injection and still write back the wrong value.  This does
matter for FRED guests, as INTR_SHADOW is exposed architecturally in the FRED
frame via the STB field.

Fixes: c989ff614f ("x86/svm: Separate STI and VMRUN instructions in svm_asm_do_resume()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

xen/arch/x86/hvm/svm/entry.S

@@ -57,6 +57,37 @@ __UNLIKELY_END(nsvm_hap)
 
         clgi
 
+        /*
+         * Interrupts need enabling prior to VMRUN.  Anywhere after CLGI is
+         * safe.
+         *
+         * On all Zen CPUs to date, VMRUN has a bug and mixes up the host and
+         * guest INTR_SHADOW state.  If there is a VMExit during VMRUN
+         * (e.g. taking NPT_FAULT during event injection), then the host's
+         * INTR_SHADOW gets written back into the VMCB, corrupting guest
+         * state.
+         *
+         * This is believed to go unnoticed for pre-FRED guests (injecting
+         * events cancels INTR_SHADOW), but FRED changes this behaviour to be
+         * able to observe and restore the shadow via the STB field in a FRED
+         * frame.
+         *
+         * In order to fix, arrange for Xen's INTR_SHADOW at the point of
+         * VMRUN to match the guest's INTR_SHADOW.  This way, if a VMExit
+         * occurs, the VMCB is corrupted with the "correct" value.
+         *
+         * Commonly the guest will not be in an INTR_SHADOW.  In this case,
+         * execute STI early to get Xen's INTR_SHADOW over and done with; the
+         * second STI does nothing as INTR_SHADOW only occurs when IF changes
+         * from 0 to 1.  If the guest is in an INTR_SHADOW, skip the early STI
+         * so the late STI does trigger INTR_SHADOW and cover VMRUN.
+         */
+        mov     VCPU_svm_vmcb(%rbx), %rax
+        testb   $1, VMCB_int_stat(%rax)
+        jnz     1f
+        sti
+1:
+
         /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
         /* SPEC_CTRL_EXIT_TO_SVM       Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
         .macro svm_vmentry_spec_ctrl
@@ -74,19 +105,12 @@ __UNLIKELY_END(nsvm_hap)
         ALTERNATIVE "", svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM
         ALTERNATIVE "", DO_SPEC_CTRL_DIV, X86_FEATURE_SC_DIV
 
-        /*
-         * Set EFLAGS.IF after CLGI covers us from real interrupts, but not
-         * immediately prior to VMRUN.  The VMRUN instruction leaks it's
-         * INTR_SHADOW into guest state if a VMExit occurs before VMRUN
-         * completes (e.g. taking #NPF during event injecting.)
-         */
-        sti
-
         mov  VCPU_svm_vmcb_pa(%rbx),%rax
         POP_GPRS skip_rax=1 /* %rax restored by VMRUN. */
 
         SPEC_CTRL_COND_VERW     /* Req: %rsp=eframe                    Clob: efl */
 
+        sti
         vmrun
 
         PUSH_AND_CLEAR_GPRS

xen/arch/x86/x86_64/asm-offsets.c

@@ -21,6 +21,10 @@
 # include "../boot/video.h"
 #endif
 
+#ifdef CONFIG_AMD_SVM
+# include "../hvm/svm/vmcb.h"
+#endif
+
 #define DEFINE(_sym, _val)                                                 \
     asm volatile ( "\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\""\
                    :: "i" (_val) )
@@ -134,6 +138,11 @@ void __dummy__(void)
     BLANK();
 #endif
 
+#ifdef CONFIG_AMD_SVM
+    OFFSET(VMCB_int_stat, struct vmcb_struct, int_stat);
+    BLANK();
+#endif
+
 #ifdef CONFIG_PV32
     OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
     BLANK();