Date:   Thu, 28 Apr 2022 13:35:41 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     seanjc@...gle.com, pbonzini@...hat.com, len.brown@...el.com,
        tony.luck@...el.com, rafael.j.wysocki@...el.com,
        reinette.chatre@...el.com, dan.j.williams@...el.com,
        peterz@...radead.org, ak@...ux.intel.com,
        kirill.shutemov@...ux.intel.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com,
        isaku.yamahata@...el.com
Subject: Re: [PATCH v3 10/21] x86/virt/tdx: Add placeholder to coveret all
 system RAM as TDX memory

On Wed, 2022-04-27 at 18:07 -0700, Dave Hansen wrote:
> On 4/27/22 17:53, Kai Huang wrote:
> > 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".
> 
> Seriously, though.  I took the effort to write these changelogs for you.
>  They were fine.  I'm not stoked about needing to proofread them again.

Yeah pretty clear to me now. Really thanks for your time.

Won't happen again.  If there's something I feel not right, I'll raise but not
slightly change.

> 
> > > > 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".
> 
> I'm pretty sure it's wrong.  "up front" is an adverb that applies to
> "reserved".  "Upfront" is an adjective and not how you used it in that
> sentence.

Thanks for explaining.  Anyway the gmail grammar can have bug.

> 
> > > > +	 * 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.
> 
> The comment was clear as mud about this.  If you're going to talk about
> alignment, then do it near the allocation that guarantees the alignment,
> not in some other function near *ANOTHER* allocation.
> 
> Also, considering that you're about to go allocate potentially gigabytes
> of physically contiguous memory, it seems laughable that you'd go to any
> trouble at all to allocate an array of pointers here.  Why not just
> 
> 	kcalloc(tdx_sysinfo.max_tdmrs, sizeof(struct tmdr_info), ...);

kmalloc() guarantees the size-alignment if the size is power-of-two.  TDMR_INFO
(512-bytes) itself is  power of two, but the 'max_tdmrs x sizeof(TDMR_INFO)' may
not be power of two.  For instance, when max_tdmrs == 3, the result is not
power-of-two.

Or am I wrong? I am not good at math though.

> 
> Or, heck, just vmalloc() the dang thing.  Why even bother with the array
> of pointers?
> 
> 
> > > > +	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?
> 
> Is this going to retry if it sees -EFAULT or -EBUSY?

No.  Currently we always shutdown the module in case of any error.  Caller won't
be able to retry.

In the future, this can be optimized.  We don't shutdown the module in case of
*some* error (i.e. -ENOMEM), but record an internal state when error happened,
so the caller can retry again.  For now, there's no retry.

-- 
Thanks,
-Kai

Powered by blists - more mailing lists