Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <73c8e61c-1057-a3ff-904d-6b7ddaaac83b@intel.com>
Date:   Wed, 27 Apr 2022 20:40:09 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v3 10/21] x86/virt/tdx: Add placeholder to coveret all
 system RAM as TDX memory
Content-Language: en-US
To:     Kai Huang <kai.huang@intel.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     seanjc@google.com, pbonzini@redhat.com, len.brown@intel.com,
        tony.luck@intel.com, rafael.j.wysocki@intel.com,
        reinette.chatre@intel.com, dan.j.williams@intel.com,
        peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        isaku.yamahata@intel.com
References: <cover.1649219184.git.kai.huang@intel.com>
 <6230ef28be8c360ab326c8f592acf1964ac065c1.1649219184.git.kai.huang@intel.com>
 <d69c08da-80fa-2001-bbe8-8c45552e74ae@intel.com>
 <228cfa7e5326fa378c1dde2b5e9022146f97b706.camel@intel.com>
 <1624e839-81e5-7bc7-533b-c5c838d35f47@intel.com>
 <a6fb489700ce00fcb32a670a2fd7bf99a113d878.camel@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <a6fb489700ce00fcb32a670a2fd7bf99a113d878.camel@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/27/22 18:35, Kai Huang wrote:
> On Wed, 2022-04-27 at 18:07 -0700, Dave Hansen wrote:
>> 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.

No, you're right, the kcalloc() wouldn't work for odd sizes.

But, the point is still that you don't need an array of pointers.  Use
vmalloc().  Use a plain old alloc_pages_exact().  Why bother wasting
the memory and addiong the complexity of an array of pointers?

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

Just make the error codes -EINVAL, please.  I don't think anything else
makes sense.