| Message ID | 17f1c66ae6360b14f175c45aa486f4bdcf6e0a20.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, Mar 02, 2024 at 12:20:36AM +1300, Kai Huang <kai.huang@intel.com> wrote: > For now the kernel only reads TDMR related global metadata fields for > module initialization. All these fields are 16-bits, and the kernel > only supports reading 16-bits fields. > > KVM will need to read a bunch of non-TDMR related metadata to create and > run TDX guests. It's essential to provide a generic metadata read > infrastructure which supports reading all 8/16/32/64 bits element sizes. > > Extend the metadata read to support reading all these element sizes. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > --- > arch/x86/virt/vmx/tdx/tdx.c | 59 +++++++++++++++++++++++++------------ > arch/x86/virt/vmx/tdx/tdx.h | 2 -- > 2 files changed, 40 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index eb208da4ff63..4ee4b8cf377c 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -271,23 +271,35 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) > return 0; > } > > -static int read_sys_metadata_field16(u64 field_id, > - int offset, > - void *stbuf) > +/* Return the metadata field element size in bytes */ > +static int get_metadata_field_bytes(u64 field_id) > { > - u16 *st_member = stbuf + offset; > + /* > + * TDX supports 8/16/32/64 bits metadata field element sizes. > + * TDX module determines the metadata element size based on the > + * "element size code" encoded in the field ID (see the comment > + * of MD_FIELD_ID_ELE_SIZE_CODE macro for specific encodings). > + */ > + return 1 << MD_FIELD_ID_ELE_SIZE_CODE(field_id); > +} > + > +static int stbuf_read_sys_metadata_field(u64 field_id, > + int offset, > + int bytes, > + void *stbuf) > +{ > + void *st_member = stbuf + offset; > u64 tmp; > int ret; > > - if (WARN_ON_ONCE(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != > - MD_FIELD_ID_ELE_SIZE_16BIT)) > + if (WARN_ON_ONCE(get_metadata_field_bytes(field_id) != bytes)) > return -EINVAL; > > ret = read_sys_metadata_field(field_id, &tmp); > if (ret) > return ret; > > - *st_member = tmp; > + memcpy(st_member, &tmp, bytes); > > return 0; > } > @@ -295,11 +307,30 @@ static int read_sys_metadata_field16(u64 field_id, > struct field_mapping { > u64 field_id; > int offset; > + int size; > }; > > #define TD_SYSINFO_MAP(_field_id, _struct, _member) \ > { .field_id = MD_FIELD_ID_##_field_id, \ > - .offset = offsetof(_struct, _member) } > + .offset = offsetof(_struct, _member), \ > + .size = sizeof(typeof(((_struct *)0)->_member)) } Because we use compile time constant for _field_id mostly, can we add build time check? Something like this. static inline metadata_size_check(u64 field_id, size_t size) { BUILD_BUG_ON(get_metadata_field_bytes(field_id) != size); } #define TD_SYSINFO_MAP(_field_id, _struct, _member) \ { .field_id = MD_FIELD_ID_##_field_id, \ .offset = offsetof(_struct, _member), \ .size = \ ({ size_t s = sizeof(typeof(((_struct *)0)->_member)); \ metadata_size_check(MD_FIELD_ID_##_field_id, s); \ s; }) }
> > @@ -295,11 +307,30 @@ static int read_sys_metadata_field16(u64 field_id, > > struct field_mapping { > > u64 field_id; > > int offset; > > + int size; > > }; > > > > #define TD_SYSINFO_MAP(_field_id, _struct, _member) \ > > { .field_id = MD_FIELD_ID_##_field_id, \ > > - .offset = offsetof(_struct, _member) } > > + .offset = offsetof(_struct, _member), \ > > + .size = sizeof(typeof(((_struct *)0)->_member)) } > > Because we use compile time constant for _field_id mostly, can we add build > time check? Something like this. > > static inline metadata_size_check(u64 field_id, size_t size) > { > BUILD_BUG_ON(get_metadata_field_bytes(field_id) != size); > } > > #define TD_SYSINFO_MAP(_field_id, _struct, _member) \ > { .field_id = MD_FIELD_ID_##_field_id, \ > .offset = offsetof(_struct, _member), \ > .size = \ > ({ size_t s = sizeof(typeof(((_struct *)0)->_member)); \ > metadata_size_check(MD_FIELD_ID_##_field_id, s); \ > s; }) } > Hmm.. The problem is "mostly" as you mentioned? My understanding is BUILD_BUG_ON() relies on the "condition" to be compile-time constant. In your KVM TDX patchset there's code to do ... for (i = 0; i < NR_WHATEVER; i++) { const struct tdx_metadata_field_mapping fields = { TD_SYSINFO_MAP(FIELD_WHATEVERE + i, ...), ... }; ... } To be honest I am not exactly sure whether the compiler can determine "FIELD_WHATEVER + i" as compile-time constant. Btw, if there's any mismatch, the current code can already catch during runtime. I think one purpose (perhaps the most important purpose) of BUILD_BUG_ON() is to catch bug early if someone changed the constant (macros etc) in the "condition". But in our case, once written no one is going to change the structure or the metadata fields. So I am not sure whether it's worth to do.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index eb208da4ff63..4ee4b8cf377c 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -271,23 +271,35 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) return 0; } -static int read_sys_metadata_field16(u64 field_id, - int offset, - void *stbuf) +/* Return the metadata field element size in bytes */ +static int get_metadata_field_bytes(u64 field_id) { - u16 *st_member = stbuf + offset; + /* + * TDX supports 8/16/32/64 bits metadata field element sizes. + * TDX module determines the metadata element size based on the + * "element size code" encoded in the field ID (see the comment + * of MD_FIELD_ID_ELE_SIZE_CODE macro for specific encodings). + */ + return 1 << MD_FIELD_ID_ELE_SIZE_CODE(field_id); +} + +static int stbuf_read_sys_metadata_field(u64 field_id, + int offset, + int bytes, + void *stbuf) +{ + void *st_member = stbuf + offset; u64 tmp; int ret; - if (WARN_ON_ONCE(MD_FIELD_ID_ELE_SIZE_CODE(field_id) != - MD_FIELD_ID_ELE_SIZE_16BIT)) + if (WARN_ON_ONCE(get_metadata_field_bytes(field_id) != bytes)) return -EINVAL; ret = read_sys_metadata_field(field_id, &tmp); if (ret) return ret; - *st_member = tmp; + memcpy(st_member, &tmp, bytes); return 0; } @@ -295,11 +307,30 @@ static int read_sys_metadata_field16(u64 field_id, struct field_mapping { u64 field_id; int offset; + int size; }; #define TD_SYSINFO_MAP(_field_id, _struct, _member) \ { .field_id = MD_FIELD_ID_##_field_id, \ - .offset = offsetof(_struct, _member) } + .offset = offsetof(_struct, _member), \ + .size = sizeof(typeof(((_struct *)0)->_member)) } + +static int read_sys_metadata(const struct field_mapping *fields, int nr_fields, + void *stbuf) +{ + int i, ret; + + for (i = 0; i < nr_fields; i++) { + ret = stbuf_read_sys_metadata_field(fields[i].field_id, + fields[i].offset, + fields[i].size, + stbuf); + if (ret) + return ret; + } + + return 0; +} #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) @@ -314,19 +345,9 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) 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; /* Populate 'tdmr_sysinfo' 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, - tdmr_sysinfo); - if (ret) - return ret; - } - - return 0; + return read_sys_metadata(fields, ARRAY_SIZE(fields), tdmr_sysinfo); } /* Calculate the actual TDMR size */ diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h index b701f69485d3..4c32c8bf156a 100644 --- a/arch/x86/virt/vmx/tdx/tdx.h +++ b/arch/x86/virt/vmx/tdx/tdx.h @@ -53,8 +53,6 @@ #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id) \ (((_field_id) & GENMASK_ULL(33, 32)) >> 32) -#define MD_FIELD_ID_ELE_SIZE_16BIT 1 - struct tdmr_reserved_area { u64 offset; u64 size;