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: <8cb0535827a73b3fac75ac8b0163045ef166efa6.camel@intel.com>
Subject: Re: [PATCH v3 10/21] x86/virt/tdx: Add placeholder to coveret all
 system RAM as TDX memory
From:   Kai Huang <kai.huang@intel.com>
To:     Dave Hansen <dave.hansen@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
Date:   Thu, 28 Apr 2022 15:55:37 +1200
In-Reply-To: <73c8e61c-1057-a3ff-904d-6b7ddaaac83b@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>
         <73c8e61c-1057-a3ff-904d-6b7ddaaac83b@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2022-04-27 at 20:40 -0700, Dave Hansen wrote:
> 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?

OK.  This makes sense.

One thing I didn't say clearly is TDMR_INFO is 512-byte aligned, but not could
be larger than 512 bytes, and the maximum number of reserved areas in TDMR_INFO
is enumerated via TDSYSINFO_STRUCT.  We can always roundup TDMR_INFO size to be
512-byte aligned, and calculate enough pages to hold maximum number of
TDMR_INFO.  In this case, we can still guarantee each TDMR_INFO is 512-byte
aligned.

I'll change to use alloc_pages_exact(), since we can get physical address of
TDMR_INFO from it easily.

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

OK will do.

-- 
Thanks,
-Kai