lkml.org 
[lkml]   [2022]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 10/21] x86/virt/tdx: Add placeholder to coveret all system RAM as TDX memory
    From
    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.

    \
     
     \ /
      Last update: 2022-04-28 05:41    [W:4.082 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site