On Wed, 2022-12-07 at 11:47 +0000, Huang, Kai wrote:
> On Mon, 2022-11-28 at 22:50 +0000, Huang, Kai wrote:
> > On Mon, 2022-11-28 at 14:19 -0800, Dave Hansen wrote:
> > > On 11/28/22 14:13, Huang, Kai wrote:
> > > > Apologize I am not entirely sure whether I fully got your point. Do you mean
> > > > something like below?
> > > ...
> > >
> > > No, something like this:
> > >
> > > static int init_tdx_module(void)
> > > {
> > > static struct tdsysinfo_struct tdx_sysinfo; /* too rotund for the stack */
> > > ...
> > > tdx_get_sysinfo(&tdx_sysinfo, ...);
> > > ...
> > >
> > > But, also, seriously, 3k on the stack is *fine* if you can shut up the
> > > warnings. This isn't going to be a deep call stack to begin with.
> > >
> >
> > Let me try to find out whether it is possible to silent the warning. If I
> > cannot, then I'll use your above way. Thanks!
>
> Hi Dave,
>
> Sorry to double asking.
>
> Adding below build flag to Makefile can silent the warning:
>
> index 38d534f2c113..f8a40d15fdfc 100644
> --- a/arch/x86/virt/vmx/tdx/Makefile
> +++ b/arch/x86/virt/vmx/tdx/Makefile
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0-only
> +CFLAGS_tdx.o += -Wframe-larger-than=4096
>
> So to confirm you want to add this flag to Makefile and just make tdx_sysinfo
> and tdx_cmr_array as local variables?
Hi Dave,
I found if I declare TDSYSINFO_STRUCT and CMR_ARRAY as local variable (on the
stack), the TDH.SYS.INFO failed in my testing due to 'invalid operand' of the
address of TDSYSINFO_STRUCT. If I declare them as static, the SEAMCALL works.
I haven't looked into the reason yet but I suspect the address isn't aligned (I
used __pa() to get the physical address). I'll take a look and report back.
In the meantime, do you have any comments? Should I still pursue to keep them
as local variable on the stack?
Thanks.
>
> Another reason I am double asking is, 'tdx_global_keyid' in this series can also
> be a local variable in init_tdx_module() but currently it is a static (as KVM
> will need it too). If I change to use local variable in the patch
> "x86/virt/tdx: Reserve TDX module global KeyID" like below:
>
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -50,9 +50,6 @@ static DEFINE_MUTEX(tdx_module_lock);
> /* All TDX-usable memory regions */
> static LIST_HEAD(tdx_memlist);
>
> -/* TDX module global KeyID. Used in TDH.SYS.CONFIG ABI. */
> -static u32 tdx_global_keyid;
> -
> /*
> * tdx_keyid_start and nr_tdx_keyids indicate that TDX is uninitialized.
> * This is used in TDX initialization error paths to take it from
> @@ -928,6 +925,7 @@ static int init_tdx_module(void)
> __aligned(CMR_INFO_ARRAY_ALIGNMENT);
> struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
> struct tdmr_info_list tdmr_list;
> + u32 global_keyid;
> int ret;
>
> ret = tdx_get_sysinfo(sysinfo, cmr_array);
> @@ -964,7 +962,7 @@ static int init_tdx_module(void)
> * Pick the first TDX KeyID as global KeyID to protect
> * TDX module metadata.
> */
> - tdx_global_keyid = tdx_keyid_start;
> + global_keyid = tdx_keyid_start;
>
> I got a warning for this particular patch:
>
> arch/x86/virt/vmx/tdx/tdx.c: In function ‘init_tdx_module’:
> arch/x86/virt/vmx/tdx/tdx.c:928:13: warning: variable ‘global_keyid’ set but not
> used [-Wunused-but-set-variable]
> 928 | u32 global_keyid;
> | ^~~~~~~~~~~~
>
> To get rid of this warning, we need to merge this patch to the later patch
> (which configures the TDMRs and global keyid to the TDX module).
>
> Should I make the tdx_global_keyid as local variable too and merge patch
> "x86/virt/tdx: Reserve TDX module global KeyID" to the later patch?
And for this one, if we merge the two patches then in fact we can just remove
'tdx_global_keyid' but use 'tdx_start_keyid' directly. I have already done in
this way. Any comments please let me know. Thanks for your time.
