Re: [PATCH v8 12/16] x86/virt/tdx: Designate the global KeyID and configure the TDX module

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 12/16] x86/virt/tdx: Designate the global KeyID and configure the TDX module
Date: Tue, 10 Jan 2023 10:48:26 +0000	[thread overview]
Message-ID: <748adb0e8df5f804371f0587ed8fef1184177484.camel@intel.com> (raw)
In-Reply-To: <e1a55f50-efc0-5098-1a2d-f0eaa0b51a82@intel.com>

On Fri, 2023-01-06 at 14:21 -0800, Dave Hansen wrote:
> On 12/8/22 22:52, Kai Huang wrote:
> > After a list of "TD Memory Regions" (TDMRs) has been constructed to
> > cover all TDX-usable memory regions, the next step is to pick up a TDX
> > private KeyID as the "global KeyID" (which protects, i.e. TDX module's
> > metadata), and configure it to the TDX module along with the TDMRs.
> 
> For whatever reason, whenever I see "i.e." in a changelog, it's usually
> going off the rails.  This is no exception.  Let's also get rid of the
> passive voice:
> 
> 	The next step After constructing a list of "TD Memory Regions"
> 	(TDMRs) to cover all TDX-usable memory regions is to designate a
> 	TDX private KeyID as the "global KeyID".  This KeyID is used by
> 	the TDX module for mapping things like the PAMT and other TDX
> 	metadata.  This KeyID is passed to the TDX module at the same
> 	time as the TDMRs.

Thanks.  Will use.

> 
> > To keep things simple, just use the first TDX KeyID as the global KeyID.
> 
> 
> 
> 
> > ---
> >  arch/x86/virt/vmx/tdx/tdx.c | 41 +++++++++++++++++++++++++++++++++++--
> >  arch/x86/virt/vmx/tdx/tdx.h |  2 ++
> >  2 files changed, 41 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 620b35e2a61b..ab961443fed5 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -916,6 +916,36 @@ static int construct_tdmrs(struct list_head *tmb_list,
> >  	return ret;
> >  }
> >  
> > +static int config_tdx_module(struct tdmr_info_list *tdmr_list, u64 global_keyid)
> > +{
> > +	u64 *tdmr_pa_array, *p;
> > +	size_t array_sz;
> > +	int i, ret;
> > +
> > +	/*
> > +	 * TDMRs are passed to the TDX module via an array of physical
> > +	 * addresses of each TDMR.  The array itself has alignment
> > +	 * requirement.
> > +	 */
> > +	array_sz = tdmr_list->nr_tdmrs * sizeof(u64) +
> > +		TDMR_INFO_PA_ARRAY_ALIGNMENT - 1;
> 
> One other way of doing this which might be a wee bit less messy:
> 
> 	array_sz = roundup_pow_of_two(array_sz);
> 	if (array_sz < TDMR_INFO_PA_ARRAY_ALIGNMENT)
> 		array_sz = TDMR_INFO_PA_ARRAY_ALIGNMENT;
> 
> Since that keeps 'array_sz' at a power-of-two, then kzalloc() will give
> you all the alignment you need, except if the array is too small, in
> which case you can just bloat it to the alignment requirement.
> 
> This would get rid of the PTR_ALIGN() below too.
> 
> Your choice.  What you have works too.

Your code can also get rid of the additional 'p' local variable.  As you said it
is simpler.  I'll use your code.  Thanks!

> 
> > +	p = kzalloc(array_sz, GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> > +	tdmr_pa_array = PTR_ALIGN(p, TDMR_INFO_PA_ARRAY_ALIGNMENT);
> > +	for (i = 0; i < tdmr_list->nr_tdmrs; i++)
> > +		tdmr_pa_array[i] = __pa(tdmr_entry(tdmr_list, i));
> > +
> > +	ret = seamcall(TDH_SYS_CONFIG, __pa(tdmr_pa_array), tdmr_list->nr_tdmrs,
> > +				global_keyid, 0, NULL, NULL);
> > +
> > +	/* Free the array as it is not required anymore. */
> > +	kfree(p);
> > +
> > +	return ret;
> > +}
> > +
> >  static int init_tdx_module(void)
> >  {
> >  	/*
> > @@ -960,17 +990,24 @@ static int init_tdx_module(void)
> >  	if (ret)
> >  		goto out_free_tdmrs;
> >  
> > +	/*
> > +	 * Use the first private KeyID as the global KeyID, and pass
> > +	 * it along with the TDMRs to the TDX module.
> > +	 */
> > +	ret = config_tdx_module(&tdmr_list, tdx_keyid_start);
> > +	if (ret)
> > +		goto out_free_pamts;
> 
> This is "consuming" tdx_keyid_start.  Does it need to get incremented
> since the first guest can't use this KeyID now?

It depends on how we treat 'tdx_keyid_start'.  If it means the first _usable_
KeyID for KVM, then we should increase it; but if it only used for the hardware-
enabled TDX KeyID range, then we don't need to increase it.

Currently it is marked as __ro_after_init so my intention is the latter (also in
the spirit of keeping this series minimal).

Eventually we will need to have functions to allocate/free TDX KeyIDs anyway for
KVM, but in that we can just treat 'tdx_keyid_start + 1' as the first usable
KeyID.

[snip]