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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <702c11db9820a074aee31c2958a565b21299607f.camel@intel.com>
Date:   Thu, 8 Dec 2022 12:56:08 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Luck, Tony" <tony.luck@...el.com>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>
Subject: Re: [PATCH v7 11/20] x86/virt/tdx: Add placeholder to construct TDMRs
 to cover all TDX memory regions

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.