[RFC,v5,023/104] x86/cpu: Add helper functions to allocate/free MKTME keyid

Commit Message

Isaku Yamahata March 4, 2022, 7:48 p.m. UTC

From: Isaku Yamahata <[e-mail]>

MKTME keyid is assigned to guest TD.  The memory controller encrypts guest
TD memory with key id.  Add helper functions to allocate/free MKTME keyid
so that TDX KVM assign keyid.

Also export MKTME global keyid that is used to encrypt TDX module and its
memory.

Signed-off-by: Isaku Yamahata <[e-mail]>
---
 arch/x86/include/asm/tdx.h |  6 ++++++
 arch/x86/virt/vmx/tdx.c    | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

Comments

Huang, Kai March 31, 2022, 1:21 a.m. UTC | #1

On Fri, 2022-03-04 at 11:48 -0800, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
> 
> MKTME keyid is assigned to guest TD.  The memory controller encrypts guest
> TD memory with key id.  Add helper functions to allocate/free MKTME keyid
> so that TDX KVM assign keyid.

Using MKTME keyid is wrong, at least not accurate I think.  We should use
explicitly use "TDX private KeyID", which is clearly documented in the spec:
  
https://software.intel.com/content/dam/develop/external/us/en/documents-tps/intel-tdx-cpu-architectural-specification.pdf

Also, description of IA32_MKTME_KEYID_PARTITIONING MSR clearly says TDX private
KeyIDs span the range (NUM_MKTME_KIDS+1) through
(NUM_MKTME_KIDS+NUM_TDX_PRIV_KIDS).  So please just use TDX private KeyID here.


> 
> Also export MKTME global keyid that is used to encrypt TDX module and its
> memory.

This needs explanation why the global keyID needs to be exported.

> 
> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
>  arch/x86/include/asm/tdx.h |  6 ++++++
>  arch/x86/virt/vmx/tdx.c    | 33 ++++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 9a8dc6afcb63..73bb472bd515 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -139,6 +139,9 @@ int tdx_detect(void);
>  int tdx_init(void);
>  bool platform_has_tdx(void);
>  const struct tdsysinfo_struct *tdx_get_sysinfo(void);
> +u32 tdx_get_global_keyid(void);
> +int tdx_keyid_alloc(void);
> +void tdx_keyid_free(int keyid);
>  #else
>  static inline void tdx_detect_cpu(struct cpuinfo_x86 *c) { }
>  static inline int tdx_detect(void) { return -ENODEV; }
> @@ -146,6 +149,9 @@ static inline int tdx_init(void) { return -ENODEV; }
>  static inline bool platform_has_tdx(void) { return false; }
>  struct tdsysinfo_struct;
>  static inline const struct tdsysinfo_struct *tdx_get_sysinfo(void) { return NULL; }
> +static inline u32 tdx_get_global_keyid(void) { return 0; };
> +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.c b/arch/x86/virt/vmx/tdx.c
> index e45f188479cb..d714106321d4 100644
> --- a/arch/x86/virt/vmx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx.c
> @@ -113,7 +113,13 @@ static int tdx_cmr_num;
>  static struct tdsysinfo_struct tdx_sysinfo;
>  
>  /* TDX global KeyID to protect TDX metadata */
> -static u32 tdx_global_keyid;
> +static u32 __read_mostly tdx_global_keyid;
> +
> +u32 tdx_get_global_keyid(void)
> +{
> +	return tdx_global_keyid;
> +}
> +EXPORT_SYMBOL_GPL(tdx_get_global_keyid);
>  
>  static bool enable_tdx_host;
>  
> @@ -189,6 +195,31 @@ static void detect_seam(struct cpuinfo_x86 *c)
>  		detect_seam_ap(c);
>  }
>  
> +/* TDX KeyID pool */
> +static DEFINE_IDA(tdx_keyid_pool);
> +
> +int tdx_keyid_alloc(void)
> +{
> +	if (WARN_ON_ONCE(!tdx_keyid_start || !tdx_keyid_num))
> +		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 + tdx_keyid_num - 1,
> +			       GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(tdx_keyid_alloc);
> +
> +void tdx_keyid_free(int keyid)
> +{
> +	/* keyid = 0 is reserved. */
> +	if (!keyid || keyid <= 0)
> +		return;
> +
> +	ida_free(&tdx_keyid_pool, keyid);
> +}
> +EXPORT_SYMBOL_GPL(tdx_keyid_free);
> +
>  static void detect_tdx_keyids_bsp(struct cpuinfo_x86 *c)
>  {
>  	u64 keyid_part;

Isaku Yamahata March 31, 2022, 8:15 p.m. UTC | #2

On Thu, Mar 31, 2022 at 02:21:06PM +1300,
Kai Huang <[e-mail]> wrote:

> On Fri, 2022-03-04 at 11:48 -0800, [e-mail] wrote:
> > From: Isaku Yamahata <[e-mail]>
> > 
> > MKTME keyid is assigned to guest TD.  The memory controller encrypts guest
> > TD memory with key id.  Add helper functions to allocate/free MKTME keyid
> > so that TDX KVM assign keyid.
> 
> Using MKTME keyid is wrong, at least not accurate I think.  We should use
> explicitly use "TDX private KeyID", which is clearly documented in the spec:
>   
> https://software.intel.com/content/dam/develop/external/us/en/documents-tps/intel-tdx-cpu-architectural-specification.pdf
> 
> Also, description of IA32_MKTME_KEYID_PARTITIONING MSR clearly says TDX private
> KeyIDs span the range (NUM_MKTME_KIDS+1) through
> (NUM_MKTME_KIDS+NUM_TDX_PRIV_KIDS).  So please just use TDX private KeyID here.
> 
> 
> > 
> > Also export MKTME global keyid that is used to encrypt TDX module and its
> > memory.
> 
> This needs explanation why the global keyID needs to be exported.

How about the followings?

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

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

Thanks,

Paolo Bonzini April 5, 2022, 1:08 p.m. UTC | #3

On 3/4/22 20:48, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
> 
> MKTME keyid is assigned to guest TD.  The memory controller encrypts guest
> TD memory with key id.  Add helper functions to allocate/free MKTME keyid
> so that TDX KVM assign keyid.
> 
> Also export MKTME global keyid that is used to encrypt TDX module and its
> memory.
> 
> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
>   arch/x86/include/asm/tdx.h |  6 ++++++
>   arch/x86/virt/vmx/tdx.c    | 33 ++++++++++++++++++++++++++++++++-
>   2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 9a8dc6afcb63..73bb472bd515 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -139,6 +139,9 @@ int tdx_detect(void);
>   int tdx_init(void);
>   bool platform_has_tdx(void);
>   const struct tdsysinfo_struct *tdx_get_sysinfo(void);
> +u32 tdx_get_global_keyid(void);
> +int tdx_keyid_alloc(void);
> +void tdx_keyid_free(int keyid);
>   #else
>   static inline void tdx_detect_cpu(struct cpuinfo_x86 *c) { }
>   static inline int tdx_detect(void) { return -ENODEV; }
> @@ -146,6 +149,9 @@ static inline int tdx_init(void) { return -ENODEV; }
>   static inline bool platform_has_tdx(void) { return false; }
>   struct tdsysinfo_struct;
>   static inline const struct tdsysinfo_struct *tdx_get_sysinfo(void) { return NULL; }
> +static inline u32 tdx_get_global_keyid(void) { return 0; };
> +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.c b/arch/x86/virt/vmx/tdx.c
> index e45f188479cb..d714106321d4 100644
> --- a/arch/x86/virt/vmx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx.c
> @@ -113,7 +113,13 @@ static int tdx_cmr_num;
>   static struct tdsysinfo_struct tdx_sysinfo;
>   
>   /* TDX global KeyID to protect TDX metadata */
> -static u32 tdx_global_keyid;
> +static u32 __read_mostly tdx_global_keyid;
> +
> +u32 tdx_get_global_keyid(void)
> +{
> +	return tdx_global_keyid;
> +}
> +EXPORT_SYMBOL_GPL(tdx_get_global_keyid);
>   
>   static bool enable_tdx_host;
>   
> @@ -189,6 +195,31 @@ static void detect_seam(struct cpuinfo_x86 *c)
>   		detect_seam_ap(c);
>   }
>   
> +/* TDX KeyID pool */
> +static DEFINE_IDA(tdx_keyid_pool);
> +
> +int tdx_keyid_alloc(void)
> +{
> +	if (WARN_ON_ONCE(!tdx_keyid_start || !tdx_keyid_num))
> +		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 + tdx_keyid_num - 1,
> +			       GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(tdx_keyid_alloc);
> +
> +void tdx_keyid_free(int keyid)
> +{
> +	/* keyid = 0 is reserved. */
> +	if (!keyid || keyid <= 0)
> +		return;
> +
> +	ida_free(&tdx_keyid_pool, keyid);
> +}
> +EXPORT_SYMBOL_GPL(tdx_keyid_free);
> +
>   static void detect_tdx_keyids_bsp(struct cpuinfo_x86 *c)
>   {
>   	u64 keyid_part;

Reviewed-by: Paolo Bonzini <[e-mail]>

Huang, Kai April 6, 2022, 1:55 a.m. UTC | #4

On Thu, 2022-03-31 at 13:15 -0700, Isaku Yamahata wrote:
> On Thu, Mar 31, 2022 at 02:21:06PM +1300,
> Kai Huang <[e-mail]> wrote:
> 
> > On Fri, 2022-03-04 at 11:48 -0800, [e-mail] wrote:
> > > From: Isaku Yamahata <[e-mail]>
> > > 
> > > MKTME keyid is assigned to guest TD.  The memory controller encrypts guest
> > > TD memory with key id.  Add helper functions to allocate/free MKTME keyid
> > > so that TDX KVM assign keyid.
> > 
> > Using MKTME keyid is wrong, at least not accurate I think.  We should use
> > explicitly use "TDX private KeyID", which is clearly documented in the spec:
> >   
> > https://software.intel.com/content/dam/develop/external/us/en/documents-tps/intel-tdx-cpu-architectural-specification.pdf
> > 
> > Also, description of IA32_MKTME_KEYID_PARTITIONING MSR clearly says TDX private
> > KeyIDs span the range (NUM_MKTME_KIDS+1) through
> > (NUM_MKTME_KIDS+NUM_TDX_PRIV_KIDS).  So please just use TDX private KeyID here.
> > 
> > 
> > > 
> > > Also export MKTME global keyid that is used to encrypt TDX module and its
> > > memory.
> > 
> > This needs explanation why the global keyID needs to be exported.
> 
> How about the followings?
> 
> TDX private host key id is assigned to guest TD.  The memory controller
> encrypts guest TD memory with the assigned host key id (HIKD).  Add helper
> functions to allocate/free TDX private host key id so that TDX KVM manage
> it.

HIKD -> HKID.  

You may also want to use KeyID in consistent way (KeyID, keyid, key id, etc).
The spec uses KeyID.

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

Find to me.

Huang, Kai April 7, 2022, 1 a.m. UTC | #5

> 
> > 
> > Also export the global TDX private host key id that is used to encrypt TDX
> > module, its memory and some dynamic data (e.g. TDR).  
> > 

Sorry I was replying too quick.

This sentence is not correct.  Hardware doesn't use global KeyID to encrypt TDX
module itself.  In current generation of TDX, global KeyID is used to encrypt
TDX memory metadata (PAMTs) and TDRs.


> > When VMM releasing
> > encrypted page to reuse it, the page needs to be flushed with the used host
> > key id.  VMM needs the global TDX private host key id to flush such pages
> > TDX module accesses with the global TDX private host key id.
> > 
> > 
> 
> Find to me.
>

Patch

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 9a8dc6afcb63..73bb472bd515 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -139,6 +139,9 @@  int tdx_detect(void);
 int tdx_init(void);
 bool platform_has_tdx(void);
 const struct tdsysinfo_struct *tdx_get_sysinfo(void);
+u32 tdx_get_global_keyid(void);
+int tdx_keyid_alloc(void);
+void tdx_keyid_free(int keyid);
 #else
 static inline void tdx_detect_cpu(struct cpuinfo_x86 *c) { }
 static inline int tdx_detect(void) { return -ENODEV; }
@@ -146,6 +149,9 @@  static inline int tdx_init(void) { return -ENODEV; }
 static inline bool platform_has_tdx(void) { return false; }
 struct tdsysinfo_struct;
 static inline const struct tdsysinfo_struct *tdx_get_sysinfo(void) { return NULL; }
+static inline u32 tdx_get_global_keyid(void) { return 0; };
+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.c b/arch/x86/virt/vmx/tdx.c
index e45f188479cb..d714106321d4 100644
--- a/arch/x86/virt/vmx/tdx.c
+++ b/arch/x86/virt/vmx/tdx.c
@@ -113,7 +113,13 @@  static int tdx_cmr_num;
 static struct tdsysinfo_struct tdx_sysinfo;
 
 /* TDX global KeyID to protect TDX metadata */
-static u32 tdx_global_keyid;
+static u32 __read_mostly tdx_global_keyid;
+
+u32 tdx_get_global_keyid(void)
+{
+	return tdx_global_keyid;
+}
+EXPORT_SYMBOL_GPL(tdx_get_global_keyid);
 
 static bool enable_tdx_host;
 
@@ -189,6 +195,31 @@  static void detect_seam(struct cpuinfo_x86 *c)
 		detect_seam_ap(c);
 }
 
+/* TDX KeyID pool */
+static DEFINE_IDA(tdx_keyid_pool);
+
+int tdx_keyid_alloc(void)
+{
+	if (WARN_ON_ONCE(!tdx_keyid_start || !tdx_keyid_num))
+		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 + tdx_keyid_num - 1,
+			       GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(tdx_keyid_alloc);
+
+void tdx_keyid_free(int keyid)
+{
+	/* keyid = 0 is reserved. */
+	if (!keyid || keyid <= 0)
+		return;
+
+	ida_free(&tdx_keyid_pool, keyid);
+}
+EXPORT_SYMBOL_GPL(tdx_keyid_free);
+
 static void detect_tdx_keyids_bsp(struct cpuinfo_x86 *c)
 {
 	u64 keyid_part;