xen/arch/x86/hvm/vmx/entry.S | 43 +++++++++++++-------- xen/arch/x86/hvm/vmx/vmcs.c | 51 +++++++++++++++++++++---- xen/arch/x86/include/asm/current.h | 3 ++ xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 1 + xen/arch/x86/x86_64/asm-offsets.c | 1 + 5 files changed, 76 insertions(+), 23 deletions(-)
The VMEntry failure handling does not distinguish VMFail Valid vs Invalid. In
the latter case, vmx_vmentry_failure() will hit a BUG() when trying to look up
VM_INSTRUCTION_ERROR.
Preserving CF around PUSH_AND_CLEAR_GPRS is rather complicated. Borrow a
spare byte in the cpu_info block.
Move all the failure logic into .text.cold to keep it out of the fastpath
cachelines.
In vmx_vmentry_failure(), only collect the instruction error in the case of
VMFailValid. As well as printing the instruction error number decode them to
human readable names, except those which pertain to STM (a.k.a. SMI
dual-monitor or executive-VMCS-pointer), which is a mode Xen does not support.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Teddy Astie <teddy.astie@vates.tech>
Slightly RFC - it's fairly hard to these these. It came from code inspection
rather than encountering a VMFailInvalid case in practice.
I'm surprised that Eclair wasn't logging a violation at the absence of an
asmlinkage or SAF lable considering that vmx_vmentry_failure() has no
declaration.
---
xen/arch/x86/hvm/vmx/entry.S | 43 +++++++++++++--------
xen/arch/x86/hvm/vmx/vmcs.c | 51 +++++++++++++++++++++----
xen/arch/x86/include/asm/current.h | 3 ++
xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 1 +
xen/arch/x86/x86_64/asm-offsets.c | 1 +
5 files changed, 76 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index cebc70064048..24789341a082 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -154,21 +154,7 @@ UNLIKELY_END(realmode)
.Lvmx_launch:
vmlaunch
-
-.Lvmx_vmentry_fail:
- sti
- PUSH_AND_CLEAR_GPRS
-
- /*
- * SPEC_CTRL_ENTRY notes
- *
- * If we end up here, no guest code has executed. The MSR lists have
- * not been processed, so we still have Xen's choice of MSR_SPEC_CTRL
- * in context, and the RSB is unchanged.
- */
-
- call vmx_vmentry_failure
- jmp .Lvmx_process_softirqs
+ jmp .Lvmx_vmentry_fail
LABEL(vmx_asm_do_vmentry)
GET_CURRENT(bx)
@@ -189,3 +175,30 @@ LABEL(vmx_asm_do_vmentry)
call do_softirq
jmp .Lvmx_do_vmentry
END(vmx_asm_vmexit_handler)
+
+ .section .text.cold, "ax", @progbits
+
+FUNC(vmx_asm_vmexit_handler.cold)
+.Lvmx_vmentry_fail:
+ /*
+ * SPEC_CTRL_ENTRY notes
+ *
+ * If we end up here, no guest code has executed. The MSR lists have
+ * not been processed, so we still have Xen's choice of MSR_SPEC_CTRL
+ * in context, and the RSB is unchanged.
+ *
+ * The guest registers are live, and the on-stack copy is up-to-date.
+ * PUSH_AND_CLEAR_GPRS clobbers flags and can't reasonably be made not
+ * to. The Carry flag (VMFail Invalid vs Valid) needs preserving.
+ *
+ * We could opencode PUSH_AND_CLEAR_GPRS but that's fragile to stack
+ * layout changes. Instead, use a spare byte in the cpuinfo block.
+ */
+ setnc STK_REL(CPUINFO_vmx_vmfail_valid, CPUINFO_error_code)(%rsp)
+
+ PUSH_AND_CLEAR_GPRS
+ sti
+
+ call vmx_vmentry_failure
+ jmp .Lvmx_process_softirqs
+END(vmx_asm_vmexit_handler.cold)
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 8e52ef4d497a..8ff8d2426e94 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1833,18 +1833,53 @@ void vmx_destroy_vmcs(struct vcpu *v)
free_xenheap_page(v->arch.hvm.vmx.msr_bitmap);
}
-void vmx_vmentry_failure(void)
+static const char *vmx_error_str(unsigned int error)
+{
+ switch ( error )
+ {
+ case VMX_INSN_VMLAUNCH_NONCLEAR_VMCS:
+ return "VMLAUNCH with non-clear VMCS";
+
+ case VMX_INSN_VMRESUME_NONLAUNCHED_VMCS:
+ return "VMRESUME with non-launched VMCS";
+
+ case VMX_INSN_VMRESUME_AFTER_VMXOFF:
+ return "VMRESUME after VMXOFF";
+
+ case VMX_INSN_INVALID_CONTROL_STATE:
+ return "Invalid control state";
+
+ case VMX_INSN_INVALID_HOST_STATE:
+ return "Invalid host state";
+
+ case VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS:
+ return "Blocked by MOV-SS";
+
+ default:
+ return "Unknown";
+ }
+}
+
+void asmlinkage __cold vmx_vmentry_failure(void)
{
struct vcpu *curr = current;
- unsigned long error;
+ bool valid = get_cpu_info()->vmx_vmfail_valid;
- __vmread(VM_INSTRUCTION_ERROR, &error);
- gprintk(XENLOG_ERR, "VM%s error: %#lx\n",
- curr->arch.hvm.vmx.launched ? "RESUME" : "LAUNCH", error);
+ gprintk(XENLOG_ERR, "VM%s Failure, VMCS %svalid\n",
+ curr->arch.hvm.vmx.launched ? "RESUME" : "LAUNCH",
+ valid ? "" : "not ");
- if ( error == VMX_INSN_INVALID_CONTROL_STATE ||
- error == VMX_INSN_INVALID_HOST_STATE )
- vmcs_dump_vcpu(curr);
+ if ( valid )
+ {
+ unsigned int error = vmread(VM_INSTRUCTION_ERROR);
+
+ gprintk(XENLOG_ERR, " Instruction Error %u, %s\n",
+ error, vmx_error_str(error));
+
+ if ( error == VMX_INSN_INVALID_CONTROL_STATE ||
+ error == VMX_INSN_INVALID_HOST_STATE )
+ vmcs_dump_vcpu(curr);
+ }
domain_crash(curr->domain);
}
diff --git a/xen/arch/x86/include/asm/current.h b/xen/arch/x86/include/asm/current.h
index 6139980ab115..3c53235d341a 100644
--- a/xen/arch/x86/include/asm/current.h
+++ b/xen/arch/x86/include/asm/current.h
@@ -80,6 +80,9 @@ struct cpu_info {
*/
bool use_pv_cr3;
+ /* Scratch space for the VT-x logic. See users. */
+ uint8_t vmx_vmfail_valid;
+
/* get_stack_bottom() must be 16-byte aligned */
};
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index 88bded5190c9..3b1c54fd055e 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -619,6 +619,7 @@ enum vmx_insn_errno
VMX_INSN_VMCLEAR_WITH_VMXON_PTR = 3,
VMX_INSN_VMLAUNCH_NONCLEAR_VMCS = 4,
VMX_INSN_VMRESUME_NONLAUNCHED_VMCS = 5,
+ VMX_INSN_VMRESUME_AFTER_VMXOFF = 6,
VMX_INSN_INVALID_CONTROL_STATE = 7,
VMX_INSN_INVALID_HOST_STATE = 8,
VMX_INSN_VMPTRLD_INVALID_PHYADDR = 9,
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 9d4536402661..65b2b96fd528 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -163,6 +163,7 @@ void __dummy__(void)
OFFSET(CPUINFO_scf, struct cpu_info, scf);
OFFSET(CPUINFO_root_pgt_changed, struct cpu_info, root_pgt_changed);
OFFSET(CPUINFO_use_pv_cr3, struct cpu_info, use_pv_cr3);
+ OFFSET(CPUINFO_vmx_vmfail_valid, struct cpu_info, vmx_vmfail_valid);
DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
BLANK();
base-commit: c4bf5bc5f0edbcbc5965c924db069483b2cf6049
--
2.39.5
On 07.07.2026 21:35, Andrew Cooper wrote:
> The VMEntry failure handling does not distinguish VMFail Valid vs Invalid. In
> the latter case, vmx_vmentry_failure() will hit a BUG() when trying to look up
> VM_INSTRUCTION_ERROR.
That's the BUG() in __vmread(), I expect? I suppose this is also connected to ...
> Slightly RFC - it's fairly hard to these these. It came from code inspection
> rather than encountering a VMFailInvalid case in practice.
... this. (Presumably the first "these" also was meant to be "observe" or "hit"
or some such.) What I'm having difficulty with is seeing how we would make it
as far: If said __vmread() hit its BUG(), wouldn't all earlier VMCS accesses
(reads or writes) have hit that, too? I.e. would this be guarding us merely
against a pretty narrow window where memory corruption might occur? And
wouldn't that then be as little or as much of a reason for crashing as any
other failed VM{READ,WRITE}?
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -154,21 +154,7 @@ UNLIKELY_END(realmode)
>
> .Lvmx_launch:
> vmlaunch
> -
> -.Lvmx_vmentry_fail:
> - sti
> - PUSH_AND_CLEAR_GPRS
> -
> - /*
> - * SPEC_CTRL_ENTRY notes
> - *
> - * If we end up here, no guest code has executed. The MSR lists have
> - * not been processed, so we still have Xen's choice of MSR_SPEC_CTRL
> - * in context, and the RSB is unchanged.
> - */
> -
> - call vmx_vmentry_failure
> - jmp .Lvmx_process_softirqs
> + jmp .Lvmx_vmentry_fail
>
> LABEL(vmx_asm_do_vmentry)
> GET_CURRENT(bx)
> @@ -189,3 +175,30 @@ LABEL(vmx_asm_do_vmentry)
> call do_softirq
> jmp .Lvmx_do_vmentry
> END(vmx_asm_vmexit_handler)
> +
> + .section .text.cold, "ax", @progbits
> +
> +FUNC(vmx_asm_vmexit_handler.cold)
Is this doing what you want when CC_SPLIT_SECTIONS=y (and assuming my patch
to extend this to assembly code would finally land)?
> +.Lvmx_vmentry_fail:
> + /*
> + * SPEC_CTRL_ENTRY notes
> + *
> + * If we end up here, no guest code has executed. The MSR lists have
> + * not been processed, so we still have Xen's choice of MSR_SPEC_CTRL
> + * in context, and the RSB is unchanged.
How do we know the MSR lists haven't been processed? VM entry can fail because
of that processing. Afaict we only know this in the "VMfail invalid" case. For
SPEC_CTRL (in the "VMfail valid" case) this means we only have Xen's choice
still in context if that's the last entry on the list.
> + * The guest registers are live, and the on-stack copy is up-to-date.
> + * PUSH_AND_CLEAR_GPRS clobbers flags and can't reasonably be made not
> + * to. The Carry flag (VMFail Invalid vs Valid) needs preserving.
Define "reasonably". What about (with my CET-SS part included)
.macro PUSH_AND_CLEAR_GPRS ssp=IS_ENABLED(CONFIG_XEN_SHSTK)
push %rdi
mov $0, %edi
push %rsi
mov %edi, %esi
push %rdx
mov %edi, %edx
push %rcx
mov %edi, %ecx
push %rax
mov %edi, %eax
.if \ssp
rdsspq %rcx
.endif
push %r8
mov %edi, %r8d
push %r9
mov %edi, %r9d
push %r10
mov %edi, %r10d
push %r11
mov %edi, %r11d
push %rbx
mov %edi, %ebx
push %rbp
#ifdef CONFIG_FRAME_POINTER
/* Indicate special exception stack frame by inverting the frame pointer. */
mov %rsp, %rbp
not %rbp
#else
mov %edi, %ebp
#endif
push %r12
mov %edi, %r12d
push %r13
mov %edi, %r13d
push %r14
mov %edi, %r14d
push %r15
mov %edi, %r15d
#ifdef CONFIG_XEN_SHSTK
push %rcx
#endif
.endm
? Whether we'd accept the slightly longer form everywhere, or whether we'd
prefer to add a parameter is TBD.
> + * We could opencode PUSH_AND_CLEAR_GPRS but that's fragile to stack
> + * layout changes. Instead, use a spare byte in the cpuinfo block.
> + */
> + setnc STK_REL(CPUINFO_vmx_vmfail_valid, CPUINFO_error_code)(%rsp)
> +
> + PUSH_AND_CLEAR_GPRS
> + sti
Any reason you have re-ordered these two? Can't STI still be the very first
insn after the label? (Of course, if - as per above - SPEC_CTRL would first
need restoring, that would likely need to be ahead of STI.)
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1833,18 +1833,53 @@ void vmx_destroy_vmcs(struct vcpu *v)
> free_xenheap_page(v->arch.hvm.vmx.msr_bitmap);
> }
>
> -void vmx_vmentry_failure(void)
> +static const char *vmx_error_str(unsigned int error)
> +{
> + switch ( error )
> + {
> + case VMX_INSN_VMLAUNCH_NONCLEAR_VMCS:
> + return "VMLAUNCH with non-clear VMCS";
> +
> + case VMX_INSN_VMRESUME_NONLAUNCHED_VMCS:
> + return "VMRESUME with non-launched VMCS";
> +
> + case VMX_INSN_VMRESUME_AFTER_VMXOFF:
> + return "VMRESUME after VMXOFF";
> +
> + case VMX_INSN_INVALID_CONTROL_STATE:
> + return "Invalid control state";
> +
> + case VMX_INSN_INVALID_HOST_STATE:
> + return "Invalid host state";
> +
> + case VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS:
> + return "Blocked by MOV-SS";
> +
> + default:
> + return "Unknown";
> + }
> +}
> +
> +void asmlinkage __cold vmx_vmentry_failure(void)
> {
> struct vcpu *curr = current;
> - unsigned long error;
> + bool valid = get_cpu_info()->vmx_vmfail_valid;
>
> - __vmread(VM_INSTRUCTION_ERROR, &error);
> - gprintk(XENLOG_ERR, "VM%s error: %#lx\n",
> - curr->arch.hvm.vmx.launched ? "RESUME" : "LAUNCH", error);
> + gprintk(XENLOG_ERR, "VM%s Failure, VMCS %svalid\n",
> + curr->arch.hvm.vmx.launched ? "RESUME" : "LAUNCH",
> + valid ? "" : "not ");
>
> - if ( error == VMX_INSN_INVALID_CONTROL_STATE ||
> - error == VMX_INSN_INVALID_HOST_STATE )
> - vmcs_dump_vcpu(curr);
> + if ( valid )
> + {
> + unsigned int error = vmread(VM_INSTRUCTION_ERROR);
> +
> + gprintk(XENLOG_ERR, " Instruction Error %u, %s\n",
> + error, vmx_error_str(error));
With this being the only call to vmx_error_str(), would that better also be
__cold (despite the compiler almost certainly inlining it, unless __cold
triggered some special inlining decisions)?
> --- a/xen/arch/x86/include/asm/current.h
> +++ b/xen/arch/x86/include/asm/current.h
> @@ -80,6 +80,9 @@ struct cpu_info {
> */
> bool use_pv_cr3;
>
> + /* Scratch space for the VT-x logic. See users. */
> + uint8_t vmx_vmfail_valid;
Any reason this isn't bool? No use of the field wants it differently afaics.
Should we - mostly for doc purposes - also wrap this in #ifdef CONFIG_VMX?
Jan
© 2016 - 2026 Red Hat, Inc.