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

From: "Huang, Kai" <kai.huang@intel.com>
To: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "Hansen, Dave" <dave.hansen@intel.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"bp@alien8.de" <bp@alien8.de>, "x86@kernel.org" <x86@kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"jgross@suse.com" <jgross@suse.com>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>
Subject: Re: [PATCH 4/5] x86/virt/tdx: Support global metadata read for all element sizes
Date: Fri, 3 May 2024 12:26:26 +0000	[thread overview]
Message-ID: <fb829c94a45f246eac0dd869478e0dcfc965232b.camel@intel.com> (raw)
In-Reply-To: <16a8d8dc15b9afd6948a4f3913be568caeff074b.camel@intel.com>

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

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

------

For now the kernel only reads TDMR related global metadata fields for
module initialization.  All these fields are 16-bits, and the kernel
only supports reading 16-bits fields.

KVM will need to read a bunch of non-TDMR related metadata to create and
run TDX guests, and KVM will at least need to additionally read 64-bit
metadata fields.

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

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

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

Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/virt/vmx/tdx/tdx.c | 51 ++++++++++++++++++++++---------------
 arch/x86/virt/vmx/tdx/tdx.h |  3 ++-
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index eb208da4ff63..e8b8f6ca7026 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -271,23 +271,22 @@ static int read_sys_metadata_field(u64 field_id, u64
*data)
        return 0;
 }

-static int read_sys_metadata_field16(u64 field_id,
-                                    int offset,
-                                    void *stbuf)
+/*
+ * Read a single global metadata field by the given field ID, and copy
+ * the data into the buf provided by the caller.  The size of the copied
+ * data is decoded from the metadata field ID.  The caller must provide
+ * enough space for the buf according to the metadata field ID.
+ */
+static int read_sys_metadata_single(u64 field_id, void *buf)
 {
-       u16 *st_member = stbuf + offset;
        u64 tmp;
        int ret;

-       if (WARN_ON_ONCE(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
-                       MD_FIELD_ID_ELE_SIZE_16BIT))
-               return -EINVAL;
-
        ret = read_sys_metadata_field(field_id, &tmp);
        if (ret)
                return ret;

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

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

+/*
+ * Read multiple global metadata fields into a structure according to a
+ * mapping table of metadata field IDs to the structure members.  When
+ * building the table, the caller must make sure the size of each
+ * structure member matches the size of corresponding metadata field.
+ */
+static int read_sys_metadata_multi(const struct field_mapping *fields,
+                                  int nr_fields, void *stbuf)
+{
+       int i, ret;
+
+       for (i = 0; i < nr_fields; i++) {
+               ret = read_sys_metadata_single(fields[i].field_id,
+                                     fields[i].offset + stbuf);
+               if (ret)
+                       return ret;
+       }
+
+       return 0;
+}
+
 #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)   \
        TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)

@@ -314,19 +334,10 @@ static int get_tdx_tdmr_sysinfo(struct
tdx_tdmr_sysinfo *tdmr_sysinfo)
                TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE,   
pamt_entry_size[TDX_PS_2M]),
                TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE,   
pamt_entry_size[TDX_PS_1G]),
        };
-       int ret;
-       int i;

        /* Populate 'tdmr_sysinfo' fields using the mapping structure
above: */
-       for (i = 0; i < ARRAY_SIZE(fields); i++) {
-               ret = read_sys_metadata_field16(fields[i].field_id,
-                                               fields[i].offset,
-                                               tdmr_sysinfo);
-               if (ret)
-                       return ret;
-       }
-
-       return 0;
+       return read_sys_metadata_multi(fields, ARRAY_SIZE(fields),
+                       tdmr_sysinfo);
 }

 /* Calculate the actual TDMR size */
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index b701f69485d3..812943516946 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -53,7 +53,8 @@
 #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id)   \
                (((_field_id) & GENMASK_ULL(33, 32)) >> 32)

-#define MD_FIELD_ID_ELE_SIZE_16BIT     1
+#define MD_FIELD_BYTES(_field_id)      \
+               (1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id))

 struct tdmr_reserved_area {
        u64 offset;
-- 
2.43.2


