Kernel KVM virtualization development
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: sashiko-reviews@lists.linux.dev, kvm@vger.kernel.org
Subject: Re: [PATCH 2/2] KVM: TDX: Enable Bus Lock VM exit
Date: Fri, 7 Aug 2026 07:51:51 -0700	[thread overview]
Message-ID: <anXxBzO41_5eaaOI@google.com> (raw)
In-Reply-To: <e606195a-6adf-412f-8b0b-4b3ca7b1ad0a@intel.com>

On Thu, Aug 06, 2026, Xiaoyao Li wrote:
> On 8/5/2026 10:56 PM, Sean Christopherson wrote:
> > On Wed, Aug 05, 2026, Xiaoyao Li wrote:
> > > On 8/5/2026 11:46 AM, sashiko-bot@kernel.org wrote:
> > > > This also appears to affect tdx_to_vmx_exit_reason(), where comparing the raw
> > > > exit reason directly against 16-bit constants like EXIT_REASON_TDCALL will
> > > > fail to match if the bus lock bit is set, leading to incorrect emulation.
> > > 
> > > This is valid. We need to adjust tdx_to_vmx_exit_reason().
> > > 
> > > However, there is a more important problem. Since Bus Lock VM exit makes bit
> > > 26 possible in EXIT REASON, the trick of "return -1" in
> > > tdx_to_vmx_exit_reason() will introduce false-positive in the following
> > > check of if(vmx_get_exit_reason(vcpu).bus_lock_detected) added by this
> > > patch.
> > > 
> > > how about something like below:
> > 
> > Way too subtle.  tdx_to_vmx_exit_reason() should return the actual union, not a
> > raw u32, otherwise it's going to be extremely difficult to avoid reintroducing
> > similar bugs.
> > 
> > And looking at this all again, we should change the handling of actual
> > EXIT_REASON_EPT_MISCONFIG exits.  Stuffing a bogus value into the exit_reason
> > is "fine", but as Sashiko points out, it's extremely brittle.  Rather than
> > stuff the exit reason, we should stuff the status to signal TDX_SW_ERROR.
> > 
> > And to do that without introducing more fragility, we should flag the raw
> > vp_enter_ret as "unsafe", and explicitly track vp_enter_status.  I.e. separate
> > the status from the exit_reason immediately after VP.ENTER, instead of mixing
> > and matching the two concepts.
> > 
> > The fastpath "handler" is also all kinds of messed up.  KVM fails to trace_kvm_exit()
> > EPT misconfigs and software errors; even though the exit reason is undefined, it
> > should still be captured in the trace, otherwise it's a huge blindspot.  And AFAICT,
> > OPERAND_BUSY should be mutually exclusive with actual VM-Entry failures, so manually
> > checking for VM-Entry failure is completely unnecessary, just handle OPERAND_BUSY.
> > If TDX ever gains fastpath handlers, then we can add a true fastpath handler at
> > that time.  But OPERAND_BUSY should be a "never do the fastpath", because AIUI,
> > VM-Enter wasn't attempted, i.e. there's nothing to handle.
> > 
> > Compile tested only, and it should be chunked over several patches, but this?
> 
> Basically, it looks good except some nits.
> 
> I'll try to split into a formal sereis. Please let me know if you want to do
> if yourself.

All you.

> > -	if (unlikely(vp_enter_ret == EXIT_REASON_EPT_MISCONFIG)) {
> > -		KVM_BUG_ON(1, vcpu->kvm);
> > +	if (KVM_BUG_ON(exit_reason.basic == EXIT_REASON_EPT_MISCONFIG, vcpu->kvm))
> 
> We need to check tdx->vp_enter_ret__unsafe instead of exit_reason becase
> tdcall_to_vmx_exit_reason() translates TDVMCALL(EXIT_REASON_EPT_VIOLATION)
> from guest to EXIT_REASON_EPT_MISCONFIG

Ah shoot.  I actually handled that, but apparently I failed to refresh my
copy+paste.  Phew, I still have the local commit.  This is what I intended:

	/*
	 * Handle TDX SW errors, including TDX_SEAMCALL_UD, TDX_SEAMCALL_GP and
	 * TDX_SEAMCALL_VMFAILINVALID.
	 */
	if (unlikely((tdx->vp_enter_status & TDX_SW_ERROR) == TDX_SW_ERROR)) {
		/* Actual EPT Misconfigs are *always* KVM/kernel bugs. */
		if (KVM_BUG_ON(exit_reason.basic == EXIT_REASON_EPT_MISCONFIG, vcpu->kvm))
			return -EIO;

		KVM_BUG_ON(!virt_rebooting, vcpu->kvm);
		goto unhandled_exit;
	}

It pairs with a change in tdx_to_vmx_exit_reason(), renamed to tdx_process_vp_enter_return(),
to force the status to TDX_SW_ERROR.  That's another motivation for separately
tracking the status: KVM can clobber it without losing the original exit info.


	case EXIT_REASON_EPT_MISCONFIG:
		/*
		 * Actual EPT Misconfigs are KVM/kernel software bugs.  Set the
		 * status accordingly to differentiate from emulated MMIO exits.
		 */
		tdx->vp_enter_status = TDX_SW_ERROR;
		break;

Full diff at the bottom.

> <...>
> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index ac8323a68b16..5564617fc12a 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -66,7 +66,12 @@ struct vcpu_tdx {
> >   	struct list_head cpu_list;
> > -	u64 vp_enter_ret;
> > +	/*
> > +	 * Discourage direct use of the raw VP.ENTER return value, as there are
> > +	 * several subtleties that need to be accounted for when working with
> > +	 * the raw value.
> > +	 */
> > +	u64 HINT_UNSAFE_IN_KVM(vp_enter_ret);
> 
> So the purpose is forcing people to think twice when using it because they
> see "__unsafe"? Maybe it's more for the reviewers and maintainers.

Not just think twice, but actively make it more difficult to use the field.  E.g.
trying to copy+paste vp_enter_ret directly will fail as there is no such field.
In the unlikely scenario that people insist on bypassing the protection, I'm sure
we could come up with an even fancier HINT_UNSAFE_IN_KVM() implementation to make
it more onerous to use the raw variable, but I don't expect that to be a problem.

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index b272c20586a7..89b9e4322151 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -921,12 +921,16 @@ static __always_inline u32 tdcall_to_vmx_exit_reason(struct kvm_vcpu *vcpu)
 	return EXIT_REASON_TDCALL;
 }
 
-static __always_inline u32 tdx_to_vmx_exit_reason(struct kvm_vcpu *vcpu)
+static __always_inline union vmx_exit_reason tdx_process_vp_enter_return(struct kvm_vcpu *vcpu,
+									 u64 vp_enter_ret)
 {
 	struct vcpu_tdx *tdx = to_tdx(vcpu);
-	u32 exit_reason;
+	union vmx_exit_reason exit_reason;
 
-	switch (tdx->vp_enter_ret & TDX_SEAMCALL_STATUS_MASK) {
+	tdx->vp_enter_ret__unsafe = vp_enter_ret;
+	tdx->vp_enter_status = vp_enter_ret & TDX_SEAMCALL_STATUS_MASK;
+
+	switch (tdx->vp_enter_status) {
 	case TDX_SUCCESS:
 	case TDX_NON_RECOVERABLE_VCPU:
 	case TDX_NON_RECOVERABLE_TD:
@@ -934,23 +938,32 @@ static __always_inline u32 tdx_to_vmx_exit_reason(struct kvm_vcpu *vcpu)
 	case TDX_NON_RECOVERABLE_TD_WRONG_APIC_MODE:
 		break;
 	default:
-		return -1u;
+		/*
+		 * Synthesize an invalid bogus Exit Reason, as the TDX-Module
+		 * never attempted to run the vCPU, i.e. the Exit Reason is
+		 * undefined, but this is NOT a failed VM-Enter.
+		 */
+		return (union vmx_exit_reason) {
+			.basic = -1,
+		};
 	}
 
-	exit_reason = tdx->vp_enter_ret;
+	exit_reason.full = (u32)vp_enter_ret;
 
-	switch (exit_reason) {
+	switch (exit_reason.basic) {
 	case EXIT_REASON_TDCALL:
 		if (tdvmcall_exit_type(vcpu))
-			return EXIT_REASON_VMCALL;
-
-		return tdcall_to_vmx_exit_reason(vcpu);
+			exit_reason.basic = EXIT_REASON_VMCALL;
+		else
+			exit_reason.basic = tdcall_to_vmx_exit_reason(vcpu);
+		break;
 	case EXIT_REASON_EPT_MISCONFIG:
 		/*
-		 * Defer KVM_BUG_ON() until tdx_handle_exit() because this is in
-		 * non-instrumentable code with interrupts disabled.
+		 * Actual EPT Misconfigs are KVM/kernel software bugs.  Set the
+		 * status accordingly to differentiate from emulated MMIO exits.
 		 */
-		return -1u;
+		tdx->vp_enter_status = TDX_SW_ERROR;
+		break;
 	default:
 		break;
 	}
@@ -962,12 +975,13 @@ static noinstr void tdx_vcpu_enter_exit(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_tdx *tdx = to_tdx(vcpu);
 	struct vcpu_vt *vt = to_vt(vcpu);
+	u64 ret;
 
 	guest_state_enter_irqoff();
 
-	tdx->vp_enter_ret = tdh_vp_enter(&tdx->vp, &tdx->vp_enter_args);
+	ret = tdh_vp_enter(&tdx->vp, &tdx->vp_enter_args);
 
-	vt->exit_reason.full = tdx_to_vmx_exit_reason(vcpu);
+	vt->exit_reason = tdx_process_vp_enter_return(vcpu, ret);
 
 	vt->exit_qualification = tdx->vp_enter_args.rcx;
 	tdx->ext_exit_qualification = tdx->vp_enter_args.rdx;
@@ -979,33 +993,6 @@ static noinstr void tdx_vcpu_enter_exit(struct kvm_vcpu *vcpu)
 	guest_state_exit_irqoff();
 }
 
-static bool tdx_failed_vmentry(struct kvm_vcpu *vcpu)
-{
-	return vmx_get_exit_reason(vcpu).failed_vmentry &&
-	       vmx_get_exit_reason(vcpu).full != -1u;
-}
-
-static fastpath_t tdx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
-{
-	u64 vp_enter_ret = to_tdx(vcpu)->vp_enter_ret;
-
-	/*
-	 * TDX_OPERAND_BUSY could be returned for SEPT due to 0-step mitigation
-	 * or for TD EPOCH due to contention with TDH.MEM.TRACK on TDH.VP.ENTER.
-	 *
-	 * When KVM requests KVM_REQ_OUTSIDE_GUEST_MODE, which has both
-	 * KVM_REQUEST_WAIT and KVM_REQUEST_NO_ACTION set, it requires target
-	 * vCPUs leaving fastpath so that interrupt can be enabled to ensure the
-	 * IPIs can be delivered. Return EXIT_FASTPATH_EXIT_HANDLED instead of
-	 * EXIT_FASTPATH_REENTER_GUEST to exit fastpath, otherwise, the
-	 * requester may be blocked endlessly.
-	 */
-	if (unlikely(tdx_operand_busy(vp_enter_ret)))
-		return EXIT_FASTPATH_EXIT_HANDLED;
-
-	return EXIT_FASTPATH_NONE;
-}
-
 #define TDX_REGS_AVAIL_SET	(BIT(VCPU_REG_EXIT_INFO_1) | \
 				 BIT(VCPU_REG_EXIT_INFO_2) | \
 				 BIT(VCPU_REGS_RAX) | \
@@ -1093,18 +1080,23 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 
 	kvm_clear_available_registers(vcpu, ~TDX_REGS_AVAIL_SET);
 
-	if (unlikely(tdx->vp_enter_ret == EXIT_REASON_EPT_MISCONFIG))
-		return EXIT_FASTPATH_NONE;
-
-	if (unlikely((tdx->vp_enter_ret & TDX_SW_ERROR) == TDX_SW_ERROR))
-		return EXIT_FASTPATH_NONE;
-
 	trace_kvm_exit(vcpu, KVM_ISA_VMX);
 
-	if (unlikely(tdx_failed_vmentry(vcpu)))
-		return EXIT_FASTPATH_NONE;
+	/*
+	 * TDX_OPERAND_BUSY could be returned for SEPT due to 0-step mitigation
+	 * or for TD EPOCH due to contention with TDH.MEM.TRACK on TDH.VP.ENTER.
+	 *
+	 * When KVM requests KVM_REQ_OUTSIDE_GUEST_MODE, which has both
+	 * KVM_REQUEST_WAIT and KVM_REQUEST_NO_ACTION set, it requires target
+	 * vCPUs leaving fastpath so that interrupt can be enabled to ensure the
+	 * IPIs can be delivered. Return EXIT_FASTPATH_EXIT_HANDLED instead of
+	 * EXIT_FASTPATH_REENTER_GUEST to exit fastpath, otherwise, the
+	 * requester may be blocked endlessly.
+	 */
+	if (unlikely(tdx_operand_busy(tdx->vp_enter_status)))
+		return EXIT_FASTPATH_EXIT_HANDLED;
 
-	return tdx_exit_handlers_fastpath(vcpu);
+	return EXIT_FASTPATH_NONE;
 }
 
 void tdx_inject_nmi(struct kvm_vcpu *vcpu)
@@ -1300,7 +1292,7 @@ static int tdx_report_fatal_error(struct kvm_vcpu *vcpu)
 	vcpu->run->system_event.ndata = 16;
 
 	/* Dump 16 general-purpose registers to userspace in ascending order. */
-	regs[index++] = tdx->vp_enter_ret;
+	regs[index++] = tdx->vp_enter_ret__unsafe;
 	regs[index++] = tdx->vp_enter_args.rcx;
 	regs[index++] = tdx->vp_enter_args.rdx;
 	regs[index++] = tdx->vp_enter_args.rbx;
@@ -2030,48 +2022,44 @@ int tdx_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
 
 int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
 {
-	struct vcpu_tdx *tdx = to_tdx(vcpu);
-	u64 vp_enter_ret = tdx->vp_enter_ret;
 	union vmx_exit_reason exit_reason = vmx_get_exit_reason(vcpu);
+	struct vcpu_tdx *tdx = to_tdx(vcpu);
 
 	if (fastpath != EXIT_FASTPATH_NONE)
 		return 1;
 
-	if (unlikely(vp_enter_ret == EXIT_REASON_EPT_MISCONFIG)) {
-		KVM_BUG_ON(1, vcpu->kvm);
-		return -EIO;
-	}
-
 	/*
 	 * Handle TDX SW errors, including TDX_SEAMCALL_UD, TDX_SEAMCALL_GP and
 	 * TDX_SEAMCALL_VMFAILINVALID.
 	 */
-	if (unlikely((vp_enter_ret & TDX_SW_ERROR) == TDX_SW_ERROR)) {
+	if (unlikely((tdx->vp_enter_status & TDX_SW_ERROR) == TDX_SW_ERROR)) {
+		/* Actual EPT Misconfigs are *always* KVM/kernel bugs. */
+		if (KVM_BUG_ON(exit_reason.basic == EXIT_REASON_EPT_MISCONFIG, vcpu->kvm))
+			return -EIO;
+
 		KVM_BUG_ON(!virt_rebooting, vcpu->kvm);
 		goto unhandled_exit;
 	}
 
-	if (unlikely(tdx_failed_vmentry(vcpu))) {
+	if (unlikely(exit_reason.failed_vmentry)) {
 		/*
 		 * If the guest state is protected, that means off-TD debug is
 		 * not enabled, TDX_NON_RECOVERABLE must be set.
 		 */
 		WARN_ON_ONCE(vcpu->arch.guest_state_protected &&
-				!(vp_enter_ret & TDX_NON_RECOVERABLE));
+			     !(tdx->vp_enter_status & TDX_NON_RECOVERABLE));
 		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
 		vcpu->run->fail_entry.hardware_entry_failure_reason = exit_reason.full;
 		vcpu->run->fail_entry.cpu = vcpu->arch.last_vmentry_cpu;
 		return 0;
 	}
 
-	if (unlikely(vp_enter_ret & (TDX_ERROR | TDX_NON_RECOVERABLE)) &&
-		exit_reason.basic != EXIT_REASON_TRIPLE_FAULT) {
-		kvm_pr_unimpl("TD vp_enter_ret 0x%llx\n", vp_enter_ret);
+	if (unlikely(tdx->vp_enter_status & (TDX_ERROR | TDX_NON_RECOVERABLE)) &&
+	    exit_reason.basic != EXIT_REASON_TRIPLE_FAULT)
 		goto unhandled_exit;
-	}
 
-	WARN_ON_ONCE(exit_reason.basic != EXIT_REASON_TRIPLE_FAULT &&
-		     (vp_enter_ret & TDX_SEAMCALL_STATUS_MASK) != TDX_SUCCESS);
+	WARN_ON_ONCE(tdx->vp_enter_status != TDX_SUCCESS &&
+		     exit_reason.basic != EXIT_REASON_TRIPLE_FAULT);
 
 	switch (exit_reason.basic) {
 	case EXIT_REASON_TRIPLE_FAULT:
@@ -2131,7 +2119,8 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
 	}
 
 unhandled_exit:
-	kvm_prepare_unexpected_reason_exit(vcpu, vp_enter_ret);
+	kvm_pr_unimpl("TD vp_enter_ret 0x%llx\n", tdx->vp_enter_ret__unsafe);
+	kvm_prepare_unexpected_reason_exit(vcpu, tdx->vp_enter_ret__unsafe);
 	return 0;
 }
 
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
index ac8323a68b16..472fe9a3e4ec 100644
--- a/arch/x86/kvm/vmx/tdx.h
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -66,7 +66,13 @@ struct vcpu_tdx {
 
 	struct list_head cpu_list;
 
-	u64 vp_enter_ret;
+	/*
+	 * Discourage use of the raw VP.ENTER return value, it should only be
+	 * used to report errors to userspace, as there are several subtleties
+	 * that need to be accounted for when working with the raw value.
+	 */
+	u64 HINT_UNSAFE_IN_KVM(vp_enter_ret);
+	u64 vp_enter_status;
 
 	enum vcpu_tdx_state state;
 

      reply	other threads:[~2026-08-07 14:51 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-05  3:12 [PATCH 0/2] KVM: TDX: Enable VM-DoS Prevention Features for TDX Xiaoyao Li
2026-08-05  3:12 ` [PATCH 1/2] KVM: TDX: Enable Notify VM exit Xiaoyao Li
2026-08-05  3:38   ` sashiko-bot
2026-08-05  4:16     ` Xiaoyao Li
2026-08-06 13:33   ` Nikolay Borisov
2026-08-06 13:50     ` Sean Christopherson
2026-08-07  0:27       ` Edgecombe, Rick P
     [not found]         ` <anUngktlwsNI6oUM@google.com>
2026-08-07  1:07           ` Xiaoyao Li
2026-08-07  6:46           ` Nikolay Borisov
2026-08-07  1:06     ` Xiaoyao Li
2026-08-07 14:38       ` Sean Christopherson
2026-08-05  3:12 ` [PATCH 2/2] KVM: TDX: Enable Bus Lock " Xiaoyao Li
2026-08-05  3:46   ` sashiko-bot
2026-08-05  7:53     ` Xiaoyao Li
2026-08-05 14:56       ` Sean Christopherson
2026-08-06  6:10         ` Xiaoyao Li
2026-08-07 14:51           ` Sean Christopherson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=anXxBzO41_5eaaOI@google.com \
    --to=seanjc@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=xiaoyao.li@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox