KVM will need to read bunch of TDX module metadata fields to create and
run TDX guests. In the long term, other in-kernel TDX users, e.g., VT-d
also likely will need to read metadata. This series provides common APIs
in the TDX host code so that KVM can use.
This series has been sent out together with the v19 KVM TDX patchset, and
actually received one minor comment from Juergen [1](thanks!), which I
fixed in this series.
Now I am sending out this series separately to reduce the size of the KVM
TDX patchset. Paolo and Sean are doing similar things [2][3].
[1]: https://lore.kernel.org/kvm/[email protected]/T/#mbe96ac2b6560897083afdbe1db446a75a724e4e9
[2]: https://lore.kernel.org/kvm/[email protected]/T/
[3]: https://lore.kernel.org/kvm/[email protected]/T/
Kai Huang (5):
x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro
x86/virt/tdx: Move TDMR metadata fields map table to local variable
x86/virt/tdx: Unbind global metadata read with 'struct
tdx_tdmr_sysinfo'
x86/virt/tdx: Support global metadata read for all element sizes
x86/virt/tdx: Export global metadata read infrastructure
arch/x86/include/asm/tdx.h | 22 ++++++++++
arch/x86/virt/vmx/tdx/tdx.c | 84 +++++++++++++++++++++----------------
arch/x86/virt/vmx/tdx/tdx.h | 2 -
3 files changed, 71 insertions(+), 37 deletions(-)
base-commit: 5bdd181821b2c65b074cfad07d7c7d5d3cfe20bf
--
2.43.2
TD_SYSINFO_MAP() macro actually takes the member of the 'struct
tdx_tdmr_sysinfo' as the second argument and uses the offsetof() to
calculate the offset for that member.
Rename the macro argument _offset to _member to reflect this.
Signed-off-by: Kai Huang <[email protected]>
Reviewed-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/virt/vmx/tdx/tdx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4d6826a76f78..2aee64d2f27f 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -297,9 +297,9 @@ struct field_mapping {
int offset;
};
-#define TD_SYSINFO_MAP(_field_id, _offset) \
+#define TD_SYSINFO_MAP(_field_id, _member) \
{ .field_id = MD_FIELD_ID_##_field_id, \
- .offset = offsetof(struct tdx_tdmr_sysinfo, _offset) }
+ .offset = offsetof(struct tdx_tdmr_sysinfo, _member) }
/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
static const struct field_mapping fields[] = {
--
2.43.2
The kernel reads all TDMR related global metadata fields based on a
table which maps the metadata fields to the corresponding members of
'struct tdx_tdmr_sysinfo'.
Currently this table is a static variable. But this table is only used
by the function which reads these metadata fields and becomes useless
after reading is done.
Change the table to function local variable. This also saves the
storage of the table from the kernel image.
Signed-off-by: Kai Huang <[email protected]>
Reviewed-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/virt/vmx/tdx/tdx.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 2aee64d2f27f..cdcb3332bc5d 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -301,17 +301,16 @@ struct field_mapping {
{ .field_id = MD_FIELD_ID_##_field_id, \
.offset = offsetof(struct tdx_tdmr_sysinfo, _member) }
-/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
-static const struct field_mapping fields[] = {
- TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs),
- TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
- TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]),
- TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]),
- TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]),
-};
-
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[] = {
+ TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs),
+ TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
+ TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]),
+ TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]),
+ TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]),
+ };
int ret;
int i;
--
2.43.2
For now the kernel only reads TDMR related global metadata fields for
module initialization, and the metadata read code only works with the
'struct tdx_tdmr_sysinfo'.
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 is not bound to any specific structure.
To start providing such infrastructure, unbound the metadata read with
the 'struct tdx_tdmr_sysinfo'.
Signed-off-by: Kai Huang <[email protected]>
Reviewed-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index cdcb3332bc5d..eb208da4ff63 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -273,9 +273,9 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
static int read_sys_metadata_field16(u64 field_id,
int offset,
- struct tdx_tdmr_sysinfo *ts)
+ void *stbuf)
{
- u16 *ts_member = ((void *)ts) + offset;
+ u16 *st_member = stbuf + offset;
u64 tmp;
int ret;
@@ -287,7 +287,7 @@ static int read_sys_metadata_field16(u64 field_id,
if (ret)
return ret;
- *ts_member = tmp;
+ *st_member = tmp;
return 0;
}
@@ -297,19 +297,22 @@ struct field_mapping {
int offset;
};
-#define TD_SYSINFO_MAP(_field_id, _member) \
- { .field_id = MD_FIELD_ID_##_field_id, \
- .offset = offsetof(struct tdx_tdmr_sysinfo, _member) }
+#define TD_SYSINFO_MAP(_field_id, _struct, _member) \
+ { .field_id = MD_FIELD_ID_##_field_id, \
+ .offset = offsetof(_struct, _member) }
+
+#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \
+ TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
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[] = {
- TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs),
- TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
- TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]),
- TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]),
- TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]),
+ 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]),
+ 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;
--
2.43.2
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 <[email protected]>
Reviewed-by: Kirill A. Shutemov <[email protected]>
---
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)) }
+
+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;
--
2.43.2
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 <[email protected]>
Reviewed-by: Kirill A. Shutemov <[email protected]>
---
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 */
--
2.43.2
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 <[email protected]>
> Reviewed-by: Kirill A. Shutemov <[email protected]>
> ---
> 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/[email protected]/
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/[email protected]/
>
> 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.
On Sat, Mar 02, 2024 at 12:20:36AM +1300,
Kai Huang <[email protected]> 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 <[email protected]>
> Reviewed-by: Kirill A. Shutemov <[email protected]>
> ---
> 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; }) }
--
Isaku Yamahata <[email protected]>
> > @@ -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.
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> TD_SYSINFO_MAP() macro actually takes the member of the 'struct
> tdx_tdmr_sysinfo' as the second argument and uses the offsetof() to
> calculate the offset for that member.
>
> Rename the macro argument _offset to _member to reflect this.
The KVM patches will want to use this macro. The fact that it is misnamed will
percolate into the KVM code if it is not updated before it gets wider callers.
(This is a reason why this is good change from KVM's perspective).
See the KVM code below:
#define TDX_INFO_MAP(_field_id, _member) \
TD_SYSINFO_MAP(_field_id, struct st, _member)
struct tdx_metadata_field_mapping st_fields[] = {
TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
TDX_INFO_MAP(TDCS_BASE_SIZE, tdcs_base_size),
TDX_INFO_MAP(TDVPS_BASE_SIZE, tdvps_base_size),
};
#undef TDX_INFO_MAP
#define TDX_INFO_MAP(_field_id, _member) \
TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
struct tdx_metadata_field_mapping fields[] = {
TDX_INFO_MAP(FEATURES0, features0),
TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
};
#undef TDX_INFO_MAP
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> The kernel reads all TDMR related global metadata fields based on a
> table which maps the metadata fields to the corresponding members of
> 'struct tdx_tdmr_sysinfo'.
>
> Currently this table is a static variable. But this table is only used
> by the function which reads these metadata fields and becomes useless
> after reading is done.
>
> Change the table to function local variable. This also saves the
> storage of the table from the kernel image.
It seems like a reasonable change, but I don't see how it helps the purpose of
this series. It seems more like generic cleanup. Can you explain?
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \
> + TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
>
> 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[] = {
> - TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs),
> - TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
> - TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,
> pamt_entry_size[TDX_PS_4K]),
> - TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,
> pamt_entry_size[TDX_PS_2M]),
> - TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,
> pamt_entry_size[TDX_PS_1G]),
> + 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]),
> + 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]),
The creation of TD_SYSINFO_MAP_TDMR_INFO part is not strictly needed, but makes
sense in the context of the signature change in read_sys_metadata_field16(). It
might be worth justifying it in the log.
On 3/05/2024 12:09 pm, Edgecombe, Rick P wrote:
> On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
>> The kernel reads all TDMR related global metadata fields based on a
>> table which maps the metadata fields to the corresponding members of
>> 'struct tdx_tdmr_sysinfo'.
>>
>> Currently this table is a static variable. But this table is only used
>> by the function which reads these metadata fields and becomes useless
>> after reading is done.
>>
>> Change the table to function local variable. This also saves the
>> storage of the table from the kernel image.
>
> It seems like a reasonable change, but I don't see how it helps the purpose of
> this series. It seems more like generic cleanup. Can you explain?
It doesn't help KVM from exporting API's perspective.
I just uses this series for some small improvement (that I believe) of
the current code too.
I can certainly drop this if you don't want it, but it's just a small
change and I don't see the benefit of sending it out separately.
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.
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?
On Fri, 2024-05-03 at 12:18 +1200, Huang, Kai wrote:
>
>
> On 3/05/2024 12:09 pm, Edgecombe, Rick P wrote:
> > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> > > The kernel reads all TDMR related global metadata fields based on a
> > > table which maps the metadata fields to the corresponding members of
> > > 'struct tdx_tdmr_sysinfo'.
> > >
> > > Currently this table is a static variable. But this table is only used
> > > by the function which reads these metadata fields and becomes useless
> > > after reading is done.
> > >
> > > Change the table to function local variable. This also saves the
> > > storage of the table from the kernel image.
> >
> > It seems like a reasonable change, but I don't see how it helps the purpose
> > of
> > this series. It seems more like generic cleanup. Can you explain?
>
> It doesn't help KVM from exporting API's perspective.
>
> I just uses this series for some small improvement (that I believe) of
> the current code too.
>
> I can certainly drop this if you don't want it, but it's just a small
> change and I don't see the benefit of sending it out separately.
The change makes sense to me by itself, but it needs to be called out as
unrelated cleanup. Otherwise it will be confusing to anyone reviewing this from
the perspective of something KVM TDX needs.
Don't have a super strong opinion. But given the choice, I would prefer it gets
separated, because to me it's a lower priority then the rest (which is high).
On 3/05/2024 12:12 pm, Edgecombe, Rick P wrote:
> On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
>> +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \
>> + TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
>>
>> 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[] = {
>> - TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs),
>> - TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
>> - TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,
>> pamt_entry_size[TDX_PS_4K]),
>> - TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,
>> pamt_entry_size[TDX_PS_2M]),
>> - TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,
>> pamt_entry_size[TDX_PS_1G]),
>> + 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]),
>> + 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]),
>
> The creation of TD_SYSINFO_MAP_TDMR_INFO part is not strictly needed, but makes
> sense in the context of the signature change in read_sys_metadata_field16(). It
> might be worth justifying it in the log.
I see your point. How about adding below paragraph to the end of this
changelog?
"
The metadata reading code uses the TD_SYSINFO_MAP() macro to describe
the mapping between the metadata fields and the members of the 'struct
tdx_tdmr_sysinfo'. I.e., it hard-codes the 'struct tdx_tdmr_sysinfo'
inside the macro.
As part of unbinding metadata read with 'struct tdx_tdmr_sysinfo', the
TD_SYSINFO_MAP() macro needs to be changed to additionally take the
structure as argument so it can accept any structure. That would make
the current code to read TDMR related metadata fields longer if using
TD_SYSINFO_MAP() directly.
Define a wrapper macro for reading TDMR related metadata fields to make
the code shorter.
"
By typing, it reminds me that I kinda need to learn how to separate the
"high level design" vs "low level implementation details". I think the
latter can be seen easily in the code, and probably can be avoided in
the changelog.
I am not sure whether adding the TD_SYSINFO_MAP_TDMR_INFO() macro belong
to which category, especially when I needed a lot text to justify this
change (thus I wonder whether it is worth to do).
Or any shorter version that you can suggest?
Thanks.
On 3/05/2024 12:29 pm, Edgecombe, Rick P wrote:
> On Fri, 2024-05-03 at 12:18 +1200, Huang, Kai wrote:
>>
>>
>> On 3/05/2024 12:09 pm, Edgecombe, Rick P wrote:
>>> On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
>>>> The kernel reads all TDMR related global metadata fields based on a
>>>> table which maps the metadata fields to the corresponding members of
>>>> 'struct tdx_tdmr_sysinfo'.
>>>>
>>>> Currently this table is a static variable. But this table is only used
>>>> by the function which reads these metadata fields and becomes useless
>>>> after reading is done.
>>>>
>>>> Change the table to function local variable. This also saves the
>>>> storage of the table from the kernel image.
>>>
>>> It seems like a reasonable change, but I don't see how it helps the purpose
>>> of
>>> this series. It seems more like generic cleanup. Can you explain?
>>
>> It doesn't help KVM from exporting API's perspective.
>>
>> I just uses this series for some small improvement (that I believe) of
>> the current code too.
>>
>> I can certainly drop this if you don't want it, but it's just a small
>> change and I don't see the benefit of sending it out separately.
>
> The change makes sense to me by itself, but it needs to be called out as
> unrelated cleanup. Otherwise it will be confusing to anyone reviewing this from
> the perspective of something KVM TDX needs.
>
> Don't have a super strong opinion. But given the choice, I would prefer it gets
> separated, because to me it's a lower priority then the rest (which is high).
OK I'll drop this one.
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
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.
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 <[email protected]>
Reviewed-by: Kirill A. Shutemov <[email protected]>
---
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]),
};
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 <[email protected]>
Reviewed-by: Kirill A. Shutemov <[email protected]>
---
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;
--
2.43.2
On 3/1/24 03:20, Kai Huang wrote:
> The kernel reads all TDMR related global metadata fields based on a
> table which maps the metadata fields to the corresponding members of
> 'struct tdx_tdmr_sysinfo'.
>
> Currently this table is a static variable. But this table is only used
> by the function which reads these metadata fields and becomes useless
> after reading is done.
Is this intended to be a problem statement? _How_ is this a problem?
> Change the table to function local variable. This also saves the
> storage of the table from the kernel image.
I'm confused how this would happen. Could you please explain your logic
a bit here?
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.
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.
On Fri, 2024-05-03 at 12:52 +1200, Huang, Kai wrote:
> "
> The metadata reading code uses the TD_SYSINFO_MAP() macro to describe
> the mapping between the metadata fields and the members of the 'struct
> tdx_tdmr_sysinfo'. I.e., it hard-codes the 'struct tdx_tdmr_sysinfo'
> inside the macro.
How about:
The TDX module initialization code currently uses the metadata reading
infrastructure to read several TDX module fields, and populate them all into the
same kernel defined struct, "struct tdx_tdmr_sysinfo". So the helper macros for
marshaling the data from the TDX module into the struct fields hardcode that
struct name.
>
> As part of unbinding metadata read with 'struct tdx_tdmr_sysinfo', the
> TD_SYSINFO_MAP() macro needs to be changed to additionally take the
> structure as argument so it can accept any structure. That would make
> the current code to read TDMR related metadata fields longer if using
> TD_SYSINFO_MAP() directly.
Future changes will allow for other types of metadata to be read, that don't
make sense to populate to that specific struct. To accommodate this the data
marshaling macro, TD_SYSINFO_MAP, will be extended to take different structs.
Unfortunately, it will result in the usage of TD_SYSINFO_MAP for populating
struct tdx_tdmr_sysinfo to change to... [some undesirable situation].
Question for you:
Is this just to make it shorter, or to avoid duplication of specifying the
struct name? Like is it a mitigation for exceeding 80 chars or 100?
>
> Define a wrapper macro for reading TDMR related metadata fields to make
> the code shorter.
> "
>
> By typing, it reminds me that I kinda need to learn how to separate the
> "high level design" vs "low level implementation details". I think the
> latter can be seen easily in the code, and probably can be avoided in
> the changelog.
Especially for TDX with all it's complexity and acronyms I think it helps to
explain in simple terms. Like imagine if someone was working at their computer
and you tapped on their shoulder, how would you introduce this change? If you
start with "TDMR related global metadata fields" and "struct tdx_tdmr_sysinfo"
they are going to have to struggle to context switch into it.
For each patch, if the connection is not clear, ease them into it. Of course
everyone has the different preferences, so YMMV. But especially the tip folks
seem to appreciate it.
>
> I am not sure whether adding the TD_SYSINFO_MAP_TDMR_INFO() macro belong
> to which category, especially when I needed a lot text to justify this
> change (thus I wonder whether it is worth to do).
>
> Or any shorter version that you can suggest?
>
I don't think it is too long.
On Fri, 2024-05-03 at 09:01 -0700, Dave Hansen wrote:
> On 3/1/24 03:20, Kai Huang wrote:
> > The kernel reads all TDMR related global metadata fields based on a
> > table which maps the metadata fields to the corresponding members of
> > 'struct tdx_tdmr_sysinfo'.
> >
> > Currently this table is a static variable. But this table is only used
> > by the function which reads these metadata fields and becomes useless
> > after reading is done.
>
> Is this intended to be a problem statement? _How_ is this a problem?
>
> > Change the table to function local variable. This also saves the
> > storage of the table from the kernel image.
>
> I'm confused how this would happen. Could you please explain your logic
> a bit here?
I think I failed to notice one thing, that although this patch can
eliminate the (static) @fields[] array in the data section, it generates
more code in the function get_tdx_tdmr_sysinfo() in order to build the
same array in the stack.
I did experiment and compared the generated code with or without the code
change in this patch:
before:
fields:
.quad -7998392933915033592 /* metadata field ID */
.long 0
.zero 4
.quad -7998392933915033591
.long 2
.zero 4
.quad -7998392933915033584
.long 4
.zero 4
.quad -7998392933915033583
.long 6
.zero 4
.quad -7998392933915033582
.long 8
.zero 4
get_tdx_tdmr_sysinfo:
pushq %rbp
movq %rsp, %rbp
subq $24, %rsp
movq %rdi, -24(%rbp)
movl $0, -4(%rbp)
jmp .L8
......
after:
get_tdx_tdmr_sysinfo:
pushq %rbp
movq %rsp, %rbp
subq $112, %rsp
movq %rdi, -104(%rbp)
movabsq $-7998392933915033592, %rax
movq %rax, -96(%rbp)
movl $0, -88(%rbp)
movabsq $-7998392933915033591, %rax
movq %rax, -80(%rbp)
movl $2, -72(%rbp)
movabsq $-7998392933915033584, %rax
movq %rax, -64(%rbp)
movl $4, -56(%rbp)
movabsq $-7998392933915033583, %rax
movq %rax, -48(%rbp)
movl $6, -40(%rbp)
movabsq $-7998392933915033582, %rax
movq %rax, -32(%rbp)
movl $8, -24(%rbp)
movl $0, -4(%rbp)
jmp .L8
......
So looks we cannot assume moving the static array to function local
variable can always save the storage.
I think the point is the compiler has to keep those constants (metadata
field ID and offset) somewhere in the object file, no matter they are in
the data section or in the code in text section, and no matter how does
the compiler generate the code.
The more reasonable benefit of this patch is to make the name scope of the
@fields[] array be only visible in the get_tdx_tdmr_sysinfo() but not the
entire file.
Thanks for the insight. I hope the above is all I missed, or am I still
missing anything?
Anyway as replied to Rick I'll drop this patch from this series.
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.
On Fri, 2024-05-03 at 19:03 +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-05-03 at 12:52 +1200, Huang, Kai wrote:
> > "
> > The metadata reading code uses the TD_SYSINFO_MAP() macro to describe
> > the mapping between the metadata fields and the members of the 'struct
> > tdx_tdmr_sysinfo'. I.e., it hard-codes the 'struct tdx_tdmr_sysinfo'
> > inside the macro.
>
> How about:
>
> The TDX module initialization code currently uses the metadata reading
> infrastructure to read several TDX module fields, and populate them all into the
> same kernel defined struct, "struct tdx_tdmr_sysinfo". So the helper macros for
> marshaling the data from the TDX module into the struct fields hardcode that
> struct name.
>
> >
> > As part of unbinding metadata read with 'struct tdx_tdmr_sysinfo', the
> > TD_SYSINFO_MAP() macro needs to be changed to additionally take the
> > structure as argument so it can accept any structure. That would make
> > the current code to read TDMR related metadata fields longer if using
> > TD_SYSINFO_MAP() directly.
>
> Future changes will allow for other types of metadata to be read, that don't
> make sense to populate to that specific struct. To accommodate this the data
> marshaling macro, TD_SYSINFO_MAP, will be extended to take different structs.
> Unfortunately, it will result in the usage of TD_SYSINFO_MAP for populating
> struct tdx_tdmr_sysinfo to change to... [some undesirable situation].
I'll change to use your words, with some small tweaks to also mention the
function to read metadata field should also be relaxed to take a typeless
'void *' buffer.
Please see below.
>
> Question for you:
> Is this just to make it shorter, or to avoid duplication of specifying the
> struct name?
>
The intention was to make it shorter, but I think both.
> Like is it a mitigation for exceeding 80 chars or 100?
Yes for not exceeding 100.
With this patch, the code actually exceeds 80 chars, but I found breaking
them to separate lines hurt the readability.
>
> >
> > Define a wrapper macro for reading TDMR related metadata fields to make
> > the code shorter.
> > "
> >
> > By typing, it reminds me that I kinda need to learn how to separate the
> > "high level design" vs "low level implementation details". I think the
> > latter can be seen easily in the code, and probably can be avoided in
> > the changelog.
>
> Especially for TDX with all it's complexity and acronyms I think it helps to
> explain in simple terms. Like imagine if someone was working at their computer
> and you tapped on their shoulder, how would you introduce this change? If you
> start with "TDMR related global metadata fields" and "struct tdx_tdmr_sysinfo"
> they are going to have to struggle to context switch into it.
>
> For each patch, if the connection is not clear, ease them into it. Of course
> everyone has the different preferences, so YMMV. But especially the tip folks
> seem to appreciate it.
>
> >
> > I am not sure whether adding the TD_SYSINFO_MAP_TDMR_INFO() macro belong
> > to which category, especially when I needed a lot text to justify this
> > change (thus I wonder whether it is worth to do).
> >
> > Or any shorter version that you can suggest?
> >
>
> I don't think it is too long.
The new changelog based on your words:
The TDX module initialization code currently uses the metadata reading
infrastructure to read several TDX module fields, and populate them all
into the same kernel defined struct, "struct tdx_tdmr_sysinfo". So the
function to read the metadata fields and the helper macros for marshaling
the data from the TDX module into the struct fields hardcode that struct
name.
Future changes will allow for other types of metadata to be read, that
don't make sense to populate to that specific struct. To accommodate
this, change the metadata reading function to take a typeless 'void *'
buffer, and extend the data marshaling macro, TD_SYSINFO_MAP, to take
different structs.
Unfortunately, this will result in the usage of TD_SYSINFO_MAP for
populating 'struct tdx_tdmr_sysinfo' to be changed to use the struct name
explicitly for each struct member and make the code longer. Define a
wrapper macro for reading TDMR related metadata fields to make the code
shorter, i.e., not exceeding the 100 characters limit while still keeping
the use of TDX_SYSINFO_MAP for each struct member in one line for better
readability.
On Fri, 2024-05-03 at 00:07 +0000, Edgecombe, Rick P wrote:
> On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> > TD_SYSINFO_MAP() macro actually takes the member of the 'struct
> > tdx_tdmr_sysinfo' as the second argument and uses the offsetof() to
> > calculate the offset for that member.
> >
> > Rename the macro argument _offset to _member to reflect this.
>
> The KVM patches will want to use this macro. The fact that it is misnamed will
> percolate into the KVM code if it is not updated before it gets wider callers.
> (This is a reason why this is good change from KVM's perspective).
>
> See the KVM code below:
>
> #define TDX_INFO_MAP(_field_id, _member) \
> TD_SYSINFO_MAP(_field_id, struct st, _member)
>
> struct tdx_metadata_field_mapping st_fields[] = {
> TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
> TDX_INFO_MAP(TDCS_BASE_SIZE, tdcs_base_size),
> TDX_INFO_MAP(TDVPS_BASE_SIZE, tdvps_base_size),
> };
> #undef TDX_INFO_MAP
>
> #define TDX_INFO_MAP(_field_id, _member) \
> TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
>
> struct tdx_metadata_field_mapping fields[] = {
> TDX_INFO_MAP(FEATURES0, features0),
> TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
> TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
> TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
> TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
> };
> #undef TDX_INFO_MAP
I was thinking how to respond. I guess your point is we can also mention
KVM will need to use this too so it's better to change it before it gets
wider callers. But I don't think it is needed because if it is misnamed
now then we already have a justification to do it.
And technically, I don't think the argument name used in KVM actually has
anything to do with the argument name used in the TD_SYSINFO_MAP() macro
definition here. What really matters is when they get used, we need to
pass the "real struct member":
struct whatever {
u64 a;
u16 b;
};
#define TDX_INFO_MAP_WHATEVER(_field_id, _xyz) \
TD_SYSINFO_MAP(_field_id, struct whatever, _xyz)
const struct tdx_metadata_field_mapping fields[] = {
TDX_INFO_MAP_WHATEVER(_FIELD_A, a),
TDX_INFO_MAP_WHATEVER(_FIELD_B, b),
};
No?
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.
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.
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!