mbox series

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

Message ID cover.1721186590.git.kai.huang@intel.com (mailing list archive)
Headers show
Series TDX host: metadata reading tweaks, bug fix and info dump | expand

Message

Huang, Kai July 17, 2024, 3:40 a.m. UTC
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).

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.

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.

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.


[1] https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#m352829aedf6680d4628c7e40dc40b332eda93355
[2] https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#mef98469c51e2382ead2c537ea189752360bd2bef
[3] https://github.com/canonical/tdx/issues/135#issuecomment-2151570238
[4] https://lore.kernel.org/kvm/cover.1708933498.git.isaku.yamahata@intel.com/T/
[5] https://cdrdv2.intel.com/v1/dl/getContent/773614
[6] https://lore.kernel.org/lkml/cover.1709288433.git.kai.huang@intel.com/T/
[7] https://lore.kernel.org/kvm/cover.1708933498.git.isaku.yamahata@intel.com/T/#me0c081438074341ad70d3525f6cf3472d7d81b0e



Kai Huang (10):
  x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro
  x86/virt/tdx: Unbind global metadata read with 'struct
    tdx_tdmr_sysinfo'
  x86/virt/tdx: Support global metadata read for all element sizes
  x86/virt/tdx: Abstract reading multiple global metadata fields as a
    helper
  x86/virt/tdx: Move field mapping table of getting TDMR info to
    function local
  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 basic information
  x86/virt/tdx: Reduce TDMR's reserved areas by using CMRs to find
    memory holes
  x86/virt/tdx: Don't initialize module that doesn't support NO_RBP_MOD
    feature

 arch/x86/virt/vmx/tdx/tdx.c | 279 +++++++++++++++++++++++++++++-------
 arch/x86/virt/vmx/tdx/tdx.h |  82 +++++++++--
 2 files changed, 302 insertions(+), 59 deletions(-)


base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd

Comments

Huang, Kai Aug. 5, 2024, 12:03 p.m. UTC | #1
On Wed, 2024-07-17 at 15:40 +1200, 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.
> 

Kindly ping.
Dan Williams Aug. 5, 2024, 10:36 p.m. UTC | #2
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.

> 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.

> 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.

> 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.
Huang, Kai Aug. 8, 2024, 12:19 a.m. UTC | #3
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.