| Message ID | 9eb6b2e3577be66ea2f711e37141ca021bf0159b.1724741926.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 |
On 27/08/24 10:14, Kai Huang wrote: Subject could be "Simplify the reading of Global Metadata Fields" > TL;DR: Remove the 'struct field_mapping' structure and use another way > to implement the TD_SYSINFO_MAP() macro to improve the current metadata > reading code, e.g., switching to use build-time check for metadata field > size over the existing runtime check. Perhaps: Remove 'struct field_mapping' and let read_sys_metadata_field16() accept the member variable address, simplifying the code in preparation for adding support for more metadata structures and field sizes. > > The TDX module provides a set of "global metadata fields". They report Global Metadata Fields > things like TDX module version, supported features, and fields related > to create/run TDX guests and so on. > > For now the kernel only reads "TD Memory Region" (TDMR) related global > metadata fields, and all of those fields are organized in one structure. The patch is self-evidently simpler (21 insertions(+), 36 deletions(-)) so there doesn't seem to be any need for further elaboration. Perhaps just round it off and stop there. ... and all of those fields are organized in one structure, but that will change in the near future. > > The kernel currently uses 'struct field_mapping' to facilitate reading > multiple metadata fields into one structure. The 'struct field_mapping' > records the mapping between the field ID and the offset of the structure > to fill out. The kernel initializes an array of 'struct field_mapping' > for each structure member (using the TD_SYSINFO_MAP() macro) and then > reads all metadata fields in a loop using that array. > > Currently the kernel only reads TDMR related metadata fields into one > structure, and the function to read one metadata field takes the pointer > of that structure and the offset: > > static int read_sys_metadata_field16(u64 field_id, > int offset, > struct tdx_sys_info_tdmr *ts) > {...} > > Future changes will need to read more metadata fields into different > structures. To support this the above function will need to be changed > to take 'void *': > > static int read_sys_metadata_field16(u64 field_id, > int offset, > void *stbuf) > {...} > > This approach loses type-safety, as Dan suggested. A better way is to > make it be .. > > static int read_sys_metadata_field16(u64 field_id, u16 *val) {...} > > .. and let the caller calculate the buffer in a type-safe way [1]. > > Also, the using of the 'struct field_mapping' loses the ability to be > able to do build-time check about the metadata field size (encoded in > the field ID) due to the field ID is "indirectly" initialized to the > 'struct field_mapping' and passed to the function to read. Thus the > current code uses runtime check instead. > > Dan suggested to remove the 'struct field_mapping', unroll the loop, > skip the array, and call the read_sys_metadata_field16() directly with > build-time check [1][2]. And to improve the readability, reimplement > the TD_SYSINFO_MAP() [3]. > > The new TD_SYSINFO_MAP() isn't perfect. 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 overall the pros of this > approach beat the cons. > > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1] > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#mbe65f0903ff7835bc418a907f0d02d7a9e0b78be [2] > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m80cde5e6504b3af74d933ea0cbfc3ca9d24697d3 [3] Probably just one link would suffice, say the permalink to Dan's comment: https://lore.kernel.org/kvm/66b16121c48f4_4fc729424@dwillia2-xfh.jf.intel.com.notmuch/ > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > > v2 -> v3: > - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP(). > > --- > arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++----------------------- > 1 file changed, 21 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index e979bf442929..7e75c1b10838 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -270,60 +270,45 @@ 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; > + BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != > + MD_FIELD_ID_ELE_SIZE_16BIT); This gets removed next patch, so why do it > > 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]), > -}; > +/* > + * Assumes locally defined @ret and @sysinfo_tdmr to convey the error > + * code and the 'struct tdx_sys_info_tdmr' instance to fill out. > + */ > +#define TD_SYSINFO_MAP(_field_id, _member) \ "MAP" made sense when it was in a struct whereas now it is reading. > + ({ \ > + if (!ret) \ > + ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > + &sysinfo_tdmr->_member); \ > + }) > > 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; > - } > + 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]); Another possibility is to put the macro at the invocation site: #define READ_SYS_INFO(_field_id, _member) \ ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ &sysinfo_tdmr->_member) 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 And so on in later patches: #define READ_SYS_INFO(_field_id, _member) \ ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ &sysinfo_version->_member) READ_SYS_INFO(MAJOR_VERSION, major); READ_SYS_INFO(MINOR_VERSION, minor); READ_SYS_INFO(UPDATE_VERSION, update); READ_SYS_INFO(INTERNAL_VERSION, internal); READ_SYS_INFO(BUILD_NUM, build_num); READ_SYS_INFO(BUILD_DATE, build_date); #undef READ_SYS_INFO > > - return 0; > + return ret; > } > > /* Calculate the actual TDMR size */
On Thu, 2024-08-29 at 10:20 +0300, Adrian Hunter wrote: > On 27/08/24 10:14, Kai Huang wrote: > > Subject could be "Simplify the reading of Global Metadata Fields" OK. > > > TL;DR: Remove the 'struct field_mapping' structure and use another way > > to implement the TD_SYSINFO_MAP() macro to improve the current metadata > > reading code, e.g., switching to use build-time check for metadata field > > size over the existing runtime check. > > Perhaps: > > Remove 'struct field_mapping' and let read_sys_metadata_field16() accept > the member variable address, simplifying the code in preparation for adding > support for more metadata structures and field sizes. Fine to me. I would like also to mention there are improvements in the new code (as suggested by Dan), so I will say: "..., simplifying and improving the code in preparation ...". > > > > > The TDX module provides a set of "global metadata fields". They report > > Global Metadata Fields OK. > > > things like TDX module version, supported features, and fields related > > to create/run TDX guests and so on. > > > > For now the kernel only reads "TD Memory Region" (TDMR) related global > > metadata fields, and all of those fields are organized in one structure. > > The patch is self-evidently simpler (21 insertions(+), 36 deletions(-)) > so there doesn't seem to be any need for further elaboration. Perhaps > just round it off and stop there. > > ... and all of those fields are organized in one structure, > but that will change in the near future. I think the code change here is not only to reduce the LoC (i.e., simplify the code), but also improve the code, so I would like to keep the things mentioned by Dan in the changelog. > > > > > The kernel currently uses 'struct field_mapping' to facilitate reading > > multiple metadata fields into one structure. The 'struct field_mapping' > > records the mapping between the field ID and the offset of the structure > > to fill out. The kernel initializes an array of 'struct field_mapping' > > for each structure member (using the TD_SYSINFO_MAP() macro) and then > > reads all metadata fields in a loop using that array. > > > > Currently the kernel only reads TDMR related metadata fields into one > > structure, and the function to read one metadata field takes the pointer > > of that structure and the offset: > > > > static int read_sys_metadata_field16(u64 field_id, > > int offset, > > struct tdx_sys_info_tdmr *ts) > > {...} > > > > Future changes will need to read more metadata fields into different > > structures. To support this the above function will need to be changed > > to take 'void *': > > > > static int read_sys_metadata_field16(u64 field_id, > > int offset, > > void *stbuf) > > {...} > > > > This approach loses type-safety, as Dan suggested. A better way is to > > make it be .. > > > > static int read_sys_metadata_field16(u64 field_id, u16 *val) {...} > > > > .. and let the caller calculate the buffer in a type-safe way [1]. > > > > Also, the using of the 'struct field_mapping' loses the ability to be > > able to do build-time check about the metadata field size (encoded in > > the field ID) due to the field ID is "indirectly" initialized to the > > 'struct field_mapping' and passed to the function to read. Thus the > > current code uses runtime check instead. > > > > Dan suggested to remove the 'struct field_mapping', unroll the loop, > > skip the array, and call the read_sys_metadata_field16() directly with > > build-time check [1][2]. And to improve the readability, reimplement > > the TD_SYSINFO_MAP() [3]. > > > > The new TD_SYSINFO_MAP() isn't perfect. 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 overall the pros of this > > approach beat the cons. > > > > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1] > > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#mbe65f0903ff7835bc418a907f0d02d7a9e0b78be [2] > > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m80cde5e6504b3af74d933ea0cbfc3ca9d24697d3 [3] > > Probably just one link would suffice, say the permalink to Dan's > comment: > > https://lore.kernel.org/kvm/66b16121c48f4_4fc729424@dwillia2-xfh.jf.intel.com.notmuch/ [1] and [2] are actually replies to different patches, so I would like to keep them. [1] and [3] are replies to the same patch, so I could remove [3], but I think keeping all of them is also fine since it's more easy to find the exact comment that I want to address. > > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > > > v2 -> v3: > > - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP(). > > > > --- > > arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++----------------------- > > 1 file changed, 21 insertions(+), 36 deletions(-) > > > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > > index e979bf442929..7e75c1b10838 100644 > > --- a/arch/x86/virt/vmx/tdx/tdx.c > > +++ b/arch/x86/virt/vmx/tdx/tdx.c > > @@ -270,60 +270,45 @@ 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; > > + BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != > > + MD_FIELD_ID_ELE_SIZE_16BIT); > > This gets removed next patch, so why do it This patch I didn't add the build-time check in the (new) TD_SYSINFO_MAP(), so I changed the runtime check to build-time check here since we can actually do it here. I think even for this middle state patch we should do the build-time check. I can move it to the (new) TD_SYSINFO_MAP() though if that's better. > > > > > 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]), > > -}; > > +/* > > + * Assumes locally defined @ret and @sysinfo_tdmr to convey the error > > + * code and the 'struct tdx_sys_info_tdmr' instance to fill out. > > + */ > > +#define TD_SYSINFO_MAP(_field_id, _member) \ > > "MAP" made sense when it was in a struct whereas > now it is reading. > > > + ({ \ > > + if (!ret) \ > > + ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > > + &sysinfo_tdmr->_member); \ > > + }) > > > > 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; > > - } > > + 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]); > > Another possibility is to put the macro at the invocation site: > > #define READ_SYS_INFO(_field_id, _member) \ > ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > &sysinfo_tdmr->_member) > > 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 > > And so on in later patches: > > #define READ_SYS_INFO(_field_id, _member) \ > ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ > &sysinfo_version->_member) > > READ_SYS_INFO(MAJOR_VERSION, major); > READ_SYS_INFO(MINOR_VERSION, minor); > READ_SYS_INFO(UPDATE_VERSION, update); > READ_SYS_INFO(INTERNAL_VERSION, internal); > READ_SYS_INFO(BUILD_NUM, build_num); > READ_SYS_INFO(BUILD_DATE, build_date); > > #undef READ_SYS_INFO > This is fine to me. But AFAICT Kirill doesn't like the "#undef" part. Kirill, do you have comments? Otherwise I will go with what Adrian suggested.
I think the subject buries the lead on what this patch does which is more like: x86/virt/tdx: Rework TD_SYSINFO_MAP to support build-time verification Kai Huang wrote: > TL;DR: Remove the 'struct field_mapping' structure and use another way I would drop the TL;DR: and just make the changelog more concise, because as it stands now it requires the reader to fully appreciate the direction of the v1 approach which this new proposal abandons: Something like: 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. The new TD_SYSINFO_MAP() 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 verfiable, 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] [..] > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1] The expectation for lore links is to capture the message-id. Note the differences with the "Link:" format above. > v2 -> v3: > - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP(). > > --- > arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++----------------------- > 1 file changed, 21 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index e979bf442929..7e75c1b10838 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -270,60 +270,45 @@ 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; > + BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != > + MD_FIELD_ID_ELE_SIZE_16BIT); Perhaps just move this to TD_SYSINFO_MAP() directly? Something like: #define TD_SYSINFO_MAP(_field_id, _member, _size) \ ({ \ BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != \ MD_FIELD_ID_ELE_SIZE_##_size##BIT); \ if (!ret) \ ret = read_sys_metadata_field##_size(MD_FIELD_ID_##_field_id, \ &sysinfo_tdmr->_member); \ })
Adrian Hunter wrote: [..] > Another possibility is to put the macro at the invocation site: > > #define READ_SYS_INFO(_field_id, _member) \ > ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > &sysinfo_tdmr->_member) > > 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 > > And so on in later patches: > > #define READ_SYS_INFO(_field_id, _member) \ > ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ > &sysinfo_version->_member) > > READ_SYS_INFO(MAJOR_VERSION, major); > READ_SYS_INFO(MINOR_VERSION, minor); > READ_SYS_INFO(UPDATE_VERSION, update); > READ_SYS_INFO(INTERNAL_VERSION, internal); > READ_SYS_INFO(BUILD_NUM, build_num); > READ_SYS_INFO(BUILD_DATE, build_date); > > #undef READ_SYS_INFO Looks like a reasonable enhancement to me.
On Fri, 2024-09-06 at 13:21 -0700, Dan Williams wrote: > I think the subject buries the lead on what this patch does which is > more like: > > x86/virt/tdx: Rework TD_SYSINFO_MAP to support build-time verification Yes this looks better. Thanks. > > Kai Huang wrote: > > TL;DR: Remove the 'struct field_mapping' structure and use another way > > I would drop the TL;DR: and just make the changelog more concise, > because as it stands now it requires the reader to fully appreciate the > direction of the v1 approach which this new proposal abandons: > > Something like: > > 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. > > The new TD_SYSINFO_MAP() 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 verfiable, 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] Thanks. Will do. Btw, Adrian suggested to rename TD_SYSINFO_MAP() to READ_SYS_INFO(), which is reasonable to me, so I will also add "rename TD_SYSINFO_MAP() to READ_SYS_INFO()" to your above text. Please let know if you have any comments. > > [..] > > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1] > > The expectation for lore links is to capture the message-id. Note the > differences with the "Link:" format above. Oh I did not know this. What's the difference between message-id and a normal link that I got by "google <patch name> + open that lore link"? > > > v2 -> v3: > > - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP(). > > > > --- > > arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++----------------------- > > 1 file changed, 21 insertions(+), 36 deletions(-) > > > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > > index e979bf442929..7e75c1b10838 100644 > > --- a/arch/x86/virt/vmx/tdx/tdx.c > > +++ b/arch/x86/virt/vmx/tdx/tdx.c > > @@ -270,60 +270,45 @@ 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; > > + BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != > > + MD_FIELD_ID_ELE_SIZE_16BIT); > > Perhaps just move this to TD_SYSINFO_MAP() directly? Sure will do. It gets moved to TD_SYSINFO_MAP() in the next patch anyway. > > Something like: > > #define TD_SYSINFO_MAP(_field_id, _member, _size) \ > ({ \ > BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != \ > MD_FIELD_ID_ELE_SIZE_##_size##BIT); \ > if (!ret) \ > ret = read_sys_metadata_field##_size(MD_FIELD_ID_##_field_id, \ > &sysinfo_tdmr->_member); \ > }) Will do. Btw this patch doesn't extend to support other sizes but only 16-bits, so I'll defer the support of "_size" to the next patch.
On Fri, 2024-09-06 at 14:30 -0700, Dan Williams wrote: > Adrian Hunter wrote: > [..] > > Another possibility is to put the macro at the invocation site: > > > > #define READ_SYS_INFO(_field_id, _member) \ > > ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > > &sysinfo_tdmr->_member) > > > > 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 > > > > And so on in later patches: > > > > #define READ_SYS_INFO(_field_id, _member) \ > > ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ > > &sysinfo_version->_member) > > > > READ_SYS_INFO(MAJOR_VERSION, major); > > READ_SYS_INFO(MINOR_VERSION, minor); > > READ_SYS_INFO(UPDATE_VERSION, update); > > READ_SYS_INFO(INTERNAL_VERSION, internal); > > READ_SYS_INFO(BUILD_NUM, build_num); > > READ_SYS_INFO(BUILD_DATE, build_date); > > > > #undef READ_SYS_INFO > > Looks like a reasonable enhancement to me. Yeah will do. Thanks.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index e979bf442929..7e75c1b10838 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -270,60 +270,45 @@ 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; + BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != + MD_FIELD_ID_ELE_SIZE_16BIT); 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]), -}; +/* + * Assumes locally defined @ret and @sysinfo_tdmr to convey the error + * code and the 'struct tdx_sys_info_tdmr' instance to fill out. + */ +#define TD_SYSINFO_MAP(_field_id, _member) \ + ({ \ + if (!ret) \ + ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ + &sysinfo_tdmr->_member); \ + }) 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; - } + 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]); - return 0; + return ret; } /* Calculate the actual TDMR size */
TL;DR: Remove the 'struct field_mapping' structure and use another way to implement the TD_SYSINFO_MAP() macro to improve the current metadata reading code, e.g., switching to use build-time check for metadata field size over the existing runtime check. 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. For now the kernel only reads "TD Memory Region" (TDMR) related global metadata fields, and all of those fields are organized in one structure. The kernel currently uses 'struct field_mapping' to facilitate reading multiple metadata fields into one structure. The 'struct field_mapping' records the mapping between the field ID and the offset of the structure to fill out. The kernel initializes an array of 'struct field_mapping' for each structure member (using the TD_SYSINFO_MAP() macro) and then reads all metadata fields in a loop using that array. Currently the kernel only reads TDMR related metadata fields into one structure, and the function to read one metadata field takes the pointer of that structure and the offset: static int read_sys_metadata_field16(u64 field_id, int offset, struct tdx_sys_info_tdmr *ts) {...} Future changes will need to read more metadata fields into different structures. To support this the above function will need to be changed to take 'void *': static int read_sys_metadata_field16(u64 field_id, int offset, void *stbuf) {...} This approach loses type-safety, as Dan suggested. A better way is to make it be .. static int read_sys_metadata_field16(u64 field_id, u16 *val) {...} .. and let the caller calculate the buffer in a type-safe way [1]. Also, the using of the 'struct field_mapping' loses the ability to be able to do build-time check about the metadata field size (encoded in the field ID) due to the field ID is "indirectly" initialized to the 'struct field_mapping' and passed to the function to read. Thus the current code uses runtime check instead. Dan suggested to remove the 'struct field_mapping', unroll the loop, skip the array, and call the read_sys_metadata_field16() directly with build-time check [1][2]. And to improve the readability, reimplement the TD_SYSINFO_MAP() [3]. The new TD_SYSINFO_MAP() isn't perfect. 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 overall the pros of this approach beat the cons. Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1] Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#mbe65f0903ff7835bc418a907f0d02d7a9e0b78be [2] Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m80cde5e6504b3af74d933ea0cbfc3ca9d24697d3 [3] Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Kai Huang <kai.huang@intel.com> --- v2 -> v3: - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP(). --- arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++----------------------- 1 file changed, 21 insertions(+), 36 deletions(-)