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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

I was asked me 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 prevents 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: c989ff614f6b ("x86/svm: Separate STI and VMRUN instructions in 
svm_asm_do_resume()")
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
CC: Jason Andryuk <jason.andryuk@xxxxxxx>
CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
CC: Doug Covelli <doug.covelli@xxxxxxxxxxxx>
CC: Sean Christopherson <seanjc@xxxxxxxxxx>
CC: Paolo Bonzini <pbonzini@xxxxxxxxxx>
CC: Borislav Petkov <bp@xxxxxxxxx>
CC: David Kaplan <david.kaplan@xxxxxxx>
CC: David Woodhouse <dwmw2@xxxxxxxxxxxx>
CC: Shivansh Dhiman <shivansh.dhiman@xxxxxxx>
CC: Nikunj Dadhania <nikunj.dadhania@xxxxxxx>
CC: Santosh Shukla <santosh.shukla@xxxxxxx>

AMD have confirmed this is broken back to at least Zen1.  Hopefully an erratum
will be issued.

Slightly RFC.  I don't currently have a Venice system to demonstrate the FRED
part of this.  CC-ing the KVM folk and AMD FRED folk too, as someone will
presumably want to adapt this fix.

Concerning encrypted VMs, there is no fix; INTR_SHADOW is encrypted state
which the VMM can't access.
---
 xen/arch/x86/hvm/svm/entry.S      | 40 ++++++++++++++++++++++++-------
 xen/arch/x86/x86_64/asm-offsets.c |  9 +++++++
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index d9613a2a8fed..99a98050793f 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/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
diff --git a/xen/arch/x86/x86_64/asm-offsets.c 
b/xen/arch/x86/x86_64/asm-offsets.c
index baf266ab8013..4872ef89b0df 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/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) )
@@ -133,6 +137,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();

base-commit: f9b72572c8d5b6d312fd0b5014f65bed20efac40
-- 
2.39.5