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

  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(+)
  

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>
  

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.
  

> > +#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?
  

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>