| Message ID | 3d75e730cc514adfc9ac3200da5abd4d5e5d1bad.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 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_sysinfo' to track all > global metadata fields. > > TDX categories global metadata fields into different "Class"es. E.g., > the current TDMR related fields are under class "TDMR Info". Instead of > making 'struct tdx_sysinfo' 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 current construct_tdmr() can just > take the structure which contains "TDMR Info" metadata fields. > > Start with moving 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo', and > rename 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo_tdmr_info' to > make it consistent with the "class name". How about 'struct tdx_sys_info' and 'struct tdx_sys_tdmr_info' to avoid duplicating 'info' in the symbol name? Do pure renames indpendent of logic changes to make patches like this easier to read. I would also move the pure rename to the front of the patches so the reviewer spends as minimal amount of time with the deprecated name in the set. > Add a new function get_tdx_sysinfo() as the place to read all metadata > fields, and call it at the beginning of init_tdx_module(). Move the > existing get_tdx_tdmr_sysinfo() to get_tdx_sysinfo(). > > Note there is a functional change: get_tdx_tdmr_sysinfo() 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> > --- > arch/x86/virt/vmx/tdx/tdx.c | 29 +++++++++++++++++------------ > arch/x86/virt/vmx/tdx/tdx.h | 32 +++++++++++++++++++++++++------- > 2 files changed, 42 insertions(+), 19 deletions(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 86c47db64e42..3253cdfa5207 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -320,11 +320,11 @@ static int stbuf_read_sysmd_multi(const struct field_mapping *fields, > } > > #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > - TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) > + TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_tdmr_info, _member) > > -static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) > +static int get_tdx_tdmr_sysinfo(struct tdx_sysinfo_tdmr_info *tdmr_sysinfo) > { > - /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ > + /* Map TD_SYSINFO fields into 'struct tdx_sysinfo_tdmr_info': */ > static const struct field_mapping fields[] = { > TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS, max_tdmrs), > TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr), > @@ -337,6 +337,11 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) > return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), tdmr_sysinfo); > } > > +static int get_tdx_sysinfo(struct tdx_sysinfo *sysinfo) > +{ > + return get_tdx_tdmr_sysinfo(&sysinfo->tdmr_info); > +} > + > /* Calculate the actual TDMR size */ > static int tdmr_size_single(u16 max_reserved_per_tdmr) > { > @@ -353,7 +358,7 @@ 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_sysinfo_tdmr_info *tdmr_sysinfo) > { > size_t tdmr_sz, tdmr_array_sz; > void *tdmr_array; > @@ -936,7 +941,7 @@ 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_tdmr_sysinfo *tdmr_sysinfo) > + struct tdx_sysinfo_tdmr_info *tdmr_sysinfo) > { > int ret; > > @@ -1109,9 +1114,13 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list) > > static int init_tdx_module(void) > { > - struct tdx_tdmr_sysinfo tdmr_sysinfo; > + struct tdx_sysinfo sysinfo; > int ret; > > + ret = get_tdx_sysinfo(&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 > @@ -1128,17 +1137,13 @@ static int init_tdx_module(void) > if (ret) > goto out_put_tdxmem; > > - ret = get_tdx_tdmr_sysinfo(&tdmr_sysinfo); > - 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_info); > 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_info); > 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 4e43cec19917..b5eb7c35f1dc 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -100,13 +100,6 @@ struct tdx_memblock { > int nid; > }; > > -/* "TDMR info" part of "Global Scope Metadata" for constructing TDMRs */ > -struct tdx_tdmr_sysinfo { > - 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 > > @@ -119,4 +112,29 @@ struct tdmr_info_list { > int max_tdmrs; /* How many 'tdmr_info's are allocated */ > }; > > +/* > + * Kernel-defined structures to contain "Global Scope Metadata". > + * > + * TDX global metadata fields are categorized by "Class". See the > + * "global_metadata.json" in the "TDX 1.5 ABI Definitions". > + * > + * 'struct tdx_sysinfo' 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 not all metadata fields in each class are defined, only those > + * used by the kernel are. > + */ > + > +/* Class "TDMR Info" */ > +struct tdx_sysinfo_tdmr_info { > + u16 max_tdmrs; > + u16 max_reserved_per_tdmr; > + u16 pamt_entry_size[TDX_PS_NR]; > +}; > + > +struct tdx_sysinfo { > + struct tdx_sysinfo_tdmr_info tdmr_info; > +}; I would just call this member 'tdmr' since the 'info' is already applied by being in tdx_sys_info.
On Mon, 2024-08-05 at 20:51 -0700, Williams, Dan J wrote: > 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_sysinfo' to track all > > global metadata fields. > > > > TDX categories global metadata fields into different "Class"es. E.g., > > the current TDMR related fields are under class "TDMR Info". Instead of > > making 'struct tdx_sysinfo' 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 current construct_tdmr() can just > > take the structure which contains "TDMR Info" metadata fields. > > > > Start with moving 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo', and > > rename 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo_tdmr_info' to > > make it consistent with the "class name". > > How about 'struct tdx_sys_info' and 'struct tdx_sys_tdmr_info' to avoid > duplicating 'info' in the symbol name? Fine to me. Will do. > > Do pure renames indpendent of logic changes to make patches like > this easier to read. > > I would also move the pure rename to the front of the patches so the > reviewer spends as minimal amount of time with the deprecated name in > the set. OK. I'll split out the pure rename patch and move it to earlier place. [...] > > + > > +/* Class "TDMR Info" */ > > +struct tdx_sysinfo_tdmr_info { > > + u16 max_tdmrs; > > + u16 max_reserved_per_tdmr; > > + u16 pamt_entry_size[TDX_PS_NR]; > > +}; > > + > > +struct tdx_sysinfo { > > + struct tdx_sysinfo_tdmr_info tdmr_info; > > +}; > > I would just call this member 'tdmr' since the 'info' is already applied > by being in tdx_sys_info. Yeah fine to me. Thanks.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 86c47db64e42..3253cdfa5207 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -320,11 +320,11 @@ static int stbuf_read_sysmd_multi(const struct field_mapping *fields, } #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ - TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) + TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_tdmr_info, _member) -static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) +static int get_tdx_tdmr_sysinfo(struct tdx_sysinfo_tdmr_info *tdmr_sysinfo) { - /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ + /* Map TD_SYSINFO fields into 'struct tdx_sysinfo_tdmr_info': */ static const struct field_mapping fields[] = { TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS, max_tdmrs), TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr), @@ -337,6 +337,11 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), tdmr_sysinfo); } +static int get_tdx_sysinfo(struct tdx_sysinfo *sysinfo) +{ + return get_tdx_tdmr_sysinfo(&sysinfo->tdmr_info); +} + /* Calculate the actual TDMR size */ static int tdmr_size_single(u16 max_reserved_per_tdmr) { @@ -353,7 +358,7 @@ 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_sysinfo_tdmr_info *tdmr_sysinfo) { size_t tdmr_sz, tdmr_array_sz; void *tdmr_array; @@ -936,7 +941,7 @@ 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_tdmr_sysinfo *tdmr_sysinfo) + struct tdx_sysinfo_tdmr_info *tdmr_sysinfo) { int ret; @@ -1109,9 +1114,13 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list) static int init_tdx_module(void) { - struct tdx_tdmr_sysinfo tdmr_sysinfo; + struct tdx_sysinfo sysinfo; int ret; + ret = get_tdx_sysinfo(&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 @@ -1128,17 +1137,13 @@ static int init_tdx_module(void) if (ret) goto out_put_tdxmem; - ret = get_tdx_tdmr_sysinfo(&tdmr_sysinfo); - 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_info); 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_info); 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 4e43cec19917..b5eb7c35f1dc 100644 --- a/arch/x86/virt/vmx/tdx/tdx.h +++ b/arch/x86/virt/vmx/tdx/tdx.h @@ -100,13 +100,6 @@ struct tdx_memblock { int nid; }; -/* "TDMR info" part of "Global Scope Metadata" for constructing TDMRs */ -struct tdx_tdmr_sysinfo { - 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 @@ -119,4 +112,29 @@ struct tdmr_info_list { int max_tdmrs; /* How many 'tdmr_info's are allocated */ }; +/* + * Kernel-defined structures to contain "Global Scope Metadata". + * + * TDX global metadata fields are categorized by "Class". See the + * "global_metadata.json" in the "TDX 1.5 ABI Definitions". + * + * 'struct tdx_sysinfo' 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 not all metadata fields in each class are defined, only those + * used by the kernel are. + */ + +/* Class "TDMR Info" */ +struct tdx_sysinfo_tdmr_info { + u16 max_tdmrs; + u16 max_reserved_per_tdmr; + u16 pamt_entry_size[TDX_PS_NR]; +}; + +struct tdx_sysinfo { + struct tdx_sysinfo_tdmr_info tdmr_info; +}; + #endif
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_sysinfo' to track all global metadata fields. TDX categories global metadata fields into different "Class"es. E.g., the current TDMR related fields are under class "TDMR Info". Instead of making 'struct tdx_sysinfo' 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 current construct_tdmr() can just take the structure which contains "TDMR Info" metadata fields. Start with moving 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo', and rename 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo_tdmr_info' to make it consistent with the "class name". Add a new function get_tdx_sysinfo() as the place to read all metadata fields, and call it at the beginning of init_tdx_module(). Move the existing get_tdx_tdmr_sysinfo() to get_tdx_sysinfo(). Note there is a functional change: get_tdx_tdmr_sysinfo() 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> --- arch/x86/virt/vmx/tdx/tdx.c | 29 +++++++++++++++++------------ arch/x86/virt/vmx/tdx/tdx.h | 32 +++++++++++++++++++++++++------- 2 files changed, 42 insertions(+), 19 deletions(-)