Re: [PATCH v2 00/10] TDX host: metadata reading tweaks, bug fix and info dump

["Huang, Kai" <kai.huang@intel.com>]

On 6/08/2024 10:36 am, Dan Williams wrote:
> Kai Huang wrote:
>> 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:
>>
>> https://github.com/intel/tdx/commits/kvm-tdxinit/
>>
>> Dear maintainers,
>>
>> This series targets x86 tip.  I also added Dan, KVM maintainers and KVM
>> list so people can review and comment.  Thanks for your time.
>>
>> 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 [1];
>>   - Reject module with no NO_RBP_MOD feature support [2];
>>   - Read CMR info to fix a module initialization failure bug [3].
>>
>> Also, the upstreaming-on-going KVM TDX support [4] requires to read more
>> global metadata fields.  In the longer term, the TDX Connect [5] (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.
>>
>> There is an "alternative option to manage global metadata" (see below)
>> but it is not as straightforward as this.
>>
>> This series starts to track all global metadata fields into a single
>> 'struct tdx_sysinfo', and reads more metadata fields to that structure
>> to address the immediate needs as mentioned above.
>>
>> More fields will be added in the near future to support KVM TDX, and the
>> actual sharing/export the "read-only" global metadata for KVM will also
>> be sent out in the near future when that becomes immediate (also see
>> "Share global metadata to KVM" below).
> 
> I think it is important to share why this unified data structure
> proposal reached escape velocity from internal review. 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.

Thanks Dan for this.  I think I can somehow integrate your words above 
into the changelog.  (It took me bit of time to digest though due to my 
bad english.)

> 
>> Note, the first couple of patches in this series were from the old
>> patchset "TDX host: Provide TDX module metadata reading APIs" [6].
>>
>> === Further read ===
>>
>> 1) Altertive option to manage global metadata
>>
>> The TDX host core-kernel could also expose/export APIs for reading
>> metadata out of TDX module directly, and all in-kernel TDX users use
>> these APIs and manage their own metadata fields.
>>
>> However this isn't as straightforward as exposing/exporting structure,
>> because the API to read multi fields to a structure requires the caller
>> to build a "mapping table" between field ID to structure member:
>>
>> 	struct kvm_used_metadata {
>> 		u64 member1;
>> 		...
>> 	};
>>
>> 	#define TD_SYSINFO_KVM_MAP(_field_id, _member)	\
>> 		TD_SYSINFO_MAP(_field_id, struct kvm_used_metadata, \
>> 				_member)
>>
>> 	struct tdx_metadata_field_mapping fields[] = {
>> 		TD_SYSINFO_KVM_MAP(FIELD_ID1, member1),
>> 		...
>> 	};
>>
>> 	ret = tdx_sysmd_read_multi(fields, ARRAY_SIZE(fields), buf);
>>
>> Another problem is some metadata field may be accessed by multiple
>> kernel components, e.g., the one reports TDX module features, in which
>> case there will be duplicated code comparing to exposing structure
>> directly.
> 
> A full explanation of what this patch is not doing is a bit overkill.

I'll remove the structure/macro/function details to make it more concise.

> 
>> 2) Share global metadata to KVM
>>
>> To achieve "read-only" centralized global metadata structure, the idea
>> way is to use __ro_after_init.  However currently all global metadata
>> are read by tdx_enable(), which is supposed to be called at any time at
>> runtime thus isn't annotated with __init.
>>
>> The __ro_after_init can be done eventually, but it can only be done
>> after moving VMXON out of KVM to the core-kernel: after that we can
>> read all metadata during kernel boot (thus __ro_after_init), but
>> doesn't necessarily have to do it in tdx_enable().
>>
>> However moving VMXON out of KVM is NOT considered as dependency for the
>> initial KVM TDX support [7].  Thus for the initial support, the idea is
>> TDX host to export a function which returns a "const struct pointer" so
>> KVM won't be able to modify any global metadata.
> 
> For now I think it is sufficient to say that metadata just gets
> populated to a central data structure. Follow on work to protect that
> data structure against post-init updates can come later.

OK.  Will do.

> 
>> 3) TDH.SYS.RD vs TDH.SYS.RDALL
>>
>> The kernel can use two SEAMCALLs to read global metadata: TDH.SYS.RD and
>> TDH.SYS.RDALL.  The former simply reads one metadata field to a 'u64'.
>> The latter tries to read all fields to a 4KB buffer.
>>
>> Currently the kernel only uses the former to read metadata, and this
>> series doesn't choose to use TDH.SYS.RDALL.
>>
>> The main reason is the "layout of all fields in the 4KB buffer" that
>> returned by TDH.SYS.RDALL isn't architectural consistent among different
>> TDX module versions.
>>
>> E.g., some metadata fields may not be supported by the old module, thus
>> they may or may not be in the 4KB buffer depending on module version.
>> And it is impractical to know whether those fields are in the buffer or
>> not.
>>
>> TDH.SYS.RDALL may be useful to read one small set of metadata fields,
>> e.g., fields in one "Class" (TDX categories all global metadata fields
>> in different "Class"es).  But this is only an optimization even if
>> TDH.SYS.RDALL can be used, so leave this to future consideration.
> 
> I appreciate the effort to include some of the discussions had while
> boiling this patchset down to its simplest near term form, but this
> much text makes the simple patches look much more controversial than
> they are. This TDH.SYS.RDALL consideration is not relevant to the
> current proposal.

I'll remove this section.

Again thanks for your feedback.