| Message ID | 393931ee1d8f0dfb199b3e81aa660f2af0351129.1709288433.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host: Provide TDX module metadata reading APIs | expand |
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote: > +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > + TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) > > static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) > { > /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ > 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]), > + TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS, max_tdmrs), > + TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, > max_reserved_per_tdmr), > + TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE, > pamt_entry_size[TDX_PS_4K]), > + TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE, > pamt_entry_size[TDX_PS_2M]), > + TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE, > pamt_entry_size[TDX_PS_1G]), The creation of TD_SYSINFO_MAP_TDMR_INFO part is not strictly needed, but makes sense in the context of the signature change in read_sys_metadata_field16(). It might be worth justifying it in the log.
On 3/05/2024 12:12 pm, Edgecombe, Rick P wrote: > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote: >> +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ >> + TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) >> >> static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) >> { >> /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ >> 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]), >> + TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS, max_tdmrs), >> + TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, >> max_reserved_per_tdmr), >> + TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE, >> pamt_entry_size[TDX_PS_4K]), >> + TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE, >> pamt_entry_size[TDX_PS_2M]), >> + TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE, >> pamt_entry_size[TDX_PS_1G]), > > The creation of TD_SYSINFO_MAP_TDMR_INFO part is not strictly needed, but makes > sense in the context of the signature change in read_sys_metadata_field16(). It > might be worth justifying it in the log. I see your point. How about adding below paragraph to the end of this changelog? " The metadata reading code uses the TD_SYSINFO_MAP() macro to describe the mapping between the metadata fields and the members of the 'struct tdx_tdmr_sysinfo'. I.e., it hard-codes the 'struct tdx_tdmr_sysinfo' inside the macro. As part of unbinding metadata read with 'struct tdx_tdmr_sysinfo', the TD_SYSINFO_MAP() macro needs to be changed to additionally take the structure as argument so it can accept any structure. That would make the current code to read TDMR related metadata fields longer if using TD_SYSINFO_MAP() directly. Define a wrapper macro for reading TDMR related metadata fields to make the code shorter. " By typing, it reminds me that I kinda need to learn how to separate the "high level design" vs "low level implementation details". I think the latter can be seen easily in the code, and probably can be avoided in the changelog. I am not sure whether adding the TD_SYSINFO_MAP_TDMR_INFO() macro belong to which category, especially when I needed a lot text to justify this change (thus I wonder whether it is worth to do). Or any shorter version that you can suggest? Thanks.
On Fri, 2024-05-03 at 12:52 +1200, Huang, Kai wrote: > " > The metadata reading code uses the TD_SYSINFO_MAP() macro to describe > the mapping between the metadata fields and the members of the 'struct > tdx_tdmr_sysinfo'. I.e., it hard-codes the 'struct tdx_tdmr_sysinfo' > inside the macro. How about: The TDX module initialization code currently uses the metadata reading infrastructure to read several TDX module fields, and populate them all into the same kernel defined struct, "struct tdx_tdmr_sysinfo". So the helper macros for marshaling the data from the TDX module into the struct fields hardcode that struct name. > > As part of unbinding metadata read with 'struct tdx_tdmr_sysinfo', the > TD_SYSINFO_MAP() macro needs to be changed to additionally take the > structure as argument so it can accept any structure. That would make > the current code to read TDMR related metadata fields longer if using > TD_SYSINFO_MAP() directly. Future changes will allow for other types of metadata to be read, that don't make sense to populate to that specific struct. To accommodate this the data marshaling macro, TD_SYSINFO_MAP, will be extended to take different structs. Unfortunately, it will result in the usage of TD_SYSINFO_MAP for populating struct tdx_tdmr_sysinfo to change to... [some undesirable situation]. Question for you: Is this just to make it shorter, or to avoid duplication of specifying the struct name? Like is it a mitigation for exceeding 80 chars or 100? > > Define a wrapper macro for reading TDMR related metadata fields to make > the code shorter. > " > > By typing, it reminds me that I kinda need to learn how to separate the > "high level design" vs "low level implementation details". I think the > latter can be seen easily in the code, and probably can be avoided in > the changelog. Especially for TDX with all it's complexity and acronyms I think it helps to explain in simple terms. Like imagine if someone was working at their computer and you tapped on their shoulder, how would you introduce this change? If you start with "TDMR related global metadata fields" and "struct tdx_tdmr_sysinfo" they are going to have to struggle to context switch into it. For each patch, if the connection is not clear, ease them into it. Of course everyone has the different preferences, so YMMV. But especially the tip folks seem to appreciate it. > > I am not sure whether adding the TD_SYSINFO_MAP_TDMR_INFO() macro belong > to which category, especially when I needed a lot text to justify this > change (thus I wonder whether it is worth to do). > > Or any shorter version that you can suggest? > I don't think it is too long.
On Fri, 2024-05-03 at 19:03 +0000, Edgecombe, Rick P wrote: > On Fri, 2024-05-03 at 12:52 +1200, Huang, Kai wrote: > > " > > The metadata reading code uses the TD_SYSINFO_MAP() macro to describe > > the mapping between the metadata fields and the members of the 'struct > > tdx_tdmr_sysinfo'. I.e., it hard-codes the 'struct tdx_tdmr_sysinfo' > > inside the macro. > > How about: > > The TDX module initialization code currently uses the metadata reading > infrastructure to read several TDX module fields, and populate them all into the > same kernel defined struct, "struct tdx_tdmr_sysinfo". So the helper macros for > marshaling the data from the TDX module into the struct fields hardcode that > struct name. > > > > > As part of unbinding metadata read with 'struct tdx_tdmr_sysinfo', the > > TD_SYSINFO_MAP() macro needs to be changed to additionally take the > > structure as argument so it can accept any structure. That would make > > the current code to read TDMR related metadata fields longer if using > > TD_SYSINFO_MAP() directly. > > Future changes will allow for other types of metadata to be read, that don't > make sense to populate to that specific struct. To accommodate this the data > marshaling macro, TD_SYSINFO_MAP, will be extended to take different structs. > Unfortunately, it will result in the usage of TD_SYSINFO_MAP for populating > struct tdx_tdmr_sysinfo to change to... [some undesirable situation]. I'll change to use your words, with some small tweaks to also mention the function to read metadata field should also be relaxed to take a typeless 'void *' buffer. Please see below. > > Question for you: > Is this just to make it shorter, or to avoid duplication of specifying the > struct name? > The intention was to make it shorter, but I think both. > Like is it a mitigation for exceeding 80 chars or 100? Yes for not exceeding 100. With this patch, the code actually exceeds 80 chars, but I found breaking them to separate lines hurt the readability. > > > > > Define a wrapper macro for reading TDMR related metadata fields to make > > the code shorter. > > " > > > > By typing, it reminds me that I kinda need to learn how to separate the > > "high level design" vs "low level implementation details". I think the > > latter can be seen easily in the code, and probably can be avoided in > > the changelog. > > Especially for TDX with all it's complexity and acronyms I think it helps to > explain in simple terms. Like imagine if someone was working at their computer > and you tapped on their shoulder, how would you introduce this change? If you > start with "TDMR related global metadata fields" and "struct tdx_tdmr_sysinfo" > they are going to have to struggle to context switch into it. > > For each patch, if the connection is not clear, ease them into it. Of course > everyone has the different preferences, so YMMV. But especially the tip folks > seem to appreciate it. > > > > > I am not sure whether adding the TD_SYSINFO_MAP_TDMR_INFO() macro belong > > to which category, especially when I needed a lot text to justify this > > change (thus I wonder whether it is worth to do). > > > > Or any shorter version that you can suggest? > > > > I don't think it is too long. The new changelog based on your words: The TDX module initialization code currently uses the metadata reading infrastructure to read several TDX module fields, and populate them all into the same kernel defined struct, "struct tdx_tdmr_sysinfo". So the function to read the metadata fields and the helper macros for marshaling the data from the TDX module into the struct fields hardcode that struct name. Future changes will allow for other types of metadata to be read, that don't make sense to populate to that specific struct. To accommodate this, change the metadata reading function to take a typeless 'void *' buffer, and extend the data marshaling macro, TD_SYSINFO_MAP, to take different structs. Unfortunately, this will result in the usage of TD_SYSINFO_MAP for populating 'struct tdx_tdmr_sysinfo' to be changed to use the struct name explicitly for each struct member and make the code longer. Define a wrapper macro for reading TDMR related metadata fields to make the code shorter, i.e., not exceeding the 100 characters limit while still keeping the use of TDX_SYSINFO_MAP for each struct member in one line for better readability.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index cdcb3332bc5d..eb208da4ff63 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -273,9 +273,9 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) static int read_sys_metadata_field16(u64 field_id, int offset, - struct tdx_tdmr_sysinfo *ts) + void *stbuf) { - u16 *ts_member = ((void *)ts) + offset; + u16 *st_member = stbuf + offset; u64 tmp; int ret; @@ -287,7 +287,7 @@ static int read_sys_metadata_field16(u64 field_id, if (ret) return ret; - *ts_member = tmp; + *st_member = tmp; return 0; } @@ -297,19 +297,22 @@ struct field_mapping { int offset; }; -#define TD_SYSINFO_MAP(_field_id, _member) \ - { .field_id = MD_FIELD_ID_##_field_id, \ - .offset = offsetof(struct tdx_tdmr_sysinfo, _member) } +#define TD_SYSINFO_MAP(_field_id, _struct, _member) \ + { .field_id = MD_FIELD_ID_##_field_id, \ + .offset = offsetof(_struct, _member) } + +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ + TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) { /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ 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]), + TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS, max_tdmrs), + TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr), + TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]), + TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]), + TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]), }; int ret; int i;