Re: [RFC PATCH 06/12] KVM: TDX: Return -EIO, not -EINVAL, on a KVM_BUG_ON() condition

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Thu, 2025-08-28 at 12:21 -0700, Sean Christopherson wrote:
> Generally speaking, the number of KVM_BUG_ON()s is fine.  What we can do though
> is reduce the amount of boilerplate and the number of paths the propagate a SEAMCALL
> err through multiple layers, e.g. by eliminating single-use helpers (which is made
> easier by reducing boilerplate and thus lines of code).
> 
> Concretely, if we combine the KVM_BUG_ON() usage with pr_tdx_error():
> 
> #define __TDX_BUG_ON(__err, __fn_str, __kvm, __fmt, __args...)			\
> ({										\
> 	struct kvm *_kvm = (__kvm);						\
> 	bool __ret = !!(__err);							\
> 										\
> 	if (WARN_ON_ONCE(__ret && (!_kvm || !_kvm->vm_bugged))) {		\
> 		if (_kvm)							\
> 			kvm_vm_bugged(_kvm);					\
> 		pr_err_ratelimited("SEAMCALL " __fn_str " failed: 0x%llx"	\
> 				   __fmt "\n",  __err,  __args); 		\
> 	}									\
> 	unlikely(__ret);							\
> })
> 
> #define TDX_BUG_ON(__err, __fn, __kvm)				\
> 	__TDX_BUG_ON(__err, #__fn, __kvm, "%s", "")
> 
> #define TDX_BUG_ON_1(__err, __fn, __rcx, __kvm)			\
> 	__TDX_BUG_ON(__err, #__fn, __kvm, ", rcx 0x%llx", __rcx)
> 
> #define TDX_BUG_ON_2(__err, __fn, __rcx, __rdx, __kvm)		\
> 	__TDX_BUG_ON(__err, #__fn, __kvm, ", rcx 0x%llx, rdx 0x%llx", __rcx, __rdx)
> 
> #define TDX_BUG_ON_3(__err, __fn, __rcx, __rdx, __r8, __kvm)	\
> 	__TDX_BUG_ON(__err, #__fn, __kvm, ", rcx 0x%llx, rdx 0x%llx, r8 0x%llx", __rcx, __rdx, __r8)

In general sounds good. But there it's a bit strange to specify them rcx, rdx,
etc in a general helper. This is fallout from the existing chain of strange
naming:

For example tdh_mem_range_block() plucks them from those registers and calls
them ext_err1 due to their conditional meaning. Then KVM gives them some more
meaning with 'entry' and 'level_state". Then prints them out as original
register names. How about keeping the KVM names, like:

#define TDX_BUG_ON_2(__err, __fn, arg1, arg2, __kvm)		\
	__TDX_BUG_ON(__err, #__fn, __kvm, ", " #arg1 " 0x%llx, " #arg2 "
0x%llx", arg1, arg2)

so you get: entry: 0x00 level:0xF00

I *think* there is a way to make this work like var args and have a single
function, but it becomes impossible for people to read.


> 
> 
> And a macro to handle retry when kicking vCPUs out of the guest:
> 
> #define tdh_do_no_vcpus(tdh_func, kvm, args...)					\
> ({										\
> 	struct kvm_tdx *__kvm_tdx = to_kvm_tdx(kvm);				\
> 	u64 __err;								\
> 										\
> 	lockdep_assert_held_write(&kvm->mmu_lock);				\

There is a functional change in that the lock assert is not required if BUSY
avoidance can be guaranteed to not happen. I don't think it should be needed
today. I guess it's probably better to not rely on hitting rare races to catch
an issue like that.

> 										\
> 	__err = tdh_func(args);							\
> 	if (unlikely(tdx_operand_busy(__err))) {				\
> 		WRITE_ONCE(__kvm_tdx->wait_for_sept_zap, true);			\
> 		kvm_make_all_cpus_request(kvm, KVM_REQ_OUTSIDE_GUEST_MODE);	\
> 										\
> 		__err = tdh_func(args);						\
> 										\
> 		WRITE_ONCE(__kvm_tdx->wait_for_sept_zap, false);		\
> 	}									\
> 	__err;									\
> })
> 
> And do a bit of massaging, then we can end up e.g. this, which IMO is much easier
> to follow than the current form of tdx_sept_remove_private_spte(), which has
> several duplicate sanity checks and error handlers.
> 
> The tdh_do_no_vcpus() macro is a little mean, but I think it's a net positive
> as eliminates quite a lot of "noise", and thus makes it easier to focus on the
> logic.  And alternative to a trampoline macro would be to implement a guard()
> and then do a scoped_guard(), but I think that'd be just as hard to read, and
> would require almost as much boilerplate as there is today.
> 
> static void tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
> 					 enum pg_level level, u64 spte)
> {
> 	struct page *page = pfn_to_page(spte_to_pfn(spte));
> 	int tdx_level = pg_level_to_tdx_sept_level(level);
> 	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> 	gpa_t gpa = gfn_to_gpa(gfn);
> 	u64 err, entry, level_state;
> 
> 	/*
> 	 * HKID is released after all private pages have been removed, and set
> 	 * before any might be populated. Warn if zapping is attempted when
> 	 * there can't be anything populated in the private EPT.
> 	 */
> 	if (KVM_BUG_ON(!is_hkid_assigned(to_kvm_tdx(kvm)), kvm))
> 		return;
> 
> 	/* TODO: handle large pages. */
> 	if (KVM_BUG_ON(level != PG_LEVEL_4K, kvm))
> 		return;
> 
> 	err = tdh_do_no_vcpus(tdh_mem_range_block, kvm, &kvm_tdx->td, gpa,
> 			      tdx_level, &entry, &level_state);
> 	if (TDX_BUG_ON_2(err, TDH_MEM_RANGE_BLOCK, entry, level_state, kvm))
> 		return;
> 
> 	/*
> 	 * TDX requires TLB tracking before dropping private page.  Do
> 	 * it here, although it is also done later.
> 	 */
> 	tdx_track(kvm);
> 
> 	/*
> 	 * When zapping private page, write lock is held. So no race condition
> 	 * with other vcpu sept operation.
> 	 * Race with TDH.VP.ENTER due to (0-step mitigation) and Guest TDCALLs.
> 	 */
> 	err = tdh_do_no_vcpus(tdh_mem_page_remove, kvm, &kvm_tdx->td, gpa,
> 			      tdx_level, &entry, &level_state);
> 	if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_REMOVE, entry, level_state, kvm))
> 		return;
> 
> 	err = tdh_phymem_page_wbinvd_hkid((u16)kvm_tdx->hkid, page);
> 	if (TDX_BUG_ON(err, TDH_PHYMEM_PAGE_WBINVD, kvm))
> 		return;
> 
> 	tdx_clear_page(page);
> }

Seems like tasteful macro-ization to me.