Messages in this thread | | | Date | Thu, 21 Mar 2024 11:37:58 +1300 | Subject | Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX module | From | "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@intel.com> 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@intel.com/ >> >> 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@ls.amr.corp.intel.com/ > > 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.
| |