Re: [PATCH v2 02/17] x86/virt/tdx: Configure add-on features on TDX module init and update

[Xu Yilun]

> There's also zero stopping us from putting version in args:
> 
> 	struct tdx_module_args args = {};
>   	int ret;
> 
> 	if (tdx_addon_feature0) {
> 		args.r9 = tdx_addon_feature0;
> 		args.version = 1;
> 	}
> 
> 	ret = seamcall_prerr(TDH_SYS_UPDATE, &args);
> 
> Eh?

I'm thinking the version is only needed for 3 SEAMCALLs. We don't have
to make version common for all 100+ SEAMCALLs. Besides the layout of
"struct tdx_module_args" correlates the fundamental assembly code of
__seamcall() in tdcall.S.

Could we make dedicated SEAMCALL wrappers for TDH_SYS_UPDATE similar to
other SEAMCALLs and wrapper the specific version handling there? I put
the diff in the end.

> 
> That gives args.version==0 in all the normal cases which just happens to
> be the exact behavior we want. It also avoids having to plumb version
> through all the seamcall*() wrappers.
> 
> But this is *exactly* the kind of thing that shouldn't be a part of an
> attestation patch series. This could very much have been a separate
> discussion and happened a month or a year ago. But now it is blocking
> this DICE thing from getting done <grumble>.

Sorry, I was thinking "don't keep version" was the conclusion...

--------8<--------

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 01fb01313077..b3b3540e431a 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1757,18 +1757,12 @@ int tdx_module_shutdown(void)

 int tdx_module_run_update(void)
 {
-       u64 seamcall_fn = TDH_SYS_UPDATE_V0;
-       struct tdx_module_args args = {};
+       u64 err;
        int ret;

-       if (tdx_addon_feature0) {
-               args.r9 = tdx_addon_feature0;
-               seamcall_fn = TDH_SYS_UPDATE;
-       }
-
-       ret = seamcall_prerr(seamcall_fn, &args);
-       if (ret)
-               return ret;
+       err = tdx_sys_update(tdx_addon_feature0);
+       if (err)
+               return -EIO;

        ret = get_tdx_sys_info_version(&tdx_sysinfo.version);
        /*
@@ -2351,7 +2345,7 @@ u64 tdh_vp_init(struct tdx_vp *vp, u64 initial_rcx, u32 x2apicid)
                .r8 = x2apicid,
        };

-       return seamcall(TDH_VP_INIT, &args);
+       return seamcall(SEAMCALL_LEAF_VER(TDH_VP_INIT, 1), &args);
 }
 EXPORT_SYMBOL_FOR_KVM(tdh_vp_init);

@@ -2463,3 +2457,16 @@ void tdx_sys_disable(void)
        if (ret && (ret & TDX_SW_ERROR) != TDX_SW_ERROR)
                pr_err("TDH.SYS.DISABLE failed: 0x%016llx\n", ret);
 }
+
+u64 tdx_sys_update(u64 features_enable0)
+{
+       struct tdx_module_args args = {
+               .r9 = features_enable0,
+       };
+       u64 fn = TDH_SYS_UPDATE;
+
+       if (features_enable0)
+               fn = SEAMCALL_LEAF_VER(TDH_SYS_UPDATE, 1);
+
+       return seamcall(fn, &args);
+}
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 32b13b0c85f9..f07e12552bf9 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -44,7 +44,7 @@
 #define TDH_VP_CREATE                  10
 #define TDH_MNG_KEY_FREEID             20
 #define TDH_MNG_INIT                   21
-#define TDH_VP_INIT                    SEAMCALL_LEAF_VER(22, 1)
+#define TDH_VP_INIT                    22
 #define TDH_PHYMEM_PAGE_RDMD           24
 #define TDH_VP_RD                      26
 #define TDH_PHYMEM_PAGE_RECLAIM                28
@@ -61,8 +61,7 @@
 #define TDH_SYS_SHUTDOWN               52
-#define TDH_SYS_UPDATE_V0              53
-#define TDH_SYS_UPDATE                 SEAMCALL_LEAF_VER(TDH_SYS_UPDATE_V0, 1)
+#define TDH_SYS_UPDATE                 53
 #define TDH_EXT_INIT                   60
 #define TDH_EXT_MEM_ADD                        61
 #define TDH_SYS_DISABLE                        69