[PATCH v11 013/113] x86/cpu: Add helper functions to allocate/free TDX private host key id

isaku.yamahata@intel.com posted 113 patches 2 years, 8 months ago
There is a newer version of this series
[PATCH v11 013/113] x86/cpu: Add helper functions to allocate/free TDX private host key id
Posted by isaku.yamahata@intel.com 2 years, 8 months ago
From: Isaku Yamahata <isaku.yamahata@intel.com>

TDX private host key id (HKID) is assigned to guest TD.  The memory
controller encrypts guest TD memory with the assigned TDX HKID.  Add helper
functions to allocate/free TDX private HKID so that TDX KVM can manage it.

Also export the global TDX private HKID that is used to encrypt TDX module,
its memory and some dynamic data (TDR).  When VMM releasing encrypted page
to reuse it, the page needs to be flushed with the used HKID.  VMM needs
the global TDX private HKID to flush such pages.

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 arch/x86/include/asm/tdx.h  | 12 ++++++++++++
 arch/x86/virt/vmx/tdx/tdx.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 0f71d3856ede..ed9cf61ff8b4 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -107,11 +107,23 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 #ifdef CONFIG_INTEL_TDX_HOST
 bool platform_tdx_enabled(void);
 int tdx_enable(void);
+/*
+ * Key id globally used by TDX module: TDX module maps TDR with this TDX global
+ * key id.  TDR includes key id assigned to the TD.  Then TDX module maps other
+ * TD-related pages with the assigned key id.  TDR requires this TDX global key
+ * id for cache flush unlike other TD-related pages.
+ */
+extern u32 tdx_global_keyid __read_mostly;
+int tdx_keyid_alloc(void);
+void tdx_keyid_free(int keyid);
+
 u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
 	       struct tdx_module_output *out);
 #else	/* !CONFIG_INTEL_TDX_HOST */
 static inline bool platform_tdx_enabled(void) { return false; }
 static inline int tdx_enable(void)  { return -EINVAL; }
+static inline int tdx_keyid_alloc(void) { return -EOPNOTSUPP; }
+static inline void tdx_keyid_free(int keyid) { }
 #endif	/* CONFIG_INTEL_TDX_HOST */
 
 #endif /* !__ASSEMBLY__ */
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index eba7e62cebec..d18ab5c4d447 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -51,6 +51,10 @@ static DEFINE_MUTEX(tdx_module_lock);
 /* All TDX-usable memory regions */
 static LIST_HEAD(tdx_memlist);
 
+/* TDX module global KeyID.  Used in TDH.SYS.CONFIG ABI. */
+u32 tdx_global_keyid __read_mostly;
+EXPORT_SYMBOL_GPL(tdx_global_keyid);
+
 /*
  * tdx_keyid_start and nr_tdx_keyids indicate that TDX is uninitialized.
  * This is used in TDX initialization error paths to take it from
@@ -132,6 +136,31 @@ static struct notifier_block tdx_memory_nb = {
 	.notifier_call = tdx_memory_notifier,
 };
 
+/* TDX KeyID pool */
+static DEFINE_IDA(tdx_keyid_pool);
+
+int tdx_keyid_alloc(void)
+{
+	if (WARN_ON_ONCE(!tdx_keyid_start || !nr_tdx_keyids))
+		return -EINVAL;
+
+	/* The first keyID is reserved for the global key. */
+	return ida_alloc_range(&tdx_keyid_pool, tdx_keyid_start + 1,
+			       tdx_keyid_start + nr_tdx_keyids - 1,
+			       GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(tdx_keyid_alloc);
+
+void tdx_keyid_free(int keyid)
+{
+	/* keyid = 0 is reserved. */
+	if (WARN_ON_ONCE(keyid <= 0))
+		return;
+
+	ida_free(&tdx_keyid_pool, keyid);
+}
+EXPORT_SYMBOL_GPL(tdx_keyid_free);
+
 static int __init tdx_init(void)
 {
 	int err;
@@ -1161,6 +1190,12 @@ static int init_tdx_module(void)
 	if (ret)
 		goto out_free_pamts;
 
+	/*
+	 * Reserve the first TDX KeyID as global KeyID to protect
+	 * TDX module metadata.
+	 */
+	tdx_global_keyid = tdx_keyid_start;
+
 	/* Initialize TDMRs to complete the TDX module initialization */
 	ret = init_tdmrs(&tdmr_list);
 	if (ret)
-- 
2.25.1
Re: [PATCH v11 013/113] x86/cpu: Add helper functions to allocate/free TDX private host key id
Posted by Zhi Wang 2 years, 8 months ago
On Thu, 12 Jan 2023 08:31:21 -0800
isaku.yamahata@intel.com wrote:

> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> TDX private host key id (HKID) is assigned to guest TD.  The memory
> controller encrypts guest TD memory with the assigned TDX HKID.  Add
> helper functions to allocate/free TDX private HKID so that TDX KVM can
> manage it.
> 
> Also export the global TDX private HKID that is used to encrypt TDX
> module, its memory and some dynamic data (TDR).  When VMM releasing
> encrypted page to reuse it, the page needs to be flushed with the used
> HKID.  VMM needs the global TDX private HKID to flush such pages.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  arch/x86/include/asm/tdx.h  | 12 ++++++++++++
>  arch/x86/virt/vmx/tdx/tdx.c | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 0f71d3856ede..ed9cf61ff8b4 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -107,11 +107,23 @@ static inline long tdx_kvm_hypercall(unsigned int
> nr, unsigned long p1, #ifdef CONFIG_INTEL_TDX_HOST
>  bool platform_tdx_enabled(void);
>  int tdx_enable(void);
> +/*
> + * Key id globally used by TDX module: TDX module maps TDR with this
> TDX global
> + * key id.  TDR includes key id assigned to the TD.  Then TDX module
> maps other
> + * TD-related pages with the assigned key id.  TDR requires this TDX
> global key
> + * id for cache flush unlike other TD-related pages.
> + */
> +extern u32 tdx_global_keyid __read_mostly;
> +int tdx_keyid_alloc(void);
> +void tdx_keyid_free(int keyid);
> +
>  u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
>  	       struct tdx_module_output *out);
>  #else	/* !CONFIG_INTEL_TDX_HOST */
>  static inline bool platform_tdx_enabled(void) { return false; }
>  static inline int tdx_enable(void)  { return -EINVAL; }
> +static inline int tdx_keyid_alloc(void) { return -EOPNOTSUPP; }
> +static inline void tdx_keyid_free(int keyid) { }
>  #endif	/* CONFIG_INTEL_TDX_HOST */
>  
>  #endif /* !__ASSEMBLY__ */
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index eba7e62cebec..d18ab5c4d447 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -51,6 +51,10 @@ static DEFINE_MUTEX(tdx_module_lock);
>  /* All TDX-usable memory regions */
>  static LIST_HEAD(tdx_memlist);
>  
> +/* TDX module global KeyID.  Used in TDH.SYS.CONFIG ABI. */
> +u32 tdx_global_keyid __read_mostly;
> +EXPORT_SYMBOL_GPL(tdx_global_keyid);
> +
>  /*
>   * tdx_keyid_start and nr_tdx_keyids indicate that TDX is uninitialized.
>   * This is used in TDX initialization error paths to take it from
> @@ -132,6 +136,31 @@ static struct notifier_block tdx_memory_nb = {
>  	.notifier_call = tdx_memory_notifier,
>  };
>  
> +/* TDX KeyID pool */
> +static DEFINE_IDA(tdx_keyid_pool);
> +
> +int tdx_keyid_alloc(void)
> +{
> +	if (WARN_ON_ONCE(!tdx_keyid_start || !nr_tdx_keyids))
> +		return -EINVAL;

Better mention that tdx_keyid_start and nr_tdx_keyids are defined in
another patches.

> +
> +	/* The first keyID is reserved for the global key. */
> +	return ida_alloc_range(&tdx_keyid_pool, tdx_keyid_start + 1,
> +			       tdx_keyid_start + nr_tdx_keyids - 1,
> +			       GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(tdx_keyid_alloc);
> +
> +void tdx_keyid_free(int keyid)
> +{
> +	/* keyid = 0 is reserved. */
> +	if (WARN_ON_ONCE(keyid <= 0))
> +		return;
> +
> +	ida_free(&tdx_keyid_pool, keyid);
> +}
> +EXPORT_SYMBOL_GPL(tdx_keyid_free);
> +
>  static int __init tdx_init(void)
>  {
>  	int err;
> @@ -1161,6 +1190,12 @@ static int init_tdx_module(void)
>  	if (ret)
>  		goto out_free_pamts;
>  
> +	/*
> +	 * Reserve the first TDX KeyID as global KeyID to protect
> +	 * TDX module metadata.
> +	 */
> +	tdx_global_keyid = tdx_keyid_start;
> +
>  	/* Initialize TDMRs to complete the TDX module initialization */
>  	ret = init_tdmrs(&tdmr_list);
>  	if (ret)
Re: [PATCH v11 013/113] x86/cpu: Add helper functions to allocate/free TDX private host key id
Posted by Sean Christopherson 2 years, 8 months ago
On Fri, Jan 13, 2023, Zhi Wang wrote:
> On Thu, 12 Jan 2023 08:31:21 -0800 isaku.yamahata@intel.com wrote:
> > @@ -132,6 +136,31 @@ static struct notifier_block tdx_memory_nb = {
> >  	.notifier_call = tdx_memory_notifier,
> >  };
> >  
> > +/* TDX KeyID pool */
> > +static DEFINE_IDA(tdx_keyid_pool);
> > +
> > +int tdx_keyid_alloc(void)
> > +{
> > +	if (WARN_ON_ONCE(!tdx_keyid_start || !nr_tdx_keyids))
> > +		return -EINVAL;
> 
> Better mention that tdx_keyid_start and nr_tdx_keyids are defined in
> another patches.

Eh, no need.  That sort of information doesn't belong in the changelog because
when this code is merged it will be a natural sequence.  The cover letter
explicitly calls out that this needs the kernel patches[*].  A footnote could be
added, but asking Isaku and co. to document every external dependency is asking
too much IMO.

[*] https://lore.kernel.org/lkml/cover.1670566861.git.kai.huang@intel.com
Re: [PATCH v11 013/113] x86/cpu: Add helper functions to allocate/free TDX private host key id
Posted by Zhi Wang 2 years, 8 months ago
On Fri, 13 Jan 2023 15:21:54 +0000
Sean Christopherson <seanjc@google.com> wrote:

> On Fri, Jan 13, 2023, Zhi Wang wrote:
> > On Thu, 12 Jan 2023 08:31:21 -0800 isaku.yamahata@intel.com wrote:
> > > @@ -132,6 +136,31 @@ static struct notifier_block tdx_memory_nb = {
> > >  	.notifier_call = tdx_memory_notifier,
> > >  };
> > >  
> > > +/* TDX KeyID pool */
> > > +static DEFINE_IDA(tdx_keyid_pool);
> > > +
> > > +int tdx_keyid_alloc(void)
> > > +{
> > > +	if (WARN_ON_ONCE(!tdx_keyid_start || !nr_tdx_keyids))
> > > +		return -EINVAL;
> > 
> > Better mention that tdx_keyid_start and nr_tdx_keyids are defined in
> > another patches.
> 
> Eh, no need.  That sort of information doesn't belong in the changelog
> because when this code is merged it will be a natural sequence.  The
> cover letter explicitly calls out that this needs the kernel patches[*].
>  A footnote could be added, but asking Isaku and co. to document every
> external dependency is asking too much IMO.
> 
> [*] https://lore.kernel.org/lkml/cover.1670566861.git.kai.huang@intel.com

Hi:

Thanks. I raised this concern from the reviewers' perspective. For example,
finding something was missing, grep, nothing was found, and jumping to
another window and grep. 

Finally, you can make sure if missing tdx_keyid_start in the patch is a
mistake or a dependency. Then the same happens on nr_tdx_keyids.

It would be nice to just say tdx_hkid_start, nr_tdx_keyid requires an
external patch in the comment. Or, just mention this patch depends
on an external patch in the comment. It will save quite some efforts.