Re: [PATCH v3 20/25] KVM: TDX: Add macro to retry SEAMCALLs when forcing vCPUs out of guest

["Huang, Kai" <kai.huang@intel.com>]

On Mon, 2025-10-27 at 12:20 -0700, Sean Christopherson wrote:
> On Fri, Oct 24, 2025, Kai Huang wrote:
> > On Thu, 2025-10-16 at 17:32 -0700, Sean Christopherson wrote:
> > > Add a macro to handle kicking vCPUs out of the guest and retrying
> > > SEAMCALLs on -EBUSY instead of providing small helpers to be used by each
> > > SEAMCALL.  Wrapping the SEAMCALLs in a macro makes it a little harder to
> > > tease out which SEAMCALL is being made, but significantly reduces the
> > > amount of copy+paste code and makes it all but impossible to leave an
> > > elevated wait_for_sept_zap.
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  arch/x86/kvm/vmx/tdx.c | 72 ++++++++++++++----------------------------
> > >  1 file changed, 23 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > index f6782b0ffa98..2e2dab89c98f 100644
> > > --- a/arch/x86/kvm/vmx/tdx.c
> > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > @@ -294,25 +294,24 @@ static inline void tdx_disassociate_vp(struct kvm_vcpu *vcpu)
> > >  	vcpu->cpu = -1;
> > >  }
> > >  
> > > -static void tdx_no_vcpus_enter_start(struct kvm *kvm)
> > > -{
> > > -	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > > -
> > > -	lockdep_assert_held_write(&kvm->mmu_lock);
> > > -
> > > -	WRITE_ONCE(kvm_tdx->wait_for_sept_zap, true);
> > > -
> > > -	kvm_make_all_cpus_request(kvm, KVM_REQ_OUTSIDE_GUEST_MODE);
> > > -}
> > > -
> > > -static void tdx_no_vcpus_enter_stop(struct kvm *kvm)
> > > -{
> > > -	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > > -
> > > -	lockdep_assert_held_write(&kvm->mmu_lock);
> > > -
> > > -	WRITE_ONCE(kvm_tdx->wait_for_sept_zap, false);
> > > -}
> > > +#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);				\
> > > +										\
> > > +	__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;									\
> > > +})
> > 
> > The comment which says "the second retry should succeed" is lost, could we
> > add it to tdh_do_no_vcpus()?
> 
> +1, definitely needs a comment.
> 
> /*
>  * Execute a SEAMCALL related to removing/blocking S-EPT entries, with a single
>  * retry (if necessary) after forcing vCPUs to exit and wait for the operation
>  * to complete.  All flows that remove/block S-EPT entries run with mmu_lock
>  * held for write, i.e. are mutually exlusive with each other, but they aren't
>  * mutually exclusive with vCPUs running (because that would be overkill), and
>  * so can fail with "operand busy" if a vCPU acquires a required lock in the
>  * TDX-Module.

LGTM.  Nit: is it more clear to just say they can contend with TDX guest?

   All flows that ..., but they aren't mutually exclusive with vCPUs
   running, i.e., they can also contend with TDCALL from guest and fail with
   "operand busy".

>  *
>  * Note, the retry is guaranteed to succeed, absent KVM and/or TDX-Module bugs.
>  */
>  
> > Also, perhaps we can just TDX_BUG_ON() inside tdh_do_no_vcpus() when the
> > second call of tdh_func() fails?
> 
> Heh, this also caught my eye when typing up the comment.  Unfortunately, I don't
> think it's worth doing the TDX_BUG_ON() inside the macro as that would require
> plumbing in the UPPERCASE name, and doesn't work well with the variadic arguments,
> e.g. TRACK wants TDX_BUG_ON(), but REMOVE and BLOCK want TDX_BUG_ON_2().
> 
> Given that REMOVE and BLOCK need to check the return value, getting the TDX_BUG_ON()
> call into the macro wouldn't buy that much.

Agreed.  Seems it sounds nice in concept but not in practice. :-)
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv