linux-kernel - Re: [PATCH v8 06/16] x86/virt/tdx: Get information about TDX module and TDX-capable memory

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Jan 2023 22:07:12 +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>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "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 v8 06/16] x86/virt/tdx: Get information about TDX module
 and TDX-capable memory

On Mon, 2023-01-09 at 11:52 -0800, Hansen, Dave wrote:
> On 1/9/23 02:25, Huang, Kai wrote:
> > On Fri, 2023-01-06 at 09:46 -0800, Dave Hansen wrote:
> ...
> > > > Note not all members in the 1024 bytes TDX module information are used
> > > > (even by the KVM).
> > > 
> > > I'm not sure what this has to do with anything.
> > 
> > You mentioned in v7 that:
> > > > > This is also a great place to mention that the tdsysinfo_struct
> contains
> > > > a *lot* of gunk which will not be used for a bit or that may never get
> > > > used.
> > 
> > https://lore.kernel.org/linux-mm/cc195eb6499cf021b4ce2e937200571915bfe66f.camel@intel.com/T/#m168e619aac945fa418ccb1d6652113003243d895
> > 
> > Perhaps I misunderstood something but I was trying to address this.
> > 
> > Should I remove this sentence?
> 
> If someone goes looking at this patch, the see tdsysinfo_struct with
> something like two dozen defined fields.  But, very few of them get used
> in this patch.  Why?  Just saying that they are unused is a bit silly.
> 
> 	The 'tdsysinfo_struct' is fairly large (1k) and contains a lot
> 	of info about the TD.  Fully define the entire structure, but
			  ^
		should be: "about the TDX module"?
			
> 	only use the fields necessary to build the PAMT and TDMRs and
> 	pr_info() some basics about the module.

Above looks great!  Thanks.

> 
> 	The rest of the fields will get used... (by kvm?  never??)

The current KVM TDX support series uses majority of the rest fields:

https://lore.kernel.org/lkml/99e5fcf2a7127347816982355fd4141ee1038a54.1667110240.git.isaku.yamahata@intel.com/

Only one field isn't used, but I don't want to assume it won't be used forever,
so I think "The rest of the fields will get used by KVM." is good enough.

> 
> ...
> > > > +	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
> > > > +	int ret;
> > > > +
> > > > +	ret = tdx_get_sysinfo(sysinfo, cmr_array);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > >  	/*
> > > >  	 * TODO:
> > > >  	 *
> > > > -	 *  - Get TDX module information and TDX-capable memory regions.
> > > >  	 *  - Build the list of TDX-usable memory regions.
> > > >  	 *  - Construct a list of TDMRs to cover all TDX-usable memory
> > > >  	 *    regions.
> > > > @@ -166,7 +239,9 @@ static int init_tdx_module(void)
> > > >  	 *
> > > >  	 *  Return error before all steps are done.
> > > >  	 */
> > > > -	return -EINVAL;
> > > > +	ret = -EINVAL;
> > > > +out:
> > > > +	return ret;
> > > >  }
> > > 
> > > I'm going to be lazy and not look into the future.  But, you don't need
> > > the "out:" label here, yet.  It doesn'serve any purpose like this, so
> > > why introduce it here?
> > 
> > The 'out' label is here because of below code:
> > 
> > 	ret = tdx_get_sysinfo(...);
> > 	if (ret)
> > 		goto out;
> > 
> > If I don't have 'out' label here in this patch, do you mean something below?
> > 
> > 	ret = tdx_get_sysinfo(...);
> > 	if (ret)
> > 		return ret;
> > 
> > 	/*
> > 	 * TODO:
> > 	 * ...
> > 	 * Return error before all steps are done.
> > 	 */
> > 	return -EINVAL;
> 
> Yes, if you remove the 'out:' label like you've shown in your reply,
> it's actually _less_ code.  The labels are really only necessary when
> you have common work to "undo" something before returning from the
> function.  Here, you can just return.
> 

Thanks will do.

I think this applies to construct_tdmrs() too (patch 09 - 11).  I'll check that
part too based on your above idea.