| Message ID | 0403cdb142b40b9838feeb222eb75a4831f6b46d.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: > 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 fields > for module initialization. There are both immediate needs to read more > fields for module initialization and near-future needs for other kernel > components like KVM to run TDX guests. > will be organized in different structures depending on their meanings. Line above seems lost > > For now the kernel only reads TDMR related fields. The TD_SYSINFO_MAP() > macro hard-codes the 'struct tdx_sys_info_tdmr' instance name. To make > it work with different instances of different structures, extend it to > take the structure instance name as an argument. > > This also means the current code which uses TD_SYSINFO_MAP() must type > 'struct tdx_sys_info_tdmr' instance name explicitly for each use. To > make the code easier to read, add a wrapper TD_SYSINFO_MAP_TDMR_INFO() > which hides the instance name. Note, were you to accept my suggestion for patch 2, TD_SYSINFO_MAP() would have gone away, and no changes would be needed to get_tdx_sys_info_tdmr(). So the above 2 paragraphs could be dropped. > > TDX also support 8/16/32/64 bits metadata field element sizes. For now > all TDMR related fields are 16-bit long thus the kernel only has one > helper: > > static int read_sys_metadata_field16(u64 field_id, u16 *val) {} > > Future changes will need to read more metadata fields with different > element sizes. To make the code short, extend the helper to take a > 'void *' buffer and the buffer size so it can work with all element > sizes. > > Note in this way the helper loses the type safety and the build-time > check inside the helper cannot work anymore because the compiler cannot > determine the exact value of the buffer size. > > To resolve those, add a wrapper of the helper which only works with > u8/u16/u32/u64 directly and do build-time check, where the compiler > can easily know both the element size (from field ID) and the buffer > size(using sizeof()), before calling the helper. > > An alternative option is to provide one helper for each element size: IMHO, it is not necessary to describe alternative options. Better to explain why the actual code change is good, rather than why something that wasn't done, wasn't so good. That discussion can stay on the mailing list. For example, this commit message can just have: Add a build-time check that the element size is correct, which enables a common function to safely read data of different sizes. Perhaps end up with: TDX supports 8/16/32/64 bit metadata field element sizes. For now all TDMR related fields are 16-bits long. Future changes will need to read more metadata fields with different element sizes. So add a build-time check that the element size is correct, which enables a common function to safely read data of different sizes. > > static int read_sys_metadata_field8(u64 field_id, u8 *val) {} > static int read_sys_metadata_field16(u64 field_id, u16 *val) {} > ... > > But this will result in duplicated code given those helpers will look > exactly the same except for the type of buffer pointer. It's feasible > to define a macro for the body of the helper and define one entry for > each element size to reduce the code, but it is a little bit > over-engineering. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > > v2 -> v3: > - Rename read_sys_metadata_field() to tdh_sys_rd() so the former can be > used as the high level wrapper. Get rid of "stbuf_" prefix since > people don't like it. > > - Rewrite after removing 'struct field_mapping' and reimplementing > TD_SYSINFO_MAP(). > > --- > arch/x86/virt/vmx/tdx/tdx.c | 45 +++++++++++++++++++++---------------- > arch/x86/virt/vmx/tdx/tdx.h | 3 ++- > 2 files changed, 28 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 7e75c1b10838..1cd9035c783f 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -250,7 +250,7 @@ static int build_tdx_memlist(struct list_head *tmb_list) > return ret; > } > > -static int read_sys_metadata_field(u64 field_id, u64 *data) > +static int tdh_sys_rd(u64 field_id, u64 *data) > { > struct tdx_module_args args = {}; > int ret; > @@ -270,43 +270,50 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) > return 0; > } > > -static int read_sys_metadata_field16(u64 field_id, u16 *val) > +static int __read_sys_metadata_field(u64 field_id, void *val, int size) > { > u64 tmp; > int ret; > > - 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); > + ret = tdh_sys_rd(field_id, &tmp); > if (ret) > return ret; > > - *val = tmp; > + memcpy(val, &tmp, size); > > return 0; > } > > +/* Wrapper to read one global metadata to u8/u16/u32/u64 */ > +#define read_sys_metadata_field(_field_id, _val) \ > + ({ \ > + BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val)))); \ > + __read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val)))); \ > + }) > + > /* > - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error > - * code and the 'struct tdx_sys_info_tdmr' instance to fill out. > + * Read one global metadata field to a member of a structure instance, > + * assuming locally defined @ret to convey the error code. > */ > -#define TD_SYSINFO_MAP(_field_id, _member) \ > - ({ \ > - if (!ret) \ > - ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > - &sysinfo_tdmr->_member); \ > +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member) \ > + ({ \ > + if (!ret) \ > + ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ > + &_stbuf->_member); \ > }) > > static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) > { > int ret = 0; > > - 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 TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > + TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member) > + > + 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]); > > return ret; > } > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index 148f9b4d1140..7458f6717873 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -53,7 +53,8 @@ > #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id) \ > (((_field_id) & GENMASK_ULL(33, 32)) >> 32) > > -#define MD_FIELD_ID_ELE_SIZE_16BIT 1 > +#define MD_FIELD_ELE_SIZE(_field_id) \ > + (1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id)) > > struct tdmr_reserved_area { > u64 offset;
On 27.08.24 г. 10:14 ч., 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. > > For now the kernel only reads "TD Memory Region" (TDMR) related fields > for module initialization. There are both immediate needs to read more > fields for module initialization and near-future needs for other kernel > components like KVM to run TDX guests. > will be organized in different structures depending on their meanings. > > For now the kernel only reads TDMR related fields. The TD_SYSINFO_MAP() > macro hard-codes the 'struct tdx_sys_info_tdmr' instance name. To make > it work with different instances of different structures, extend it to > take the structure instance name as an argument. > > This also means the current code which uses TD_SYSINFO_MAP() must type > 'struct tdx_sys_info_tdmr' instance name explicitly for each use. To > make the code easier to read, add a wrapper TD_SYSINFO_MAP_TDMR_INFO() > which hides the instance name. > > TDX also support 8/16/32/64 bits metadata field element sizes. For now > all TDMR related fields are 16-bit long thus the kernel only has one > helper: > > static int read_sys_metadata_field16(u64 field_id, u16 *val) {} > > Future changes will need to read more metadata fields with different > element sizes. To make the code short, extend the helper to take a > 'void *' buffer and the buffer size so it can work with all element > sizes. > > Note in this way the helper loses the type safety and the build-time > check inside the helper cannot work anymore because the compiler cannot > determine the exact value of the buffer size. > > To resolve those, add a wrapper of the helper which only works with > u8/u16/u32/u64 directly and do build-time check, where the compiler > can easily know both the element size (from field ID) and the buffer > size(using sizeof()), before calling the helper. > > An alternative option is to provide one helper for each element size: > > static int read_sys_metadata_field8(u64 field_id, u8 *val) {} > static int read_sys_metadata_field16(u64 field_id, u16 *val) {} > ... > > But this will result in duplicated code given those helpers will look > exactly the same except for the type of buffer pointer. It's feasible > to define a macro for the body of the helper and define one entry for > each element size to reduce the code, but it is a little bit > over-engineering. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > > v2 -> v3: > - Rename read_sys_metadata_field() to tdh_sys_rd() so the former can be > used as the high level wrapper. Get rid of "stbuf_" prefix since > people don't like it. > > - Rewrite after removing 'struct field_mapping' and reimplementing > TD_SYSINFO_MAP(). > > --- > arch/x86/virt/vmx/tdx/tdx.c | 45 +++++++++++++++++++++---------------- > arch/x86/virt/vmx/tdx/tdx.h | 3 ++- > 2 files changed, 28 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 7e75c1b10838..1cd9035c783f 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -250,7 +250,7 @@ static int build_tdx_memlist(struct list_head *tmb_list) > return ret; > } > > -static int read_sys_metadata_field(u64 field_id, u64 *data) > +static int tdh_sys_rd(u64 field_id, u64 *data) > { > struct tdx_module_args args = {}; > int ret; > @@ -270,43 +270,50 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) > return 0; > } > > -static int read_sys_metadata_field16(u64 field_id, u16 *val) > +static int __read_sys_metadata_field(u64 field_id, void *val, int size) The type of 'size' should be size_t. > { > u64 tmp; > int ret; > > - 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); > + ret = tdh_sys_rd(field_id, &tmp); > if (ret) > return ret; > > - *val = tmp; > + memcpy(val, &tmp, size); > > return 0; > } > > +/* Wrapper to read one global metadata to u8/u16/u32/u64 */ > +#define read_sys_metadata_field(_field_id, _val) \ > + ({ \ > + BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val)))); \ > + __read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val)))); \ > + }) > + > /* > - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error > - * code and the 'struct tdx_sys_info_tdmr' instance to fill out. > + * Read one global metadata field to a member of a structure instance, > + * assuming locally defined @ret to convey the error code. > */ > -#define TD_SYSINFO_MAP(_field_id, _member) \ > - ({ \ > - if (!ret) \ > - ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > - &sysinfo_tdmr->_member); \ > +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member) \ > + ({ \ > + if (!ret) \ > + ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ > + &_stbuf->_member); \ > }) > > static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) > { > int ret = 0; > > - 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 TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > + TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member) nit: I guess its a personal preference but honestly I think the amount of macro indirection (3 levels) here is crazy, despite each being rather simple. Just use TD_SYSINFO_MAP directly, saving the typing of "sysinfo_tdmr" doesn't seem like a big deal. You can probably take it even a bit further and simply opencode read_sys_metadata_field macro inside TD_SYSINFO_MAP and be left with just it, no ? No other patch in this series uses read_sys_metadata_field stand alone, if anything factoring it out could be deferred until the first users gets introduced. > + > + 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]); > > return ret; > } > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index 148f9b4d1140..7458f6717873 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -53,7 +53,8 @@ > #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id) \ > (((_field_id) & GENMASK_ULL(33, 32)) >> 32) > > -#define MD_FIELD_ID_ELE_SIZE_16BIT 1 > +#define MD_FIELD_ELE_SIZE(_field_id) \ That ELE seems a bit ambiguous, ELEM seems more natural and is in line with other macros in the kernel. > + (1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id)) > > struct tdmr_reserved_area { > u64 offset;
> > > > For now the kernel only reads "TD Memory Region" (TDMR) related fields > > for module initialization. There are both immediate needs to read more > > fields for module initialization and near-future needs for other kernel > > components like KVM to run TDX guests. > > will be organized in different structures depending on their meanings. > > Line above seems lost Hmm.. It should be removed. I thought I have done the spell check for all the patches :-( > > > > > For now the kernel only reads TDMR related fields. The TD_SYSINFO_MAP() > > macro hard-codes the 'struct tdx_sys_info_tdmr' instance name. To make > > it work with different instances of different structures, extend it to > > take the structure instance name as an argument. > > > > This also means the current code which uses TD_SYSINFO_MAP() must type > > 'struct tdx_sys_info_tdmr' instance name explicitly for each use. To > > make the code easier to read, add a wrapper TD_SYSINFO_MAP_TDMR_INFO() > > which hides the instance name. > > Note, were you to accept my suggestion for patch 2, TD_SYSINFO_MAP() would > have gone away, and no changes would be needed to get_tdx_sys_info_tdmr(). > So the above 2 paragraphs could be dropped. Yeah seems better to me. I'll use your way unless someone objects. > > > > > TDX also support 8/16/32/64 bits metadata field element sizes. For now > > all TDMR related fields are 16-bit long thus the kernel only has one > > helper: > > > > static int read_sys_metadata_field16(u64 field_id, u16 *val) {} > > > > Future changes will need to read more metadata fields with different > > element sizes. To make the code short, extend the helper to take a > > 'void *' buffer and the buffer size so it can work with all element > > sizes. > > > > Note in this way the helper loses the type safety and the build-time > > check inside the helper cannot work anymore because the compiler cannot > > determine the exact value of the buffer size. > > > > To resolve those, add a wrapper of the helper which only works with > > u8/u16/u32/u64 directly and do build-time check, where the compiler > > can easily know both the element size (from field ID) and the buffer > > size(using sizeof()), before calling the helper. > > > > An alternative option is to provide one helper for each element size: > > IMHO, it is not necessary to describe alternative options. > I am not sure? My understanding is we should mention those alternatives in the changelog so that the reviewers can have a better view? [...]
> > -static int read_sys_metadata_field16(u64 field_id, u16 *val) > > +static int __read_sys_metadata_field(u64 field_id, void *val, int size) > > The type of 'size' should be size_t. OK will do. > > > { > > u64 tmp; > > int ret; > > > > - 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); > > + ret = tdh_sys_rd(field_id, &tmp); > > if (ret) > > return ret; > > > > - *val = tmp; > > + memcpy(val, &tmp, size); > > > > return 0; > > } > > > > +/* Wrapper to read one global metadata to u8/u16/u32/u64 */ > > +#define read_sys_metadata_field(_field_id, _val) \ > > + ({ \ > > + BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val)))); \ > > + __read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val)))); \ > > + }) > > + > > /* > > - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error > > - * code and the 'struct tdx_sys_info_tdmr' instance to fill out. > > + * Read one global metadata field to a member of a structure instance, > > + * assuming locally defined @ret to convey the error code. > > */ > > -#define TD_SYSINFO_MAP(_field_id, _member) \ > > - ({ \ > > - if (!ret) \ > > - ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ > > - &sysinfo_tdmr->_member); \ > > +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member) \ > > + ({ \ > > + if (!ret) \ > > + ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ > > + &_stbuf->_member); \ > > }) > > > > static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) > > { > > int ret = 0; > > > > - 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 TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > > + TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member) > > nit: I guess its a personal preference but honestly I think the amount > of macro indirection (3 levels) here is crazy, despite each being rather > simple. Just use TD_SYSINFO_MAP directly, saving the typing of > "sysinfo_tdmr" doesn't seem like a big deal. We have a different interpretation of "crazy" :-) > > You can probably take it even a bit further and simply opencode > read_sys_metadata_field macro inside TD_SYSINFO_MAP and be left with > just it, no ? No other patch in this series uses read_sys_metadata_field > stand alone, if anything factoring it out could be deferred until the > first users gets introduced. Joke aside, anyway, with what Adrian suggested we just need the TD_SYSINFO_MAP() but don't need those wrappers anymore. I'll go with this if no one says differently. > > > + > > + 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]); > > > > return ret; > > } > > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > > index 148f9b4d1140..7458f6717873 100644 > > --- a/arch/x86/virt/vmx/tdx/tdx.h > > +++ b/arch/x86/virt/vmx/tdx/tdx.h > > @@ -53,7 +53,8 @@ > > #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id) \ > > (((_field_id) & GENMASK_ULL(33, 32)) >> 32) > > > > -#define MD_FIELD_ID_ELE_SIZE_16BIT 1 > > +#define MD_FIELD_ELE_SIZE(_field_id) \ > > That ELE seems a bit ambiguous, ELEM seems more natural and is in line > with other macros in the kernel. OK will do. Thanks for the review!
Kai Huang wrote: > Future changes will need to read more metadata fields with different > element sizes. To make the code short, extend the helper to take a > 'void *' buffer and the buffer size so it can work with all element > sizes. The whole point was to have compile time type safety for global struct member and the read routine. So, no, using 'void *' is a step backwards. Take some inspiration from build_mmio_read() and just have a macro that takes the size as an argument to the macro, not the runtime routine. The macro statically selects the right routine to call and does not need to grow a size parameter itself.
On Fri, 2024-09-06 at 14:34 -0700, Dan Williams wrote: > Kai Huang wrote: > > Future changes will need to read more metadata fields with different > > element sizes. To make the code short, extend the helper to take a > > 'void *' buffer and the buffer size so it can work with all element > > sizes. > > The whole point was to have compile time type safety for global struct > member and the read routine. So, no, using 'void *' is a step backwards. > > Take some inspiration from build_mmio_read() and just have a macro that > takes the size as an argument to the macro, not the runtime routine. > AFAICT build_mmio_read() macro defines the body of the assembly to read requested size from a port. But instead of a single macro which can be used universally for all port reading, the kernel uses build_mmio_read() to define multiple read functions: build_mmio_read(readb, "b", unsigned char, "=q", :"memory") build_mmio_read(readw, "w", unsigned short, "=r", :"memory") build_mmio_read(readl, "l", unsigned int, "=r", :"memory") ... So just to understand correctly, do you mean something like below? #define build_sysmd_read(_bits) \ static __maybe_unused int \ __read_sys_metadata_field##_bits(u64 field_id, u##_bits *val) \ { \ u64 tmp; \ int ret; \ \ ret = tdh_sys_rd(field_id, &tmp); \ if (ret) \ return ret; \ *val = tmp; \ return ret; \ } build_sysmd_read(8) build_sysmd_read(16) build_sysmd_read(32) build_sysmd_read(64) > The > macro statically selects the right routine to call and does not need to > grow a size parameter itself. Sorry for my ignorance. Do you mean using switch-case statement to select right routine?
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 7e75c1b10838..1cd9035c783f 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -250,7 +250,7 @@ static int build_tdx_memlist(struct list_head *tmb_list) return ret; } -static int read_sys_metadata_field(u64 field_id, u64 *data) +static int tdh_sys_rd(u64 field_id, u64 *data) { struct tdx_module_args args = {}; int ret; @@ -270,43 +270,50 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) return 0; } -static int read_sys_metadata_field16(u64 field_id, u16 *val) +static int __read_sys_metadata_field(u64 field_id, void *val, int size) { u64 tmp; int ret; - 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); + ret = tdh_sys_rd(field_id, &tmp); if (ret) return ret; - *val = tmp; + memcpy(val, &tmp, size); return 0; } +/* Wrapper to read one global metadata to u8/u16/u32/u64 */ +#define read_sys_metadata_field(_field_id, _val) \ + ({ \ + BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val)))); \ + __read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val)))); \ + }) + /* - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error - * code and the 'struct tdx_sys_info_tdmr' instance to fill out. + * Read one global metadata field to a member of a structure instance, + * assuming locally defined @ret to convey the error code. */ -#define TD_SYSINFO_MAP(_field_id, _member) \ - ({ \ - if (!ret) \ - ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ - &sysinfo_tdmr->_member); \ +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member) \ + ({ \ + if (!ret) \ + ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ + &_stbuf->_member); \ }) static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) { int ret = 0; - 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 TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ + TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member) + + 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]); return ret; } diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h index 148f9b4d1140..7458f6717873 100644 --- a/arch/x86/virt/vmx/tdx/tdx.h +++ b/arch/x86/virt/vmx/tdx/tdx.h @@ -53,7 +53,8 @@ #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id) \ (((_field_id) & GENMASK_ULL(33, 32)) >> 32) -#define MD_FIELD_ID_ELE_SIZE_16BIT 1 +#define MD_FIELD_ELE_SIZE(_field_id) \ + (1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id)) struct tdmr_reserved_area { u64 offset;
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 fields for module initialization. There are both immediate needs to read more fields for module initialization and near-future needs for other kernel components like KVM to run TDX guests. will be organized in different structures depending on their meanings. For now the kernel only reads TDMR related fields. The TD_SYSINFO_MAP() macro hard-codes the 'struct tdx_sys_info_tdmr' instance name. To make it work with different instances of different structures, extend it to take the structure instance name as an argument. This also means the current code which uses TD_SYSINFO_MAP() must type 'struct tdx_sys_info_tdmr' instance name explicitly for each use. To make the code easier to read, add a wrapper TD_SYSINFO_MAP_TDMR_INFO() which hides the instance name. TDX also support 8/16/32/64 bits metadata field element sizes. For now all TDMR related fields are 16-bit long thus the kernel only has one helper: static int read_sys_metadata_field16(u64 field_id, u16 *val) {} Future changes will need to read more metadata fields with different element sizes. To make the code short, extend the helper to take a 'void *' buffer and the buffer size so it can work with all element sizes. Note in this way the helper loses the type safety and the build-time check inside the helper cannot work anymore because the compiler cannot determine the exact value of the buffer size. To resolve those, add a wrapper of the helper which only works with u8/u16/u32/u64 directly and do build-time check, where the compiler can easily know both the element size (from field ID) and the buffer size(using sizeof()), before calling the helper. An alternative option is to provide one helper for each element size: static int read_sys_metadata_field8(u64 field_id, u8 *val) {} static int read_sys_metadata_field16(u64 field_id, u16 *val) {} ... But this will result in duplicated code given those helpers will look exactly the same except for the type of buffer pointer. It's feasible to define a macro for the body of the helper and define one entry for each element size to reduce the code, but it is a little bit over-engineering. Signed-off-by: Kai Huang <kai.huang@intel.com> --- v2 -> v3: - Rename read_sys_metadata_field() to tdh_sys_rd() so the former can be used as the high level wrapper. Get rid of "stbuf_" prefix since people don't like it. - Rewrite after removing 'struct field_mapping' and reimplementing TD_SYSINFO_MAP(). --- arch/x86/virt/vmx/tdx/tdx.c | 45 +++++++++++++++++++++---------------- arch/x86/virt/vmx/tdx/tdx.h | 3 ++- 2 files changed, 28 insertions(+), 20 deletions(-)