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