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.
