lkml.org 
[lkml]   [2022]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 09/21] x86/virt/tdx: Get information about TDX module and convertible memory
    From
    Date
    On Wed, 2022-05-25 at 16:57 +1200, Kai Huang wrote:
    > On Wed, 2022-05-25 at 16:47 +1200, Kai Huang wrote:
    > > On Fri, 2022-04-29 at 10:47 -0700, Dave Hansen wrote:
    > > > On 4/28/22 16:14, Kai Huang wrote:
    > > > > On Thu, 2022-04-28 at 07:06 -0700, Dave Hansen wrote:
    > > > > > On 4/27/22 17:15, Kai Huang wrote:
    > > > > > > > Couldn't we get rid of that comment if you did something like:
    > > > > > > >
    > > > > > > > ret = tdx_get_sysinfo(&tdx_cmr_array, &tdx_sysinfo);
    > > > > > >
    > > > > > > Yes will do.
    > > > > > >
    > > > > > > > and preferably make the variables function-local.
    > > > > > >
    > > > > > > 'tdx_sysinfo' will be used by KVM too.
    > > > > >
    > > > > > In other words, it's not a part of this series so I can't review whether
    > > > > > this statement is correct or whether there's a better way to hand this
    > > > > > information over to KVM.
    > > > > >
    > > > > > This (minor) nugget influencing the design also isn't even commented or
    > > > > > addressed in the changelog.
    > > > >
    > > > > TDSYSINFO_STRUCT is 1024B and CMR array is 512B, so I don't think it should be
    > > > > in the stack. I can change to use dynamic allocation at the beginning and free
    > > > > it at the end of the function. KVM support patches can change it to static
    > > > > variable in the file.
    > > >
    > > > 2k of stack is big, but it isn't a deal breaker for something that's not
    > > > nested anywhere and that's only called once in a pretty controlled
    > > > setting and not in interrupt context. I wouldn't cry about it.
    > >
    > > Hi Dave,
    > >
    > > I got below complaining when I use local variable for TDSYSINFO_STRUCT and CMR
    > > array:
    > >
    > > arch/x86/virt/vmx/tdx/tdx.c:383:1: warning: the frame size of 3072 bytes is
    > > larger than 1024 bytes [-Wframe-larger-than=]
    > > 383 | }
    > >
    > > So I don't think we can use local variable for them. I'll still use static
    > > variables to avoid dynamic allocation. In the commit message, I'll explain they
    > > are too big to put into the stack, and KVM will need to use TDSYSINFO_STRUCT
    > > reported by TDX module anyway.
    > >
    > > Let me know if you don't agree?
    >
    > Btw, CMR array alone can be put into the stack. It will never be used by KVM,
    > so I'll put CMR array as local variable, but keep tdx_sysinfo as static
    > variable.
    >

    Sorry for multiple emails about this. If I put CMR array to the stack, I still
    sometimes get the build warning. So will use static variables.

    Also, constructing TDMRs internally needs to use tdx_sysinfo (max_tdmrs,
    pamt_entry_size, max_rsvd_per_tdmr), so with static variable they don't need to
    be passed around as function arguments.

    --
    Thanks,
    -Kai


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