Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <2386151bc0a42b2eda895d85b459bf7930306694.camel@intel.com>
Subject: Re: [RFC PATCH v5 023/104] x86/cpu: Add helper functions to
 allocate/free MKTME keyid
From:   Kai Huang <kai.huang@intel.com>
To:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>, erdemaktas@google.com,
        Connor Kuehl <ckuehl@redhat.com>,
        Sean Christopherson <seanjc@google.com>
Date:   Thu, 31 Mar 2022 14:21:06 +1300
In-Reply-To: <a1d1e4f26c6ef44a557e873be2818e6a03e12038.1646422845.git.isaku.yamahata@intel.com>
References: <cover.1646422845.git.isaku.yamahata@intel.com>
         <a1d1e4f26c6ef44a557e873be2818e6a03e12038.1646422845.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 2022-03-04 at 11:48 -0800, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> 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 <isaku.yamahata@intel.com>
> ---
>  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;