| Message ID | f3c63fb80e0de56e15348d078aa3ba1b1aa9b3c6.1728903647.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 10/14/24 04:31, Kai Huang wrote: > +#define READ_SYS_INFO(_field_id, _member) \ > + ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > + &sysinfo_tdmr->_member) > > - return 0; > + 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]); I know what Dan asked for here, but I dislike how this ended up. The existing stuff *has* type safety, despite the void*. It at least checks the size, which is the biggest problem. Also, this isn't really an unrolled loop. It still effectively has gotos, just like the for loop did. It just buries the goto in the "ret = ret ?: " construct. It hides the control flow logic. Logically, this whole function is ret = read_something1(); if (ret) goto out; ret = read_something2(); if (ret) goto out; ... I'd *much* rather have that goto be: for () { ret = read_something(); if (ret) break; // aka. goto out } Than have something *look* like straight control flow when it isn't.
Dave Hansen wrote: > On 10/14/24 04:31, Kai Huang wrote: > > +#define READ_SYS_INFO(_field_id, _member) \ > > + ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > > + &sysinfo_tdmr->_member) > > > > - return 0; > > + 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]); > > I know what Dan asked for here, but I dislike how this ended up. > > The existing stuff *has* type safety, despite the void*. It at least > checks the size, which is the biggest problem. > > Also, this isn't really an unrolled loop. It still effectively has > gotos, just like the for loop did. It just buries the goto in the "ret > = ret ?: " construct. It hides the control flow logic. > > Logically, this whole function is > > ret = read_something1(); > if (ret) > goto out; > > ret = read_something2(); > if (ret) > goto out; > > ... > > I'd *much* rather have that goto be: > > for () { > ret = read_something(); > if (ret) > break; // aka. goto out > } > > Than have something *look* like straight control flow when it isn't. Yeah, the hiding of the control flow was the weakest part of the suggestion. My main gripe was runtime validation of details that could be validated at compile time. There is no real need for control flow at all, i.e. early exit is not needed as these are not resources that need to be unwound. It simply needs to count whether all of the reads happened, so something like this is sufficient: success += READ_SYS_INFO(MAX_TDMRS, max_tdmrs); success += READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr); success += READ_SYS_INFO(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]); success += READ_SYS_INFO(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]); success += READ_SYS_INFO(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]); if (success != 5) return false;
On Mon, 2024-10-14 at 12:13 -0700, Dan Williams wrote: > Dave Hansen wrote: > > On 10/14/24 04:31, Kai Huang wrote: > > > +#define READ_SYS_INFO(_field_id, _member) \ > > > + ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > > > + &sysinfo_tdmr->_member) > > > > > > - return 0; > > > + 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]); > > > > I know what Dan asked for here, but I dislike how this ended up. > > > > The existing stuff *has* type safety, despite the void*. It at least > > checks the size, which is the biggest problem. > > > > Also, this isn't really an unrolled loop. It still effectively has > > gotos, just like the for loop did. It just buries the goto in the "ret > > = ret ?: " construct. It hides the control flow logic. > > > > Logically, this whole function is > > > > ret = read_something1(); > > if (ret) > > goto out; > > > > ret = read_something2(); > > if (ret) > > goto out; > > > > ... > > > > I'd *much* rather have that goto be: > > > > for () { > > ret = read_something(); > > if (ret) > > break; // aka. goto out > > } > > > > Than have something *look* like straight control flow when it isn't. Yeah understood. Thanks for letting me know. The 'for() loop' approach would need the original 'struct field_mapping' to hold the mapping between field ID and the offset/size info, though. > > Yeah, the hiding of the control flow was the weakest part of the > suggestion. My main gripe was runtime validation of details that could > be validated at compile time. I am looking into how to do build-time verification while still using the original 'struct field_mapping' approach. If we can do this, I hope this can address your concern about doing runtime check instead of build-time? Adrian provided one suggestion [*] that we can use __builtin_choose_expr() to achieve this: " BUILD_BUG_ON() requires a function, but it is still be possible to add a build time check in TD_SYSINFO_MAP e.g. #define TD_SYSINFO_CHECK_SIZE(_field_id, _size) \ __builtin_choose_expr(MD_FIELD_ELE_SIZE(_field_id) == _size, _size, (void)0) #define _TD_SYSINFO_MAP(_field_id, _offset, _size) \ { .field_id = _field_id, \ .offset = _offset, \ .size = TD_SYSINFO_CHECK_SIZE(_field_id, _size) } #define TD_SYSINFO_MAP(_field_id, _struct, _member) \ _TD_SYSINFO_MAP(MD_FIELD_ID_##_field_id, \ offsetof(_struct, _member), \ sizeof(typeof(((_struct *)0)->_member))) " I tried this, and it worked for most cases where the field ID is a simple integer constant, but I got build error for the CMRs: for (u16 i = 0; i < cmr_info->num_cmrs; i++) { const struct field_mapping fields[] = { TD_SYSINFO_CMRINFO_MAP(CMR_BASE0 + i, cmr_base[i]), TD_SYSINFO_CMRINFO_MAP(CMR_SIZE0 + i, cmr_size[i]), }; ... } .. where field ID for CMR[i] is calculated by CMR0. The MD_FIELD_ELE_SIZE(_field_id) works for 'CMR_BASE0 + i' for BUILD_BUG_ON(), but somehow the compiler fails to determine the 'MD_FIELD_ELE_SIZE(_field_id) == _size' as a constant_express and caused build failure. I am still looking into this. [*] https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m379ce041f025dc20e7b58fa6dbdc484c2ce53af4 > There is no real need for control flow at all, i.e. early exit is not > needed as these are not resources that need to be unwound. It simply > needs to count whether all of the reads happened, so something like this > is sufficient: > > success += READ_SYS_INFO(MAX_TDMRS, max_tdmrs); > success += READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr); > success += READ_SYS_INFO(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]); > success += READ_SYS_INFO(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]); > success += READ_SYS_INFO(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]); > > if (success != 5) > return false; > If we go with this approach, it seems we can even get rid of the @success. int ret = 0; #define READ_SYS_INFO(_field_id, _member) \ read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ &sysinfo_tdmr->_member) ret |= READ_SYS_INFO(MAX_TDMRS, max_tdmrs); ... #undef READ_SYS_INFO return ret; The tdh_sys_rd() always return -EIO when TDH.SYS.RD fails, so the above either return 0 when all reads were successful or -EIO when there's any failed. I can also go with route if Dave is fine?
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index e979bf442929..2f7e4abc1bb9 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -270,60 +270,44 @@ 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; - 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]), -}; +#define read_sys_metadata_field16(_field_id, _val) \ +({ \ + BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(_field_id) != \ + MD_FIELD_ID_ELE_SIZE_16BIT); \ + __read_sys_metadata_field16(_field_id, _val); \ +}) 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; - } +#define READ_SYS_INFO(_field_id, _member) \ + ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ + &sysinfo_tdmr->_member) - return 0; + 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 + + return ret; } /* Calculate the actual TDMR size */
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. Rename TD_SYSINFO_MAP() to READ_SYS_INFO() since it no longer stores the mapping of metadata field ID and the structure member offset. For now READ_SYS_INFO() is only used in get_tdx_sys_info_tdmr(). Future changes will need to read other metadata fields that are organized in different structures. Do #undef READ_SYS_INFO() after use so the same pattern can be used for reading other metadata fields. To avoid needing to duplicate build-time verification in each READ_SYS_INFO() in future changes, add a wrapper macro to do build-time verification and call it from READ_SYS_INFO(). The READ_SYS_INFO() 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 verifiable, 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] Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Kai Huang <kai.huang@intel.com> --- v4 -> v5: - No change v3 -> v4: - Rename TD_SYSINFO_MAP() to READ_SYS_INFO() - Ardian. - #undef READ_SYS_INFO() - Ardian. - Rewrite changelog based on text from Dan, with some clarification around using READ_SYS_INFO() and #undef it. - Move the BUILD_BUG_ON() out of read_sys_metadata_field16() - Dan. - Use permalink in changelog - Dan. v2 -> v3: - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP(). --- arch/x86/virt/vmx/tdx/tdx.c | 58 ++++++++++++++----------------------- 1 file changed, 21 insertions(+), 37 deletions(-)