[5/5] x86/virt/tdx: Export global metadata read infrastructure

Commit Message

Huang, Kai March 1, 2024, 11:20 a.m. UTC

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.

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 <[e-mail]>
Reviewed-by: Kirill A. Shutemov <[e-mail]>
---
 arch/x86/include/asm/tdx.h  | 22 ++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++-----------------
 2 files changed, 30 insertions(+), 17 deletions(-)

Comments

Xiaoyao Li March 13, 2024, 3:44 a.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Kirill A. Shutemov <[e-mail]>
> ---
>   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 */

Huang, Kai March 15, 2024, 12:24 a.m. UTC | #2

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/[e-mail]/

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.

Xiaoyao Li March 15, 2024, 2:10 a.m. UTC | #3

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/[e-mail]/
> 
> 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.

Edgecombe, Rick P May 3, 2024, 12:21 a.m. UTC | #4

On Sat, 2024-03-02 at 00:20 +1300, 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.
> 
Could 2 be a static inline in a helper, and then only have one export?

Huang, Kai May 3, 2024, 2:17 a.m. UTC | #5

On 3/05/2024 12:21 pm, Edgecombe, Rick P wrote:
> On Sat, 2024-03-02 at 00:20 +1300, 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.
>>
> Could 2 be a static inline in a helper, and then only have one export?

I assume you were thinking about making 2) call the 1), so we don't need 
to export 2).

This is not feasible due to:

a). 1) must 'struct tdx_metadata_field_mapping' table as input, and for 
that we need to ues the TDX_SYSINFO_MAP() macro to define a structure 
for just one 'u64' and define a 'struct tdx_metadata_field_mapping' 
table which only has one element for that.

b). TDX_SYSINFO_MAP() macro actually doesn't support taking a metadata 
field "variable", but can only support using the name of the metadata field.

However we can try to make 1) as a wrapper of 2).  But this would 
require some change to the patch 4.

I'll reply separately to patch 4 and you can take a look whether that is 
better.

Edgecombe, Rick P May 3, 2024, 6:31 p.m. UTC | #6

On Fri, 2024-05-03 at 14:17 +1200, Huang, Kai wrote:
> 
> 
> On 3/05/2024 12:21 pm, Edgecombe, Rick P wrote:
> > On Sat, 2024-03-02 at 00:20 +1300, 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.
> > > 
> > Could 2 be a static inline in a helper, and then only have one export?
> 
> I assume you were thinking about making 2) call the 1), so we don't need 
> to export 2).
> 
> This is not feasible due to:
> 
> a). 1) must 'struct tdx_metadata_field_mapping' table as input, and for 
> that we need to ues the TDX_SYSINFO_MAP() macro to define a structure 
> for just one 'u64' and define a 'struct tdx_metadata_field_mapping' 
> table which only has one element for that.
> 
> b). TDX_SYSINFO_MAP() macro actually doesn't support taking a metadata 
> field "variable", but can only support using the name of the metadata field.
> 
> However we can try to make 1) as a wrapper of 2).  But this would 
> require some change to the patch 4.
> 
> I'll reply separately to patch 4 and you can take a look whether that is 
> better.

Having one export would be nice. Yea, since the multiple field processing
version just loops anyway, doing it like you propose seems reasonable.

Huang, Kai May 6, 2024, 11:35 a.m. UTC | #7

On Fri, 2024-05-03 at 18:31 +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-05-03 at 14:17 +1200, Huang, Kai wrote:
> > 
> > 
> > On 3/05/2024 12:21 pm, Edgecombe, Rick P wrote:
> > > On Sat, 2024-03-02 at 00:20 +1300, 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.
> > > > 
> > > Could 2 be a static inline in a helper, and then only have one export?
> > 
> > I assume you were thinking about making 2) call the 1), so we don't need 
> > to export 2).
> > 
> > This is not feasible due to:
> > 
> > a). 1) must 'struct tdx_metadata_field_mapping' table as input, and for 
> > that we need to ues the TDX_SYSINFO_MAP() macro to define a structure 
> > for just one 'u64' and define a 'struct tdx_metadata_field_mapping' 
> > table which only has one element for that.
> > 
> > b). TDX_SYSINFO_MAP() macro actually doesn't support taking a metadata 
> > field "variable", but can only support using the name of the metadata field.
> > 
> > However we can try to make 1) as a wrapper of 2).  But this would 
> > require some change to the patch 4.
> > 
> > I'll reply separately to patch 4 and you can take a look whether that is 
> > better.
> 
> Having one export would be nice. Yea, since the multiple field processing
> version just loops anyway, doing it like you propose seems reasonable.

Sure I'll switch to use the new code as replied in patch 4.

Dave Hansen May 6, 2024, 3:43 p.m. UTC | #8

On 3/1/24 03:20, 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.

All of this hinges on how KVM ends up using this infrastructure.

I want to see the whole picture before _any_ of this gets applied.
These 5 patches only vaguely hint at that bigger picture.

I know folks want to draw some kind of line and say "x86" over here and
"kvm" over there.  But that inclination really doesn't help anyone get
that big picture view.

Huang, Kai May 6, 2024, 10:13 p.m. UTC | #9

On 7/05/2024 3:43 am, Dave Hansen wrote:
> On 3/1/24 03:20, 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.
> 
> All of this hinges on how KVM ends up using this infrastructure.
> 
> I want to see the whole picture before _any_ of this gets applied.
> These 5 patches only vaguely hint at that bigger picture.
> 
> I know folks want to draw some kind of line and say "x86" over here and
> "kvm" over there.  But that inclination really doesn't help anyone get
> that big picture view.

Understood.  I'll work with Rick on this and get back to you guys.

Thanks for the feedback!

Patch

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 */