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

From: "Huang, Kai" <kai.huang@intel.com>
To: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "Luck, Tony" <tony.luck@intel.com>,
	"bagasdotme@gmail.com" <bagasdotme@gmail.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"Shahar, Sagi" <sagis@google.com>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"Gao, Chao" <chao.gao@intel.com>,
	"Brown, Len" <len.brown@intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: Re: [PATCH v7 11/20] x86/virt/tdx: Add placeholder to construct TDMRs to cover all TDX memory regions
Date: Wed, 7 Dec 2022 11:47:35 +0000	[thread overview]
Message-ID: <98935273b05feb55fc52c69a48d31018e0124e58.camel@intel.com> (raw)
In-Reply-To: <02d2a49cbd319814a7afdf57ca3cc9809123952a.camel@intel.com>

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?

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?
