Re: [PATCH v3 1/8] x86/virt/tdx: Rename 'struct tdx_tdmr_sysinfo' to reflect the spec better

[Adrian Hunter <adrian.hunter@intel.com>]

On 27/08/24 10:14, Kai Huang wrote:

"to reflect the spec better" is a bit vague.  How about:

x86/virt/tdx: Rename 'struct tdx_tdmr_sysinfo' to 'struct tdx_sys_info_tdmr'

Rename 'struct tdx_tdmr_sysinfo' to 'struct tdx_sys_info_tdmr' to
prepare for adding similar structures that will all be prefixed by
'tdx_sys_info_'.

> The TDX module provides a set of "global metadata fields".  They report

Since it is a name of something, could capitalize "Global Metadata Fields"

> things like TDX module version, supported features, and fields related
> to create/run TDX guests and so on.
> 
> TDX organizes those metadata fields by "Class"es based on the meaning of

by "Class"es	->	into "Classes"

> those fields.  E.g., for now the kernel only reads "TD Memory Region"
> (TDMR) related fields for module initialization.  Those fields are
> defined under class "TDMR Info".
> 
> There are both immediate needs to read more metadata fields for module
> initialization and near-future needs for other kernel components like
> KVM to run TDX guests.  To meet all those requirements, the idea is the
> TDX host core-kernel to provide a centralized, canonical, and read-only
> structure for the global metadata that comes out from the TDX module for
> all kernel components to use.
> 
> More specifically, the target is to end up with something like:
> 
>        struct tdx_sys_info {
> 	       struct tdx_sys_info_classA a;
> 	       struct tdx_sys_info_classB b;
> 	       ...
>        };
> 
> Currently the kernel organizes all fields under "TDMR Info" class in
> 'struct tdx_tdmr_sysinfo'.  To prepare for the above target, rename the
> structure to 'struct tdx_sys_info_tdmr' to follow the class name better.
> 
> No functional change intended.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> v2 -> v3:
>  - Split out as a separate patch and place it at beginning:
> 
>    https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m8fec7c429242d640cf5e756eb68e3b822e6dff8b
>  
>  - Rename to 'struct tdx_sys_info_tdmr':
> 
>    https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#md73dd9b02a492acf4a6facae63e8d030e320967d
>    https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m8fec7c429242d640cf5e756eb68e3b822e6dff8b
> 
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 36 ++++++++++++++++++------------------
>  arch/x86/virt/vmx/tdx/tdx.h |  2 +-
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 4e2b2e2ac9f9..e979bf442929 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -272,7 +272,7 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
>  
>  static int read_sys_metadata_field16(u64 field_id,
>  				     int offset,
> -				     struct tdx_tdmr_sysinfo *ts)
> +				     struct tdx_sys_info_tdmr *ts)
>  {
>  	u16 *ts_member = ((void *)ts) + offset;
>  	u64 tmp;
> @@ -298,9 +298,9 @@ struct field_mapping {
>  
>  #define TD_SYSINFO_MAP(_field_id, _offset) \
>  	{ .field_id = MD_FIELD_ID_##_field_id,	   \
> -	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _offset) }
> +	  .offset   = offsetof(struct tdx_sys_info_tdmr, _offset) }
>  
> -/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
> +/* Map TD_SYSINFO fields into 'struct tdx_sys_info_tdmr': */
>  static const struct field_mapping fields[] = {
>  	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs),
>  	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
> @@ -309,16 +309,16 @@ static const struct field_mapping fields[] = {
>  	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
>  };
>  
> -static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
> +static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
>  {
>  	int ret;
>  	int i;
>  
> -	/* Populate 'tdmr_sysinfo' fields using the mapping structure above: */
> +	/* Populate 'sysinfo_tdmr' 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);
> +						sysinfo_tdmr);
>  		if (ret)
>  			return ret;
>  	}
> @@ -342,13 +342,13 @@ static int tdmr_size_single(u16 max_reserved_per_tdmr)
>  }
>  
>  static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
> -			   struct tdx_tdmr_sysinfo *tdmr_sysinfo)
> +			   struct tdx_sys_info_tdmr *sysinfo_tdmr)
>  {
>  	size_t tdmr_sz, tdmr_array_sz;
>  	void *tdmr_array;
>  
> -	tdmr_sz = tdmr_size_single(tdmr_sysinfo->max_reserved_per_tdmr);
> -	tdmr_array_sz = tdmr_sz * tdmr_sysinfo->max_tdmrs;
> +	tdmr_sz = tdmr_size_single(sysinfo_tdmr->max_reserved_per_tdmr);
> +	tdmr_array_sz = tdmr_sz * sysinfo_tdmr->max_tdmrs;
>  
>  	/*
>  	 * To keep things simple, allocate all TDMRs together.
> @@ -367,7 +367,7 @@ static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
>  	 * at a given index in the TDMR list.
>  	 */
>  	tdmr_list->tdmr_sz = tdmr_sz;
> -	tdmr_list->max_tdmrs = tdmr_sysinfo->max_tdmrs;
> +	tdmr_list->max_tdmrs = sysinfo_tdmr->max_tdmrs;
>  	tdmr_list->nr_consumed_tdmrs = 0;
>  
>  	return 0;
> @@ -921,11 +921,11 @@ static int tdmrs_populate_rsvd_areas_all(struct tdmr_info_list *tdmr_list,
>  /*
>   * Construct a list of TDMRs on the preallocated space in @tdmr_list
>   * to cover all TDX memory regions in @tmb_list based on the TDX module
> - * TDMR global information in @tdmr_sysinfo.
> + * TDMR global information in @sysinfo_tdmr.
>   */
>  static int construct_tdmrs(struct list_head *tmb_list,
>  			   struct tdmr_info_list *tdmr_list,
> -			   struct tdx_tdmr_sysinfo *tdmr_sysinfo)
> +			   struct tdx_sys_info_tdmr *sysinfo_tdmr)
>  {
>  	int ret;
>  
> @@ -934,12 +934,12 @@ static int construct_tdmrs(struct list_head *tmb_list,
>  		return ret;
>  
>  	ret = tdmrs_set_up_pamt_all(tdmr_list, tmb_list,
> -			tdmr_sysinfo->pamt_entry_size);
> +			sysinfo_tdmr->pamt_entry_size);
>  	if (ret)
>  		return ret;
>  
>  	ret = tdmrs_populate_rsvd_areas_all(tdmr_list, tmb_list,
> -			tdmr_sysinfo->max_reserved_per_tdmr);
> +			sysinfo_tdmr->max_reserved_per_tdmr);
>  	if (ret)
>  		tdmrs_free_pamt_all(tdmr_list);
>  
> @@ -1098,7 +1098,7 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list)
>  
>  static int init_tdx_module(void)
>  {
> -	struct tdx_tdmr_sysinfo tdmr_sysinfo;
> +	struct tdx_sys_info_tdmr sysinfo_tdmr;
>  	int ret;
>  
>  	/*
> @@ -1117,17 +1117,17 @@ static int init_tdx_module(void)
>  	if (ret)
>  		goto out_put_tdxmem;
>  
> -	ret = get_tdx_tdmr_sysinfo(&tdmr_sysinfo);
> +	ret = get_tdx_sys_info_tdmr(&sysinfo_tdmr);
>  	if (ret)
>  		goto err_free_tdxmem;
>  
>  	/* Allocate enough space for constructing TDMRs */
> -	ret = alloc_tdmr_list(&tdx_tdmr_list, &tdmr_sysinfo);
> +	ret = alloc_tdmr_list(&tdx_tdmr_list, &sysinfo_tdmr);
>  	if (ret)
>  		goto err_free_tdxmem;
>  
>  	/* Cover all TDX-usable memory regions in TDMRs */
> -	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &tdmr_sysinfo);
> +	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, &sysinfo_tdmr);
>  	if (ret)
>  		goto err_free_tdmrs;
>  
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index b701f69485d3..148f9b4d1140 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -100,7 +100,7 @@ struct tdx_memblock {
>  };
>  
>  /* "TDMR info" part of "Global Scope Metadata" for constructing TDMRs */
> -struct tdx_tdmr_sysinfo {
> +struct tdx_sys_info_tdmr {
>  	u16 max_tdmrs;
>  	u16 max_reserved_per_tdmr;
>  	u16 pamt_entry_size[TDX_PS_NR];