diff mbox series

[4/5] x86/virt/tdx: Support global metadata read for all element sizes

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

Commit Message

Huang, Kai March 1, 2024, 11:20 a.m. UTC
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(-)

Comments

Isaku Yamahata March 16, 2024, 12:49 a.m. UTC | #1
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; }) }
Huang, Kai March 20, 2024, 12:24 p.m. UTC | #2
> > @@ -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.
Edgecombe, Rick P May 3, 2024, 12:19 a.m. UTC | #3
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang 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.

It makes it sound like KVM needs 8 bit fields. I think it only needs 16 and 64.
(need to verify fully) But the code to support those can be smaller if it's
generic to all sizes.

It might be worth mentioning which fields and why to make it generic. To make
sure it doesn't come off as a premature implementation.
Huang, Kai May 3, 2024, 1:14 a.m. UTC | #4
On 3/05/2024 12:19 pm, Edgecombe, Rick P wrote:
> On Sat, 2024-03-02 at 00:20 +1300, Kai Huang 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.
> 
> It makes it sound like KVM needs 8 bit fields. I think it only needs 16 and 64.
> (need to verify fully) But the code to support those can be smaller if it's
> generic to all sizes.
> 
> It might be worth mentioning which fields and why to make it generic. To make
> sure it doesn't come off as a premature implementation.

How about:

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, and KVM will at least need to additionally be able to 
read 64-bit metadata fields.

For example, the KVM will need to read the 64-bit ATTRIBUTES_FIXED{0|1} 
fields to determine whether a particular attribute is legal when 
creating a TDX guest.  Please see 'global_metadata.json in [1] for more 
information.

To resolve this once for all, extend the existing metadata reading code 
to provide a generic metadata read infrastructure which supports reading 
all 8/16/32/64 bits element sizes.

[1] https://cdrdv2.intel.com/v1/dl/getContent/795381
Huang, Kai May 3, 2024, 12:10 p.m. UTC | #5
On Fri, 2024-05-03 at 13:14 +1200, Huang, Kai wrote:
> 
> On 3/05/2024 12:19 pm, Edgecombe, Rick P wrote:
> > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang 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.
> > 
> > It makes it sound like KVM needs 8 bit fields. I think it only needs 16 and 64.
> > (need to verify fully) But the code to support those can be smaller if it's
> > generic to all sizes.
> > 
> > It might be worth mentioning which fields and why to make it generic. To make
> > sure it doesn't come off as a premature implementation.
> 
> How about:
> 
> 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, and KVM will at least need to additionally be able to 
> read 64-bit metadata fields.
> 
> For example, the KVM will need to read the 64-bit ATTRIBUTES_FIXED{0|1} 
> fields to determine whether a particular attribute is legal when 
> creating a TDX guest.  Please see 'global_metadata.json in [1] for more 
> information.
> 
> To resolve this once for all, extend the existing metadata reading code 
> to provide a generic metadata read infrastructure which supports reading 
> all 8/16/32/64 bits element sizes.
> 
> [1] https://cdrdv2.intel.com/v1/dl/getContent/795381
> 

As replied to the patch 5, if we want to only export one API but make the
other as static inline, we need to export the one which reads a single
metadata field, and  make the one which reads multiple as static inline.

After implementing it, it turns out this way is probably slightly better:

The new function to read single field can now actually work with
'u8/u16/u32/u64' directly:

  u16 field_id1_value;
  u64 field_id2_value;
  
  read_sys_medata_single(field_id1, &field_id1_value);
  read_sys_metada_single(field_id2, &field_id2_value);

Please help to review below updated patch?

With it, the patch 5 will only need to export one and the other can be
static inline.

------------

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, and KVM will at least need to additionally read 64-bit
metadata fields.

For example, the KVM will need to read the 64-bit ATTRIBUTES_FIXED{0|1}
fields to determine whether a particular attribute is legal when
creating a TDX guest.  Please see 'global_metadata.json in [1] for more
information.

To resolve this once for all, extend the existing metadata reading code
to provide a generic metadata read infrastructure which supports reading
all 8/16/32/64 bits element sizes.

[1] https://cdrdv2.intel.com/v1/dl/getContent/795381

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 | 51 ++++++++++++++++++++++---------------
 arch/x86/virt/vmx/tdx/tdx.h |  3 ++-
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index eb208da4ff63..e8b8f6ca7026 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -271,23 +271,22 @@ 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 a single global metadata field by the given field ID, and copy
+ * the data into the buf provided by the caller.  The size of the copied
+ * data is decoded from the metadata field ID.  The caller must provide
+ * enough space for the buf according to the metadata field ID.
+ */
+static int read_sys_metadata_single(u64 field_id, void *buf)
 {
-       u16 *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))
-               return -EINVAL;
-
        ret = read_sys_metadata_field(field_id, &tmp);
        if (ret)
                return ret;

-       *st_member = tmp;
+       memcpy(buf, &tmp, MD_FIELD_BYTES(field_id));

        return 0;
 }
@@ -301,6 +300,27 @@ struct field_mapping {
        { .field_id = MD_FIELD_ID_##_field_id,          \
          .offset   = offsetof(_struct, _member) }

+/*
+ * Read multiple global metadata fields into a structure according to a
+ * mapping table of metadata field IDs to the structure members.  When
+ * building the table, the caller must make sure the size of each
+ * structure member matches the size of corresponding metadata field.
+ */
+static int read_sys_metadata_multi(const struct field_mapping *fields,
+                                  int nr_fields, void *stbuf)
+{
+       int i, ret;
+
+       for (i = 0; i < nr_fields; i++) {
+               ret = read_sys_metadata_single(fields[i].field_id,
+                                     fields[i].offset + 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 +334,10 @@ 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]),
        };
Huang, Kai May 3, 2024, 12:26 p.m. UTC | #6
On Fri, 2024-05-03 at 12:10 +0000, Huang, Kai wrote:
> On Fri, 2024-05-03 at 13:14 +1200, Huang, Kai wrote:
> > 
> > On 3/05/2024 12:19 pm, Edgecombe, Rick P wrote:
> > > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang 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.
> > > 
> > > It makes it sound like KVM needs 8 bit fields. I think it only needs 16 and 64.
> > > (need to verify fully) But the code to support those can be smaller if it's
> > > generic to all sizes.
> > > 
> > > It might be worth mentioning which fields and why to make it generic. To make
> > > sure it doesn't come off as a premature implementation.
> > 
> > How about:
> > 
> > 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, and KVM will at least need to additionally be able to 
> > read 64-bit metadata fields.
> > 
> > For example, the KVM will need to read the 64-bit ATTRIBUTES_FIXED{0|1} 
> > fields to determine whether a particular attribute is legal when 
> > creating a TDX guest.  Please see 'global_metadata.json in [1] for more 
> > information.
> > 
> > To resolve this once for all, extend the existing metadata reading code 
> > to provide a generic metadata read infrastructure which supports reading 
> > all 8/16/32/64 bits element sizes.
> > 
> > [1] https://cdrdv2.intel.com/v1/dl/getContent/795381
> > 
> 
> As replied to the patch 5, if we want to only export one API but make the
> other as static inline, we need to export the one which reads a single
> metadata field, and  make the one which reads multiple as static inline.
> 
> After implementing it, it turns out this way is probably slightly better:
> 
> The new function to read single field can now actually work with
> 'u8/u16/u32/u64' directly:
> 
>   u16 field_id1_value;
>   u64 field_id2_value;
>   
>   read_sys_medata_single(field_id1, &field_id1_value);
>   read_sys_metada_single(field_id2, &field_id2_value);
> 
> Please help to review below updated patch?
> 
> With it, the patch 5 will only need to export one and the other can be
> static inline.
> 

Hmm.. Sorry the previous reply didn't include the full patch due to some
copy/paste error.  Below is the full patch:

------

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, and KVM will at least need to additionally read 64-bit
metadata fields.

For example, the KVM will need to read the 64-bit ATTRIBUTES_FIXED{0|1}
fields to determine whether a particular attribute is legal when
creating a TDX guest.  Please see 'global_metadata.json in [1] for more
information.

To resolve this once for all, extend the existing metadata reading code
to provide a generic metadata read infrastructure which supports reading
all 8/16/32/64 bits element sizes.

[1] https://cdrdv2.intel.com/v1/dl/getContent/795381

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 | 51 ++++++++++++++++++++++---------------
 arch/x86/virt/vmx/tdx/tdx.h |  3 ++-
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index eb208da4ff63..e8b8f6ca7026 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -271,23 +271,22 @@ 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 a single global metadata field by the given field ID, and copy
+ * the data into the buf provided by the caller.  The size of the copied
+ * data is decoded from the metadata field ID.  The caller must provide
+ * enough space for the buf according to the metadata field ID.
+ */
+static int read_sys_metadata_single(u64 field_id, void *buf)
 {
-       u16 *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))
-               return -EINVAL;
-
        ret = read_sys_metadata_field(field_id, &tmp);
        if (ret)
                return ret;

-       *st_member = tmp;
+       memcpy(buf, &tmp, MD_FIELD_BYTES(field_id));

        return 0;
 }
@@ -301,6 +300,27 @@ struct field_mapping {
        { .field_id = MD_FIELD_ID_##_field_id,          \
          .offset   = offsetof(_struct, _member) }

+/*
+ * Read multiple global metadata fields into a structure according to a
+ * mapping table of metadata field IDs to the structure members.  When
+ * building the table, the caller must make sure the size of each
+ * structure member matches the size of corresponding metadata field.
+ */
+static int read_sys_metadata_multi(const struct field_mapping *fields,
+                                  int nr_fields, void *stbuf)
+{
+       int i, ret;
+
+       for (i = 0; i < nr_fields; i++) {
+               ret = read_sys_metadata_single(fields[i].field_id,
+                                     fields[i].offset + 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 +334,10 @@ 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_multi(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..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;
Edgecombe, Rick P May 3, 2024, 6:32 p.m. UTC | #7
On Fri, 2024-05-03 at 12:26 +0000, Huang, Kai wrote:
> 
> To resolve this once for all, extend the existing metadata reading code
> to provide a generic metadata read infrastructure which supports reading
> all 8/16/32/64 bits element sizes.

I think the key point is that it is actually simpler to support all field types
then only 16 and 64.
Huang, Kai May 6, 2024, 10:59 a.m. UTC | #8
On Fri, 2024-05-03 at 18:32 +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-05-03 at 12:26 +0000, Huang, Kai wrote:
> > 
> > To resolve this once for all, extend the existing metadata reading code
> > to provide a generic metadata read infrastructure which supports reading
> > all 8/16/32/64 bits element sizes.
> 
> I think the key point is that it is actually simpler to support all field types
> then only 16 and 64.

How about add below to the end of the changelog:

The metadata field ID encodes the element size.  The TDH.SYS.RD SEAMCALL
always returns 64-bit metadata data regardless of the true element size of
the metadata field.  The current function to read 16-bit metadata fields
checks the element size encoded in the field ID is indeed 16-bits.

Change that function to just covert the data from the SEAMCALL to the size
encoded in the metadata field ID to provide a function which supports all
element sizes.  It's actually simpler to support reading all element sizes
in one function rather than providing two functions to read 16 and 64 bits
elements respectively.
diff mbox series

Patch

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;