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

[Dave Hansen <dave.hansen@intel.com>]

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.

>>> 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.

>>> +	 * 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), ...);

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?