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@xxxxxxxxxxxxxxxxxxxxxxxxx.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@xxxxxxxxx/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.
