diff mbox series

[3/9] x86/virt/tdx: Support global metadata read for all element sizes

Message ID 210f7747058e01c4d2ed683660a4cb18c5d88440.1718538552.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

Commit Message

Kai Huang June 16, 2024, 12:01 p.m. UTC
The TDX module provides "global metadata fields" for software to query.
Each metadata field is accessible by a unique "metadata field ID".  TDX
supports 8/16/32/64 bits metadata element sizes.  The size of each
metadata field is encoded in its associated metadata field ID.

For now the kernel only reads "TD Memory Region" (TDMR) related global
metadata fields for module initialization.  All these metadata fields
are 16-bit, and the kernel only supports reading 16-bit fields.

Future changes will need to read more metadata fields with other element
sizes.  To resolve this once for all, extend the existing metadata
reading code to support reading all 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 | 29 ++++++++++++++++-------------
 arch/x86/virt/vmx/tdx/tdx.h |  3 ++-
 2 files changed, 18 insertions(+), 14 deletions(-)

Comments

Nikolay Borisov June 18, 2024, 11:42 a.m. UTC | #1
<snip>

>   
> -static int read_sys_metadata_field16(u64 field_id,
> -				     int offset,
> -				     void *stbuf)
> +/*
> + * Read one global metadata field and store the data to a location of a
> + * given buffer specified by the offset and size (in bytes).
> + */
> +static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,

read_system_metadat_field or read_sys_metadata_field or simply
read_metadata_field

> +				  int bytes)
s/bytes/size
>   {
> -	u16 *st_member = stbuf + offset;
> +	void *st_member = stbuf + offset;

Again, this could be renamed to just 'member', what value does the 'st' 
prefix bring?

>   	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(MD_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;
>   }
> @@ -294,11 +296,13 @@ 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) }
> +#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)) }
>   
>   #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
>   	TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
> @@ -319,9 +323,8 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
>   
>   	/* 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);
> +		ret = stbuf_read_sysmd_field(fields[i].field_id, tdmr_sysinfo,
> +				fields[i].offset, fields[i].size);
>   		if (ret)
>   			return ret;
>   	}
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index b701f69485d3..812943516946 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_BYTES(_field_id)	\

Just name it MD_FIELD_SIZE, even the MD_FIELD_ID member is called 
ELEMENT_SIZE_CODE, rather than ELEMENT_BYTES_CODE or some such.

> +		(1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id))
>   
>   struct tdmr_reserved_area {
>   	u64 offset;
Kai Huang June 18, 2024, 11:28 p.m. UTC | #2
On Tue, 2024-06-18 at 14:42 +0300, Nikolay Borisov wrote:
> <snip>
> 
> >   
> > -static int read_sys_metadata_field16(u64 field_id,
> > -				     int offset,
> > -				     void *stbuf)
> > +/*
> > + * Read one global metadata field and store the data to a location of a
> > + * given buffer specified by the offset and size (in bytes).
> > + */
> > +static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
> 
> read_system_metadat_field or read_sys_metadata_field or simply
> read_metadata_field

read_sys_metadata_field() is already taken.

What's wrong of stbuf_read_sysmd_field()?  It indicates the function reads
one system metadata field to a structure member.

> 
> > +				  int bytes)
> s/bytes/size
> >   {
> > -	u16 *st_member = stbuf + offset;
> > +	void *st_member = stbuf + offset;
> 
> Again, this could be renamed to just 'member', what value does the 'st' 
> prefix bring?

Will do.

[...]


> >   
> > -#define MD_FIELD_ID_ELE_SIZE_16BIT	1
> > +#define MD_FIELD_BYTES(_field_id)	\
> 
> Just name it MD_FIELD_SIZE, even the MD_FIELD_ID member is called 
> ELEMENT_SIZE_CODE, rather than ELEMENT_BYTES_CODE or some such.

Will do.

I will also change the 'bytes' argument to 'size' in
stbuf_read_sysmd_field() (or whatever name we finally have).
Nikolay Borisov June 19, 2024, 8:05 a.m. UTC | #3
On 16.06.24 г. 15:01 ч., Kai Huang wrote:
> The TDX module provides "global metadata fields" for software to query.
> Each metadata field is accessible by a unique "metadata field ID".  TDX
> supports 8/16/32/64 bits metadata element sizes.  The size of each
> metadata field is encoded in its associated metadata field ID.
> 
> For now the kernel only reads "TD Memory Region" (TDMR) related global
> metadata fields for module initialization.  All these metadata fields
> are 16-bit, and the kernel only supports reading 16-bit fields.
> 
> Future changes will need to read more metadata fields with other element
> sizes.  To resolve this once for all, extend the existing metadata
> reading code to support reading all 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 | 29 ++++++++++++++++-------------
>   arch/x86/virt/vmx/tdx/tdx.h |  3 ++-
>   2 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 854312e97eff..4392e82a9bcb 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -270,23 +270,25 @@ 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)
> +/*
> + * Read one global metadata field and store the data to a location of a
> + * given buffer specified by the offset and size (in bytes).
> + */
> +static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
> +				  int bytes)

Actually I think opencoding read_sys_metadata_field in 
stbuf_read_sysmd_field and leaving the function named as
read_sys_metadata_field would be best. The new function is still very 
short and linear.

Another point - why do proliferate the offset calculations in the lower 
layers of TDX. Simply pass in a buffer and a size, calculate the exact 
location in the callers of the read functions.

<snip>
Kai Huang June 19, 2024, 9:51 a.m. UTC | #4
> >   
> > -static int read_sys_metadata_field16(u64 field_id,
> > -				     int offset,
> > -				     void *stbuf)
> > +/*
> > + * Read one global metadata field and store the data to a location of a
> > + * given buffer specified by the offset and size (in bytes).
> > + */
> > +static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
> > +				  int bytes)
> 
> Actually I think opencoding read_sys_metadata_field in 
> stbuf_read_sysmd_field and leaving the function named as
> read_sys_metadata_field would be best. The new function is still very 
> short and linear.

I am not sure why it is better.  IMHO having a wrapper function of a
SEAMCALL leaf, read_sys_metadata_field() for TDH.SYS.RD in this case,
isn't a bad idea.  To me it is even better as it is clearer to me.

I can see you don't like the "stbuf" prefix, so we can consider to remove
it, but for now I'd wait for some more time to see whether other people
also have comments.

> 
> Another point - why do proliferate the offset calculations in the lower 
> layers of TDX. Simply pass in a buffer and a size, calculate the exact 
> location in the callers of the read functions.

The current upstream code takes the 'offset' as argument.  Here I just
extend it to take the size.  I don't feel I have strong justification to
do additional change.  Also to me between the two it's hard to say which
is definitely better than the other.
diff mbox series

Patch

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 854312e97eff..4392e82a9bcb 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -270,23 +270,25 @@  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)
+/*
+ * Read one global metadata field and store the data to a location of a
+ * given buffer specified by the offset and size (in bytes).
+ */
+static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
+				  int bytes)
 {
-	u16 *st_member = stbuf + offset;
+	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(MD_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;
 }
@@ -294,11 +296,13 @@  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) }
+#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)) }
 
 #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
 	TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
@@ -319,9 +323,8 @@  static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 
 	/* 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);
+		ret = stbuf_read_sysmd_field(fields[i].field_id, tdmr_sysinfo,
+				fields[i].offset, fields[i].size);
 		if (ret)
 			return ret;
 	}
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index b701f69485d3..812943516946 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_BYTES(_field_id)	\
+		(1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id))
 
 struct tdmr_reserved_area {
 	u64 offset;