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.