On Fri, 2024-05-03 at 13:14 +1200, Huang, Kai wrote:
>
> On 3/05/2024 12:19 pm, Edgecombe, Rick P wrote:
> > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> > > For now the kernel only reads TDMR related global metadata fields for
> > > module initialization. All these fields are 16-bits, and the kernel
> > > only supports reading 16-bits fields.
> > >
> > > KVM will need to read a bunch of non-TDMR related metadata to create and
> > > run TDX guests. It's essential to provide a generic metadata read
> > > infrastructure which supports reading all 8/16/32/64 bits element sizes.
> > >
> > > Extend the metadata read to support reading all these element sizes.
> >
> > It makes it sound like KVM needs 8 bit fields. I think it only needs 16 and 64.
> > (need to verify fully) But the code to support those can be smaller if it's
> > generic to all sizes.
> >
> > It might be worth mentioning which fields and why to make it generic. To make
> > sure it doesn't come off as a premature implementation.
>
> How about:
>
> For now the kernel only reads TDMR related global metadata fields for
> module initialization. All these fields are 16-bits, and the kernel
> only supports reading 16-bits fields.
>
> KVM will need to read a bunch of non-TDMR related metadata to create and
> run TDX guests, and KVM will at least need to additionally be able to
> read 64-bit metadata fields.
>
> For example, the KVM will need to read the 64-bit ATTRIBUTES_FIXED{0|1}
> fields to determine whether a particular attribute is legal when
> creating a TDX guest. Please see 'global_metadata.json in [1] for more
> information.
>
> To resolve this once for all, extend the existing metadata reading code
> to provide a generic metadata read infrastructure which supports reading
> all 8/16/32/64 bits element sizes.
>
> [1] https://cdrdv2.intel.com/v1/dl/getContent/795381
>
As replied to the patch 5, if we want to only export one API but make the
other as static inline, we need to export the one which reads a single
metadata field, and make the one which reads multiple as static inline.
After implementing it, it turns out this way is probably slightly better:
The new function to read single field can now actually work with
'u8/u16/u32/u64' directly:
u16 field_id1_value;
u64 field_id2_value;
read_sys_medata_single(field_id1, &field_id1_value);
read_sys_metada_single(field_id2, &field_id2_value);
Please help to review below updated patch?
With it, the patch 5 will only need to export one and the other can be
static inline.
------------
For now the kernel only reads TDMR related global metadata fields for
module initialization. All these fields are 16-bits, and the kernel
only supports reading 16-bits fields.
KVM will need to read a bunch of non-TDMR related metadata to create and
run TDX guests, and KVM will at least need to additionally read 64-bit
metadata fields.
For example, the KVM will need to read the 64-bit ATTRIBUTES_FIXED{0|1}
fields to determine whether a particular attribute is legal when
creating a TDX guest. Please see 'global_metadata.json in [1] for more
information.
To resolve this once for all, extend the existing metadata reading code
to provide a generic metadata read infrastructure which supports reading
all 8/16/32/64 bits element sizes.
[1] https://cdrdv2.intel.com/v1/dl/getContent/795381
Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
---
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]),
};
