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
