Re: [PATCH v8 08/16] x86/virt/tdx: Add placeholder to construct TDMRs to cover all TDX memory regions

From: "Huang, Kai" <kai.huang@intel.com>
To: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "Luck, Tony" <tony.luck@intel.com>,
	"bagasdotme@gmail.com" <bagasdotme@gmail.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"Shahar, Sagi" <sagis@google.com>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"Gao, Chao" <chao.gao@intel.com>,
	"Brown, Len" <len.brown@intel.com>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: Re: [PATCH v8 08/16] x86/virt/tdx: Add placeholder to construct TDMRs to cover all TDX memory regions
Date: Tue, 10 Jan 2023 00:40:35 +0000	[thread overview]
Message-ID: <81b814f096513e69e3099ab2b54034deadf8d7fd.camel@intel.com> (raw)
In-Reply-To: <c70c30fb-073f-4355-c6a6-052d013a99da@intel.com>

On Fri, 2023-01-06 at 11:24 -0800, Dave Hansen wrote:
> > +struct tdmr_info_list {
> > +	struct tdmr_info *first_tdmr;
> 
> This is named badly.  This is really a pointer to an array.  While it
> _does_ of course point to the first member of the array, the naming
> should make it clear that there are multiple tdmr_infos here.

Will change to 'tdmrs' as in your code.

> 
> > +	int tdmr_sz;
> > +	int max_tdmrs;
> > +	int nr_tdmrs;	/* Actual number of TDMRs */
> > +};
> 
> This 'tdmr_info_list's is declared in an unfortunate place.  I thought
> the tdmr_size_single() function below was related to it.

I think I can move it "tdx.h", which is claimed to have both TDX-arch data
structures and linux-defined structures anyway.

I think I can also move 'enum tdx_module_status_t' and 'struct tdx_memblock'
declarations to "tdx.h" too so that all declarations are in "tdx.h".

> 
> Also, tdmr_sz and max_tdmrs can both be derived from 'sysinfo'.  Do they
> really need to be stored here?  

It's not mandatory to keep them here.  I did it mainly because I want to avoid
passing 'sysinfo' as argument for almost all functions related to constructing
TDMRs.

For instance, 'tdmr_sz' is used to calculate the position of each individual
TDMR at a given index.  Instead of passing additional 'sysinfo' (or sysinfo-
>max_reserved_per_tdmr):

	struct tdmr_info *tdmr_entry(struct tdmr_info_list *tdmr_list,  int
idx,
				     struct tdsysinfo_struct *sysinfo) { ... }

I perfer:

	struct tdmr_info *tdmr_entry(struct tdmr_info_list *tdmr_list, int idx)
	{...}

tdmr_entry() is basically called in all 3 steps (fill out TDMRs, allocate PAMTs,
and designate reserved areas).  Having 'sysinfo' in it will require almost all
functions related to constructing TDMRs to have 'sysinfo' as argument, which
only makes the code more complicated and hurts the readability IMHO.

> If so, I think I'd probably do something
> like this with the structure:
> 
> struct tdmr_info_list {
> 	struct tdmr_info *tdmrs;
> 	int nr_consumed_tdmrs; // How many @tdmrs are in use
> 
> 	/* Metadata for freeing this structure: */
> 	int tdmr_sz;   // Size of one 'tdmr_info' (has a flex array)
> 	int max_tdmrs; // How many @tdmrs are allocated
> };
> 
> Modulo whataver folks are doing for comments these days.

Looks nice to me.  Will use.  A slight thing is 'tdmr_sz' is also used to get
the TDMR at a given index, but not just freeing the structure.

Btw, is C++ style comment "//" OK in kernel code?
> 
> > +/* Calculate the actual TDMR size */
> > +static int tdmr_size_single(u16 max_reserved_per_tdmr)
> > +{
> > +	int tdmr_sz;
> > +
> > +	/*
> > +	 * The actual size of TDMR depends on the maximum
> > +	 * number of reserved areas.
> > +	 */
> > +	tdmr_sz = sizeof(struct tdmr_info);
> > +	tdmr_sz += sizeof(struct tdmr_reserved_area) *
> > max_reserved_per_tdmr;
> > +
> > +	return ALIGN(tdmr_sz, TDMR_INFO_ALIGNMENT);
> > +}
> > +
> > +static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
> > +			   struct tdsysinfo_struct *sysinfo)
> > +{
> > +	size_t tdmr_sz, tdmr_array_sz;
> > +	void *tdmr_array;
> > +
> > +	tdmr_sz = tdmr_size_single(sysinfo->max_reserved_per_tdmr);
> > +	tdmr_array_sz = tdmr_sz * sysinfo->max_tdmrs;
> > +
> > +	/*
> > +	 * To keep things simple, allocate all TDMRs together.
> > +	 * The buffer needs to be physically contiguous to make
> > +	 * sure each TDMR is physically contiguous.
> > +	 */
> > +	tdmr_array = alloc_pages_exact(tdmr_array_sz,
> > +			GFP_KERNEL | __GFP_ZERO);
> > +	if (!tdmr_array)
> > +		return -ENOMEM;
> > +
> > +	tdmr_list->first_tdmr = tdmr_array;
> > +	/*
> 
> 	^ probably missing whitepsace before the comment
> 

Will add, assuming you mean a new empty line.  Thanks for the tip.

> > +	 * Keep the size of TDMR to find the target TDMR
> > +	 * at a given index in the TDMR list.
> > +	 */
> > +	tdmr_list->tdmr_sz = tdmr_sz;
> > +	tdmr_list->max_tdmrs = sysinfo->max_tdmrs;
> > +	tdmr_list->nr_tdmrs = 0;
> > +
> > +	return 0;
> > +}
> > +

[snip]