Re: [PATCH v7 11/20] x86/virt/tdx: Add placeholder to construct TDMRs to cover all TDX memory regions

[Huang, Kai]

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.