Message-ID: <6720064bf2c69_bc69d2947b@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 28 Oct 2024 14:46:52 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Kai Huang <kai.huang@...el.com>, <dave.hansen@...el.com>,
	<kirill.shutemov@...ux.intel.com>, <tglx@...utronix.de>, <bp@...en8.de>,
	<peterz@...radead.org>, <mingo@...hat.com>, <hpa@...or.com>,
	<dan.j.williams@...el.com>, <seanjc@...gle.com>, <pbonzini@...hat.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<rick.p.edgecombe@...el.com>, <isaku.yamahata@...el.com>,
	<adrian.hunter@...el.com>, <nik.borisov@...e.com>, <kai.huang@...el.com>
Subject: Re: [PATCH v6 03/10] x86/virt/tdx: Use auto-generated code to read
 global metadata

Kai Huang wrote:
> From: Paolo Bonzini <pbonzini@...hat.com>
> 
> The TDX module provides a set of "Global Metadata Fields".  Currently
> the kernel only reads "TD Memory Region" (TDMR) related fields for
> module initialization.  There are needs to read more global metadata
> fields including TDX module version [1], supported features [2] and
> "Convertible Memory Regions" (CMRs) to fix a module initialization
> failure [3].  Future changes to support KVM TDX and other features like
> TDX Connect will need to read more.
> 
> The current global metadata reading code has limitations (e.g., it only
> has a primitive helper to read metadata field with 16-bit element size,
> while TDX supports 8/16/32/64 bits metadata element sizes).  It needs
> tweaks in order to read more metadata fields.
> 
> But even with the tweaks, when new code is added to read a new field,
> the reviewers will still need to review against the spec to make sure
> the new code doesn't screw up things like using the wrong metadata
> field ID (each metadata field is associated with a unique field ID,
> which is a TDX-defined u64 constant) etc.
> 
> TDX documents all global metadata fields in a 'global_metadata.json'
> file as part of TDX spec [4].  JSON format is machine readable.  Instead
> of tweaking the metadata reading code, use a script [5] to generate the
> code so that:
> 
>   1) Using the generated C is simple.
>   2) Adding a field is dirty simple, e.g., the script just pulls the

Probably meant "dirt simple", but if this is fixed up on apply I'd drop
the idiom and just say "simple".

...don't spin the patch just for this nit.

>      field ID out of the JSON for a given field thus no manual review is
>      needed.
> 
> Specifically, to match the layout of the 'struct tdx_sys_info' and its
> sub-structures, the script uses a table with each entry containing the
> the name of the sub-structures (which reflects the "Class") and the
> "Field Name" of all its fields, and auto-generate:
> 
>   1) The 'struct tdx_sys_info' and all 'struct tdx_sys_info_xx'
>      sub-structures in 'tdx_global_metadata.h'
> 
>   2) The main function 'get_tdx_sys_info()' which reads all metadata to
>      'struct tdx_sys_info' and the 'get_tdx_sys_info_xx()' functions
>      which read 'struct tdx_sys_info_xx()' in 'tdx_global_metadata.c'.
> 
> Using the generated C is simple: 1) include "tdx_global_metadata.h" to
> the local "tdx.h"; 2) explicitly include "tdx_global_metadata.c" to the
> local "tdx.c" after the read_sys_metadata_field() primitive (which is a
> wrapper of TDH.SYS.RD SEAMCALL to read global metadata).
> 
> Adding a field is also simple: 1) just add the new field to an existing
> structure, or add it with a new structure; 2) re-run the script to
> generate the new code; 3) update the existing tdx_global_metadata.{hc}
> with the new ones.
> 
> For now, use the auto-generated code to read the aforesaid metadata
> fields: 1) TDX module version; 2) supported features; 3) CMRs.
> 
> Reading CMRs is more complicated than reading a simple field, since
> there are two arrays containing the "CMR_BASE" and "CMR_SIZE" for each
> CMR respectively.
> 
> TDX spec [3] section "Metadata Access Interface", sub-section "Arrays of
> Metadata Fields" defines the way to read metadata fields in an array.
> There's a "Base field ID" (say, X) for the array and the field ID for
> entry array[i] is X + i.
> 
> For CMRs, the field "NUM_CMRS" reports the number of CMR entries that
> can be read, and the code needs to use the value reported via "NUM_CMRS"
> to loop despite the JSON file says the "Num Fields" of both "CMR_BASE"
> and "CMR_SIZE" are 32.
> 
> The tdx_global_metadata.{hc} can be generated by running below:
> 
>  #python tdx.py global_metadata.json tdx_global_metadata.h \
> 	tdx_global_metadata.c
> 
> .. where tdx.py can be found in [5] and global_metadata.json can be
> fetched from [4].
> 
> Link: https://lore.kernel.org/lkml/4b3adb59-50ea-419e-ad02-e19e8ca20dee@intel.com/ [1]
> Link: https://lore.kernel.org/all/fc0e8ab7-86d4-4428-be31-82e1ece6dd21@intel.com/ [2]
> Link: https://lore.kernel.org/kvm/0853b155ec9aac09c594caa60914ed6ea4dc0a71.camel@intel.com/ [5]

Just an fyi, that lore accepts the simple:

https://lore.kernel.org/$msg_id

...format, no need to record the list name in the URL (127734e23aed
("Documentation: best practices for using Link trailers"))

> Link: https://github.com/canonical/tdx/issues/135 [3]
> Link: https://cdrdv2.intel.com/v1/dl/getContent/795381 [4]
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> Co-developed-by: Kai Huang <kai.huang@...el.com>
> Signed-off-by: Kai Huang <kai.huang@...el.com>

Looks good to me, with or without the above nits addressed.

Reviewed-by: Dan Williams <dan.j.williams@...el.com>

Powered by blists - more mailing lists