On Thu, Mar 21, 2024 at 11:37:58AM +1300,
"Huang, Kai" <kai.huang@xxxxxxxxx> wrote:
>
>
> 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;
> })
As we can move out KVM_BUG_ON() to the call site, we can simply have
seamcall() or seamcall_ret().
The call site has to check error. whether it is TDX_SW_ERROR or not.
And if it hit the unexpected error, it will mark the guest bugged.
> #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?
We can eleiminate tdx_kvm_seamcall() and use seamcall() or seamcall_ret()
directly.
> 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?
I intended for bad code generation. If the loop on NO_ENTRY error harms the
code generation, we might be able to use __seamcall() or __seamcall_ret()
instead of seamcall(), seamcall_ret().
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
