diff mbox series

[v15,05/23] x86/virt/tdx: Handle SEAMCALL no entropy error in common code

Message ID 9565b2ccc347752607039e036fd8d19d78401b53.1699527082.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series TDX host kernel support | expand

Commit Message

Huang, Kai Nov. 9, 2023, 11:55 a.m. UTC 
Some SEAMCALLs use the RDRAND hardware and can fail for the same reasons
as RDRAND.  Use the kernel RDRAND retry logic for them.

There are three __seamcall*() variants.  Do the SEAMCALL retry in common
code and add a wrapper for each of them.

Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Kirill A. Shutemov <kirll.shutemov@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

v14 -> v15:
 - Added Sathy's tag.

v13 -> v14:
 - Use real function sc_retry() instead of using macros. (Dave)
 - Added Kirill's tag.

v12 -> v13:
 - New implementation due to TDCALL assembly series.

---
 arch/x86/include/asm/tdx.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Dave Hansen Nov. 9, 2023, 4:38 p.m. UTC | #1 
On 11/9/23 03:55, Kai Huang wrote:
> Some SEAMCALLs use the RDRAND hardware and can fail for the same reasons
> as RDRAND.  Use the kernel RDRAND retry logic for them.
> 
> There are three __seamcall*() variants.  Do the SEAMCALL retry in common
> code and add a wrapper for each of them.

The new common wrapper looks great, thanks:

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Isaku Yamahata Nov. 14, 2023, 7:24 p.m. UTC | #2 
On Fri, Nov 10, 2023 at 12:55:42AM +1300,
Kai Huang <kai.huang@intel.com> wrote:

> Some SEAMCALLs use the RDRAND hardware and can fail for the same reasons
> as RDRAND.  Use the kernel RDRAND retry logic for them.
> 
> There are three __seamcall*() variants.  Do the SEAMCALL retry in common
> code and add a wrapper for each of them.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Kirill A. Shutemov <kirll.shutemov@linux.intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> 
> v14 -> v15:
>  - Added Sathy's tag.
> 
> v13 -> v14:
>  - Use real function sc_retry() instead of using macros. (Dave)
>  - Added Kirill's tag.
> 
> v12 -> v13:
>  - New implementation due to TDCALL assembly series.
> 
> ---
>  arch/x86/include/asm/tdx.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index ea9a0320b1f8..f1c0c15469f8 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -24,6 +24,11 @@
>  #define TDX_SEAMCALL_GP			(TDX_SW_ERROR | X86_TRAP_GP)
>  #define TDX_SEAMCALL_UD			(TDX_SW_ERROR | X86_TRAP_UD)
>  
> +/*
> + * TDX module SEAMCALL leaf function error codes
> + */
> +#define TDX_RND_NO_ENTROPY	0x8000020300000000ULL
> +
>  #ifndef __ASSEMBLY__
>  
>  /*
> @@ -84,6 +89,27 @@ u64 __seamcall(u64 fn, struct tdx_module_args *args);
>  u64 __seamcall_ret(u64 fn, struct tdx_module_args *args);
>  u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);
>  
> +#include <asm/archrandom.h>
> +
> +typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args *args);
> +
> +static inline u64 sc_retry(sc_func_t func, u64 fn,
> +			   struct tdx_module_args *args)
> +{
> +	int retry = RDRAND_RETRY_LOOPS;
> +	u64 ret;
> +
> +	do {
> +		ret = func(fn, args);
> +	} while (ret == TDX_RND_NO_ENTROPY && --retry);

This loop assumes that args isn't touched when TDX_RND_NO_ENTRYPOY is returned.
It's not true.  TDH.SYS.INIT() and TDH.SYS.LP.INIT() clear RCX, RDX, etc on
error including TDX_RND_NO_ENTRY.  Because TDH.SYS.INIT() takes RCX as input,
this wrapper doesn't work.  TDH.SYS.LP.INIT() doesn't use RCX, RDX ... as
input. So it doesn't matter.

Other SEAMCALLs doesn't touch registers on the no entropy error.
TDH.EXPORTS.STATE.IMMUTABLE(), TDH.IMPORTS.STATE.IMMUTABLE(), TDH.MNG.ADDCX(),
and TDX.MNG.CREATE().  TDH.SYS.INIT() is an exception.
Huang, Kai Nov. 15, 2023, 10:41 a.m. UTC | #3 
> > +#include <asm/archrandom.h>
> > +
> > +typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args *args);
> > +
> > +static inline u64 sc_retry(sc_func_t func, u64 fn,
> > +			   struct tdx_module_args *args)
> > +{
> > +	int retry = RDRAND_RETRY_LOOPS;
> > +	u64 ret;
> > +
> > +	do {
> > +		ret = func(fn, args);
> > +	} while (ret == TDX_RND_NO_ENTROPY && --retry);
> 
> This loop assumes that args isn't touched when TDX_RND_NO_ENTRYPOY is returned.
> It's not true.  TDH.SYS.INIT() and TDH.SYS.LP.INIT() clear RCX, RDX, etc on
> error including TDX_RND_NO_ENTRY.  Because TDH.SYS.INIT() takes RCX as input,
> this wrapper doesn't work.  TDH.SYS.LP.INIT() doesn't use RCX, RDX ... as
> input. So it doesn't matter.
> 
> Other SEAMCALLs doesn't touch registers on the no entropy error.
> TDH.EXPORTS.STATE.IMMUTABLE(), TDH.IMPORTS.STATE.IMMUTABLE(), TDH.MNG.ADDCX(),
> and TDX.MNG.CREATE().  TDH.SYS.INIT() is an exception.

If I am reading the spec (TDX module 1.5 ABI) correctly the TDH.SYS.INIT doesn't
return TDX_RND_NO_ENTROPY.  TDH.SYS.LP.INIT indeed can return NO_ENTROPY but as
you said it doesn't take any register as input.  So technically the code works
fine.  (Even the TDH.SYS.INIT can return NO_ENTROPY the code still works fine
because the RCX must be 0 for TDH.SYS.INIT.)

Also, I can hardly think out of any reason why TDX module needs to clobber input
registers in case of NO_ENTROPY for *ANY* SEAMCALL.  But despite that, I am not
opposing the idea that it *MIGHT* be better to "not assume" NO_ENTROPY will
never clobber registers either, e.g., for the sake of future extendibility.  In
this case, the below diff should address:

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index a621721f63dd..962a7a6be721 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -97,12 +97,23 @@ typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args
*args);
 static inline u64 sc_retry(sc_func_t func, u64 fn,
                           struct tdx_module_args *args)
 {
+       struct tdx_module_args _args = *args;
        int retry = RDRAND_RETRY_LOOPS;
        u64 ret;
 
-       do {
-               ret = func(fn, args);
-       } while (ret == TDX_RND_NO_ENTROPY && --retry);
+again:
+       ret = func(fn, args);
+       if (ret == TDX_RND_NO_ENTROPY && --retry) {
+               /*
+                * Do not assume TDX module will never clobber the input
+                * registers when any SEAMCALL fails with out of entropy.
+                * In this case the original input registers in @args
+                * are clobbered.  Always restore the input registers
+                * before retrying the SEAMCALL.
+                */
+               *args = _args;
+               goto again;
+       }
 
        return ret;
 }


The downside is we will have an additional memory copy of 'struct
tdx_module_args' for each SEAMCALL, but I don't believe this will have any
difference in practice.

Or, we can go and ask TDX module guys to promise no input registers will be
clobbered in case of NO_ENTROPY.

Hi Dave,

Do you have any opinion?
Isaku Yamahata Nov. 15, 2023, 7:26 p.m. UTC | #4 
On Wed, Nov 15, 2023 at 10:41:46AM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> 
> > > +#include <asm/archrandom.h>
> > > +
> > > +typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args *args);
> > > +
> > > +static inline u64 sc_retry(sc_func_t func, u64 fn,
> > > +			   struct tdx_module_args *args)
> > > +{
> > > +	int retry = RDRAND_RETRY_LOOPS;
> > > +	u64 ret;
> > > +
> > > +	do {
> > > +		ret = func(fn, args);
> > > +	} while (ret == TDX_RND_NO_ENTROPY && --retry);
> > 
> > This loop assumes that args isn't touched when TDX_RND_NO_ENTRYPOY is returned.
> > It's not true.  TDH.SYS.INIT() and TDH.SYS.LP.INIT() clear RCX, RDX, etc on
> > error including TDX_RND_NO_ENTRY.  Because TDH.SYS.INIT() takes RCX as input,
> > this wrapper doesn't work.  TDH.SYS.LP.INIT() doesn't use RCX, RDX ... as
> > input. So it doesn't matter.
> > 
> > Other SEAMCALLs doesn't touch registers on the no entropy error.
> > TDH.EXPORTS.STATE.IMMUTABLE(), TDH.IMPORTS.STATE.IMMUTABLE(), TDH.MNG.ADDCX(),
> > and TDX.MNG.CREATE().  TDH.SYS.INIT() is an exception.
> 
> If I am reading the spec (TDX module 1.5 ABI) correctly the TDH.SYS.INIT doesn't
> return TDX_RND_NO_ENTROPY.

The next updated spec would fix it.
                                  

> TDH.SYS.LP.INIT indeed can return NO_ENTROPY but as
> you said it doesn't take any register as input.  So technically the code works
> fine.  (Even the TDH.SYS.INIT can return NO_ENTROPY the code still works fine
> because the RCX must be 0 for TDH.SYS.INIT.)

Ah yes, I agree with you. So it doesn't matter.


> Also, I can hardly think out of any reason why TDX module needs to clobber input
> registers in case of NO_ENTROPY for *ANY* SEAMCALL.  But despite that, I am not
> opposing the idea that it *MIGHT* be better to "not assume" NO_ENTROPY will
> never clobber registers either, e.g., for the sake of future extendibility.  In
> this case, the below diff should address:

Now we agreed that TDH.SYS.INIT() and TDH.SYS.LP.INIT() doesn't matter,
I'm fine with this patch. (TDX KVM handles other SEAMCALLS itself.)

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
diff mbox series

Patch

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index ea9a0320b1f8..f1c0c15469f8 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -24,6 +24,11 @@ 
 #define TDX_SEAMCALL_GP			(TDX_SW_ERROR | X86_TRAP_GP)
 #define TDX_SEAMCALL_UD			(TDX_SW_ERROR | X86_TRAP_UD)
 
+/*
+ * TDX module SEAMCALL leaf function error codes
+ */
+#define TDX_RND_NO_ENTROPY	0x8000020300000000ULL
+
 #ifndef __ASSEMBLY__
 
 /*
@@ -84,6 +89,27 @@  u64 __seamcall(u64 fn, struct tdx_module_args *args);
 u64 __seamcall_ret(u64 fn, struct tdx_module_args *args);
 u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);
 
+#include <asm/archrandom.h>
+
+typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args *args);
+
+static inline u64 sc_retry(sc_func_t func, u64 fn,
+			   struct tdx_module_args *args)
+{
+	int retry = RDRAND_RETRY_LOOPS;
+	u64 ret;
+
+	do {
+		ret = func(fn, args);
+	} while (ret == TDX_RND_NO_ENTROPY && --retry);
+
+	return ret;
+}
+
+#define seamcall(_fn, _args)		sc_retry(__seamcall, (_fn), (_args))
+#define seamcall_ret(_fn, _args)	sc_retry(__seamcall_ret, (_fn), (_args))
+#define seamcall_saved_ret(_fn, _args)	sc_retry(__seamcall_saved_ret, (_fn), (_args))
+
 bool platform_tdx_enabled(void);
 #else
 static inline bool platform_tdx_enabled(void) { return false; }