| Message ID | bafe7cfc3c78473aac78499c1eca5abf9bb3ecf5.1721186590.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host: metadata reading tweaks, bug fix and info dump | expand |
Kai Huang wrote: > 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 > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > > v1 -> v2: > - New patch to fix a comment spotted by Nikolay. > > --- > 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 fdb879ef6c45..4e43cec19917 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 Given this is JSON any plan to just check-in "global_metadata.json" somewhere in tools/ with a script that queries for a set of fields and spits them out into a Linux data structure + set of TD_SYSINFO_*_MAP() calls? Then no future review bandwidth needs to be spent on manually checking offsets names and values, they will just be pulled from the script.
On Mon, 2024-08-05 at 20:43 -0700, Williams, Dan J wrote: > Kai Huang wrote: > > 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 > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > > > v1 -> v2: > > - New patch to fix a comment spotted by Nikolay. > > > > --- > > 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 fdb879ef6c45..4e43cec19917 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 > > Given this is JSON any plan to just check-in "global_metadata.json" > somewhere in tools/ with a script that queries for a set of fields and > spits them out into a Linux data structure + set of TD_SYSINFO_*_MAP() > calls? Then no future review bandwidth needs to be spent on manually > checking offsets names and values, they will just be pulled from the > script. This seems a good idea. I'll add this to my TODO list and evaluate it first. One minor issue is some metadata fields may need special handling. E.g., MAX_VCPUS_PER_TD (which is u16) may not be supported by some old TDX modules, but this isn't an error because we can just treats it as U16_MAX.
Huang, Kai wrote: [..] > > Given this is JSON any plan to just check-in "global_metadata.json" > > somewhere in tools/ with a script that queries for a set of fields and > > spits them out into a Linux data structure + set of TD_SYSINFO_*_MAP() > > calls? Then no future review bandwidth needs to be spent on manually > > checking offsets names and values, they will just be pulled from the > > script. > > This seems a good idea. I'll add this to my TODO list and evaluate it > first. > > One minor issue is some metadata fields may need special handling. E.g., > MAX_VCPUS_PER_TD (which is u16) may not be supported by some old TDX > modules, but this isn't an error because we can just treats it as > U16_MAX. TDX Module had better not be breaking us when they remove metadata fields. So if you know of fields that get removed the module absolutely cannot cause existing code paths. Linux could maybe grant that some values start returning an explicit "deprecated" error code in the future and Linux adds handling for that common case. Outside of that metadata fields are forever and the module needs to ship placeholder values that fail gracefully on older kernels. OS software should not be expected to keep up with the whims of metadata field removals without an explicit plan to make those future removals benign to legacy kernels.
On Tue, 2024-08-06 at 12:06 -0700, Williams, Dan J wrote: > Huang, Kai wrote: > [..] > > > Given this is JSON any plan to just check-in "global_metadata.json" > > > somewhere in tools/ with a script that queries for a set of fields and > > > spits them out into a Linux data structure + set of TD_SYSINFO_*_MAP() > > > calls? Then no future review bandwidth needs to be spent on manually > > > checking offsets names and values, they will just be pulled from the > > > script. > > > > This seems a good idea. I'll add this to my TODO list and evaluate it > > first. > > > > One minor issue is some metadata fields may need special handling. E.g., > > MAX_VCPUS_PER_TD (which is u16) may not be supported by some old TDX > > modules, but this isn't an error because we can just treats it as > > U16_MAX. > > TDX Module had better not be breaking us when they remove metadata > fields. So if you know of fields that get removed the module absolutely > cannot cause existing code paths. > I don't think they will remove any metadata field. They may add more field(s) when new feature is added to a new version module, but not remove. One thing we might need to confirm is an new version of module should always support the features that old modules support. But IMHO this should not be relevant if we have a policy below. > Linux could maybe grant that some > values start returning an explicit "deprecated" error code in the future > and Linux adds handling for that common case. Outside of that metadata > fields are forever and the module needs to ship placeholder values that > fail gracefully on older kernels. > > OS software should not be expected to keep up with the whims of metadata > field removals without an explicit plan to make those future removals > benign to legacy kernels. I think we can have a simple rule like below? The kernel decides which metadata fields are essential, and which are optional. If any essential field is missing, then kernel cannot use TDX. In this way the kernel doesn't depend on whims of TDX module version changes.
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h index fdb879ef6c45..4e43cec19917 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
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 Signed-off-by: Kai Huang <kai.huang@intel.com> --- v1 -> v2: - New patch to fix a comment spotted by Nikolay. --- arch/x86/virt/vmx/tdx/tdx.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)