Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX module

[Huang, Kai]

On 21/03/2024 10:36 am, Isaku Yamahata wrote:
> On Wed, Mar 20, 2024 at 01:03:21PM +1300,
> "Huang, Kai" <kai.huang@xxxxxxxxx> wrote:
>
> > > +static inline u64 tdx_seamcall(u64 op, struct tdx_module_args *in,
> > > +			       struct tdx_module_args *out)
> > > +{
> > > +	u64 ret;
> > > +
> > > +	if (out) {
> > > +		*out = *in;
> > > +		ret = seamcall_ret(op, out);
> > > +	} else
> > > +		ret = seamcall(op, in);
> >
> > I think it's silly to have the @out argument in this way.
> >
> > What is the main reason to still have it?
> >
> > Yeah we used to have the @out in __seamcall() assembly function.  The
> > assembly code checks the @out and skips copying registers to @out when it is
> > NULL.
> >
> > But it got removed when we tried to unify the assembly for TDCALL/TDVMCALL
> > and SEAMCALL to have a *SINGLE* assembly macro.
> >
> > https://lore.kernel.org/lkml/cover.1692096753.git.kai.huang@xxxxxxxxx/
> >
> > To me that means we should just accept the fact we will always have a valid
> > @out.
> >
> > But there might be some case that you _obviously_ need the @out and I
> > missed?
>
> As I replied at [1], those four wrappers need to return values.
> The first three on error, the last one on success.
>
>    [1] https://lore.kernel.org/kvm/20240320202040.GH1994522@xxxxxxxxxxxxxxxxxxxxx/
>
>    tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
>    tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
>    tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
>    u64 tdh_vp_rd(struct vcpu_tdx *tdx, u64 field, u64 *value)
>
> We can delete out from other wrappers.

Ah, OK.  I got you don't want to invent separate wrappers for each 
seamcall() variants like:

 - tdx_seamcall(u64 fn, struct tdx_module_args *args);
 - tdx_seamcall_ret(u64 fn, struct tdx_module_args *args);
 - tdx_seamcall_saved_ret(u64 fn, struct tdx_module_args *args);

To be honest I found they were kinda annoying myself during the "unify 
TDCALL/SEAMCALL and TDVMCALL assembly" patchset.

But life is hard...

And given (it seems) we are going to remove kvm_spurious_fault(), I 
think the tdx_seamcall() variants are just very simple wrapper of plain 
seamcall() variants.

So how about we have some macros:

static inline bool is_seamcall_err_kernel_defined(u64 err)
{
	return err & TDX_SW_ERROR;
}

#define TDX_KVM_SEAMCALL(_kvm, _seamcall_func, _fn, _args)	\
	({				\
		u64 _ret = _seamcall_func(_fn, _args);
		KVM_BUG_ON(_kvm, is_seamcall_err_kernel_defined(_ret));
		_ret;
	})

#define tdx_kvm_seamcall(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall, _fn, _args)

#define tdx_kvm_seamcall_ret(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall_ret, _fn, _args)

#define tdx_kvm_seamcall_saved_ret(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall_saved_ret, _fn, _args)

This is consistent with what we have in TDX host code, and this handles 
NO_ENTROPY error internally.

Or, maybe we can just use the seamcall_ret() for ALL SEAMCALLs, except 
using seamcall_saved_ret() for TDH.VP.ENTER.

u64 tdx_kvm_seamcall(sruct kvm*kvm, u64 fn,
			struct tdx_module_args *args)
{
	u64 ret = seamcall_ret(fn, args);

	KVM_BUG_ON(kvm, is_seamcall_err_kernel_defined(ret);

	return ret;
}

IIUC this at least should give us a single tdx_kvm_seamcall() API for 
majority (99%) code sites?

And obviously I'd like other people to weigh in too.

> Because only TDH.MNG.CREATE() and TDH.MNG.ADDCX() can return TDX_RND_NO_ENTROPY, > we can use __seamcall().  The TDX spec doesn't guarantee such error code
> convention.  It's very unlikely, though.

I don't quite follow the "convention" part.  Can you elaborate?

NO_ENTROPY is already handled in seamcall() variants.  Can we just use 
them directly?

> > > +static inline u64 tdh_sys_lp_shutdown(void)
> > > +{
> > > +	struct tdx_module_args in = {
> > > +	};
> > > +
> > > +	return tdx_seamcall(TDH_SYS_LP_SHUTDOWN, &in, NULL);
> > > +}
> >
> > As Sean already pointed out, I am sure it's/should not used in this series.
> >
> > That being said, I found it's not easy to determine whether one wrapper will
> > be used by this series or not.  The other option is we introduce the
> > wrapper(s) when they get actally used, but I can see (especially at this
> > stage) it's also a apple vs orange question that people may have different
> > preference.
> >
> > Perhaps we can say something like below in changelog ...
> >
> > "
> > Note, not all VM-managing related SEAMCALLs have a wrapper here, but only
> > provide wrappers that are essential to the run the TDX guest with basic
> > feature set.
> > "
> >
> > ... so that people will at least to pay attention to this during the review?
>
> Makes sense. We can split this patch into other patches that first use the
> wrappers.

Obviously I didn't want to make you do dramatic patchset reorganization, 
so it's up to you.