Re: [PATCH v3 10/21] x86/virt/tdx: Add placeholder to coveret all system RAM as TDX memory

[Kai Huang]

On Wed, 2022-04-27 at 15:24 -0700, Dave Hansen wrote:
> On 4/5/22 21:49, Kai Huang wrote:
> > TDX provides increased levels of memory confidentiality and integrity.
> > This requires special hardware support for features like memory
> > encryption and storage of memory integrity checksums.  Not all memory
> > satisfies these requirements.
> > 
> > As a result, TDX introduced the concept of a "Convertible Memory Region"
> > (CMR).  During boot, the firmware builds a list of all of the memory
> > ranges which can provide the TDX security guarantees.  The list of these
> > ranges, along with TDX module information, is available to the kernel by
> > querying the TDX module.
> > 
> > In order to provide crypto protection to TD guests, the TDX architecture
> 
> There's that "crypto protection" thing again.  I'm not really a fan of
> the changes made to this changelog since I wrote it. :)

Sorry about that.  I'll remove "In order to provide crypto protection to TD
guests".

> 
> > also needs additional metadata to record things like which TD guest
> > "owns" a given page of memory.  This metadata essentially serves as the
> > 'struct page' for the TDX module.  The space for this metadata is not
> > reserved by the hardware upfront and must be allocated by the kernel
> 
> 			    ^ "up front"

Thanks will change to "up front".

Btw, the gmail grammar check gives me a red line if I use "up front", but it
doesn't complain "upfront".

> 
> ...
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 482e6d858181..ec27350d53c1 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/cpu.h>
> >  #include <linux/smp.h>
> >  #include <linux/atomic.h>
> > +#include <linux/slab.h>
> >  #include <asm/msr-index.h>
> >  #include <asm/msr.h>
> >  #include <asm/cpufeature.h>
> > @@ -594,8 +595,29 @@ static int tdx_get_sysinfo(void)
> >  	return sanitize_cmrs(tdx_cmr_array, cmr_num);
> >  }
> >  
> > +static void free_tdmrs(struct tdmr_info **tdmr_array, int tdmr_num)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < tdmr_num; i++) {
> > +		struct tdmr_info *tdmr = tdmr_array[i];
> > +
> > +		/* kfree() works with NULL */
> > +		kfree(tdmr);
> > +		tdmr_array[i] = NULL;
> > +	}
> > +}
> > +
> > +static int construct_tdmrs(struct tdmr_info **tdmr_array, int *tdmr_num)
> > +{
> > +	/* Return -EFAULT until constructing TDMRs is done */
> > +	return -EFAULT;
> > +}
> > +
> >  static int init_tdx_module(void)
> >  {
> > +	struct tdmr_info **tdmr_array;
> > +	int tdmr_num;
> >  	int ret;
> >  
> >  	/* TDX module global initialization */
> > @@ -613,11 +635,36 @@ static int init_tdx_module(void)
> >  	if (ret)
> >  		goto out;
> >  
> > +	/*
> > +	 * Prepare enough space to hold pointers of TDMRs (TDMR_INFO).
> > +	 * TDX requires TDMR_INFO being 512 aligned.  Each TDMR is
> 
> 					 ^ "512-byte aligned"
> 
> Right?

Yes.  Will update.

> 
> > +	 * allocated individually within construct_tdmrs() to meet
> > +	 * this requirement.
> > +	 */
> > +	tdmr_array = kcalloc(tdx_sysinfo.max_tdmrs, sizeof(struct tdmr_info *),
> > +			GFP_KERNEL);
> 
> Where, exactly is that alignment provided?  A 'struct tdmr_info *' is 8
> bytes so a tdx_sysinfo.max_tdmrs=8 kcalloc() would only guarantee
> 64-byte alignment.

The entries in the array only contain a pointer to TDMR_INFO.  The actual
TDMR_INFO is allocated separately. The array itself is never used by TDX
hardware so it doesn't matter.  We just need to guarantee each TDMR_INFO is
512B-byte aligned.

> 
> Also, I'm surprised that this is an array of virtual address pointers.
> The previous interactions with the TDX module seemed to all take
> physical addresses.  How is it that this hardware structure which has
> hardware alignment constraints is holding virtual addresses?

In later patches when TDMRs are configured to the TDX module, the input will be
converted to physical address, and there will be another array which is used by
the TDX module hardware.  This array is used to by kernel only to construct
TDMRs.

> 
> > +	if (!tdmr_array) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	/* Construct TDMRs to build TDX memory */
> > +	ret = construct_tdmrs(tdmr_array, &tdmr_num);
> > +	if (ret)
> > +		goto out_free_tdmrs;
> > +
> >  	/*
> >  	 * Return -EFAULT until all steps of TDX module
> >  	 * initialization are done.
> >  	 */
> >  	ret = -EFAULT;
> 
> There's the -EFAULT again.  I'd replace these with a better error code.

I couldn't think out a better error code.  -EINVAL looks doesn't suit.  -EAGAIN
also doesn't make sense for now since we always shutdown the TDX module in case
of any error so caller should never retry.  I think we need some error code to
tell "the job isn't done yet".  Perhaps -EBUSY?



-- 
Thanks,
-Kai