| Message ID | ec9fc9f1d45348ddfc73362ddfb310cc5f129646.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 3/1/2024 7:20 PM, Kai Huang wrote: > KVM will need to read a bunch of non-TDMR related metadata to create and > run TDX guests. Export the metadata read infrastructure for KVM to use. > > Specifically, export two helpers: > > 1) The helper which reads multiple metadata fields to a buffer of a > structure based on the "field ID -> structure member" mapping table. > > 2) The low level helper which just reads a given field ID. How about introducing a helper to read a single metadata field comparing to 1) instead of the low level helper. The low level helper tdx_sys_metadata_field_read() requires the data buf to be u64 *. So the caller needs to use a temporary variable and handle the memcpy when the field is less than 8 bytes. so why not expose a high level helper to read single field, e.g., +int tdx_sys_metadata_read_single(u64 field_id, int bytes, void *buf) +{ + return stbuf_read_sys_metadata_field(field_id, 0, bytes, buf); +} +EXPORT_SYMBOL_GPL(tdx_sys_metadata_read_single); > The two helpers cover cases when the user wants to cache a bunch of > metadata fields to a certain structure and when the user just wants to > query a specific metadata field on demand. They are enough for KVM to > use (and also should be enough for other potential users). > > Signed-off-by: Kai Huang <kai.huang@intel.com> > Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > --- > arch/x86/include/asm/tdx.h | 22 ++++++++++++++++++++++ > arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++----------------- > 2 files changed, 30 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h > index eba178996d84..709b9483f9e4 100644 > --- a/arch/x86/include/asm/tdx.h > +++ b/arch/x86/include/asm/tdx.h > @@ -116,6 +116,28 @@ static inline u64 sc_retry(sc_func_t func, u64 fn, > int tdx_cpu_enable(void); > int tdx_enable(void); > const char *tdx_dump_mce_info(struct mce *m); > + > +struct tdx_metadata_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), \ > + .size = sizeof(typeof(((_struct *)0)->_member)) } > + > +/* > + * Read multiple global metadata fields to a buffer of a structure > + * based on the "field ID -> structure member" mapping table. > + */ > +int tdx_sys_metadata_read(const struct tdx_metadata_field_mapping *fields, > + int nr_fields, void *stbuf); > + > +/* Read a single global metadata field */ > +int tdx_sys_metadata_field_read(u64 field_id, u64 *data); > + > #else > static inline void tdx_init(void) { } > static inline int tdx_cpu_enable(void) { return -ENODEV; } > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 4ee4b8cf377c..dc21310776ab 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -251,7 +251,7 @@ static int build_tdx_memlist(struct list_head *tmb_list) > return ret; > } > > -static int read_sys_metadata_field(u64 field_id, u64 *data) > +int tdx_sys_metadata_field_read(u64 field_id, u64 *data) > { > struct tdx_module_args args = {}; > int ret; > @@ -270,6 +270,7 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) > > return 0; > } > +EXPORT_SYMBOL_GPL(tdx_sys_metadata_field_read); > > /* Return the metadata field element size in bytes */ > static int get_metadata_field_bytes(u64 field_id) > @@ -295,7 +296,7 @@ static int stbuf_read_sys_metadata_field(u64 field_id, > if (WARN_ON_ONCE(get_metadata_field_bytes(field_id) != bytes)) > return -EINVAL; > > - ret = read_sys_metadata_field(field_id, &tmp); > + ret = tdx_sys_metadata_field_read(field_id, &tmp); > if (ret) > return ret; > > @@ -304,19 +305,8 @@ static int stbuf_read_sys_metadata_field(u64 field_id, > return 0; > } > > -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), \ > - .size = sizeof(typeof(((_struct *)0)->_member)) } > - > -static int read_sys_metadata(const struct field_mapping *fields, int nr_fields, > - void *stbuf) > +int tdx_sys_metadata_read(const struct tdx_metadata_field_mapping *fields, > + int nr_fields, void *stbuf) > { > int i, ret; > > @@ -331,6 +321,7 @@ static int read_sys_metadata(const struct field_mapping *fields, int nr_fields, > > return 0; > } > +EXPORT_SYMBOL_GPL(tdx_sys_metadata_read); > > #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) > @@ -338,7 +329,7 @@ static int read_sys_metadata(const struct field_mapping *fields, int nr_fields, > 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[] = { > + const struct tdx_metadata_field_mapping fields[] = { > 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]), > @@ -347,7 +338,7 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) > }; > > /* Populate 'tdmr_sysinfo' fields using the mapping structure above: */ > - return read_sys_metadata(fields, ARRAY_SIZE(fields), tdmr_sysinfo); > + return tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdmr_sysinfo); > } > > /* Calculate the actual TDMR size */
On 13/03/2024 4:44 pm, Xiaoyao Li wrote: > On 3/1/2024 7:20 PM, Kai Huang wrote: >> KVM will need to read a bunch of non-TDMR related metadata to create and >> run TDX guests. Export the metadata read infrastructure for KVM to use. >> >> Specifically, export two helpers: >> >> 1) The helper which reads multiple metadata fields to a buffer of a >> structure based on the "field ID -> structure member" mapping table. >> >> 2) The low level helper which just reads a given field ID. > > How about introducing a helper to read a single metadata field comparing > to 1) instead of the low level helper. > > The low level helper tdx_sys_metadata_field_read() requires the data buf > to be u64 *. So the caller needs to use a temporary variable and handle > the memcpy when the field is less than 8 bytes. > > so why not expose a high level helper to read single field, e.g., > > +int tdx_sys_metadata_read_single(u64 field_id, int bytes, void *buf) > +{ > + return stbuf_read_sys_metadata_field(field_id, 0, bytes, buf); > +} > +EXPORT_SYMBOL_GPL(tdx_sys_metadata_read_single); As replied here where these APIs are (supposedly) to be used: https://lore.kernel.org/kvm/e88e5448-e354-4ec6-b7de-93dd8f7786b5@intel.com/ I don't see why we need to use a temporary 'u64'. We can just use it directly or type cast to 'u16' when needed, which has the same result of doing explicit memory copy based on size. So I am not convinced at this stage that we need the code as you suggested. At least I believe the current APIs are sufficient for KVM to use. However I'll put more background on how KVM is going to use into the changelog to justify the current APIs are enough.
On 3/15/2024 8:24 AM, Huang, Kai wrote: > > > On 13/03/2024 4:44 pm, Xiaoyao Li wrote: >> On 3/1/2024 7:20 PM, Kai Huang wrote: >>> KVM will need to read a bunch of non-TDMR related metadata to create and >>> run TDX guests. Export the metadata read infrastructure for KVM to use. >>> >>> Specifically, export two helpers: >>> >>> 1) The helper which reads multiple metadata fields to a buffer of a >>> structure based on the "field ID -> structure member" mapping table. >>> >>> 2) The low level helper which just reads a given field ID. >> >> How about introducing a helper to read a single metadata field >> comparing to 1) instead of the low level helper. >> >> The low level helper tdx_sys_metadata_field_read() requires the data >> buf to be u64 *. So the caller needs to use a temporary variable and >> handle the memcpy when the field is less than 8 bytes. >> >> so why not expose a high level helper to read single field, e.g., >> >> +int tdx_sys_metadata_read_single(u64 field_id, int bytes, void *buf) >> +{ >> + return stbuf_read_sys_metadata_field(field_id, 0, bytes, buf); >> +} >> +EXPORT_SYMBOL_GPL(tdx_sys_metadata_read_single); > > As replied here where these APIs are (supposedly) to be used: > > https://lore.kernel.org/kvm/e88e5448-e354-4ec6-b7de-93dd8f7786b5@intel.com/ > > I don't see why we need to use a temporary 'u64'. We can just use it > directly or type cast to 'u16' when needed, which has the same result of > doing explicit memory copy based on size. The way to cast a u64 to u16 is based on the fact that the variable is u64 at first. Given u16 feild_x; We have to have a u64 tmp, passed to tdx_sys_metadata_field_read() to hold the output of metadata read, then filed_x = (u16) tmp; If we pass field_x into tdx_sys_metadata_field_read(), the following (64-16) bits might be corrupted. > So I am not convinced at this stage that we need the code as you > suggested. At least I believe the current APIs are sufficient for KVM > to use. > > However I'll put more background on how KVM is going to use into the > changelog to justify the current APIs are enough.
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h index eba178996d84..709b9483f9e4 100644 --- a/arch/x86/include/asm/tdx.h +++ b/arch/x86/include/asm/tdx.h @@ -116,6 +116,28 @@ static inline u64 sc_retry(sc_func_t func, u64 fn, int tdx_cpu_enable(void); int tdx_enable(void); const char *tdx_dump_mce_info(struct mce *m); + +struct tdx_metadata_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), \ + .size = sizeof(typeof(((_struct *)0)->_member)) } + +/* + * Read multiple global metadata fields to a buffer of a structure + * based on the "field ID -> structure member" mapping table. + */ +int tdx_sys_metadata_read(const struct tdx_metadata_field_mapping *fields, + int nr_fields, void *stbuf); + +/* Read a single global metadata field */ +int tdx_sys_metadata_field_read(u64 field_id, u64 *data); + #else static inline void tdx_init(void) { } static inline int tdx_cpu_enable(void) { return -ENODEV; } diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 4ee4b8cf377c..dc21310776ab 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -251,7 +251,7 @@ static int build_tdx_memlist(struct list_head *tmb_list) return ret; } -static int read_sys_metadata_field(u64 field_id, u64 *data) +int tdx_sys_metadata_field_read(u64 field_id, u64 *data) { struct tdx_module_args args = {}; int ret; @@ -270,6 +270,7 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) return 0; } +EXPORT_SYMBOL_GPL(tdx_sys_metadata_field_read); /* Return the metadata field element size in bytes */ static int get_metadata_field_bytes(u64 field_id) @@ -295,7 +296,7 @@ static int stbuf_read_sys_metadata_field(u64 field_id, if (WARN_ON_ONCE(get_metadata_field_bytes(field_id) != bytes)) return -EINVAL; - ret = read_sys_metadata_field(field_id, &tmp); + ret = tdx_sys_metadata_field_read(field_id, &tmp); if (ret) return ret; @@ -304,19 +305,8 @@ static int stbuf_read_sys_metadata_field(u64 field_id, return 0; } -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), \ - .size = sizeof(typeof(((_struct *)0)->_member)) } - -static int read_sys_metadata(const struct field_mapping *fields, int nr_fields, - void *stbuf) +int tdx_sys_metadata_read(const struct tdx_metadata_field_mapping *fields, + int nr_fields, void *stbuf) { int i, ret; @@ -331,6 +321,7 @@ static int read_sys_metadata(const struct field_mapping *fields, int nr_fields, return 0; } +EXPORT_SYMBOL_GPL(tdx_sys_metadata_read); #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) @@ -338,7 +329,7 @@ static int read_sys_metadata(const struct field_mapping *fields, int nr_fields, 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[] = { + const struct tdx_metadata_field_mapping fields[] = { 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]), @@ -347,7 +338,7 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) }; /* Populate 'tdmr_sysinfo' fields using the mapping structure above: */ - return read_sys_metadata(fields, ARRAY_SIZE(fields), tdmr_sysinfo); + return tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdmr_sysinfo); } /* Calculate the actual TDMR size */