From df8d101a3854725fd3db3c52f074808e6b3cc539 Mon Sep 17 00:00:00 2001 From: Andrew Cooper Date: Mon, 31 Aug 2026 13:48:51 +0100 Subject: [PATCH] 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: c989ff614f6b ("x86/svm: Separate STI and VMRUN instructions in svm_asm_do_resume()") Signed-off-by: Andrew Cooper Acked-by: Jan Beulich --- 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 d9613a2a8f..99a9805079 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 0549fd5e8d..3bc0f06c2e 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) ) @@ -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();