linux-kernel - Re: [PATCH 3/9] x86/virt/tdx: Support global metadata read for all element sizes

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: <3bf4682d487878c1a3701c64c7e2f1d04d7d1331.camel@intel.com>
Date: Wed, 19 Jun 2024 09:51:50 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "nik.borisov@...e.com" <nik.borisov@...e.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "Hansen, Dave"
	<dave.hansen@...el.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>,
	"Williams, Dan J" <dan.j.williams@...el.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "seanjc@...gle.com" <seanjc@...gle.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "binbin.wu@...ux.intel.com"
	<binbin.wu@...ux.intel.com>, "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH 3/9] x86/virt/tdx: Support global metadata read for all
 element sizes


> >   
> > -static int read_sys_metadata_field16(u64 field_id,
> > -				     int offset,
> > -				     void *stbuf)
> > +/*
> > + * Read one global metadata field and store the data to a location of a
> > + * given buffer specified by the offset and size (in bytes).
> > + */
> > +static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
> > +				  int bytes)
> 
> Actually I think opencoding read_sys_metadata_field in 
> stbuf_read_sysmd_field and leaving the function named as
> read_sys_metadata_field would be best. The new function is still very 
> short and linear.

I am not sure why it is better.  IMHO having a wrapper function of a
SEAMCALL leaf, read_sys_metadata_field() for TDH.SYS.RD in this case,
isn't a bad idea.  To me it is even better as it is clearer to me.

I can see you don't like the "stbuf" prefix, so we can consider to remove
it, but for now I'd wait for some more time to see whether other people
also have comments.

> 
> Another point - why do proliferate the offset calculations in the lower 
> layers of TDX. Simply pass in a buffer and a size, calculate the exact 
> location in the callers of the read functions.

The current upstream code takes the 'offset' as argument.  Here I just
extend it to take the size.  I don't feel I have strong justification to
do additional change.  Also to me between the two it's hard to say which
is definitely better than the other.