[PATCH v4 0/8] TDX host: metadata reading tweaks, bug fix and info dump

[PATCH v4 0/8] TDX host: metadata reading tweaks, bug fix and info dump

[Kai Huang]

TL;DR:

This series does necessary tweaks to TDX host "global metadata" reading
code to fix some immediate issues in the TDX module initialization code,
with intention to also provide a flexible code base to support sharing
global metadata to KVM (and other kernel components) for future needs.

This series, and additional patches to initialize TDX when loading KVM
module and read essential metadata fields for KVM TDX can be found at
[1].

Hi Dave (and maintainers),

This series targets x86 tip.  Also add Dan, KVM maintainers and KVM list
so people can also review and comment.

This is a pre-work of the "quite near future" KVM TDX support (see the
kvm-coco-queue branch [2], which already includes all the patches in the
v2 of this series).  I appreciate if you can review, comment and take
this series if the patches look good to you.

v3 -> v4:
  - Change to add a build_sysmd_read(_size) macro to build one primitive
    for each metadata field element size, similar to build_mmio_read()
    macro -- Dan.

    https://lore.kernel.org/kvm/66db75497a213_22a2294b@dwillia2-xfh.jf.intel.com.notmuch/

  - Replace TD_SYSINFO_MAP() with READ_SYS_INFO() and #undef it after
    use -- Adrian, Dan.

    https://lore.kernel.org/kvm/66db7469dbfdd_22a2294c0@dwillia2-xfh.jf.intel.com.notmuch/

  - Use permalink in the changelog -- Dan.
  - Other comments from Dan, Adrian and Nikolay.  Please see individual
    patches.
  - Collect tags from Dan, Adrian, Nikolay (Thanks!).

 v3: https://lore.kernel.org/kvm/5235e05e-1d73-4f70-9b5d-b8648b1f4524@intel.com/T/

v2 -> v3 (address comments from Dan):
  - Replace the first couple of "metadata reading tweaks" patches with
    two new patches using a different approach (removin the 'struct
    field_mapping' and reimplement the TD_SYSINFO_MAP()):

    https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6
    https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#mbe65f0903ff7835bc418a907f0d02d7a9e0b78be
    https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m80cde5e6504b3af74d933ea0cbfc3ca9d24697d3

  - Split out the renaming of 'struct tdx_tdmr_sysinfo' as a separate
    patch and place it at the beginning of this series.

    https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m8fec7c429242d640cf5e756eb68e3b822e6dff8b

  - Refine this cover letter ("More info" section)

    https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m11868c9f486dcc4cfbbb690c7c18dfa4570e433f

  - Address other comments.  See changelog of individual patches.

 v2: https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/

v1 -> v2:
  - Fix comments from Chao and Nikolay.
  - A new patch to refine an out-dated comment by Nikolay.
  - Collect tags from Nikolay (thanks!).

 v1: https://lore.kernel.org/linux-kernel/cover.1718538552.git.kai.huang@intel.com/T/

=== More info ===

TDX module provides a set of "global metadata fields" for software to
query.  They report things like TDX module version, supported features
fields required for creating TDX guests and so on.

Today the TDX host code already reads "TD Memory Region" (TDMR) related
metadata fields for module initialization.  There are immediate needs
that require TDX host code to read more metadata fields:

 - Dump basic TDX module info [3];
 - Reject module with no NO_RBP_MOD feature support [4];
 - Read CMR info to fix a module initialization failure bug [5].

Also, the "quite near future" KVM TDX support requires to read more
global metadata fields.  In the longer term, the TDX Connect [6] (which
supports assigning trusted IO devices to TDX guest) may also require
other kernel components (e.g., pci/vt-d) to access more metadata.

To meet all of those, the idea is the TDX host core-kernel to provide a
centralized, canonical, and read-only structure to contain all global
metadata that comes out of TDX module for all kernel components to use.

An alternative way is to expose metadata reading API(s) for in-kernel
TDX users to use, but the reasons of choosing to provide a centural
structure are, quoted from Dan:

  The idea that x86 gets to review growth to this structure over time is
  an asset for maintainability and oversight of what is happening in the
  downstream consumers like KVM and TSM (for TDX Connect).

  A dynamic retrieval API removes that natural auditing of data structure
  patches from tip.git.

  Yes, it requires more touches than letting use cases consume new
  metadata fields at will, but that's net positive for maintainence of
  the kernel and the feedback loop to the TDX module.

This series starts to track all global metadata fields into a single
'struct tdx_sys_info', and reads more metadata fields to that structure
to address the immediate needs as mentioned above.

More fields will be added to support KVM TDX.  For the initial support
all metadata fields are populated in this single structure and shared to
KVM via a 'const pointer' to that structure (see last patches in [1]).


[1] https://github.com/intel/tdx/commits/kvm-tdxinit-host-metadata-v4/
[2] https://git.kernel.org/pub/scm/virt/kvm/kvm.git/log/?h=kvm-coco-queue
[3] https://lore.kernel.org/lkml/4b3adb59-50ea-419e-ad02-e19e8ca20dee@intel.com/
[4] https://lore.kernel.org/lkml/fc0e8ab7-86d4-4428-be31-82e1ece6dd21@intel.com/
[5] https://github.com/canonical/tdx/issues/135#issuecomment-2151570238
[6] https://cdrdv2.intel.com/v1/dl/getContent/773614



Kai Huang (8):
  x86/virt/tdx: Rename 'struct tdx_tdmr_sysinfo' to reflect the spec
    better
  x86/virt/tdx: Rework TD_SYSINFO_MAP to support build-time verification
  x86/virt/tdx: Prepare to support reading other global metadata fields
  x86/virt/tdx: Refine a comment to reflect the latest TDX spec
  x86/virt/tdx: Start to track all global metadata in one structure
  x86/virt/tdx: Print TDX module version
  x86/virt/tdx: Require the module to assert it has the NO_RBP_MOD
    mitigation
  x86/virt/tdx: Reduce TDMR's reserved areas by using CMRs to find
    memory holes

 arch/x86/virt/vmx/tdx/tdx.c | 282 +++++++++++++++++++++++++++---------
 arch/x86/virt/vmx/tdx/tdx.h |  85 ++++++++++-
 2 files changed, 294 insertions(+), 73 deletions(-)


base-commit: a6f489fee2633f8595934a850981bd4284abdbba
-- 
2.46.0

[PATCH v4 1/8] x86/virt/tdx: Rename 'struct tdx_tdmr_sysinfo' to reflect the spec better

[Kai Huang]

The TDX module provides a set of "Global Metadata Fields".  They report
things like TDX module version, supported features, and fields related
to create/run TDX guests and so on.

TDX organizes those metadata fields by "Classes" based on the meaning of
those fields.  E.g., for now the kernel only reads "TD Memory Region"
(TDMR) related fields for module initialization.  Those fields are
defined under class "TDMR Info".

There are both immediate needs to read more metadata fields for module
initialization and near-future needs for other kernel components like
KVM to run TDX guests.  To meet all those requirements, the idea is the
TDX host core-kernel to provide a centralized, canonical, and read-only
structure for the global metadata that comes out from the TDX module for
all kernel components to use.

More specifically, the target is to end up with something like:

       struct tdx_sys_info {
	       struct tdx_sys_info_classA a;
	       struct tdx_sys_info_classB b;
	       ...
       };

Currently the kernel organizes all fields under "TDMR Info" class in
'struct tdx_tdmr_sysinfo'.  To prepare for the above target, rename the
structure to 'struct tdx_sys_info_tdmr' to follow the class name better.

No functional change intended.

Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---

v3 -> v4:
 - "global metadata fields" -> "Global Metadata Fields" - Ardian.
 - "Class"es -> "Classes" - Ardian.
 - Add tag from Ardian and Dan.

v2 -> v3:
 - Split out as a separate patch and place it at beginning:

   https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m8fec7c429242d640cf5e756eb68e3b822e6dff8b
 
 - Rename to 'struct tdx_sys_info_tdmr':

   https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#md73dd9b02a492acf4a6facae63e8d030e320967d
   https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m8fec7c429242d640cf5e756eb68e3b822e6dff8b


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

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4e2b2e2ac9f9..e979bf442929 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -272,7 +272,7 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
 
 static int read_sys_metadata_field16(u64 field_id,
 				     int offset,
-				     struct tdx_tdmr_sysinfo *ts)
+				     struct tdx_sys_info_tdmr *ts)
 {
 	u16 *ts_member = ((void *)ts) + offset;
 	u64 tmp;
@@ -298,9 +298,9 @@ struct field_mapping {
 
 #define TD_SYSINFO_MAP(_field_id, _offset) \
 	{ .field_id = MD_FIELD_ID_##_field_id,	   \
-	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _offset) }
+	  .offset   = offsetof(struct tdx_sys_info_tdmr, _offset) }
 
-/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
+/* Map TD_SYSINFO fields into 'struct tdx_sys_info_tdmr': */
 static const struct field_mapping fields[] = {
 	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs),
 	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
@@ -309,16 +309,16 @@ static const struct field_mapping fields[] = {
 	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
 };
 
-static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
+static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
 {
 	int ret;
 	int i;
 
-	/* Populate 'tdmr_sysinfo' fields using the mapping structure above: */
+	/* Populate 'sysinfo_tdmr' fields using the mapping structure above: */
 	for (i = 0; i < ARRAY_SIZE(fields); i++) {
 		ret = read_sys_metadata_field16(fields[i].field_id,
 						fields[i].offset,
-						tdmr_sysinfo);
+						sysinfo_tdmr);
 		if (ret)
 			return ret;
 	}
@@ -342,13 +342,13 @@ static int tdmr_size_single(u16 max_reserved_per_tdmr)
 }
 
 static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
-			   struct tdx_tdmr_sysinfo *tdmr_sysinfo)
+			   struct tdx_sys_info_tdmr *sysinfo_tdmr)
 {
 	size_t tdmr_sz, tdmr_array_sz;
 	void *tdmr_array;
 
-	tdmr_sz = tdmr_size_single(tdmr_sysinfo->max_reserved_per_tdmr);
-	tdmr_array_sz = tdmr_sz * tdmr_sysinfo->max_tdmrs;
+	tdmr_sz = tdmr_size_single(sysinfo_tdmr->max_reserved_per_tdmr);
+	tdmr_array_sz = tdmr_sz * sysinfo_tdmr->max_tdmrs;
 
 	/*
 	 * To keep things simple, allocate all TDMRs together.
@@ -367,7 +367,7 @@ static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
 	 * at a given index in the TDMR list.
 	 */
 	tdmr_list->tdmr_sz = tdmr_sz;
-	tdmr_list->max_tdmrs = tdmr_sysinfo->max_tdmrs;
+	tdmr_list->max_tdmrs = sysinfo_tdmr->max_tdmrs;
 	tdmr_list->nr_consumed_tdmrs = 0;
 
 	return 0;
@@ -921,11 +921,11 @@ static int tdmrs_populate_rsvd_areas_all(struct tdmr_info_list *tdmr_list,
 /*
  * Construct a list of TDMRs on the preallocated space in @tdmr_list
  * to cover all TDX memory regions in @tmb_list based on the TDX module
- * TDMR global information in @tdmr_sysinfo.
+ * TDMR global information in @sysinfo_tdmr.
  */
 static int construct_tdmrs(struct list_head *tmb_list,
 			   struct tdmr_info_list *tdmr_list,
-			   struct tdx_tdmr_sysinfo *tdmr_sysinfo)
+			   struct tdx_sys_info_tdmr *sysinfo_tdmr)
 {
 	int ret;
 
@@ -934,12 +934,12 @@ static int construct_tdmrs(struct list_head *tmb_list,
 		return ret;
 
 	ret = tdmrs_set_up_pamt_all(tdmr_list, tmb_list,
-			tdmr_sysinfo->pamt_entry_size);
+			sysinfo_tdmr->pamt_entry_size);
 	if (ret)
 		return ret;
 
 	ret = tdmrs_populate_rsvd_areas_all(tdmr_list, tmb_list,
-			tdmr_sysinfo->max_reserved_per_tdmr);
+			sysinfo_tdmr->max_reserved_per_tdmr);
 	if (ret)
 		tdmrs_free_pamt_all(tdmr_list);
 
@@ -1098,7 +1098,7 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list)
 
 static int init_tdx_module(void)
 {
-	struct tdx_tdmr_sysinfo tdmr_sysinfo;
+	struct tdx_sys_info_tdmr sysinfo_tdmr;
 	int ret;
 
 	/*
@@ -1117,17 +1117,17 @@ static int init_tdx_module(void)
 	if (ret)
 		goto out_put_tdxmem;
 
-	ret = get_tdx_tdmr_sysinfo(&tdmr_sysinfo);
+	ret = get_tdx_sys_info_tdmr(&sysinfo_tdmr);
 	if (ret)
 		goto err_free_tdxmem;
 
 	/* Allocate enough space for constructing TDMRs */
-	ret = alloc_tdmr_list(&tdx_tdmr_list, &tdmr_sysinfo);
+	ret = alloc_tdmr_list(&tdx_tdmr_list, &sysinfo_tdmr);
 	if (ret)
 		goto err_free_tdxmem;
 
 	/* Cover all TDX-usable memory regions in TDMRs */
-	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &tdmr_sysinfo);
+	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &sysinfo_tdmr);
 	if (ret)
 		goto err_free_tdmrs;
 
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index b701f69485d3..148f9b4d1140 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -100,7 +100,7 @@ struct tdx_memblock {
 };
 
 /* "TDMR info" part of "Global Scope Metadata" for constructing TDMRs */
-struct tdx_tdmr_sysinfo {
+struct tdx_sys_info_tdmr {
 	u16 max_tdmrs;
 	u16 max_reserved_per_tdmr;
 	u16 pamt_entry_size[TDX_PS_NR];
-- 
2.46.0

[PATCH v4 2/8] x86/virt/tdx: Rework TD_SYSINFO_MAP to support build-time verification

[Kai Huang]

Dan noticed [1] that read_sys_metadata_field16() has a runtime warning
to validate that the metadata field size matches the passed in buffer
size.  In turns out that all the information to perform that validation
is available at build time.  Rework TD_SYSINFO_MAP() to stop providing
runtime data to read_sys_metadata_field16() and instead just pass typed
fields to read_sys_metadata_field16() and let the compiler catch any
mismatches.  Rename TD_SYSINFO_MAP() to READ_SYS_INFO() since it no
longer stores the mapping of metadata field ID and the structure member
offset.

For now READ_SYS_INFO() is only used in get_tdx_sys_info_tdmr().  Future
changes will need to read other metadata fields that are organized in
different structures.  Do #undef READ_SYS_INFO() after use so the same
pattern can be used for reading other metadata fields.  To avoid needing
to duplicate build-time verification in each READ_SYS_INFO() in future
changes, add a wrapper macro to do build-time verification and call it
from READ_SYS_INFO().

The READ_SYS_INFO() has a couple quirks for readability.  It requires
the function that uses it to define a local variable @ret to carry the
error code and set the initial value to 0.  It also hard-codes the
variable name of the structure pointer used in the function, but it is
less code, build-time verifiable, and the same readability as the former
'struct field_mapping' approach.

Link: http://lore.kernel.org/66b16121c48f4_4fc729424@dwillia2-xfh.jf.intel.com.notmuch [1]
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v3 -> v4:
 - Rename TD_SYSINFO_MAP() to READ_SYS_INFO() - Ardian.
 - #undef READ_SYS_INFO() - Ardian.
 - Rewrite changelog based on text from Dan, with some clarification
   around using READ_SYS_INFO() and #undef it.
 - Move the BUILD_BUG_ON() out of read_sys_metadata_field16() - Dan.
 - Use permalink in changelog - Dan.

v2 -> v3:
 - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP().

---
 arch/x86/virt/vmx/tdx/tdx.c | 58 ++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 37 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index e979bf442929..2f7e4abc1bb9 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -270,60 +270,44 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
 	return 0;
 }
 
-static int read_sys_metadata_field16(u64 field_id,
-				     int offset,
-				     struct tdx_sys_info_tdmr *ts)
+static int __read_sys_metadata_field16(u64 field_id, u16 *val)
 {
-	u16 *ts_member = ((void *)ts) + offset;
 	u64 tmp;
 	int ret;
 
-	if (WARN_ON_ONCE(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
-			MD_FIELD_ID_ELE_SIZE_16BIT))
-		return -EINVAL;
-
 	ret = read_sys_metadata_field(field_id, &tmp);
 	if (ret)
 		return ret;
 
-	*ts_member = tmp;
+	*val = tmp;
 
 	return 0;
 }
 
-struct field_mapping {
-	u64 field_id;
-	int offset;
-};
-
-#define TD_SYSINFO_MAP(_field_id, _offset) \
-	{ .field_id = MD_FIELD_ID_##_field_id,	   \
-	  .offset   = offsetof(struct tdx_sys_info_tdmr, _offset) }
-
-/* Map TD_SYSINFO fields into 'struct tdx_sys_info_tdmr': */
-static const struct field_mapping fields[] = {
-	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs),
-	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
-	TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]),
-	TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]),
-	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
-};
+#define read_sys_metadata_field16(_field_id, _val)		\
+({								\
+	BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(_field_id) !=	\
+			MD_FIELD_ID_ELE_SIZE_16BIT);		\
+	__read_sys_metadata_field16(_field_id, _val);		\
+})
 
 static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
 {
-	int ret;
-	int i;
+	int ret = 0;
 
-	/* Populate 'sysinfo_tdmr' fields using the mapping structure above: */
-	for (i = 0; i < ARRAY_SIZE(fields); i++) {
-		ret = read_sys_metadata_field16(fields[i].field_id,
-						fields[i].offset,
-						sysinfo_tdmr);
-		if (ret)
-			return ret;
-	}
+#define READ_SYS_INFO(_field_id, _member)				\
+	ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
+					&sysinfo_tdmr->_member)
 
-	return 0;
+	READ_SYS_INFO(MAX_TDMRS,	     max_tdmrs);
+	READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
+	READ_SYS_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
+	READ_SYS_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
+	READ_SYS_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
+
+#undef READ_SYS_INFO
+
+	return ret;
 }
 
 /* Calculate the actual TDMR size */
-- 
2.46.0

[PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Kai Huang]

The TDX module provides a set of "Global Metadata Fields".  They report
things like TDX module version, supported features, and fields related
to create/run TDX guests and so on.  TDX supports 8/16/32/64 bits
metadata field element sizes.  For a given metadata field, the element
size is encoded in the metadata field ID.

For now the kernel only reads "TD Memory Region" (TDMR) related metadata
fields and they are all 16-bit.  Thus the kernel only has one primitive
__read_sys_metadata_field16() to read 16-bit metadata field and the
macro, read_sys_metadata_field16(), which does additional build-time
check of the field ID makes sure the field is indeed 16-bit.

Future changes will need to read more metadata fields with different
element sizes.  Choose to provide one primitive for each element size to
support that.  Similar to the build_mmio_read() macro, reimplement the
body of __read_sys_metadata_field16() as a macro build_sysmd_read(_size)
in size-agnostic way, so it can be used to generate one primitive for
each element size:

  build_sysmd_read(8)
  build_sysmd_read(16)
  ..

Also extend read_sys_metadata_field16() take the '_size' as argument
(and rename it to read_sys_metadata_field() to make it size-agnostic) to
allow the READ_SYS_INFO() macro to choose which primitive to use.

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

v3 -> v4:
 - Change to use one primitive for each element size, similar to
   build_mmio_read() macro - Dan.
 - Rewrite changelog based on the new code.
 - "global metadata fields" -> "Global Metadata Fields" - Ardian.

v2 -> v3:
 - Rename read_sys_metadata_field() to tdh_sys_rd() so the former can be
   used as the high level wrapper.  Get rid of "stbuf_" prefix since
   people don't like it.
 
 - Rewrite after removing 'struct field_mapping' and reimplementing
   TD_SYSINFO_MAP().
 
---
 arch/x86/virt/vmx/tdx/tdx.c | 40 ++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 2f7e4abc1bb9..b5c8dde9caf0 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -250,7 +250,7 @@ static int build_tdx_memlist(struct list_head *tmb_list)
 	return ret;
 }
 
-static int read_sys_metadata_field(u64 field_id, u64 *data)
+static int tdh_sys_rd(u64 field_id, u64 *data)
 {
 	struct tdx_module_args args = {};
 	int ret;
@@ -270,25 +270,29 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
 	return 0;
 }
 
-static int __read_sys_metadata_field16(u64 field_id, u16 *val)
-{
-	u64 tmp;
-	int ret;
-
-	ret = read_sys_metadata_field(field_id, &tmp);
-	if (ret)
-		return ret;
-
-	*val = tmp;
-
-	return 0;
+#define build_sysmd_read(_size)							\
+static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
+{										\
+	u64 tmp;								\
+	int ret;								\
+										\
+	ret = tdh_sys_rd(field_id, &tmp);					\
+	if (ret)								\
+		return ret;							\
+										\
+	*val = tmp;								\
+										\
+	return 0;								\
 }
 
-#define read_sys_metadata_field16(_field_id, _val)		\
+build_sysmd_read(16)
+
+#define read_sys_metadata_field(_field_id, _val, _size)		\
 ({								\
 	BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(_field_id) !=	\
-			MD_FIELD_ID_ELE_SIZE_16BIT);		\
-	__read_sys_metadata_field16(_field_id, _val);		\
+			MD_FIELD_ID_ELE_SIZE_##_size##BIT);	\
+	BUILD_BUG_ON(_size != sizeof(*_val) * 8);		\
+	__read_sys_metadata_field##_size(_field_id, _val);	\
 })
 
 static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
@@ -296,8 +300,8 @@ static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
 	int ret = 0;
 
 #define READ_SYS_INFO(_field_id, _member)				\
-	ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
-					&sysinfo_tdmr->_member)
+	ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
+					&sysinfo_tdmr->_member, 16)
 
 	READ_SYS_INFO(MAX_TDMRS,	     max_tdmrs);
 	READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
-- 
2.46.0

[PATCH v4 4/8] x86/virt/tdx: Refine a comment to reflect the latest TDX spec

[Kai Huang]

The old versions of "Intel TDX Module v1.5 ABI Specification" contain
the definitions of all global metadata field IDs directly in a table.

However, the latest spec moves those definitions to a dedicated
'global_metadata.json' file as part of a new (separate) "Intel TDX
Module v1.5 ABI definitions" [1].

Update the comment to reflect this.

[1]: https://cdrdv2.intel.com/v1/dl/getContent/795381

Reported-by: Nikolay Borisov <nik.borisov@suse.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
---
 arch/x86/virt/vmx/tdx/tdx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 148f9b4d1140..38f8656162e3 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -29,7 +29,7 @@
 /*
  * Global scope metadata field ID.
  *
- * See Table "Global Scope Metadata", TDX module 1.5 ABI spec.
+ * See the "global_metadata.json" in the "TDX 1.5 ABI definitions".
  */
 #define MD_FIELD_ID_MAX_TDMRS			0x9100000100000008ULL
 #define MD_FIELD_ID_MAX_RESERVED_PER_TDMR	0x9100000100000009ULL
-- 
2.46.0

[PATCH v4 5/8] x86/virt/tdx: Start to track all global metadata in one structure

[Kai Huang]

The TDX module provides a set of "Global Metadata Fields".  They report
things like TDX module version, supported features, and fields related
to create/run TDX guests and so on.

Currently the kernel only reads "TD Memory Region" (TDMR) related fields
for module initialization.  There are immediate needs which require the
TDX module initialization to read more global metadata including module
version, supported features and "Convertible Memory Regions" (CMRs).

Also, KVM will need to read more metadata fields to support baseline TDX
guests.  In the longer term, other TDX features like TDX Connect (which
supports assigning trusted IO devices to TDX guest) may also require
other kernel components such as pci/vt-d to access global metadata.

To meet all those requirements, the idea is the TDX host core-kernel to
to provide a centralized, canonical, and read-only structure for the
global metadata that comes out from the TDX module for all kernel
components to use.

As the first step, introduce a new 'struct tdx_sys_info' to track all
global metadata fields.

TDX categories global metadata fields into different "Classes".  E.g.,
the TDMR related fields are under class "TDMR Info".  Instead of making
'struct tdx_sys_info' a plain structure to contain all metadata fields,
organize them in smaller structures based on the "Class".

This allows those metadata fields to be used in finer granularity thus
makes the code more clear.  E.g., the construct_tdmr() can just take the
structure which contains "TDMR Info" metadata fields.

Add a new function get_tdx_sys_info() as the placeholder to read all
metadata fields, and call it at the beginning of init_tdx_module().  For
now it only calls get_tdx_sys_info_tdmr() to read TDMR related fields.

Note there is a functional change: get_tdx_sys_info_tdmr() is moved from
after build_tdx_memlist() to before it, but it is fine to do so.

Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
---

v3 -> v4:
 - "global metadata fields" -> "Global Metadata Fields" - Ardian.
 - "Class"es -> "Classes" - Ardian.
 - Add tag from Ardian.

v2 -> v3:
 - Split out the part to rename 'struct tdx_tdmr_sysinfo' to 'struct
   tdx_sys_info_tdmr'.


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

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index b5c8dde9caf0..44ef74d1fafb 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -314,6 +314,11 @@ static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
 	return ret;
 }
 
+static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
+{
+	return get_tdx_sys_info_tdmr(&sysinfo->tdmr);
+}
+
 /* Calculate the actual TDMR size */
 static int tdmr_size_single(u16 max_reserved_per_tdmr)
 {
@@ -1086,9 +1091,13 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list)
 
 static int init_tdx_module(void)
 {
-	struct tdx_sys_info_tdmr sysinfo_tdmr;
+	struct tdx_sys_info sysinfo;
 	int ret;
 
+	ret = get_tdx_sys_info(&sysinfo);
+	if (ret)
+		return ret;
+
 	/*
 	 * To keep things simple, assume that all TDX-protected memory
 	 * will come from the page allocator.  Make sure all pages in the
@@ -1105,17 +1114,13 @@ static int init_tdx_module(void)
 	if (ret)
 		goto out_put_tdxmem;
 
-	ret = get_tdx_sys_info_tdmr(&sysinfo_tdmr);
-	if (ret)
-		goto err_free_tdxmem;
-
 	/* Allocate enough space for constructing TDMRs */
-	ret = alloc_tdmr_list(&tdx_tdmr_list, &sysinfo_tdmr);
+	ret = alloc_tdmr_list(&tdx_tdmr_list, &sysinfo.tdmr);
 	if (ret)
 		goto err_free_tdxmem;
 
 	/* Cover all TDX-usable memory regions in TDMRs */
-	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &sysinfo_tdmr);
+	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &sysinfo.tdmr);
 	if (ret)
 		goto err_free_tdmrs;
 
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 38f8656162e3..b9a89da39e1e 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -80,6 +80,35 @@ struct tdmr_info {
 	DECLARE_FLEX_ARRAY(struct tdmr_reserved_area, reserved_areas);
 } __packed __aligned(TDMR_INFO_ALIGNMENT);
 
+/*
+ * Data structures for "Global Scope Metadata".
+ *
+ * TDX global metadata fields are categorized by "Classes".  See the
+ * "global_metadata.json" in the "TDX 1.5 ABI Definitions".
+ *
+ * 'struct tdx_sys_info' is the main structure to contain all metadata
+ * used by the kernel.  It contains sub-structures with each reflecting
+ * the "Class" in the 'global_metadata.json'.
+ *
+ * Note the structure name may not exactly follow the name of the
+ * "Class" in the TDX spec, but the comment of that structure always
+ * reflect that.
+ *
+ * Also note not all metadata fields in each class are defined, only
+ * those used by the kernel are.
+ */
+
+/* Class "TDMR info" */
+struct tdx_sys_info_tdmr {
+	u16 max_tdmrs;
+	u16 max_reserved_per_tdmr;
+	u16 pamt_entry_size[TDX_PS_NR];
+};
+
+struct tdx_sys_info {
+	struct tdx_sys_info_tdmr tdmr;
+};
+
 /*
  * Do not put any hardware-defined TDX structure representations below
  * this comment!
@@ -99,13 +128,6 @@ struct tdx_memblock {
 	int nid;
 };
 
-/* "TDMR info" part of "Global Scope Metadata" for constructing TDMRs */
-struct tdx_sys_info_tdmr {
-	u16 max_tdmrs;
-	u16 max_reserved_per_tdmr;
-	u16 pamt_entry_size[TDX_PS_NR];
-};
-
 /* Warn if kernel has less than TDMR_NR_WARN TDMRs after allocation */
 #define TDMR_NR_WARN 4
 
-- 
2.46.0

[PATCH v4 6/8] x86/virt/tdx: Print TDX module version

[Kai Huang]

Currently the kernel doesn't print any TDX module version information.
In practice such information is useful, especially to the developers.

For instance:

1) When something goes wrong around using TDX, the module version is
   normally the first information the users want to know [1].

2) After initializing TDX module, the users want to quickly know module
   version to see whether the loaded module is the expected one.

Dump TDX module version.  The actual dmesg will look like:

  virt/tdx: Initializing TDX module: 1.5.00.00.0481 (build_date 20230323).

And dump right after reading global metadata, so that this information is
printed no matter whether module initialization fails or not.

Link: https://lore.kernel.org/lkml/4b3adb59-50ea-419e-ad02-e19e8ca20dee@intel.com/ [1]
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
v3 -> v4:
 - Omit dumping TDX_FEATURES0 - Dan.
 - As a result, move TDX_FEATURES0 related code out to NO_MOD_RBP patch.
 - Update changelog accordingly.
 - Simplify changelog for the use case 2).
 - Use permalink - Dan.

v2 -> v3:
 - 'struct tdx_sysinfo_module_info' -> 'struct tdx_sys_info_features'
 - 'struct tdx_sysinfo_module_version' -> 'struct tdx_sys_info_version'
 - Remove the 'sys_attributes' and the check of debug/production module.

 https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#md73dd9b02a492acf4a6facae63e8d030e320967d



---
 arch/x86/virt/vmx/tdx/tdx.c | 51 +++++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h | 20 ++++++++++++++-
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 44ef74d1fafb..d599aaaa2730 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -286,6 +286,7 @@ static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
 }
 
 build_sysmd_read(16)
+build_sysmd_read(32)
 
 #define read_sys_metadata_field(_field_id, _val, _size)		\
 ({								\
@@ -295,6 +296,26 @@ build_sysmd_read(16)
 	__read_sys_metadata_field##_size(_field_id, _val);	\
 })
 
+static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version)
+{
+	int ret = 0;
+
+#define READ_SYS_INFO(_field_id, _member, _size)			\
+	ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
+					&sysinfo_version->_member, _size)
+
+	READ_SYS_INFO(MAJOR_VERSION,    major,      16);
+	READ_SYS_INFO(MINOR_VERSION,    minor,      16);
+	READ_SYS_INFO(UPDATE_VERSION,   update,     16);
+	READ_SYS_INFO(INTERNAL_VERSION, internal,   16);
+	READ_SYS_INFO(BUILD_NUM,	build_num,  16);
+	READ_SYS_INFO(BUILD_DATE,	build_date, 32);
+
+#undef READ_SYS_INFO
+
+	return ret;
+}
+
 static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
 {
 	int ret = 0;
@@ -316,9 +337,37 @@ static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
 
 static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
 {
+	int ret;
+
+	ret = get_tdx_sys_info_version(&sysinfo->version);
+	if (ret)
+		return ret;
+
 	return get_tdx_sys_info_tdmr(&sysinfo->tdmr);
 }
 
+static void print_sys_info_version(struct tdx_sys_info_version *version)
+{
+	/*
+	 * TDX module version encoding:
+	 *
+	 *   <major>.<minor>.<update>.<internal>.<build_num>
+	 *
+	 * When printed as text, <major> and <minor> are 1-digit,
+	 * <update> and <internal> are 2-digits and <build_num>
+	 * is 4-digits.
+	 */
+	pr_info("Initializing TDX module: %u.%u.%02u.%02u.%04u (build_date %u).\n",
+			version->major, version->minor,	version->update,
+			version->internal, version->build_num,
+			version->build_date);
+}
+
+static void print_basic_sys_info(struct tdx_sys_info *sysinfo)
+{
+	print_sys_info_version(&sysinfo->version);
+}
+
 /* Calculate the actual TDMR size */
 static int tdmr_size_single(u16 max_reserved_per_tdmr)
 {
@@ -1098,6 +1147,8 @@ static int init_tdx_module(void)
 	if (ret)
 		return ret;
 
+	print_basic_sys_info(&sysinfo);
+
 	/*
 	 * To keep things simple, assume that all TDX-protected memory
 	 * will come from the page allocator.  Make sure all pages in the
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index b9a89da39e1e..a8d0ab3e5acd 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -31,6 +31,12 @@
  *
  * See the "global_metadata.json" in the "TDX 1.5 ABI definitions".
  */
+#define MD_FIELD_ID_BUILD_DATE			0x8800000200000001ULL
+#define MD_FIELD_ID_BUILD_NUM			0x8800000100000002ULL
+#define MD_FIELD_ID_MINOR_VERSION		0x0800000100000003ULL
+#define MD_FIELD_ID_MAJOR_VERSION		0x0800000100000004ULL
+#define MD_FIELD_ID_UPDATE_VERSION		0x0800000100000005ULL
+#define MD_FIELD_ID_INTERNAL_VERSION		0x0800000100000006ULL
 #define MD_FIELD_ID_MAX_TDMRS			0x9100000100000008ULL
 #define MD_FIELD_ID_MAX_RESERVED_PER_TDMR	0x9100000100000009ULL
 #define MD_FIELD_ID_PAMT_4K_ENTRY_SIZE		0x9100000100000010ULL
@@ -54,6 +60,7 @@
 		(((_field_id) & GENMASK_ULL(33, 32)) >> 32)
 
 #define MD_FIELD_ID_ELE_SIZE_16BIT	1
+#define MD_FIELD_ID_ELE_SIZE_32BIT	2
 
 struct tdmr_reserved_area {
 	u64 offset;
@@ -98,6 +105,16 @@ struct tdmr_info {
  * those used by the kernel are.
  */
 
+/* Class "TDX Module Version" */
+struct tdx_sys_info_version {
+	u16 major;
+	u16 minor;
+	u16 update;
+	u16 internal;
+	u16 build_num;
+	u32 build_date;
+};
+
 /* Class "TDMR info" */
 struct tdx_sys_info_tdmr {
 	u16 max_tdmrs;
@@ -106,7 +123,8 @@ struct tdx_sys_info_tdmr {
 };
 
 struct tdx_sys_info {
-	struct tdx_sys_info_tdmr tdmr;
+	struct tdx_sys_info_version	version;
+	struct tdx_sys_info_tdmr	tdmr;
 };
 
 /*
-- 
2.46.0

[PATCH v4 7/8] x86/virt/tdx: Require the module to assert it has the NO_RBP_MOD mitigation

[Kai Huang]

Old TDX modules can clobber RBP in the TDH.VP.ENTER SEAMCALL.  However
RBP is used as frame pointer in the x86_64 calling convention, and
clobbering RBP could result in bad things like being unable to unwind
the stack if any non-maskable exceptions (NMI, #MC etc) happens in that
gap.

A new "NO_RBP_MOD" feature was introduced to more recent TDX modules to
not clobber RBP.  This feature is reported in the TDX_FEATURES0 global
metadata field via bit 18.

Don't initialize the TDX module if this feature is not supported [1].

Link: https://lore.kernel.org/all/fc0e8ab7-86d4-4428-be31-82e1ece6dd21@intel.com/ [1]
Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---

v3 -> v4:
 - Move reading TDX_FEATURES0 code to this patch.
 - Change patch title and use permalink - Dan.

 Hi Dan, Ardian, Nikolay,

 The code to read TDX_FEATURES0 was not included in this patch when you
 gave your tag.  I didn't remove them.  Please let me know if you want
 me to remove your tag.  Thanks!

v2 -> v3:
 - check_module_compatibility() -> check_features().
 - Improve error message.

 https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#md9e2eeef927838cbf20d7b361cdbea518b8aec50


---
 arch/x86/virt/vmx/tdx/tdx.c | 37 +++++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h | 17 +++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index d599aaaa2730..cd8cca5139ac 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -287,6 +287,7 @@ static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
 
 build_sysmd_read(16)
 build_sysmd_read(32)
+build_sysmd_read(64)
 
 #define read_sys_metadata_field(_field_id, _val, _size)		\
 ({								\
@@ -296,6 +297,21 @@ build_sysmd_read(32)
 	__read_sys_metadata_field##_size(_field_id, _val);	\
 })
 
+static int get_tdx_sys_info_features(struct tdx_sys_info_features *sysinfo_features)
+{
+	int ret = 0;
+
+#define READ_SYS_INFO(_field_id, _member)				\
+	ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
+					&sysinfo_features->_member, 64)
+
+	READ_SYS_INFO(TDX_FEATURES0, tdx_features0);
+
+#undef READ_SYS_INFO
+
+	return ret;
+}
+
 static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version)
 {
 	int ret = 0;
@@ -339,6 +355,10 @@ static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
 {
 	int ret;
 
+	ret = get_tdx_sys_info_features(&sysinfo->features);
+	if (ret)
+		return ret;
+
 	ret = get_tdx_sys_info_version(&sysinfo->version);
 	if (ret)
 		return ret;
@@ -368,6 +388,18 @@ static void print_basic_sys_info(struct tdx_sys_info *sysinfo)
 	print_sys_info_version(&sysinfo->version);
 }
 
+static int check_features(struct tdx_sys_info *sysinfo)
+{
+	u64 tdx_features0 = sysinfo->features.tdx_features0;
+
+	if (!(tdx_features0 & TDX_FEATURES0_NO_RBP_MOD)) {
+		pr_err("frame pointer (RBP) clobber bug present, upgrade TDX module\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /* Calculate the actual TDMR size */
 static int tdmr_size_single(u16 max_reserved_per_tdmr)
 {
@@ -1149,6 +1181,11 @@ static int init_tdx_module(void)
 
 	print_basic_sys_info(&sysinfo);
 
+	/* Check whether the kernel can support this module */
+	ret = check_features(&sysinfo);
+	if (ret)
+		return ret;
+
 	/*
 	 * To keep things simple, assume that all TDX-protected memory
 	 * will come from the page allocator.  Make sure all pages in the
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index a8d0ab3e5acd..9314f6ecbcb5 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -31,6 +31,7 @@
  *
  * See the "global_metadata.json" in the "TDX 1.5 ABI definitions".
  */
+#define MD_FIELD_ID_TDX_FEATURES0		0x0A00000300000008ULL
 #define MD_FIELD_ID_BUILD_DATE			0x8800000200000001ULL
 #define MD_FIELD_ID_BUILD_NUM			0x8800000100000002ULL
 #define MD_FIELD_ID_MINOR_VERSION		0x0800000100000003ULL
@@ -61,6 +62,7 @@
 
 #define MD_FIELD_ID_ELE_SIZE_16BIT	1
 #define MD_FIELD_ID_ELE_SIZE_32BIT	2
+#define MD_FIELD_ID_ELE_SIZE_64BIT	3
 
 struct tdmr_reserved_area {
 	u64 offset;
@@ -105,6 +107,20 @@ struct tdmr_info {
  * those used by the kernel are.
  */
 
+/*
+ * Class "TDX Module Info".
+ *
+ * This class also contains other fields like SYS_ATTRIBUTES and the
+ * NUM_TDX_FEATURES.  For now only TDX_FEATURES0 is needed, but still
+ * keep the structure to follow the spec (and for future extension).
+ */
+struct tdx_sys_info_features {
+	u64 tdx_features0;
+};
+
+/* Bit definitions of TDX_FEATURES0 metadata field */
+#define TDX_FEATURES0_NO_RBP_MOD	_BITULL(18)
+
 /* Class "TDX Module Version" */
 struct tdx_sys_info_version {
 	u16 major;
@@ -123,6 +139,7 @@ struct tdx_sys_info_tdmr {
 };
 
 struct tdx_sys_info {
+	struct tdx_sys_info_features	features;
 	struct tdx_sys_info_version	version;
 	struct tdx_sys_info_tdmr	tdmr;
 };
-- 
2.46.0

[PATCH v4 8/8] x86/virt/tdx: Reduce TDMR's reserved areas by using CMRs to find memory holes

[Kai Huang]

A TDX module initialization failure was reported on a Emerald Rapids
platform:

  virt/tdx: initialization failed: TDMR [0x0, 0x80000000): reserved areas exhausted.
  virt/tdx: module initialization failed (-28)

As part of initializing the TDX module, the kernel informs the TDX
module of all "TDX-usable memory regions" using an array of TDX defined
structure "TD Memory Region" (TDMR).  Each TDMR must be in 1GB aligned
and in 1GB granularity, and all "non-TDX-usable memory holes" within a
given TDMR are marked as "reserved areas".  The TDX module reports a
maximum number of reserved areas that can be supported per TDMR (16).

The kernel builds the "TDX-usable memory regions" based on memblocks
(which reflects e820), and uses this list to find all "reserved areas"
for each TDMR.

It turns out that the kernel's view of memory holes is too fine grained
and sometimes exceeds the number of holes that the TDX module can track
per TDMR [1], resulting in the above failure.

Thankfully the module also lists memory that is potentially convertible
in a list of "Convertible Memory Regions" (CMRs).  That coarser grained
CMR list tends to track usable memory in the memory map even if it might
be reserved for host usage like 'ACPI data' [2].

Use that list to relax what the kernel considers unusable memory.  If it
falls in a CMR no need to instantiate a hole, and rely on the fact that
kernel will keep what it considers 'reserved' out of the page allocator.

Also dump the CMRs in dmesg.  They are helpful when something goes wrong
around "constructing the TDMRs and configuring the TDX module with
them".  Note there are no existing userspace tools that the user can get
CMRs since they can only be read via SEAMCALL (no CPUID, MSR etc).

[1] BIOS-E820 table of the problematic platform:

  BIOS-e820: [mem 0x0000000000000000-0x000000000009efff] usable
  BIOS-e820: [mem 0x000000000009f000-0x00000000000fffff] reserved
  BIOS-e820: [mem 0x0000000000100000-0x000000005d168fff] usable
  BIOS-e820: [mem 0x000000005d169000-0x000000005d22afff] ACPI data
  BIOS-e820: [mem 0x000000005d22b000-0x000000005d3cefff] usable
  BIOS-e820: [mem 0x000000005d3cf000-0x000000005d469fff] reserved
  BIOS-e820: [mem 0x000000005d46a000-0x000000005e5b2fff] usable
  BIOS-e820: [mem 0x000000005e5b3000-0x000000005e5c2fff] reserved
  BIOS-e820: [mem 0x000000005e5c3000-0x000000005e5d2fff] usable
  BIOS-e820: [mem 0x000000005e5d3000-0x000000005e5e4fff] reserved
  BIOS-e820: [mem 0x000000005e5e5000-0x000000005eb57fff] usable
  BIOS-e820: [mem 0x000000005eb58000-0x0000000061357fff] ACPI NVS
  BIOS-e820: [mem 0x0000000061358000-0x000000006172afff] usable
  BIOS-e820: [mem 0x000000006172b000-0x0000000061794fff] ACPI data
  BIOS-e820: [mem 0x0000000061795000-0x00000000617fefff] usable
  BIOS-e820: [mem 0x00000000617ff000-0x0000000061912fff] ACPI data
  BIOS-e820: [mem 0x0000000061913000-0x0000000061998fff] usable
  BIOS-e820: [mem 0x0000000061999000-0x00000000619dffff] ACPI data
  BIOS-e820: [mem 0x00000000619e0000-0x00000000619e1fff] usable
  BIOS-e820: [mem 0x00000000619e2000-0x00000000619e9fff] reserved
  BIOS-e820: [mem 0x00000000619ea000-0x0000000061a26fff] usable
  BIOS-e820: [mem 0x0000000061a27000-0x0000000061baefff] ACPI data
  BIOS-e820: [mem 0x0000000061baf000-0x00000000623c2fff] usable
  BIOS-e820: [mem 0x00000000623c3000-0x0000000062471fff] reserved
  BIOS-e820: [mem 0x0000000062472000-0x0000000062823fff] usable
  BIOS-e820: [mem 0x0000000062824000-0x0000000063a24fff] reserved
  BIOS-e820: [mem 0x0000000063a25000-0x0000000063d57fff] usable
  BIOS-e820: [mem 0x0000000063d58000-0x0000000064157fff] reserved
  BIOS-e820: [mem 0x0000000064158000-0x0000000064158fff] usable
  BIOS-e820: [mem 0x0000000064159000-0x0000000064194fff] reserved
  BIOS-e820: [mem 0x0000000064195000-0x000000006e9cefff] usable
  BIOS-e820: [mem 0x000000006e9cf000-0x000000006eccefff] reserved
  BIOS-e820: [mem 0x000000006eccf000-0x000000006f6fefff] ACPI NVS
  BIOS-e820: [mem 0x000000006f6ff000-0x000000006f7fefff] ACPI data
  BIOS-e820: [mem 0x000000006f7ff000-0x000000006f7fffff] usable
  BIOS-e820: [mem 0x000000006f800000-0x000000008fffffff] reserved
  ......

[2] Convertible Memory Regions of the problematic platform:

  virt/tdx: CMR: [0x100000, 0x6f800000)
  virt/tdx: CMR: [0x100000000, 0x107a000000)
  virt/tdx: CMR: [0x1080000000, 0x207c000000)
  virt/tdx: CMR: [0x2080000000, 0x307c000000)
  virt/tdx: CMR: [0x3080000000, 0x407c000000)

Fixes: dde3b60d572c ("x86/virt/tdx: Designate reserved areas for all TDMRs")
Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v3 -> v4:
 - Trim down changelog - Dan.
 - "must be marked as reserved areas" -> "are marked as reserved areas" - Ardian.
 - Remove all WARN_ON_ONCE() for CMR sanity checks, and clarify in the
   comment that CMRs are verified by MCHECK before it enables TDX so we
   can trust hardware.
 - Change CMR_BASE(i) macro back to just define CMR_BASE and do the
   "+i" in the code.

v2 -> v3:

 - Add the Fixes tag, although this patch depends on previous patches.
 - CMR_BASE0 -> CMR_BASE(_i), CMR_SIZE0 -> CMR_SIZE(_i) to silence the
   build-check error.

v1 -> v2:
 - Change to walk over CMRs directly to find out memory holes, instead
   of walking over TDX memory blocks and explicitly check whether a hole
   is subregion of CMR.  (Chao)
 - Mention any constant macro definitions in global metadata structures
   are TDX architectural. (Binbin)
 - Slightly improve the changelog.


---
 arch/x86/virt/vmx/tdx/tdx.c | 103 ++++++++++++++++++++++++++++++------
 arch/x86/virt/vmx/tdx/tdx.h |  12 +++++
 2 files changed, 99 insertions(+), 16 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index cd8cca5139ac..aac13c3c10f5 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -332,6 +332,58 @@ static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version
 	return ret;
 }
 
+/* Update the @sysinfo_cmr->num_cmrs to trim tail empty CMRs */
+static void trim_empty_tail_cmrs(struct tdx_sys_info_cmr *sysinfo_cmr)
+{
+	int i;
+
+	/*
+	 * The TDX module may report the maximum number of CMRs that
+	 * TDX architecturally supports as the actual number of CMRs,
+	 * despite the latter is smaller.  In this case all the tail
+	 * CMRs will be empty.  Trim them away.
+	 *
+	 * Note MCHECK verifies CMRs before enabling TDX on hardware.
+	 * Skip other sanity checks (e.g., verify CMR is 4KB aligned)
+	 * but trust MCHECK to work properly.  CMRs are printed later
+	 * anyway, and the worst case is module fails to initialize.
+	 */
+	for (i = 0; i < sysinfo_cmr->num_cmrs; i++)
+		if (!sysinfo_cmr->cmr_size[i])
+			break;
+
+	sysinfo_cmr->num_cmrs = i;
+}
+
+static int get_tdx_sys_info_cmr(struct tdx_sys_info_cmr *sysinfo_cmr)
+{
+	int ret = 0;
+	u16 i;
+
+#define READ_SYS_INFO(_field_id, _member, _size)			\
+	ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
+					&sysinfo_cmr->_member, _size)
+
+	READ_SYS_INFO(NUM_CMRS, num_cmrs, 16);
+
+	if (ret)
+		return ret;
+
+	for (i = 0; i < sysinfo_cmr->num_cmrs; i++) {
+		READ_SYS_INFO(CMR_BASE + i, cmr_base[i], 64);
+		READ_SYS_INFO(CMR_SIZE + i, cmr_size[i], 64);
+	}
+
+	if (ret)
+		return ret;
+
+	trim_empty_tail_cmrs(sysinfo_cmr);
+
+#undef READ_SYS_INFO
+
+	return 0;
+}
+
 static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
 {
 	int ret = 0;
@@ -363,6 +415,10 @@ static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
 	if (ret)
 		return ret;
 
+	ret = get_tdx_sys_info_cmr(&sysinfo->cmr);
+	if (ret)
+		return ret;
+
 	return get_tdx_sys_info_tdmr(&sysinfo->tdmr);
 }
 
@@ -383,9 +439,23 @@ static void print_sys_info_version(struct tdx_sys_info_version *version)
 			version->build_date);
 }
 
+static void print_sys_info_cmr(struct tdx_sys_info_cmr *sysinfo_cmr)
+{
+	int i;
+
+	for (i = 0; i < sysinfo_cmr->num_cmrs; i++) {
+		u64 cmr_base = sysinfo_cmr->cmr_base[i];
+		u64 cmr_size = sysinfo_cmr->cmr_size[i];
+
+		pr_info("CMR[%d]: [0x%llx, 0x%llx)\n", i, cmr_base,
+				cmr_base + cmr_size);
+	}
+}
+
 static void print_basic_sys_info(struct tdx_sys_info *sysinfo)
 {
 	print_sys_info_version(&sysinfo->version);
+	print_sys_info_cmr(&sysinfo->cmr);
 }
 
 static int check_features(struct tdx_sys_info *sysinfo)
@@ -821,29 +891,28 @@ static int tdmr_add_rsvd_area(struct tdmr_info *tdmr, int *p_idx, u64 addr,
 }
 
 /*
- * Go through @tmb_list to find holes between memory areas.  If any of
+ * Go through all CMRs in @sysinfo_cmr to find memory holes.  If any of
  * those holes fall within @tdmr, set up a TDMR reserved area to cover
  * the hole.
  */
-static int tdmr_populate_rsvd_holes(struct list_head *tmb_list,
+static int tdmr_populate_rsvd_holes(struct tdx_sys_info_cmr *sysinfo_cmr,
 				    struct tdmr_info *tdmr,
 				    int *rsvd_idx,
 				    u16 max_reserved_per_tdmr)
 {
-	struct tdx_memblock *tmb;
 	u64 prev_end;
-	int ret;
+	int i, ret;
 
 	/*
 	 * Start looking for reserved blocks at the
 	 * beginning of the TDMR.
 	 */
 	prev_end = tdmr->base;
-	list_for_each_entry(tmb, tmb_list, list) {
+	for (i = 0; i < sysinfo_cmr->num_cmrs; i++) {
 		u64 start, end;
 
-		start = PFN_PHYS(tmb->start_pfn);
-		end   = PFN_PHYS(tmb->end_pfn);
+		start = sysinfo_cmr->cmr_base[i];
+		end   = start + sysinfo_cmr->cmr_size[i];
 
 		/* Break if this region is after the TDMR */
 		if (start >= tdmr_end(tdmr))
@@ -944,16 +1013,16 @@ static int rsvd_area_cmp_func(const void *a, const void *b)
 
 /*
  * Populate reserved areas for the given @tdmr, including memory holes
- * (via @tmb_list) and PAMTs (via @tdmr_list).
+ * (via @sysinfo_cmr) and PAMTs (via @tdmr_list).
  */
 static int tdmr_populate_rsvd_areas(struct tdmr_info *tdmr,
-				    struct list_head *tmb_list,
+				    struct tdx_sys_info_cmr *sysinfo_cmr,
 				    struct tdmr_info_list *tdmr_list,
 				    u16 max_reserved_per_tdmr)
 {
 	int ret, rsvd_idx = 0;
 
-	ret = tdmr_populate_rsvd_holes(tmb_list, tdmr, &rsvd_idx,
+	ret = tdmr_populate_rsvd_holes(sysinfo_cmr, tdmr, &rsvd_idx,
 			max_reserved_per_tdmr);
 	if (ret)
 		return ret;
@@ -972,10 +1041,10 @@ static int tdmr_populate_rsvd_areas(struct tdmr_info *tdmr,
 
 /*
  * Populate reserved areas for all TDMRs in @tdmr_list, including memory
- * holes (via @tmb_list) and PAMTs.
+ * holes (via @sysinfo_cmr) and PAMTs.
  */
 static int tdmrs_populate_rsvd_areas_all(struct tdmr_info_list *tdmr_list,
-					 struct list_head *tmb_list,
+					 struct tdx_sys_info_cmr *sysinfo_cmr,
 					 u16 max_reserved_per_tdmr)
 {
 	int i;
@@ -984,7 +1053,7 @@ static int tdmrs_populate_rsvd_areas_all(struct tdmr_info_list *tdmr_list,
 		int ret;
 
 		ret = tdmr_populate_rsvd_areas(tdmr_entry(tdmr_list, i),
-				tmb_list, tdmr_list, max_reserved_per_tdmr);
+				sysinfo_cmr, tdmr_list, max_reserved_per_tdmr);
 		if (ret)
 			return ret;
 	}
@@ -999,7 +1068,8 @@ static int tdmrs_populate_rsvd_areas_all(struct tdmr_info_list *tdmr_list,
  */
 static int construct_tdmrs(struct list_head *tmb_list,
 			   struct tdmr_info_list *tdmr_list,
-			   struct tdx_sys_info_tdmr *sysinfo_tdmr)
+			   struct tdx_sys_info_tdmr *sysinfo_tdmr,
+			   struct tdx_sys_info_cmr *sysinfo_cmr)
 {
 	int ret;
 
@@ -1012,7 +1082,7 @@ static int construct_tdmrs(struct list_head *tmb_list,
 	if (ret)
 		return ret;
 
-	ret = tdmrs_populate_rsvd_areas_all(tdmr_list, tmb_list,
+	ret = tdmrs_populate_rsvd_areas_all(tdmr_list, sysinfo_cmr,
 			sysinfo_tdmr->max_reserved_per_tdmr);
 	if (ret)
 		tdmrs_free_pamt_all(tdmr_list);
@@ -1208,7 +1278,8 @@ static int init_tdx_module(void)
 		goto err_free_tdxmem;
 
 	/* Cover all TDX-usable memory regions in TDMRs */
-	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &sysinfo.tdmr);
+	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &sysinfo.tdmr,
+			&sysinfo.cmr);
 	if (ret)
 		goto err_free_tdmrs;
 
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 9314f6ecbcb5..b933abefe8b4 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -43,6 +43,9 @@
 #define MD_FIELD_ID_PAMT_4K_ENTRY_SIZE		0x9100000100000010ULL
 #define MD_FIELD_ID_PAMT_2M_ENTRY_SIZE		0x9100000100000011ULL
 #define MD_FIELD_ID_PAMT_1G_ENTRY_SIZE		0x9100000100000012ULL
+#define MD_FIELD_ID_NUM_CMRS			0x9000000100000000ULL
+#define MD_FIELD_ID_CMR_BASE			0x9000000300000080ULL
+#define MD_FIELD_ID_CMR_SIZE			0x9000000300000100ULL
 
 /*
  * Sub-field definition of metadata field ID.
@@ -131,6 +134,14 @@ struct tdx_sys_info_version {
 	u32 build_date;
 };
 
+/* Class "CMR Info" */
+#define TDX_MAX_CMRS	32
+struct tdx_sys_info_cmr {
+	u16 num_cmrs;
+	u64 cmr_base[TDX_MAX_CMRS];
+	u64 cmr_size[TDX_MAX_CMRS];
+};
+
 /* Class "TDMR info" */
 struct tdx_sys_info_tdmr {
 	u16 max_tdmrs;
@@ -141,6 +152,7 @@ struct tdx_sys_info_tdmr {
 struct tdx_sys_info {
 	struct tdx_sys_info_features	features;
 	struct tdx_sys_info_version	version;
+	struct tdx_sys_info_cmr		cmr;
 	struct tdx_sys_info_tdmr	tdmr;
 };
 
-- 
2.46.0

Re: [PATCH v4 5/8] x86/virt/tdx: Start to track all global metadata in one structure

[Nikolay Borisov]

On 24.09.24 г. 14:28 ч., Kai Huang wrote:
> The TDX module provides a set of "Global Metadata Fields".  They report
> things like TDX module version, supported features, and fields related
> to create/run TDX guests and so on.
> 
> Currently the kernel only reads "TD Memory Region" (TDMR) related fields
> for module initialization.  There are immediate needs which require the
> TDX module initialization to read more global metadata including module
> version, supported features and "Convertible Memory Regions" (CMRs).
> 
> Also, KVM will need to read more metadata fields to support baseline TDX
> guests.  In the longer term, other TDX features like TDX Connect (which
> supports assigning trusted IO devices to TDX guest) may also require
> other kernel components such as pci/vt-d to access global metadata.
> 
> To meet all those requirements, the idea is the TDX host core-kernel to
> to provide a centralized, canonical, and read-only structure for the
> global metadata that comes out from the TDX module for all kernel
> components to use.
> 
> As the first step, introduce a new 'struct tdx_sys_info' to track all
> global metadata fields.
> 
> TDX categories global metadata fields into different "Classes".  E.g.,
> the TDMR related fields are under class "TDMR Info".  Instead of making
> 'struct tdx_sys_info' a plain structure to contain all metadata fields,
> organize them in smaller structures based on the "Class".
> 
> This allows those metadata fields to be used in finer granularity thus
> makes the code more clear.  E.g., the construct_tdmr() can just take the
> structure which contains "TDMR Info" metadata fields.
> 
> Add a new function get_tdx_sys_info() as the placeholder to read all
> metadata fields, and call it at the beginning of init_tdx_module().  For
> now it only calls get_tdx_sys_info_tdmr() to read TDMR related fields.
> 
> Note there is a functional change: get_tdx_sys_info_tdmr() is moved from
> after build_tdx_memlist() to before it, but it is fine to do so.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Nikolay Borisov]

On 24.09.24 г. 14:28 ч., Kai Huang wrote:
> The TDX module provides a set of "Global Metadata Fields".  They report
> things like TDX module version, supported features, and fields related
> to create/run TDX guests and so on.  TDX supports 8/16/32/64 bits
> metadata field element sizes.  For a given metadata field, the element
> size is encoded in the metadata field ID.
> 
> For now the kernel only reads "TD Memory Region" (TDMR) related metadata
> fields and they are all 16-bit.  Thus the kernel only has one primitive
> __read_sys_metadata_field16() to read 16-bit metadata field and the
> macro, read_sys_metadata_field16(), which does additional build-time
> check of the field ID makes sure the field is indeed 16-bit.
> 
> Future changes will need to read more metadata fields with different
> element sizes.  Choose to provide one primitive for each element size to
> support that.  Similar to the build_mmio_read() macro, reimplement the
> body of __read_sys_metadata_field16() as a macro build_sysmd_read(_size)
> in size-agnostic way, so it can be used to generate one primitive for
> each element size:
> 
>    build_sysmd_read(8)
>    build_sysmd_read(16)
>    ..
> 
> Also extend read_sys_metadata_field16() take the '_size' as argument
> (and rename it to read_sys_metadata_field() to make it size-agnostic) to
> allow the READ_SYS_INFO() macro to choose which primitive to use.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>

<snip>

> +#define build_sysmd_read(_size)							\
> +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
> +{										\
> +	u64 tmp;								\
> +	int ret;								\
> +										\
> +	ret = tdh_sys_rd(field_id, &tmp);					\
> +	if (ret)								\
> +		return ret;							\
> +										\
> +	*val = tmp;								\
> +										\
> +	return 0;								\
>   }
>   
> -#define read_sys_metadata_field16(_field_id, _val)		\
> +build_sysmd_read(16)

nit: Generally the unwritten convention for this kind of macro 
definition is to capitalize them and be of the from:

DEFINE_xxxxx - similar to how event classes are defined.

perhaps naming this macro:

DEFINE_TDX_METADATA_READER() ought to be more descriptive, also the
"md" contraction of metadata also seems a bit quirky (at least to me).

It's not a deal breaker but if there is going to be another posting this 
might be something to consider.

<snip>

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

> 
> > +#define build_sysmd_read(_size)							\
> > +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
> > +{										\
> > +	u64 tmp;								\
> > +	int ret;								\
> > +										\
> > +	ret = tdh_sys_rd(field_id, &tmp);					\
> > +	if (ret)								\
> > +		return ret;							\
> > +										\
> > +	*val = tmp;								\
> > +										\
> > +	return 0;								\
> >   }
> >   
> > -#define read_sys_metadata_field16(_field_id, _val)		\
> > +build_sysmd_read(16)
> 
> nit: Generally the unwritten convention for this kind of macro 
> definition is to capitalize them and be of the from:
> 
> DEFINE_xxxxx - similar to how event classes are defined.
> 
> perhaps naming this macro:
> 
> DEFINE_TDX_METADATA_READER() ought to be more descriptive, also the
> "md" contraction of metadata also seems a bit quirky (at least to me).
> 
> It's not a deal breaker but if there is going to be another posting this 
> might be something to consider.
> 

Thanks for the comments.

I don't have opinion on this.  Dan said we can do something like
build_mmio_read() macro and this is where build_sysmd_read() came from.  Dan
used build_tdg_sys_rd() in his patch too:

https://lore.kernel.org/all/172618717675.516322.6087817418162288917.stgit@dwillia2-xfh.jf.intel.com/

Btw I actually agree that your DEFINE_xx() is more formal thus is better when
the macro is used by multiple C files.  But here only tdx.c uses it, and IMHO
it's also fine to have a informal but shorter name here.

Anyway let's see whether other people have anything to say.

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Dave Hansen]

On 9/24/24 04:28, Kai Huang wrote:
> +#define build_sysmd_read(_size)							\
> +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
> +{										\
> +	u64 tmp;								\
> +	int ret;								\
> +										\
> +	ret = tdh_sys_rd(field_id, &tmp);					\
> +	if (ret)								\
> +		return ret;							\
> +										\
> +	*val = tmp;								\
> +										\
> +	return 0;								\
>  }

Why?  What's so important about having the compiler do the copy?


#define read_sys_metadata_field(id, val) 	\
	__read_sys_metadata_field(id, val, sizeof (*(val)))

static int __read_sys_metadata_field(u64 field_id, void *ptr, int size)
{
	...
	memcpy(ptr, &tmp, size);

	return 0;
}

There's one simple #define there so that users don't have to do the
sizeof and can't screw it up.

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

On 27/09/2024 3:47 am, Hansen, Dave wrote:
> On 9/24/24 04:28, Kai Huang wrote:
>> +#define build_sysmd_read(_size)							\
>> +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
>> +{										\
>> +	u64 tmp;								\
>> +	int ret;								\
>> +										\
>> +	ret = tdh_sys_rd(field_id, &tmp);					\
>> +	if (ret)								\
>> +		return ret;							\
>> +										\
>> +	*val = tmp;								\
>> +										\
>> +	return 0;								\
>>   }
> 
> Why?  What's so important about having the compiler do the copy?
> 
> 
> #define read_sys_metadata_field(id, val) 	\
> 	__read_sys_metadata_field(id, val, sizeof (*(val)))
> 
> static int __read_sys_metadata_field(u64 field_id, void *ptr, int size)
> {
> 	...
> 	memcpy(ptr, &tmp, size);
> 
> 	return 0;
> }
> 
> There's one simple #define there so that users don't have to do the
> sizeof and can't screw it up.

Yes we can do this.  This is basically what I did in the previous version:

https://lore.kernel.org/kvm/0403cdb142b40b9838feeb222eb75a4831f6b46d.1724741926.git.kai.huang@intel.com/

But Dan commented using typeless 'void *' and 'size' is kinda a step 
backwards and we should do something similar to build_mmio_read():

https://lore.kernel.org/kvm/66db75497a213_22a2294b@dwillia2-xfh.jf.intel.com.notmuch/

Hi Dan,

I think what Dave suggested makes sense.  If the concern is using 
__read_sys_metadata_field() directly isn't typesafe, we can add a 
comment to it saying callers should not use it directly and use 
read_sys_metadata_field() instead.

Dave's approach also makes the LoC slightly shorter, and cleaner from 
the perspective that we don't need to explicitly specify the '16/32/64' 
in the READ_SYS_INFO() macro anymore as shown in here:

https://lore.kernel.org/kvm/79c256b8978310803bb4de48cd81dd373330cbc2.1727173372.git.kai.huang@intel.com/

Please let me know your comment?

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Dave Hansen]

On 9/26/24 15:22, Huang, Kai wrote:
> But Dan commented using typeless 'void *' and 'size' is kinda a step
> backwards and we should do something similar to build_mmio_read():

Well, void* is typeless, but at least it knows the size in this case.
It's not completely aimless.  I was thinking of how things like
get_user() work.

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

On 27/09/2024 10:26 am, Hansen, Dave wrote:
> On 9/26/24 15:22, Huang, Kai wrote:
>> But Dan commented using typeless 'void *' and 'size' is kinda a step
>> backwards and we should do something similar to build_mmio_read():
> 
> Well, void* is typeless, but at least it knows the size in this case.
> It's not completely aimless.  I was thinking of how things like
> get_user() work.

get_user(x,ptr) only works with simple types:

  * @ptr must have pointer-to-simple-variable type, and the result of
  * dereferencing @ptr must be assignable to @x without a cast.

The compiler knows the type of both @x and @(*ptr), so it knows 
type-safety and size to copy.

I think we can eliminate the __read_sys_metadata_field() by implementing 
it as a macro directly and get rid of 'void *' and 'size':

static int tdh_sys_rd(u64 field_id, u64 *val) {}

/* @_valptr must be pointer to u8/u16/u32/u64 */
#define read_sys_metadata_field(_field_id, _valptr)	\
({							\
	u64 ___tmp;					\
	int ___ret;					\
							\
	BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=	\
		sizeof(*_valptr));			\
							\
	___ret = tdh_sys_rd(_field_id, &___tmp);	\
							\
	*_valptr = ___tmp;				\
	___ret;
})

It sets *_valptr unconditionally but we can also only do it when ___ret 
is 0.

The caller will need to do:

static int get_tdx_metadata_X_which_is_32bit(...)
{
	u32 metadata_X;
	int ret;

	ret = read_sys_metadata_field(MD_FIELD_ID_X, &metadata_X);

	return ret;
}

I haven't compiled and tested but it seems feasible.

Any comments?

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Dan Williams]

Huang, Kai wrote:
> 
> 
> On 27/09/2024 10:26 am, Hansen, Dave wrote:
> > On 9/26/24 15:22, Huang, Kai wrote:
> >> But Dan commented using typeless 'void *' and 'size' is kinda a step
> >> backwards and we should do something similar to build_mmio_read():
> > 
> > Well, void* is typeless, but at least it knows the size in this case.
> > It's not completely aimless.  I was thinking of how things like
> > get_user() work.
> 
> get_user(x,ptr) only works with simple types:
> 
>   * @ptr must have pointer-to-simple-variable type, and the result of
>   * dereferencing @ptr must be assignable to @x without a cast.
> 
> The compiler knows the type of both @x and @(*ptr), so it knows 
> type-safety and size to copy.
> 
> I think we can eliminate the __read_sys_metadata_field() by implementing 
> it as a macro directly and get rid of 'void *' and 'size':
> 
> static int tdh_sys_rd(u64 field_id, u64 *val) {}
> 
> /* @_valptr must be pointer to u8/u16/u32/u64 */
> #define read_sys_metadata_field(_field_id, _valptr)	\
> ({							\
> 	u64 ___tmp;					\
> 	int ___ret;					\
> 							\
> 	BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=	\
> 		sizeof(*_valptr));			\
> 							\
> 	___ret = tdh_sys_rd(_field_id, &___tmp);	\
> 							\
> 	*_valptr = ___tmp;				\
> 	___ret;
> })
> 
> It sets *_valptr unconditionally but we can also only do it when ___ret 
> is 0.
> 
> The caller will need to do:
> 
> static int get_tdx_metadata_X_which_is_32bit(...)
> {
> 	u32 metadata_X;
> 	int ret;
> 
> 	ret = read_sys_metadata_field(MD_FIELD_ID_X, &metadata_X);
> 
> 	return ret;
> }
> 
> I haven't compiled and tested but it seems feasible.
> 
> Any comments?

If it works this approach addresses all the concerns I had with getting
the compiler to validate field sizes.

Should be straightforward to put this in a shared location so that it
can optionally use tdg_sys_rd internally.

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

On Tue, 2024-10-01 at 00:56 -0700, Dan Williams wrote:
> Huang, Kai wrote:
> > 
> > 
> > On 27/09/2024 10:26 am, Hansen, Dave wrote:
> > > On 9/26/24 15:22, Huang, Kai wrote:
> > > > But Dan commented using typeless 'void *' and 'size' is kinda a step
> > > > backwards and we should do something similar to build_mmio_read():
> > > 
> > > Well, void* is typeless, but at least it knows the size in this case.
> > > It's not completely aimless.  I was thinking of how things like
> > > get_user() work.
> > 
> > get_user(x,ptr) only works with simple types:
> > 
> >   * @ptr must have pointer-to-simple-variable type, and the result of
> >   * dereferencing @ptr must be assignable to @x without a cast.
> > 
> > The compiler knows the type of both @x and @(*ptr), so it knows 
> > type-safety and size to copy.
> > 
> > I think we can eliminate the __read_sys_metadata_field() by implementing 
> > it as a macro directly and get rid of 'void *' and 'size':
> > 
> > static int tdh_sys_rd(u64 field_id, u64 *val) {}
> > 
> > /* @_valptr must be pointer to u8/u16/u32/u64 */
> > #define read_sys_metadata_field(_field_id, _valptr)	\
> > ({							\
> > 	u64 ___tmp;					\
> > 	int ___ret;					\
> > 							\
> > 	BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=	\
> > 		sizeof(*_valptr));			\
> > 							\
> > 	___ret = tdh_sys_rd(_field_id, &___tmp);	\
> > 							\
> > 	*_valptr = ___tmp;				\
> > 	___ret;
> > })
> > 
> > It sets *_valptr unconditionally but we can also only do it when ___ret 
> > is 0.
> > 
> > The caller will need to do:
> > 
> > static int get_tdx_metadata_X_which_is_32bit(...)
> > {
> > 	u32 metadata_X;
> > 	int ret;
> > 
> > 	ret = read_sys_metadata_field(MD_FIELD_ID_X, &metadata_X);
> > 
> > 	return ret;
> > }
> > 
> > I haven't compiled and tested but it seems feasible.
> > 
> > Any comments?
> 
> If it works this approach addresses all the concerns I had with getting
> the compiler to validate field sizes.

Yes I just quickly tested on my box and it worked -- TDX module can be
initialized successfully and all metadata fields (module version, CMRs etc) seem
to be correct.

Hi Dave,

Please let me know if you have any concern?  Otherwise I will go with this
route.

> 
> Should be straightforward to put this in a shared location so that it
> can optionally use tdg_sys_rd internally.

Yeah it's doable.  As you also noticed guest and host use different calls: guest
uses tdg_vm_rd() (which is in Kirill's not-yet-merged series[*]), and host uses
tdh_sys_rd().  

[*]
https://lore.kernel.org/all/20240828093505.2359947-2-kirill.shutemov@linux.intel.com/

This can be resolved by adding a new argument to the read_sys_metadata_field()
macro, e.g.,:

/* @_valptr must be pointer to u8/u16/u32/u64 */
#define read_sys_metadata_field(_read_func, _field_id, _valptr)		\
({									\
	u64 ___tmp;							\
	int ___ret;							\
									\
	BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=			\
		sizeof(*_valptr));					\
									\
	___ret = _read_func(_field_id, &___tmp);			\
									\
	*_valptr = ___tmp;						\
	___ret;							
	\
})

We can put it in <asm/tdx.h> (together with the MD_FIELD_ELE_SIZE() macro) for
guest and host to share.

And in guest code (arch/x86/coco/tdx/tdx.c), we can have a wrapper:

#define tdg_read_sys_metadata_field(_field_id, _valptr)		\
	read_sys_metadata_field(tdg_vm_rd, _field_id, _valptr)

Similarly, in the host code (arch/x86/virt/vmx/tdx/tdx.c), we can have:

#define tdh_read_sys_metadata_field(_field_id, _valptr)		\
	read_sys_metadata_field(tdh_sys_rd, _field_id, _valptr)

We can start with this if you think it's better.

But I would like to discuss this more:

Once we start to share, it feels a little bit odd to share only the
read_sys_metadata_field() macro, because we can probably share others too:

1) The metadata field ID definitions and bit definitions (this is obvious).
2) tdh_sys_rd() and tdg_vm_rd() are similar and can be shared too:

/* Read TD-scoped metadata */
static inline u64 __maybe_unused tdg_vm_rd(u64 field, u64 *value)
{
	struct tdx_module_args args = {
		.rdx = field,
	};
	u64 ret;

	ret = __tdcall_ret(TDG_VM_RD, &args);
	*value = args.r8;

	return ret;
}

static int tdh_sys_rd(u64 field_id, u64 *data)    
{                                    
        struct tdx_module_args args = {};                        
        int ret;                                                           
                                                                           
        /*                                                 
         * TDH.SYS.RD -- reads one global metadata field               
         *  - RDX (in): the field to read                               
         *  - R8 (out): the field data                                       
         */                                           
        args.rdx = field_id;                                                  
        ret = seamcall_prerr_ret(TDH_SYS_RD, &args);
        if (ret)                                                    
                return ret;                             
                             
        *data = args.r8;                            
                                                                 
        return 0;                               
}

There are minor differences, e.g., tdh_sys_rd() only sets *data when TDH_SYS_RD
succeeded but tdg_vm_rd() unconditionally sets *value (spec says R8 is set to 0
if TDG_VM_RD or TDH_SYS_RD fails, and guest code kinda depends on this).

Putting aside which is better, those differences are resolvable.  And if we
start to share, it appears we should share this too.

And then the TDH_SYS_RD definition (and probably all TDH_SYS_xx SEAMCALL leaf
definitions) should be moved to <asm/tdx.h> too.

And then the header will grow bigger and bigger, which brings us a question on
how to better organize all those definitions: 1) TDCALL/SEAMCALL leaf function
definitions (e.g., TDH_SYS_RD); 2) TDCALL/SEAMCALL low level functions
(__tdcall(), __seamcall()); 3) TDCALL/SEAMCALL leaf function wrappers; 4) sys
metadata field ID definitions; 5) sys metadata read helper macros;

So to me we can have a separate series to address how to better organize TDX
code and how to share code between guest and host (I can start to work on it if
it's better to be done sooner rather than later), thus I am not sure we want to
start sharing read_sys_metadata_field() macro in _this_ series.

But of course, if it is worth to share read_sys_metadata_field() in this series,
I can add a patch as the last patch of this series to only share
read_sys_metadata_field() in <asm/tdx.h> as mentioned above.

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Dave Hansen]

On 10/1/24 03:44, Huang, Kai wrote:
> Please let me know if you have any concern?  Otherwise I will go with
> this route.
I still see some long unwieldy #defines in the mail thread.  That's my
biggest worry.

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

On Tue, 2024-10-01 at 08:19 -0700, Dave Hansen wrote:
> On 10/1/24 03:44, Huang, Kai wrote:
> > Please let me know if you have any concern?  Otherwise I will go with
> > this route.
> I still see some long unwieldy #defines in the mail thread.  That's my
> biggest worry.

I suppose you mean the read_sys_metadata_field() macro?

We can split that into two smaller macros by moving BUILD_BUG_ON() out:

/* Don't use this directly, use read_sys_metadata_read() instead. */
#define __read_sys_metadata_field(_field_id, _valptr)   \
({                                                      \
        u64 ___tmp;                                     \
        int ___ret;                                     \
                                                        \
        ___ret = tdh_sys_rd(_field_id, &___tmp);        \
        *_valptr = ___tmp;                              \
                                                        \
        ___ret;                                         \
})

/* @_valptr must be pointer of u8/u16/u32/u64. */
#define read_sys_metadata_field(_field_id, _valptr)     \
({                                                      \
        BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=    \
                        sizeof(*_valptr));              \
        __read_sys_metadata_field(_field_id, _valptr);  \
})

Does this look good to you?

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

On Tue, 2024-10-01 at 21:40 +0000, Huang, Kai wrote:
> On Tue, 2024-10-01 at 08:19 -0700, Dave Hansen wrote:
> > On 10/1/24 03:44, Huang, Kai wrote:
> > > Please let me know if you have any concern?  Otherwise I will go with
> > > this route.
> > I still see some long unwieldy #defines in the mail thread.  That's my
> > biggest worry.
> 
> I suppose you mean the read_sys_metadata_field() macro?
> 
> We can split that into two smaller macros by moving BUILD_BUG_ON() out:
> 
> /* Don't use this directly, use read_sys_metadata_read() instead. */
> #define __read_sys_metadata_field(_field_id, _valptr)   \
> ({                                                      \
>         u64 ___tmp;                                     \
>         int ___ret;                                     \
>                                                         \
>         ___ret = tdh_sys_rd(_field_id, &___tmp);        \
>         *_valptr = ___tmp;                              \
>                                                         \
>         ___ret;                                         \
> })
> 
> /* @_valptr must be pointer of u8/u16/u32/u64. */
> #define read_sys_metadata_field(_field_id, _valptr)     \
> ({                                                      \
>         BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=    \
>                         sizeof(*_valptr));              \
>         __read_sys_metadata_field(_field_id, _valptr);  \
> })
> 
> Does this look good to you?

Hi Dave,

Would you let me know are you OK with this?

If not, I will revert back to what you suggested:

https://lore.kernel.org/lkml/cover.1727173372.git.kai.huang@intel.com/T/#m3874080ef158a4704c4082259d3594aa0a322fc8

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Dave Hansen]

On 10/6/24 20:07, Huang, Kai wrote:
> Would you let me know are you OK with this?

It still looks unwieldy. Is there no other choice?

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

On Sun, 2024-10-06 at 20:44 -0700, Dave Hansen wrote:
> On 10/6/24 20:07, Huang, Kai wrote:
> > Would you let me know are you OK with this?
> 
> It still looks unwieldy. Is there no other choice?

Sorry I cannot figure out a better way.  I would love to hear if you have
anything in mind?

Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

On Mon, 2024-10-07 at 06:53 +0000, Huang, Kai wrote:
> On Sun, 2024-10-06 at 20:44 -0700, Dave Hansen wrote:
> > On 10/6/24 20:07, Huang, Kai wrote:
> > > Would you let me know are you OK with this?
> > 
> > It still looks unwieldy. Is there no other choice?
> 
> Sorry I cannot figure out a better way.  I would love to hear if you have
> anything in mind?

Hi Dave,

Since you have concern about the unwieldy macro, I'll switch back to what you
suggested:

https://lore.kernel.org/lkml/cover.1727173372.git.kai.huang@intel.com/T/#m3874080ef158a4704c4082259d3594aa0a322fc8

.. unless I hear something new from you.  Thanks for the review.