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

[Huang, Kai]

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]