Re: [PATCH v19 033/130] KVM: TDX: Add helper function to read TDX metadata in array

Huang, Kai
2024-03-14T22:28:12Z

On 26/02/2024 9:25 pm, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> To read meta data in series, use table.
> Instead of metadata_read(fid0, &data0); metadata_read(...); ...
> table = { {fid0, &data0}, ...}; metadata-read(tables).

This explains nothing why the code introduced in patch 5 cannot be used.

> TODO: Once the TDX host code introduces its framework to read TDX metadata,
> drop this patch and convert the code that uses this.

Seriously, what is this?? Please treat your patches as "official" patches.

>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
> v18:
> - newly added
> ---
> arch/x86/kvm/vmx/tdx.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index cde971122c1e..dce21f675155 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -6,6 +6,7 @@
> #include "capabilities.h"
> #include "x86_ops.h"
> #include "x86.h"
> +#include "tdx_arch.h"
> #include "tdx.h"
>
> #undef pr_fmt
> @@ -39,6 +40,50 @@ static void __used tdx_guest_keyid_free(int keyid)
> ida_free(&tdx_guest_keyid_pool, keyid);
> }
>
> +#define TDX_MD_MAP(_fid, _ptr) \
> + { .fid = MD_FIELD_ID_##_fid, \
> + .ptr = (_ptr), }
> +
> +struct tdx_md_map {
> + u64 fid;
> + void *ptr;
> +};
> +
> +static size_t tdx_md_element_size(u64 fid)
> +{
> + switch (TDX_MD_ELEMENT_SIZE_CODE(fid)) {
> + case TDX_MD_ELEMENT_SIZE_8BITS:
> + return 1;
> + case TDX_MD_ELEMENT_SIZE_16BITS:
> + return 2;
> + case TDX_MD_ELEMENT_SIZE_32BITS:
> + return 4;
> + case TDX_MD_ELEMENT_SIZE_64BITS:
> + return 8;
> + default:
> + WARN_ON_ONCE(1);
> + return 0;
> + }
> +}
> +
> +static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> +{
> + struct tdx_md_map *m;
> + int ret, i;
> + u64 tmp;
> +
> + for (i = 0; i < nr_maps; i++) {
> + m = &maps[i];
> + ret = tdx_sys_metadata_field_read(m->fid, &tmp);
> + if (ret)
> + return ret;
> +
> + memcpy(m->ptr, &tmp, tdx_md_element_size(m->fid));
> + }
> +
> + return 0;
> +}
> +

It's just insane to have two duplicated mechanism for metadata reading.

This will only confuse people.

If there's anything missing in patch 1-5, we can enhance them.