linux-kernel - Re: [PATCH 2/5] x86/virt/tdx: Move TDMR metadata fields map table to local variable

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]

Message-ID: <6cbb56898011b8b0adbff216f1318dc5529b7d1f.camel@intel.com>
Date: Fri, 3 May 2024 00:29:01 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Huang, Kai"
	<kai.huang@...el.com>
CC: "Hansen, Dave" <dave.hansen@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "bp@...en8.de" <bp@...en8.de>, "x86@...nel.org"
	<x86@...nel.org>, "peterz@...radead.org" <peterz@...radead.org>,
	"hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com" <mingo@...hat.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "jgross@...e.com" <jgross@...e.com>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>
Subject: Re: [PATCH 2/5] x86/virt/tdx: Move TDMR metadata fields map table to
 local variable

On Fri, 2024-05-03 at 12:18 +1200, Huang, Kai wrote:
> 
> 
> On 3/05/2024 12:09 pm, Edgecombe, Rick P wrote:
> > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> > > The kernel reads all TDMR related global metadata fields based on a
> > > table which maps the metadata fields to the corresponding members of
> > > 'struct tdx_tdmr_sysinfo'.
> > > 
> > > Currently this table is a static variable.  But this table is only used
> > > by the function which reads these metadata fields and becomes useless
> > > after reading is done.
> > > 
> > > Change the table to function local variable.  This also saves the
> > > storage of the table from the kernel image.
> > 
> > It seems like a reasonable change, but I don't see how it helps the purpose
> > of
> > this series. It seems more like generic cleanup. Can you explain?
> 
> It doesn't help KVM from exporting API's perspective.
> 
> I just uses this series for some small improvement (that I believe) of 
> the current code too.
> 
> I can certainly drop this if you don't want it, but it's just a small 
> change and I don't see the benefit of sending it out separately.

The change makes sense to me by itself, but it needs to be called out as
unrelated cleanup. Otherwise it will be confusing to anyone reviewing this from
the perspective of something KVM TDX needs.

Don't have a super strong opinion. But given the choice, I would prefer it gets
separated, because to me it's a lower priority then the rest (which is high).