diff mbox series

[v7,07/20] x86/virt/tdx: Do TDX module global initialization

Message ID 40824ec3e3dc759705dcfa1cb2929d18c12b417a.1668988357.git.kai.huang@intel.com
State New
Headers
Series TDX host kernel support |

Commit Message

Kai Huang Nov. 21, 2022, 12:26 a.m. UTC 
  The first step of initializing the module is to call TDH.SYS.INIT once
on any logical cpu to do module global initialization.  Do the module
global initialization.

It also detects the TDX module, as seamcall() returns -ENODEV when the
module is not loaded.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v6 -> v7:
 - Improved changelog.

---
 arch/x86/virt/vmx/tdx/tdx.c | 19 +++++++++++++++++--
 arch/x86/virt/vmx/tdx/tdx.h |  1 +
 2 files changed, 18 insertions(+), 2 deletions(-)

Comments

Dave Hansen Nov. 22, 2022, 7:14 p.m. UTC | #1 
On 11/20/22 16:26, Kai Huang wrote:
> The first step of initializing the module is to call TDH.SYS.INIT once
> on any logical cpu to do module global initialization.  Do the module
> global initialization.
> 
> It also detects the TDX module, as seamcall() returns -ENODEV when the
> module is not loaded.

Part of making a good patch set is telling a bit of a story.  In patch
4, you laid out 6 steps necessary to initialize TDX.  On top of that,
there is infrastructure  It would be great to lay that out in a way that
folks can actually follow along.

For instance, it would be great to tell the reader here that this patch
is an inflection point.  It is transitioning out of the infrastructure
(patches 1->6) and into the actual "multi-steps" of initialization that
the module spec requires.

This patch is *TOTALLY* different from the one before it because it
actually _starts_ to do something useful.

But, you wouldn't know it from the changelog.

>  arch/x86/virt/vmx/tdx/tdx.c | 19 +++++++++++++++++--
>  arch/x86/virt/vmx/tdx/tdx.h |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 5db1a05cb4bd..f292292313bd 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -208,8 +208,23 @@ static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
>   */
>  static int init_tdx_module(void)
>  {
> -	/* The TDX module hasn't been detected */
> -	return -ENODEV;
> +	int ret;
> +
> +	/*
> +	 * Call TDH.SYS.INIT to do the global initialization of
> +	 * the TDX module.  It also detects the module.
> +	 */
> +	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
> +	if (ret)
> +		goto out;

Please also note that the 0's are all just unused parameters.  They mean
nothing.

> +
> +	/*
> +	 * Return -EINVAL until all steps of TDX module initialization
> +	 * process are done.
> +	 */
> +	ret = -EINVAL;
> +out:
> +	return ret;
>  }

It might be a bit unconventional, but can you imagine how well it would
tell the story if this comment said:

	/*
	 * TODO:
	 *  - Logical-CPU scope initialization (TDH_SYS_INIT_LP)
	 *  - Enumerate capabilities and platform configuration
	      (TDH_SYS_CONFIG)
	 ...
	 */

and then each of the following patches that *did* those things removed
the TODO line from the list.

That TODO list could have been added in patch 4.

>  static void shutdown_tdx_module(void)
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 215cc1065d78..0b415805c921 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -15,6 +15,7 @@
>  /*
>   * TDX module SEAMCALL leaf functions
>   */
> +#define TDH_SYS_INIT		33
>  #define TDH_SYS_LP_SHUTDOWN	44
>  
>  /*
Kai Huang Nov. 23, 2022, 11:45 a.m. UTC | #2 
On Tue, 2022-11-22 at 11:14 -0800, Dave Hansen wrote:
> On 11/20/22 16:26, Kai Huang wrote:
> > The first step of initializing the module is to call TDH.SYS.INIT once
> > on any logical cpu to do module global initialization.  Do the module
> > global initialization.
> > 
> > It also detects the TDX module, as seamcall() returns -ENODEV when the
> > module is not loaded.
> 
> Part of making a good patch set is telling a bit of a story.  In patch
> 4, you laid out 6 steps necessary to initialize TDX.  On top of that,
> there is infrastructure  It would be great to lay that out in a way that
> folks can actually follow along.
> 
> For instance, it would be great to tell the reader here that this patch
> is an inflection point.  It is transitioning out of the infrastructure
> (patches 1->6) and into the actual "multi-steps" of initialization that
> the module spec requires.
> 
> This patch is *TOTALLY* different from the one before it because it
> actually _starts_ to do something useful.
> 
> But, you wouldn't know it from the changelog.

I'll try to enhance the changelog to make them more connected.  Right now I
don't have a clear clue on how to write in best way.

> 
> >  arch/x86/virt/vmx/tdx/tdx.c | 19 +++++++++++++++++--
> >  arch/x86/virt/vmx/tdx/tdx.h |  1 +
> >  2 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 5db1a05cb4bd..f292292313bd 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -208,8 +208,23 @@ static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
> >   */
> >  static int init_tdx_module(void)
> >  {
> > -	/* The TDX module hasn't been detected */
> > -	return -ENODEV;
> > +	int ret;
> > +
> > +	/*
> > +	 * Call TDH.SYS.INIT to do the global initialization of
> > +	 * the TDX module.  It also detects the module.
> > +	 */
> > +	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
> > +	if (ret)
> > +		goto out;
> 
> Please also note that the 0's are all just unused parameters.  They mean
> nothing.

Will add to the comment.

> 
> > +
> > +	/*
> > +	 * Return -EINVAL until all steps of TDX module initialization
> > +	 * process are done.
> > +	 */
> > +	ret = -EINVAL;
> > +out:
> > +	return ret;
> >  }
> 
> It might be a bit unconventional, but can you imagine how well it would
> tell the story if this comment said:
> 
> 	/*
> 	 * TODO:
> 	 *  - Logical-CPU scope initialization (TDH_SYS_INIT_LP)
> 	 *  - Enumerate capabilities and platform configuration
> 	      (TDH_SYS_CONFIG)
> 	 ...
> 	 */
> 
> and then each of the following patches that *did* those things removed
> the TODO line from the list.
> 
> That TODO list could have been added in patch 4.
> 

Thanks for suggestion.  Will do.

I think I can do this to "construct TDMRs" related patches too.
diff mbox series

Patch

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 5db1a05cb4bd..f292292313bd 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -208,8 +208,23 @@  static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
  */
 static int init_tdx_module(void)
 {
-	/* The TDX module hasn't been detected */
-	return -ENODEV;
+	int ret;
+
+	/*
+	 * Call TDH.SYS.INIT to do the global initialization of
+	 * the TDX module.  It also detects the module.
+	 */
+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (ret)
+		goto out;
+
+	/*
+	 * Return -EINVAL until all steps of TDX module initialization
+	 * process are done.
+	 */
+	ret = -EINVAL;
+out:
+	return ret;
 }
 
 static void shutdown_tdx_module(void)
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 215cc1065d78..0b415805c921 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -15,6 +15,7 @@ 
 /*
  * TDX module SEAMCALL leaf functions
  */
+#define TDH_SYS_INIT		33
 #define TDH_SYS_LP_SHUTDOWN	44
 
 /*