Re: [PATCH 10/13] x86/virt/tdx: Add SEAMCALL wrappers to remove a TD private page

[Yan Zhao <yan.y.zhao@intel.com>]

On Tue, Jan 07, 2025 at 02:13:19PM -0800, Dave Hansen wrote:
> On 1/6/25 22:43, Yan Zhao wrote:
> > +u64 tdh_phymem_page_wbinvd_hkid(struct page *page, u16 hkid)
> > +{
> > +       struct tdx_module_args args = {};
> > +
> > +       args.rcx = page_to_phys(page) | ((hpa_t)hkid << boot_cpu_data.x86_phys_bits);
> 
> I've seen this idiom enough times. You need a helper:
> 
> u64 mk_keyed_paddr(struct page *page, u64 keyid)
> {
> 	u64 ret;
> 
> 	ret = page_to_phys(page);
> 	/* KeyID bits are just above the physical address bits: */
> 	ret |= keyid << boot_cpu_data.x86_phys_bits;
> 	
> 	return ret;
> }

Thanks. There's actually a helper at [1].
I made the helper in [1] as a fixup patch since it needs to be applied to both
tdh_phymem_page_wbinvd_tdr() and tdh_phymem_page_wbinvd_hkid().


[1] https://lore.kernel.org/all/Z3zPSPrFtxM7l5cD@yzhao56-desk.sh.intel.com/

> 
> Although I'm also debating a bit what the typing on 'keyid' should be.
> Right now it's quite tied to the physical address width, but that's not
> fundamental to TDX. It could absolutely change in the future.
Changing from "u64" to "u16" was raised during our internal review.
The main reasons for this change are to avoid overflow and also because:

- In MSR IA32_TME_ACTIVATE, MK_TME_KEYID_BITS and TDX_RESERVED_KEYID_BITS are
  only defined with 4 bits, so at most 16 bits in HPA can be configured as
  KeyIDs.

- TDX spec explictly requires KeyID to be 16 bits for SEAMCALLs TDH.SYS.CONFIG
  and TDH.MNG.CREATE.

- Corresponding variables for tdx_global_keyid, tdx_guest_keyid_start,
  tdx_nr_guest_keyids, nr_tdx_keyids, tdx_keyid_start are all u16 in TDX module
  code.

There is a proposed fix to change the type of KeyID to u16 as shown below (not
yet split and sent out). Do you think this change to u16 makes sense?

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 296d123a6c1c..5f3931e62c06 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -122,7 +122,7 @@ const char *tdx_dump_mce_info(struct mce *m);
 const struct tdx_sys_info *tdx_get_sysinfo(void);
 
 int tdx_guest_keyid_alloc(void);
-void tdx_guest_keyid_free(unsigned int keyid);
+void tdx_guest_keyid_free(u16 keyid);
 
 struct tdx_td {
 	/* TD root structure: */
@@ -164,7 +164,7 @@ u64 tdh_mem_page_aug(struct tdx_td *td, gfn_t gfn, struct page *private_page,
 u64 tdh_mem_range_block(struct tdx_td *td, gfn_t gfn, int tdx_level,
 			u64 *extended_err1, u64 *extended_err2);
 u64 tdh_mng_key_config(struct tdx_td *td);
-u64 tdh_mng_create(struct tdx_td *td, u64 hkid);
+u64 tdh_mng_create(struct tdx_td *td, u16 hkid);
 u64 tdh_vp_create(struct tdx_td *td, struct tdx_vp *vp);
 u64 tdh_mng_rd(struct tdx_td *td, u64 field, u64 *data);
 u64 tdh_mr_extend(struct tdx_td *td, gpa_t gpa, u64 *extended_err1, u64 *extended_err2);
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index d86bfcbd6873..4e7330df4e32 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -308,13 +308,13 @@ static __always_inline void tdvmcall_set_return_val(struct kvm_vcpu *vcpu,
 static inline void tdx_hkid_free(struct kvm_tdx *kvm_tdx)
 {
 	tdx_guest_keyid_free(kvm_tdx->hkid);
-	kvm_tdx->hkid = -1;
+	kvm_tdx->hkid_assigned = false;
 	atomic_dec(&nr_configured_hkid);
 }
 
 static inline bool is_hkid_assigned(struct kvm_tdx *kvm_tdx)
 {
-	return kvm_tdx->hkid > 0;
+	return kvm_tdx->hkid_assigned;
 }
 
 static inline void tdx_disassociate_vp(struct kvm_vcpu *vcpu)
@@ -2354,7 +2354,8 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
 	ret = tdx_guest_keyid_alloc();
 	if (ret < 0)
 		return ret;
-	kvm_tdx->hkid = ret;
+	kvm_tdx->hkid = (u16)ret;
+	kvm_tdx->hkid_assigned = true;
 
 	atomic_inc(&nr_configured_hkid);
 
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
index b07ca9ecaf95..67b44e98bf49 100644
--- a/arch/x86/kvm/vmx/tdx.h
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -29,7 +29,8 @@ struct kvm_tdx {
 
 	u64 attributes;
 	u64 xfam;
-	int hkid;
+	u16 hkid;
+	bool hkid_assigned;
 
 	u64 tsc_offset;
 
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 1986d360e9b7..5dd83e59c5b6 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -40,9 +40,9 @@
 #include <asm/mce.h>
 #include "tdx.h"
 
-static u32 tdx_global_keyid __ro_after_init;
-static u32 tdx_guest_keyid_start __ro_after_init;
-static u32 tdx_nr_guest_keyids __ro_after_init;
+static u16 tdx_global_keyid __ro_after_init;
+static u16 tdx_guest_keyid_start __ro_after_init;
+static u16 tdx_nr_guest_keyids __ro_after_init;
 
 static DEFINE_IDA(tdx_guest_keyid_pool);
 
@@ -979,7 +979,7 @@ static int construct_tdmrs(struct list_head *tmb_list,
 	return ret;
 }
 
-static int config_tdx_module(struct tdmr_info_list *tdmr_list, u64 global_keyid)
+static int config_tdx_module(struct tdmr_info_list *tdmr_list, u16 global_keyid)
 {
 	struct tdx_module_args args = {};
 	u64 *tdmr_pa_array;
@@ -1375,8 +1375,8 @@ const char *tdx_dump_mce_info(struct mce *m)
 	return "TDX private memory error. Possible kernel bug.";
 }
 
-static __init int record_keyid_partitioning(u32 *tdx_keyid_start,
-					    u32 *nr_tdx_keyids)
+static __init int record_keyid_partitioning(u16 *tdx_keyid_start,
+					    u16 *nr_tdx_keyids)
 {
 	u32 _nr_mktme_keyids, _tdx_keyid_start, _nr_tdx_keyids;
 	int ret;
@@ -1394,6 +1394,11 @@ static __init int record_keyid_partitioning(u32 *tdx_keyid_start,
 	/* TDX KeyIDs start after the last MKTME KeyID. */
 	_tdx_keyid_start = _nr_mktme_keyids + 1;
 
+	/*
+	 * In IA32_TME_ACTIVATE, MK_TME_KEYID_BITS and TDX_RESERVED_KEYID_BITS
+	 * are only 4 bits, so at most 16 bits in HPA can be configured as
+	 * KeyIDs.
+	 */
 	*tdx_keyid_start = _tdx_keyid_start;
 	*nr_tdx_keyids = _nr_tdx_keyids;
 
@@ -1467,7 +1472,7 @@ static void __init check_tdx_erratum(void)
 
 void __init tdx_init(void)
 {
-	u32 tdx_keyid_start, nr_tdx_keyids;
+	u16 tdx_keyid_start, nr_tdx_keyids;
 	int err;
 
 	err = record_keyid_partitioning(&tdx_keyid_start, &nr_tdx_keyids);
@@ -1544,7 +1549,7 @@ int tdx_guest_keyid_alloc(void)
 }
 EXPORT_SYMBOL_GPL(tdx_guest_keyid_alloc);
 
-void tdx_guest_keyid_free(unsigned int keyid)
+void tdx_guest_keyid_free(u16 keyid)
 {
 	ida_free(&tdx_guest_keyid_pool, keyid);
 }
@@ -1697,7 +1702,7 @@ u64 tdh_mng_key_config(struct tdx_td *td)
 }
 EXPORT_SYMBOL_GPL(tdh_mng_key_config);
 
-u64 tdh_mng_create(struct tdx_td *td, u64 hkid)
+u64 tdh_mng_create(struct tdx_td *td, u16 hkid)
 {
 	struct tdx_module_args args = {
 		.rcx = tdx_tdr_pa(td),